Re: [FFmpeg-devel] [PATCH] fate: Add h264 test for frame num gaps

[Michael Niedermayer]

On Fri, Dec 09, 2016 at 03:21:11PM +, Derek Buitenhuis wrote:
> Signed-off-by: Derek Buitenhuis 
> ---
> Sample is here: 
> https://trac.ffmpeg.org/attachment/ticket/5458/nondeterministic_cut.h264
> ---
>  tests/fate/h264.mak   |  2 ++
>  tests/ref/fate/h264-missing-frame | 35 +++
>  2 files changed, 37 insertions(+)
>  create mode 100644 tests/ref/fate/h264-missing-frame

tested on linux x86-32/64, arm-qemu, mips-qemu mingw32/64

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

I have often repented speaking, but never of holding my tongue.
-- Xenocrates


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] opus_parser: fix leaking channel_maps on error

[Michael Niedermayer]

On Fri, Dec 09, 2016 at 11:42:59PM +0100, Andreas Cadhalpun wrote:
> On 09.12.2016 15:23, Michael Niedermayer wrote:
> > On Fri, Dec 09, 2016 at 12:08:10AM +0100, Andreas Cadhalpun wrote:
> >> Signed-off-by: Andreas Cadhalpun 
> >> ---
> >>  libavcodec/opus_parser.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/libavcodec/opus_parser.c b/libavcodec/opus_parser.c
> >> index c30fd7b..21a73ee 100644
> >> --- a/libavcodec/opus_parser.c
> >> +++ b/libavcodec/opus_parser.c
> >> @@ -116,11 +116,11 @@ static int opus_find_frame_end(AVCodecParserContext 
> >> *ctx, AVCodecContext *avctx,
> >>  
> >>  if (avctx->extradata && !s->extradata_parsed) {
> >>  ret = ff_opus_parse_extradata(avctx, >ctx);
> >> +av_freep(>ctx.channel_maps);
> >>  if (ret < 0) {
> >>  av_log(avctx, AV_LOG_ERROR, "Error parsing Ogg extradata.\n");
> >>  return AVERROR_INVALIDDATA;
> >>  }
> >> -av_freep(>ctx.channel_maps);
> >>  s->extradata_parsed = 1;
> >>  }
> > 
> > isnt it more correct for ff_opus_parse_extradata() to cleanup what
> > it allocated on error ?
> 
> Probably, but opusdec already did it the other way around.
> I'm fine with changing that, though. See attached patch.
> 
> Best regards,
> Andreas
> 

>  opus.c|1 +
>  opusdec.c |1 -
>  2 files changed, 1 insertion(+), 1 deletion(-)
> 3e534ddbb39e05f8c7a5cfeba4b65ed587c3a519  
> 0001-opus_parser-fix-leaking-channel_maps-on-error.patch
> From 2edf5f9a571d483858928dd6bb9e84031ef8a391 Mon Sep 17 00:00:00 2001
> From: Andreas Cadhalpun 
> Date: Fri, 9 Dec 2016 00:00:18 +0100
> Subject: [PATCH 1/2] opus_parser: fix leaking channel_maps on error
> 
> Make ff_opus_parse_extradata free allocated memory on error instead of
> expecting callers to free it in that case.
> 
> Signed-off-by: Andreas Cadhalpun 
> ---
>  libavcodec/opus.c| 1 +
>  libavcodec/opusdec.c | 1 -
>  2 files changed, 1 insertion(+), 1 deletion(-)

LGTM

thx

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Complexity theory is the science of finding the exact solution to an
approximation. Benchmarking OTOH is finding an approximation of the exact


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

Re: [FFmpeg-devel] [PATCH] fate: Add h264 test for frame num gaps

[Michael Niedermayer]

On Fri, Dec 09, 2016 at 03:21:11PM +, Derek Buitenhuis wrote:
> Signed-off-by: Derek Buitenhuis 
> ---
> Sample is here: 
> https://trac.ffmpeg.org/attachment/ticket/5458/nondeterministic_cut.h264
> ---
>  tests/fate/h264.mak   |  2 ++
>  tests/ref/fate/h264-missing-frame | 35 +++
>  2 files changed, 37 insertions(+)
>  create mode 100644 tests/ref/fate/h264-missing-frame

file uploaded

thx

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

Those who would give up essential Liberty, to purchase a little
temporary Safety, deserve neither Liberty nor Safety -- Benjamin Franklin


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/mpeg12dec: Add FF_CODEC_CAP_SKIP_FRAME_FILL_PARAM

[Michael Niedermayer]

On Sat, Dec 10, 2016 at 12:11:35AM +0100, Andreas Cadhalpun wrote:
> On 09.12.2016 02:50, Michael Niedermayer wrote:
> > On Fri, Dec 09, 2016 at 01:02:08AM +0100, Andreas Cadhalpun wrote:
> >> On 08.12.2016 22:53, Michael Niedermayer wrote:
> >>> This decreases the amount of computations and memory needed for analysing 
> >>> mpeg1/2 streams
> >>>
> >>> Signed-off-by: Michael Niedermayer 
> >>> ---
> >>>  libavcodec/mpeg12dec.c | 6 +-
> >>>  1 file changed, 5 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c
> >>> index ac8160daff..63979079c8 100644
> >>> --- a/libavcodec/mpeg12dec.c
> >>> +++ b/libavcodec/mpeg12dec.c
> >>> @@ -1655,7 +1655,6 @@ static int mpeg_field_start(MpegEncContext *s, 
> >>> const uint8_t *buf, int buf_size)
> >>>  if (sd)
> >>>  memcpy(sd->data, s1->a53_caption, s1->a53_caption_size);
> >>>  av_freep(>a53_caption);
> >>> -avctx->properties |= FF_CODEC_PROPERTY_CLOSED_CAPTIONS;
> >>>  }
> >>>  
> >>>  if (s1->has_stereo3d) {
> >>> @@ -2258,6 +2257,7 @@ static int mpeg_decode_a53_cc(AVCodecContext *avctx,
> >>>  s1->a53_caption  = av_malloc(s1->a53_caption_size);
> >>>  if (s1->a53_caption)
> >>>  memcpy(s1->a53_caption, p + 7, s1->a53_caption_size);
> >>> +avctx->properties |= FF_CODEC_PROPERTY_CLOSED_CAPTIONS;
> >>>  }
> >>>  return 1;
> >>>  } else if (buf_size >= 11 &&
> >>> @@ -2313,6 +2313,7 @@ static int mpeg_decode_a53_cc(AVCodecContext *avctx,
> >>>  p += 6;
> >>>  }
> >>>  }
> >>> +avctx->properties |= FF_CODEC_PROPERTY_CLOSED_CAPTIONS;
> >>>  }
> >>>  return 1;
> >>>  }
> >>
> >> How are the above changes related to the commit message?
> > 
> > the update is moved from code that is skiped if skip_frame is set
> > to code that is not skiped so the change below doesnt loose that
> > from being executed
> 
> Thanks for explaining that. Maybe mention it in the commit message.
> I can confirm that this patch significantly accelerates analyzing,
> so it looks good to me.

changed, applied

thx

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

Many things microsoft did are stupid, but not doing something just because
microsoft did it is even more stupid. If everything ms did were stupid they
would be bankrupt already.


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] opus_parser: replace ff_parse_close with opus_parse_close

[Hendrik Leppkes]

On Fri, Dec 9, 2016 at 11:46 PM, Andreas Cadhalpun
 wrote:
> On 09.12.2016 15:02, Hendrik Leppkes wrote:
>> On Fri, Dec 9, 2016 at 12:09 AM, Andreas Cadhalpun
>>  wrote:
>>> The former expects priv_data to be the ParseContext directly, so using
>>> it does not work.
>>>
>>
>> As an alternative re-order the OpusParseContext so that ParseContext
>> comes first, it then would work, and thats basically how its done in
>> the other parsers from what I can tell.
>
> Good idea, that makes the patch a bit shorter.
>

LGTM.

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

Re: [FFmpeg-devel] [PATCH] lavf: add ffprobe demuxer

[Andreas Cadhalpun]

On 09.12.2016 15:55, Stefano Sabatini wrote:
> On date Friday 2016-11-25 11:58:42 +0100, Nicolas George encoded:
>> Le quintidi 5 frimaire, an CCXXV, Andreas Cadhalpun a écrit :
>>> I think a better idea would be to require '-strict experimental',
>>> as code disabled by default does neither get build- nor FATE-tested
>>> much.
>>
>> That is an excellent idea!
> 
> -strict is for codecs, there is no such thing at the moment for
> muxers/demuxers (so it should be implemented for muxers/demuxers).

It works fine for the mp4 muxer, just try it yourself:
$ ffmpeg -f lavfi -i testsrc=d=1 -c libvpx-vp9 -f mp4 /dev/null -y
$ ffmpeg -f lavfi -i testsrc=d=1 -c libvpx-vp9 -f mp4 -strict experimental 
/dev/null -y

I even showed how it can be implemented in this case [1]:
if (avf->strict_std_compliance > FF_COMPLIANCE_EXPERIMENTAL) {
av_log(avf, AV_LOG_ERROR, "The ffprobe demuxer is experimental and 
requires '-strict experimental'.\n");
return AVERROR_EXPERIMENTAL;
}

Best regards,
Andreas


1: https://ffmpeg.org/pipermail/ffmpeg-devel/2016-December/203901.html
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [DECISION] Revoke the decision of dropping ffserver

[Andreas Cadhalpun]

On 09.12.2016 10:26, wm4 wrote:
> On Fri, 9 Dec 2016 00:30:24 +0100
> Andreas Cadhalpun  wrote:
> 
>> On 08.12.2016 15:53, wm4 wrote:
>>> (I'm still waiting for related work to be merged from Libav).  
>>
>> Why don't you merge it yourself instead of waiting for others?
> 
> The commit to be merged next appears to have some intrusive h264 decoder
> changes. I'm not going to touch that.

You could skip that commit and mention it in the TODO/FIXME/UNMERGED
section of doc/libav-merge.txt.

>>> So yes, removing things can mean progress.  
>>
>> However, removing ffserver now doesn't, because the libraries
>> have to keep backwards-compatibility until the next major bump
>> anyway.
> 
> I'd agree with this, except I know that if the time comes for a major
> bump that necessitates immediate removal of ffserver, a new discussion
> about ffserver will break out, and the bump (or removal of the
> deprecated things ffserver relies on) would be delayed.

Prediction is very difficult, especially about the future.

> If this is how development work (maybe it does, maybe it doesn't), then
> shitflinging is an inherent part of the project's developer culture and
> the only way to achieve something. Blergh.

According to our code of conduct this should not be part of FFmpeg's
developer culture.

> Maybe this is not a problem anymore. ffserver.c still brings up some
> deprecation warnings, but surely michaelni will send a patch soon to
> fix those.

I wouldn't be so sure about that, as these warnings aren't particularly
urgent and other parts of the code base produce similar warnings.

> This discussion is worth leading in general anyway.
> 
>>> Nobody would care if ffserver actually used the ffmpeg libs correctly.
>>> But it still requires ffserver-specific code in libavformat. And even
>>> after all of michaelni's oddly-timed hard work to cleanup ffserver,
>>> ffserver itself uses internal libavformat stuff (see
>>> libavformat/libavformat.v - it also includes internal headers).  
>>
>> Yes, that's the last point that needs to be fixed before the next
>> major bump if ffserver is to be kept.
> 
> I didn't check whether the internal libavformat would break ffserver on
> the next bump,

If these symbols get removed from libavformat/libavformat.v and are thus
not exported in the shared library anymore, ffserver can't use them
(except when linked statically).

> or if there are other hidden internal or deprecated API uses, but yeah.

Since there is now ffservertest it should be easy to check if something
important breaks when the major bump happens.

>>> This project is frustrating because it puts features (even broken,
>>> half-implemented ones) over robust implementation and ease of use, and
>>> on top of it is unable to enforce any policy or decisions the community
>>> may have made. You have to waste your time discussing about nothing to
>>> overly... self-confident... people when trying to make a change.  
>>
>> You don't have to waste everyone's time complaining like that.
>> It'd be much better if you'd instead spend the time working on reducing
>> the backlog in the merges from Libav.
> 
> I could as well say: you don't have to waste everyone's time defending
> ffserver,

That's not an accurate description of my position[1] on this.

> you could just spend time on fixing it instead.

That's exactly what I did. [2][3][4]

> You don't have to say anything about the general way ffmpeg development
> works?

It mainly works by sending patches to the mailing list, where they are
discussed based on technical arguments.

Best regards,
Andreas


1: https://ffmpeg.org/pipermail/ffmpeg-devel/2016-November/203575.html
2: https://ffmpeg.org/pipermail/ffmpeg-devel/2016-November/203672.html
3: https://ffmpeg.org/pipermail/ffmpeg-devel/2016-November/203670.html
4: https://ffmpeg.org/pipermail/ffmpeg-devel/2016-November/203671.html
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avcodec/mpeg12dec: Add FF_CODEC_CAP_SKIP_FRAME_FILL_PARAM

[Andreas Cadhalpun]

On 09.12.2016 02:50, Michael Niedermayer wrote:
> On Fri, Dec 09, 2016 at 01:02:08AM +0100, Andreas Cadhalpun wrote:
>> On 08.12.2016 22:53, Michael Niedermayer wrote:
>>> This decreases the amount of computations and memory needed for analysing 
>>> mpeg1/2 streams
>>>
>>> Signed-off-by: Michael Niedermayer 
>>> ---
>>>  libavcodec/mpeg12dec.c | 6 +-
>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c
>>> index ac8160daff..63979079c8 100644
>>> --- a/libavcodec/mpeg12dec.c
>>> +++ b/libavcodec/mpeg12dec.c
>>> @@ -1655,7 +1655,6 @@ static int mpeg_field_start(MpegEncContext *s, const 
>>> uint8_t *buf, int buf_size)
>>>  if (sd)
>>>  memcpy(sd->data, s1->a53_caption, s1->a53_caption_size);
>>>  av_freep(>a53_caption);
>>> -avctx->properties |= FF_CODEC_PROPERTY_CLOSED_CAPTIONS;
>>>  }
>>>  
>>>  if (s1->has_stereo3d) {
>>> @@ -2258,6 +2257,7 @@ static int mpeg_decode_a53_cc(AVCodecContext *avctx,
>>>  s1->a53_caption  = av_malloc(s1->a53_caption_size);
>>>  if (s1->a53_caption)
>>>  memcpy(s1->a53_caption, p + 7, s1->a53_caption_size);
>>> +avctx->properties |= FF_CODEC_PROPERTY_CLOSED_CAPTIONS;
>>>  }
>>>  return 1;
>>>  } else if (buf_size >= 11 &&
>>> @@ -2313,6 +2313,7 @@ static int mpeg_decode_a53_cc(AVCodecContext *avctx,
>>>  p += 6;
>>>  }
>>>  }
>>> +avctx->properties |= FF_CODEC_PROPERTY_CLOSED_CAPTIONS;
>>>  }
>>>  return 1;
>>>  }
>>
>> How are the above changes related to the commit message?
> 
> the update is moved from code that is skiped if skip_frame is set
> to code that is not skiped so the change below doesnt loose that
> from being executed

Thanks for explaining that. Maybe mention it in the commit message.
I can confirm that this patch significantly accelerates analyzing,
so it looks good to me.

Best regards,
Andreas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 2/2] opus_parser: replace ff_parse_close with opus_parse_close

[Andreas Cadhalpun]

On 09.12.2016 15:02, Hendrik Leppkes wrote:
> On Fri, Dec 9, 2016 at 12:09 AM, Andreas Cadhalpun
>  wrote:
>> The former expects priv_data to be the ParseContext directly, so using
>> it does not work.
>>
> 
> As an alternative re-order the OpusParseContext so that ParseContext
> comes first, it then would work, and thats basically how its done in
> the other parsers from what I can tell.

Good idea, that makes the patch a bit shorter.

Best regards,
Andreas

>From 88596cbc50f43f7d29e1f9a3a4a115b3e8e60aaa Mon Sep 17 00:00:00 2001
From: Andreas Cadhalpun 
Date: Fri, 9 Dec 2016 00:01:35 +0100
Subject: [PATCH 2/2] opus_parser: make ParseContext the first element in
 OpusParseContext

ff_parse_close expects priv_data to be the ParseContext directly and
thus doesn't work if it isn't at the beginning of OpusParseContext.

Signed-off-by: Andreas Cadhalpun 
---
 libavcodec/opus_parser.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavcodec/opus_parser.c b/libavcodec/opus_parser.c
index c30fd7b..893573e 100644
--- a/libavcodec/opus_parser.c
+++ b/libavcodec/opus_parser.c
@@ -31,10 +31,10 @@
 #include "parser.h"
 
 typedef struct OpusParseContext {
+ParseContext pc;
 OpusContext ctx;
 OpusPacket pkt;
 int extradata_parsed;
-ParseContext pc;
 int ts_framing;
 } OpusParseContext;
 
-- 
2.10.2

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

Re: [FFmpeg-devel] [PATCH 1/2] opus_parser: fix leaking channel_maps on error

[Andreas Cadhalpun]

On 09.12.2016 15:23, Michael Niedermayer wrote:
> On Fri, Dec 09, 2016 at 12:08:10AM +0100, Andreas Cadhalpun wrote:
>> Signed-off-by: Andreas Cadhalpun 
>> ---
>>  libavcodec/opus_parser.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/libavcodec/opus_parser.c b/libavcodec/opus_parser.c
>> index c30fd7b..21a73ee 100644
>> --- a/libavcodec/opus_parser.c
>> +++ b/libavcodec/opus_parser.c
>> @@ -116,11 +116,11 @@ static int opus_find_frame_end(AVCodecParserContext 
>> *ctx, AVCodecContext *avctx,
>>  
>>  if (avctx->extradata && !s->extradata_parsed) {
>>  ret = ff_opus_parse_extradata(avctx, >ctx);
>> +av_freep(>ctx.channel_maps);
>>  if (ret < 0) {
>>  av_log(avctx, AV_LOG_ERROR, "Error parsing Ogg extradata.\n");
>>  return AVERROR_INVALIDDATA;
>>  }
>> -av_freep(>ctx.channel_maps);
>>  s->extradata_parsed = 1;
>>  }
> 
> isnt it more correct for ff_opus_parse_extradata() to cleanup what
> it allocated on error ?

Probably, but opusdec already did it the other way around.
I'm fine with changing that, though. See attached patch.

Best regards,
Andreas

>From 2edf5f9a571d483858928dd6bb9e84031ef8a391 Mon Sep 17 00:00:00 2001
From: Andreas Cadhalpun 
Date: Fri, 9 Dec 2016 00:00:18 +0100
Subject: [PATCH 1/2] opus_parser: fix leaking channel_maps on error

Make ff_opus_parse_extradata free allocated memory on error instead of
expecting callers to free it in that case.

Signed-off-by: Andreas Cadhalpun 
---
 libavcodec/opus.c| 1 +
 libavcodec/opusdec.c | 1 -
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavcodec/opus.c b/libavcodec/opus.c
index 08d94b6..1eeb92c 100644
--- a/libavcodec/opus.c
+++ b/libavcodec/opus.c
@@ -403,6 +403,7 @@ av_cold int ff_opus_parse_extradata(AVCodecContext *avctx,
 } else if (idx >= streams + stereo_streams) {
 av_log(avctx, AV_LOG_ERROR,
"Invalid channel map for output channel %d: %d\n", i, idx);
+av_freep(>channel_maps);
 return AVERROR_INVALIDDATA;
 }
 
diff --git a/libavcodec/opusdec.c b/libavcodec/opusdec.c
index ec793c6..329f784 100644
--- a/libavcodec/opusdec.c
+++ b/libavcodec/opusdec.c
@@ -646,7 +646,6 @@ static av_cold int opus_decode_init(AVCodecContext *avctx)
 /* find out the channel configuration */
 ret = ff_opus_parse_extradata(avctx, c);
 if (ret < 0) {
-av_freep(>channel_maps);
 av_freep(>fdsp);
 return ret;
 }
-- 
2.10.2

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

Re: [FFmpeg-devel] [PATCH] avformat/options_table: Set the default maximum number of streams to 100

[Michael Niedermayer]

On Fri, Dec 09, 2016 at 08:33:29PM +0100, Marton Balint wrote:
> 
> On Fri, 9 Dec 2016, Michael Niedermayer wrote:
> 
> >On Thu, Dec 08, 2016 at 09:47:53PM +0100, Nicolas George wrote:
> >>L'octidi 18 frimaire, an CCXXV, Michael Niedermayer a écrit :
> >>>A. Is a heap limit for av_*alloc*() acceptable ?
> >>>B. Are case based limits acceptable ?
> >>
> >>No. This is the task of the operating system.
> >>
> >
> >>>also even if C is choosen, a small set of limits on the main parameters
> >>>still is needed to allow efficient fuzzing, all issues reported
> >>>by oss-fuzz recently are "hangs" due to slow decoding,
> >>
> >>Then set a limit at the operating system level.
> >
> >You are misunderstanding the problem i think
> >
> >The goal of a fuzzer is to find bugs, crashes, undefined, bad things,
> >OOM, hangs.
> >
> >If the code under test can allocate arbitrary amounts of memory and
> >take arbitrary amounts of time in a significant number of non-bug
> >cases then the fuzzer cannot reliably find the corresponding bugs.
> >
> >moving the threshold of where to declare something OOM or hang around
> >will not solve this.
> >blocking high resolution, high channel count, high stream count
> >cases OTOH should improve the rate of false positives.
> 
> Then you should run the fuzzer with the limits you find optimal, no?
> 
> Reducing the defaults which causes rare-but-existing files to stop
> working does not make much sense to me. I particularly don't like
> the stream count limits, because parsing possibly corrupted sources
> (e.g. mpegts) can easily generate a high number of mostly harmless
> empty streams as far as I remember.
> 
> It just might make more sense to create a section in the
> documentation or a wiki page which describes that if you are working
> with untrusted files, you should use a sandbox, use system resource
> limits, and you might use these options as well.
> 

> If we still want a default limit, that limit should be IMHO
> _insanely_ high, tens of thousands, not hundreds.

The OOM case that ive debuged was in the tens of thousands so
a limit in that range will likely not stop much

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

Let us carefully observe those good qualities wherein our enemies excel us
and endeavor to excel them, by avoiding what is faulty, and imitating what
is excellent in them. -- Plutarch


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

[FFmpeg-devel] [PATCH] avutil/log: avoid build error if valgrind was removed after configure

[Andreas Oberritter]

In an automated build system, where valgrind and ffmpeg get updated in
parallel, a race condition may occur in which valgrind/valgrind.h is
available when configure runs but not if libavutil/log.c gets compiled.

Using CONFIG_VALGRIND_BACKTRACE helps to avoid this problem, because
it allows to prevent the inclusion of the autodetected valgrind.h by
using a configure switch.

Signed-off-by: Andreas Oberritter 
---
Please put me on CC: when replying, as I'm not subscribed to the list.

 libavutil/log.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavutil/log.c b/libavutil/log.c
index 44c11cb..202c0f1 100644
--- a/libavutil/log.c
+++ b/libavutil/log.c
@@ -47,7 +47,7 @@ static pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER;
 
 #define LINE_SZ 1024
 
-#if HAVE_VALGRIND_VALGRIND_H
+#if CONFIG_VALGRIND_BACKTRACE
 #include 
 /* this is the log level at which valgrind will output a full backtrace */
 #define BACKTRACE_LOGLEVEL AV_LOG_ERROR
-- 
2.7.4

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

Re: [FFmpeg-devel] [PATCH] Support limiting the number of pixels per image

[Michael Niedermayer]

On Fri, Dec 09, 2016 at 12:45:22AM +0100, Andreas Cadhalpun wrote:
> On 08.12.2016 19:30, Michael Niedermayer wrote:
> > TODO: split into 2 patches (one per lib), docs & bump
> > 
> > This allows preventing some OOM and "slow decoding" cases by limiting the 
> > maximum resolution
> > this may be useful to avoid fuzzers getting stuck in boring cases
> > 
> > Signed-off-by: Michael Niedermayer 
> > ---
> >  libavcodec/avcodec.h |  8 
> >  libavcodec/options_table.h   |  1 +
> >  libavutil/imgutils.c | 22 ++
> >  tests/ref/fate/api-mjpeg-codec-param |  2 ++
> >  tests/ref/fate/api-png-codec-param   |  2 ++
> >  5 files changed, 31 insertions(+), 4 deletions(-)
> 
> That's probably OK.
> One caveat is that currently not every demuxer uses av_image_check_size,
> but I'm working on fixing that.

> Do you plan to reduce the default in a future patch?

Iam happy to leave that to others to do

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

Its not that you shouldnt use gotos but rather that you should write
readable code and code with gotos often but not always is less readable


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

Re: [FFmpeg-devel] [PATCH] Support limiting the number of pixels per image

[Paul B Mahol]

On 12/9/16, Michael Niedermayer  wrote:
> Adds av_image_check_size2() with max_pixels and pix_fmt parameters.
> pix_fmt is unused and is added to avoid a 2nd API change later
>
> The old function uses AVOptions to obtain the max_pixels value to simplify
> the transition.
>
> TODO: split into 2 patches (one per lib), docs & bump
>
> This allows preventing some OOM and "slow decoding" cases by limiting the
> maximum resolution
> this may be useful to avoid fuzzers getting stuck in boring cases
>
> Signed-off-by: Michael Niedermayer 
> ---
>  libavcodec/avcodec.h |  8 
>  libavcodec/options_table.h   |  1 +
>  libavutil/imgutils.c | 31 ++-
>  libavutil/imgutils.h | 14 ++
>  tests/ref/fate/api-mjpeg-codec-param |  2 ++
>  tests/ref/fate/api-png-codec-param   |  2 ++
>  6 files changed, 53 insertions(+), 5 deletions(-)
>
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index 7ac2adaf66..81052b10ef 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -3570,6 +3570,14 @@ typedef struct AVCodecContext {
>   */
>  int trailing_padding;
>
> +/**
> + * The number of pixels per image to maximally accept.
> + *
> + * - decoding: set by user
> + * - encoding: unused
> + */
> +int max_pixels;

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

Re: [FFmpeg-devel] [PATCH] Support limiting the number of pixels per image

[Marton Balint]

On Fri, 9 Dec 2016, Michael Niedermayer wrote:


Adds av_image_check_size2() with max_pixels and pix_fmt parameters.
pix_fmt is unused and is added to avoid a 2nd API change later

The old function uses AVOptions to obtain the max_pixels value to simplify
the transition.

TODO: split into 2 patches (one per lib), docs & bump

This allows preventing some OOM and "slow decoding" cases by limiting the 
maximum resolution
this may be useful to avoid fuzzers getting stuck in boring cases

Signed-off-by: Michael Niedermayer 
---
libavcodec/avcodec.h |  8 
libavcodec/options_table.h   |  1 +
libavutil/imgutils.c | 31 ++-
libavutil/imgutils.h | 14 ++
tests/ref/fate/api-mjpeg-codec-param |  2 ++
tests/ref/fate/api-png-codec-param   |  2 ++
6 files changed, 53 insertions(+), 5 deletions(-)

diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
index 7ac2adaf66..81052b10ef 100644
--- a/libavcodec/avcodec.h
+++ b/libavcodec/avcodec.h
@@ -3570,6 +3570,14 @@ typedef struct AVCodecContext {
 */
int trailing_padding;

+/**
+ * The number of pixels per image to maximally accept.
+ *
+ * - decoding: set by user
+ * - encoding: unused
+ */
+int max_pixels;


Don't we want to make this an int64, for future extendability?


+
} AVCodecContext;

AVRational av_codec_get_pkt_timebase (const AVCodecContext *avctx);
diff --git a/libavcodec/options_table.h b/libavcodec/options_table.h
index ee79859953..2f5eb252fe 100644
--- a/libavcodec/options_table.h
+++ b/libavcodec/options_table.h
@@ -570,6 +570,7 @@ static const AVOption avcodec_options[] = {
{"codec_whitelist", "List of decoders that are allowed to be used", 
OFFSET(codec_whitelist), AV_OPT_TYPE_STRING, { .str = NULL },  CHAR_MIN, CHAR_MAX, A|V|S|D },
{"pixel_format", "set pixel format", OFFSET(pix_fmt), AV_OPT_TYPE_PIXEL_FMT, 
{.i64=AV_PIX_FMT_NONE}, -1, INT_MAX, 0 },
{"video_size", "set video size", OFFSET(width), AV_OPT_TYPE_IMAGE_SIZE, 
{.str=NULL}, 0, INT_MAX, 0 },
+{"max_pixels", "Maximum number of pixels", OFFSET(max_pixels), 
AV_OPT_TYPE_INT, {.i64 = INT_MAX }, 0, INT_MAX, A|V|S|D },


INT_MAX is probably OK, as who knows what will crash for bigger sizes, 
right?


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

Re: [FFmpeg-devel] [PATCH] avformat/options_table: Set the default maximum number of streams to 100

[Marton Balint]

On Fri, 9 Dec 2016, Michael Niedermayer wrote:


On Thu, Dec 08, 2016 at 09:47:53PM +0100, Nicolas George wrote:

L'octidi 18 frimaire, an CCXXV, Michael Niedermayer a écrit :

A. Is a heap limit for av_*alloc*() acceptable ?
B. Are case based limits acceptable ?


No. This is the task of the operating system.




also even if C is choosen, a small set of limits on the main parameters
still is needed to allow efficient fuzzing, all issues reported
by oss-fuzz recently are "hangs" due to slow decoding,


Then set a limit at the operating system level.


You are misunderstanding the problem i think

The goal of a fuzzer is to find bugs, crashes, undefined, bad things,
OOM, hangs.

If the code under test can allocate arbitrary amounts of memory and
take arbitrary amounts of time in a significant number of non-bug
cases then the fuzzer cannot reliably find the corresponding bugs.

moving the threshold of where to declare something OOM or hang around
will not solve this.
blocking high resolution, high channel count, high stream count
cases OTOH should improve the rate of false positives.


Then you should run the fuzzer with the limits you find optimal, no?

Reducing the defaults which causes rare-but-existing files to stop working 
does not make much sense to me. I particularly don't like the stream count 
limits, because parsing possibly corrupted sources (e.g. mpegts) can 
easily generate a high number of mostly harmless empty streams as far as I 
remember.


It just might make more sense to create a section in the documentation or 
a wiki page which describes that if you are working with untrusted files, 
you should use a sandbox, use system resource limits, and you might use 
these options as well.


If we still want a default limit, that limit should be IMHO _insanely_ 
high, tens of thousands, not hundreds.


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

[FFmpeg-devel] [PATCH] Support limiting the number of pixels per image

[Michael Niedermayer]

Adds av_image_check_size2() with max_pixels and pix_fmt parameters.
pix_fmt is unused and is added to avoid a 2nd API change later

The old function uses AVOptions to obtain the max_pixels value to simplify
the transition.

TODO: split into 2 patches (one per lib), docs & bump

This allows preventing some OOM and "slow decoding" cases by limiting the 
maximum resolution
this may be useful to avoid fuzzers getting stuck in boring cases

Signed-off-by: Michael Niedermayer 
---
 libavcodec/avcodec.h |  8 
 libavcodec/options_table.h   |  1 +
 libavutil/imgutils.c | 31 ++-
 libavutil/imgutils.h | 14 ++
 tests/ref/fate/api-mjpeg-codec-param |  2 ++
 tests/ref/fate/api-png-codec-param   |  2 ++
 6 files changed, 53 insertions(+), 5 deletions(-)

diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
index 7ac2adaf66..81052b10ef 100644
--- a/libavcodec/avcodec.h
+++ b/libavcodec/avcodec.h
@@ -3570,6 +3570,14 @@ typedef struct AVCodecContext {
  */
 int trailing_padding;
 
+/**
+ * The number of pixels per image to maximally accept.
+ *
+ * - decoding: set by user
+ * - encoding: unused
+ */
+int max_pixels;
+
 } AVCodecContext;
 
 AVRational av_codec_get_pkt_timebase (const AVCodecContext *avctx);
diff --git a/libavcodec/options_table.h b/libavcodec/options_table.h
index ee79859953..2f5eb252fe 100644
--- a/libavcodec/options_table.h
+++ b/libavcodec/options_table.h
@@ -570,6 +570,7 @@ static const AVOption avcodec_options[] = {
 {"codec_whitelist", "List of decoders that are allowed to be used", 
OFFSET(codec_whitelist), AV_OPT_TYPE_STRING, { .str = NULL },  CHAR_MIN, 
CHAR_MAX, A|V|S|D },
 {"pixel_format", "set pixel format", OFFSET(pix_fmt), AV_OPT_TYPE_PIXEL_FMT, 
{.i64=AV_PIX_FMT_NONE}, -1, INT_MAX, 0 },
 {"video_size", "set video size", OFFSET(width), AV_OPT_TYPE_IMAGE_SIZE, 
{.str=NULL}, 0, INT_MAX, 0 },
+{"max_pixels", "Maximum number of pixels", OFFSET(max_pixels), 
AV_OPT_TYPE_INT, {.i64 = INT_MAX }, 0, INT_MAX, A|V|S|D },
 {NULL},
 };
 
diff --git a/libavutil/imgutils.c b/libavutil/imgutils.c
index 37808e53d0..96f2bbdc4f 100644
--- a/libavutil/imgutils.c
+++ b/libavutil/imgutils.c
@@ -30,6 +30,7 @@
 #include "mathematics.h"
 #include "pixdesc.h"
 #include "rational.h"
+#include "opt.h"
 
 void av_image_fill_max_pixsteps(int max_pixsteps[4], int max_pixstep_comps[4],
 const AVPixFmtDescriptor *pixdesc)
@@ -248,7 +249,7 @@ static const AVClass imgutils_class = {
 .parent_log_context_offset = offsetof(ImgUtils, log_ctx),
 };
 
-int av_image_check_size(unsigned int w, unsigned int h, int log_offset, void 
*log_ctx)
+int av_image_check_size2(unsigned int w, unsigned int h, int64_t max_pixels, 
enum AVPixelFormat pix_fmt, int log_offset, void *log_ctx)
 {
 ImgUtils imgutils = {
 .class  = _class,
@@ -256,11 +257,31 @@ int av_image_check_size(unsigned int w, unsigned int h, 
int log_offset, void *lo
 .log_ctx= log_ctx,
 };
 
-if ((int)w>0 && (int)h>0 && (w+128)*(uint64_t)(h+128) < INT_MAX/8)
-return 0;
+if ((int)w<=0 || (int)h<=0 || (w+128)*(uint64_t)(h+128) >= INT_MAX/8) {
+av_log(, AV_LOG_ERROR, "Picture size %ux%u is invalid\n", w, 
h);
+return AVERROR(EINVAL);
+}
 
-av_log(, AV_LOG_ERROR, "Picture size %ux%u is invalid\n", w, h);
-return AVERROR(EINVAL);
+if (max_pixels < INT64_MAX) {
+if (w*(int64_t)h > max_pixels) {
+av_log(, AV_LOG_ERROR,
+"Picture size %ux%u exceeds specified max pixel count 
%"PRId64"\n",
+w, h, max_pixels);
+return AVERROR(EINVAL);
+}
+}
+
+return 0;
+}
+
+int av_image_check_size(unsigned int w, unsigned int h, int log_offset, void 
*log_ctx)
+{
+int64_t max = INT64_MAX;
+
+if (log_ctx)
+av_opt_get_int(log_ctx, "max_pixels",  AV_OPT_SEARCH_CHILDREN, );
+
+return av_image_check_size2(w, h, max, AV_PIX_FMT_NONE, log_offset, 
log_ctx);
 }
 
 int av_image_check_sar(unsigned int w, unsigned int h, AVRational sar)
diff --git a/libavutil/imgutils.h b/libavutil/imgutils.h
index 23282a38fa..19f34deced 100644
--- a/libavutil/imgutils.h
+++ b/libavutil/imgutils.h
@@ -192,6 +192,20 @@ int av_image_copy_to_buffer(uint8_t *dst, int dst_size,
 int av_image_check_size(unsigned int w, unsigned int h, int log_offset, void 
*log_ctx);
 
 /**
+ * Check if the given dimension of an image is valid, meaning that all
+ * bytes of the image can be addressed with a signed int.
+ *
+ * @param w the width of the picture
+ * @param h the height of the picture
+ * @param max_pixels the maximum number of pixels the user wants to accept
+ * @param pix_fmt the pixel format, can be AV_PIX_FMT_NONE if unknown.
+ * @param log_offset the offset to sum to the log level for logging with 
log_ctx
+ * @param log_ctx the parent 

Re: [FFmpeg-devel] [PATCH] Support limiting the number of pixels per image

[wm4]

On Fri, 9 Dec 2016 14:11:30 +0100
Michael Niedermayer  wrote:

> On Fri, Dec 09, 2016 at 10:14:14AM +0100, wm4 wrote:
> > On Thu,  8 Dec 2016 19:30:25 +0100
> > Michael Niedermayer  wrote:
> >   
> > > TODO: split into 2 patches (one per lib), docs & bump
> > > 
> > > This allows preventing some OOM and "slow decoding" cases by limiting the 
> > > maximum resolution
> > > this may be useful to avoid fuzzers getting stuck in boring cases
> > > 
> > > Signed-off-by: Michael Niedermayer 
> > > ---
> > >  libavcodec/avcodec.h |  8 
> > >  libavcodec/options_table.h   |  1 +
> > >  libavutil/imgutils.c | 22 ++
> > >  tests/ref/fate/api-mjpeg-codec-param |  2 ++
> > >  tests/ref/fate/api-png-codec-param   |  2 ++
> > >  5 files changed, 31 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> > > index 7ac2adaf66..81052b10ef 100644
> > > --- a/libavcodec/avcodec.h
> > > +++ b/libavcodec/avcodec.h
> > > @@ -3570,6 +3570,14 @@ typedef struct AVCodecContext {
> > >   */
> > >  int trailing_padding;
> > >  
> > > +/**
> > > + * The number of pixels per image to maximally accept.
> > > + *
> > > + * - decoding: set by user
> > > + * - encoding: unused
> > > + */
> > > +int max_pixels;
> > > +
> > >  } AVCodecContext;
> > >  
> > >  AVRational av_codec_get_pkt_timebase (const AVCodecContext 
> > > *avctx);
> > > diff --git a/libavcodec/options_table.h b/libavcodec/options_table.h
> > > index ee79859953..2f5eb252fe 100644
> > > --- a/libavcodec/options_table.h
> > > +++ b/libavcodec/options_table.h
> > > @@ -570,6 +570,7 @@ static const AVOption avcodec_options[] = {
> > >  {"codec_whitelist", "List of decoders that are allowed to be used", 
> > > OFFSET(codec_whitelist), AV_OPT_TYPE_STRING, { .str = NULL },  CHAR_MIN, 
> > > CHAR_MAX, A|V|S|D },
> > >  {"pixel_format", "set pixel format", OFFSET(pix_fmt), 
> > > AV_OPT_TYPE_PIXEL_FMT, {.i64=AV_PIX_FMT_NONE}, -1, INT_MAX, 0 },
> > >  {"video_size", "set video size", OFFSET(width), AV_OPT_TYPE_IMAGE_SIZE, 
> > > {.str=NULL}, 0, INT_MAX, 0 },
> > > +{"max_pixels", "Maximum number of pixels", OFFSET(max_pixels), 
> > > AV_OPT_TYPE_INT, {.i64 = INT_MAX }, 0, INT_MAX, A|V|S|D },
> > >  {NULL},
> > >  };
> > >  
> > > diff --git a/libavutil/imgutils.c b/libavutil/imgutils.c
> > > index 37808e53d0..7c42ec7e17 100644
> > > --- a/libavutil/imgutils.c
> > > +++ b/libavutil/imgutils.c
> > > @@ -30,6 +30,7 @@
> > >  #include "mathematics.h"
> > >  #include "pixdesc.h"
> > >  #include "rational.h"
> > > +#include "opt.h"
> > >  
> > >  void av_image_fill_max_pixsteps(int max_pixsteps[4], int 
> > > max_pixstep_comps[4],
> > >  const AVPixFmtDescriptor *pixdesc)
> > > @@ -256,11 +257,24 @@ int av_image_check_size(unsigned int w, unsigned 
> > > int h, int log_offset, void *lo
> > >  .log_ctx= log_ctx,
> > >  };
> > >  
> > > -if ((int)w>0 && (int)h>0 && (w+128)*(uint64_t)(h+128) < INT_MAX/8)
> > > -return 0;
> > > +if ((int)w<=0 || (int)h<=0 || (w+128)*(uint64_t)(h+128) >= 
> > > INT_MAX/8) {
> > > +av_log(, AV_LOG_ERROR, "Picture size %ux%u is 
> > > invalid\n", w, h);
> > > +return AVERROR(EINVAL);
> > > +}
> > >  
> > > -av_log(, AV_LOG_ERROR, "Picture size %ux%u is invalid\n", 
> > > w, h);
> > > -return AVERROR(EINVAL);
> > > +if (log_ctx) {
> > > +int64_t max = INT_MAX;
> > > +if (av_opt_get_int(log_ctx, "max_pixels",  
> > > AV_OPT_SEARCH_CHILDREN, ) >= 0) {  
> > 
> > IMHO terrible implementation. Just add a new function that takes an
> > honest argument?  
> 
> adding a new function is possible but more complex, there are
> currently 79 uses of this, all need to be changed eventually.

This argument holds up only if they have a max_pixels AVOption, of
course. There are probably not 79 of them.

> and if we add max width and height this would start over again.
> on top of this, this function should probably have a pixel format
> parameter too. So if we change it that should be added too.

I agree at least about the latter. At least it would be simpler than
changing linesizes to size_t and such.

> 
> >   
> > > +if (w*h > max) {
> > > +av_log(, AV_LOG_ERROR,
> > > +   "Picture size %ux%u exceeds specified max pixel 
> > > count %"PRId64"\n",
> > > +   w, h, max);
> > > +return AVERROR(EINVAL);
> > > +}
> > > +}
> > > +}
> > > +
> > > +return 0;
> > >  }
> > >  
> > >  int av_image_check_sar(unsigned int w, unsigned int h, AVRational sar)
> > > diff --git a/tests/ref/fate/api-mjpeg-codec-param 
> > > b/tests/ref/fate/api-mjpeg-codec-param
> > > index c67d1b1ab0..08313adab3 100644
> > > --- a/tests/ref/fate/api-mjpeg-codec-param

[FFmpeg-devel] [PATCH] Save FFmpeg colorspace info in openh264 video files.

[Gregory J. Wolfe]

As of version 1.6, libopenh264 saves (in the output video file)
information about the color primaries, transfer characteristics,
and color matrix used when the video pixel data was created.
This patch sets the required libopenh264 data structures using
the FFmpeg colorspace information so that video players will
know how to properly decode video files created using FFmpeg
and libopenh264.

Signed-off-by: Gregory J. Wolfe 
---
 libavcodec/libopenh264enc.c | 61 +
 1 file changed, 61 insertions(+)

diff --git a/libavcodec/libopenh264enc.c b/libavcodec/libopenh264enc.c
index e84de27..d8a7ea3 100644
--- a/libavcodec/libopenh264enc.c
+++ b/libavcodec/libopenh264enc.c
@@ -205,6 +205,67 @@ FF_ENABLE_DEPRECATION_WARNINGS
 }
 }
 
+#if OPENH264_VER_AT_LEAST(1, 6)
+// set video signal type information
+param.sSpatialLayers[0].bVideoSignalTypePresent = true;
+param.sSpatialLayers[0].uiVideoFormat = VF_UNDEF; // default; choices are 
VF_: COMPONENT, PAL, NTSC, SECAM, MAC, UNDEF
+param.sSpatialLayers[0].bFullRange = avctx->color_range == 
AVCOL_RANGE_JPEG;
+param.sSpatialLayers[0].bColorDescriptionPresent = true;
+// These switches are intended to filter out all but the values supported 
by libopenh264.
+// An unsupported value causes the associated quantity to be set to 
"unspecified" and a
+// warning message to be issued.
+switch (avctx->color_primaries) {
+case AVCOL_PRI_BT709:param.sSpatialLayers[0].uiColorPrimaries  
= CP_BT709; break;
+case AVCOL_PRI_UNSPECIFIED:  param.sSpatialLayers[0].uiColorPrimaries  
= CP_UNDEF; break;
+case AVCOL_PRI_BT470M:   param.sSpatialLayers[0].uiColorPrimaries  
= CP_BT470M;break;
+case AVCOL_PRI_BT470BG:  param.sSpatialLayers[0].uiColorPrimaries  
= CP_BT470BG;   break;
+case AVCOL_PRI_SMPTE170M:param.sSpatialLayers[0].uiColorPrimaries  
= CP_SMPTE170M; break;
+case AVCOL_PRI_SMPTE240M:param.sSpatialLayers[0].uiColorPrimaries  
= CP_SMPTE240M; break;
+case AVCOL_PRI_FILM: param.sSpatialLayers[0].uiColorPrimaries  
= CP_FILM;  break;
+case AVCOL_PRI_BT2020:   param.sSpatialLayers[0].uiColorPrimaries  
= CP_BT2020;break;
+default: param.sSpatialLayers[0].uiColorPrimaries  
= CP_UNDEF;
+av_log(avctx, AV_LOG_WARNING, "Unsupported color primaries value %d 
was specified;" 
+" color primaries value has been set to \"unspecified\"\n", 
avctx->color_primaries);
+break;
+}
+switch (avctx->color_trc) {
+case AVCOL_TRC_BT709:
param.sSpatialLayers[0].uiTransferCharacteristics = TRC_BT709;break;
+case AVCOL_TRC_UNSPECIFIED:  
param.sSpatialLayers[0].uiTransferCharacteristics = TRC_UNDEF;break;
+case AVCOL_TRC_GAMMA22:  
param.sSpatialLayers[0].uiTransferCharacteristics = TRC_BT470M;   break;
+case AVCOL_TRC_GAMMA28:  
param.sSpatialLayers[0].uiTransferCharacteristics = TRC_BT470BG;  break;
+case AVCOL_TRC_SMPTE170M:
param.sSpatialLayers[0].uiTransferCharacteristics = TRC_SMPTE170M;break;
+case AVCOL_TRC_SMPTE240M:
param.sSpatialLayers[0].uiTransferCharacteristics = TRC_SMPTE240M;break;
+case AVCOL_TRC_LINEAR:   
param.sSpatialLayers[0].uiTransferCharacteristics = TRC_LINEAR;   break;
+case AVCOL_TRC_LOG:  
param.sSpatialLayers[0].uiTransferCharacteristics = TRC_LOG100;   break;
+case AVCOL_TRC_LOG_SQRT: 
param.sSpatialLayers[0].uiTransferCharacteristics = TRC_LOG316;   break;
+case AVCOL_TRC_IEC61966_2_4: 
param.sSpatialLayers[0].uiTransferCharacteristics = TRC_IEC61966_2_4; break;
+case AVCOL_TRC_BT1361_ECG:   
param.sSpatialLayers[0].uiTransferCharacteristics = TRC_BT1361E;  break;
+case AVCOL_TRC_IEC61966_2_1: 
param.sSpatialLayers[0].uiTransferCharacteristics = TRC_IEC61966_2_1; break;
+case AVCOL_TRC_BT2020_10:
param.sSpatialLayers[0].uiTransferCharacteristics = TRC_BT2020_10;break;
+case AVCOL_TRC_BT2020_12:
param.sSpatialLayers[0].uiTransferCharacteristics = TRC_BT2020_12;break;
+default: 
param.sSpatialLayers[0].uiTransferCharacteristics = TRC_UNDEF;
+av_log(avctx, AV_LOG_WARNING, "Unsupported transfer characteristics 
value %d was specified;" 
+" transfer characteristics value has been set to 
\"unspecified\"\n", avctx->color_trc);
+break;
+}
+switch (avctx->colorspace) {
+case AVCOL_SPC_RGB:  param.sSpatialLayers[0].uiColorMatrix 
= CM_GBR;   break;
+case AVCOL_SPC_BT709:param.sSpatialLayers[0].uiColorMatrix 
= CM_BT709; break;
+case AVCOL_SPC_UNSPECIFIED:  param.sSpatialLayers[0].uiColorMatrix 
= CM_UNDEF; break;

Re: [FFmpeg-devel] [PATCH 1/2] Avoid using the term "file" and prefer "url" in some docs and comments

[wm4]

On Fri, 9 Dec 2016 14:33:08 +0100
Michael Niedermayer  wrote:

> On Fri, Dec 09, 2016 at 09:55:52AM +0100, wm4 wrote:
> > On Fri, 9 Dec 2016 03:48:39 +0100
> > Michael Niedermayer  wrote:
> >   
> > > On Thu, Dec 08, 2016 at 11:13:16AM -0900, Lou Logan wrote:  
> > > > On Mon,  5 Dec 2016 13:52:50 +0100, Michael Niedermayer wrote:
> > > > 
> > > > > This should make it less ambigous that these are URLs
> > > > > 
> > > > > Signed-off-by: Michael Niedermayer 
> > > > > ---
> > > > >  doc/ffmpeg.texi  | 18 +-
> > > > >  doc/ffplay.texi  |  6 +++---
> > > > >  doc/ffprobe.texi | 10 +-
> > > > >  ffmpeg_opt.c |  4 ++--
> > > > >  4 files changed, 19 insertions(+), 19 deletions(-)
> > > > 
> > > > Although this is a trivial patch, approximately 7 hours between sending
> > > > a patch and applying without feedback isn't enough time. At least 24
> > > > hours would be preferrable.
> > > 
> > > there were open and fully public security bugs, the use of untrusted
> > > filenames which look like urls aka files as in
> > > "http://someserver.com;
> > > would allow potential remote code execution  
> >   
> 
> > I guess "security bugs" now justify any kind of change?  
> 
> what exactly is this comment supposed to mean ?

I'm just saying that security fixes doesn't mean that quality control
should be lacking. Maybe rather the opposite.

> 
> > 
> > It's clear that a user has to prefix filenames with file: or so, since
> > it will interpret various strings as not-files (like as an option or an
> > URL). Thus it's not a security bug, just an user error.  
> 
> There are really just 2 options, either its safe to use arbitrary
> filenames in the arguments, or it has to be documented that these are
> not arbitrary filenames, aka its not safe to put arbitrary things in
> there.

Yes. If you allow "plain" local fileanes, it's inherently ambiguous.
Maybe we could go as far as disallowing such filenames by default, and
requiring that the filename starts with "/", "./" or "file:".

I also claimed that a filename can be misinterpreted as option - but
that's probably not the case: filenames for input always are passed to
"-i" or similar options. Only output filenames are implicit.

Anyway, I might have assumed that this discussion is about patch 2/2,
not 1/2.

> And it certainly was not clear enough as tickets are being opened on
> the assumtation that this was safe and that by security researchers
> 
> 
> 
> [...]

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

Re: [FFmpeg-devel] [PATCH 1/2] Avoid using the term "file" and prefer "url" in some docs and comments

[wm4]

On Fri, 9 Dec 2016 14:45:24 +0100
Michael Niedermayer  wrote:

> On Fri, Dec 09, 2016 at 09:53:00AM +0100, wm4 wrote:
> > On Mon, 5 Dec 2016 21:10:00 +0100
> > Michael Niedermayer  wrote:
> >   
> > > On Mon, Dec 05, 2016 at 01:52:50PM +0100, Michael Niedermayer wrote:  
> > > > This should make it less ambigous that these are URLs
> > > > 
> > > > Signed-off-by: Michael Niedermayer 
> > > > ---
> > > >  doc/ffmpeg.texi  | 18 +-
> > > >  doc/ffplay.texi  |  6 +++---
> > > >  doc/ffprobe.texi | 10 +-
> > > >  ffmpeg_opt.c |  4 ++--
> > > >  4 files changed, 19 insertions(+), 19 deletions(-)
> > > 
> > > applied
> > > 
> > > 
> > > [...]  
> > 
> > I guess my remarks mean nothing.  
> 
> You had no remarks about this patch. You did comment on a different
> patch in this patchset, which has not been applied.
> 
> And i didnt reply to your comment there yet due to lack of time and
> i hoped others would comment too.
> 
> 
> [...]

My error. Apologies!
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] libavcodec/tests: Add avpacket test to .gitignore

[Michael Niedermayer]

On Fri, Dec 09, 2016 at 03:22:52PM +, Derek Buitenhuis wrote:
> Signed-off-by: Derek Buitenhuis 
> ---
> Happened to notice this when adding a FATE test.
> ---
>  libavcodec/tests/.gitignore | 1 +
>  1 file changed, 1 insertion(+)

LGTM

thx

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

No great genius has ever existed without some touch of madness. -- Aristotle


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

[FFmpeg-devel] [PATCH] libavcodec/tests: Add avpacket test to .gitignore

[Derek Buitenhuis]

Signed-off-by: Derek Buitenhuis 
---
Happened to notice this when adding a FATE test.
---
 libavcodec/tests/.gitignore | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libavcodec/tests/.gitignore b/libavcodec/tests/.gitignore
index d8ab947abe..0ab029696d 100644
--- a/libavcodec/tests/.gitignore
+++ b/libavcodec/tests/.gitignore
@@ -1,4 +1,5 @@
 /avfft
+/avpacket
 /cabac
 /dct
 /fft
-- 
2.11.0

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

[FFmpeg-devel] [PATCH] avcodec/flacdec: Check for invalid vlcs

[Michael Niedermayer]

Signed-off-by: Michael Niedermayer 
---
 libavcodec/flacdec.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/libavcodec/flacdec.c b/libavcodec/flacdec.c
index af81115ff8..0fffc2dd94 100644
--- a/libavcodec/flacdec.c
+++ b/libavcodec/flacdec.c
@@ -259,7 +259,13 @@ static int decode_residuals(FLACContext *s, int32_t 
*decoded, int pred_order)
 *decoded++ = get_sbits_long(>gb, tmp);
 } else {
 for (; i < samples; i++) {
-*decoded++ = get_sr_golomb_flac(>gb, tmp, INT_MAX, 0);
+int v = get_sr_golomb_flac(>gb, tmp, INT_MAX, 0);
+if (v == 0x8000){
+av_log(s->avctx, AV_LOG_ERROR, "invalid residual\n");
+return AVERROR_INVALIDDATA;
+}
+
+*decoded++ = v;
 }
 }
 i= 0;
-- 
2.11.0

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

[FFmpeg-devel] [PATCH] fate: Add h264 test for frame num gaps

[Derek Buitenhuis]

Signed-off-by: Derek Buitenhuis 
---
Sample is here: 
https://trac.ffmpeg.org/attachment/ticket/5458/nondeterministic_cut.h264
---
 tests/fate/h264.mak   |  2 ++
 tests/ref/fate/h264-missing-frame | 35 +++
 2 files changed, 37 insertions(+)
 create mode 100644 tests/ref/fate/h264-missing-frame

diff --git a/tests/fate/h264.mak b/tests/fate/h264.mak
index 718a3a862a..d40681f9c9 100644
--- a/tests/fate/h264.mak
+++ b/tests/fate/h264.mak
@@ -194,6 +194,7 @@ FATE_H264  := $(FATE_H264:%=fate-h264-conformance-%)
\
   fate-h264-intra-refresh-recovery  \
   fate-h264-lossless\
   fate-h264-3386\
+  fate-h264-missing-frame   \
 
 FATE_H264-$(call DEMDEC, H264, H264) += $(FATE_H264)
 FATE_H264-$(call DEMDEC,  MOV, H264) += fate-h264-crop-to-container
@@ -432,6 +433,7 @@ fate-h264-lossless:   CMD = 
framecrc -i $(TARGET_SAM
 fate-h264-mixed-nal-coding:   CMD = framecrc -i 
$(TARGET_SAMPLES)/h264/mixed-nal-coding.mp4
 fate-h264-unescaped-extradata:CMD = framecrc -i 
$(TARGET_SAMPLES)/h264/unescaped_extradata.mp4 -an -frames 10
 fate-h264-3386:   CMD = framecrc -i 
$(TARGET_SAMPLES)/h264/bbc2.sample.h264
+fate-h264-missing-frame:  CMD = framecrc -i 
$(TARGET_SAMPLES)/h264/nondeterministic_cut.h264
 
 fate-h264-reinit-%:   CMD = framecrc -i 
$(TARGET_SAMPLES)/h264/$(@:fate-h264-%=%).h264 -vf 
format=yuv444p10le,scale=w=352:h=288
 
diff --git a/tests/ref/fate/h264-missing-frame 
b/tests/ref/fate/h264-missing-frame
new file mode 100644
index 00..04d229913d
--- /dev/null
+++ b/tests/ref/fate/h264-missing-frame
@@ -0,0 +1,35 @@
+#tb 0: 1/30
+#media_type 0: video
+#codec_id 0: rawvideo
+#dimensions 0: 1440x1080
+#sar 0: 1/1
+0,  0,  0,1,  2332800, 0x009dacb8
+0,  1,  1,1,  2332800, 0xb1e50764
+0,  2,  2,1,  2332800, 0xe29481e0
+0,  3,  3,1,  2332800, 0x0b1b4de4
+0,  4,  4,1,  2332800, 0x726a644c
+0,  5,  5,1,  2332800, 0x7a09c4a5
+0,  6,  6,1,  2332800, 0x2e9059ea
+0,  7,  7,1,  2332800, 0x52071fdc
+0,  8,  8,1,  2332800, 0x4fa00417
+0,  9,  9,1,  2332800, 0x6037fb4d
+0, 10, 10,1,  2332800, 0x887ffae2
+0, 11, 11,1,  2332800, 0x887ffae2
+0, 12, 12,1,  2332800, 0x887ffae2
+0, 13, 13,1,  2332800, 0x887ffae2
+0, 14, 14,1,  2332800, 0x887ffae2
+0, 15, 15,1,  2332800, 0x887ffae2
+0, 16, 16,1,  2332800, 0x887ffae2
+0, 17, 17,1,  2332800, 0x887ffae2
+0, 18, 18,1,  2332800, 0x887ffae2
+0, 19, 19,1,  2332800, 0x887ffae2
+0, 20, 20,1,  2332800, 0x887ffae2
+0, 21, 21,1,  2332800, 0x887ffae2
+0, 22, 22,1,  2332800, 0x887ffae2
+0, 23, 23,1,  2332800, 0x887ffae2
+0, 24, 24,1,  2332800, 0x887ffae2
+0, 25, 25,1,  2332800, 0x887ffae2
+0, 26, 26,1,  2332800, 0x887ffae2
+0, 27, 27,1,  2332800, 0x887ffae2
+0, 28, 28,1,  2332800, 0x887ffae2
+0, 29, 29,1,  2332800, 0x824bb91b
-- 
2.11.0

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

Re: [FFmpeg-devel] [PATCH] lavf: add ffprobe demuxer

[Hendrik Leppkes]

On Fri, Dec 9, 2016 at 3:59 PM, Nicolas George  wrote:
> Le nonidi 19 frimaire, an CCXXV, Stefano Sabatini a écrit :
>> -strict is for codecs, there is no such thing at the moment for
>> muxers/demuxers (so it should be implemented for muxers/demuxers).
>
> AVFormatContext has strict_std_compliance too.
>

-f_strict through the ffmpeg CLI, FWIW, to avoid clashes with the
codec-level -strict.

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

Re: [FFmpeg-devel] [PATCH v3] h264_slice: Wait for refs to be available before we use them in error concealment

[Derek Buitenhuis]

On 12/9/2016 2:29 PM, Michael Niedermayer wrote:
> this looks reasonable

Pushed.

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

Re: [FFmpeg-devel] [PATCH] lavf: add ffprobe demuxer

[Nicolas George]

Le nonidi 19 frimaire, an CCXXV, Stefano Sabatini a écrit :
> -strict is for codecs, there is no such thing at the moment for
> muxers/demuxers (so it should be implemented for muxers/demuxers).

AVFormatContext has strict_std_compliance too.

Regards,

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

Re: [FFmpeg-devel] [PATCH] lavf: add ffprobe demuxer

[Stefano Sabatini]

On date Friday 2016-11-25 11:58:42 +0100, Nicolas George encoded:
> Le quintidi 5 frimaire, an CCXXV, Andreas Cadhalpun a écrit :
> > I think a better idea would be to require '-strict experimental',
> > as code disabled by default does neither get build- nor FATE-tested
> > much.
> 
> That is an excellent idea!

-strict is for codecs, there is no such thing at the moment for
muxers/demuxers (so it should be implemented for muxers/demuxers).
-- 
FFmpeg = Faithless and Free Magic Patchable Evanescent Guide
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCHv4] avformat: parse iTunes gapless information

[Michael Niedermayer]

On Thu, Dec 08, 2016 at 01:12:07PM -0800, Christopher Snowhill wrote:
> ---
>  libavformat/mp3dec.c | 70 
> +++-
>  1 file changed, 69 insertions(+), 1 deletion(-)
> 
> diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c
> index 56c7f8c..47f4028 100644
> --- a/libavformat/mp3dec.c
> +++ b/libavformat/mp3dec.c
> @@ -295,6 +295,59 @@ static void mp3_parse_vbri_tag(AVFormatContext *s, 
> AVStream *st, int64_t base)
>  }
>  }
>  
> +static void mp3_parse_itunes_tag(AVFormatContext *s, AVStream *st, 
> MPADecodeHeader *c, int64_t base, int vbrtag_size, unsigned int *size, 
> uint64_t *duration)
> +{
> +uint32_t v;
> +AVDictionaryEntry *de;
> +MP3DecContext *mp3 = s->priv_data;
> +size_t length;
> +uint32_t zero, start_pad, end_pad;
> +uint64_t last_eight_frames_offset;
> +int i;
> +

> +if (!s->metadata || !(de = av_dict_get(s->metadata, "iTunSMPB", NULL, 
> 0)))
> +  return;

inconsistent indention


> +
> +length = strlen(de->value);
> +
> +/* Minimum length is one digit per field plus the whitespace, maximum 
> length should depend on field type
> + * There are four fields we need in the first six, the rest are 
> currently zero padding */
> +if (length < (12 + 11) || length > (10 * 8 + 2 * 16 + 11))
> +return;
> +
> +if (sscanf(de->value, "%"PRIx32" %"PRIx32" %"PRIx32" %"PRIx64" %"PRIx32" 
> %"PRIx64, , _pad, _pad, duration, , 
> _eight_frames_offset) < 6 ||

> +duration < 0 ||

duration is a pointer



> +start_pad > (576 * 2 * 32) ||
> +end_pad > (576 * 2 * 64) ||

> +last_eight_frames_offset >= (avio_size(s->pb) - base - vbrtag_size)) 
> {

avio_size() could fail i think


> +*duration = 0;
> +return;
> +}
> +

> +mp3->start_pad = (signed int) start_pad;
> +mp3->end_pad = (signed int) end_pad;

useless casts



> +if (end_pad >= 528 + 1)
> +mp3->end_pad = end_pad - (528 + 1);
> +st->start_skip_samples = mp3->start_pad + 528 + 1;
> +av_log(s, AV_LOG_DEBUG, "pad %d %d\n", mp3->start_pad, mp3->end_pad);
> +if (!s->pb->seekable)
> +return;
> +
> +*size = (unsigned int) last_eight_frames_offset;

> +if (avio_seek(s->pb, base + vbrtag_size + last_eight_frames_offset, 
> SEEK_SET) < 0)
> + return;

tabs are not allowed in ffmpeg git

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

What does censorship reveal? It reveals fear. -- Julian Assange


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

Re: [FFmpeg-devel] [PATCH v3] h264_slice: Wait for refs to be available before we use them in error concealment

[Michael Niedermayer]

On Thu, Dec 08, 2016 at 04:55:49PM +, Derek Buitenhuis wrote:
> This could happen when there was a frame number gap and frame threading was 
> used.
> 
> This fixes #5458.
> 
> Debugging-by: Ronald S. Bultje 
> Debugging-by: Justin Ruggles 
> Signed-off-by: Derek Buitenhuis 
> ---
> Extra help from Ronald enlightened me to the fact that there
> is actually this field.
> 
> Patch 1/2 in the previous (v2) set is no longer needed.
> ---
>  libavcodec/h264_slice.c | 3 +++
>  1 file changed, 3 insertions(+)

this looks reasonable

thx

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Observe your enemies, for they first find out your faults. -- Antisthenes


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] opus_parser: fix leaking channel_maps on error

