Re: [FFmpeg-devel] [PATCH] avcodec/mpegvideo: Fix all but one tsan warning in fate-vsynth1-mpeg4

2017-07-09 Thread Ronald S. Bultje 
Hi,

On Sun, Jul 9, 2017 at 9:19 PM, Michael Niedermayer 
wrote:

> Signed-off-by: Michael Niedermayer 
> ---
>  libavcodec/mpegvideo.c | 15 ++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/libavcodec/mpegvideo.c b/libavcodec/mpegvideo.c
> index e29558b3a2..574b3854e0 100644
> --- a/libavcodec/mpegvideo.c
> +++ b/libavcodec/mpegvideo.c
> @@ -495,7 +495,20 @@ int ff_mpeg_update_thread_context(AVCodecContext
> *dst,
>  // in that case dst may need a reinit
>  if (!s->context_initialized) {
>  int err;
> -memcpy(s, s1, sizeof(MpegEncContext));
> +#define COPY(x) s->x = s1->x;
>

Unused?


> +#define COPYR(a, b) memcpy(>a, >a, ((uint8_t*)>b) -
> ((uint8_t*)>a))
> +COPYR(h263_aic, qscale);
> +COPYR(lambda,   mv_dir);
> +COPYR(no_rounding,  dest);
> +COPYR(mb_index2xy,  resync_mb_x);
> +COPYR(next_p_frame_damaged, h263_aic_dir);
> +COPYR(h263_slice_structured,mcsel);
> +COPYR(quant_precision,  first_slice_line);
> +COPYR(flipflop_rounding,gb);
> +COPYR(gop_picture_number,   picture_structure);
> +COPYR(picture_structure,pblocks);
> +COPYR(decode_mb,er);
> +COPYR(error_rate,   noise_reduction);


This is very obscure... The obscure part is what happens when fields get
(through refactoring) reordered or new fields get inserted...

I also appear to remember that we used to use this same pattern for h264
also, but we don't anymore. Does anyone remember why?

Ronald
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avcodec/mpegvideo: Fix all but one tsan warning in fate-vsynth1-mpeg4

2017-07-09 Thread Muhammad Faiz 
On Mon, Jul 10, 2017 at 8:19 AM, Michael Niedermayer
 wrote:
> Signed-off-by: Michael Niedermayer 
> ---
>  libavcodec/mpegvideo.c | 15 ++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/libavcodec/mpegvideo.c b/libavcodec/mpegvideo.c
> index e29558b3a2..574b3854e0 100644
> --- a/libavcodec/mpegvideo.c
> +++ b/libavcodec/mpegvideo.c
> @@ -495,7 +495,20 @@ int ff_mpeg_update_thread_context(AVCodecContext *dst,
>  // in that case dst may need a reinit
>  if (!s->context_initialized) {
>  int err;
> -memcpy(s, s1, sizeof(MpegEncContext));
> +#define COPY(x) s->x = s1->x;
> +#define COPYR(a, b) memcpy(>a, >a, ((uint8_t*)>b) - 
> ((uint8_t*)>a))
> +COPYR(h263_aic, qscale);
> +COPYR(lambda,   mv_dir);
> +COPYR(no_rounding,  dest);
> +COPYR(mb_index2xy,  resync_mb_x);
> +COPYR(next_p_frame_damaged, h263_aic_dir);
> +COPYR(h263_slice_structured,mcsel);
> +COPYR(quant_precision,  first_slice_line);
> +COPYR(flipflop_rounding,gb);
> +COPYR(gop_picture_number,   picture_structure);
> +COPYR(picture_structure,pblocks);
> +COPYR(decode_mb,er);
> +COPYR(error_rate,   noise_reduction);

Of course this is ugly.
What about conditional memcpy, e.g if dst_byte != src_byte then
dst_byte = src_byte?
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH] avcodec/mpegvideo: Fix all but one tsan warning in fate-vsynth1-mpeg4

2017-07-09 Thread Michael Niedermayer 
Signed-off-by: Michael Niedermayer 
---
 libavcodec/mpegvideo.c | 15 ++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/libavcodec/mpegvideo.c b/libavcodec/mpegvideo.c
index e29558b3a2..574b3854e0 100644
--- a/libavcodec/mpegvideo.c
+++ b/libavcodec/mpegvideo.c
@@ -495,7 +495,20 @@ int ff_mpeg_update_thread_context(AVCodecContext *dst,
 // in that case dst may need a reinit
 if (!s->context_initialized) {
 int err;
-memcpy(s, s1, sizeof(MpegEncContext));
+#define COPY(x) s->x = s1->x;
+#define COPYR(a, b) memcpy(>a, >a, ((uint8_t*)>b) - 
((uint8_t*)>a))
+COPYR(h263_aic, qscale);
+COPYR(lambda,   mv_dir);
+COPYR(no_rounding,  dest);
+COPYR(mb_index2xy,  resync_mb_x);
+COPYR(next_p_frame_damaged, h263_aic_dir);
+COPYR(h263_slice_structured,mcsel);
+COPYR(quant_precision,  first_slice_line);
+COPYR(flipflop_rounding,gb);
+COPYR(gop_picture_number,   picture_structure);
+COPYR(picture_structure,pblocks);
+COPYR(decode_mb,er);
+COPYR(error_rate,   noise_reduction);
 
 s->avctx = dst;
 s->bitstream_buffer  = NULL;
-- 
2.13.0

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avformat/avio: Remove no-op code in url_find_protocol().

2017-07-09 Thread Muhammad Faiz 
On Fri, Jul 7, 2017 at 1:32 PM, Muhammad Faiz  wrote:
> On Fri, Jul 7, 2017 at 11:27 AM, Wan-Teh Chang
>  wrote:
>> Hi Muhammad,
>>
>> On Thu, Jul 6, 2017 at 7:56 PM, Muhammad Faiz  wrote:
>>> On Fri, Jul 7, 2017 at 9:18 AM, Wan-Teh Chang
>>>  wrote:
 In url_find_protocol(), proto_str is either "file" or a string
 consisting of only the characters in URL_SCHEME_CHARS, which does not
 include ','. Therefore the strchr(proto_str, ',') call always returns
 NULL.

 Note: The code was added in commit
 6161c41817f6e53abb3021d67ca0f19def682718.

 Signed-off-by: Wan-Teh Chang 
 ---
  libavformat/avio.c | 2 --
  1 file changed, 2 deletions(-)

 diff --git a/libavformat/avio.c b/libavformat/avio.c
 index 1e79c9dd5c..64248e098b 100644
 --- a/libavformat/avio.c
 +++ b/libavformat/avio.c
 @@ -263,8 +263,6 @@ static const struct URLProtocol 
 *url_find_protocol(const char *filename)
  av_strlcpy(proto_str, filename,
 FFMIN(proto_len + 1, sizeof(proto_str)));

 -if ((ptr = strchr(proto_str, ',')))
 -*ptr = '\0';
>>>
>>> What about handling "subfile," ?
>>
>> I don't know what url_find_protocol() is intended to do, but I can
>> show you what it actually does.
>>
>> Here is the relevant code in libavformat/avio.c:
>>
>> ==
>> #define URL_SCHEME_CHARS\
>> "abcdefghijklmnopqrstuvwxyz"\
>> "ABCDEFGHIJKLMNOPQRSTUVWXYZ"\
>> "0123456789+-."
>>
>> static const struct URLProtocol *url_find_protocol(const char *filename)
>> {
>> const URLProtocol **protocols;
>> char proto_str[128], proto_nested[128], *ptr;
>> size_t proto_len = strspn(filename, URL_SCHEME_CHARS);
>> int i;
>>
>> if (filename[proto_len] != ':' &&
>> (strncmp(filename, "subfile,", 8) || !strchr(filename +
>> proto_len + 1, ':')) ||
>> is_dos_path(filename))
>> strcpy(proto_str, "file");
>> else
>> av_strlcpy(proto_str, filename,
>>FFMIN(proto_len + 1, sizeof(proto_str)));
>>
>> if ((ptr = strchr(proto_str, ',')))
>> *ptr = '\0';
>> ==
>>
>> Since I don't know how "subfile," should be handled by
>> url_find_protocol(), I ran the following three test inputs in the
>> debugger:
>>
>> If |filename| is "subfile,", then proto_len is 7 and the if branch
>> copies "file" into proto_str.
>>
>> If |filename| is "subfile,abcdefg", then proto_len is 7 and the if
>> branch copies "file" into proto_str.
>>
>> If |filename| is "subfile,abcdefg:hijk", then proto_len is 7 and the
>> else branch copies "subfile" into proto_str.
>
> Oh, I see. I was wrong.
>
>>
>> Is this the expected behavior?
>
> I don't know. However it is the previous behavior, so LGTM.
>
> Thank's.

Applied.

Thank's.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 3/3] avcodec/aacpsdsp_template: Fixes integer overflow in ps_add_squares_c()

2017-07-09 Thread Ivan Kalvachev 
On 7/9/17, Ronald S. Bultje  wrote:
> Hi,
>
> On Sun, Jul 9, 2017 at 4:39 AM, Reimar Döffinger 
> wrote:
>
>> On 09.07.2017, at 02:52, "Ronald S. Bultje"  wrote:
>> > On Sat, Jul 8, 2017 at 5:17 PM, Michael Niedermayer
>> 
>> > wrote:
>> >
>> >>
>> >> Does anyone object to this patch ?
>> >> Or does anyone have a better idea on how to fix this ?
>> >> if not id like to apply it
>> >
>> >
>> > I think Rostislav's point is: why fix it, if it can only happen with
>> > corrupt input? The before and after situation is identical: garbage in,
>> > garbage out. If the compiler does funny things that makes the garbage
>> > slightly differently bad, is that really so devilishly bad? It's still
>> > garbage. Is anything improved by this?
>>
>> The way C works, you MUST assume any undefined behaviour can at any point
>> [..] become exploitable.[..] If you don't like that, C is the wrong
>> language to use.
>
>
> I think I've read "the boy who cried wolf" a few too many times to my kids,
> but the form of this discussion is currently too polarizing/political for
> my taste.

So, you stir the pot and then run away.
You ignore technical explanations and you call discussion too political.
And you "imply" that developers are lairs.

Maybe you are not aware what you are going,
but I do assure you, it aint pretty.

I think you owe some people an apology.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] movenc: write clap tag

2017-07-09 Thread Dave Rice 

> On Jul 7, 2017, at 7:06 PM, Derek Buitenhuis  
> wrote:
> 
> On 7/7/2017 10:13 PM, James Almer wrote:
>> Isn't this necessary only for files with raw video? As is, this box
>> would be written on any mov file with a video stream.
> 
> This was addressed a previous email:
> 
>http://lists.ffmpeg.org/pipermail/ffmpeg-devel/2017-July/213350.html
> 
> I guess the spec is up for interpretation.

The quicktime spec says "This is a mandatory extension for all uncompressed 
Y´CbCr data formats”. It doesn’t clarify if the clap atom is recommended or not 
in mov files that are not “uncompressed Y´CbCr”, though it would make sense if 
the video container needs to store cropping data. I think constraining the 
change for only  “uncompressed Y´CbCr” would be more cautious though. I’ll 
revise my patch to include the condition and resubmit.

If the patch only impacts “uncompressed Y´CbCr” would any fate updates be 
needed?
Dave Rice
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] movenc: write clap tag

2017-07-09 Thread Dave Rice 

> On Jul 7, 2017, at 11:32 AM, Derek Buitenhuis  
> wrote:
> 
> On 7/7/2017 3:20 AM, Dave Rice wrote:
>> Resolves https://trac.ffmpeg.org/ticket/6145 and writes a clap atom that is 
>> coincident with the frames width and height.
>> 
>> 
>> From 23d80d0d47829fed61e817b1e7c3f6d420c9ab5c Mon Sep 17 00:00:00 2001
>> From: Dave Rice 
>> Date: Thu, 6 Jul 2017 21:12:38 -0400
>> Subject: [PATCH] movenc: write clap tag
>> 
>> ---
>> libavformat/movenc.c | 19 +++
>> 1 file changed, 19 insertions(+)
> 
> Whoever pushes, add the ticket number into the commit message.
> 
> [...]
> 
>> +avio_wb32(pb, 0); /* horizOff_N (= 0) */
>> +avio_wb32(pb, 1); /* horizOff_D (= 1) */
>> +avio_wb32(pb, 0); /* vertOff_N (= 0) */
>> +avio_wb32(pb, 1); /* vertOff_D (= 1) */
> 
> Near as I can tell, these related the active image area? Is it useful to ever 
> set
> this to anything but 0 (aka the whole image is active)?

Usually not a good idea to set a denominator to zero. The clap atom 
documentation at 
https://developer.apple.com/library/content/documentation/QuickTime/QTFF/QTFFChap3/qtff3.html#//apple_ref/doc/uid/TP4939-CH205-125850
 

 specifies zero for numerators and one for denominators as default when the 
ratio of an actual aperture is not used.

> Was thinking of stuff like
> analogue video, a la http://chromashift.org/aspectratio/. I assume the answer 
> is
> probably 'no’.

The atom is analogous to the PixelCrop* elements of Matroska. For instance a 
standard definition NTSC video may typically be digitized to a frame size of 
720x486 but the active image might only be 704x480, so the aperture atom could 
express the difference.

> Patch itself LGTM.

[…] 

Thanks,
Dave Rice
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH] web/template_head2: Add net neutrality script

2017-07-09 Thread Michael Niedermayer 
See: https://www.battleforthenet.com/july12/

Signed-off-by: Michael Niedermayer 
---
 src/template_head2 | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/template_head2 b/src/template_head2
index a0b11ab..d7bb7bd 100644
--- a/src/template_head2
+++ b/src/template_head2
@@ -3,6 +3,7 @@

[FFmpeg-devel] [PATCH] h264: Support multi-field closed captions by using AVBufferRef and not resetting per field

2017-07-09 Thread Kieran Kunhya 
$subj


0001-h264-Support-multi-field-closed-captions-by-using-AV.patch
Description: Binary data
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] Ticket 6375, Too many packets buffered for output stream

2017-07-09 Thread Reimar Döffinger 
On 09.07.2017, at 22:37, Hendrik Leppkes  wrote:

> 
> Either extremely badly intereleaved streams or streams that just start
> super late in the file have always been very problematic for decoding,
> resulting in various hacks over the years to add "best-guess" default
> values so at least something happens, even if the filtering has to
> resample/rescale to match that guess in the end.

Not sure if you're saying you want to avoid that?
My point was just, assuming it is even possible to have one frame for every 
stream is not really correct, and it's not JUST bad files.
But as an alternative suggestion, if you do not want obscure behaviour or 
hacks, maybe what really should happen is a better message?
Something like
"Stream A produced more data than could be buffered while waiting for first 
data on streams B, C and D. To fix this, please disable either stream A or 
streams B, C and D. Alternatively you can try to increase the buffer size (give 
option to increase)."
Another workaround might actually be to specify the input file multiple times 
(in the cases where that is possible), selecting different stream subsets each 
time.
Since disabling streams is of course not useful if the user really needs all of 
those (I don't think the ticket and current message has enough information to 
know if the problematic streams are actually useful).
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] Ticket 6375, Too many packets buffered for output stream

2017-07-09 Thread Hendrik Leppkes 
On Sun, Jul 9, 2017 at 1:38 PM, Reimar Döffinger
 wrote:
> On 09.07.2017, at 13:09, Michael Niedermayer  wrote:
>
>> Hi all
>>
>> It does appear this regression affects alot of people, judging from
>> the multiple different people in the ticket.
>> Also people ask me about this, for example baptiste yesterday hit it
>> too.
>>
>> I belive multiple people where in favour of the change that caused
>> this regression. Does someone who was in favor of this change have
>> time to fix this ticket ?
>>
>> I belive its important to fix this as it seems affecting many people.
>>
>> Thanks
>>
>> For reference:
>> Ticket: https://trac.ffmpeg.org/ticket/6375
>> Regressing Commit: 
>> https://github.com/FFmpeg/FFmpeg/commit/af1761f7b5b1b72197dc40934953b775c2d951cc
>
> Huh? I don't know if the commit message is accurate, but if it is the basic 
> concept of this patch is utterly broken and can never work.
> There can be hours of video data before you actually get a frame on one of 
> the "missing" streams (subtitles might be the most obvious case, but there 
> are others), and buffering that much data simply is not possible.

The same limitation has always existed, it just moved to a different
place in a different form. For that matter, its only used for video
and audio, subtitles don't have any filtering or even well-defined
codec parameters.
There are just a few concepts colliding here: Many output formats
require information what the streams are before we actually start
writing streams, so we need to see a frame for those streams.

On top of that, FFmpeg (the application) is also quite inflexibel for
any re-configuration, and since it didn't actually ask the decoder it
was going to use for its output information, there could be
mis-matching data, which was usually fixed by making avformat behave
just the way ffmpeg.c wanted it to, instead of just reporting neutral
stream data.
Having ffmpeg.c decode a frame and take the actual output format from
the decoder has solved a large variety of issues. It may not be
perfect (yet), but within the limitations of crazy media files and
generally conflicting paradigms in various media formats, I think its
still far better then before.

Either extremely badly intereleaved streams or streams that just start
super late in the file have always been very problematic for decoding,
resulting in various hacks over the years to add "best-guess" default
values so at least something happens, even if the filtering has to
resample/rescale to match that guess in the end.

> Though since it seems to cause issues with audio files with cover image 
> there's probably also bugs in the implementation itself, since handling those 
> correctly is entirely possible...

I agree that the cover art thing is likely just a bug since those
cover art streams don't behave like normal streams. I'm a bit
short-pressed on time these days, but if noone else looks into it, I
can give it a try later this week.

- Hendrik
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 3/3] avcodec/aacpsdsp_template: Fixes integer overflow in ps_add_squares_c()

2017-07-09 Thread Reimar Döffinger 
On 09.07.2017, at 16:08, "Ronald S. Bultje"  wrote:
> Hi,
> 
> On Sun, Jul 9, 2017 at 4:39 AM, Reimar Döffinger 
> wrote:
> 
>> On 09.07.2017, at 02:52, "Ronald S. Bultje"  wrote:
>>> On Sat, Jul 8, 2017 at 5:17 PM, Michael Niedermayer
>> 
>>> wrote:
>>> 
 
 Does anyone object to this patch ?
 Or does anyone have a better idea on how to fix this ?
 if not id like to apply it
>>> 
>>> 
>>> I think Rostislav's point is: why fix it, if it can only happen with
>>> corrupt input? The before and after situation is identical: garbage in,
>>> garbage out. If the compiler does funny things that makes the garbage
>>> slightly differently bad, is that really so devilishly bad? It's still
>>> garbage. Is anything improved by this?
>> 
>> The way C works, you MUST assume any undefined behaviour can at any point
>> [..] become exploitable.[..] If you don't like that, C is the wrong
>> language to use.
> 
> 
> I think I've read "the boy who cried wolf" a few too many times to my kids,
> but the form of this discussion is currently too polarizing/political for
> my taste.

That is my reading of the C standard, is that political or even just 
controversial?
I mean of course you can ignore standards (see MPEG-4 ASP, and in some ways 
that was actually fairly reasonable thing to do at the time), and I don't fix 
every undefined behaviour case in my code when I can't think of any reasonable 
solution.
So there is a cost-benefit discussion in principle.
I believe the cost of not fixing undefined behaviour, just by virtue of going 
outside what the standard guarantees should be considered fairly high.
That is an opinion, but is there any disagreement that undefined behaviour is 
at least an issue of some degree?
If we can agree on that, then the question would only be how much effort/code 
ugliness is reasonable.
There is also the point (which I hope I mentioned in the parts cut out) that 
just making sure that these cases are not already exploitable right now with 
the current compiler can in many cases be quite a pain (and does not have tool 
support), so I think fixing it would often be the lowest-effort method.
I'd also like to point out that even ignoring all that, ubsan + fuzzing is a 
powerful tool to improve code quality, and I would argue than at least some 
amount of code complexity is justified just for having them work well.
And it's also that to my mind the patch did not look THAT bad...
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] lavfi/buffersrc: push the frame deeper if requested.

2017-07-09 Thread Marton Balint 



On Sun, 9 Jul 2017, Paul B Mahol wrote:


On 7/9/17, Marton Balint  wrote:


On Sun, 9 Jul 2017, Paul B Mahol wrote:


On 7/9/17, Marton Balint  wrote:


On Sat, 24 Jun 2017, Nicolas George wrote:


Le sextidi 6 messidor, an CCXXV, Paul B Mahol a ecrit :

The reverted commit breaks lot of filters and I failed to find fix,
the only logical solution


It breaks a corner-case option of, indeed a lot of, filters. I have not
yet been able to track down the exact cause of the problem, I still have
all this in my work tree.



The revert itself broke a rather simple buffersrc->buffersink case as
well:

ffmpeg -f lavfi -i "testsrc[out0];sine[out1]" -bsf:a noise=dropamount=1
out.avi

ffmpeg is now consuming all memory, it seems video frames are stuck in
the
filter graph forever.

(the dropamount parameter to the noise bitstream filter was just
committed
to be able to drop all packets of a stream)


I believe you are referring to another commit which also needs to be
reverted.


No, definitely this one (04aa09c4bcf2d5a634a35da3a3ae3fc1abe30ef8). The
input frames are arriving just fine, so ffmpeg can initialize the
filtergraph, but only one video frame comes out of the filtergraph, the
others are stuck there forever.


Nope. Without that revert bunch of filters are simply broken.


I am not saying they aren't, I'm just showing a case where your revert is 
breaking another case which was working before. Nicolas also stated there 
can be cases where the old code (before the revert) worked better. I just 
hope that I found a simple enough case, so somebody can take a look and 
find the root cause.


Regards,
Marton
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] lavfi/buffersrc: push the frame deeper if requested.

2017-07-09 Thread Paul B Mahol 
On 7/9/17, Marton Balint  wrote:
>
> On Sun, 9 Jul 2017, Paul B Mahol wrote:
>
>> On 7/9/17, Marton Balint  wrote:
>>>
>>> On Sat, 24 Jun 2017, Nicolas George wrote:
>>>
 Le sextidi 6 messidor, an CCXXV, Paul B Mahol a ecrit :
> The reverted commit breaks lot of filters and I failed to find fix,
> the only logical solution

 It breaks a corner-case option of, indeed a lot of, filters. I have not
 yet been able to track down the exact cause of the problem, I still have
 all this in my work tree.

>>>
>>> The revert itself broke a rather simple buffersrc->buffersink case as
>>> well:
>>>
>>> ffmpeg -f lavfi -i "testsrc[out0];sine[out1]" -bsf:a noise=dropamount=1
>>> out.avi
>>>
>>> ffmpeg is now consuming all memory, it seems video frames are stuck in
>>> the
>>> filter graph forever.
>>>
>>> (the dropamount parameter to the noise bitstream filter was just
>>> committed
>>> to be able to drop all packets of a stream)
>>
>> I believe you are referring to another commit which also needs to be
>> reverted.
>
> No, definitely this one (04aa09c4bcf2d5a634a35da3a3ae3fc1abe30ef8). The
> input frames are arriving just fine, so ffmpeg can initialize the
> filtergraph, but only one video frame comes out of the filtergraph, the
> others are stuck there forever.

Nope. Without that revert bunch of filters are simply broken.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] lavfi/buffersrc: push the frame deeper if requested.

2017-07-09 Thread Marton Balint 


On Sun, 9 Jul 2017, Paul B Mahol wrote:


On 7/9/17, Marton Balint  wrote:


On Sat, 24 Jun 2017, Nicolas George wrote:


Le sextidi 6 messidor, an CCXXV, Paul B Mahol a ecrit :

The reverted commit breaks lot of filters and I failed to find fix,
the only logical solution


It breaks a corner-case option of, indeed a lot of, filters. I have not
yet been able to track down the exact cause of the problem, I still have
all this in my work tree.



The revert itself broke a rather simple buffersrc->buffersink case as
well:

ffmpeg -f lavfi -i "testsrc[out0];sine[out1]" -bsf:a noise=dropamount=1
out.avi

ffmpeg is now consuming all memory, it seems video frames are stuck in the
filter graph forever.

(the dropamount parameter to the noise bitstream filter was just committed
to be able to drop all packets of a stream)


I believe you are referring to another commit which also needs to be reverted.


No, definitely this one (04aa09c4bcf2d5a634a35da3a3ae3fc1abe30ef8). The 
input frames are arriving just fine, so ffmpeg can initialize the 
filtergraph, but only one video frame comes out of the filtergraph, the 
others are stuck there forever.


Regards,
Marton
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH] avfilter: add ADM filter

2017-07-09 Thread Ashish Singh 
Hi, this is ADM filter also known as DLM (Detail loss metric), one of the
component filters of VMAF. It takes two videos as input.
Run it using: ffmpeg -i main -i ref -lavfi adm -f null -
Currently it outputs the average adm score over all frames.

---
 Changelog|   1 +
 doc/filters.texi |  19 ++
 libavfilter/Makefile |   1 +
 libavfilter/adm.h|  30 ++
 libavfilter/allfilters.c |   1 +
 libavfilter/vf_adm.c | 832 +++
 6 files changed, 884 insertions(+)
 create mode 100644 libavfilter/adm.h
 create mode 100644 libavfilter/vf_adm.c

diff --git a/Changelog b/Changelog
index 1778980..ec67228 100644
--- a/Changelog
+++ b/Changelog
@@ -10,6 +10,7 @@ version :
 - config.log and other configuration files moved into ffbuild/ directory
 - update cuvid/nvenc headers to Video Codec SDK 8.0.14
 - afir audio filter
+- adm video filter
 
 version 3.3:
 - CrystalHD decoder moved to new decode API
diff --git a/doc/filters.texi b/doc/filters.texi
index 5985db6..b63edf0 100644
--- a/doc/filters.texi
+++ b/doc/filters.texi
@@ -4394,6 +4394,25 @@ build.
 
 Below is a description of the currently available video filters.
 
+@section adm
+
+Obtain the average ADM/DLM (Detail Loss Metric) between two input videos.
+
+This filter takes two input videos.
+
+Both input videos must have the same resolution and pixel format for
+this filter to work correctly. Also it assumes that both inputs
+have the same number of frames, which are compared one by one.
+
+The obtained average ADM is printed through the logging system.
+
+In the below example the input file @file{main.mpg} being processed is compared
+with the reference file @file{ref.mpg}.
+
+@example
+ffmpeg -i main.mpg -i ref.mpg -lavfi adm -f null -
+@end example
+
 @section alphaextract
 
 Extract the alpha component from the input as a grayscale video. This
diff --git a/libavfilter/Makefile b/libavfilter/Makefile
index f7dfe8a..abce480 100644
--- a/libavfilter/Makefile
+++ b/libavfilter/Makefile
@@ -122,6 +122,7 @@ OBJS-$(CONFIG_SINE_FILTER)   += asrc_sine.o
 OBJS-$(CONFIG_ANULLSINK_FILTER)  += asink_anullsink.o
 
 # video filters
+OBJS-$(CONFIG_ADM_FILTER)+= vf_adm.o dualinput.o 
framesync.o
 OBJS-$(CONFIG_ALPHAEXTRACT_FILTER)   += vf_extractplanes.o
 OBJS-$(CONFIG_ALPHAMERGE_FILTER) += vf_alphamerge.o
 OBJS-$(CONFIG_ASS_FILTER)+= vf_subtitles.o
diff --git a/libavfilter/adm.h b/libavfilter/adm.h
new file mode 100644
index 000..4735d53
--- /dev/null
+++ b/libavfilter/adm.h
@@ -0,0 +1,30 @@
+/*
+ * Copyright (c) 2017 Ronald S. Bultje 
+ * Copyright (c) 2017 Ashish Pratap Singh 
+ *
+ * This file is part of FFmpeg.
+ *
+ * FFmpeg is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 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
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser 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
+ */
+
+#ifndef AVFILTER_ADM_H
+#define AVFILTER_ADM_H
+
+int compute_adm(const float *ref, const float *dis, int w, int h,
+int ref_stride, int dis_stride, double *score,
+double *score_num, double *score_den, double *scores,
+double border_factor, void *s);
+
+#endif /* AVFILTER_ADM_H */
diff --git a/libavfilter/allfilters.c b/libavfilter/allfilters.c
index cd35ae4..80e523b 100644
--- a/libavfilter/allfilters.c
+++ b/libavfilter/allfilters.c
@@ -134,6 +134,7 @@ static void register_all(void)
 
 REGISTER_FILTER(ANULLSINK,  anullsink,  asink);
 
+REGISTER_FILTER(ADM,adm,vf);
 REGISTER_FILTER(ALPHAEXTRACT,   alphaextract,   vf);
 REGISTER_FILTER(ALPHAMERGE, alphamerge, vf);
 REGISTER_FILTER(ASS,ass,vf);
diff --git a/libavfilter/vf_adm.c b/libavfilter/vf_adm.c
new file mode 100644
index 000..cbce05e
--- /dev/null
+++ b/libavfilter/vf_adm.c
@@ -0,0 +1,832 @@
+/*
+ * Copyright (c) 2017 Ronald S. Bultje 
+ * Copyright (c) 2017 Ashish Pratap Singh 
+ *
+ * This file is part of FFmpeg.
+ *
+ * FFmpeg is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * FFmpeg is

Re: [FFmpeg-devel] [PATCH] lavfi/buffersrc: push the frame deeper if requested.

2017-07-09 Thread Paul B Mahol 
On 7/9/17, Marton Balint  wrote:
>
> On Sat, 24 Jun 2017, Nicolas George wrote:
>
>> Le sextidi 6 messidor, an CCXXV, Paul B Mahol a ecrit :
>>> The reverted commit breaks lot of filters and I failed to find fix,
>>> the only logical solution
>>
>> It breaks a corner-case option of, indeed a lot of, filters. I have not
>> yet been able to track down the exact cause of the problem, I still have
>> all this in my work tree.
>>
>
> The revert itself broke a rather simple buffersrc->buffersink case as
> well:
>
> ffmpeg -f lavfi -i "testsrc[out0];sine[out1]" -bsf:a noise=dropamount=1
> out.avi
>
> ffmpeg is now consuming all memory, it seems video frames are stuck in the
> filter graph forever.
>
> (the dropamount parameter to the noise bitstream filter was just committed
> to be able to drop all packets of a stream)

I believe you are referring to another commit which also needs to be reverted.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] lavfi/buffersrc: push the frame deeper if requested.

2017-07-09 Thread Marton Balint 


On Sat, 24 Jun 2017, Nicolas George wrote:


Le sextidi 6 messidor, an CCXXV, Paul B Mahol a écrit :

The reverted commit breaks lot of filters and I failed to find fix,
the only logical solution


It breaks a corner-case option of, indeed a lot of, filters. I have not
yet been able to track down the exact cause of the problem, I still have
all this in my work tree.



The revert itself broke a rather simple buffersrc->buffersink case as 
well:


ffmpeg -f lavfi -i "testsrc[out0];sine[out1]" -bsf:a noise=dropamount=1 out.avi

ffmpeg is now consuming all memory, it seems video frames are stuck in the 
filter graph forever.


(the dropamount parameter to the noise bitstream filter was just committed 
to be able to drop all packets of a stream)


Regards,
Marton
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avcodec/noise_bsf: add support for dropping packets

2017-07-09 Thread Marton Balint 


On Sat, 8 Jul 2017, Michael Niedermayer wrote:


On Sat, Jul 08, 2017 at 01:21:32PM +0200, Marton Balint wrote:

Signed-off-by: Marton Balint 
---
 doc/bitstream_filters.texi | 14 +++---
 libavcodec/noise_bsf.c |  8 
 libavcodec/version.h   |  2 +-
 3 files changed, 20 insertions(+), 4 deletions(-)


should be ok


Thanks, applied.



supporting duplicatig random previous packets might be interresting
too

thx

PS: these should also be tested in fate


I agree :). Will put it on my todo list...

Regards,
Marton
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH] avfilter: add ANSNR filter

2017-07-09 Thread Ashish Singh 
---
 Changelog|   1 +
 doc/filters.texi |  20 +++
 libavfilter/Makefile |   1 +
 libavfilter/allfilters.c |   1 +
 libavfilter/ansnr.h  |  29 
 libavfilter/vf_ansnr.c   | 418 +++
 6 files changed, 470 insertions(+)
 create mode 100644 libavfilter/ansnr.h
 create mode 100644 libavfilter/vf_ansnr.c

diff --git a/Changelog b/Changelog
index 1778980..bfe848a 100644
--- a/Changelog
+++ b/Changelog
@@ -10,6 +10,7 @@ version :
 - config.log and other configuration files moved into ffbuild/ directory
 - update cuvid/nvenc headers to Video Codec SDK 8.0.14
 - afir audio filter
+- ansnr video filter
 
 version 3.3:
 - CrystalHD decoder moved to new decode API
diff --git a/doc/filters.texi b/doc/filters.texi
index 5985db6..7a0856b 100644
--- a/doc/filters.texi
+++ b/doc/filters.texi
@@ -4419,6 +4419,26 @@ input reaches end of stream. This will cause problems if 
your encoding
 pipeline drops frames. If you're trying to apply an image as an
 overlay to a video stream, consider the @var{overlay} filter instead.
 
+@section ansnr
+
+Obtain the average ANSNR (Anti-Noise Signal to Noise
+Ratio) between two input videos.
+
+This filter takes in input two input videos.
+
+Both video inputs must have the same resolution and pixel format for
+this filter to work correctly. Also it assumes that both inputs
+have the same number of frames, which are compared one by one.
+
+The obtained average ANSNR is printed through the logging system.
+
+In the below example the input file @file{main.mpg} being processed is compared
+with the reference file @file{ref.mpg}.
+
+@example
+ffmpeg -i main.mpg -i ref.mpg -lavfi ansnr -f null -
+@end example
+
 @section ass
 
 Same as the @ref{subtitles} filter, except that it doesn't require libavcodec
diff --git a/libavfilter/Makefile b/libavfilter/Makefile
index f7dfe8a..705e5a1 100644
--- a/libavfilter/Makefile
+++ b/libavfilter/Makefile
@@ -124,6 +124,7 @@ OBJS-$(CONFIG_ANULLSINK_FILTER)  += 
asink_anullsink.o
 # video filters
 OBJS-$(CONFIG_ALPHAEXTRACT_FILTER)   += vf_extractplanes.o
 OBJS-$(CONFIG_ALPHAMERGE_FILTER) += vf_alphamerge.o
+OBJS-$(CONFIG_ANSNR_FILTER)  += vf_ansnr.o dualinput.o 
framesync.o
 OBJS-$(CONFIG_ASS_FILTER)+= vf_subtitles.o
 OBJS-$(CONFIG_ATADENOISE_FILTER) += vf_atadenoise.o
 OBJS-$(CONFIG_AVGBLUR_FILTER)+= vf_avgblur.o
diff --git a/libavfilter/allfilters.c b/libavfilter/allfilters.c
index cd35ae4..c1f67c4 100644
--- a/libavfilter/allfilters.c
+++ b/libavfilter/allfilters.c
@@ -136,6 +136,7 @@ static void register_all(void)
 
 REGISTER_FILTER(ALPHAEXTRACT,   alphaextract,   vf);
 REGISTER_FILTER(ALPHAMERGE, alphamerge, vf);
+REGISTER_FILTER(ANSNR,  ansnr,  vf);
 REGISTER_FILTER(ASS,ass,vf);
 REGISTER_FILTER(ATADENOISE, atadenoise, vf);
 REGISTER_FILTER(AVGBLUR,avgblur,vf);
diff --git a/libavfilter/ansnr.h b/libavfilter/ansnr.h
new file mode 100644
index 000..44fb3ba
--- /dev/null
+++ b/libavfilter/ansnr.h
@@ -0,0 +1,29 @@
+/*
+ * Copyright (c) 2017 Ronald S. Bultje 
+ * Copyright (c) 2017 Ashish Pratap Singh 
+ *
+ * This file is part of FFmpeg.
+ *
+ * FFmpeg is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 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
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser 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
+ */
+
+#ifndef AVFILTER_ANSNR_H
+#define AVFILTER_ANSNR_H
+
+static int compute_ansnr(const void *ref, const void *dis, int w,
+ int h, int ref_stride, int dis_stride, double *score,
+ double *score_psnr, double peak, double psnr_max, 
void *ctx);
+
+#endif /* AVFILTER_ANSNR_H */
diff --git a/libavfilter/vf_ansnr.c b/libavfilter/vf_ansnr.c
new file mode 100644
index 000..e11289b
--- /dev/null
+++ b/libavfilter/vf_ansnr.c
@@ -0,0 +1,418 @@
+/*
+ * Copyright (c) 2017 Ronald S. Bultje 
+ * Copyright (c) 2017 Ashish Pratap Singh 
+ *
+ * This file is part of FFmpeg.
+ *
+ * FFmpeg is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your

[FFmpeg-devel] [PATCH] avfilter: add ANSNR filter

2017-07-09 Thread Ashish Singh 
Hi, added metadata scores and changed multipe string comparisons to few integer 
comparisons.

---
 Changelog|   1 +
 doc/filters.texi |  20 +++
 libavfilter/Makefile |   1 +
 libavfilter/allfilters.c |   1 +
 libavfilter/ansnr.h  |  29 
 libavfilter/vf_ansnr.c   | 416 +++
 6 files changed, 468 insertions(+)
 create mode 100644 libavfilter/ansnr.h
 create mode 100644 libavfilter/vf_ansnr.c

diff --git a/Changelog b/Changelog
index 1778980..bfe848a 100644
--- a/Changelog
+++ b/Changelog
@@ -10,6 +10,7 @@ version :
 - config.log and other configuration files moved into ffbuild/ directory
 - update cuvid/nvenc headers to Video Codec SDK 8.0.14
 - afir audio filter
+- ansnr video filter
 
 version 3.3:
 - CrystalHD decoder moved to new decode API
diff --git a/doc/filters.texi b/doc/filters.texi
index 5985db6..7a0856b 100644
--- a/doc/filters.texi
+++ b/doc/filters.texi
@@ -4419,6 +4419,26 @@ input reaches end of stream. This will cause problems if 
your encoding
 pipeline drops frames. If you're trying to apply an image as an
 overlay to a video stream, consider the @var{overlay} filter instead.
 
+@section ansnr
+
+Obtain the average ANSNR (Anti-Noise Signal to Noise
+Ratio) between two input videos.
+
+This filter takes in input two input videos.
+
+Both video inputs must have the same resolution and pixel format for
+this filter to work correctly. Also it assumes that both inputs
+have the same number of frames, which are compared one by one.
+
+The obtained average ANSNR is printed through the logging system.
+
+In the below example the input file @file{main.mpg} being processed is compared
+with the reference file @file{ref.mpg}.
+
+@example
+ffmpeg -i main.mpg -i ref.mpg -lavfi ansnr -f null -
+@end example
+
 @section ass
 
 Same as the @ref{subtitles} filter, except that it doesn't require libavcodec
diff --git a/libavfilter/Makefile b/libavfilter/Makefile
index f7dfe8a..705e5a1 100644
--- a/libavfilter/Makefile
+++ b/libavfilter/Makefile
@@ -124,6 +124,7 @@ OBJS-$(CONFIG_ANULLSINK_FILTER)  += 
asink_anullsink.o
 # video filters
 OBJS-$(CONFIG_ALPHAEXTRACT_FILTER)   += vf_extractplanes.o
 OBJS-$(CONFIG_ALPHAMERGE_FILTER) += vf_alphamerge.o
+OBJS-$(CONFIG_ANSNR_FILTER)  += vf_ansnr.o dualinput.o 
framesync.o
 OBJS-$(CONFIG_ASS_FILTER)+= vf_subtitles.o
 OBJS-$(CONFIG_ATADENOISE_FILTER) += vf_atadenoise.o
 OBJS-$(CONFIG_AVGBLUR_FILTER)+= vf_avgblur.o
diff --git a/libavfilter/allfilters.c b/libavfilter/allfilters.c
index cd35ae4..c1f67c4 100644
--- a/libavfilter/allfilters.c
+++ b/libavfilter/allfilters.c
@@ -136,6 +136,7 @@ static void register_all(void)
 
 REGISTER_FILTER(ALPHAEXTRACT,   alphaextract,   vf);
 REGISTER_FILTER(ALPHAMERGE, alphamerge, vf);
+REGISTER_FILTER(ANSNR,  ansnr,  vf);
 REGISTER_FILTER(ASS,ass,vf);
 REGISTER_FILTER(ATADENOISE, atadenoise, vf);
 REGISTER_FILTER(AVGBLUR,avgblur,vf);
diff --git a/libavfilter/ansnr.h b/libavfilter/ansnr.h
new file mode 100644
index 000..44fb3ba
--- /dev/null
+++ b/libavfilter/ansnr.h
@@ -0,0 +1,29 @@
+/*
+ * Copyright (c) 2017 Ronald S. Bultje 
+ * Copyright (c) 2017 Ashish Pratap Singh 
+ *
+ * This file is part of FFmpeg.
+ *
+ * FFmpeg is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 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
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser 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
+ */
+
+#ifndef AVFILTER_ANSNR_H
+#define AVFILTER_ANSNR_H
+
+static int compute_ansnr(const void *ref, const void *dis, int w,
+ int h, int ref_stride, int dis_stride, double *score,
+ double *score_psnr, double peak, double psnr_max, 
void *ctx);
+
+#endif /* AVFILTER_ANSNR_H */
diff --git a/libavfilter/vf_ansnr.c b/libavfilter/vf_ansnr.c
new file mode 100644
index 000..78c71e1
--- /dev/null
+++ b/libavfilter/vf_ansnr.c
@@ -0,0 +1,416 @@
+/*
+ * Copyright (c) 2017 Ronald S. Bultje 
+ * Copyright (c) 2017 Ashish Pratap Singh 
+ *
+ * This file is part of FFmpeg.
+ *
+ * FFmpeg is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as

Re: [FFmpeg-devel] [PATCH 3/3] avcodec/aacpsdsp_template: Fixes integer overflow in ps_add_squares_c()

2017-07-09 Thread Michael Niedermayer 
On Sun, Jul 09, 2017 at 10:08:50AM -0400, Ronald S. Bultje wrote:
> Hi,
> 
> On Sun, Jul 9, 2017 at 4:39 AM, Reimar Döffinger 
> wrote:
> 
> > On 09.07.2017, at 02:52, "Ronald S. Bultje"  wrote:
> > > On Sat, Jul 8, 2017 at 5:17 PM, Michael Niedermayer
> > 
> > > wrote:
> > >
> > >>
> > >> Does anyone object to this patch ?
> > >> Or does anyone have a better idea on how to fix this ?
> > >> if not id like to apply it
> > >
> > >
> > > I think Rostislav's point is: why fix it, if it can only happen with
> > > corrupt input? The before and after situation is identical: garbage in,
> > > garbage out. If the compiler does funny things that makes the garbage
> > > slightly differently bad, is that really so devilishly bad? It's still
> > > garbage. Is anything improved by this?
> >
> > The way C works, you MUST assume any undefined behaviour can at any point
> > [..] become exploitable.[..] If you don't like that, C is the wrong
> > language to use.
> 
> 
> I think I've read "the boy who cried wolf" a few too many times to my kids,
> but the form of this discussion is currently too polarizing/political for
> my taste.

I dont know about polarizing, it quite possibly is but
If anyone belives this is just political or theoretical and its safe to
trigger undefined behavior if the output is not used, here are some
examples found after 30sec of google use where that is not so:
https://blogs.msdn.microsoft.com/oldnewthing/20140627-00/?p=633
https://blog.regehr.org/archives/759

This is intended "for the archives" and not intended to pull
anyone into a discussion they dont want to be in.

[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Why not whip the teacher when the pupil misbehaves? -- Diogenes of Sinope


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avcodec/pthread_slice: rewrite implementation

2017-07-09 Thread Muhammad Faiz 
On Sun, Jul 9, 2017 at 6:45 PM, Ronald S. Bultje  wrote:
> Hi,
>
> On Sun, Jul 9, 2017 at 6:27 AM, Muhammad Faiz  wrote:
>>
>> Avoid pthread_cond_broadcast that wakes up all workers. Make each of them
>> uses distict mutex/cond. Also let main thread help running jobs.
>
>
> Hm... I'm not against this, but I actually had plans to write some
> extensions and allow the main thread to do something else (this is probably
> fairly vp9-specific), and this would prevent that. In vp9, specifically for
> tile decoding, I'd like to use the main thread for 1 thing and the worker
> threads for other things. Maybe we can make this configurable?

Post a new patch to address this issue and fix fate failure.

Thank's
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH] avcodec/aacps (fixed point): Fix multiple signed integer overflows

2017-07-09 Thread Michael Niedermayer 
Fixes: runtime error: signed integer overflow: 1421978265 - -1810326882 cannot 
be represented in type 'int'
Fixes: 2527/clusterfuzz-testcase-minimized-5260915396050944

Found-by: continuous fuzzing process 
https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer 
---
 libavcodec/aacps.c | 25 -
 1 file changed, 8 insertions(+), 17 deletions(-)

diff --git a/libavcodec/aacps.c b/libavcodec/aacps.c
index 473da7bd43..5758b919a1 100644
--- a/libavcodec/aacps.c
+++ b/libavcodec/aacps.c
@@ -697,26 +697,17 @@ static void decorrelation(PSContext *ps, INTFLOAT 
(*out)[32][2], const INTFLOAT
 for (i = 0; i < NR_PAR_BANDS[is34]; i++) {
 for (n = n0; n < nL; n++) {
 int decayed_peak;
-int denom;
-
 decayed_peak = (int)(((int64_t)peak_decay_factor * \
peak_decay_nrg[i] + 0x4000) >> 
31);
 peak_decay_nrg[i] = FFMAX(decayed_peak, power[i][n]);
-power_smooth[i] += (power[i][n] - power_smooth[i] + 2) >> 2;
-peak_decay_diff_smooth[i] += (peak_decay_nrg[i] - power[i][n] - \
-  peak_decay_diff_smooth[i] + 2) >> 2;
-denom = peak_decay_diff_smooth[i] + (peak_decay_diff_smooth[i] >> 
1);
-if (denom > power_smooth[i]) {
-  int p = power_smooth[i];
-  while (denom < 0x4000) {
-denom <<= 1;
-p <<= 1;
-  }
-  transient_gain[i][n] = p / (denom >> 16);
-}
-else {
-  transient_gain[i][n] = 1 << 16;
-}
+power_smooth[i] += (power[i][n] + 2LL - power_smooth[i]) >> 2;
+peak_decay_diff_smooth[i] += (peak_decay_nrg[i] + 2LL - 
power[i][n] - \
+  peak_decay_diff_smooth[i]) >> 2;
+
+if (peak_decay_diff_smooth[i]) {
+transient_gain[i][n] = FFMIN(power_smooth[i]*43691LL / 
peak_decay_diff_smooth[i], 1<<16);
+} else
+transient_gain[i][n] = 1 << 16;
 }
 }
 #else
-- 
2.13.0

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH v2] avcodec/pthread_slice: rewrite implementation

2017-07-09 Thread Muhammad Faiz 
Avoid pthread_cond_broadcast that wakes up all workers. Make each of them
uses distict mutex/cond. Also let main thread help running jobs, but still
allocate thread_count workers. The last worker is currently unused, emulated
by main thread.
Similar to 'avfilter/pthread: rewrite implementation'

Benchmark on x86_64 with 4 cpus (2 cores x 2 hyperthread)
./ffmpeg -threads $threads -thread_type slice -i 10slices.mp4 -f rawvideo -y 
/dev/null
threads=2:
old: 1m15.888s
new:  1m5.710s
threads=3:
old:  1m6.480s
new:  1m5.260s
threads=4:
old:  1m2.292s
new:   59.677s
threads=5:
old:   58.939s
new:   55.166s

Signed-off-by: Muhammad Faiz 
---
 libavcodec/pthread_slice.c | 219 +
 1 file changed, 142 insertions(+), 77 deletions(-)

diff --git a/libavcodec/pthread_slice.c b/libavcodec/pthread_slice.c
index 60f5b78..7223205 100644
--- a/libavcodec/pthread_slice.c
+++ b/libavcodec/pthread_slice.c
@@ -22,6 +22,7 @@
  * @see doc/multithreading.txt
  */
 
+#include 
 #include "config.h"
 
 #include "avcodec.h"
@@ -38,21 +39,26 @@
 typedef int (action_func)(AVCodecContext *c, void *arg);
 typedef int (action_func2)(AVCodecContext *c, void *arg, int jobnr, int 
threadnr);
 
+typedef struct WorkerContext WorkerContext;
+
 typedef struct SliceThreadContext {
-pthread_t *workers;
+AVCodecContext *avctx;
+WorkerContext *workers;
 action_func *func;
 action_func2 *func2;
 void *args;
 int *rets;
-int job_count;
-int job_size;
-
-pthread_cond_t last_job_cond;
-pthread_cond_t current_job_cond;
-pthread_mutex_t current_job_lock;
-unsigned current_execute;
-int current_job;
+unsigned job_count;
+unsigned job_size;
+
+pthread_mutex_t mutex_user;
+pthread_mutex_t mutex_done;
+pthread_cond_t cond_done;
+atomic_uint first_job;
+atomic_uint current_job;
+atomic_uint nb_finished_jobs;
 int done;
+int worker_done;
 
 int *entries;
 int entries_count;
@@ -61,42 +67,55 @@ typedef struct SliceThreadContext {
 pthread_mutex_t *progress_mutex;
 } SliceThreadContext;
 
-static void* attribute_align_arg worker(void *v)
+struct WorkerContext {
+SliceThreadContext  *ctx;
+pthread_t   thread;
+pthread_mutex_t mutex;
+pthread_cond_t  cond;
+int done;
+};
+
+static unsigned run_jobs(SliceThreadContext *c)
 {
-AVCodecContext *avctx = v;
-SliceThreadContext *c = avctx->internal->thread_ctx;
-unsigned last_execute = 0;
-int our_job = c->job_count;
-int thread_count = avctx->thread_count;
-int self_id;
-
-pthread_mutex_lock(>current_job_lock);
-self_id = c->current_job++;
-for (;;){
-int ret;
-while (our_job >= c->job_count) {
-if (c->current_job == thread_count + c->job_count)
-pthread_cond_signal(>last_job_cond);
-
-while (last_execute == c->current_execute && !c->done)
-pthread_cond_wait(>current_job_cond, >current_job_lock);
-last_execute = c->current_execute;
-our_job = self_id;
-
-if (c->done) {
-pthread_mutex_unlock(>current_job_lock);
-return NULL;
-}
-}
-pthread_mutex_unlock(>current_job_lock);
+unsigned current_job, first_job, nb_finished_jobs;
+
+current_job = first_job = atomic_fetch_add_explicit(>first_job, 1, 
memory_order_acq_rel);
 
-ret = c->func ? c->func(avctx, (char*)c->args + our_job*c->job_size):
-c->func2(avctx, c->args, our_job, self_id);
+do {
+int ret = c->func ? c->func(c->avctx, (char *)c->args + current_job * 
(size_t) c->job_size)
+  : c->func2(c->avctx, c->args, current_job, 
first_job);
 if (c->rets)
-c->rets[our_job%c->job_count] = ret;
+c->rets[current_job] = ret;
+nb_finished_jobs = atomic_fetch_add_explicit(>nb_finished_jobs, 1, 
memory_order_relaxed) + 1;
+} while ((current_job = atomic_fetch_add_explicit(>current_job, 1, 
memory_order_acq_rel)) < c->job_count);
 
-pthread_mutex_lock(>current_job_lock);
-our_job = c->current_job++;
+return nb_finished_jobs;
+}
+
+static void* attribute_align_arg worker(void *v)
+{
+WorkerContext *w = v;
+SliceThreadContext *c = w->ctx;
+
+pthread_mutex_lock(>mutex);
+pthread_cond_signal(>cond);
+
+while (1) {
+w->done = 1;
+while (w->done)
+pthread_cond_wait(>cond, >mutex);
+
+if (c->done) {
+pthread_mutex_unlock(>mutex);
+return NULL;
+}
+
+if (run_jobs(c) == c->job_count) {
+pthread_mutex_lock(>mutex_done);
+c->worker_done = 1;
+pthread_cond_signal(>cond_done);
+pthread_mutex_unlock(>mutex_done);
+}
 }
 }
 
@@ -105,24 +124,36 @@ void

Re: [FFmpeg-devel] [PATCH 3/3] avcodec/aacpsdsp_template: Fixes integer overflow in ps_add_squares_c()

2017-07-09 Thread Ronald S. Bultje 
Hi,

On Sun, Jul 9, 2017 at 4:39 AM, Reimar Döffinger 
wrote:

> On 09.07.2017, at 02:52, "Ronald S. Bultje"  wrote:
> > On Sat, Jul 8, 2017 at 5:17 PM, Michael Niedermayer
> 
> > wrote:
> >
> >>
> >> Does anyone object to this patch ?
> >> Or does anyone have a better idea on how to fix this ?
> >> if not id like to apply it
> >
> >
> > I think Rostislav's point is: why fix it, if it can only happen with
> > corrupt input? The before and after situation is identical: garbage in,
> > garbage out. If the compiler does funny things that makes the garbage
> > slightly differently bad, is that really so devilishly bad? It's still
> > garbage. Is anything improved by this?
>
> The way C works, you MUST assume any undefined behaviour can at any point
> [..] become exploitable.[..] If you don't like that, C is the wrong
> language to use.


I think I've read "the boy who cried wolf" a few too many times to my kids,
but the form of this discussion is currently too polarizing/political for
my taste.

Ronald
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avcodec/htmlsubtitles: Be a bit more picky on syntax

2017-07-09 Thread Clément Bœsch 
On Sun, Jul 09, 2017 at 02:28:28PM +0200, Clément Bœsch wrote:
[...]
> 544
> 00:53:43,780 --> 00:53:45,941
> <> - he calmed himself.
> 

Side note here: "<<" and ">>" are used for « and ». It's important to not
break this case IMO.

> 
> 545
> 00:53:46,216 --> 00:53:49,083
> < the hotel is within easy reach.
> 

Same here, the confusion between 'l' and 'I' makes me think it's the
result of an OCR, so the use of these ascii quotation should be pretty
common.

-- 
Clément B.


signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avcodec/htmlsubtitles: Be a bit more picky on syntax

2017-07-09 Thread Clément Bœsch 
On Sat, Jul 08, 2017 at 11:07:48PM +0200, Michael Niedermayer wrote:
[...]
> 5 days passed with no comments and no suggestions on how to implement
> this better.
> does anyone object to the fix as in the patch to be applied ?

misc patterns I found you may want to give a try (note that these are
completely broken garbage you may want to ignore):

82
00:04:50,430 --> 00:05:01,030

www.SeriesSub.com>


57
00:03:38,318 --> 00:03:42,778
<>
<>


2
00:01:10,000 --> 00:01:14,500
>>> RebelSubTeam <<<


306
00:20:31,849 --> 00:20:56,839

<<<>>>


37
00:02:37,750 --> 00:02:43,700
~ASUKO MARCH!~
>>:<<
translation by: cangii
Retiming by: furransu


1214
02:11:00,738 --> 02:11:05,538
<<< ÓÍÀÊÑ ÒÈÉÌ < 2015 > UNACS TEAM >>>


1
00:00:01,000 --> 00:00:02,308
<< Antes <<
Ela é hum...


544
00:53:43,780 --> 00:53:45,941
<> - he calmed himself.


545
00:53:46,216 --> 00:53:49,083
< 00:38:01,591
mint asztalt foglaltatni
a <<>Le Cirque-ben.


I really have a LOT of matches, so these are very common patterns.

Feel free to apply your patch if it generally doesn't make these cases
worse.

Regards,

-- 
Clément B.


signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avcodec/pthread_slice: rewrite implementation

2017-07-09 Thread Ronald S. Bultje 
Hi,

On Sun, Jul 9, 2017 at 6:27 AM, Muhammad Faiz  wrote:

> Avoid pthread_cond_broadcast that wakes up all workers. Make each of them
> uses distict mutex/cond. Also let main thread help running jobs.


Hm... I'm not against this, but I actually had plans to write some
extensions and allow the main thread to do something else (this is probably
fairly vp9-specific), and this would prevent that. In vp9, specifically for
tile decoding, I'd like to use the main thread for 1 thing and the worker
threads for other things. Maybe we can make this configurable?

Ronald
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] Ticket 6375, Too many packets buffered for output stream

2017-07-09 Thread Reimar Döffinger 
On 09.07.2017, at 13:09, Michael Niedermayer  wrote:

> Hi all
> 
> It does appear this regression affects alot of people, judging from
> the multiple different people in the ticket.
> Also people ask me about this, for example baptiste yesterday hit it
> too.
> 
> I belive multiple people where in favour of the change that caused
> this regression. Does someone who was in favor of this change have
> time to fix this ticket ?
> 
> I belive its important to fix this as it seems affecting many people.
> 
> Thanks
> 
> For reference:
> Ticket: https://trac.ffmpeg.org/ticket/6375
> Regressing Commit: 
> https://github.com/FFmpeg/FFmpeg/commit/af1761f7b5b1b72197dc40934953b775c2d951cc

Huh? I don't know if the commit message is accurate, but if it is the basic 
concept of this patch is utterly broken and can never work.
There can be hours of video data before you actually get a frame on one of the 
"missing" streams (subtitles might be the most obvious case, but there are 
others), and buffering that much data simply is not possible.
You can do something like it if instead of failing when the buffering limit is 
reached you then force stream processing of what is available, which is kind of 
a compromise that usually works well but also makes things a bit unpredictable.
Though since it seems to cause issues with audio files with cover image there's 
probably also bugs in the implementation itself, since handling those correctly 
is entirely possible...
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] Ticket 6375, Too many packets buffered for output stream

2017-07-09 Thread Michael Niedermayer 
Hi all

It does appear this regression affects alot of people, judging from
the multiple different people in the ticket.
Also people ask me about this, for example baptiste yesterday hit it
too.

I belive multiple people where in favour of the change that caused
this regression. Does someone who was in favor of this change have
time to fix this ticket ?

I belive its important to fix this as it seems affecting many people.

Thanks

For reference:
Ticket: https://trac.ffmpeg.org/ticket/6375
Regressing Commit: 
https://github.com/FFmpeg/FFmpeg/commit/af1761f7b5b1b72197dc40934953b775c2d951cc
Request for more time at the time when the change was pushed:
http://lists.ffmpeg.org/pipermail/ffmpeg-devel/2017-March/207775.html

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Into a blind darkness they enter who follow after the Ignorance,
they as if into a greater darkness enter who devote themselves
to the Knowledge alone. -- Isha Upanishad


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH] avcodec/pthread_slice: rewrite implementation

2017-07-09 Thread Muhammad Faiz 
Avoid pthread_cond_broadcast that wakes up all workers. Make each of them
uses distict mutex/cond. Also let main thread help running jobs.
Similar to 'avfilter/pthread: rewrite implementation'

Benchmark on x86_64 with 4 cpus (2 cores x 2 hyperthread)
./ffmpeg -threads $threads -thread_type slice -i 10slices.mp4 -f rawvideo -y 
/dev/null
threads=2:
old: 1m21.492s
new: 1m11.075s
threads=3:
old: 1m12.554s
new: 1m11.771s
threads=4:
old:  1m8.915s
new:  1m4.194s
threads=5:
old:  1m2.417s
new:   57.524s
threads=6:
old:  1m6.710s
new:  1m0.731s
threads=7:
old:  1m5.217s
new:  1m2.166s
threads=8:
old:  1m6.974s
new:  1m2.431s
threads=9:
old:   59.830s
new:   58.162s
threads=10:
old:   58.711s
new:   55.859s

Signed-off-by: Muhammad Faiz 
---
 libavcodec/pthread_slice.c | 217 -
 1 file changed, 138 insertions(+), 79 deletions(-)

diff --git a/libavcodec/pthread_slice.c b/libavcodec/pthread_slice.c
index 60f5b78..a290dab 100644
--- a/libavcodec/pthread_slice.c
+++ b/libavcodec/pthread_slice.c
@@ -22,6 +22,7 @@
  * @see doc/multithreading.txt
  */
 
+#include 
 #include "config.h"
 
 #include "avcodec.h"
@@ -38,21 +39,25 @@
 typedef int (action_func)(AVCodecContext *c, void *arg);
 typedef int (action_func2)(AVCodecContext *c, void *arg, int jobnr, int 
threadnr);
 
+typedef struct WorkerContext WorkerContext;
+
 typedef struct SliceThreadContext {
-pthread_t *workers;
+AVCodecContext *avctx;
+WorkerContext *workers;
 action_func *func;
 action_func2 *func2;
 void *args;
 int *rets;
-int job_count;
-int job_size;
-
-pthread_cond_t last_job_cond;
-pthread_cond_t current_job_cond;
-pthread_mutex_t current_job_lock;
-unsigned current_execute;
-int current_job;
+unsigned job_count;
+unsigned job_size;
+
+pthread_mutex_t mutex_user;
+pthread_mutex_t mutex_done;
+pthread_cond_t cond_done;
+atomic_uint current_job;
+atomic_uint nb_finished_jobs;
 int done;
+int worker_done;
 
 int *entries;
 int entries_count;
@@ -61,68 +66,93 @@ typedef struct SliceThreadContext {
 pthread_mutex_t *progress_mutex;
 } SliceThreadContext;
 
-static void* attribute_align_arg worker(void *v)
+struct WorkerContext {
+SliceThreadContext  *ctx;
+pthread_t   thread;
+pthread_mutex_t mutex;
+pthread_cond_t  cond;
+int done;
+};
+
+static unsigned run_jobs(SliceThreadContext *c)
 {
-AVCodecContext *avctx = v;
-SliceThreadContext *c = avctx->internal->thread_ctx;
-unsigned last_execute = 0;
-int our_job = c->job_count;
-int thread_count = avctx->thread_count;
-int self_id;
-
-pthread_mutex_lock(>current_job_lock);
-self_id = c->current_job++;
-for (;;){
-int ret;
-while (our_job >= c->job_count) {
-if (c->current_job == thread_count + c->job_count)
-pthread_cond_signal(>last_job_cond);
-
-while (last_execute == c->current_execute && !c->done)
-pthread_cond_wait(>current_job_cond, >current_job_lock);
-last_execute = c->current_execute;
-our_job = self_id;
-
-if (c->done) {
-pthread_mutex_unlock(>current_job_lock);
-return NULL;
-}
-}
-pthread_mutex_unlock(>current_job_lock);
+unsigned current_job, nb_finished_jobs = 0;
+int thread_count = c->avctx->thread_count;
 
-ret = c->func ? c->func(avctx, (char*)c->args + our_job*c->job_size):
-c->func2(avctx, c->args, our_job, self_id);
+while (nb_finished_jobs != c->job_count &&
+   (current_job = atomic_fetch_add_explicit(>current_job, 1, 
memory_order_acq_rel)) < c->job_count) {
+int ret = c->func ? c->func(c->avctx, (char *)c->args + current_job * 
(size_t) c->job_size)
+  : c->func2(c->avctx, c->args, current_job, 
current_job % thread_count);
 if (c->rets)
-c->rets[our_job%c->job_count] = ret;
+c->rets[current_job] = ret;
+nb_finished_jobs = atomic_fetch_add_explicit(>nb_finished_jobs, 1, 
memory_order_acq_rel) + 1;
+}
+
+return nb_finished_jobs;
+}
+
+static void* attribute_align_arg worker(void *v)
+{
+WorkerContext *w = v;
+SliceThreadContext *c = w->ctx;
+
+pthread_mutex_lock(>mutex);
+pthread_cond_signal(>cond);
+
+while (1) {
+w->done = 1;
+while (w->done)
+pthread_cond_wait(>cond, >mutex);
+
+if (c->done) {
+pthread_mutex_unlock(>mutex);
+return NULL;
+}
 
-pthread_mutex_lock(>current_job_lock);
-our_job = c->current_job++;
+if (run_jobs(c) == c->job_count) {
+pthread_mutex_lock(>mutex_done);
+c->worker_done = 1;
+pthread_cond_signal(>cond_done);
+pthread_mutex_unlock(>mutex_done);
+

Re: [FFmpeg-devel] [PATCH 3/3] avcodec/aacpsdsp_template: Fixes integer overflow in ps_add_squares_c()

2017-07-09 Thread Ivan Kalvachev 
On 7/9/17, Ronald S. Bultje  wrote:
> Hi,
>
> On Sat, Jul 8, 2017 at 5:17 PM, Michael Niedermayer 
> wrote:
>
>> On Mon, Jul 03, 2017 at 01:37:09AM +0200, Michael Niedermayer wrote:
>> > On Sun, Jul 02, 2017 at 02:24:53PM +0100, Rostislav Pehlivanov wrote:
>> > > On 2 July 2017 at 03:28, Michael Niedermayer 
>> wrote:
>> > >
>> > > > Fixes: runtime error: signed integer overflow: 1965219850 +
>> > > > 995792909
>> > > > cannot be represented in type 'int'
>> > > > Fixes: part of 2096/clusterfuzz-testcase-minimized-4901566068817920
>> > > >
>> > > > Found-by: continuous fuzzing process https://github.com/google/oss-
>> > > > fuzz/tree/master/projects/ffmpeg
>> > > > Signed-off-by
>> > > > > ffmpeg%0ASigned-off-by>:
>> > > > Michael Niedermayer 
>> > > > ---
>> > > >  libavcodec/aacpsdsp_template.c | 3 ++-
>> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
>> > > >
>> > > > diff --git a/libavcodec/aacpsdsp_template.c b/libavcodec/aacpsdsp_
>> > > > template.c
>> > > > index 9e1a95c1a1..2c0afd4512 100644
>> > > > --- a/libavcodec/aacpsdsp_template.c
>> > > > +++ b/libavcodec/aacpsdsp_template.c
>> > > > @@ -26,9 +26,10 @@
>> > > >  #include "libavutil/attributes.h"
>> > > >  #include "aacpsdsp.h"
>> > > >
>> > > > -static void ps_add_squares_c(INTFLOAT *dst, const INTFLOAT
>> (*src)[2], int
>> > > > n)
>> > > > +static void ps_add_squares_c(INTFLOAT *dst_param, const INTFLOAT
>> > > > (*src)[2], int n)
>> > > >  {
>> > > >  int i;
>> > > > +SUINTFLOAT *dst = dst_param;
>> > > >  for (i = 0; i < n; i++)
>> > > >  dst[i] += AAC_MADD28(src[i][0], src[i][0], src[i][1],
>> src[i][1]);
>> > > >  }
>> > > >
>> > > >
>> > > What's the issue with just _not_ fixing it here? It only occurs on
>> fuzzed
>> > > inputs, doesn't crash on any known platform ever, makes the code
>> uglier and
>> > > why? Because some fuzzer is super pedantic.
>> > > Why not fix the fuzzer? Why not just mark this as a false positive,
>> since
>> > > fixing it is pointless from the standpoint of security (you can't
>> exploit
>> > > overflows in transforms or functions like this), and all developers
>> hate it.
>> >
>> > Iam not sure you understand the problem.
>> > signed integer overflows are undefined behavior, undefined behavior
>> > is not allowed in C.
>> >
>> >
>> > Theres also no option to mark anything as false positive.
>> > If the tool makes a mistake, the issue needs to be reported to its
>> > authors and needs to be fixed.
>> > I belive the tool is correct, if someone thinks its not correct, the
>> > place to report this to is likely where the clang sanitizers are
>> > developed.
>> >
>> > I do understand that this is annoying but this is how C works.
>> >
>> > About "doesn't crash on any known platform ever",
>> > "you can't exploit  overflows in transforms or functions like this"
>> >
>> > undefined behavior has bitten people with unexpected results in the
>> > past. for example "if (a+b < a)" to test for overflow, but because the
>> condition
>> > can only be true in case of overflow and overflow isnt allowed in C
>> > the compiler didnt assemble this the way the human thought.
>> >
>> > In the case of ps_add_squares_c(), dst[i] has to increase if iam
>> > not mistaken. In case of overflow it can decrease but overflow is
>> > not allowed so a compiler can prune anything that implies dst[] to be
>> > negative.
>> > dst[] is used downstream in checks of greater / less and in FFMAX
>> > In this code the compiler can assume that the sign bit is clear,
>> > what happens when  the compilers optimizer has false assumtations
>> > i dont know but i know its not good.
>> >
>> > That said even if no such chain of conditions exist its still invalid
>> > code if theres undefined behavior in it
>>
>> Does anyone object to this patch ?
>> Or does anyone have a better idea on how to fix this ?
>> if not id like to apply it
>
>
> I think Rostislav's point is: why fix it, if it can only happen with
> corrupt input? The before and after situation is identical: garbage in,
> garbage out. If the compiler does funny things that makes the garbage
> slightly differently bad, is that really so devilishly bad? It's still
> garbage. Is anything improved by this?

As I have tried to explain before,
you can harden a program by compiling it
with `gcc -ftrapv` .

In such a program the overflow would trap
and in normal case would lead to termination.

Do you want your browser to die by a small bit error in a random clip?
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 3/3] avcodec/aacpsdsp_template: Fixes integer overflow in ps_add_squares_c()

2017-07-09 Thread Reimar Döffinger 
On 09.07.2017, at 02:52, "Ronald S. Bultje"  wrote:
> On Sat, Jul 8, 2017 at 5:17 PM, Michael Niedermayer 
> wrote:
> 
>> 
>> Does anyone object to this patch ?
>> Or does anyone have a better idea on how to fix this ?
>> if not id like to apply it
> 
> 
> I think Rostislav's point is: why fix it, if it can only happen with
> corrupt input? The before and after situation is identical: garbage in,
> garbage out. If the compiler does funny things that makes the garbage
> slightly differently bad, is that really so devilishly bad? It's still
> garbage. Is anything improved by this?

The way C works, you MUST assume any undefined behaviour can at any point 
(different compiler, compiler options, ...) become exploitable.
You can try to justify it with assumptions (but even that is usually very hard, 
is and will the value really never be used in a condition affecting, however 
indirectly, a pointer value for example?), but those are just arbitrary 
assumptions not backed by any standard.
If you don't like that, C is the wrong language to use.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel