[FFmpeg-devel] [PATCH 1/2] avformat: always unref the packet after parsing

[Hendrik Leppkes]

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

[Hendrik Leppkes]

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

[wm4]

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

[Hendrik Leppkes]

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

[wm4]

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

[Paul B Mahol]

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

[wm4]

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

[Ganesh Ajjanagadde]

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

[Ganesh Ajjanagadde]

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

[Ganesh Ajjanagadde]

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

[Ganesh Ajjanagadde]

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

[Timothy Gu]

---
 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

[Timothy Gu]

---
 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

[Ganesh Ajjanagadde]

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

[Ganesh Ajjanagadde]

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

[Ganesh Ajjanagadde]

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

[Derek Buitenhuis]

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

[Michael Niedermayer]

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

[Michael Niedermayer]

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

[Michael Niedermayer]

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

[Ganesh Ajjanagadde]

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

[Ganesh Ajjanagadde]

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

[Ganesh Ajjanagadde]

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

[Ganesh Ajjanagadde]

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}

[Timothy Gu]

---
 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}

[Ganesh Ajjanagadde]

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

[Michael Niedermayer]

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}

[Timothy Gu]

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

[Ganesh Ajjanagadde]

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

[Sebastian Dröge]

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

[Sebastian Dröge]

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

[Sebastian Dröge]

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

[Ganesh Ajjanagadde]

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

[Ganesh Ajjanagadde]

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

[Ganesh Ajjanagadde]

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

[Ganesh Ajjanagadde]

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

[Nicolas George]

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

[Ganesh Ajjanagadde]

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

[Sebastian Dröge]

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

[Sebastian Dröge]

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

[Sebastian Dröge]

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

[Michael Niedermayer]

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

[Michael Niedermayer]

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

[Nicolas George]

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

[Michael Niedermayer]

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

[Marton Balint]

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

[Nicolas George]

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

[Ganesh Ajjanagadde]

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

[Ronald S. Bultje]

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

[Ronald S. Bultje]

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

[Ganesh Ajjanagadde]

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

[Ganesh Ajjanagadde]

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

[Marton Balint]

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

[Marton Balint]

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

[Marton Balint]

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

[Marton Balint]

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

[Ronald S. Bultje]

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

[Nicolas George]

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

[Ganesh Ajjanagadde]

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

[Nicolas George]

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

[Michael Niedermayer]

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

[Marton Balint]

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

[Ganesh Ajjanagadde]

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

[Michael Niedermayer]

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

[Ganesh Ajjanagadde]

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

[Michael Niedermayer]

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

[Michael Niedermayer]

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

[Ganesh Ajjanagadde]

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

[Ganesh Ajjanagadde]

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

[Ganesh Ajjanagadde]

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

[Ganesh Ajjanagadde]

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}

[Michael Niedermayer]

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

[Ronald S. Bultje]

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

[Ganesh Ajjanagadde]

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

[Michael Niedermayer]

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

[highgod0401]

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

[Michael Niedermayer]

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

[Ganesh Ajjanagadde]

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

[Ganesh Ajjanagadde]

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

[Rodger Combs]

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}

[Timothy Gu]

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

[Bryan Huh]

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

[Sebastian Dröge]

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