[Michael Niedermayer]

On Fri, Dec 09, 2016 at 12:08:10AM +0100, Andreas Cadhalpun wrote:
> Signed-off-by: Andreas Cadhalpun 
> ---
>  libavcodec/opus_parser.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavcodec/opus_parser.c b/libavcodec/opus_parser.c
> index c30fd7b..21a73ee 100644
> --- a/libavcodec/opus_parser.c
> +++ b/libavcodec/opus_parser.c
> @@ -116,11 +116,11 @@ static int opus_find_frame_end(AVCodecParserContext 
> *ctx, AVCodecContext *avctx,
>  
>  if (avctx->extradata && !s->extradata_parsed) {
>  ret = ff_opus_parse_extradata(avctx, >ctx);
> +av_freep(>ctx.channel_maps);
>  if (ret < 0) {
>  av_log(avctx, AV_LOG_ERROR, "Error parsing Ogg extradata.\n");
>  return AVERROR_INVALIDDATA;
>  }
> -av_freep(>ctx.channel_maps);
>  s->extradata_parsed = 1;
>  }

isnt it more correct for ff_opus_parse_extradata() to cleanup what
it allocated on error ?

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

Breaking DRM is a little like attempting to break through a door even
though the window is wide open and the only thing in the house is a bunch
of things you dont want and which you would get tomorrow for free anyway


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] fate: improve fate-mov dependencies

[James Almer]

On 12/9/2016 9:18 AM, Michael Niedermayer wrote:
> On Thu, Dec 08, 2016 at 03:11:03PM -0300, James Almer wrote:
>> Signed-off-by: James Almer 
>> ---
>>  tests/fate/mov.mak | 20 +---
>>  1 file changed, 9 insertions(+), 11 deletions(-)
> 
> should be ok

Set pushed, thanks.

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

Re: [FFmpeg-devel] [PATCH] avformat/options_table: Set the default maximum number of streams to 100

[Michael Niedermayer]

On Fri, Dec 09, 2016 at 11:35:08AM +0100, Nicolas George wrote:
> Le nonidi 19 frimaire, an CCXXV, Michael Niedermayer a écrit :
> > > > A. Is a heap limit for av_*alloc*() acceptable ?
> 
> > moving the threshold of where to declare something OOM or hang around
> > will not solve this.
> 
> Yet this is your "A" proposal. Or am I misunderstanding you?

no, i missed that "A" alone would not solve the fuzzer aspect of this
somewere in what i wrote ...
if thats what you mean then you are correct

"A" would mainly help for user app being crashed with the lib OOM

"B" would mainly help the fuzzer issue but also in a more limited sense
reduce the crash issue.
Here even with seperate processes one probably prefers processes not
maxing out their OS limits. A webpage with thousand images (not unlikely)
would either have a thousand processes or one taking down all the image
decoding. Whichever way its implemented more robustness against OOM and
hangs is a good thing

Also we could declare some decoders with capability flags as safe to
be used in the same process.
For example the simple image decoders can surely be made to be safe
with just a max_pixel limit, and that should have users who would
prefer not to need a seperate process.

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

Observe your enemies, for they first find out your faults. -- Antisthenes


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] opus_parser: replace ff_parse_close with opus_parse_close

[Hendrik Leppkes]

On Fri, Dec 9, 2016 at 12:09 AM, Andreas Cadhalpun
 wrote:
> The former expects priv_data to be the ParseContext directly, so using
> it does not work.
>

As an alternative re-order the OpusParseContext so that ParseContext
comes first, it then would work, and thats basically how its done in
the other parsers from what I can tell.

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

Re: [FFmpeg-devel] [PATCH 1/2] Avoid using the term "file" and prefer "url" in some docs and comments

[Michael Niedermayer]

On Fri, Dec 09, 2016 at 09:53:00AM +0100, wm4 wrote:
> On Mon, 5 Dec 2016 21:10:00 +0100
> Michael Niedermayer  wrote:
> 
> > On Mon, Dec 05, 2016 at 01:52:50PM +0100, Michael Niedermayer wrote:
> > > This should make it less ambigous that these are URLs
> > > 
> > > Signed-off-by: Michael Niedermayer 
> > > ---
> > >  doc/ffmpeg.texi  | 18 +-
> > >  doc/ffplay.texi  |  6 +++---
> > >  doc/ffprobe.texi | 10 +-
> > >  ffmpeg_opt.c |  4 ++--
> > >  4 files changed, 19 insertions(+), 19 deletions(-)  
> > 
> > applied
> > 
> > 
> > [...]
> 
> I guess my remarks mean nothing.

You had no remarks about this patch. You did comment on a different
patch in this patchset, which has not been applied.

And i didnt reply to your comment there yet due to lack of time and
i hoped others would comment too.


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

Avoid a single point of failure, be that a person or equipment.


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] Avoid using the term "file" and prefer "url" in some docs and comments

[Michael Niedermayer]

On Fri, Dec 09, 2016 at 09:55:52AM +0100, wm4 wrote:
> On Fri, 9 Dec 2016 03:48:39 +0100
> Michael Niedermayer  wrote:
> 
> > On Thu, Dec 08, 2016 at 11:13:16AM -0900, Lou Logan wrote:
> > > On Mon,  5 Dec 2016 13:52:50 +0100, Michael Niedermayer wrote:
> > >   
> > > > This should make it less ambigous that these are URLs
> > > > 
> > > > Signed-off-by: Michael Niedermayer 
> > > > ---
> > > >  doc/ffmpeg.texi  | 18 +-
> > > >  doc/ffplay.texi  |  6 +++---
> > > >  doc/ffprobe.texi | 10 +-
> > > >  ffmpeg_opt.c |  4 ++--
> > > >  4 files changed, 19 insertions(+), 19 deletions(-)  
> > > 
> > > Although this is a trivial patch, approximately 7 hours between sending
> > > a patch and applying without feedback isn't enough time. At least 24
> > > hours would be preferrable.  
> > 
> > there were open and fully public security bugs, the use of untrusted
> > filenames which look like urls aka files as in
> > "http://someserver.com;
> > would allow potential remote code execution
> 

> I guess "security bugs" now justify any kind of change?

what exactly is this comment supposed to mean ?


> 
> It's clear that a user has to prefix filenames with file: or so, since
> it will interpret various strings as not-files (like as an option or an
> URL). Thus it's not a security bug, just an user error.

There are really just 2 options, either its safe to use arbitrary
filenames in the arguments, or it has to be documented that these are
not arbitrary filenames, aka its not safe to put arbitrary things in
there.

And it certainly was not clear enough as tickets are being opened on
the assumtation that this was safe and that by security researchers



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

You can kill me, but you cannot change the truth.


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

Re: [FFmpeg-devel] [PATCH] Support limiting the number of pixels per image

[Michael Niedermayer]

On Fri, Dec 09, 2016 at 10:14:14AM +0100, wm4 wrote:
> On Thu,  8 Dec 2016 19:30:25 +0100
> Michael Niedermayer  wrote:
> 
> > TODO: split into 2 patches (one per lib), docs & bump
> > 
> > This allows preventing some OOM and "slow decoding" cases by limiting the 
> > maximum resolution
> > this may be useful to avoid fuzzers getting stuck in boring cases
> > 
> > Signed-off-by: Michael Niedermayer 
> > ---
> >  libavcodec/avcodec.h |  8 
> >  libavcodec/options_table.h   |  1 +
> >  libavutil/imgutils.c | 22 ++
> >  tests/ref/fate/api-mjpeg-codec-param |  2 ++
> >  tests/ref/fate/api-png-codec-param   |  2 ++
> >  5 files changed, 31 insertions(+), 4 deletions(-)
> > 
> > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> > index 7ac2adaf66..81052b10ef 100644
> > --- a/libavcodec/avcodec.h
> > +++ b/libavcodec/avcodec.h
> > @@ -3570,6 +3570,14 @@ typedef struct AVCodecContext {
> >   */
> >  int trailing_padding;
> >  
> > +/**
> > + * The number of pixels per image to maximally accept.
> > + *
> > + * - decoding: set by user
> > + * - encoding: unused
> > + */
> > +int max_pixels;
> > +
> >  } AVCodecContext;
> >  
> >  AVRational av_codec_get_pkt_timebase (const AVCodecContext *avctx);
> > diff --git a/libavcodec/options_table.h b/libavcodec/options_table.h
> > index ee79859953..2f5eb252fe 100644
> > --- a/libavcodec/options_table.h
> > +++ b/libavcodec/options_table.h
> > @@ -570,6 +570,7 @@ static const AVOption avcodec_options[] = {
> >  {"codec_whitelist", "List of decoders that are allowed to be used", 
> > OFFSET(codec_whitelist), AV_OPT_TYPE_STRING, { .str = NULL },  CHAR_MIN, 
> > CHAR_MAX, A|V|S|D },
> >  {"pixel_format", "set pixel format", OFFSET(pix_fmt), 
> > AV_OPT_TYPE_PIXEL_FMT, {.i64=AV_PIX_FMT_NONE}, -1, INT_MAX, 0 },
> >  {"video_size", "set video size", OFFSET(width), AV_OPT_TYPE_IMAGE_SIZE, 
> > {.str=NULL}, 0, INT_MAX, 0 },
> > +{"max_pixels", "Maximum number of pixels", OFFSET(max_pixels), 
> > AV_OPT_TYPE_INT, {.i64 = INT_MAX }, 0, INT_MAX, A|V|S|D },
> >  {NULL},
> >  };
> >  
> > diff --git a/libavutil/imgutils.c b/libavutil/imgutils.c
> > index 37808e53d0..7c42ec7e17 100644
> > --- a/libavutil/imgutils.c
> > +++ b/libavutil/imgutils.c
> > @@ -30,6 +30,7 @@
> >  #include "mathematics.h"
> >  #include "pixdesc.h"
> >  #include "rational.h"
> > +#include "opt.h"
> >  
> >  void av_image_fill_max_pixsteps(int max_pixsteps[4], int 
> > max_pixstep_comps[4],
> >  const AVPixFmtDescriptor *pixdesc)
> > @@ -256,11 +257,24 @@ int av_image_check_size(unsigned int w, unsigned int 
> > h, int log_offset, void *lo
> >  .log_ctx= log_ctx,
> >  };
> >  
> > -if ((int)w>0 && (int)h>0 && (w+128)*(uint64_t)(h+128) < INT_MAX/8)
> > -return 0;
> > +if ((int)w<=0 || (int)h<=0 || (w+128)*(uint64_t)(h+128) >= INT_MAX/8) {
> > +av_log(, AV_LOG_ERROR, "Picture size %ux%u is invalid\n", 
> > w, h);
> > +return AVERROR(EINVAL);
> > +}
> >  
> > -av_log(, AV_LOG_ERROR, "Picture size %ux%u is invalid\n", w, 
> > h);
> > -return AVERROR(EINVAL);
> > +if (log_ctx) {
> > +int64_t max = INT_MAX;
> > +if (av_opt_get_int(log_ctx, "max_pixels",  AV_OPT_SEARCH_CHILDREN, 
> > ) >= 0) {
> 
> IMHO terrible implementation. Just add a new function that takes an
> honest argument?

adding a new function is possible but more complex, there are
currently 79 uses of this, all need to be changed eventually.
and if we add max width and height this would start over again.
on top of this, this function should probably have a pixel format
parameter too. So if we change it that should be added too.


> 
> > +if (w*h > max) {
> > +av_log(, AV_LOG_ERROR,
> > +   "Picture size %ux%u exceeds specified max pixel 
> > count %"PRId64"\n",
> > +   w, h, max);
> > +return AVERROR(EINVAL);
> > +}
> > +}
> > +}
> > +
> > +return 0;
> >  }
> >  
> >  int av_image_check_sar(unsigned int w, unsigned int h, AVRational sar)
> > diff --git a/tests/ref/fate/api-mjpeg-codec-param 
> > b/tests/ref/fate/api-mjpeg-codec-param
> > index c67d1b1ab0..08313adab3 100644
> > --- a/tests/ref/fate/api-mjpeg-codec-param
> > +++ b/tests/ref/fate/api-mjpeg-codec-param
> > @@ -155,6 +155,7 @@ stream=0, decode=0
> >  codec_whitelist=
> >  pixel_format=yuvj422p
> >  video_size=400x225
> > +max_pixels=2147483647
> >  stream=0, decode=1
> >  b=0
> >  ab=0
> > @@ -312,3 +313,4 @@ stream=0, decode=1
> >  codec_whitelist=
> >  pixel_format=yuvj422p
> >  video_size=400x225
> > +max_pixels=2147483647
> > diff --git a/tests/ref/fate/api-png-codec-param 
> > b/tests/ref/fate/api-png-codec-param
> > index bd53441894..7a9a921461 100644
> > --- 

Re: [FFmpeg-devel] [PATCH 1/2] fate: improve fate-mov dependencies

[Michael Niedermayer]

On Thu, Dec 08, 2016 at 03:11:03PM -0300, James Almer wrote:
> Signed-off-by: James Almer 
> ---
>  tests/fate/mov.mak | 20 +---
>  1 file changed, 9 insertions(+), 11 deletions(-)

should be ok

[...]
-- 
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 2/2] fate: add a monoscopic spherical matroska test

[Michael Niedermayer]

On Thu, Dec 08, 2016 at 03:11:04PM -0300, James Almer wrote:
> Signed-off-by: James Almer 
> ---
> Sample can be found in http://0x0.st/Lpj.mkv
> 
>  tests/fate/matroska.mak|  4 
>  tests/ref/fate/matroska-spherical-mono | 16 
>  2 files changed, 20 insertions(+)
>  create mode 100644 tests/ref/fate/matroska-spherical-mono

Tested-by: Michael on linux x86-32/64, qemu-arm, qemu-mips and mingw32/64

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

Awnsering whenever a program halts or runs forever is
On a turing machine, in general impossible (turings halting problem).
On any real computer, always possible as a real computer has a finite number
of states N, and will either halt in less than N cycles or never halt.


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

[FFmpeg-devel] [PATCH] avformat/http: allow content range to set filesize

[PHILIP DE NIER]

Hi,
Attached patch fixes issue #6007 for me.
I think it could improved / extended because the "if (s->seekable == -1 && 
(!s->is_akamai || s->content_range_len != 2147483647))" is probably better 
placed http_read_header along with the similar is_mediagateway workaround. I 
wasn't sure whether the is_akamai should only be triggered if the filesize was 
read from the Content-Range header.
Philip
From 0ac79d03981e823a1922e7e1f58af7f3b02dca7d Mon Sep 17 00:00:00 2001
From: Philip de Nier 
Date: Wed, 7 Dec 2016 15:09:09 +
Subject: [PATCH] http: allow content range to set filesize

Set filesize to Content-Length if present and Transfer-Encoding is
not chunked. Otherwise set to Content-Range instance length if present.

Solves issue where filesize is set to unknown when using byte range
requests in combination with chunked transfers.

Fixes issue #6007
---
 libavformat/http.c | 17 +
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/libavformat/http.c b/libavformat/http.c
index 944a6cf..8dfaa5d 100644
--- a/libavformat/http.c
+++ b/libavformat/http.c
@@ -63,6 +63,7 @@ typedef struct HTTPContext {
 int http_code;
 /* Used if "Transfer-Encoding: chunked" otherwise -1. */
 uint64_t chunksize;
+uint64_t content_len, content_range_len;
 uint64_t off, end_off, filesize;
 char *location;
 HTTPAuthState auth_state;
@@ -618,9 +619,9 @@ static void parse_content_range(URLContext *h, const char *p)
 p += 6;
 s->off = strtoull(p, NULL, 10);
 if ((slash = strchr(p, '/')) && strlen(slash) > 0)
-s->filesize = strtoull(slash + 1, NULL, 10);
+s->content_range_len = strtoull(slash + 1, NULL, 10);
 }
-if (s->seekable == -1 && (!s->is_akamai || s->filesize != 2147483647))
+if (s->seekable == -1 && (!s->is_akamai || s->content_range_len != 2147483647))
 h->is_streamed = 0; /* we _can_ in fact seek */
 }
 
@@ -810,7 +811,7 @@ static int process_line(URLContext *h, char *line, int line_count,
 *new_location = 1;
 } else if (!av_strcasecmp(tag, "Content-Length") &&
s->filesize == UINT64_MAX) {
-s->filesize = strtoull(p, NULL, 10);
+s->content_len = strtoull(p, NULL, 10);
 } else if (!av_strcasecmp(tag, "Content-Range")) {
 parse_content_range(h, p);
 } else if (!av_strcasecmp(tag, "Accept-Ranges") &&
@@ -819,7 +820,6 @@ static int process_line(URLContext *h, char *line, int line_count,
 h->is_streamed = 0;
 } else if (!av_strcasecmp(tag, "Transfer-Encoding") &&
!av_strncasecmp(p, "chunked", 7)) {
-s->filesize  = UINT64_MAX;
 s->chunksize = 0;
 } else if (!av_strcasecmp(tag, "WWW-Authenticate")) {
 ff_http_auth_handle_header(>auth_state, tag, p);
@@ -974,6 +974,8 @@ static int http_read_header(URLContext *h, int *new_location)
 int err = 0;
 
 s->chunksize = UINT64_MAX;
+s->content_range_len = UINT64_MAX;
+s->content_len = UINT64_MAX;
 
 for (;;) {
 if ((err = http_get_line(s, line, sizeof(line))) < 0)
@@ -989,6 +991,13 @@ static int http_read_header(URLContext *h, int *new_location)
 s->line_count++;
 }
 
+if (s->filesize == UINT64_MAX) {
+if (s->chunksize == UINT64_MAX && s->content_len != UINT64_MAX)
+s->filesize = s->content_len;
+else if (s->content_range_len != UINT64_MAX)
+s->filesize = s->content_range_len;
+}
+
 if (s->seekable == -1 && s->is_mediagateway && s->filesize == 20)
 h->is_streamed = 1; /* we can in fact _not_ seek */
 
-- 
1.9.1

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

Re: [FFmpeg-devel] [PATCH] avformat/options_table: Set the default maximum number of streams to 100

[Ronald S. Bultje]

Hi,

On Thu, Dec 8, 2016 at 7:03 PM, Andreas Cadhalpun <
andreas.cadhal...@googlemail.com> wrote:

> On 08.12.2016 22:59, Carl Eugen Hoyos wrote:
> > 2016-12-08 18:37 GMT+01:00 Michael Niedermayer :
> >
> >> -{"max_streams", "maximum number of streams", OFFSET(max_streams),
> AV_OPT_TYPE_INT, { .i64 = INT_MAX }, 0, INT_MAX, D },
> >> +{"max_streams", "maximum number of streams", OFFSET(max_streams),
> AV_OPT_TYPE_INT, { .i64 = 100 }, 0, INT_MAX, D },
> >
> > I wanted to suggest 1000 which is still a magnitude less than the
> provided
> > crashing sample but 255 also sounds ok to me.
>
> Either value is OK. The important thing is that it's several orders of
> magnitude lower than INT_MAX.


On IRC, we discussed at what values OOM start occurring, which seems to be
around 30k-60k, so from there I suggested a value like 10k or 5k. 1000
seems a little low but I think I can live with it (I doubt ATM I can come
up with legit use cases that use 1000 streams).

If people hit the limit (whatever value we choose), I would propose that we
make the error message very specific, something similar to
AVERROR_PATCHWELCOME. This way, people understand this is not a hard
limitation and can be changed easily; fuzzers will obviously ignore this
message.

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

Re: [FFmpeg-devel] [PATCHv2] avfilter/formats: allow unknown channel layouts by default

[Nicolas George]

L'octidi 18 frimaire, an CCXXV, Marton Balint a écrit :
> Since the default in the libav fork is to only allow known layouts, making
> unknown layouts allowed by default here can be a security risk for filters
> directly merged from libav. However, usually it is simple to detect such 
> cases,
> use of av_get_channel_layout_nb_channels is a good indicator, so I suggest we
> change this regardless.

No objection from me, and the changes look consistent.

Regards,

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

Re: [FFmpeg-devel] [PATCH] avformat/options_table: Set the default maximum number of streams to 100

[Nicolas George]

Le nonidi 19 frimaire, an CCXXV, Michael Niedermayer a écrit :
> > > A. Is a heap limit for av_*alloc*() acceptable ?

> moving the threshold of where to declare something OOM or hang around
> will not solve this.

Yet this is your "A" proposal. Or am I misunderstanding you?

Regards,

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

Re: [FFmpeg-devel] [DECISION] Revoke the decision of dropping ffserver

[wm4]

On Fri, 9 Dec 2016 00:30:24 +0100
Andreas Cadhalpun  wrote:

> On 08.12.2016 15:53, wm4 wrote:
> > (I'm still waiting for related work to be merged from Libav).  
> 
> Why don't you merge it yourself instead of waiting for others?

The commit to be merged next appears to have some intrusive h264 decoder
changes. I'm not going to touch that.

> > So yes, removing things can mean progress.  
> 
> However, removing ffserver now doesn't, because the libraries
> have to keep backwards-compatibility until the next major bump
> anyway.

I'd agree with this, except I know that if the time comes for a major
bump that necessitates immediate removal of ffserver, a new discussion
about ffserver will break out, and the bump (or removal of the
deprecated things ffserver relies on) would be delayed. If this is how
development work (maybe it does, maybe it doesn't), then shitflinging
is an inherent part of the project's developer culture and the only way
to achieve something. Blergh.

Maybe this is not a problem anymore. ffserver.c still brings up some
deprecation warnings, but surely michaelni will send a patch soon to
fix those.

This discussion is worth leading in general anyway.

> > Nobody would care if ffserver actually used the ffmpeg libs correctly.
> > But it still requires ffserver-specific code in libavformat. And even
> > after all of michaelni's oddly-timed hard work to cleanup ffserver,
> > ffserver itself uses internal libavformat stuff (see
> > libavformat/libavformat.v - it also includes internal headers).  
> 
> Yes, that's the last point that needs to be fixed before the next
> major bump if ffserver is to be kept.

I didn't check whether the internal libavformat would break ffserver on
the next bump, or if there are other hidden internal or deprecated API
uses, but yeah.

> > This project is frustrating because it puts features (even broken,
> > half-implemented ones) over robust implementation and ease of use, and
> > on top of it is unable to enforce any policy or decisions the community
> > may have made. You have to waste your time discussing about nothing to
> > overly... self-confident... people when trying to make a change.  
> 
> You don't have to waste everyone's time complaining like that.
> It'd be much better if you'd instead spend the time working on reducing
> the backlog in the merges from Libav.

I could as well say: you don't have to waste everyone's time defending
ffserver, you could just spend time on fixing it instead.

You don't have to say anything about the general way ffmpeg development
works?

> The next bump, which is already discussed on the Libav mailing list, will
> be merged that much sooner.

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

Re: [FFmpeg-devel] [PATCH] Support limiting the number of pixels per image

[wm4]

On Thu,  8 Dec 2016 19:30:25 +0100
Michael Niedermayer  wrote:

> TODO: split into 2 patches (one per lib), docs & bump
> 
> This allows preventing some OOM and "slow decoding" cases by limiting the 
> maximum resolution
> this may be useful to avoid fuzzers getting stuck in boring cases
> 
> Signed-off-by: Michael Niedermayer 
> ---
>  libavcodec/avcodec.h |  8 
>  libavcodec/options_table.h   |  1 +
>  libavutil/imgutils.c | 22 ++
>  tests/ref/fate/api-mjpeg-codec-param |  2 ++
>  tests/ref/fate/api-png-codec-param   |  2 ++
>  5 files changed, 31 insertions(+), 4 deletions(-)
> 
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index 7ac2adaf66..81052b10ef 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -3570,6 +3570,14 @@ typedef struct AVCodecContext {
>   */
>  int trailing_padding;
>  
> +/**
> + * The number of pixels per image to maximally accept.
> + *
> + * - decoding: set by user
> + * - encoding: unused
> + */
> +int max_pixels;
> +
>  } AVCodecContext;
>  
>  AVRational av_codec_get_pkt_timebase (const AVCodecContext *avctx);
> diff --git a/libavcodec/options_table.h b/libavcodec/options_table.h
> index ee79859953..2f5eb252fe 100644
> --- a/libavcodec/options_table.h
> +++ b/libavcodec/options_table.h
> @@ -570,6 +570,7 @@ static const AVOption avcodec_options[] = {
>  {"codec_whitelist", "List of decoders that are allowed to be used", 
> OFFSET(codec_whitelist), AV_OPT_TYPE_STRING, { .str = NULL },  CHAR_MIN, 
> CHAR_MAX, A|V|S|D },
>  {"pixel_format", "set pixel format", OFFSET(pix_fmt), AV_OPT_TYPE_PIXEL_FMT, 
> {.i64=AV_PIX_FMT_NONE}, -1, INT_MAX, 0 },
>  {"video_size", "set video size", OFFSET(width), AV_OPT_TYPE_IMAGE_SIZE, 
> {.str=NULL}, 0, INT_MAX, 0 },
> +{"max_pixels", "Maximum number of pixels", OFFSET(max_pixels), 
> AV_OPT_TYPE_INT, {.i64 = INT_MAX }, 0, INT_MAX, A|V|S|D },
>  {NULL},
>  };
>  
> diff --git a/libavutil/imgutils.c b/libavutil/imgutils.c
> index 37808e53d0..7c42ec7e17 100644
> --- a/libavutil/imgutils.c
> +++ b/libavutil/imgutils.c
> @@ -30,6 +30,7 @@
>  #include "mathematics.h"
>  #include "pixdesc.h"
>  #include "rational.h"
> +#include "opt.h"
>  
>  void av_image_fill_max_pixsteps(int max_pixsteps[4], int 
> max_pixstep_comps[4],
>  const AVPixFmtDescriptor *pixdesc)
> @@ -256,11 +257,24 @@ int av_image_check_size(unsigned int w, unsigned int h, 
> int log_offset, void *lo
>  .log_ctx= log_ctx,
>  };
>  
> -if ((int)w>0 && (int)h>0 && (w+128)*(uint64_t)(h+128) < INT_MAX/8)
> -return 0;
> +if ((int)w<=0 || (int)h<=0 || (w+128)*(uint64_t)(h+128) >= INT_MAX/8) {
> +av_log(, AV_LOG_ERROR, "Picture size %ux%u is invalid\n", 
> w, h);
> +return AVERROR(EINVAL);
> +}
>  
> -av_log(, AV_LOG_ERROR, "Picture size %ux%u is invalid\n", w, h);
> -return AVERROR(EINVAL);
> +if (log_ctx) {
> +int64_t max = INT_MAX;
> +if (av_opt_get_int(log_ctx, "max_pixels",  AV_OPT_SEARCH_CHILDREN, 
> ) >= 0) {

IMHO terrible implementation. Just add a new function that takes an
honest argument?

> +if (w*h > max) {
> +av_log(, AV_LOG_ERROR,
> +   "Picture size %ux%u exceeds specified max pixel count 
> %"PRId64"\n",
> +   w, h, max);
> +return AVERROR(EINVAL);
> +}
> +}
> +}
> +
> +return 0;
>  }
>  
>  int av_image_check_sar(unsigned int w, unsigned int h, AVRational sar)
> diff --git a/tests/ref/fate/api-mjpeg-codec-param 
> b/tests/ref/fate/api-mjpeg-codec-param
> index c67d1b1ab0..08313adab3 100644
> --- a/tests/ref/fate/api-mjpeg-codec-param
> +++ b/tests/ref/fate/api-mjpeg-codec-param
> @@ -155,6 +155,7 @@ stream=0, decode=0
>  codec_whitelist=
>  pixel_format=yuvj422p
>  video_size=400x225
> +max_pixels=2147483647
>  stream=0, decode=1
>  b=0
>  ab=0
> @@ -312,3 +313,4 @@ stream=0, decode=1
>  codec_whitelist=
>  pixel_format=yuvj422p
>  video_size=400x225
> +max_pixels=2147483647
> diff --git a/tests/ref/fate/api-png-codec-param 
> b/tests/ref/fate/api-png-codec-param
> index bd53441894..7a9a921461 100644
> --- a/tests/ref/fate/api-png-codec-param
> +++ b/tests/ref/fate/api-png-codec-param
> @@ -155,6 +155,7 @@ stream=0, decode=0
>  codec_whitelist=
>  pixel_format=rgba
>  video_size=128x128
> +max_pixels=2147483647
>  stream=0, decode=1
>  b=0
>  ab=0
> @@ -312,3 +313,4 @@ stream=0, decode=1
>  codec_whitelist=
>  pixel_format=rgba
>  video_size=128x128
> +max_pixels=2147483647

In general I'd rather have the current pixel limit _removed_. It causes
problems with processing high-res images.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org

Re: [FFmpeg-devel] [PATCH] avformat/options_table: Set the default maximum number of streams to 100

[wm4]

On Thu, 8 Dec 2016 20:57:05 +0100
Michael Niedermayer  wrote:

> On Thu, Dec 08, 2016 at 07:48:46PM +0100, wm4 wrote:
> > On Thu, 8 Dec 2016 19:36:20 +0100
> > Michael Niedermayer  wrote:
> >   
> > > On Thu, Dec 08, 2016 at 07:25:59PM +0100, wm4 wrote:  
> > > > On Thu,  8 Dec 2016 18:37:42 +0100
> > > > Michael Niedermayer  wrote:
> > > > 
> > > > > Suggested-by: Andreas Cadhalpun 
> > > > > Signed-off-by: Michael Niedermayer 
> > > > > ---
> > > > >  libavformat/options_table.h | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/libavformat/options_table.h b/libavformat/options_table.h
> > > > > index d5448e503f..19cb87ae4e 100644
> > > > > --- a/libavformat/options_table.h
> > > > > +++ b/libavformat/options_table.h
> > > > > @@ -105,7 +105,7 @@ static const AVOption avformat_options[] = {
> > > > >  {"format_whitelist", "List of demuxers that are allowed to be used", 
> > > > > OFFSET(format_whitelist), AV_OPT_TYPE_STRING, { .str = NULL },  
> > > > > CHAR_MIN, CHAR_MAX, D },
> > > > >  {"protocol_whitelist", "List of protocols that are allowed to be 
> > > > > used", OFFSET(protocol_whitelist), AV_OPT_TYPE_STRING, { .str = NULL 
> > > > > },  CHAR_MIN, CHAR_MAX, D },
> > > > >  {"protocol_blacklist", "List of protocols that are not allowed to be 
> > > > > used", OFFSET(protocol_blacklist), AV_OPT_TYPE_STRING, { .str = NULL 
> > > > > },  CHAR_MIN, CHAR_MAX, D },
> > > > > -{"max_streams", "maximum number of streams", OFFSET(max_streams), 
> > > > > AV_OPT_TYPE_INT, { .i64 = INT_MAX }, 0, INT_MAX, D },
> > > > > +{"max_streams", "maximum number of streams", OFFSET(max_streams), 
> > > > > AV_OPT_TYPE_INT, { .i64 = 100 }, 0, INT_MAX, D },
> > > > >  {NULL},
> > > > >  };
> > > > >  
> > > > 
> > > > That seems awfully low. Why limit stream count to 100,
> > > 
> > > Is this too little for real world streams ?
> > > what limit would not interfere with a positive user experience ?
> > > 
> > >   
> > > > while allowing
> > > > e.g. 2GB large font attachments?
> > > 
> > > theres no limit on attachments currently except the natural int size
> > > we may want to have a tighter limit there too
> > > 
> > >   
> > 
> > This will lead to thousands of options tuning various limits that
> > 1. nobody wants to use, 2. even if they want, will not find, and 3. are
> > ugly and intrusive.
> > 
> > And then there'll probably still be a way to easily OOM ffmpeg, and
> > using a sandbox will still be superior over setting these options.  
> 
> Ive implemented the generic solution with one parameter in
> [FFmpeg-devel] [PATCH 3/3] avutil/mem: Support limiting the heap usage
> 
> which you and others dont like
> i understand why this isnt liked and i understand why many seperate
> options arent liked but
> 
> Either of them is needed or FFmpeg cannot saftely be used via normal
> lib mechanisms and must be run as a seperate process that the main
> app has to expect to crash with out of memory. (if the input is
> untrusted media)
> 
> This is something the community must decide.
> A. Is a heap limit for av_*alloc*() acceptable ?
> B. Are case based limits acceptable ?
> C. document that libavcodec, libavformat and ffmpeg must be run as a
>seperate process if the media comes from untrusted sources.
> 
> one of these options has to be choosen.
> 
> also even if C is choosen, a small set of limits on the main parameters
> still is needed to allow efficient fuzzing, all issues reported
> by oss-fuzz recently are "hangs" due to slow decoding,
> with the pixel max patch it instead of being slow hits this:
> Picture size 512x65520 exceeds specified max pixel count 414719
> in the case i tried
> 
> [...]

No to A and B. Those are hacks - which means they work for some cases,
but will cause more problems than they solve eventually. A has
unacceptable global state, B would require more and more options to
control uninteresting details to enforce memory allocation limits
consequently and would never be complete.

In theory it would be nice if you could pass some sort of custom
allocation context to the libs, which would provide memory allocation
callbacks. Then an API user could limit the amount of memory some
ffmpeg component allocates. This would of course not be global state,
but a context passed to AVCodecContext or individual functions.

On one hand, this would be very intrusive - requiring all allocating
functions to use such a context. On the other hand, we need something
similar to remove the global av_log callback, so there might be an
argument there.

But in general this seems like something for per-OS or per-user limits.

Btw. I doubt there's any serious service out there that does not run
ffmpeg in a sandbox for processing user uploads and such.
___
ffmpeg-devel mailing list

Re: [FFmpeg-devel] [PATCH] avformat/options_table: Set the default maximum number of streams to 100

[wm4]

On Fri, 9 Dec 2016 02:44:11 +0100
Michael Niedermayer  wrote:

> On Thu, Dec 08, 2016 at 09:47:53PM +0100, Nicolas George wrote:
> > L'octidi 18 frimaire, an CCXXV, Michael Niedermayer a écrit :  
> > > A. Is a heap limit for av_*alloc*() acceptable ?
> > > B. Are case based limits acceptable ?  
> > 
> > No. This is the task of the operating system.
> >   
> 
> > > also even if C is choosen, a small set of limits on the main parameters
> > > still is needed to allow efficient fuzzing, all issues reported
> > > by oss-fuzz recently are "hangs" due to slow decoding,  
> > 
> > Then set a limit at the operating system level.  
> 
> You are misunderstanding the problem i think
> 
> The goal of a fuzzer is to find bugs, crashes, undefined, bad things,
> OOM, hangs.
> 
> If the code under test can allocate arbitrary amounts of memory and
> take arbitrary amounts of time in a significant number of non-bug
> cases then the fuzzer cannot reliably find the corresponding bugs.
> 
> moving the threshold of where to declare something OOM or hang around
> will not solve this.
> blocking high resolution, high channel count, high stream count
> cases OTOH should improve the rate of false positives.
> 
> also, secondary, resources spent on waiting for hangs to separate from
> slow decoding and real OOM to separate from cases just needing alot
> of memory, are resources that could be used for other things like
> fuzzing more seperate cases.
> 
> but either way, iam the wrong one to disscuss changes to oss-fuzz with
> if you do have ideas that would improve it ...
> 
> [...]
> 

I'm not sure why we need to accommodate the very special needs of
fuzzers now, instead of fuzzers finding ways to avoid these situations.

Fuzzers could for example just inject their own malloc impl into the
process, that limits allocations, or something.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/2] Avoid using the term "file" and prefer "url" in some docs and comments

[wm4]

On Fri, 9 Dec 2016 03:48:39 +0100
Michael Niedermayer  wrote:

> On Thu, Dec 08, 2016 at 11:13:16AM -0900, Lou Logan wrote:
> > On Mon,  5 Dec 2016 13:52:50 +0100, Michael Niedermayer wrote:
> >   
> > > This should make it less ambigous that these are URLs
> > > 
> > > Signed-off-by: Michael Niedermayer 
> > > ---
> > >  doc/ffmpeg.texi  | 18 +-
> > >  doc/ffplay.texi  |  6 +++---
> > >  doc/ffprobe.texi | 10 +-
> > >  ffmpeg_opt.c |  4 ++--
> > >  4 files changed, 19 insertions(+), 19 deletions(-)  
> > 
> > Although this is a trivial patch, approximately 7 hours between sending
> > a patch and applying without feedback isn't enough time. At least 24
> > hours would be preferrable.  
> 
> there were open and fully public security bugs, the use of untrusted
> filenames which look like urls aka files as in
> "http://someserver.com;
> would allow potential remote code execution

I guess "security bugs" now justify any kind of change?

It's clear that a user has to prefix filenames with file: or so, since
it will interpret various strings as not-files (like as an option or an
URL). Thus it's not a security bug, just an user error.

> i applied this quickly as it seemed important to me to clarify that
> the command line arguments are not just normal file names
> in addition to fixing the bug which depended on such files
> 
> can you help me clarify and improve this further ?
> I suspect you can reword this quicker yourself than with me messing
> around further
> 
> The really important point is that one cannot saftely put a random
> untrusted string or filename in place of these arguments.
> untrusted filenames needs "file:" prefix at least
> 
> Thanks and sorry for havning applied this so quickly
> 
> [...]
> 

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

Re: [FFmpeg-devel] [PATCH 1/2] Avoid using the term "file" and prefer "url" in some docs and comments

[wm4]

On Mon, 5 Dec 2016 21:10:00 +0100
Michael Niedermayer  wrote:

> On Mon, Dec 05, 2016 at 01:52:50PM +0100, Michael Niedermayer wrote:
> > This should make it less ambigous that these are URLs
> > 
> > Signed-off-by: Michael Niedermayer 
> > ---
> >  doc/ffmpeg.texi  | 18 +-
> >  doc/ffplay.texi  |  6 +++---
> >  doc/ffprobe.texi | 10 +-
> >  ffmpeg_opt.c |  4 ++--
> >  4 files changed, 19 insertions(+), 19 deletions(-)  
> 
> applied
> 
> 
> [...]

I guess my remarks mean nothing.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel