[FFmpeg-devel] [PATCH 1/2] avformat: always unref the packet after parsing
This fixes a memory leak when side-data is present. --- libavformat/utils.c | 9 - 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/libavformat/utils.c b/libavformat/utils.c index 7e4f54f..3f82659 100644 --- a/libavformat/utils.c +++ b/libavformat/utils.c @@ -1285,12 +1285,11 @@ static int parse_packet(AVFormatContext *s, AVPacket *pkt, int stream_index) compute_pkt_fields(s, st, st->parser, &out_pkt, next_dts, next_pts); -if ((ret = add_to_pktbuf(&s->internal->parse_queue, &out_pkt, - &s->internal->parse_queue_end, - 1))) { -av_packet_unref(&out_pkt); +ret = add_to_pktbuf(&s->internal->parse_queue, &out_pkt, +&s->internal->parse_queue_end, 1); +av_packet_unref(&out_pkt); +if (ret < 0) goto fail; -} } /* end of the stream => close and free the parser */ -- 2.6.2.windows.1 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 2/2] avformat: unref packet after storing it in internal packet queue
Fixes a memory leak when using genpts --- libavformat/utils.c | 1 + 1 file changed, 1 insertion(+) diff --git a/libavformat/utils.c b/libavformat/utils.c index 3f82659..5c4d452 100644 --- a/libavformat/utils.c +++ b/libavformat/utils.c @@ -1559,6 +1559,7 @@ int av_read_frame(AVFormatContext *s, AVPacket *pkt) ret = add_to_pktbuf(&s->internal->packet_buffer, pkt, &s->internal->packet_buffer_end, 1); +av_packet_unref(pkt); if (ret < 0) return ret; } -- 2.6.2.windows.1 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/2] avformat: always unref the packet after parsing
On Sun, 1 Nov 2015 11:21:26 +0100 Hendrik Leppkes wrote: > This fixes a memory leak when side-data is present. > --- > libavformat/utils.c | 9 - > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/libavformat/utils.c b/libavformat/utils.c > index 7e4f54f..3f82659 100644 > --- a/libavformat/utils.c > +++ b/libavformat/utils.c > @@ -1285,12 +1285,11 @@ static int parse_packet(AVFormatContext *s, AVPacket > *pkt, int stream_index) > > compute_pkt_fields(s, st, st->parser, &out_pkt, next_dts, next_pts); > > -if ((ret = add_to_pktbuf(&s->internal->parse_queue, &out_pkt, > - &s->internal->parse_queue_end, > - 1))) { > -av_packet_unref(&out_pkt); > +ret = add_to_pktbuf(&s->internal->parse_queue, &out_pkt, > +&s->internal->parse_queue_end, 1); > +av_packet_unref(&out_pkt); > +if (ret < 0) > goto fail; > -} > } > > /* end of the stream => close and free the parser */ I thought he semantics of add_to_pktbuf was to transfer packet ownership if the last parameter is 1? ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/2] avformat: always unref the packet after parsing
On Sun, Nov 1, 2015 at 12:57 PM, wm4 wrote: > On Sun, 1 Nov 2015 11:21:26 +0100 > Hendrik Leppkes wrote: > >> This fixes a memory leak when side-data is present. >> --- >> libavformat/utils.c | 9 - >> 1 file changed, 4 insertions(+), 5 deletions(-) >> >> diff --git a/libavformat/utils.c b/libavformat/utils.c >> index 7e4f54f..3f82659 100644 >> --- a/libavformat/utils.c >> +++ b/libavformat/utils.c >> @@ -1285,12 +1285,11 @@ static int parse_packet(AVFormatContext *s, AVPacket >> *pkt, int stream_index) >> >> compute_pkt_fields(s, st, st->parser, &out_pkt, next_dts, next_pts); >> >> -if ((ret = add_to_pktbuf(&s->internal->parse_queue, &out_pkt, >> - &s->internal->parse_queue_end, >> - 1))) { >> -av_packet_unref(&out_pkt); >> +ret = add_to_pktbuf(&s->internal->parse_queue, &out_pkt, >> +&s->internal->parse_queue_end, 1); >> +av_packet_unref(&out_pkt); >> +if (ret < 0) >> goto fail; >> -} >> } >> >> /* end of the stream => close and free the parser */ > > I thought he semantics of add_to_pktbuf was to transfer packet > ownership if the last parameter is 1? Actually 0 does that, 1 creates a new reference, including copying data into a refcounted buffer if needed, which is wanted here since the output buffer from the parser is not persistent otherwise. - Hendrik ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/2] avformat: always unref the packet after parsing
On Sun, 1 Nov 2015 13:03:50 +0100 Hendrik Leppkes wrote: > On Sun, Nov 1, 2015 at 12:57 PM, wm4 wrote: > > On Sun, 1 Nov 2015 11:21:26 +0100 > > Hendrik Leppkes wrote: > > > >> This fixes a memory leak when side-data is present. > >> --- > >> libavformat/utils.c | 9 - > >> 1 file changed, 4 insertions(+), 5 deletions(-) > >> > >> diff --git a/libavformat/utils.c b/libavformat/utils.c > >> index 7e4f54f..3f82659 100644 > >> --- a/libavformat/utils.c > >> +++ b/libavformat/utils.c > >> @@ -1285,12 +1285,11 @@ static int parse_packet(AVFormatContext *s, > >> AVPacket *pkt, int stream_index) > >> > >> compute_pkt_fields(s, st, st->parser, &out_pkt, next_dts, > >> next_pts); > >> > >> -if ((ret = add_to_pktbuf(&s->internal->parse_queue, &out_pkt, > >> - &s->internal->parse_queue_end, > >> - 1))) { > >> -av_packet_unref(&out_pkt); > >> +ret = add_to_pktbuf(&s->internal->parse_queue, &out_pkt, > >> +&s->internal->parse_queue_end, 1); > >> +av_packet_unref(&out_pkt); > >> +if (ret < 0) > >> goto fail; > >> -} > >> } > >> > >> /* end of the stream => close and free the parser */ > > > > I thought he semantics of add_to_pktbuf was to transfer packet > > ownership if the last parameter is 1? > > Actually 0 does that, 1 creates a new reference, including copying > data into a refcounted buffer if needed, which is wanted here since > the output buffer from the parser is not persistent otherwise. Both patches LGTM then. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] avformat/ipmovie: put video decoding_map_size into packet and use it in decoder
The size of decoding map can differ from one calculated internally, producing artifacts while decoding video. Signed-off-by: Paul B Mahol --- libavcodec/interplayvideo.c | 14 +- libavformat/ipmovie.c | 7 --- 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/libavcodec/interplayvideo.c b/libavcodec/interplayvideo.c index a29b5fe..1460741 100644 --- a/libavcodec/interplayvideo.c +++ b/libavcodec/interplayvideo.c @@ -38,6 +38,7 @@ #include #include +#include "libavutil/intreadwrite.h" #include "avcodec.h" #include "bytestream.h" #include "hpeldsp.h" @@ -949,7 +950,7 @@ static void ipvideo_decode_opcodes(IpvideoContext *s, AVFrame *frame) } } if (bytestream2_get_bytes_left(&s->stream_ptr) > 1) { -av_log(s->avctx, AV_LOG_ERROR, +av_log(s->avctx, AV_LOG_DEBUG, "decode finished with %d bytes left over\n", bytestream2_get_bytes_left(&s->stream_ptr)); } @@ -987,12 +988,15 @@ static int ipvideo_decode_frame(AVCodecContext *avctx, AVFrame *frame = data; int ret; +if (buf_size < 2) +return AVERROR_INVALIDDATA; + /* decoding map contains 4 bits of information per 8x8 block */ -s->decoding_map_size = avctx->width * avctx->height / (8 * 8 * 2); +s->decoding_map_size = AV_RL16(avpkt->data); /* compressed buffer needs to be large enough to at least hold an entire * decoding map */ -if (buf_size < s->decoding_map_size) +if (buf_size < s->decoding_map_size + 2) return buf_size; if (av_packet_get_side_data(avpkt, AV_PKT_DATA_PARAM_CHANGE, NULL)) { @@ -1000,8 +1004,8 @@ static int ipvideo_decode_frame(AVCodecContext *avctx, av_frame_unref(s->second_last_frame); } -s->decoding_map = buf; -bytestream2_init(&s->stream_ptr, buf + s->decoding_map_size, +s->decoding_map = buf + 2; +bytestream2_init(&s->stream_ptr, buf + 2 + s->decoding_map_size, buf_size - s->decoding_map_size); if ((ret = ff_get_buffer(avctx, frame, AV_GET_BUFFER_FLAG_REF)) < 0) diff --git a/libavformat/ipmovie.c b/libavformat/ipmovie.c index 99b193d..df9b8f0 100644 --- a/libavformat/ipmovie.c +++ b/libavformat/ipmovie.c @@ -156,7 +156,7 @@ static int load_ipmovie_packet(IPMVEContext *s, AVIOContext *pb, /* send both the decode map and the video data together */ -if (av_new_packet(pkt, s->decode_map_chunk_size + s->video_chunk_size)) +if (av_new_packet(pkt, 2 + s->decode_map_chunk_size + s->video_chunk_size)) return CHUNK_NOMEM; if (s->has_palette) { @@ -178,7 +178,8 @@ static int load_ipmovie_packet(IPMVEContext *s, AVIOContext *pb, avio_seek(pb, s->decode_map_chunk_offset, SEEK_SET); s->decode_map_chunk_offset = 0; -if (avio_read(pb, pkt->data, s->decode_map_chunk_size) != +AV_WL16(pkt->data, s->decode_map_chunk_size); +if (avio_read(pb, pkt->data + 2, s->decode_map_chunk_size) != s->decode_map_chunk_size) { av_packet_unref(pkt); return CHUNK_EOF; @@ -187,7 +188,7 @@ static int load_ipmovie_packet(IPMVEContext *s, AVIOContext *pb, avio_seek(pb, s->video_chunk_offset, SEEK_SET); s->video_chunk_offset = 0; -if (avio_read(pb, pkt->data + s->decode_map_chunk_size, +if (avio_read(pb, pkt->data + 2 + s->decode_map_chunk_size, s->video_chunk_size) != s->video_chunk_size) { av_packet_unref(pkt); return CHUNK_EOF; -- 1.9.1 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avformat/ipmovie: put video decoding_map_size into packet and use it in decoder
On Sun, 1 Nov 2015 17:07:07 +0100 Paul B Mahol wrote: > The size of decoding map can differ from one calculated > internally, producing artifacts while decoding video. > > Signed-off-by: Paul B Mahol > --- > libavcodec/interplayvideo.c | 14 +- > libavformat/ipmovie.c | 7 --- > 2 files changed, 13 insertions(+), 8 deletions(-) > > diff --git a/libavcodec/interplayvideo.c b/libavcodec/interplayvideo.c > index a29b5fe..1460741 100644 > --- a/libavcodec/interplayvideo.c > +++ b/libavcodec/interplayvideo.c > @@ -38,6 +38,7 @@ > #include > #include > > +#include "libavutil/intreadwrite.h" > #include "avcodec.h" > #include "bytestream.h" > #include "hpeldsp.h" > @@ -949,7 +950,7 @@ static void ipvideo_decode_opcodes(IpvideoContext *s, > AVFrame *frame) > } > } > if (bytestream2_get_bytes_left(&s->stream_ptr) > 1) { > -av_log(s->avctx, AV_LOG_ERROR, > +av_log(s->avctx, AV_LOG_DEBUG, > "decode finished with %d bytes left over\n", > bytestream2_get_bytes_left(&s->stream_ptr)); > } > @@ -987,12 +988,15 @@ static int ipvideo_decode_frame(AVCodecContext *avctx, > AVFrame *frame = data; > int ret; > > +if (buf_size < 2) > +return AVERROR_INVALIDDATA; > + > /* decoding map contains 4 bits of information per 8x8 block */ > -s->decoding_map_size = avctx->width * avctx->height / (8 * 8 * 2); > +s->decoding_map_size = AV_RL16(avpkt->data); > > /* compressed buffer needs to be large enough to at least hold an entire > * decoding map */ > -if (buf_size < s->decoding_map_size) > +if (buf_size < s->decoding_map_size + 2) > return buf_size; > > if (av_packet_get_side_data(avpkt, AV_PKT_DATA_PARAM_CHANGE, NULL)) { > @@ -1000,8 +1004,8 @@ static int ipvideo_decode_frame(AVCodecContext *avctx, > av_frame_unref(s->second_last_frame); > } > > -s->decoding_map = buf; > -bytestream2_init(&s->stream_ptr, buf + s->decoding_map_size, > +s->decoding_map = buf + 2; > +bytestream2_init(&s->stream_ptr, buf + 2 + s->decoding_map_size, > buf_size - s->decoding_map_size); > > if ((ret = ff_get_buffer(avctx, frame, AV_GET_BUFFER_FLAG_REF)) < 0) > diff --git a/libavformat/ipmovie.c b/libavformat/ipmovie.c > index 99b193d..df9b8f0 100644 > --- a/libavformat/ipmovie.c > +++ b/libavformat/ipmovie.c > @@ -156,7 +156,7 @@ static int load_ipmovie_packet(IPMVEContext *s, > AVIOContext *pb, > > /* send both the decode map and the video data together */ > > -if (av_new_packet(pkt, s->decode_map_chunk_size + > s->video_chunk_size)) > +if (av_new_packet(pkt, 2 + s->decode_map_chunk_size + > s->video_chunk_size)) > return CHUNK_NOMEM; > > if (s->has_palette) { > @@ -178,7 +178,8 @@ static int load_ipmovie_packet(IPMVEContext *s, > AVIOContext *pb, > avio_seek(pb, s->decode_map_chunk_offset, SEEK_SET); > s->decode_map_chunk_offset = 0; > > -if (avio_read(pb, pkt->data, s->decode_map_chunk_size) != > +AV_WL16(pkt->data, s->decode_map_chunk_size); > +if (avio_read(pb, pkt->data + 2, s->decode_map_chunk_size) != > s->decode_map_chunk_size) { > av_packet_unref(pkt); > return CHUNK_EOF; > @@ -187,7 +188,7 @@ static int load_ipmovie_packet(IPMVEContext *s, > AVIOContext *pb, > avio_seek(pb, s->video_chunk_offset, SEEK_SET); > s->video_chunk_offset = 0; > > -if (avio_read(pb, pkt->data + s->decode_map_chunk_size, > +if (avio_read(pb, pkt->data + 2 + s->decode_map_chunk_size, > s->video_chunk_size) != s->video_chunk_size) { > av_packet_unref(pkt); > return CHUNK_EOF; I can confirm this fixes it. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 0/3] transition strtod to av_strtod
Patch 1/3 was just a minor typo I noticed while working on av_strtod. Patch 2/3 offers improved flexibility in setting parameters, and it may be noted that with this, all of libavfilter uses av_strtod as opposed to strtod. Patch 3/3 accordingly documents it. Since all of libavfilter now uses av_strtod, it is placed in a common location - otherwise this remark will need to be applied to all filters, increasing the verbosity and maintainence burden for little gain. Patch for vf_frei0r untested, as I lack it on my system. Ganesh Ajjanagadde (3): avutil/eval: minor typo avfilter/vf_frei0r: use av_strtod instead of strtod for added flexibility doc/filters: move scattered remark on av_strtod to a common location doc/filters.texi| 10 ++ libavfilter/vf_frei0r.c | 3 ++- libavutil/eval.h| 2 +- 3 files changed, 9 insertions(+), 6 deletions(-) -- 2.6.2 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 1/3] avutil/eval: minor typo
Signed-off-by: Ganesh Ajjanagadde --- libavutil/eval.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libavutil/eval.h b/libavutil/eval.h index 6159b0f..dacd22b 100644 --- a/libavutil/eval.h +++ b/libavutil/eval.h @@ -102,7 +102,7 @@ void av_expr_free(AVExpr *e); * @param numstr a string representing a number, may contain one of * the International System number postfixes, for example 'K', 'M', * 'G'. If 'i' is appended after the postfix, powers of 2 are used - * instead of powers of 10. The 'B' postfix multiplies the value for + * instead of powers of 10. The 'B' postfix multiplies the value by * 8, and can be appended after another postfix or used alone. This * allows using for example 'KB', 'MiB', 'G' and 'B' as postfix. * @param tail if non-NULL puts here the pointer to the char next -- 2.6.2 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 2/3] avfilter/vf_frei0r: use av_strtod instead of strtod for added flexibility
This converts the usage of strtod to av_strtod in order to be more free in accepting double parameters. Signed-off-by: Ganesh Ajjanagadde --- libavfilter/vf_frei0r.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/libavfilter/vf_frei0r.c b/libavfilter/vf_frei0r.c index 0a98fea..9aa3edc 100644 --- a/libavfilter/vf_frei0r.c +++ b/libavfilter/vf_frei0r.c @@ -30,6 +30,7 @@ #include "config.h" #include "libavutil/avstring.h" #include "libavutil/common.h" +#include "libavutil/eval.h" #include "libavutil/imgutils.h" #include "libavutil/internal.h" #include "libavutil/mathematics.h" @@ -104,7 +105,7 @@ static int set_param(AVFilterContext *ctx, f0r_param_info_t info, int index, cha break; case F0R_PARAM_DOUBLE: -val.d = strtod(param, &tail); +val.d = av_strtod(param, &tail); if (*tail || val.d == HUGE_VAL) goto fail; break; -- 2.6.2 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 3/3] doc/filters: move scattered remark on av_strtod to a common location
All filters now use av_strtod for accepting floating point parameters. There was an isolated remark on this, but the point applies generally now. This moves the comment and suitably elaborates on it for additional clarity. A link to the code for av_strtod is also provided for the user's benefit. Signed-off-by: Ganesh Ajjanagadde --- doc/filters.texi | 10 ++ 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/doc/filters.texi b/doc/filters.texi index 15ea77a..0a20531 100644 --- a/doc/filters.texi +++ b/doc/filters.texi @@ -45,7 +45,11 @@ lower half the output generated by the @var{crop,vflip} filterchain. Some filters take in input a list of parameters: they are specified after the filter name and an equal sign, and are separated from each other -by a colon. +by a colon. Floating point parameters may all accept an optional IS postfix, +e.g 'K', 'M', 'G' for Kilo, Mega, Giga. If 'i' is appended after the postfix, +powers of 2 are used instead of powers of 10. The 'B' postfix multiplies the +value by 8, and can be appended after another postfix or used alone. See also +@code{av_strtod}. There exist so-called @var{source filters} that do not have an audio/video input, and @var{sink filters} that will not have audio/video @@ -14371,9 +14375,7 @@ format is guessed from @var{movie_name} or by probing. @item seek_point, sp Specifies the seek point in seconds. The frames will be output -starting from this seek point. The parameter is evaluated with -@code{av_strtod}, so the numerical value may be suffixed by an IS -postfix. The default value is "0". +starting from this seek point. The default value is "0". @item streams, s Specifies the streams to read. Several streams can be specified, -- 2.6.2 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 1/2] pixblockdsp: x86: Condense diff_pixels_* to a shared macro
--- libavcodec/x86/pixblockdsp.asm | 66 -- 1 file changed, 31 insertions(+), 35 deletions(-) diff --git a/libavcodec/x86/pixblockdsp.asm b/libavcodec/x86/pixblockdsp.asm index 7c5377b..a7d9816 100644 --- a/libavcodec/x86/pixblockdsp.asm +++ b/libavcodec/x86/pixblockdsp.asm @@ -80,54 +80,50 @@ cglobal get_pixels, 3, 4, 5 mova [r0+0x70], m3 RET -INIT_MMX mmx ; void ff_diff_pixels_mmx(int16_t *block, const uint8_t *s1, const uint8_t *s2, ; int stride); -cglobal diff_pixels, 4,5 -movsxdifnidn r3, r3d -pxor m7, m7 -add r0, 128 -mov r4, -128 -.loop: -mova m0, [r1] -mova m2, [r2] -mova m1, m0 -mova m3, m2 -punpcklbwm0, m7 -punpckhbwm1, m7 -punpcklbwm2, m7 -punpckhbwm3, m7 -psubwm0, m2 -psubwm1, m3 -mova [r0+r4+0], m0 -mova [r0+r4+8], m1 -add r1, r3 -add r2, r3 -add r4, 16 -jne .loop -REP_RET - -INIT_XMM sse2 -cglobal diff_pixels, 4, 5, 5 +%macro DIFF_PIXELS 0 +cglobal diff_pixels, 4,5,5 movsxdifnidn r3, r3d pxor m4, m4 add r0, 128 mov r4, -128 .loop: -movh m0, [r1] -movh m2, [r2] -movh m1, [r1+r3] -movh m3, [r2+r3] +movq m0, [r1] +movq m2, [r2] +%if mmsize == 8 +movq m1, m0 +movq m3, m2 +punpcklbwm0, m4 +punpckhbwm1, m4 +punpcklbwm2, m4 +punpckhbwm3, m4 +%else +movq m1, [r1+r3] +movq m3, [r2+r3] punpcklbwm0, m4 punpcklbwm1, m4 punpcklbwm2, m4 punpcklbwm3, m4 +%endif psubwm0, m2 psubwm1, m3 -mova [r0+r4+0 ], m0 -mova [r0+r4+16], m1 +mova [r0+r4+0], m0 +mova [r0+r4+mmsize], m1 +%if mmsize == 8 +add r1, r3 +add r2, r3 +%else lea r1, [r1+r3*2] lea r2, [r2+r3*2] -add r4, 32 +%endif +add r4, 2 * mmsize jne .loop -RET +REP_RET +%endmacro + +INIT_MMX mmx +DIFF_PIXELS + +INIT_XMM sse2 +DIFF_PIXELS -- 2.1.4 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 2/2] Add pixblockdsp checkasm tests
--- tests/checkasm/Makefile | 1 + tests/checkasm/checkasm.c| 3 ++ tests/checkasm/checkasm.h| 1 + tests/checkasm/pixblockdsp.c | 107 +++ 4 files changed, 112 insertions(+) create mode 100644 tests/checkasm/pixblockdsp.c diff --git a/tests/checkasm/Makefile b/tests/checkasm/Makefile index c29ceef..1c0290b 100644 --- a/tests/checkasm/Makefile +++ b/tests/checkasm/Makefile @@ -5,6 +5,7 @@ AVCODECOBJS-$(CONFIG_FLACDSP) += flacdsp.o AVCODECOBJS-$(CONFIG_H264PRED) += h264pred.o AVCODECOBJS-$(CONFIG_H264QPEL) += h264qpel.o AVCODECOBJS-$(CONFIG_JPEG2000_DECODER) += jpeg2000dsp.o +AVCODECOBJS-$(CONFIG_PIXBLOCKDSP) += pixblockdsp.o AVCODECOBJS-$(CONFIG_V210_ENCODER) += v210enc.o AVCODECOBJS-$(CONFIG_VP9_DECODER) += vp9dsp.o diff --git a/tests/checkasm/checkasm.c b/tests/checkasm/checkasm.c index e875120..3548bb3 100644 --- a/tests/checkasm/checkasm.c +++ b/tests/checkasm/checkasm.c @@ -76,6 +76,9 @@ static const struct { #if CONFIG_JPEG2000_DECODER { "jpeg2000dsp", checkasm_check_jpeg2000dsp }, #endif +#if CONFIG_PIXBLOCKDSP +{ "pixblockdsp", checkasm_check_pixblockdsp }, +#endif #if CONFIG_V210_ENCODER { "v210enc", checkasm_check_v210enc }, #endif diff --git a/tests/checkasm/checkasm.h b/tests/checkasm/checkasm.h index 1775a25..73b7e42 100644 --- a/tests/checkasm/checkasm.h +++ b/tests/checkasm/checkasm.h @@ -35,6 +35,7 @@ void checkasm_check_flacdsp(void); void checkasm_check_h264pred(void); void checkasm_check_h264qpel(void); void checkasm_check_jpeg2000dsp(void); +void checkasm_check_pixblockdsp(void); void checkasm_check_v210enc(void); void checkasm_check_vp9dsp(void); diff --git a/tests/checkasm/pixblockdsp.c b/tests/checkasm/pixblockdsp.c new file mode 100644 index 000..d59d162 --- /dev/null +++ b/tests/checkasm/pixblockdsp.c @@ -0,0 +1,107 @@ +/* + * Copyright (c) 2015 Tiancheng "Timothy" Gu + * + * This file is part of FFmpeg. + * + * FFmpeg is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * FFmpeg is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License along + * with FFmpeg; if not, write to the Free Software Foundation, Inc., + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + */ + +#include +#include "checkasm.h" +#include "libavcodec/pixblockdsp.h" +#include "libavutil/common.h" +#include "libavutil/internal.h" +#include "libavutil/intreadwrite.h" + +#define BUF_SIZE 512 +#define TOTAL_BUF_SIZE FFALIGN(BUF_SIZE * 2 + BUF_SIZE / 64, 4) + +#define randomize_buffers() \ +do {\ +int i; \ +for (i = 0; i < TOTAL_BUF_SIZE; i += 4) { \ +uint32_t r = rnd(); \ +AV_WN32A(src10 + i, r); \ +AV_WN32A(src11 + i, r); \ +r = rnd(); \ +AV_WN32A(src20 + i, r); \ +AV_WN32A(src21 + i, r); \ +r = rnd(); \ +AV_WN32A(dst0_ + i, r); \ +AV_WN32A(dst1_ + i, r); \ +} \ +} while (0) + +#define check_get_pixels(type) \ +do { \ +int i; \ +declare_func(void, int16_t *block, const uint8_t *pixels, ptrdiff_t line_size);\ + \ +for (i = 0; i < BUF_SIZE / 64; i++) { \ +int src_offset = i * 64 * sizeof(type) + i; /* Test various alignments */ \ +int dst_offset = i * 64; /* dst must be aligned */ \ +randomize_buffers(); \ +call_ref(dst0 + dst_offset, src10 + src_offset, 8); \ +call_new(dst1 + dst_offset, src11 + src_offset, 8); \ +if (memcmp(src10, src11, TOTAL_BUF_SIZE)|| memcmp(dst0, dst1, TOTAL_BUF_SIZE)) \ +fail(); \ +bench_new(dst1 + dst_offset, src11 + src_offset, 8); \ +
[FFmpeg-devel] [PATCH 0/2] add FFDIFFSIGN for comparators
This patch series draws upon remarks of Mark and Ronald regarding the lack of safety of the common idiom return a - b for qsort comparators. I made an observation that the interesting (x > y) - (x < y) idiom works well: it not only avoids branches commonly not optimized by compilers when the ternary operator method is used, but is also safe with respect to overflow. See e.g the link: https://stackoverflow.com/questions/10996418/efficient-integer-compare-function/10997428#10997428 On a suggestion from Nicolas, slightly modified by me, 1/2 adds FFDIFFSIGN macro to avutil/common, and documents it and its rationale. Note that is quite generic, and in particular works for all of the standard built in integral and numerical types. NaN's will compare equal with everything, which seems reasonable and matches C++'s std::sort AFAIK. 2/2 utilizes FFDIFFSIGN in comparators across the codebase. A few were left out due to reasons mentioned in a comment for 2/2. Ganesh Ajjanagadde (2): avutil/common: add FFDIFFSIGN macro all: use FFDIFFSIGN to resolve possible undefined behavior in comparators cmdutils.c | 2 +- cmdutils_opencl.c | 2 +- ffmpeg.c| 3 +-- libavcodec/aacsbr_template.c| 2 +- libavcodec/motion_est.c | 2 +- libavfilter/f_sendcmd.c | 8 +++- libavfilter/vf_deshake.c| 3 +-- libavfilter/vf_palettegen.c | 2 +- libavfilter/vf_removegrain.c| 5 + libavformat/subtitles.c | 9 +++-- libavutil/common.h | 11 +++ libswresample/swresample-test.c | 2 +- 12 files changed, 26 insertions(+), 25 deletions(-) -- 2.6.2 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 1/2] avutil/common: add FFDIFFSIGN macro
This is of use for defining comparator callbacks. Common approaches like return x-y are not safe due to the risks of overflow. Furthermore, the (x > y) - (x < y) trick is optimized to branchless code. This also documents this macro accordingly. Signed-off-by: Ganesh Ajjanagadde --- libavutil/common.h | 11 +++ 1 file changed, 11 insertions(+) diff --git a/libavutil/common.h b/libavutil/common.h index 6594f7d..6f0f582 100644 --- a/libavutil/common.h +++ b/libavutil/common.h @@ -76,6 +76,17 @@ */ #define FFNABS(a) ((a) <= 0 ? (a) : (-(a))) +/** + * Comparator. + * For two numerical expressions x and y, gives 1 if x > y, -1 if x < y, and 0 + * if x == y. This is useful for instance in a qsort comparator callback. + * Furthermore, compilers are able to optimize this to branchless code, and + * there is no risk of overflow with signed types. + * As with many macros, this evaluates its argument multiple times, it thus + * must not have a side-effect. + */ +#define FFDIFFSIGN(x,y) (((x)>(y)) - ((x)<(y))) + #define FFMAX(a,b) ((a) > (b) ? (a) : (b)) #define FFMAX3(a,b,c) FFMAX(FFMAX(a,b),c) #define FFMIN(a,b) ((a) > (b) ? (b) : (a)) -- 2.6.2 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 2/2] all: use FFDIFFSIGN to resolve possible undefined behavior in comparators
FFDIFFSIGN was created explicitly for this purpose, since the common return a - b idiom is unsafe regarding overflow on signed integers. It optimizes to branchless code on common compilers. FFDIFFSIGN also has the subjective benefit of being easier to read due to lack of ternary operators. Tested with FATE. Things not covered by this are unsigned integers, for which overflows are well defined, and also places where overflow is clearly impossible, e.g an instance where the a - b was being done on 24 bit values. I can convert some of these to FFDIFFSIGN if people find it a readability improvement. Signed-off-by: Ganesh Ajjanagadde --- cmdutils.c | 2 +- cmdutils_opencl.c | 2 +- ffmpeg.c| 3 +-- libavcodec/aacsbr_template.c| 2 +- libavcodec/motion_est.c | 2 +- libavfilter/f_sendcmd.c | 8 +++- libavfilter/vf_deshake.c| 3 +-- libavfilter/vf_palettegen.c | 2 +- libavfilter/vf_removegrain.c| 5 + libavformat/subtitles.c | 9 +++-- libswresample/swresample-test.c | 2 +- 11 files changed, 15 insertions(+), 25 deletions(-) diff --git a/cmdutils.c b/cmdutils.c index e3e9891..41daa95 100644 --- a/cmdutils.c +++ b/cmdutils.c @@ -1421,7 +1421,7 @@ static int compare_codec_desc(const void *a, const void *b) const AVCodecDescriptor * const *da = a; const AVCodecDescriptor * const *db = b; -return (*da)->type != (*db)->type ? (*da)->type - (*db)->type : +return (*da)->type != (*db)->type ? FFDIFFSIGN((*da)->type, (*db)->type) : strcmp((*da)->name, (*db)->name); } diff --git a/cmdutils_opencl.c b/cmdutils_opencl.c index d9095b6..5621ebc 100644 --- a/cmdutils_opencl.c +++ b/cmdutils_opencl.c @@ -206,7 +206,7 @@ end: static int compare_ocl_device_desc(const void *a, const void *b) { -return ((const OpenCLDeviceBenchmark*)a)->runtime - ((const OpenCLDeviceBenchmark*)b)->runtime; +return FFDIFFSIGN((const OpenCLDeviceBenchmark*)a->runtime , (const OpenCLDeviceBenchmark*)b->runtime); } int opt_opencl_bench(void *optctx, const char *opt, const char *arg) diff --git a/ffmpeg.c b/ffmpeg.c index f8b071a..d3b8c4d 100644 --- a/ffmpeg.c +++ b/ffmpeg.c @@ -2578,8 +2578,7 @@ static InputStream *get_input_stream(OutputStream *ost) static int compare_int64(const void *a, const void *b) { -int64_t va = *(int64_t *)a, vb = *(int64_t *)b; -return va < vb ? -1 : va > vb ? +1 : 0; +return FFDIFFSIGN(*(const int64_t *)a, *(const int64_t *)b); } static int init_output_stream(OutputStream *ost, char *error, int error_len) diff --git a/libavcodec/aacsbr_template.c b/libavcodec/aacsbr_template.c index d31b71e..1e6f149 100644 --- a/libavcodec/aacsbr_template.c +++ b/libavcodec/aacsbr_template.c @@ -104,7 +104,7 @@ av_cold void AAC_RENAME(ff_aac_sbr_ctx_close)(SpectralBandReplication *sbr) static int qsort_comparison_function_int16(const void *a, const void *b) { -return *(const int16_t *)a - *(const int16_t *)b; +return FFDIFFSIGN(*(const int16_t *)a , *(const int16_t *)b); } static inline int in_table_int16(const int16_t *table, int last_el, int16_t needle) diff --git a/libavcodec/motion_est.c b/libavcodec/motion_est.c index 9f71568..8560819 100644 --- a/libavcodec/motion_est.c +++ b/libavcodec/motion_est.c @@ -73,7 +73,7 @@ static int minima_cmp(const void *a, const void *b){ const Minima *da = (const Minima *) a; const Minima *db = (const Minima *) b; -return da->height - db->height; +return FFDIFFSIGN(da->height , db->height); } #define FLAG_QPEL 1 //must be 1 diff --git a/libavfilter/f_sendcmd.c b/libavfilter/f_sendcmd.c index 37aedc5..f06773a 100644 --- a/libavfilter/f_sendcmd.c +++ b/libavfilter/f_sendcmd.c @@ -364,11 +364,9 @@ static int cmp_intervals(const void *a, const void *b) { const Interval *i1 = a; const Interval *i2 = b; -int64_t ts_diff = i1->start_ts - i2->start_ts; -int ret; - -ret = ts_diff > 0 ? 1 : ts_diff < 0 ? -1 : 0; -return ret == 0 ? i1->index - i2->index : ret; +if (i1->start_ts == i2->start_ts) +return FFDIFFSIGN(i1->index, i2->index); +return FFDIFFSIGN(i1->start_ts, i2->start_ts); } static av_cold int init(AVFilterContext *ctx) diff --git a/libavfilter/vf_deshake.c b/libavfilter/vf_deshake.c index e32436d..79fcc20 100644 --- a/libavfilter/vf_deshake.c +++ b/libavfilter/vf_deshake.c @@ -94,8 +94,7 @@ AVFILTER_DEFINE_CLASS(deshake); static int cmp(const void *a, const void *b) { -const double va = *(const double *)a, vb = *(const double *)b; -return va < vb ? -1 : ( va > vb ? 1 : 0 ); +return FFDIFFSIGN(*(const double *)a, *(const double *)b); } /** diff --git a/libavfilter/vf_palettegen.c b/libavfilter/vf_palettegen.c index df57c10..fccc5ca 100644 --- a/libavfilter/vf_palettegen.c +++ b/libavfilter/vf_palettegen.c @@ -130,7 +130,7 @@
[FFmpeg-devel] [PATCH] hlsenc: Only write PAT/PMT once per segment
This saves a lot of muxing overhead, especially on lower bitrate segments. Signed-off-by: Derek Buitenhuis --- libavformat/hlsenc.c | 10 +- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c index 8daf53f..d7884e5 100644 --- a/libavformat/hlsenc.c +++ b/libavformat/hlsenc.c @@ -560,8 +560,16 @@ static int hls_start(AVFormatContext *s) } av_dict_free(&options); -if (oc->oformat->priv_class && oc->priv_data) +/* We only require one PAT/PMT per segment. */ +if (oc->oformat->priv_class && oc->priv_data) { +char period[21]; + +snprintf(period, sizeof(period), "%d", (INT_MAX / 2) - 1); + av_opt_set(oc->priv_data, "mpegts_flags", "resend_headers", 0); +av_opt_set(oc->priv_data, "sdt_period", period, 0); +av_opt_set(oc->priv_data, "pat_period", period, 0); +} if (c->vtt_basename) avformat_write_header(vtt_oc,NULL); -- 2.6.2 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 1/2] ffserver: Do not add or rescale AV_NOPTS_VALUE from the demuxer
From: Michael Niedermayer Signed-off-by: Michael Niedermayer --- ffserver.c |8 +--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/ffserver.c b/ffserver.c index 526cbfc..8ddc210 100644 --- a/ffserver.c +++ b/ffserver.c @@ -2272,7 +2272,7 @@ static int http_prepare_data(HTTPContext *c) } else { int source_index = pkt.stream_index; /* update first pts if needed */ -if (c->first_pts == AV_NOPTS_VALUE) { +if (c->first_pts == AV_NOPTS_VALUE && pkt.dts != AV_NOPTS_VALUE) { c->first_pts = av_rescale_q(pkt.dts, c->fmt_in->streams[pkt.stream_index]->time_base, AV_TIME_BASE_Q); c->start_time = cur_time; } @@ -2311,8 +2311,10 @@ static int http_prepare_data(HTTPContext *c) * XXX: need more abstract handling */ if (c->is_packetized) { /* compute send time and duration */ -c->cur_pts = av_rescale_q(pkt.dts, ist->time_base, AV_TIME_BASE_Q); -c->cur_pts -= c->first_pts; +if (pkt.dts != AV_NOPTS_VALUE) { +c->cur_pts = av_rescale_q(pkt.dts, ist->time_base, AV_TIME_BASE_Q); +c->cur_pts -= c->first_pts; +} c->cur_frame_duration = av_rescale_q(pkt.duration, ist->time_base, AV_TIME_BASE_Q); /* find RTP context */ c->packet_stream_index = pkt.stream_index; -- 1.7.9.5 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 2/2] ffserver: Clear avio context after closing it
From: Michael Niedermayer Fixes: ==13287== Invalid read of size 4 ==13287==at 0x45161A: flush_buffer (aviobuf.c:143) ==13287==by 0x451971: avio_flush (aviobuf.c:200) ==13287==by 0x512CCF: av_write_trailer (mux.c:1016) ==13287==by 0x41A5E0: close_connection (ffserver.c:853) ==13287==by 0x421EDC: rtsp_cmd_interrupt (ffserver.c:3245) ==13287==by 0x420B9C: rtsp_parse_request (ffserver.c:2854) ==13287==by 0x41A9C2: handle_connection (ffserver.c:930) ==13287==by 0x41A04B: http_server (ffserver.c:700) ==13287==by 0x423A60: main (ffserver.c:3897) ==13287== Address 0xb6cd258 is 88 bytes inside a block of size 192 free'd ==13287==at 0x4C2B5D9: free (vg_replace_malloc.c:446) ==13287==by 0x1004DAC: av_free (mem.c:239) ==13287==by 0x454835: avio_close_dyn_buf (aviobuf.c:1170) ==13287==by 0x41F385: http_prepare_data (ffserver.c:2368) ==13287==by 0x41F59B: http_send_data (ffserver.c:2416) ==13287==by 0x41ABE2: handle_connection (ffserver.c:986) ==13287==by 0x41A04B: http_server (ffserver.c:700) ==13287==by 0x423A60: main (ffserver.c:3897) Signed-off-by: Michael Niedermayer --- ffserver.c |1 + 1 file changed, 1 insertion(+) diff --git a/ffserver.c b/ffserver.c index 8ddc210..3bf6c07 100644 --- a/ffserver.c +++ b/ffserver.c @@ -2366,6 +2366,7 @@ static int http_prepare_data(HTTPContext *c) av_freep(&c->pb_buffer); len = avio_close_dyn_buf(ctx->pb, &c->pb_buffer); +ctx->pb = NULL; c->cur_frame_bytes = len; c->buffer_ptr = c->pb_buffer; c->buffer_end = c->pb_buffer + len; -- 1.7.9.5 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 3/4] avutil/eval: check av_expr_parse_and_eval error code in the test build
On Sat, Oct 31, 2015 at 10:47:56AM -0400, Ganesh Ajjanagadde wrote: > This returns the error code from main in the test, in this case ENOMEM. > This should not matter, and will ensure that no warnings are triggered > when av_warn_unused_result is added to avutil/eval.h. > > Signed-off-by: Ganesh Ajjanagadde > --- > libavutil/eval.c | 22 +- > 1 file changed, 13 insertions(+), 9 deletions(-) breaks fate TESTeval --- ./tests/ref/fate/eval 2015-10-30 03:22:52.209691909 +0100 +++ tests/data/fate/eval2015-11-01 18:48:37.730502524 +0100 @@ -1,281 +1 @@ Evaluating '' -'' -> nan - -Evaluating '1;2' -'1;2' -> 2.00 - -Evaluating '-20' -'-20' -> -20.00 - -Evaluating '-PI' -'-PI' -> -3.141593 - [...] -12.70 == 12.7 -0.931323 == 0.931322575 Test eval failed. Look at tests/data/fate/eval.err for details. make: *** [fate-eval] Error 234 make: *** Waiting for unfinished jobs cat tests/data/fate/eval.err [Eval @ 0x7ffe40285770] Undefined constant or missing '(' in '' [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Dictatorship naturally arises out of democracy, and the most aggravated form of tyranny and slavery out of the most extreme liberty. -- Plato signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 0/2] first steps to resolving float to int undefined behavior
Floating point to integer conversion is well defined when the float lies within the representation bounds of the integer after discarding the fractional part. However, in other cases, unfortunately the standard leaves it undefined. In particular, assuming that it saturates in a sane way is a dangerous assumption. In light of recent events, I would not have bothered if this was a merely theoretical issue, and that common environments saturate correctly. Sadly, x86 (for example) converts casts to a cvttsd2si instruction which saturates numbers > INT64_MAX to INT64_MIN. This is mathematically completely bogus. http://blog.frama-c.com/index.php?post/2013/10/09/Overflow-float-integer gives a nice overview of the issue. 1/2 adds an av_clipd64 API for this purpose to clip a float to an integral range. Obviously it will be slower than a cvttsd2si, but there is no option if one wants safe and well-defined behavior. Of course, if one knows a priori that a float lives in the integral type's range, then there is no issue. Safe speedups are entirely possible, but API should be finalized first IMHO. Most common anticipated usages are clipping to [INT64_MIN, INT64_MAX] or [INT_MIN, INT_MAX]. I have given some thought as to whether a separate av_clipd32 API (for example) is necessary. It seems to me to not be the case, since an IEEE-754 double is guaranteed to represent exactly integers up to ~ 2^53. Similarly, av_clipf64 and the like also seem unnecessary, since a double is guaranteed to represent all the values a float does. Such an API may be useful for performance though; I do not know how/what float to double conversions entail and at the moment ignore such complications. 1/2 also accordingly documents av_clipd64. 2/2 gives a simple illustration of it in action; something I observed while working on av_strtod. There are many more such things, but I would like to get the API finalized (i.e 1/2 in) before moving forward. I do not know the procedure or intricacies of version bumps, and request help for it. It also remains to be seen how much of the codebase is affected by this issue. The ffserver one here, although undefined behavior, does not seem to be an actual security vulnerability at least on x86: the negative value will be rejected from the config in subsequent code. This is also an issue that needs some thought: some of the future work may need to be backported, but a version bump may also be needed for the API addition. Ganesh Ajjanagadde (2): avutil/common: add av_clipd64 ffserver_config: replace strtod by av_strtod and correct undefined behavior ffserver_config.c | 21 + libavutil/common.h | 31 +++ 2 files changed, 36 insertions(+), 16 deletions(-) -- 2.6.2 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 1/2] avutil/common: add av_clipd64
The rationale for this function is reflected in the documentation for it, and is copied here: Clip a double value into the long long amin-amax range. This function is needed because conversion of floating point to integers when it does not fit in the integer's representation does not necessarily saturate correctly (usually converted to a cvttsd2si on x86) which saturates numbers > INT64_MAX to INT64_MIN. The standard marks such conversions as undefined behavior, allowing this sort of mathematically bogus conversions. This provides a safe alternative that is slower obviously but assures safety and better mathematical behavior. @param a value to clip @param amin minimum value of the clip range @param amax maximum value of the clip range @return clipped value Note that a priori if one can guarantee from the calling side that the double is in range, it is safe to simply do an explicit/implicit cast, and that will be far faster. However, otherwise, this function should be used. -- As a general remark, Clang/GCC has a -Wfloat-conversion so that at least implicit conversions can be found. This helped me in auditing the codebase. In order to reduce noise while testing, an explicit cast on the return was placed. I can remove it if people prefer, though I like the cast as it makes the intent of possible narrowing explicit. Signed-off-by: Ganesh Ajjanagadde --- libavutil/common.h | 31 +++ 1 file changed, 31 insertions(+) diff --git a/libavutil/common.h b/libavutil/common.h index 6f0f582..e778dd2 100644 --- a/libavutil/common.h +++ b/libavutil/common.h @@ -298,6 +298,34 @@ static av_always_inline av_const double av_clipd_c(double a, double amin, double else return a; } +/** + * Clip a double value into the long long amin-amax range. + * This function is needed because conversion of floating point to integers when + * it does not fit in the integer's representation does not necessarily saturate + * correctly (usually converted to a cvttsd2si on x86) which saturates numbers + * > INT64_MAX to INT64_MIN. The standard marks such conversions as undefined + * behavior, allowing this sort of mathematically bogus conversions. This provides + * a safe alternative that is slower obviously but assures safety and better + * mathematical behavior. + * @param a value to clip + * @param amin minimum value of the clip range + * @param amax maximum value of the clip range + * @return clipped value + */ +static av_always_inline av_const int64_t av_clipd64_c(double a, int64_t amin, int64_t amax) +{ +#if defined(HAVE_AV_CONFIG_H) && defined(ASSERT_LEVEL) && ASSERT_LEVEL >= 2 +if (amin > amax) abort(); +#endif +// INT64_MAX+1,INT64_MIN are exactly representable as IEEE doubles +if (a >= 9223372036854775808.0) +return INT64_MAX; +if (a <= INT64_MIN) +return INT64_MIN; +// Finally safe to call av_clipd_c +return (int64_t)av_clipd_c(a, amin, amax); +} + /** Compute ceil(log2(x)). * @param x value used to compute ceil(log2(x)) * @return computed ceiling of log2(x) @@ -511,6 +539,9 @@ static av_always_inline av_const int av_popcount64_c(uint64_t x) #ifndef av_clipd # define av_clipd av_clipd_c #endif +#ifndef av_clipd64 +# define av_clipd64 av_clipd64_c +#endif #ifndef av_popcount # define av_popcount av_popcount_c #endif -- 2.6.2 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 2/2] ffserver_config: replace strtod by av_strtod and correct undefined behavior
This uses av_strtod for added flexibility, and av_clipd64 for ensuring that no undefined behavior gets invoked. Signed-off-by: Ganesh Ajjanagadde --- ffserver_config.c | 21 + 1 file changed, 5 insertions(+), 16 deletions(-) diff --git a/ffserver_config.c b/ffserver_config.c index 9fc1f00..aca617b 100644 --- a/ffserver_config.c +++ b/ffserver_config.c @@ -19,6 +19,7 @@ */ #include +#include "libavutil/eval.h" #include "libavutil/opt.h" #include "libavutil/parseutils.h" #include "libavutil/avstring.h" @@ -757,7 +758,7 @@ static int ffserver_parse_config_feed(FFServerConfig *config, const char *cmd, } else { WARNING("Truncate N syntax in configuration file is deprecated. " "Use Truncate alone with no arguments.\n"); -feed->truncate = strtod(arg, NULL); +feed->truncate = av_clipd64(av_strtod(arg, NULL), INT_MIN, INT_MAX); } } else if (!av_strcasecmp(cmd, "FileMaxSize")) { char *p1; @@ -765,22 +766,10 @@ static int ffserver_parse_config_feed(FFServerConfig *config, const char *cmd, ffserver_get_arg(arg, sizeof(arg), p); p1 = arg; -fsize = strtod(p1, &p1); -switch(av_toupper(*p1)) { -case 'K': -fsize *= 1024; -break; -case 'M': -fsize *= 1024 * 1024; -break; -case 'G': -fsize *= 1024 * 1024 * 1024; -break; -default: +fsize = av_strtod(p1, &p1); +if (!fsize || fabs(fsize) == HUGE_VAL) ERROR("Invalid file size: '%s'\n", arg); -break; -} -feed->feed_max_size = (int64_t)fsize; +feed->feed_max_size = av_clipd64(fsize, INT64_MIN, INT64_MAX); if (feed->feed_max_size < FFM_PACKET_SIZE*4) { ERROR("Feed max file size is too small. Must be at least %d.\n", FFM_PACKET_SIZE*4); -- 2.6.2 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 3/4] avutil/eval: check av_expr_parse_and_eval error code in the test build
On Sun, Nov 1, 2015 at 12:52 PM, Michael Niedermayer wrote: > On Sat, Oct 31, 2015 at 10:47:56AM -0400, Ganesh Ajjanagadde wrote: >> This returns the error code from main in the test, in this case ENOMEM. >> This should not matter, and will ensure that no warnings are triggered >> when av_warn_unused_result is added to avutil/eval.h. >> >> Signed-off-by: Ganesh Ajjanagadde >> --- >> libavutil/eval.c | 22 +- >> 1 file changed, 13 insertions(+), 9 deletions(-) > > breaks fate > > TESTeval > --- ./tests/ref/fate/eval 2015-10-30 03:22:52.209691909 +0100 > +++ tests/data/fate/eval2015-11-01 18:48:37.730502524 +0100 > @@ -1,281 +1 @@ > Evaluating '' > -'' -> nan > - > -Evaluating '1;2' > -'1;2' -> 2.00 > - > -Evaluating '-20' > -'-20' -> -20.00 > - > -Evaluating '-PI' > -'-PI' -> -3.141593 > - > [...] > -12.70 == 12.7 > -0.931323 == 0.931322575 > Test eval failed. Look at tests/data/fate/eval.err for details. > make: *** [fate-eval] Error 234 > make: *** Waiting for unfinished jobs > > cat tests/data/fate/eval.err > [Eval @ 0x7ffe40285770] Undefined constant or missing '(' in '' So I know why this occurs: it is the first condition with the nan stuff, which returns an error code < 0. There are various methods I can think of: 1. Let the fate build be slightly noisy, with one -Warn-unused-result for this. 2. Simply do a ret = av_expr_parse_and_eval(...) for the first one. This kills the warning, and is quite readable. 3. in addition to the above, do a check for ENOMEM. I find this quite silly, since it is a test build. Let me know what you prefer, and sorry for not being thorough with this. > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > Dictatorship naturally arises out of democracy, and the most aggravated > form of tyranny and slavery out of the most extreme liberty. -- Plato > > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] boxblur: Templatize blur{8,16}
--- libavfilter/vf_boxblur.c | 110 +++ 1 file changed, 44 insertions(+), 66 deletions(-) diff --git a/libavfilter/vf_boxblur.c b/libavfilter/vf_boxblur.c index ef01cf9..6934076 100644 --- a/libavfilter/vf_boxblur.c +++ b/libavfilter/vf_boxblur.c @@ -204,75 +204,53 @@ static int config_input(AVFilterLink *inlink) return 0; } -static inline void blur8(uint8_t *dst, int dst_step, const uint8_t *src, int src_step, -int len, int radius) -{ -/* Naive boxblur would sum source pixels from x-radius .. x+radius - * for destination pixel x. That would be O(radius*width). - * If you now look at what source pixels represent 2 consecutive - * output pixels, then you see they are almost identical and only - * differ by 2 pixels, like: - * src0 1 - * dst0 1 - * src11 - * dst11 - * src0-src1 1 -1 - * so when you know one output pixel you can find the next by just adding - * and subtracting 1 input pixel. - * The following code adopts this faster variant. - */ -const int length = radius*2 + 1; -const int inv = ((1<<16) + length/2)/length; -int x, sum = src[radius*src_step]; - -for (x = 0; x < radius; x++) -sum += src[x*src_step]<<1; - -sum = sum*inv + (1<<15); - -for (x = 0; x <= radius; x++) { -sum += (src[(radius+x)*src_step] - src[(radius-x)*src_step])*inv; -dst[x*dst_step] = sum>>16; -} - -for (; x < len-radius; x++) { -sum += (src[(radius+x)*src_step] - src[(x-radius-1)*src_step])*inv; -dst[x*dst_step] = sum >>16; -} - -for (; x < len; x++) { -sum += (src[(2*len-radius-x-1)*src_step] - src[(x-radius-1)*src_step])*inv; -dst[x*dst_step] = sum>>16; -} +/* Naive boxblur would sum source pixels from x-radius .. x+radius + * for destination pixel x. That would be O(radius*width). + * If you now look at what source pixels represent 2 consecutive + * output pixels, then you see they are almost identical and only + * differ by 2 pixels, like: + * src0 1 + * dst0 1 + * src11 + * dst11 + * src0-src1 1 -1 + * so when you know one output pixel you can find the next by just adding + * and subtracting 1 input pixel. + * The following code adopts this faster variant. + */ +#define BLUR(type, depth) \ +static inline void blur ## depth(type *dst, int dst_step, const type *src, \ + int src_step, int len, int radius) \ +{ \ +const int length = radius*2 + 1;\ +const int inv = ((1<<16) + length/2)/length;\ +int x, sum = src[radius*src_step]; \ +\ +for (x = 0; x < radius; x++)\ +sum += src[x*src_step]<<1; \ +\ +sum = sum*inv + (1<<15);\ +\ +for (x = 0; x <= radius; x++) { \ +sum += (src[(radius+x)*src_step] - src[(radius-x)*src_step])*inv; \ +dst[x*dst_step] = sum>>16; \ +} \ +\ +for (; x < len-radius; x++) { \ +sum += (src[(radius+x)*src_step] - src[(x-radius-1)*src_step])*inv; \ +dst[x*dst_step] = sum >>16; \ +} \ +\ +for (; x < len; x++) { \ +sum += (src[(2*len-radius-x-1)*src_step] - src[(x-radius-1)*src_step])*inv; \ +dst[x*dst_step] = sum>>16; \ +} \ } -static inline void blur16(uint16_t *dst, int dst_step, const uint16_t *src, int src_step, - int len, int radius) -{ -const int length = radius*2 + 1; -const int inv = ((1<<16) + length/2)/length; -int x, sum = src[radius*src_step]; - -for (x = 0; x < radius; x++) -sum += src[x*src_step]<<1; - -sum = sum*inv + (1<<15); - -for (x = 0; x <= radius; x++) { -
Re: [FFmpeg-devel] [PATCH] boxblur: Templatize blur{8,16}
On Sun, Nov 1, 2015 at 1:40 PM, Timothy Gu wrote: > --- > libavfilter/vf_boxblur.c | 110 > +++ > 1 file changed, 44 insertions(+), 66 deletions(-) > > diff --git a/libavfilter/vf_boxblur.c b/libavfilter/vf_boxblur.c > index ef01cf9..6934076 100644 > --- a/libavfilter/vf_boxblur.c > +++ b/libavfilter/vf_boxblur.c > @@ -204,75 +204,53 @@ static int config_input(AVFilterLink *inlink) > return 0; > } > > -static inline void blur8(uint8_t *dst, int dst_step, const uint8_t *src, int > src_step, > -int len, int radius) > -{ > -/* Naive boxblur would sum source pixels from x-radius .. x+radius > - * for destination pixel x. That would be O(radius*width). > - * If you now look at what source pixels represent 2 consecutive > - * output pixels, then you see they are almost identical and only > - * differ by 2 pixels, like: > - * src0 1 > - * dst0 1 > - * src11 > - * dst11 > - * src0-src1 1 -1 > - * so when you know one output pixel you can find the next by just adding > - * and subtracting 1 input pixel. > - * The following code adopts this faster variant. > - */ > -const int length = radius*2 + 1; > -const int inv = ((1<<16) + length/2)/length; > -int x, sum = src[radius*src_step]; > - > -for (x = 0; x < radius; x++) > -sum += src[x*src_step]<<1; > - > -sum = sum*inv + (1<<15); > - > -for (x = 0; x <= radius; x++) { > -sum += (src[(radius+x)*src_step] - src[(radius-x)*src_step])*inv; > -dst[x*dst_step] = sum>>16; > -} > - > -for (; x < len-radius; x++) { > -sum += (src[(radius+x)*src_step] - src[(x-radius-1)*src_step])*inv; > -dst[x*dst_step] = sum >>16; > -} > - > -for (; x < len; x++) { > -sum += (src[(2*len-radius-x-1)*src_step] - > src[(x-radius-1)*src_step])*inv; > -dst[x*dst_step] = sum>>16; > -} > +/* Naive boxblur would sum source pixels from x-radius .. x+radius > + * for destination pixel x. That would be O(radius*width). > + * If you now look at what source pixels represent 2 consecutive > + * output pixels, then you see they are almost identical and only > + * differ by 2 pixels, like: > + * src0 1 > + * dst0 1 > + * src11 > + * dst11 > + * src0-src1 1 -1 > + * so when you know one output pixel you can find the next by just adding > + * and subtracting 1 input pixel. > + * The following code adopts this faster variant. > + */ > +#define BLUR(type, depth) \ > +static inline void blur ## depth(type *dst, int dst_step, const type *src, \ > + int src_step, int len, int radius) \ > +{ \ > +const int length = radius*2 + 1;\ > +const int inv = ((1<<16) + length/2)/length;\ > +int x, sum = src[radius*src_step]; \ > +\ > +for (x = 0; x < radius; x++)\ > +sum += src[x*src_step]<<1; \ > +\ > +sum = sum*inv + (1<<15);\ > +\ > +for (x = 0; x <= radius; x++) { \ > +sum += (src[(radius+x)*src_step] - src[(radius-x)*src_step])*inv; \ > +dst[x*dst_step] = sum>>16; \ > +} \ > +\ > +for (; x < len-radius; x++) { \ > +sum += (src[(radius+x)*src_step] - src[(x-radius-1)*src_step])*inv; \ > +dst[x*dst_step] = sum >>16; \ > +} \ > +\ > +for (; x < len; x++) { \ > +sum += (src[(2*len-radius-x-1)*src_step] - > src[(x-radius-1)*src_step])*inv; \ > +dst[x*dst_step] = sum>>16; \ > +} \ > } > > -static inline void blur16(uint16_t *dst, int dst_step, const uint16_t *src, > int src_step, > - int len, int radius) > -{ > -const int leng
Re: [FFmpeg-devel] [PATCH 1/4] avfilter/all: check and propagate the return code of av_expr_parse_and_eval
On Sat, Oct 31, 2015 at 10:47:54AM -0400, Ganesh Ajjanagadde wrote: > This function can return ENOMEM that needs to be propagated. > > Signed-off-by: Ganesh Ajjanagadde > --- > libavfilter/vf_pad.c| 10 ++ > libavfilter/vf_rotate.c | 3 +-- > libavfilter/vf_scale.c | 5 +++-- > libavfilter/vf_zscale.c | 5 +++-- > 4 files changed, 13 insertions(+), 10 deletions(-) > > diff --git a/libavfilter/vf_pad.c b/libavfilter/vf_pad.c > index 63dc6a8..e10d41b 100644 > --- a/libavfilter/vf_pad.c > +++ b/libavfilter/vf_pad.c > @@ -114,9 +114,10 @@ static int config_input(AVFilterLink *inlink) > var_values[VAR_VSUB] = 1 << s->draw.vsub_max; > > /* evaluate width and height */ > -av_expr_parse_and_eval(&res, (expr = s->w_expr), > +if ((ret = av_expr_parse_and_eval(&res, (expr = s->w_expr), > var_names, var_values, > - NULL, NULL, NULL, NULL, NULL, 0, ctx); > + NULL, NULL, NULL, NULL, NULL, 0, ctx)) < 0) > +goto eval_fail; > s->w = var_values[VAR_OUT_W] = var_values[VAR_OW] = res; > if ((ret = av_expr_parse_and_eval(&res, (expr = s->h_expr), >var_names, var_values, > @@ -131,9 +132,10 @@ static int config_input(AVFilterLink *inlink) > s->w = var_values[VAR_OUT_W] = var_values[VAR_OW] = res; > > /* evaluate x and y */ > -av_expr_parse_and_eval(&res, (expr = s->x_expr), > +if ((ret = av_expr_parse_and_eval(&res, (expr = s->x_expr), > var_names, var_values, > - NULL, NULL, NULL, NULL, NULL, 0, ctx); > + NULL, NULL, NULL, NULL, NULL, 0, ctx)) < 0) > +goto eval_fail; > s->x = var_values[VAR_X] = res; > if ((ret = av_expr_parse_and_eval(&res, (expr = s->y_expr), >var_names, var_values, > diff --git a/libavfilter/vf_rotate.c b/libavfilter/vf_rotate.c > index f12a103..d5e01e2 100644 > --- a/libavfilter/vf_rotate.c > +++ b/libavfilter/vf_rotate.c > @@ -235,8 +235,7 @@ static int config_props(AVFilterLink *outlink) > } while (0) > > /* evaluate width and height */ > -av_expr_parse_and_eval(&res, expr = rot->outw_expr_str, var_names, > rot->var_values, > - func1_names, func1, NULL, NULL, rot, 0, ctx); > +SET_SIZE_EXPR(outw, "out_w"); > rot->var_values[VAR_OUT_W] = rot->var_values[VAR_OW] = res; > rot->outw = res + 0.5; > SET_SIZE_EXPR(outh, "out_w"); > diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c > index 1032688..2cf14e0 100644 > --- a/libavfilter/vf_scale.c > +++ b/libavfilter/vf_scale.c > @@ -265,9 +265,10 @@ static int config_props(AVFilterLink *outlink) > var_values[VAR_OVSUB] = 1 << out_desc->log2_chroma_h; > > /* evaluate width and height */ > -av_expr_parse_and_eval(&res, (expr = scale->w_expr), > +if ((ret = av_expr_parse_and_eval(&res, (expr = scale->w_expr), > var_names, var_values, > - NULL, NULL, NULL, NULL, NULL, 0, ctx); > + NULL, NULL, NULL, NULL, NULL, 0, ctx)) < 0) > +goto fail; > scale->w = var_values[VAR_OUT_W] = var_values[VAR_OW] = res; > if ((ret = av_expr_parse_and_eval(&res, (expr = scale->h_expr), >var_names, var_values, the first evaluation can fail and such failure does not represent an error for the calling code this is needed to support referencing the output parameters, for example scale=iw/ih*oh*sar:240 here the first evalution of width fails but the 3rd succeeds as the 2nd made "oh" available please make sure the other hunks dont introduce similar issues in other filters (theres similar such double eval in other filters) also it would make sense to add comments to each such occurance of double evaluation so noone else adds a check there by mistake [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The worst form of inequality is to try to make unequal things equal. -- Aristotle signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] boxblur: Templatize blur{8,16}
On Sun, Nov 1, 2015 at 11:10 AM Ganesh Ajjanagadde wrote: > Have not tested, but just a general comment. Personally, I follow the > twice repition is ok, thrice means it is good to factor out. I would > have been happier if the diff-stat was better than 44+/66- in terms of > deletions. The most significant reason why the addition is as high as it is is the comment describing the algorithm. Without taking into account the 14-line comment, the diffstat would be 30+/52-, i.e. 1.73x. The only part of the function that is different is the prototype, which I believe is small enough to warrant a refactoring. Timothy ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/4] avfilter/all: check and propagate the return code of av_expr_parse_and_eval
On Sun, Nov 1, 2015 at 2:14 PM, Michael Niedermayer wrote: > On Sat, Oct 31, 2015 at 10:47:54AM -0400, Ganesh Ajjanagadde wrote: >> This function can return ENOMEM that needs to be propagated. >> >> Signed-off-by: Ganesh Ajjanagadde >> --- >> libavfilter/vf_pad.c| 10 ++ >> libavfilter/vf_rotate.c | 3 +-- >> libavfilter/vf_scale.c | 5 +++-- >> libavfilter/vf_zscale.c | 5 +++-- >> 4 files changed, 13 insertions(+), 10 deletions(-) >> >> diff --git a/libavfilter/vf_pad.c b/libavfilter/vf_pad.c >> index 63dc6a8..e10d41b 100644 >> --- a/libavfilter/vf_pad.c >> +++ b/libavfilter/vf_pad.c >> @@ -114,9 +114,10 @@ static int config_input(AVFilterLink *inlink) >> var_values[VAR_VSUB] = 1 << s->draw.vsub_max; >> >> /* evaluate width and height */ >> -av_expr_parse_and_eval(&res, (expr = s->w_expr), >> +if ((ret = av_expr_parse_and_eval(&res, (expr = s->w_expr), >> var_names, var_values, >> - NULL, NULL, NULL, NULL, NULL, 0, ctx); >> + NULL, NULL, NULL, NULL, NULL, 0, ctx)) < 0) >> +goto eval_fail; >> s->w = var_values[VAR_OUT_W] = var_values[VAR_OW] = res; >> if ((ret = av_expr_parse_and_eval(&res, (expr = s->h_expr), >>var_names, var_values, >> @@ -131,9 +132,10 @@ static int config_input(AVFilterLink *inlink) >> s->w = var_values[VAR_OUT_W] = var_values[VAR_OW] = res; >> >> /* evaluate x and y */ >> -av_expr_parse_and_eval(&res, (expr = s->x_expr), >> +if ((ret = av_expr_parse_and_eval(&res, (expr = s->x_expr), >> var_names, var_values, >> - NULL, NULL, NULL, NULL, NULL, 0, ctx); >> + NULL, NULL, NULL, NULL, NULL, 0, ctx)) < 0) >> +goto eval_fail; >> s->x = var_values[VAR_X] = res; >> if ((ret = av_expr_parse_and_eval(&res, (expr = s->y_expr), >>var_names, var_values, >> diff --git a/libavfilter/vf_rotate.c b/libavfilter/vf_rotate.c >> index f12a103..d5e01e2 100644 >> --- a/libavfilter/vf_rotate.c >> +++ b/libavfilter/vf_rotate.c >> @@ -235,8 +235,7 @@ static int config_props(AVFilterLink *outlink) >> } while (0) >> >> /* evaluate width and height */ >> -av_expr_parse_and_eval(&res, expr = rot->outw_expr_str, var_names, >> rot->var_values, >> - func1_names, func1, NULL, NULL, rot, 0, ctx); >> +SET_SIZE_EXPR(outw, "out_w"); >> rot->var_values[VAR_OUT_W] = rot->var_values[VAR_OW] = res; >> rot->outw = res + 0.5; >> SET_SIZE_EXPR(outh, "out_w"); > > >> diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c >> index 1032688..2cf14e0 100644 >> --- a/libavfilter/vf_scale.c >> +++ b/libavfilter/vf_scale.c >> @@ -265,9 +265,10 @@ static int config_props(AVFilterLink *outlink) >> var_values[VAR_OVSUB] = 1 << out_desc->log2_chroma_h; >> >> /* evaluate width and height */ >> -av_expr_parse_and_eval(&res, (expr = scale->w_expr), >> +if ((ret = av_expr_parse_and_eval(&res, (expr = scale->w_expr), >> var_names, var_values, >> - NULL, NULL, NULL, NULL, NULL, 0, ctx); >> + NULL, NULL, NULL, NULL, NULL, 0, ctx)) < 0) >> +goto fail; >> scale->w = var_values[VAR_OUT_W] = var_values[VAR_OW] = res; >> if ((ret = av_expr_parse_and_eval(&res, (expr = scale->h_expr), >>var_names, var_values, > > the first evaluation can fail and such failure does not represent an > error for the calling code > > this is needed to support referencing the output parameters, > for example > scale=iw/ih*oh*sar:240 > > here the first evalution of width fails but the 3rd succeeds as > the 2nd made "oh" available > > please make sure the other hunks dont introduce similar issues > in other filters (theres similar such double eval in other filters) > also it would make sense to add comments to each such occurance of > double evaluation so noone else adds a check there by mistake So if I understand correctly there is both ENOMEM and EINVAL. I think ENOMEM should be a "hard" failure that exits immediately - I doubt that a second evaluation makes sense in such a case. For EINVAL, your point makes sense. This will need a little more careful rework. Thanks. Will repost patches once other two are reviewed. > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > The worst form of inequality is to try to make unequal things equal. > -- Aristotle > > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 0/2] mpegtsenc: Add support for muxing Opus into MPEG-TS
Hi everybody, the following patches are adding support for muxing Opus into MPEG-TS. ffmpeg could already demux such files since a while, but I guess it would also be nice to be able to create them. Please review carefully, thanks! Sebastian ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 1/2] mpegtsenc: Add support for muxing Opus in MPEG-TS
From: Sebastian Dröge Signed-off-by: Sebastian Dröge --- libavformat/mpegtsenc.c | 180 +++- 1 file changed, 179 insertions(+), 1 deletion(-) diff --git a/libavformat/mpegtsenc.c b/libavformat/mpegtsenc.c index 4d74252..a7e78ac 100644 --- a/libavformat/mpegtsenc.c +++ b/libavformat/mpegtsenc.c @@ -227,6 +227,9 @@ typedef struct MpegTSWriteStream { uint8_t *payload; AVFormatContext *amux; AVRational user_tb; + +/* For Opus */ +int opus_queued_samples; } MpegTSWriteStream; static void mpegts_write_pat(AVFormatContext *s) @@ -314,6 +317,9 @@ static int mpegts_write_pmt(AVFormatContext *s, MpegTSService *service) case AV_CODEC_ID_TRUEHD: stream_type = STREAM_TYPE_AUDIO_TRUEHD; break; +case AV_CODEC_ID_OPUS: +stream_type = STREAM_TYPE_PRIVATE_DATA; +break; default: stream_type = STREAM_TYPE_PRIVATE_DATA; break; @@ -340,6 +346,82 @@ static int mpegts_write_pmt(AVFormatContext *s, MpegTSService *service) *q++ = 'S'; *q++ = 'D'; } +if (st->codec->codec_id==AV_CODEC_ID_OPUS) { +/* 6 bytes registration descriptor, 4 bytes Opus audio descriptor */ +if (q - data > SECTION_LENGTH - 6 - 4) { +err = 1; +break; +} + +*q++ = 0x05; /* MPEG-2 registration descriptor*/ +*q++ = 4; +*q++ = 'O'; +*q++ = 'p'; +*q++ = 'u'; +*q++ = 's'; + +*q++ = 0x7f; /* DVB extension descriptor */ +*q++ = 2; +*q++ = 0x80; + +if (st->codec->extradata && st->codec->extradata_size >= 19) { +if (st->codec->extradata[18] == 0 && st->codec->channels <= 2) { +/* RTP mapping family */ +*q++ = st->codec->channels; +} else if (st->codec->extradata[18] == 1 && st->codec->channels <= 8 && + st->codec->extradata_size >= 22 + st->codec->channels) { +static const uint8_t coupled_stream_counts[9] = { +1, 0, 1, 1, 2, 2, 2, 3, 3 +}; +static const uint8_t channel_map_a[8][8] = { +{0}, +{0, 1}, +{0, 2, 1}, +{0, 1, 2, 3}, +{0, 4, 1, 2, 3}, +{0, 4, 1, 2, 3, 5}, +{0, 4, 1, 2, 3, 5, 6}, +{0, 6, 1, 2, 3, 4, 5, 7}, +}; +static const uint8_t channel_map_b[8][8] = { +{0}, +{0, 1}, +{0, 1, 2}, +{0, 1, 2, 3}, +{0, 1, 2, 3, 4}, +{0, 1, 2, 3, 4, 5}, +{0, 1, 2, 3, 4, 5, 6}, +{0, 1, 2, 3, 4, 5, 6, 7}, +}; +/* Vorbis mapping family */ + +if (st->codec->extradata[19] == st->codec->channels - coupled_stream_counts[st->codec->channels] && +st->codec->extradata[20] == coupled_stream_counts[st->codec->channels] && +memcmp(&st->codec->extradata[21], channel_map_a[st->codec->channels], st->codec->channels) == 0) { +*q++ = st->codec->channels; +} else if (st->codec->channels >= 2 && st->codec->extradata[19] == st->codec->channels && + st->codec->extradata[20] == 0 && + memcmp(&st->codec->extradata[21], channel_map_b[st->codec->channels], st->codec->channels) == 0) { +*q++ = st->codec->channels | 0x80; +} else { +/* Unsupported, could write an extended descriptor here */ +av_log(s, AV_LOG_ERROR, "Unsupported Opus Vorbis-style channel mapping"); +*q++ = 0xff; +} +} else { +/* Unsupported */ +av_log(s, AV_LOG_ERROR, "Unsupported Opus channel mapping for family %d", st->codec->extradata[18]); +*q++ = 0xff; +} +} else if (st->codec->channels <= 2) { +/* Assume RTP mapping family */ +*q++ = st->codec->channels; +} else { +/* Unsupported */ +av_log(s, AV_LOG_ERROR, "Unsupported Opus cha
[FFmpeg-devel] [PATCH 2/2] mpegtsenc: Implement writing of Opus trim_start/trim_end control values
From: Sebastian Dröge Signed-off-by: Sebastian Dröge --- libavformat/mpegtsenc.c | 47 --- 1 file changed, 44 insertions(+), 3 deletions(-) diff --git a/libavformat/mpegtsenc.c b/libavformat/mpegtsenc.c index a7e78ac..023ca02 100644 --- a/libavformat/mpegtsenc.c +++ b/libavformat/mpegtsenc.c @@ -230,6 +230,8 @@ typedef struct MpegTSWriteStream { /* For Opus */ int opus_queued_samples; +int opus_pending_trim_start; +int opus_pending_trim_end; } MpegTSWriteStream; static void mpegts_write_pat(AVFormatContext *s) @@ -825,6 +827,9 @@ static int mpegts_write_header(AVFormatContext *s) if (ret < 0) goto fail; } +if (st->codec->codec_id == AV_CODEC_ID_OPUS) { +ts_st->opus_pending_trim_start = st->codec->initial_padding * 48000 / st->codec->sample_rate; +} } av_freep(&pids); @@ -1506,24 +1511,47 @@ static int mpegts_write_packet_internal(AVFormatContext *s, AVPacket *pkt) if (ret < 0) return ret; } else if (st->codec->codec_id == AV_CODEC_ID_OPUS) { +uint8_t *side_data; +int side_data_size; + if (pkt->size < 2) { av_log(s, AV_LOG_ERROR, "Opus packet too short\n"); return AVERROR_INVALIDDATA; } +side_data = av_packet_get_side_data(pkt, +AV_PKT_DATA_SKIP_SAMPLES, +&side_data_size); + +if (side_data && side_data_size >= 10) { +uint32_t discard_padding = AV_RL32(side_data + 4) * 48000 / st->codec->sample_rate; +ts_st->opus_pending_trim_end += discard_padding; +} + /* Add Opus control header */ if ((AV_RB16(pkt->data) >> 5) != 0x3ff) { int i, n; +int ctrl_header_size; +int trim_start = 0, trim_end = 0; opus_samples = opus_get_packet_samples(s, pkt); -data = av_malloc(pkt->size + 2 + pkt->size / 255 + 1); +ctrl_header_size = pkt->size + 2 + pkt->size / 255 + 1; +if (ts_st->opus_pending_trim_start) + ctrl_header_size += 2; +if (ts_st->opus_pending_trim_end) + ctrl_header_size += 2; + +data = av_malloc(ctrl_header_size); if (!data) return AVERROR(ENOMEM); -/* TODO: Write trim if needed */ data[0] = 0x7f; data[1] = 0xe0; +if (ts_st->opus_pending_trim_start) +data[1] |= 0x10; +if (ts_st->opus_pending_trim_end) +data[1] |= 0x08; n = pkt->size; i = 2; @@ -1535,9 +1563,22 @@ static int mpegts_write_packet_internal(AVFormatContext *s, AVPacket *pkt) av_assert0(2 + pkt->size / 255 + 1 == i); +if (ts_st->opus_pending_trim_start) { +trim_start = FFMIN(ts_st->opus_pending_trim_start, opus_samples); +AV_WB16(data + i, trim_start); +i += 2; +ts_st->opus_pending_trim_start -= trim_start; +} +if (ts_st->opus_pending_trim_end) { +trim_end = FFMIN(ts_st->opus_pending_trim_end, opus_samples - trim_start); +AV_WB16(data + i, trim_end); +i += 2; +ts_st->opus_pending_trim_end -= trim_end; +} + memcpy(data + i, pkt->data, pkt->size); buf = data; -size= pkt->size + 2 + pkt->size / 255 + 1; +size= ctrl_header_size; } else { /* TODO: Can we get TS formatted data here? If so we will * need to count the samples of that too! */ -- 2.6.2 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCHv2 3/4] avutil/eval: check av_expr_parse_and_eval error code in the test build
This returns the error code from main in the test in the case of ENOMEM. This should not matter, and will ensure that no warnings are triggered when av_warn_unused_result is added to avutil/eval.h. Tested with FATE. Signed-off-by: Ganesh Ajjanagadde --- libavutil/eval.c | 22 +- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/libavutil/eval.c b/libavutil/eval.c index 7642b91..acb3339 100644 --- a/libavutil/eval.c +++ b/libavutil/eval.c @@ -750,7 +750,7 @@ static const char *const const_names[] = { int main(int argc, char **argv) { -int i; +int i, ret; double d; const char *const *expr; static const char *const exprs[] = { @@ -854,30 +854,34 @@ int main(int argc, char **argv) for (expr = exprs; *expr; expr++) { printf("Evaluating '%s'\n", *expr); -av_expr_parse_and_eval(&d, *expr, +if ((ret = av_expr_parse_and_eval(&d, *expr, const_names, const_values, - NULL, NULL, NULL, NULL, NULL, 0, NULL); + NULL, NULL, NULL, NULL, NULL, 0, NULL)) == AVERROR(ENOMEM)) +return ret; if (isnan(d)) printf("'%s' -> nan\n\n", *expr); else printf("'%s' -> %f\n\n", *expr, d); } -av_expr_parse_and_eval(&d, "1+(5-2)^(3-1)+1/2+sin(PI)-max(-2.2,-3.1)", +if ((ret = av_expr_parse_and_eval(&d, "1+(5-2)^(3-1)+1/2+sin(PI)-max(-2.2,-3.1)", const_names, const_values, - NULL, NULL, NULL, NULL, NULL, 0, NULL); + NULL, NULL, NULL, NULL, NULL, 0, NULL)) < 0) +return ret; printf("%f == 12.7\n", d); -av_expr_parse_and_eval(&d, "80G/80Gi", +if ((ret = av_expr_parse_and_eval(&d, "80G/80Gi", const_names, const_values, - NULL, NULL, NULL, NULL, NULL, 0, NULL); + NULL, NULL, NULL, NULL, NULL, 0, NULL)) < 0) +return ret; printf("%f == 0.931322575\n", d); if (argc > 1 && !strcmp(argv[1], "-t")) { for (i = 0; i < 1050; i++) { START_TIMER; -av_expr_parse_and_eval(&d, "1+(5-2)^(3-1)+1/2+sin(PI)-max(-2.2,-3.1)", +if ((ret = av_expr_parse_and_eval(&d, "1+(5-2)^(3-1)+1/2+sin(PI)-max(-2.2,-3.1)", const_names, const_values, - NULL, NULL, NULL, NULL, NULL, 0, NULL); + NULL, NULL, NULL, NULL, NULL, 0, NULL)) < 0) +return ret; STOP_TIMER("av_expr_parse_and_eval"); } } -- 2.6.2 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 3/4] avutil/eval: check av_expr_parse_and_eval error code in the test build
On Sun, Nov 1, 2015 at 1:33 PM, Ganesh Ajjanagadde wrote: > On Sun, Nov 1, 2015 at 12:52 PM, Michael Niedermayer > wrote: >> On Sat, Oct 31, 2015 at 10:47:56AM -0400, Ganesh Ajjanagadde wrote: >>> This returns the error code from main in the test, in this case ENOMEM. >>> This should not matter, and will ensure that no warnings are triggered >>> when av_warn_unused_result is added to avutil/eval.h. >>> >>> Signed-off-by: Ganesh Ajjanagadde >>> --- >>> libavutil/eval.c | 22 +- >>> 1 file changed, 13 insertions(+), 9 deletions(-) >> >> breaks fate >> >> TESTeval >> --- ./tests/ref/fate/eval 2015-10-30 03:22:52.209691909 +0100 >> +++ tests/data/fate/eval2015-11-01 18:48:37.730502524 +0100 >> @@ -1,281 +1 @@ >> Evaluating '' >> -'' -> nan >> - >> -Evaluating '1;2' >> -'1;2' -> 2.00 >> - >> -Evaluating '-20' >> -'-20' -> -20.00 >> - >> -Evaluating '-PI' >> -'-PI' -> -3.141593 >> - >> [...] >> -12.70 == 12.7 >> -0.931323 == 0.931322575 >> Test eval failed. Look at tests/data/fate/eval.err for details. >> make: *** [fate-eval] Error 234 >> make: *** Waiting for unfinished jobs >> >> cat tests/data/fate/eval.err >> [Eval @ 0x7ffe40285770] Undefined constant or missing '(' in '' > > So I know why this occurs: it is the first condition with the nan > stuff, which returns an error code < 0. There are various methods I > can think of: > 1. Let the fate build be slightly noisy, with one -Warn-unused-result for > this. > 2. Simply do a ret = av_expr_parse_and_eval(...) for the first one. > This kills the warning, and is quite readable. > 3. in addition to the above, do a check for ENOMEM. I find this quite > silly, since it is a test build. > > Let me know what you prefer, and sorry for not being thorough with this. v2 posted using the ENOMEM check - it maybe silly, but is correct, suppresses the warning, and is consistent with the approach I adopted for the filters. > >> >> [...] >> -- >> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB >> >> Dictatorship naturally arises out of democracy, and the most aggravated >> form of tyranny and slavery out of the most extreme liberty. -- Plato >> >> ___ >> ffmpeg-devel mailing list >> ffmpeg-devel@ffmpeg.org >> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >> ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCHv2 1/4] avfilter/all: check and propagate the return code of av_expr_parse_and_eval
This function can return ENOMEM that needs to be propagated. Signed-off-by: Ganesh Ajjanagadde --- libavfilter/vf_pad.c| 12 +++- libavfilter/vf_rotate.c | 5 +++-- libavfilter/vf_scale.c | 5 +++-- libavfilter/vf_zscale.c | 5 +++-- 4 files changed, 16 insertions(+), 11 deletions(-) diff --git a/libavfilter/vf_pad.c b/libavfilter/vf_pad.c index 63dc6a8..a40f5fa 100644 --- a/libavfilter/vf_pad.c +++ b/libavfilter/vf_pad.c @@ -114,9 +114,10 @@ static int config_input(AVFilterLink *inlink) var_values[VAR_VSUB] = 1 << s->draw.vsub_max; /* evaluate width and height */ -av_expr_parse_and_eval(&res, (expr = s->w_expr), +if ((ret = av_expr_parse_and_eval(&res, (expr = s->w_expr), var_names, var_values, - NULL, NULL, NULL, NULL, NULL, 0, ctx); + NULL, NULL, NULL, NULL, NULL, 0, ctx)) == AVERROR(ENOMEM)) +goto eval_fail; s->w = var_values[VAR_OUT_W] = var_values[VAR_OW] = res; if ((ret = av_expr_parse_and_eval(&res, (expr = s->h_expr), var_names, var_values, @@ -126,14 +127,15 @@ static int config_input(AVFilterLink *inlink) /* evaluate the width again, as it may depend on the evaluated output height */ if ((ret = av_expr_parse_and_eval(&res, (expr = s->w_expr), var_names, var_values, - NULL, NULL, NULL, NULL, NULL, 0, ctx)) < 0) + NULL, NULL, NULL, NULL, NULL, 0, ctx)) == AVERROR(ENOMEM)) goto eval_fail; s->w = var_values[VAR_OUT_W] = var_values[VAR_OW] = res; /* evaluate x and y */ -av_expr_parse_and_eval(&res, (expr = s->x_expr), +if ((ret = av_expr_parse_and_eval(&res, (expr = s->x_expr), var_names, var_values, - NULL, NULL, NULL, NULL, NULL, 0, ctx); + NULL, NULL, NULL, NULL, NULL, 0, ctx)) == AVERROR(ENOMEM)) +goto eval_fail; s->x = var_values[VAR_X] = res; if ((ret = av_expr_parse_and_eval(&res, (expr = s->y_expr), var_names, var_values, diff --git a/libavfilter/vf_rotate.c b/libavfilter/vf_rotate.c index f12a103..035c2e9 100644 --- a/libavfilter/vf_rotate.c +++ b/libavfilter/vf_rotate.c @@ -235,8 +235,9 @@ static int config_props(AVFilterLink *outlink) } while (0) /* evaluate width and height */ -av_expr_parse_and_eval(&res, expr = rot->outw_expr_str, var_names, rot->var_values, - func1_names, func1, NULL, NULL, rot, 0, ctx); +if ((ret = av_expr_parse_and_eval(&res, expr = rot->outw_expr_str, var_names, rot->var_values, + func1_names, func1, NULL, NULL, rot, 0, ctx)) == AVERROR(ENOMEM)) +return ret; rot->var_values[VAR_OUT_W] = rot->var_values[VAR_OW] = res; rot->outw = res + 0.5; SET_SIZE_EXPR(outh, "out_w"); diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c index 1032688..401d837 100644 --- a/libavfilter/vf_scale.c +++ b/libavfilter/vf_scale.c @@ -265,9 +265,10 @@ static int config_props(AVFilterLink *outlink) var_values[VAR_OVSUB] = 1 << out_desc->log2_chroma_h; /* evaluate width and height */ -av_expr_parse_and_eval(&res, (expr = scale->w_expr), +if ((ret = av_expr_parse_and_eval(&res, (expr = scale->w_expr), var_names, var_values, - NULL, NULL, NULL, NULL, NULL, 0, ctx); + NULL, NULL, NULL, NULL, NULL, 0, ctx)) == AVERROR(ENOMEM)) +goto fail; scale->w = var_values[VAR_OUT_W] = var_values[VAR_OW] = res; if ((ret = av_expr_parse_and_eval(&res, (expr = scale->h_expr), var_names, var_values, diff --git a/libavfilter/vf_zscale.c b/libavfilter/vf_zscale.c index 86e5f5f..2b3c11f 100644 --- a/libavfilter/vf_zscale.c +++ b/libavfilter/vf_zscale.c @@ -206,9 +206,10 @@ static int config_props(AVFilterLink *outlink) var_values[VAR_OVSUB] = 1 << out_desc->log2_chroma_h; /* evaluate width and height */ -av_expr_parse_and_eval(&res, (expr = s->w_expr), +if ((ret = av_expr_parse_and_eval(&res, (expr = s->w_expr), var_names, var_values, - NULL, NULL, NULL, NULL, NULL, 0, ctx); + NULL, NULL, NULL, NULL, NULL, 0, ctx)) == AVERROR(ENOMEM)) +goto fail; s->w = var_values[VAR_OUT_W] = var_values[VAR_OW] = res; if ((ret = av_expr_parse_and_eval(&res, (expr = s->h_expr), var_names, var_values, -- 2.6.2 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/4] avfilter/all: check and propagate the return code of av_expr_parse_and_eval
On Sun, Nov 1, 2015 at 2:14 PM, Michael Niedermayer wrote: > On Sat, Oct 31, 2015 at 10:47:54AM -0400, Ganesh Ajjanagadde wrote: >> This function can return ENOMEM that needs to be propagated. >> >> Signed-off-by: Ganesh Ajjanagadde >> --- >> libavfilter/vf_pad.c| 10 ++ >> libavfilter/vf_rotate.c | 3 +-- >> libavfilter/vf_scale.c | 5 +++-- >> libavfilter/vf_zscale.c | 5 +++-- >> 4 files changed, 13 insertions(+), 10 deletions(-) >> >> diff --git a/libavfilter/vf_pad.c b/libavfilter/vf_pad.c >> index 63dc6a8..e10d41b 100644 >> --- a/libavfilter/vf_pad.c >> +++ b/libavfilter/vf_pad.c >> @@ -114,9 +114,10 @@ static int config_input(AVFilterLink *inlink) >> var_values[VAR_VSUB] = 1 << s->draw.vsub_max; >> >> /* evaluate width and height */ >> -av_expr_parse_and_eval(&res, (expr = s->w_expr), >> +if ((ret = av_expr_parse_and_eval(&res, (expr = s->w_expr), >> var_names, var_values, >> - NULL, NULL, NULL, NULL, NULL, 0, ctx); >> + NULL, NULL, NULL, NULL, NULL, 0, ctx)) < 0) >> +goto eval_fail; >> s->w = var_values[VAR_OUT_W] = var_values[VAR_OW] = res; >> if ((ret = av_expr_parse_and_eval(&res, (expr = s->h_expr), >>var_names, var_values, >> @@ -131,9 +132,10 @@ static int config_input(AVFilterLink *inlink) >> s->w = var_values[VAR_OUT_W] = var_values[VAR_OW] = res; >> >> /* evaluate x and y */ >> -av_expr_parse_and_eval(&res, (expr = s->x_expr), >> +if ((ret = av_expr_parse_and_eval(&res, (expr = s->x_expr), >> var_names, var_values, >> - NULL, NULL, NULL, NULL, NULL, 0, ctx); >> + NULL, NULL, NULL, NULL, NULL, 0, ctx)) < 0) >> +goto eval_fail; >> s->x = var_values[VAR_X] = res; >> if ((ret = av_expr_parse_and_eval(&res, (expr = s->y_expr), >>var_names, var_values, >> diff --git a/libavfilter/vf_rotate.c b/libavfilter/vf_rotate.c >> index f12a103..d5e01e2 100644 >> --- a/libavfilter/vf_rotate.c >> +++ b/libavfilter/vf_rotate.c >> @@ -235,8 +235,7 @@ static int config_props(AVFilterLink *outlink) >> } while (0) >> >> /* evaluate width and height */ >> -av_expr_parse_and_eval(&res, expr = rot->outw_expr_str, var_names, >> rot->var_values, >> - func1_names, func1, NULL, NULL, rot, 0, ctx); >> +SET_SIZE_EXPR(outw, "out_w"); >> rot->var_values[VAR_OUT_W] = rot->var_values[VAR_OW] = res; >> rot->outw = res + 0.5; >> SET_SIZE_EXPR(outh, "out_w"); > > >> diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c >> index 1032688..2cf14e0 100644 >> --- a/libavfilter/vf_scale.c >> +++ b/libavfilter/vf_scale.c >> @@ -265,9 +265,10 @@ static int config_props(AVFilterLink *outlink) >> var_values[VAR_OVSUB] = 1 << out_desc->log2_chroma_h; >> >> /* evaluate width and height */ >> -av_expr_parse_and_eval(&res, (expr = scale->w_expr), >> +if ((ret = av_expr_parse_and_eval(&res, (expr = scale->w_expr), >> var_names, var_values, >> - NULL, NULL, NULL, NULL, NULL, 0, ctx); >> + NULL, NULL, NULL, NULL, NULL, 0, ctx)) < 0) >> +goto fail; >> scale->w = var_values[VAR_OUT_W] = var_values[VAR_OW] = res; >> if ((ret = av_expr_parse_and_eval(&res, (expr = scale->h_expr), >>var_names, var_values, > > the first evaluation can fail and such failure does not represent an > error for the calling code > > this is needed to support referencing the output parameters, > for example > scale=iw/ih*oh*sar:240 > > here the first evalution of width fails but the 3rd succeeds as > the 2nd made "oh" available > > please make sure the other hunks dont introduce similar issues > in other filters (theres similar such double eval in other filters) > also it would make sense to add comments to each such occurance of > double evaluation so noone else adds a check there by mistake reworked and posted, but did not add comments: I can't think of anything meaningful beyond what is there already - the hint of double evaluation was already there (I just failed to realize why and its implications), and I am adding checks to all for ENOMEM. The asymmetry of the ENOMEM and the < 0 together with existing comments is something I can't see a way to improve upon. > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > The worst form of inequality is to try to make unequal things equal. > -- Aristotle > > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-de
Re: [FFmpeg-devel] [PATCHv2 1/4] avfilter/all: check and propagate the return code of av_expr_parse_and_eval
Le primidi 11 brumaire, an CCXXIV, Ganesh Ajjanagadde a écrit : > This function can return ENOMEM that needs to be propagated. > > Signed-off-by: Ganesh Ajjanagadde > --- > libavfilter/vf_pad.c| 12 +++- > libavfilter/vf_rotate.c | 5 +++-- > libavfilter/vf_scale.c | 5 +++-- > libavfilter/vf_zscale.c | 5 +++-- > 4 files changed, 16 insertions(+), 11 deletions(-) > > diff --git a/libavfilter/vf_pad.c b/libavfilter/vf_pad.c > index 63dc6a8..a40f5fa 100644 > --- a/libavfilter/vf_pad.c > +++ b/libavfilter/vf_pad.c > @@ -114,9 +114,10 @@ static int config_input(AVFilterLink *inlink) > var_values[VAR_VSUB] = 1 << s->draw.vsub_max; > > /* evaluate width and height */ > -av_expr_parse_and_eval(&res, (expr = s->w_expr), > +if ((ret = av_expr_parse_and_eval(&res, (expr = s->w_expr), > var_names, var_values, > - NULL, NULL, NULL, NULL, NULL, 0, ctx); > + NULL, NULL, NULL, NULL, NULL, 0, ctx)) == > AVERROR(ENOMEM)) > +goto eval_fail; I am quite unhappy about this, it is cluttering the code for no good reason and makes the test fragile. Regards, -- Nicolas George signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCHv2 1/4] avfilter/all: check and propagate the return code of av_expr_parse_and_eval
On Sun, Nov 1, 2015 at 3:45 PM, Nicolas George wrote: > Le primidi 11 brumaire, an CCXXIV, Ganesh Ajjanagadde a écrit : >> This function can return ENOMEM that needs to be propagated. >> >> Signed-off-by: Ganesh Ajjanagadde >> --- >> libavfilter/vf_pad.c| 12 +++- >> libavfilter/vf_rotate.c | 5 +++-- >> libavfilter/vf_scale.c | 5 +++-- >> libavfilter/vf_zscale.c | 5 +++-- >> 4 files changed, 16 insertions(+), 11 deletions(-) >> >> diff --git a/libavfilter/vf_pad.c b/libavfilter/vf_pad.c >> index 63dc6a8..a40f5fa 100644 >> --- a/libavfilter/vf_pad.c >> +++ b/libavfilter/vf_pad.c >> @@ -114,9 +114,10 @@ static int config_input(AVFilterLink *inlink) >> var_values[VAR_VSUB] = 1 << s->draw.vsub_max; >> >> /* evaluate width and height */ >> -av_expr_parse_and_eval(&res, (expr = s->w_expr), >> +if ((ret = av_expr_parse_and_eval(&res, (expr = s->w_expr), >> var_names, var_values, >> - NULL, NULL, NULL, NULL, NULL, 0, ctx); >> + NULL, NULL, NULL, NULL, NULL, 0, ctx)) == >> AVERROR(ENOMEM)) >> +goto eval_fail; > > I am quite unhappy about this, it is cluttering the code for no good reason > and makes the test fragile. The fragility is something I don't like either, suggestions? As for the cluttering, there are lots of if (!...) return AVERROR(ENOMEM) all over the place. I assumed that people want such things checked in essentially all circumstances since OOM can hit anywhere. If that is not the case, please help by giving an explanation of when they do/do not matter so that I can focus better on the more essential ones. > > Regards, > > -- > Nicolas George > > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/2] mpegtsenc: Add support for muxing Opus in MPEG-TS
On So, 2015-11-01 at 22:20 +0200, Sebastian Dröge wrote: > +data = av_malloc(pkt->size + 2 + pkt->size / 255 + 1); > [...] > +n = pkt->size; > +i = 2; > +do { > +data[i] = FFMIN(n, 255); > +n -= 255; > +i++; > +} while (n > 0); These two parts are actually wrong in some edge cases. New patch following soon. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCHv2 1/2] mpegtsenc: Add support for muxing Opus in MPEG-TS
From: Sebastian Dröge Signed-off-by: Sebastian Dröge --- libavformat/mpegtsenc.c | 180 +++- 1 file changed, 179 insertions(+), 1 deletion(-) diff --git a/libavformat/mpegtsenc.c b/libavformat/mpegtsenc.c index 4d74252..0674064 100644 --- a/libavformat/mpegtsenc.c +++ b/libavformat/mpegtsenc.c @@ -227,6 +227,9 @@ typedef struct MpegTSWriteStream { uint8_t *payload; AVFormatContext *amux; AVRational user_tb; + +/* For Opus */ +int opus_queued_samples; } MpegTSWriteStream; static void mpegts_write_pat(AVFormatContext *s) @@ -314,6 +317,9 @@ static int mpegts_write_pmt(AVFormatContext *s, MpegTSService *service) case AV_CODEC_ID_TRUEHD: stream_type = STREAM_TYPE_AUDIO_TRUEHD; break; +case AV_CODEC_ID_OPUS: +stream_type = STREAM_TYPE_PRIVATE_DATA; +break; default: stream_type = STREAM_TYPE_PRIVATE_DATA; break; @@ -340,6 +346,82 @@ static int mpegts_write_pmt(AVFormatContext *s, MpegTSService *service) *q++ = 'S'; *q++ = 'D'; } +if (st->codec->codec_id==AV_CODEC_ID_OPUS) { +/* 6 bytes registration descriptor, 4 bytes Opus audio descriptor */ +if (q - data > SECTION_LENGTH - 6 - 4) { +err = 1; +break; +} + +*q++ = 0x05; /* MPEG-2 registration descriptor*/ +*q++ = 4; +*q++ = 'O'; +*q++ = 'p'; +*q++ = 'u'; +*q++ = 's'; + +*q++ = 0x7f; /* DVB extension descriptor */ +*q++ = 2; +*q++ = 0x80; + +if (st->codec->extradata && st->codec->extradata_size >= 19) { +if (st->codec->extradata[18] == 0 && st->codec->channels <= 2) { +/* RTP mapping family */ +*q++ = st->codec->channels; +} else if (st->codec->extradata[18] == 1 && st->codec->channels <= 8 && + st->codec->extradata_size >= 22 + st->codec->channels) { +static const uint8_t coupled_stream_counts[9] = { +1, 0, 1, 1, 2, 2, 2, 3, 3 +}; +static const uint8_t channel_map_a[8][8] = { +{0}, +{0, 1}, +{0, 2, 1}, +{0, 1, 2, 3}, +{0, 4, 1, 2, 3}, +{0, 4, 1, 2, 3, 5}, +{0, 4, 1, 2, 3, 5, 6}, +{0, 6, 1, 2, 3, 4, 5, 7}, +}; +static const uint8_t channel_map_b[8][8] = { +{0}, +{0, 1}, +{0, 1, 2}, +{0, 1, 2, 3}, +{0, 1, 2, 3, 4}, +{0, 1, 2, 3, 4, 5}, +{0, 1, 2, 3, 4, 5, 6}, +{0, 1, 2, 3, 4, 5, 6, 7}, +}; +/* Vorbis mapping family */ + +if (st->codec->extradata[19] == st->codec->channels - coupled_stream_counts[st->codec->channels] && +st->codec->extradata[20] == coupled_stream_counts[st->codec->channels] && +memcmp(&st->codec->extradata[21], channel_map_a[st->codec->channels], st->codec->channels) == 0) { +*q++ = st->codec->channels; +} else if (st->codec->channels >= 2 && st->codec->extradata[19] == st->codec->channels && + st->codec->extradata[20] == 0 && + memcmp(&st->codec->extradata[21], channel_map_b[st->codec->channels], st->codec->channels) == 0) { +*q++ = st->codec->channels | 0x80; +} else { +/* Unsupported, could write an extended descriptor here */ +av_log(s, AV_LOG_ERROR, "Unsupported Opus Vorbis-style channel mapping"); +*q++ = 0xff; +} +} else { +/* Unsupported */ +av_log(s, AV_LOG_ERROR, "Unsupported Opus channel mapping for family %d", st->codec->extradata[18]); +*q++ = 0xff; +} +} else if (st->codec->channels <= 2) { +/* Assume RTP mapping family */ +*q++ = st->codec->channels; +} else { +/* Unsupported */ +av_log(s, AV_LOG_ERROR, "Unsupported Opus cha
[FFmpeg-devel] [PATCHv2 2/2] mpegtsenc: Implement writing of Opus trim_start/trim_end control values
From: Sebastian Dröge Signed-off-by: Sebastian Dröge --- libavformat/mpegtsenc.c | 47 --- 1 file changed, 44 insertions(+), 3 deletions(-) diff --git a/libavformat/mpegtsenc.c b/libavformat/mpegtsenc.c index 0674064..51d1a0b 100644 --- a/libavformat/mpegtsenc.c +++ b/libavformat/mpegtsenc.c @@ -230,6 +230,8 @@ typedef struct MpegTSWriteStream { /* For Opus */ int opus_queued_samples; +int opus_pending_trim_start; +int opus_pending_trim_end; } MpegTSWriteStream; static void mpegts_write_pat(AVFormatContext *s) @@ -825,6 +827,9 @@ static int mpegts_write_header(AVFormatContext *s) if (ret < 0) goto fail; } +if (st->codec->codec_id == AV_CODEC_ID_OPUS) { +ts_st->opus_pending_trim_start = st->codec->initial_padding * 48000 / st->codec->sample_rate; +} } av_freep(&pids); @@ -1506,24 +1511,47 @@ static int mpegts_write_packet_internal(AVFormatContext *s, AVPacket *pkt) if (ret < 0) return ret; } else if (st->codec->codec_id == AV_CODEC_ID_OPUS) { +uint8_t *side_data; +int side_data_size; + if (pkt->size < 2) { av_log(s, AV_LOG_ERROR, "Opus packet too short\n"); return AVERROR_INVALIDDATA; } +side_data = av_packet_get_side_data(pkt, +AV_PKT_DATA_SKIP_SAMPLES, +&side_data_size); + +if (side_data && side_data_size >= 10) { +uint32_t discard_padding = AV_RL32(side_data + 4) * 48000 / st->codec->sample_rate; +ts_st->opus_pending_trim_end += discard_padding; +} + /* Add Opus control header */ if ((AV_RB16(pkt->data) >> 5) != 0x3ff) { int i, n; +int ctrl_header_size; +int trim_start = 0, trim_end = 0; opus_samples = opus_get_packet_samples(s, pkt); -data = av_malloc(pkt->size + 2 + pkt->size / 255 + 1); +ctrl_header_size = pkt->size + 2 + pkt->size / 255 + 1; +if (ts_st->opus_pending_trim_start) + ctrl_header_size += 2; +if (ts_st->opus_pending_trim_end) + ctrl_header_size += 2; + +data = av_malloc(ctrl_header_size); if (!data) return AVERROR(ENOMEM); -/* TODO: Write trim if needed */ data[0] = 0x7f; data[1] = 0xe0; +if (ts_st->opus_pending_trim_start) +data[1] |= 0x10; +if (ts_st->opus_pending_trim_end) +data[1] |= 0x08; n = pkt->size; i = 2; @@ -1535,9 +1563,22 @@ static int mpegts_write_packet_internal(AVFormatContext *s, AVPacket *pkt) av_assert0(2 + pkt->size / 255 + 1 == i); +if (ts_st->opus_pending_trim_start) { +trim_start = FFMIN(ts_st->opus_pending_trim_start, opus_samples); +AV_WB16(data + i, trim_start); +i += 2; +ts_st->opus_pending_trim_start -= trim_start; +} +if (ts_st->opus_pending_trim_end) { +trim_end = FFMIN(ts_st->opus_pending_trim_end, opus_samples - trim_start); +AV_WB16(data + i, trim_end); +i += 2; +ts_st->opus_pending_trim_end -= trim_end; +} + memcpy(data + i, pkt->data, pkt->size); buf = data; -size= pkt->size + 2 + pkt->size / 255 + 1; +size= ctrl_header_size; } else { /* TODO: Can we get TS formatted data here? If so we will * need to count the samples of that too! */ -- 2.6.2 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/3] avutil/eval: minor typo
On Sun, Nov 01, 2015 at 11:57:53AM -0500, Ganesh Ajjanagadde wrote: > Signed-off-by: Ganesh Ajjanagadde > --- > libavutil/eval.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavutil/eval.h b/libavutil/eval.h > index 6159b0f..dacd22b 100644 > --- a/libavutil/eval.h > +++ b/libavutil/eval.h > @@ -102,7 +102,7 @@ void av_expr_free(AVExpr *e); > * @param numstr a string representing a number, may contain one of > * the International System number postfixes, for example 'K', 'M', > * 'G'. If 'i' is appended after the postfix, powers of 2 are used > - * instead of powers of 10. The 'B' postfix multiplies the value for > + * instead of powers of 10. The 'B' postfix multiplies the value by ok [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Dictatorship naturally arises out of democracy, and the most aggravated form of tyranny and slavery out of the most extreme liberty. -- Plato signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCHv2 1/4] avfilter/all: check and propagate the return code of av_expr_parse_and_eval
On Sun, Nov 01, 2015 at 04:16:18PM -0500, Ganesh Ajjanagadde wrote: > On Sun, Nov 1, 2015 at 3:45 PM, Nicolas George wrote: > > Le primidi 11 brumaire, an CCXXIV, Ganesh Ajjanagadde a écrit : > >> This function can return ENOMEM that needs to be propagated. > >> > >> Signed-off-by: Ganesh Ajjanagadde > >> --- > >> libavfilter/vf_pad.c| 12 +++- > >> libavfilter/vf_rotate.c | 5 +++-- > >> libavfilter/vf_scale.c | 5 +++-- > >> libavfilter/vf_zscale.c | 5 +++-- > >> 4 files changed, 16 insertions(+), 11 deletions(-) > >> > >> diff --git a/libavfilter/vf_pad.c b/libavfilter/vf_pad.c > >> index 63dc6a8..a40f5fa 100644 > >> --- a/libavfilter/vf_pad.c > >> +++ b/libavfilter/vf_pad.c > >> @@ -114,9 +114,10 @@ static int config_input(AVFilterLink *inlink) > >> var_values[VAR_VSUB] = 1 << s->draw.vsub_max; > >> > >> /* evaluate width and height */ > >> -av_expr_parse_and_eval(&res, (expr = s->w_expr), > >> +if ((ret = av_expr_parse_and_eval(&res, (expr = s->w_expr), > >> var_names, var_values, > >> - NULL, NULL, NULL, NULL, NULL, 0, ctx); > >> + NULL, NULL, NULL, NULL, NULL, 0, ctx)) == > >> AVERROR(ENOMEM)) > >> +goto eval_fail; > > > > I am quite unhappy about this, it is cluttering the code for no good reason > > and makes the test fragile. > > The fragility is something I don't like either, suggestions? less fragile could be ret < 0 && ret != the error code generated by NaN i dont know what to do about the clutter or if this approuch makes sense or if just droping the return warning for av_expr_parse_and_eval would be better ... [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I know you won't believe me, but the highest form of Human Excellence is to question oneself and others. -- Socrates signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCHv2 1/4] avfilter/all: check and propagate the return code of av_expr_parse_and_eval
Le primidi 11 brumaire, an CCXXIV, Michael Niedermayer a écrit : > less fragile could be ret < 0 && ret != the error code generated by NaN > i dont know what to do about the clutter or if this approuch makes > sense or if just droping the return warning for av_expr_parse_and_eval > would be better ... The current code does not crash when errors happens (and ENOMEM doest not happen for this kind of small allocation anyway), my advice is that it is better left as is. Regards, -- Nicolas George signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/2] avutil/common: add FFDIFFSIGN macro
On Sun, Nov 01, 2015 at 12:19:47PM -0500, Ganesh Ajjanagadde wrote: > This is of use for defining comparator callbacks. Common approaches like > return x-y are not safe due to the risks of overflow. > Furthermore, the (x > y) - (x < y) trick is optimized to branchless > code. > This also documents this macro accordingly. > > Signed-off-by: Ganesh Ajjanagadde > --- > libavutil/common.h | 11 +++ > 1 file changed, 11 insertions(+) > > diff --git a/libavutil/common.h b/libavutil/common.h > index 6594f7d..6f0f582 100644 > --- a/libavutil/common.h > +++ b/libavutil/common.h > @@ -76,6 +76,17 @@ > */ > #define FFNABS(a) ((a) <= 0 ? (a) : (-(a))) > > +/** > + * Comparator. > + * For two numerical expressions x and y, gives 1 if x > y, -1 if x < y, and > 0 > + * if x == y. This is useful for instance in a qsort comparator callback. > + * Furthermore, compilers are able to optimize this to branchless code, and > + * there is no risk of overflow with signed types. > + * As with many macros, this evaluates its argument multiple times, it thus > + * must not have a side-effect. > + */ > +#define FFDIFFSIGN(x,y) (((x)>(y)) - ((x)<(y))) LGTM (maybe wait a bit before pushing so others can comment too, people may find the text too verbose or something else) [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB It is dangerous to be right in matters on which the established authorities are wrong. -- Voltaire signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 3/5] concatdec: move duration calculating code to open_file
On Fri, 30 Oct 2015, Nicolas George wrote: Le quartidi 4 brumaire, an CCXXIV, Marton Balint a écrit : Hmm. I need this computed here for the next patch. Maybe we could calcualate the duration here and then update it in open_next_file as well? That is probably possible somehow but a bit tricky. Can you explain why you need the duration at this point? Because I add it to the metadata store here in the next patch, which metadata will be set for every file packet, so calculating it at the end of file is not an option. I will rework the patch series and drop this change, so only the metadata will be affected by the unknown duration, and not the file->duration which is used to calculate the packet timestamps. Regards, Marton ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 3/5] concatdec: move duration calculating code to open_file
Le primidi 11 brumaire, an CCXXIV, Marton Balint a écrit : > Because I add it to the metadata store here in the next patch, which > metadata will be set for every file packet, so calculating it at the end of > file is not an option. I see. But do you really need the duration metadata when it is not authoritative? Regards, -- Nicolas George signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCHv2 1/4] avfilter/all: check and propagate the return code of av_expr_parse_and_eval
On Sun, Nov 1, 2015 at 5:46 PM, Nicolas George wrote: > Le primidi 11 brumaire, an CCXXIV, Michael Niedermayer a écrit : >> less fragile could be ret < 0 && ret != the error code generated by NaN >> i dont know what to do about the clutter or if this approuch makes >> sense or if just droping the return warning for av_expr_parse_and_eval >> would be better ... > > The current code does not crash when errors happens (and ENOMEM doest not > happen for this kind of small allocation anyway), my advice is that it is > better left as is. Can't users give arbitrarily long arithmetic expressions? The expressions seem to me to be essentially untrusted data. I agree in normal usage, strings are small. I won't press this further if it is still a nack due to your point about the lack of e.g dereferencing here, or in general usage of av_expr_parse_and_eval. Thanks all for reviews. > > Regards, > > -- > Nicolas George > > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/2] avutil/common: add av_clipd64
Hi, On Sun, Nov 1, 2015 at 1:03 PM, Ganesh Ajjanagadde wrote: > The rationale for this function is reflected in the documentation for > it, and is copied here: > > Clip a double value into the long long amin-amax range. > This function is needed because conversion of floating point to integers > when > it does not fit in the integer's representation does not necessarily > saturate > correctly (usually converted to a cvttsd2si on x86) which saturates numbers > > INT64_MAX to INT64_MIN. The standard marks such conversions as undefined > behavior, allowing this sort of mathematically bogus conversions. This > provides > a safe alternative that is slower obviously but assures safety and better > mathematical behavior. > @param a value to clip > @param amin minimum value of the clip range > @param amax maximum value of the clip range > @return clipped value > > Note that a priori if one can guarantee from the calling side that the > double is in range, it is safe to simply do an explicit/implicit cast, > and that will be far faster. However, otherwise, this function should be > used. > > > -- > As a general remark, Clang/GCC has a -Wfloat-conversion so that at least > implicit conversions can be found. This helped me in auditing the > codebase. In order to reduce noise while testing, an explicit cast on the > return > was placed. I can remove it if people prefer, though I like the cast as > it makes the intent of possible narrowing explicit. > > Signed-off-by: Ganesh Ajjanagadde > --- > libavutil/common.h | 31 +++ > 1 file changed, 31 insertions(+) > > diff --git a/libavutil/common.h b/libavutil/common.h > index 6f0f582..e778dd2 100644 > --- a/libavutil/common.h > +++ b/libavutil/common.h > @@ -298,6 +298,34 @@ static av_always_inline av_const double > av_clipd_c(double a, double amin, double > else return a; > } > > +/** > + * Clip a double value into the long long amin-amax range. > + * This function is needed because conversion of floating point to > integers when > + * it does not fit in the integer's representation does not necessarily > saturate > + * correctly (usually converted to a cvttsd2si on x86) which saturates > numbers > + * > INT64_MAX to INT64_MIN. The standard marks such conversions as > undefined > + * behavior, allowing this sort of mathematically bogus conversions. This > provides > + * a safe alternative that is slower obviously but assures safety and > better > + * mathematical behavior. > + * @param a value to clip > + * @param amin minimum value of the clip range > + * @param amax maximum value of the clip range > + * @return clipped value > + */ > +static av_always_inline av_const int64_t av_clipd64_c(double a, int64_t > amin, int64_t amax) > +{ > +#if defined(HAVE_AV_CONFIG_H) && defined(ASSERT_LEVEL) && ASSERT_LEVEL >= > 2 > +if (amin > amax) abort(); > +#endif > +// INT64_MAX+1,INT64_MIN are exactly representable as IEEE doubles > +if (a >= 9223372036854775808.0) > +return INT64_MAX; > +if (a <= INT64_MIN) > +return INT64_MIN; > +// Finally safe to call av_clipd_c > +return (int64_t)av_clipd_c(a, amin, amax); > +} Is this different from llrint? Ronald ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 0/2] first steps to resolving float to int undefined behavior
Hi, On Sun, Nov 1, 2015 at 1:03 PM, Ganesh Ajjanagadde wrote: > Floating point to integer conversion is well defined when the float lies > within > the representation bounds of the integer after discarding the fractional > part. > However, in other cases, unfortunately the standard leaves it undefined. > In particular, assuming that it saturates in a sane way is a dangerous > assumption. > > In light of recent events, I would not have bothered if this was a merely > theoretical > issue, and that common environments saturate correctly. Sadly, x86 (for > example) > converts casts to a cvttsd2si instruction which saturates numbers > > INT64_MAX to > INT64_MIN. This is mathematically completely bogus. > http://blog.frama-c.com/index.php?post/2013/10/09/Overflow-float-integer > gives > a nice overview of the issue. > > 1/2 adds an av_clipd64 API for this purpose to clip a float to an integral > range. > Obviously it will be slower than a cvttsd2si, but there is no option if > one wants > safe and well-defined behavior. Of course, if one knows a priori that a > float > lives in the integral type's range, then there is no issue. Safe speedups > are > entirely possible, but API should be finalized first IMHO. > Most common anticipated usages are clipping to [INT64_MIN, INT64_MAX] or > [INT_MIN, INT_MAX]. > I have given some thought as to whether a separate av_clipd32 API (for > example) > is necessary. It seems to me to not be the case, since an IEEE-754 double > is > guaranteed to represent exactly integers up to ~ 2^53. > Similarly, av_clipf64 and the like also seem unnecessary, since a double > is guaranteed > to represent all the values a float does. Such an API may be useful for > performance > though; I do not know how/what float to double conversions entail and at > the > moment ignore such complications. > > 1/2 also accordingly documents av_clipd64. So, is this a bug in llrint, or is this a failure to use llrint, or is this different from llrint? It sounds to me that llrint should be used, not our own alternative. Ronald ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/2] avutil/common: add av_clipd64
On Sun, Nov 1, 2015 at 6:09 PM, Ronald S. Bultje wrote: > Hi, > > On Sun, Nov 1, 2015 at 1:03 PM, Ganesh Ajjanagadde > wrote: >> >> The rationale for this function is reflected in the documentation for >> it, and is copied here: >> >> Clip a double value into the long long amin-amax range. >> This function is needed because conversion of floating point to integers >> when >> it does not fit in the integer's representation does not necessarily >> saturate >> correctly (usually converted to a cvttsd2si on x86) which saturates >> numbers >> > INT64_MAX to INT64_MIN. The standard marks such conversions as undefined >> behavior, allowing this sort of mathematically bogus conversions. This >> provides >> a safe alternative that is slower obviously but assures safety and better >> mathematical behavior. >> @param a value to clip >> @param amin minimum value of the clip range >> @param amax maximum value of the clip range >> @return clipped value >> >> Note that a priori if one can guarantee from the calling side that the >> double is in range, it is safe to simply do an explicit/implicit cast, >> and that will be far faster. However, otherwise, this function should be >> used. >> >> >> -- >> As a general remark, Clang/GCC has a -Wfloat-conversion so that at least >> implicit conversions can be found. This helped me in auditing the >> codebase. In order to reduce noise while testing, an explicit cast on the >> return >> was placed. I can remove it if people prefer, though I like the cast as >> it makes the intent of possible narrowing explicit. >> >> Signed-off-by: Ganesh Ajjanagadde >> --- >> libavutil/common.h | 31 +++ >> 1 file changed, 31 insertions(+) >> >> diff --git a/libavutil/common.h b/libavutil/common.h >> index 6f0f582..e778dd2 100644 >> --- a/libavutil/common.h >> +++ b/libavutil/common.h >> @@ -298,6 +298,34 @@ static av_always_inline av_const double >> av_clipd_c(double a, double amin, double >> else return a; >> } >> >> +/** >> + * Clip a double value into the long long amin-amax range. >> + * This function is needed because conversion of floating point to >> integers when >> + * it does not fit in the integer's representation does not necessarily >> saturate >> + * correctly (usually converted to a cvttsd2si on x86) which saturates >> numbers >> + * > INT64_MAX to INT64_MIN. The standard marks such conversions as >> undefined >> + * behavior, allowing this sort of mathematically bogus conversions. This >> provides >> + * a safe alternative that is slower obviously but assures safety and >> better >> + * mathematical behavior. >> + * @param a value to clip >> + * @param amin minimum value of the clip range >> + * @param amax maximum value of the clip range >> + * @return clipped value >> + */ >> +static av_always_inline av_const int64_t av_clipd64_c(double a, int64_t >> amin, int64_t amax) >> +{ >> +#if defined(HAVE_AV_CONFIG_H) && defined(ASSERT_LEVEL) && ASSERT_LEVEL >= >> 2 >> +if (amin > amax) abort(); >> +#endif >> +// INT64_MAX+1,INT64_MIN are exactly representable as IEEE doubles >> +if (a >= 9223372036854775808.0) >> +return INT64_MAX; >> +if (a <= INT64_MIN) >> +return INT64_MIN; >> +// Finally safe to call av_clipd_c >> +return (int64_t)av_clipd_c(a, amin, amax); >> +} > > > Is this different from llrint? Very much so, man llrint: "If x is a NaN or an infinity, or the rounded value is too large to be stored in a long (long long in the case of the ll* functions), then a domain error occurs, and the return value is unspecified." If you are curious, just do an llrint on e.g INT64_MAX + 100, I am pretty sure you won't be pleased with the result :). This is exactly the motivation of the patch - it provides a safe, sane way to get a float to an integer range when it may not fit. > > Ronald ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 0/2] first steps to resolving float to int undefined behavior
On Sun, Nov 1, 2015 at 6:12 PM, Ronald S. Bultje wrote: > Hi, > > On Sun, Nov 1, 2015 at 1:03 PM, Ganesh Ajjanagadde > wrote: >> >> Floating point to integer conversion is well defined when the float lies >> within >> the representation bounds of the integer after discarding the fractional >> part. >> However, in other cases, unfortunately the standard leaves it undefined. >> In particular, assuming that it saturates in a sane way is a dangerous >> assumption. >> >> In light of recent events, I would not have bothered if this was a merely >> theoretical >> issue, and that common environments saturate correctly. Sadly, x86 (for >> example) >> converts casts to a cvttsd2si instruction which saturates numbers > >> INT64_MAX to >> INT64_MIN. This is mathematically completely bogus. >> http://blog.frama-c.com/index.php?post/2013/10/09/Overflow-float-integer >> gives >> a nice overview of the issue. >> >> 1/2 adds an av_clipd64 API for this purpose to clip a float to an integral >> range. >> Obviously it will be slower than a cvttsd2si, but there is no option if >> one wants >> safe and well-defined behavior. Of course, if one knows a priori that a >> float >> lives in the integral type's range, then there is no issue. Safe speedups >> are >> entirely possible, but API should be finalized first IMHO. >> Most common anticipated usages are clipping to [INT64_MIN, INT64_MAX] or >> [INT_MIN, INT_MAX]. >> I have given some thought as to whether a separate av_clipd32 API (for >> example) >> is necessary. It seems to me to not be the case, since an IEEE-754 double >> is >> guaranteed to represent exactly integers up to ~ 2^53. >> Similarly, av_clipf64 and the like also seem unnecessary, since a double >> is guaranteed >> to represent all the values a float does. Such an API may be useful for >> performance >> though; I do not know how/what float to double conversions entail and at >> the >> moment ignore such complications. >> >> 1/2 also accordingly documents av_clipd64. > > > So, is this a bug in llrint, or is this a failure to use llrint, or is this > different from llrint? It sounds to me that llrint should be used, not our > own alternative. Not a bug, just a standard "undefined behavior" cop-out from the standards committee. We need to roll our own: all standard functions cop out when it does not fit in the integer range, and Intel (and others) have wonderfully exploited the cop-out: see e.g the link I gave and my other reply. > > Ronald ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 1/4] fate: add concat demuxer tests
Signed-off-by: Marton Balint --- tests/Makefile |1 + tests/extended.ffconcat| 114 + tests/fate-run.sh | 20 + tests/fate/concatdec.mak | 21 + tests/ref/fate/concat-demuxer-extended-lavf-mxf|1 + .../ref/fate/concat-demuxer-extended-lavf-mxf_d10 |1 + tests/ref/fate/concat-demuxer-simple1-lavf-mxf | 1713 ++ tests/ref/fate/concat-demuxer-simple1-lavf-mxf_d10 | 1193 ++ tests/ref/fate/concat-demuxer-simple2-lavf-ts | 2326 tests/simple1.ffconcat | 12 + tests/simple2.ffconcat | 19 + 11 files changed, 5421 insertions(+) create mode 100644 tests/extended.ffconcat create mode 100644 tests/fate/concatdec.mak create mode 100644 tests/ref/fate/concat-demuxer-extended-lavf-mxf create mode 100644 tests/ref/fate/concat-demuxer-extended-lavf-mxf_d10 create mode 100644 tests/ref/fate/concat-demuxer-simple1-lavf-mxf create mode 100644 tests/ref/fate/concat-demuxer-simple1-lavf-mxf_d10 create mode 100644 tests/ref/fate/concat-demuxer-simple2-lavf-ts create mode 100644 tests/simple1.ffconcat create mode 100644 tests/simple2.ffconcat diff --git a/tests/Makefile b/tests/Makefile index 7ee4a46..62544d0 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -113,6 +113,7 @@ include $(SRC_PATH)/tests/fate/audio.mak include $(SRC_PATH)/tests/fate/bmp.mak include $(SRC_PATH)/tests/fate/cdxl.mak include $(SRC_PATH)/tests/fate/checkasm.mak +include $(SRC_PATH)/tests/fate/concatdec.mak include $(SRC_PATH)/tests/fate/cover-art.mak include $(SRC_PATH)/tests/fate/demux.mak include $(SRC_PATH)/tests/fate/dfa.mak diff --git a/tests/extended.ffconcat b/tests/extended.ffconcat new file mode 100644 index 000..7359113 --- /dev/null +++ b/tests/extended.ffconcat @@ -0,0 +1,114 @@ +ffconcat version 1.0 + +file %SRCFILE% + +file %SRCFILE% +duration 1 +file_packet_metadata dummy=1 + +file %SRCFILE% +inpoint 00:00.00 +outpoint 00:00.04 + +file %SRCFILE% +inpoint 00:00.04 +outpoint 00:00.08 + +file %SRCFILE% +inpoint 00:00.08 +outpoint 00:00.12 + +file %SRCFILE% +inpoint 00:00.12 +outpoint 00:00.16 + +file %SRCFILE% +inpoint 00:00.16 +outpoint 00:00.20 + +file %SRCFILE% +inpoint 00:00.20 +outpoint 00:00.24 + +file %SRCFILE% +inpoint 00:00.24 +outpoint 00:00.28 + +file %SRCFILE% +inpoint 00:00.28 +outpoint 00:00.32 + +file %SRCFILE% +inpoint 00:00.32 +outpoint 00:00.36 + +file %SRCFILE% +inpoint 00:00.36 +outpoint 00:00.40 + +file %SRCFILE% +inpoint 00:00.40 +outpoint 00:00.44 + +file %SRCFILE% +inpoint 00:00.44 +outpoint 00:00.48 + +file %SRCFILE% +inpoint 00:00.48 +outpoint 00:00.52 + +file %SRCFILE% +inpoint 00:00.52 +outpoint 00:00.56 + +file %SRCFILE% +inpoint 00:00.56 +outpoint 00:00.60 + +file %SRCFILE% +inpoint 00:00.60 +outpoint 00:00.64 + +file %SRCFILE% +inpoint 00:00.64 +outpoint 00:00.68 + +file %SRCFILE% +inpoint 00:00.68 +outpoint 00:00.72 + +file %SRCFILE% +inpoint 00:00.72 +outpoint 00:00.76 + +file %SRCFILE% +inpoint 00:00.76 +outpoint 00:00.80 + +file %SRCFILE% +inpoint 00:00.80 +outpoint 00:00.84 + +file %SRCFILE% +inpoint 00:00.84 +outpoint 00:00.88 + +file %SRCFILE% +inpoint 00:00.88 +outpoint 00:00.92 + +file %SRCFILE% +inpoint 00:00.92 +outpoint 00:00.96 + +file %SRCFILE% +inpoint 00:00.96 +outpoint 00:01.00 + +file %SRCFILE% +outpoint 00:00.40 + +file %SRCFILE% +inpoint 00:00.40 + diff --git a/tests/fate-run.sh b/tests/fate-run.sh index a3938dc..6c350d0 100755 --- a/tests/fate-run.sh +++ b/tests/fate-run.sh @@ -249,6 +249,26 @@ gapless(){ do_md5sum $decfile3 } +concat(){ +template=tests/$1 +sample=$(target_path $2) +mode=$3 +extra_args=$4 + +concatfile="${outdir}/${test}.ffconcat" +packetfile="${outdir}/${test}.ffprobe" +cleanfiles="$concatfile $packetfile" + +awk "{gsub(/%SRCFILE%/, \"$sample\"); print}" $template > $concatfile + +if [ "$mode" == "md5" ]; then + run ffprobe${PROGSUF} -show_streams -show_packets -v 0 -fflags keepside -safe 0 $extra_args $concatfile > $packetfile + do_md5sum $packetfile +else + run ffprobe${PROGSUF} -show_streams -show_packets -v 0 -fflags keepside -safe 0 $extra_args $concatfile +fi +} + mkdir -p "$outdir" # Disable globbing: command arguments may contain globbing characters and diff --git a/tests/fate/concatdec.mak b/tests/fate/concatdec.mak new file mode 100644 index 000..344cbc2 --- /dev/null +++ b/tests/fate/concatdec.mak @@ -0,0 +1,21 @@ +FATE_CONCAT_DEMUXER_SIMPLE1_LAVF-$(call ENCDEC2, MPEG2VIDEO, PCM_S16LE, MXF) += mxf +FATE_CONCAT_DEMUXER_SIMPLE1_LAVF-$(call ENCDEC2, MPEG2VIDEO, PCM_S16LE, MXF) += mxf_d10 +
[FFmpeg-devel] [PATCH 2/4] concatdec: factorize duration calculating code to get_duration function
Signed-off-by: Marton Balint --- libavformat/concatdec.c | 19 --- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/libavformat/concatdec.c b/libavformat/concatdec.c index 0180a7e..15094bf 100644 --- a/libavformat/concatdec.c +++ b/libavformat/concatdec.c @@ -285,6 +285,16 @@ static int match_streams(AVFormatContext *avf) return 0; } +static int64_t get_duration(ConcatFile *file, AVFormatContext *avf) +{ +int64_t duration = avf->duration; +if (file->inpoint != AV_NOPTS_VALUE) +duration -= (file->inpoint - file->file_start_time); +if (file->outpoint != AV_NOPTS_VALUE) +duration -= avf->duration - (file->outpoint - file->file_start_time); +return duration; +} + static int open_file(AVFormatContext *avf, unsigned fileno) { ConcatContext *cat = avf->priv_data; @@ -469,13 +479,8 @@ static int open_next_file(AVFormatContext *avf) ConcatContext *cat = avf->priv_data; unsigned fileno = cat->cur_file - cat->files; -if (cat->cur_file->duration == AV_NOPTS_VALUE) { -cat->cur_file->duration = cat->avf->duration; -if (cat->cur_file->inpoint != AV_NOPTS_VALUE) -cat->cur_file->duration -= (cat->cur_file->inpoint - cat->cur_file->file_start_time); -if (cat->cur_file->outpoint != AV_NOPTS_VALUE) -cat->cur_file->duration -= cat->avf->duration - (cat->cur_file->outpoint - cat->cur_file->file_start_time); -} +if (cat->cur_file->duration == AV_NOPTS_VALUE) +cat->cur_file->duration = get_duration(cat->cur_file, cat->avf); if (++fileno >= cat->nb_files) { cat->eof = 1; -- 2.1.4 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 3/4] concatdec: add option for adding segment start time and duration metadata
Signed-off-by: Marton Balint --- doc/demuxers.texi | 8 libavformat/concatdec.c | 12 2 files changed, 20 insertions(+) diff --git a/doc/demuxers.texi b/doc/demuxers.texi index 34bfc9b..41d3722 100644 --- a/doc/demuxers.texi +++ b/doc/demuxers.texi @@ -204,6 +204,14 @@ Currently, the only conversion is adding the h264_mp4toannexb bitstream filter to H.264 streams in MP4 format. This is necessary in particular if there are resolution changes. +@item segment_time_metadata +If set to 1, every packet will contain the @var{lavf.concat.start_time} and the +@var{lavf.concat.duration} packet metadata values which are the start_time and +the duration of the respective file segments in the concatenated output +expressed in microseconds. The duration is only estimated in some files (e.g. +MP3 without extra headers) +The default is 0. + @end table @section flv diff --git a/libavformat/concatdec.c b/libavformat/concatdec.c index 15094bf..603f1b7 100644 --- a/libavformat/concatdec.c +++ b/libavformat/concatdec.c @@ -62,6 +62,7 @@ typedef struct { int eof; ConcatMatchMode stream_match_mode; unsigned auto_convert; +int segment_time_metadata; } ConcatContext; static int concat_probe(AVProbeData *probe) @@ -326,6 +327,15 @@ static int open_file(AVFormatContext *avf, unsigned fileno) cat->files[fileno - 1].duration; file->file_start_time = (cat->avf->start_time == AV_NOPTS_VALUE) ? 0 : cat->avf->start_time; file->file_inpoint = (file->inpoint == AV_NOPTS_VALUE) ? file->file_start_time : file->inpoint; + +if (cat->segment_time_metadata) { +int64_t duration = file->duration; +if (duration == AV_NOPTS_VALUE) +duration = get_duration(file, cat->avf); +av_dict_set_int(&file->metadata, "lavf.concatdec.start_time", file->start_time, 0); +av_dict_set_int(&file->metadata, "lavf.concatdec.duration", duration, 0); +} + if ((ret = match_streams(avf)) < 0) return ret; if (file->inpoint != AV_NOPTS_VALUE) { @@ -709,6 +719,8 @@ static const AVOption options[] = { OFFSET(safe), AV_OPT_TYPE_INT, {.i64 = -1}, -1, 1, DEC }, { "auto_convert", "automatically convert bitstream format", OFFSET(auto_convert), AV_OPT_TYPE_INT, {.i64 = 1}, 0, 1, DEC }, +{ "segment_time_metadata", "output file segment start time and duration as packet metadata", + OFFSET(segment_time_metadata), AV_OPT_TYPE_INT, {.i64 = 0}, 0, 1, DEC }, { NULL } }; -- 2.1.4 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 4/4] lavfi/select: add support for concatdec_select option
This option can be used to select useful frames from an ffconcat file which is using inpoints and outpoints but where the source files are not intra frame only. Signed-off-by: Marton Balint --- doc/filters.texi | 16 libavfilter/f_select.c | 20 2 files changed, 36 insertions(+) diff --git a/doc/filters.texi b/doc/filters.texi index 15ea77a..805f121 100644 --- a/doc/filters.texi +++ b/doc/filters.texi @@ -13176,6 +13176,15 @@ value between 0 and 1 to indicate a new scene; a low value reflects a low probability for the current frame to introduce a new scene, while a higher value means the current frame is more likely to be one (see the example below) +@item concatdec_select +The concat demuxer can set the @var{lavf.concat.start_time} and the +@var{lavf.concat.duration} packet metadata values which are also present in the +decoded frames. + +The @var{concatdec_select} variable is -1 if the frame pts is at least +start_time but less than start_time + duration, 0 otherwise, and NaN if the +mentioned metadata entires are not present. + @end table The default value of the select expression is "1". @@ -13250,6 +13259,13 @@ Send even and odd frames to separate outputs, and compose them: @example select=n=2:e='mod(n, 2)+1' [odd][even]; [odd] pad=h=2*ih [tmp]; [tmp][even] overlay=y=h @end example + +@item +Select useful frames from an ffconcat file which is using inpoints and +outpoints but where the source files are not intra frame only. +@example +ffmpeg -copyts -vsync 0 -segment_time_metadata 1 -i input.ffconcat -vf select=concatdec_select -af aselect=concatdec_select output.avi +@end example @end itemize @section selectivecolor diff --git a/libavfilter/f_select.c b/libavfilter/f_select.c index 2b926e1..d5f1470 100644 --- a/libavfilter/f_select.c +++ b/libavfilter/f_select.c @@ -82,6 +82,8 @@ static const char *const var_names[] = { "scene", +"concatdec_select", ///< frame usefulness based on pts and frame metadata originating from the concat demuxer + NULL }; @@ -132,6 +134,8 @@ enum var_name { VAR_SCENE, +VAR_CONCATDEC_SELECT, + VAR_VARS_NB }; @@ -278,6 +282,21 @@ static double get_scene_score(AVFilterContext *ctx, AVFrame *frame) return ret; } +static double get_concatdec_select(AVFrame *frame, int64_t pts) +{ +AVDictionary *metadata = av_frame_get_metadata(frame); +AVDictionaryEntry *e1 = av_dict_get(metadata, "lavf.concatdec.start_time", NULL, 0); +AVDictionaryEntry *e2 = av_dict_get(metadata, "lavf.concatdec.duration", NULL, 0); +if (e1 && e1->value && e2 && e2->value) { +int64_t start_time = strtoll(e1->value, NULL, 10); +int64_t duration = strtoll(e2->value, NULL, 10); +if (pts >= start_time && pts < start_time + duration) +return -1; +return 0; +} +return NAN; +} + #define D2TS(d) (isnan(d) ? AV_NOPTS_VALUE : (int64_t)(d)) #define TS2D(ts) ((ts) == AV_NOPTS_VALUE ? NAN : (double)(ts)) @@ -297,6 +316,7 @@ static void select_frame(AVFilterContext *ctx, AVFrame *frame) select->var_values[VAR_T ] = TS2D(frame->pts) * av_q2d(inlink->time_base); select->var_values[VAR_POS] = av_frame_get_pkt_pos(frame) == -1 ? NAN : av_frame_get_pkt_pos(frame); select->var_values[VAR_KEY] = frame->key_frame; +select->var_values[VAR_CONCATDEC_SELECT] = get_concatdec_select(frame, av_rescale_q(frame->pts, inlink->time_base, AV_TIME_BASE_Q)); switch (inlink->type) { case AVMEDIA_TYPE_AUDIO: -- 2.1.4 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 0/2] first steps to resolving float to int undefined behavior
Hi, On Sun, Nov 1, 2015 at 6:15 PM, Ganesh Ajjanagadde wrote: > On Sun, Nov 1, 2015 at 6:12 PM, Ronald S. Bultje > wrote: > > Hi, > > > > On Sun, Nov 1, 2015 at 1:03 PM, Ganesh Ajjanagadde < > gajjanaga...@gmail.com> > > wrote: > >> > >> Floating point to integer conversion is well defined when the float lies > >> within > >> the representation bounds of the integer after discarding the fractional > >> part. > >> However, in other cases, unfortunately the standard leaves it undefined. > >> In particular, assuming that it saturates in a sane way is a dangerous > >> assumption. > >> > >> In light of recent events, I would not have bothered if this was a > merely > >> theoretical > >> issue, and that common environments saturate correctly. Sadly, x86 (for > >> example) > >> converts casts to a cvttsd2si instruction which saturates numbers > > >> INT64_MAX to > >> INT64_MIN. This is mathematically completely bogus. > >> > http://blog.frama-c.com/index.php?post/2013/10/09/Overflow-float-integer > >> gives > >> a nice overview of the issue. > >> > >> 1/2 adds an av_clipd64 API for this purpose to clip a float to an > integral > >> range. > >> Obviously it will be slower than a cvttsd2si, but there is no option if > >> one wants > >> safe and well-defined behavior. Of course, if one knows a priori that a > >> float > >> lives in the integral type's range, then there is no issue. Safe > speedups > >> are > >> entirely possible, but API should be finalized first IMHO. > >> Most common anticipated usages are clipping to [INT64_MIN, INT64_MAX] or > >> [INT_MIN, INT_MAX]. > >> I have given some thought as to whether a separate av_clipd32 API (for > >> example) > >> is necessary. It seems to me to not be the case, since an IEEE-754 > double > >> is > >> guaranteed to represent exactly integers up to ~ 2^53. > >> Similarly, av_clipf64 and the like also seem unnecessary, since a double > >> is guaranteed > >> to represent all the values a float does. Such an API may be useful for > >> performance > >> though; I do not know how/what float to double conversions entail and at > >> the > >> moment ignore such complications. > >> > >> 1/2 also accordingly documents av_clipd64. > > > > > > So, is this a bug in llrint, or is this a failure to use llrint, or is > this > > different from llrint? It sounds to me that llrint should be used, not > our > > own alternative. > > Not a bug, just a standard "undefined behavior" cop-out from the > standards committee. We need to roll our own: all standard functions > cop out when it does not fit in the integer range, and Intel (and > others) have wonderfully exploited the cop-out: see e.g the link I > gave and my other reply. So how are we going to define "correct" behaviour? Let's take your INT64_MAX + 100 example. INT64_MAX is nowhere near the correct answer. Now what? Ronald ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCHv2 1/4] avfilter/all: check and propagate the return code of av_expr_parse_and_eval
Le primidi 11 brumaire, an CCXXIV, Ganesh Ajjanagadde a écrit : > Can't users give arbitrarily long arithmetic expressions? They can, and the current code deals gracefully with them. Regards, -- Nicolas George signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 0/2] first steps to resolving float to int undefined behavior
On Sun, Nov 1, 2015 at 6:18 PM, Ronald S. Bultje wrote: > Hi, > > On Sun, Nov 1, 2015 at 6:15 PM, Ganesh Ajjanagadde > wrote: >> >> On Sun, Nov 1, 2015 at 6:12 PM, Ronald S. Bultje >> wrote: >> > Hi, >> > >> > On Sun, Nov 1, 2015 at 1:03 PM, Ganesh Ajjanagadde >> > >> > wrote: >> >> >> >> Floating point to integer conversion is well defined when the float >> >> lies >> >> within >> >> the representation bounds of the integer after discarding the >> >> fractional >> >> part. >> >> However, in other cases, unfortunately the standard leaves it >> >> undefined. >> >> In particular, assuming that it saturates in a sane way is a dangerous >> >> assumption. >> >> >> >> In light of recent events, I would not have bothered if this was a >> >> merely >> >> theoretical >> >> issue, and that common environments saturate correctly. Sadly, x86 (for >> >> example) >> >> converts casts to a cvttsd2si instruction which saturates numbers > >> >> INT64_MAX to >> >> INT64_MIN. This is mathematically completely bogus. >> >> >> >> http://blog.frama-c.com/index.php?post/2013/10/09/Overflow-float-integer >> >> gives >> >> a nice overview of the issue. >> >> >> >> 1/2 adds an av_clipd64 API for this purpose to clip a float to an >> >> integral >> >> range. >> >> Obviously it will be slower than a cvttsd2si, but there is no option if >> >> one wants >> >> safe and well-defined behavior. Of course, if one knows a priori that a >> >> float >> >> lives in the integral type's range, then there is no issue. Safe >> >> speedups >> >> are >> >> entirely possible, but API should be finalized first IMHO. >> >> Most common anticipated usages are clipping to [INT64_MIN, INT64_MAX] >> >> or >> >> [INT_MIN, INT_MAX]. >> >> I have given some thought as to whether a separate av_clipd32 API (for >> >> example) >> >> is necessary. It seems to me to not be the case, since an IEEE-754 >> >> double >> >> is >> >> guaranteed to represent exactly integers up to ~ 2^53. >> >> Similarly, av_clipf64 and the like also seem unnecessary, since a >> >> double >> >> is guaranteed >> >> to represent all the values a float does. Such an API may be useful for >> >> performance >> >> though; I do not know how/what float to double conversions entail and >> >> at >> >> the >> >> moment ignore such complications. >> >> >> >> 1/2 also accordingly documents av_clipd64. >> > >> > >> > So, is this a bug in llrint, or is this a failure to use llrint, or is >> > this >> > different from llrint? It sounds to me that llrint should be used, not >> > our >> > own alternative. >> >> Not a bug, just a standard "undefined behavior" cop-out from the >> standards committee. We need to roll our own: all standard functions >> cop out when it does not fit in the integer range, and Intel (and >> others) have wonderfully exploited the cop-out: see e.g the link I >> gave and my other reply. > > > So how are we going to define "correct" behaviour? Let's take your INT64_MAX > + 100 example. INT64_MAX is nowhere near the correct answer. Now what? Why not? It is the saturated result, and the "best effort" answer. It is far more sane (when such things are needed) than returning INT64_MIN which is actually undefined behavior that can't be relied on. Maybe I was not clear enough, but I defined this API to do a mathematically best effort saturation. > > Ronald ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 0/2] first steps to resolving float to int undefined behavior
Le primidi 11 brumaire, an CCXXIV, Ronald S. Bultje a écrit : > So, is this a bug in llrint, or is this a failure to use llrint, or is this > different from llrint? It sounds to me that llrint should be used, not our > own alternative. Is there a sized version of the function? int64rint? Otherwise, these functions for the native types are as useless as the native types themselves. Regards, -- Nicolas George signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/2] all: use FFDIFFSIGN to resolve possible undefined behavior in comparators
On Sun, Nov 01, 2015 at 12:19:48PM -0500, Ganesh Ajjanagadde wrote: > FFDIFFSIGN was created explicitly for this purpose, since the common > return a - b idiom is unsafe regarding overflow on signed integers. It > optimizes to branchless code on common compilers. > > FFDIFFSIGN also has the subjective benefit of being easier to read due > to lack of ternary operators. > > Tested with FATE. > > Things not covered by this are unsigned integers, for which overflows > are well defined, and also places where overflow is clearly impossible, > e.g an instance where the a - b was being done on 24 bit values. > I can convert some of these to FFDIFFSIGN if people find it a > readability improvement. > > Signed-off-by: Ganesh Ajjanagadde > --- > cmdutils.c | 2 +- > cmdutils_opencl.c | 2 +- > ffmpeg.c| 3 +-- > libavcodec/aacsbr_template.c| 2 +- > libavcodec/motion_est.c | 2 +- > libavfilter/f_sendcmd.c | 8 +++- > libavfilter/vf_deshake.c| 3 +-- > libavfilter/vf_palettegen.c | 2 +- > libavfilter/vf_removegrain.c| 5 + > libavformat/subtitles.c | 9 +++-- > libswresample/swresample-test.c | 2 +- > 11 files changed, 15 insertions(+), 25 deletions(-) > > diff --git a/cmdutils.c b/cmdutils.c > index e3e9891..41daa95 100644 > --- a/cmdutils.c > +++ b/cmdutils.c > @@ -1421,7 +1421,7 @@ static int compare_codec_desc(const void *a, const void > *b) > const AVCodecDescriptor * const *da = a; > const AVCodecDescriptor * const *db = b; > > -return (*da)->type != (*db)->type ? (*da)->type - (*db)->type : > +return (*da)->type != (*db)->type ? FFDIFFSIGN((*da)->type, (*db)->type) > : Overflow is probably impossible for all "normal" enums, you wrote above that you omitted cases where its impossible to overflow, so you maybe want to omit this too > strcmp((*da)->name, (*db)->name); > } > > diff --git a/cmdutils_opencl.c b/cmdutils_opencl.c > index d9095b6..5621ebc 100644 > --- a/cmdutils_opencl.c > +++ b/cmdutils_opencl.c > @@ -206,7 +206,7 @@ end: > > static int compare_ocl_device_desc(const void *a, const void *b) > { > -return ((const OpenCLDeviceBenchmark*)a)->runtime - ((const > OpenCLDeviceBenchmark*)b)->runtime; > +return FFDIFFSIGN((const OpenCLDeviceBenchmark*)a->runtime , (const > OpenCLDeviceBenchmark*)b->runtime); > } nitpick: The reading of the 2 values should be on seperate lines to make this a bt more readable > > int opt_opencl_bench(void *optctx, const char *opt, const char *arg) > diff --git a/ffmpeg.c b/ffmpeg.c > index f8b071a..d3b8c4d 100644 > --- a/ffmpeg.c > +++ b/ffmpeg.c > @@ -2578,8 +2578,7 @@ static InputStream *get_input_stream(OutputStream *ost) > > static int compare_int64(const void *a, const void *b) > { > -int64_t va = *(int64_t *)a, vb = *(int64_t *)b; > -return va < vb ? -1 : va > vb ? +1 : 0; > +return FFDIFFSIGN(*(const int64_t *)a, *(const int64_t *)b); ok > } > > static int init_output_stream(OutputStream *ost, char *error, int error_len) > diff --git a/libavcodec/aacsbr_template.c b/libavcodec/aacsbr_template.c > index d31b71e..1e6f149 100644 > --- a/libavcodec/aacsbr_template.c > +++ b/libavcodec/aacsbr_template.c > @@ -104,7 +104,7 @@ av_cold void > AAC_RENAME(ff_aac_sbr_ctx_close)(SpectralBandReplication *sbr) > > static int qsort_comparison_function_int16(const void *a, const void *b) > { > -return *(const int16_t *)a - *(const int16_t *)b; > +return FFDIFFSIGN(*(const int16_t *)a , *(const int16_t *)b); a int16 difference cannot overflow > } > > static inline int in_table_int16(const int16_t *table, int last_el, int16_t > needle) > diff --git a/libavcodec/motion_est.c b/libavcodec/motion_est.c > index 9f71568..8560819 100644 > --- a/libavcodec/motion_est.c > +++ b/libavcodec/motion_est.c > @@ -73,7 +73,7 @@ static int minima_cmp(const void *a, const void *b){ > const Minima *da = (const Minima *) a; > const Minima *db = (const Minima *) b; > > -return da->height - db->height; > +return FFDIFFSIGN(da->height , db->height); > } this may be speed relevant, is this faster or slower or same speed than before ? also i suspect overflow is not be an issue here > > #define FLAG_QPEL 1 //must be 1 > diff --git a/libavfilter/f_sendcmd.c b/libavfilter/f_sendcmd.c > index 37aedc5..f06773a 100644 > --- a/libavfilter/f_sendcmd.c > +++ b/libavfilter/f_sendcmd.c > @@ -364,11 +364,9 @@ static int cmp_intervals(const void *a, const void *b) > { > const Interval *i1 = a; > const Interval *i2 = b; > -int64_t ts_diff = i1->start_ts - i2->start_ts; > -int ret; > - > -ret = ts_diff > 0 ? 1 : ts_diff < 0 ? -1 : 0; > -return ret == 0 ? i1->index - i2->index : ret; > +if (i1->start_ts == i2->start_ts) > +return FFDIFFSIGN(i1->index, i2->i
Re: [FFmpeg-devel] [PATCH 3/5] concatdec: move duration calculating code to open_file
On Mon, 2 Nov 2015, Nicolas George wrote: Le primidi 11 brumaire, an CCXXIV, Marton Balint a écrit : Because I add it to the metadata store here in the next patch, which metadata will be set for every file packet, so calculating it at the end of file is not an option. I see. But do you really need the duration metadata when it is not authoritative? I use it in the select filter at the end of the patch series... If duration is inaccurate, there is not much I can think of other than documenting it as a possible problem. Regards, Marton ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCHv2 1/4] avfilter/all: check and propagate the return code of av_expr_parse_and_eval
On Sun, Nov 1, 2015 at 6:20 PM, Nicolas George wrote: > Le primidi 11 brumaire, an CCXXIV, Ganesh Ajjanagadde a écrit : >> Can't users give arbitrarily long arithmetic expressions? > > They can, and the current code deals gracefully with them. ok, all patches except 2/4 (which maybe a valid typo fix) dropped. > > Regards, > > -- > Nicolas George > > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/4] avfilter/vf_rotate: correct log message
On Sat, Oct 31, 2015 at 10:47:55AM -0400, Ganesh Ajjanagadde wrote: > There seems to be some typos in the log messages that are fixed by this. probably ok [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB He who knows, does not speak. He who speaks, does not know. -- Lao Tsu signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 0/2] first steps to resolving float to int undefined behavior
On Sun, Nov 1, 2015 at 6:25 PM, Nicolas George wrote: > Le primidi 11 brumaire, an CCXXIV, Ronald S. Bultje a écrit : >> So, is this a bug in llrint, or is this a failure to use llrint, or is this >> different from llrint? It sounds to me that llrint should be used, not our >> own alternative. > > Is there a sized version of the function? int64rint? Otherwise, these > functions for the native types are as useless as the native types > themselves. No, not in ISO C, or even libc AFAIK. ISO C tends to favor the "non-sized" types in their signatures. The reason (I believe) stems from the fact that an implementation is free to not even define the sized types: 7.20.1.1, point 3 - "These types are optional. However, if an implementation provides integer types with widths of 8, 16, 32, or 64 bits, no padding bits, and (for the signed types) that have a two’s complement representation [all platforms supported by the project], it shall define the corresponding typedef names." - Thus they want to limit their scope to mostly (or perharps only?) stdint.h. Even otherwise, these functions are somewhat useless due to the undefined behavior outside the range. All they really do is get rounding done correctly according to the current FPU environment and associated rounding modes, which can be manipulated in C99/C11. > > Regards, > > -- > Nicolas George > > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] hlsenc: Only write PAT/PMT once per segment
On Sun, Nov 01, 2015 at 05:34:38PM +, Derek Buitenhuis wrote: > This saves a lot of muxing overhead, especially on lower bitrate > segments. > > Signed-off-by: Derek Buitenhuis > --- > libavformat/hlsenc.c | 10 +- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c > index 8daf53f..d7884e5 100644 > --- a/libavformat/hlsenc.c > +++ b/libavformat/hlsenc.c > @@ -560,8 +560,16 @@ static int hls_start(AVFormatContext *s) > } > av_dict_free(&options); > > -if (oc->oformat->priv_class && oc->priv_data) > +/* We only require one PAT/PMT per segment. */ > +if (oc->oformat->priv_class && oc->priv_data) { > +char period[21]; > + > +snprintf(period, sizeof(period), "%d", (INT_MAX / 2) - 1); > + > av_opt_set(oc->priv_data, "mpegts_flags", "resend_headers", 0); > +av_opt_set(oc->priv_data, "sdt_period", period, 0); > +av_opt_set(oc->priv_data, "pat_period", period, 0); should be ok if tested with some hls player [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB It is what and why we do it that matters, not just one of them. signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 0/2] first steps to resolving float to int undefined behavior
On Sun, Nov 01, 2015 at 06:51:19PM -0500, Ganesh Ajjanagadde wrote: > On Sun, Nov 1, 2015 at 6:25 PM, Nicolas George wrote: > > Le primidi 11 brumaire, an CCXXIV, Ronald S. Bultje a écrit : > >> So, is this a bug in llrint, or is this a failure to use llrint, or is this > >> different from llrint? It sounds to me that llrint should be used, not our > >> own alternative. > > > > Is there a sized version of the function? int64rint? Otherwise, these > > functions for the native types are as useless as the native types > > themselves. > > No, not in ISO C, or even libc AFAIK. ISO C tends to favor the > "non-sized" types in their signatures. The reason (I believe) stems > from the fact that an implementation is free to not even define the > sized types: > 7.20.1.1, point 3 - "These types are optional. However, if an > implementation provides integer types with widths of 8, 16, 32, or 64 > bits, no padding bits, and (for the signed types) that have a two’s > complement representation [all platforms supported by the project], it > shall define the corresponding typedef names." - > Thus they want to limit their scope to mostly (or perharps only?) stdint.h. > > Even otherwise, these functions are somewhat useless due to the > undefined behavior outside the range. All they really do is get > rounding done correctly according to the current FPU environment and > associated rounding modes, which can be manipulated in C99/C11. quite some of the undefined behavior also makes more optimizations possible for advanced compilers random silly example min = 0; for(i=0; i signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 0/2] first steps to resolving float to int undefined behavior
On Sun, Nov 1, 2015 at 7:15 PM, Michael Niedermayer wrote: > On Sun, Nov 01, 2015 at 06:51:19PM -0500, Ganesh Ajjanagadde wrote: >> On Sun, Nov 1, 2015 at 6:25 PM, Nicolas George wrote: >> > Le primidi 11 brumaire, an CCXXIV, Ronald S. Bultje a écrit : >> >> So, is this a bug in llrint, or is this a failure to use llrint, or is >> >> this >> >> different from llrint? It sounds to me that llrint should be used, not our >> >> own alternative. >> > >> > Is there a sized version of the function? int64rint? Otherwise, these >> > functions for the native types are as useless as the native types >> > themselves. >> >> No, not in ISO C, or even libc AFAIK. ISO C tends to favor the >> "non-sized" types in their signatures. The reason (I believe) stems >> from the fact that an implementation is free to not even define the >> sized types: >> 7.20.1.1, point 3 - "These types are optional. However, if an >> implementation provides integer types with widths of 8, 16, 32, or 64 >> bits, no padding bits, and (for the signed types) that have a two’s >> complement representation [all platforms supported by the project], it >> shall define the corresponding typedef names." - >> Thus they want to limit their scope to mostly (or perharps only?) stdint.h. >> >> Even otherwise, these functions are somewhat useless due to the >> undefined behavior outside the range. All they really do is get >> rounding done correctly according to the current FPU environment and >> associated rounding modes, which can be manipulated in C99/C11. > > quite some of the undefined behavior also makes more optimizations > possible for advanced compilers > random silly example > > min = 0; > for(i=0; i float c = whatever() > min = fmin(min, c); > out[i] = llrint(c); > } > > here the compiler can remove any and all handling of NaN and +-Inf > from fmin() because llrint(c) implies c is within the range > represetable of the integer types > > with a llrint() equivalent that is defined for all cases this is not > possible anymore Yes, which is why I have mentioned that we need to use a safe version only when we need to guarantee safety, and are dealing with arbitrary doubles. But such cases are there, such as the ffserver_config patch I posted. > > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > During times of universal deceit, telling the truth becomes a > revolutionary act. -- George Orwell > > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/2] all: use FFDIFFSIGN to resolve possible undefined behavior in comparators
On Sun, Nov 1, 2015 at 6:26 PM, Michael Niedermayer wrote: > On Sun, Nov 01, 2015 at 12:19:48PM -0500, Ganesh Ajjanagadde wrote: >> FFDIFFSIGN was created explicitly for this purpose, since the common >> return a - b idiom is unsafe regarding overflow on signed integers. It >> optimizes to branchless code on common compilers. >> >> FFDIFFSIGN also has the subjective benefit of being easier to read due >> to lack of ternary operators. >> >> Tested with FATE. >> >> Things not covered by this are unsigned integers, for which overflows >> are well defined, and also places where overflow is clearly impossible, >> e.g an instance where the a - b was being done on 24 bit values. >> I can convert some of these to FFDIFFSIGN if people find it a >> readability improvement. >> >> Signed-off-by: Ganesh Ajjanagadde >> --- >> cmdutils.c | 2 +- >> cmdutils_opencl.c | 2 +- >> ffmpeg.c| 3 +-- >> libavcodec/aacsbr_template.c| 2 +- >> libavcodec/motion_est.c | 2 +- >> libavfilter/f_sendcmd.c | 8 +++- >> libavfilter/vf_deshake.c| 3 +-- >> libavfilter/vf_palettegen.c | 2 +- >> libavfilter/vf_removegrain.c| 5 + >> libavformat/subtitles.c | 9 +++-- >> libswresample/swresample-test.c | 2 +- >> 11 files changed, 15 insertions(+), 25 deletions(-) >> >> diff --git a/cmdutils.c b/cmdutils.c >> index e3e9891..41daa95 100644 >> --- a/cmdutils.c >> +++ b/cmdutils.c >> @@ -1421,7 +1421,7 @@ static int compare_codec_desc(const void *a, const >> void *b) >> const AVCodecDescriptor * const *da = a; >> const AVCodecDescriptor * const *db = b; >> >> -return (*da)->type != (*db)->type ? (*da)->type - (*db)->type : >> +return (*da)->type != (*db)->type ? FFDIFFSIGN((*da)->type, >> (*db)->type) : > > Overflow is probably impossible for all "normal" enums, > you wrote above that you omitted cases where its impossible to > overflow, so you maybe want to omit this too This one was deliberate as a safety measure for future work with the codec types: implementers should be free to use the full enum range, and I don't want there to be safety issues. This is an instance of what I meant by "defensive programming". Strongly prefer not to change. > > > >> strcmp((*da)->name, (*db)->name); >> } >> >> diff --git a/cmdutils_opencl.c b/cmdutils_opencl.c >> index d9095b6..5621ebc 100644 >> --- a/cmdutils_opencl.c >> +++ b/cmdutils_opencl.c >> @@ -206,7 +206,7 @@ end: >> >> static int compare_ocl_device_desc(const void *a, const void *b) >> { >> -return ((const OpenCLDeviceBenchmark*)a)->runtime - ((const >> OpenCLDeviceBenchmark*)b)->runtime; >> +return FFDIFFSIGN((const OpenCLDeviceBenchmark*)a->runtime , (const >> OpenCLDeviceBenchmark*)b->runtime); >> } > > nitpick: The reading of the 2 values should be on seperate lines > to make this a bt more readable ok, as you may see I favored the one line return style due to its terseness. I personally don't mind either way and you favor the other, so changed. > > >> >> int opt_opencl_bench(void *optctx, const char *opt, const char *arg) >> diff --git a/ffmpeg.c b/ffmpeg.c >> index f8b071a..d3b8c4d 100644 >> --- a/ffmpeg.c >> +++ b/ffmpeg.c >> @@ -2578,8 +2578,7 @@ static InputStream *get_input_stream(OutputStream *ost) >> >> static int compare_int64(const void *a, const void *b) >> { >> -int64_t va = *(int64_t *)a, vb = *(int64_t *)b; >> -return va < vb ? -1 : va > vb ? +1 : 0; >> +return FFDIFFSIGN(*(const int64_t *)a, *(const int64_t *)b); > > ok > > >> } >> >> static int init_output_stream(OutputStream *ost, char *error, int error_len) >> diff --git a/libavcodec/aacsbr_template.c b/libavcodec/aacsbr_template.c >> index d31b71e..1e6f149 100644 >> --- a/libavcodec/aacsbr_template.c >> +++ b/libavcodec/aacsbr_template.c >> @@ -104,7 +104,7 @@ av_cold void >> AAC_RENAME(ff_aac_sbr_ctx_close)(SpectralBandReplication *sbr) >> >> static int qsort_comparison_function_int16(const void *a, const void *b) >> { >> -return *(const int16_t *)a - *(const int16_t *)b; >> +return FFDIFFSIGN(*(const int16_t *)a , *(const int16_t *)b); > > a int16 difference cannot overflow right, changed. > > >> } >> >> static inline int in_table_int16(const int16_t *table, int last_el, int16_t >> needle) >> diff --git a/libavcodec/motion_est.c b/libavcodec/motion_est.c >> index 9f71568..8560819 100644 >> --- a/libavcodec/motion_est.c >> +++ b/libavcodec/motion_est.c >> @@ -73,7 +73,7 @@ static int minima_cmp(const void *a, const void *b){ >> const Minima *da = (const Minima *) a; >> const Minima *db = (const Minima *) b; >> >> -return da->height - db->height; >> +return FFDIFFSIGN(da->height , db->height); >> } > > this may be speed relevant, > is this faster or slower or same speed than before ? > also i suspect overflow is not be an issue
[FFmpeg-devel] [PATCHv2 2/2] all: use FFDIFFSIGN to resolve possible undefined behavior in comparators
FFDIFFSIGN was created explicitly for this purpose, since the common return a - b idiom is unsafe regarding overflow on signed integers. It optimizes to branchless code on common compilers. FFDIFFSIGN also has the subjective benefit of being easier to read due to lack of ternary operators. Tested with FATE. Things not covered by this are unsigned integers, for which overflows are well defined, and also places where overflow is clearly impossible, e.g an instance where the a - b was being done on 24 bit values. I can convert some of these to FFDIFFSIGN if people find it a readability improvement. Reviewed-by: Michael Niedermayer Signed-off-by: Ganesh Ajjanagadde --- cmdutils.c | 2 +- cmdutils_opencl.c| 4 +++- ffmpeg.c | 3 +-- libavfilter/f_sendcmd.c | 6 +- libavfilter/vf_deshake.c | 3 +-- libavfilter/vf_palettegen.c | 2 +- libavfilter/vf_removegrain.c | 5 ++--- libavformat/subtitles.c | 9 +++-- 8 files changed, 13 insertions(+), 21 deletions(-) diff --git a/cmdutils.c b/cmdutils.c index e3e9891..41daa95 100644 --- a/cmdutils.c +++ b/cmdutils.c @@ -1421,7 +1421,7 @@ static int compare_codec_desc(const void *a, const void *b) const AVCodecDescriptor * const *da = a; const AVCodecDescriptor * const *db = b; -return (*da)->type != (*db)->type ? (*da)->type - (*db)->type : +return (*da)->type != (*db)->type ? FFDIFFSIGN((*da)->type, (*db)->type) : strcmp((*da)->name, (*db)->name); } diff --git a/cmdutils_opencl.c b/cmdutils_opencl.c index d9095b6..dd21344 100644 --- a/cmdutils_opencl.c +++ b/cmdutils_opencl.c @@ -206,7 +206,9 @@ end: static int compare_ocl_device_desc(const void *a, const void *b) { -return ((const OpenCLDeviceBenchmark*)a)->runtime - ((const OpenCLDeviceBenchmark*)b)->runtime; +const OpenCLDeviceBenchmark* va = (const OpenCLDeviceBenchmark*)a; +const OpenCLDeviceBenchmark* vb = (const OpenCLDeviceBenchmark*)b; +return FFDIFFSIGN(va->runtime , vb->runtime); } int opt_opencl_bench(void *optctx, const char *opt, const char *arg) diff --git a/ffmpeg.c b/ffmpeg.c index f8b071a..d3b8c4d 100644 --- a/ffmpeg.c +++ b/ffmpeg.c @@ -2578,8 +2578,7 @@ static InputStream *get_input_stream(OutputStream *ost) static int compare_int64(const void *a, const void *b) { -int64_t va = *(int64_t *)a, vb = *(int64_t *)b; -return va < vb ? -1 : va > vb ? +1 : 0; +return FFDIFFSIGN(*(const int64_t *)a, *(const int64_t *)b); } static int init_output_stream(OutputStream *ost, char *error, int error_len) diff --git a/libavfilter/f_sendcmd.c b/libavfilter/f_sendcmd.c index 37aedc5..fb30220 100644 --- a/libavfilter/f_sendcmd.c +++ b/libavfilter/f_sendcmd.c @@ -364,11 +364,7 @@ static int cmp_intervals(const void *a, const void *b) { const Interval *i1 = a; const Interval *i2 = b; -int64_t ts_diff = i1->start_ts - i2->start_ts; -int ret; - -ret = ts_diff > 0 ? 1 : ts_diff < 0 ? -1 : 0; -return ret == 0 ? i1->index - i2->index : ret; +return 2 * FFDIFFSIGN(i1->start_ts, i2->start_ts) + FFDIFFSIGN(i1->index, i2->index); } static av_cold int init(AVFilterContext *ctx) diff --git a/libavfilter/vf_deshake.c b/libavfilter/vf_deshake.c index e32436d..79fcc20 100644 --- a/libavfilter/vf_deshake.c +++ b/libavfilter/vf_deshake.c @@ -94,8 +94,7 @@ AVFILTER_DEFINE_CLASS(deshake); static int cmp(const void *a, const void *b) { -const double va = *(const double *)a, vb = *(const double *)b; -return va < vb ? -1 : ( va > vb ? 1 : 0 ); +return FFDIFFSIGN(*(const double *)a, *(const double *)b); } /** diff --git a/libavfilter/vf_palettegen.c b/libavfilter/vf_palettegen.c index df57c10..fccc5ca 100644 --- a/libavfilter/vf_palettegen.c +++ b/libavfilter/vf_palettegen.c @@ -130,7 +130,7 @@ static int cmp_color(const void *a, const void *b) { const struct range_box *box1 = a; const struct range_box *box2 = b; -return box1->color - box2->color; +return FFDIFFSIGN(box1->color , box2->color); } static av_always_inline int diff(const uint32_t a, const uint32_t b) diff --git a/libavfilter/vf_removegrain.c b/libavfilter/vf_removegrain.c index 3a28b15..898e50f 100644 --- a/libavfilter/vf_removegrain.c +++ b/libavfilter/vf_removegrain.c @@ -83,10 +83,9 @@ static int mode01(int c, int a1, int a2, int a3, int a4, int a5, int a6, int a7, static int cmp_int(const void *p1, const void *p2) { -int left = *(const int *)p1; +int left = *(const int *)p1; int right = *(const int *)p2; - -return ((left > right) - (left < right)); +return FFDIFFSIGN(left, right); } static int mode02(int c, int a1, int a2, int a3, int a4, int a5, int a6, int a7, int a8) diff --git a/libavformat/subtitles.c b/libavformat/subtitles.c index 471d600..7c6cd5f 100644 --- a/libavformat/subtitles.c +++ b/libavformat/subtitles.c @@ -146,1
Re: [FFmpeg-devel] [PATCH 1/3] avutil/eval: minor typo
On Sun, Nov 1, 2015 at 5:10 PM, Michael Niedermayer wrote: > On Sun, Nov 01, 2015 at 11:57:53AM -0500, Ganesh Ajjanagadde wrote: >> Signed-off-by: Ganesh Ajjanagadde >> --- >> libavutil/eval.h | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/libavutil/eval.h b/libavutil/eval.h >> index 6159b0f..dacd22b 100644 >> --- a/libavutil/eval.h >> +++ b/libavutil/eval.h >> @@ -102,7 +102,7 @@ void av_expr_free(AVExpr *e); >> * @param numstr a string representing a number, may contain one of >> * the International System number postfixes, for example 'K', 'M', >> * 'G'. If 'i' is appended after the postfix, powers of 2 are used >> - * instead of powers of 10. The 'B' postfix multiplies the value for >> + * instead of powers of 10. The 'B' postfix multiplies the value by > > ok pushed, thanks. > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > Dictatorship naturally arises out of democracy, and the most aggravated > form of tyranny and slavery out of the most extreme liberty. -- Plato > > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] boxblur: Templatize blur{8,16}
On Sun, Nov 01, 2015 at 10:40:03AM -0800, Timothy Gu wrote: > --- > libavfilter/vf_boxblur.c | 110 > +++ > 1 file changed, 44 insertions(+), 66 deletions(-) LGTM thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The bravest are surely those who have the clearest vision of what is before them, glory and danger alike, and yet notwithstanding go out to meet it. -- Thucydides signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 0/2] first steps to resolving float to int undefined behavior
Hi, On Sun, Nov 1, 2015 at 7:21 PM, Ganesh Ajjanagadde wrote: > On Sun, Nov 1, 2015 at 7:15 PM, Michael Niedermayer > wrote: > > On Sun, Nov 01, 2015 at 06:51:19PM -0500, Ganesh Ajjanagadde wrote: > >> On Sun, Nov 1, 2015 at 6:25 PM, Nicolas George wrote: > >> > Le primidi 11 brumaire, an CCXXIV, Ronald S. Bultje a écrit : > >> >> So, is this a bug in llrint, or is this a failure to use llrint, or > is this > >> >> different from llrint? It sounds to me that llrint should be used, > not our > >> >> own alternative. > >> > > >> > Is there a sized version of the function? int64rint? Otherwise, these > >> > functions for the native types are as useless as the native types > >> > themselves. > >> > >> No, not in ISO C, or even libc AFAIK. ISO C tends to favor the > >> "non-sized" types in their signatures. The reason (I believe) stems > >> from the fact that an implementation is free to not even define the > >> sized types: > >> 7.20.1.1, point 3 - "These types are optional. However, if an > >> implementation provides integer types with widths of 8, 16, 32, or 64 > >> bits, no padding bits, and (for the signed types) that have a two’s > >> complement representation [all platforms supported by the project], it > >> shall define the corresponding typedef names." - > >> Thus they want to limit their scope to mostly (or perharps only?) > stdint.h. > >> > >> Even otherwise, these functions are somewhat useless due to the > >> undefined behavior outside the range. All they really do is get > >> rounding done correctly according to the current FPU environment and > >> associated rounding modes, which can be manipulated in C99/C11. > > > > quite some of the undefined behavior also makes more optimizations > > possible for advanced compilers > > random silly example > > > > min = 0; > > for(i=0; i > float c = whatever() > > min = fmin(min, c); > > out[i] = llrint(c); > > } > > > > here the compiler can remove any and all handling of NaN and +-Inf > > from fmin() because llrint(c) implies c is within the range > > represetable of the integer types > > > > with a llrint() equivalent that is defined for all cases this is not > > possible anymore > > Yes, which is why I have mentioned that we need to use a safe version > only when we need to guarantee safety, and are dealing with arbitrary > doubles. But such cases are there, such as the ffserver_config patch I > posted. OK, so let me sum up my current thoughts: - I don't really care about ffserver, so I have no opinion on whether the patch is appropriate. It may well be. Let's assume it should. - I think the name of this function is confusing. The av_clip* namespace seems reserved for int to int clips, so using it for a float to int conversion with clip is kind of weird. I would use a different namespace for it. - I'm not sure the code should live in this header. av_clip* is internal API (afaiu), and ffserver should not use internal api. I know it does, but it shouldn't, so adding new api specifically so ffserver can use it is ... all upside-down. Maybe the code should (for now) live in ffserver, or in an installed header, so it's ok for apps (like ffserver) to use it, if that's the intention. So, finally, maybe I'm being pedantic now, but it seems one of these usages overflows if we go over 2^64 = 16000 petabytes. Is that right? Do we really care? I mean, we're talking ffserver here, ffserver has much bigger issues than this. Ronald ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 0/2] first steps to resolving float to int undefined behavior
On Sun, Nov 1, 2015 at 7:59 PM, Ronald S. Bultje wrote: > Hi, > > On Sun, Nov 1, 2015 at 7:21 PM, Ganesh Ajjanagadde wrote: > >> On Sun, Nov 1, 2015 at 7:15 PM, Michael Niedermayer >> wrote: >> > On Sun, Nov 01, 2015 at 06:51:19PM -0500, Ganesh Ajjanagadde wrote: >> >> On Sun, Nov 1, 2015 at 6:25 PM, Nicolas George wrote: >> >> > Le primidi 11 brumaire, an CCXXIV, Ronald S. Bultje a écrit : >> >> >> So, is this a bug in llrint, or is this a failure to use llrint, or >> is this >> >> >> different from llrint? It sounds to me that llrint should be used, >> not our >> >> >> own alternative. >> >> > >> >> > Is there a sized version of the function? int64rint? Otherwise, these >> >> > functions for the native types are as useless as the native types >> >> > themselves. >> >> >> >> No, not in ISO C, or even libc AFAIK. ISO C tends to favor the >> >> "non-sized" types in their signatures. The reason (I believe) stems >> >> from the fact that an implementation is free to not even define the >> >> sized types: >> >> 7.20.1.1, point 3 - "These types are optional. However, if an >> >> implementation provides integer types with widths of 8, 16, 32, or 64 >> >> bits, no padding bits, and (for the signed types) that have a two’s >> >> complement representation [all platforms supported by the project], it >> >> shall define the corresponding typedef names." - >> >> Thus they want to limit their scope to mostly (or perharps only?) >> stdint.h. >> >> >> >> Even otherwise, these functions are somewhat useless due to the >> >> undefined behavior outside the range. All they really do is get >> >> rounding done correctly according to the current FPU environment and >> >> associated rounding modes, which can be manipulated in C99/C11. >> > >> > quite some of the undefined behavior also makes more optimizations >> > possible for advanced compilers >> > random silly example >> > >> > min = 0; >> > for(i=0; i> > float c = whatever() >> > min = fmin(min, c); >> > out[i] = llrint(c); >> > } >> > >> > here the compiler can remove any and all handling of NaN and +-Inf >> > from fmin() because llrint(c) implies c is within the range >> > represetable of the integer types >> > >> > with a llrint() equivalent that is defined for all cases this is not >> > possible anymore >> >> Yes, which is why I have mentioned that we need to use a safe version >> only when we need to guarantee safety, and are dealing with arbitrary >> doubles. But such cases are there, such as the ffserver_config patch I >> posted. > > > OK, so let me sum up my current thoughts: > - I don't really care about ffserver, so I have no opinion on whether the > patch is appropriate. It may well be. Let's assume it should. I don't care about ffserver either. Gave this as the simplest illustration (and where I originally found the issue). There is at least one example in libavfilter: af_adelay:148 (not a vulnerability due to the check in practice, still undefined behavior as delay is untrusted), cmdutils.c, and possibly many more. I think all agree by now that this is a real issue in parts of the codebase, and we need a function for this. How many places/where all, I don't know. > - I think the name of this function is confusing. The av_clip* namespace > seems reserved for int to int clips, so using it for a float to int > conversion with clip is kind of weird. I would use a different namespace > for it. Mathematically, it is the same as clipping, albeit "lossy", but then again all clipping is by nature "lossy" on inputs outside the clipping range, hence the choice of name. Any ideas for a namespace for this? > - I'm not sure the code should live in this header. av_clip* is internal > API (afaiu), and ffserver should not use internal api. I know it does, but > it shouldn't, so adding new api specifically so ffserver can use it is ... > all upside-down. It is NOT specifically for ffserver. If it was, I would not have bothered at all with the patch. > Maybe the code should (for now) live in ffserver, or in an > installed header, so it's ok for apps (like ffserver) to use it, if that's > the intention. Not just for apps, also for libavfilter, likely libavcodec as well. Hence went in libavutil. > > So, finally, maybe I'm being pedantic now, but it seems one of these usages > overflows if we go over 2^64 = 16000 petabytes. Is that right? Do we really > care? I mean, we're talking ffserver here, ffserver has much bigger issues > than this. You are right, gave it as an illustration of API usage and an issue, albeit not a terribly important one :) - user's config is an untrusted file. > > Ronald > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/3] avfilter/vf_frei0r: use av_strtod instead of strtod for added flexibility
On Sun, Nov 01, 2015 at 11:57:54AM -0500, Ganesh Ajjanagadde wrote: > This converts the usage of strtod to av_strtod in order to be more free > in accepting double parameters. "more free" is a bit unclear and confusing the change itself should be ok thanks [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB it is not once nor twice but times without number that the same ideas make their appearance in the world. -- Aristotle signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avutil/opencl_internal: add av_warn_unused_result
From: Ganesh Ajjanagadde Date: 2015-10-29 10:53 To: FFmpeg development discussions and patches CC: Ganesh Ajjanagadde Subject: Re: [FFmpeg-devel] [PATCH] avutil/opencl_internal: add av_warn_unused_result On Thu, Oct 15, 2015 at 6:24 PM, Ganesh Ajjanagadde wrote: > Signed-off-by: Ganesh Ajjanagadde > --- > libavutil/opencl_internal.h | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/libavutil/opencl_internal.h b/libavutil/opencl_internal.h > index dacd930..a4f5885 100644 > --- a/libavutil/opencl_internal.h > +++ b/libavutil/opencl_internal.h > @@ -30,4 +30,5 @@ typedef struct { > void *ctx; > } FFOpenclParam; > > +av_warn_unused_result > int avpriv_opencl_set_parameter(FFOpenclParam *opencl_param, ...); > -- > 2.6.1 > ping Hi Looks good to me. Thanks Best regards ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 0/2] first steps to resolving float to int undefined behavior
On Sun, Nov 01, 2015 at 07:59:08PM -0500, Ronald S. Bultje wrote: > Hi, > > On Sun, Nov 1, 2015 at 7:21 PM, Ganesh Ajjanagadde wrote: > > > On Sun, Nov 1, 2015 at 7:15 PM, Michael Niedermayer > > wrote: > > > On Sun, Nov 01, 2015 at 06:51:19PM -0500, Ganesh Ajjanagadde wrote: > > >> On Sun, Nov 1, 2015 at 6:25 PM, Nicolas George wrote: > > >> > Le primidi 11 brumaire, an CCXXIV, Ronald S. Bultje a écrit : > > >> >> So, is this a bug in llrint, or is this a failure to use llrint, or > > is this > > >> >> different from llrint? It sounds to me that llrint should be used, > > not our > > >> >> own alternative. > > >> > > > >> > Is there a sized version of the function? int64rint? Otherwise, these > > >> > functions for the native types are as useless as the native types > > >> > themselves. > > >> > > >> No, not in ISO C, or even libc AFAIK. ISO C tends to favor the > > >> "non-sized" types in their signatures. The reason (I believe) stems > > >> from the fact that an implementation is free to not even define the > > >> sized types: > > >> 7.20.1.1, point 3 - "These types are optional. However, if an > > >> implementation provides integer types with widths of 8, 16, 32, or 64 > > >> bits, no padding bits, and (for the signed types) that have a two’s > > >> complement representation [all platforms supported by the project], it > > >> shall define the corresponding typedef names." - > > >> Thus they want to limit their scope to mostly (or perharps only?) > > stdint.h. > > >> > > >> Even otherwise, these functions are somewhat useless due to the > > >> undefined behavior outside the range. All they really do is get > > >> rounding done correctly according to the current FPU environment and > > >> associated rounding modes, which can be manipulated in C99/C11. > > > > > > quite some of the undefined behavior also makes more optimizations > > > possible for advanced compilers > > > random silly example > > > > > > min = 0; > > > for(i=0; i > > float c = whatever() > > > min = fmin(min, c); > > > out[i] = llrint(c); > > > } > > > > > > here the compiler can remove any and all handling of NaN and +-Inf > > > from fmin() because llrint(c) implies c is within the range > > > represetable of the integer types > > > > > > with a llrint() equivalent that is defined for all cases this is not > > > possible anymore > > > > Yes, which is why I have mentioned that we need to use a safe version > > only when we need to guarantee safety, and are dealing with arbitrary > > doubles. But such cases are there, such as the ffserver_config patch I > > posted. > > > OK, so let me sum up my current thoughts: > - I don't really care about ffserver, so I have no opinion on whether the > patch is appropriate. It may well be. Let's assume it should. > - I think the name of this function is confusing. The av_clip* namespace > seems reserved for int to int clips, so using it for a float to int > conversion with clip is kind of weird. I would use a different namespace > for it. > - I'm not sure the code should live in this header. av_clip* is internal > API (afaiu), and ffserver should not use internal api. I know it does, but > it shouldn't, so adding new api specifically so ffserver can use it is ... > all upside-down. Maybe the code should (for now) live in ffserver, or in an > installed header, so it's ok for apps (like ffserver) to use it, if that's > the intention. common.h is a installed header see /usr/include/libavutil/common.h > > So, finally, maybe I'm being pedantic now, but it seems one of these usages > overflows if we go over 2^64 = 16000 petabytes. Is that right? Do we really > care? I mean, we're talking ffserver here, ffserver has much bigger issues > than this. > > Ronald > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Asymptotically faster algorithms should always be preferred if you have asymptotical amounts of data signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/3] avfilter/vf_frei0r: use av_strtod instead of strtod for added flexibility
On Sun, Nov 1, 2015 at 8:23 PM, Michael Niedermayer wrote: > On Sun, Nov 01, 2015 at 11:57:54AM -0500, Ganesh Ajjanagadde wrote: >> This converts the usage of strtod to av_strtod in order to be more free >> in accepting double parameters. > > "more free" is a bit unclear and confusing > the change itself should be ok "more flexible" maybe? If no one replies, will push with "more flexible". > > thanks > > [...] > > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > it is not once nor twice but times without number that the same ideas make > their appearance in the world. -- Aristotle > > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avutil/opencl_internal: add av_warn_unused_result
On Sun, Nov 1, 2015 at 8:23 PM, highgod0401 wrote: > > From: Ganesh Ajjanagadde > Date: 2015-10-29 10:53 > To: FFmpeg development discussions and patches > CC: Ganesh Ajjanagadde > Subject: Re: [FFmpeg-devel] [PATCH] avutil/opencl_internal: add > av_warn_unused_result > On Thu, Oct 15, 2015 at 6:24 PM, Ganesh Ajjanagadde > wrote: >> Signed-off-by: Ganesh Ajjanagadde >> --- >> libavutil/opencl_internal.h | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/libavutil/opencl_internal.h b/libavutil/opencl_internal.h >> index dacd930..a4f5885 100644 >> --- a/libavutil/opencl_internal.h >> +++ b/libavutil/opencl_internal.h >> @@ -30,4 +30,5 @@ typedef struct { >> void *ctx; >> } FFOpenclParam; >> >> +av_warn_unused_result >> int avpriv_opencl_set_parameter(FFOpenclParam *opencl_param, ...); >> -- >> 2.6.1 >> > > ping > > Hi > > Looks good to me. already reviewed and pushed (with a small but crucial addition) in commit 8a5b60a6b1d841b74c2670f5165c8b05321f395a. Thanks. > > Thanks > Best regards > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] ffmpeg: fix -copy_prior_start 0 with -copyts and input -ss
Also rearranged the relevant check to reduce code duplication --- ffmpeg.c | 15 +++ 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/ffmpeg.c b/ffmpeg.c index dbb2520..526f094 100644 --- a/ffmpeg.c +++ b/ffmpeg.c @@ -1760,7 +1760,6 @@ static void do_streamcopy(InputStream *ist, OutputStream *ost, const AVPacket *p InputFile *f = input_files [ist->file_index]; int64_t start_time = (of->start_time == AV_NOPTS_VALUE) ? 0 : of->start_time; int64_t ost_tb_start_time = av_rescale_q(start_time, AV_TIME_BASE_Q, ost->st->time_base); -int64_t ist_tb_start_time = av_rescale_q(start_time, AV_TIME_BASE_Q, ist->st->time_base); AVPicture pict; AVPacket opkt; @@ -1770,13 +1769,13 @@ static void do_streamcopy(InputStream *ist, OutputStream *ost, const AVPacket *p !ost->copy_initial_nonkeyframes) return; -if (pkt->pts == AV_NOPTS_VALUE) { -if (!ost->frame_number && ist->pts < start_time && -!ost->copy_prior_start) -return; -} else { -if (!ost->frame_number && pkt->pts < ist_tb_start_time && -!ost->copy_prior_start) +if (!ost->frame_number && !ost->copy_prior_start) { +int64_t comp_start = start_time; +if (copy_ts && f->start_time != AV_NOPTS_VALUE) +comp_start = FFMAX(start_time, f->start_time + f->ts_offset); +if (pkt->pts == AV_NOPTS_VALUE ? +ist->pts < comp_start : +pkt->pts < av_rescale_q(comp_start, AV_TIME_BASE_Q, ist->st->time_base)) return; } -- 2.6.2 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] boxblur: Templatize blur{8,16}
On Sun, Nov 1, 2015 at 4:45 PM Michael Niedermayer wrote: > On Sun, Nov 01, 2015 at 10:40:03AM -0800, Timothy Gu wrote: > > --- > > libavfilter/vf_boxblur.c | 110 > +++ > > 1 file changed, 44 insertions(+), 66 deletions(-) > > LGTM > thx > Pushed, thanks for the review. Timothy ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] avformat/cache: Separate read and seek return-values within cache_read
Fixes an issue where an int64_t ffurl_seek return-value was being stored in a generic int "r" variable, leading to integer overflow when seeking into a large file (>2GB), and ultimately a "Failed to perform internal seek" error mesage. The "r" variable was used both for storing the result of any seeks and for the result of any reads. Instead, I separate the variable into an int64_t seek-variable and int read-variable. To test, try running `ffprobe 'cache:http://'` on a file that is ~3GB large, whose moov atom is at the end of the file --- libavformat/cache.c | 49 + 1 files changed, 25 insertions(+), 24 deletions(-) diff --git a/libavformat/cache.c b/libavformat/cache.c index a762aa9..b654f45 100644 --- a/libavformat/cache.c +++ b/libavformat/cache.c @@ -156,7 +156,8 @@ static int cache_read(URLContext *h, unsigned char *buf, int size) { Context *c= h->priv_data; CacheEntry *entry, *next[2] = {NULL, NULL}; -int r; +int64_t off; +int bytes_read = 0; entry = av_tree_find(c->root, &c->logical_pos, cmp, (void**)next); @@ -169,21 +170,21 @@ static int cache_read(URLContext *h, unsigned char *buf, int size) if (in_block_pos < entry->size) { int64_t physical_target = entry->physical_pos + in_block_pos; -if (c->cache_pos != physical_target) { -r = lseek(c->fd, physical_target, SEEK_SET); -} else -r = c->cache_pos; +if (c->cache_pos != physical_target) +off = lseek(c->fd, physical_target, SEEK_SET); +else +off = c->cache_pos; -if (r >= 0) { -c->cache_pos = r; -r = read(c->fd, buf, FFMIN(size, entry->size - in_block_pos)); +if (off >= 0) { +c->cache_pos = off; +bytes_read = read(c->fd, buf, FFMIN(size, entry->size - in_block_pos)); } -if (r > 0) { -c->cache_pos += r; -c->logical_pos += r; +if (bytes_read > 0) { +c->cache_pos += bytes_read; +c->logical_pos += bytes_read; c->cache_hit ++; -return r; +return bytes_read; } } } @@ -191,30 +192,30 @@ static int cache_read(URLContext *h, unsigned char *buf, int size) // Cache miss or some kind of fault with the cache if (c->logical_pos != c->inner_pos) { -r = ffurl_seek(c->inner, c->logical_pos, SEEK_SET); -if (r<0) { +off = ffurl_seek(c->inner, c->logical_pos, SEEK_SET); +if (off<0) { av_log(h, AV_LOG_ERROR, "Failed to perform internal seek\n"); -return r; +return off; } -c->inner_pos = r; +c->inner_pos = off; } -r = ffurl_read(c->inner, buf, size); -if (r == 0 && size>0) { +bytes_read = ffurl_read(c->inner, buf, size); +if (bytes_read == 0 && size>0) { c->is_true_eof = 1; av_assert0(c->end >= c->logical_pos); } -if (r<=0) -return r; -c->inner_pos += r; +if (bytes_read<=0) +return bytes_read; +c->inner_pos += bytes_read; c->cache_miss ++; -add_entry(h, buf, r); -c->logical_pos += r; +add_entry(h, buf, bytes_read); +c->logical_pos += bytes_read; c->end = FFMAX(c->end, c->logical_pos); -return r; +return bytes_read; } static int64_t cache_seek(URLContext *h, int64_t pos, int whence) -- 1.7.1 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCHv3 2/2] mpegtsenc: Implement writing of Opus trim_start/trim_end control values
From: Sebastian Dröge Signed-off-by: Sebastian Dröge --- libavformat/mpegtsenc.c | 43 --- 1 file changed, 40 insertions(+), 3 deletions(-) diff --git a/libavformat/mpegtsenc.c b/libavformat/mpegtsenc.c index 0674064..11ce130 100644 --- a/libavformat/mpegtsenc.c +++ b/libavformat/mpegtsenc.c @@ -230,6 +230,7 @@ typedef struct MpegTSWriteStream { /* For Opus */ int opus_queued_samples; +int opus_pending_trim_start; } MpegTSWriteStream; static void mpegts_write_pat(AVFormatContext *s) @@ -825,6 +826,9 @@ static int mpegts_write_header(AVFormatContext *s) if (ret < 0) goto fail; } +if (st->codec->codec_id == AV_CODEC_ID_OPUS) { +ts_st->opus_pending_trim_start = st->codec->initial_padding * 48000 / st->codec->sample_rate; +} } av_freep(&pids); @@ -1513,17 +1517,38 @@ static int mpegts_write_packet_internal(AVFormatContext *s, AVPacket *pkt) /* Add Opus control header */ if ((AV_RB16(pkt->data) >> 5) != 0x3ff) { +uint8_t *side_data; +int side_data_size; int i, n; +int ctrl_header_size; +int trim_start = 0, trim_end = 0; opus_samples = opus_get_packet_samples(s, pkt); -data = av_malloc(pkt->size + 2 + pkt->size / 255 + 1); +side_data = av_packet_get_side_data(pkt, +AV_PKT_DATA_SKIP_SAMPLES, +&side_data_size); + +if (side_data && side_data_size >= 10) { +trim_end = AV_RL32(side_data + 4) * 48000 / st->codec->sample_rate; +} + +ctrl_header_size = pkt->size + 2 + pkt->size / 255 + 1; +if (ts_st->opus_pending_trim_start) + ctrl_header_size += 2; +if (trim_end) + ctrl_header_size += 2; + +data = av_malloc(ctrl_header_size); if (!data) return AVERROR(ENOMEM); -/* TODO: Write trim if needed */ data[0] = 0x7f; data[1] = 0xe0; +if (ts_st->opus_pending_trim_start) +data[1] |= 0x10; +if (trim_end) +data[1] |= 0x08; n = pkt->size; i = 2; @@ -1535,9 +1560,21 @@ static int mpegts_write_packet_internal(AVFormatContext *s, AVPacket *pkt) av_assert0(2 + pkt->size / 255 + 1 == i); +if (ts_st->opus_pending_trim_start) { +trim_start = FFMIN(ts_st->opus_pending_trim_start, opus_samples); +AV_WB16(data + i, trim_start); +i += 2; +ts_st->opus_pending_trim_start -= trim_start; +} +if (trim_end) { +trim_end = FFMIN(trim_end, opus_samples - trim_start); +AV_WB16(data + i, trim_end); +i += 2; +} + memcpy(data + i, pkt->data, pkt->size); buf = data; -size= pkt->size + 2 + pkt->size / 255 + 1; +size= ctrl_header_size; } else { /* TODO: Can we get TS formatted data here? If so we will * need to count the samples of that too! */ -- 2.6.2 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel