[FFmpeg-devel,1/3] mpeg12dec: validate color space

Submitted by Andreas Cadhalpun on Dec. 22, 2016, 11:57 p.m.

Details

Message ID 73b49b76-c0bd-be78-b9a0-2b95dffc0f2a@googlemail.com
State New
Headers show

Commit Message

Andreas Cadhalpun Dec. 22, 2016, 11:57 p.m.
Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
---
 libavcodec/mpeg12dec.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Andreas Cadhalpun Jan. 6, 2017, 8:43 p.m.
On 23.12.2016 00:57, Andreas Cadhalpun wrote:
> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
> ---
>  libavcodec/mpeg12dec.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c
> index 63979079c8..d3dc67ad6a 100644
> --- a/libavcodec/mpeg12dec.c
> +++ b/libavcodec/mpeg12dec.c
> @@ -1470,6 +1470,10 @@ static void mpeg_decode_sequence_display_extension(Mpeg1Context *s1)
>          s->avctx->color_primaries = get_bits(&s->gb, 8);
>          s->avctx->color_trc       = get_bits(&s->gb, 8);
>          s->avctx->colorspace      = get_bits(&s->gb, 8);
> +        if (!av_color_space_name(s->avctx->colorspace)) {
> +            av_log(s->avctx, AV_LOG_WARNING, "Invalid color space %d, setting to unspecified\n", s->avctx->colorspace);
> +            s->avctx->colorspace = AVCOL_SPC_UNSPECIFIED;
> +        }
>      }
>      w = get_bits(&s->gb, 14);
>      skip_bits(&s->gb, 1); // marker
> 

Ping for the series.

Best regards,
Andreas
Michael Niedermayer Jan. 7, 2017, 2:35 a.m.
On Fri, Jan 06, 2017 at 09:43:24PM +0100, Andreas Cadhalpun wrote:
> On 23.12.2016 00:57, Andreas Cadhalpun wrote:
> > Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
> > ---
> >  libavcodec/mpeg12dec.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c
> > index 63979079c8..d3dc67ad6a 100644
> > --- a/libavcodec/mpeg12dec.c
> > +++ b/libavcodec/mpeg12dec.c
> > @@ -1470,6 +1470,10 @@ static void mpeg_decode_sequence_display_extension(Mpeg1Context *s1)
> >          s->avctx->color_primaries = get_bits(&s->gb, 8);
> >          s->avctx->color_trc       = get_bits(&s->gb, 8);
> >          s->avctx->colorspace      = get_bits(&s->gb, 8);
> > +        if (!av_color_space_name(s->avctx->colorspace)) {
> > +            av_log(s->avctx, AV_LOG_WARNING, "Invalid color space %d, setting to unspecified\n", s->avctx->colorspace);
> > +            s->avctx->colorspace = AVCOL_SPC_UNSPECIFIED;
> > +        }
> >      }
> >      w = get_bits(&s->gb, 14);
> >      skip_bits(&s->gb, 1); // marker
> > 
> 
> Ping for the series.

i have no real objection to it.
iam used to see these being exported unchanged though so it feels a
bit odd


[...]
Ronald S. Bultje Jan. 7, 2017, 12:44 p.m.
Hi,

On Fri, Jan 6, 2017 at 9:35 PM, Michael Niedermayer <michael@niedermayer.cc>
wrote:

> On Fri, Jan 06, 2017 at 09:43:24PM +0100, Andreas Cadhalpun wrote:
> > On 23.12.2016 00:57, Andreas Cadhalpun wrote:
> > > Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
> > > ---
> > >  libavcodec/mpeg12dec.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c
> > > index 63979079c8..d3dc67ad6a 100644
> > > --- a/libavcodec/mpeg12dec.c
> > > +++ b/libavcodec/mpeg12dec.c
> > > @@ -1470,6 +1470,10 @@ static void mpeg_decode_sequence_display_extension(Mpeg1Context
> *s1)
> > >          s->avctx->color_primaries = get_bits(&s->gb, 8);
> > >          s->avctx->color_trc       = get_bits(&s->gb, 8);
> > >          s->avctx->colorspace      = get_bits(&s->gb, 8);
> > > +        if (!av_color_space_name(s->avctx->colorspace)) {
> > > +            av_log(s->avctx, AV_LOG_WARNING, "Invalid color space %d,
> setting to unspecified\n", s->avctx->colorspace);
> > > +            s->avctx->colorspace = AVCOL_SPC_UNSPECIFIED;
> > > +        }
> > >      }
> > >      w = get_bits(&s->gb, 14);
> > >      skip_bits(&s->gb, 1); // marker
> > >
> >
> > Ping for the series.
>
> i have no real objection to it.
> iam used to see these being exported unchanged though so it feels a
> bit odd


Doesn't this destroy exporting of newly added colorspaces that have no
explicit mention yet in libavutil? I'm not 100% sure this is a good thing.

Ronald
Andreas Cadhalpun Jan. 26, 2017, 1:20 a.m.
On 07.01.2017 13:44, Ronald S. Bultje wrote:
> On Fri, Jan 6, 2017 at 9:35 PM, Michael Niedermayer <michael@niedermayer.cc>
> wrote:
> 
>> On Fri, Jan 06, 2017 at 09:43:24PM +0100, Andreas Cadhalpun wrote:
>>> On 23.12.2016 00:57, Andreas Cadhalpun wrote:
>>>> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
>>>> ---
>>>>  libavcodec/mpeg12dec.c | 4 ++++
>>>>  1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c
>>>> index 63979079c8..d3dc67ad6a 100644
>>>> --- a/libavcodec/mpeg12dec.c
>>>> +++ b/libavcodec/mpeg12dec.c
>>>> @@ -1470,6 +1470,10 @@ static void mpeg_decode_sequence_display_extension(Mpeg1Context
>> *s1)
>>>>          s->avctx->color_primaries = get_bits(&s->gb, 8);
>>>>          s->avctx->color_trc       = get_bits(&s->gb, 8);
>>>>          s->avctx->colorspace      = get_bits(&s->gb, 8);
>>>> +        if (!av_color_space_name(s->avctx->colorspace)) {
>>>> +            av_log(s->avctx, AV_LOG_WARNING, "Invalid color space %d,
>> setting to unspecified\n", s->avctx->colorspace);
>>>> +            s->avctx->colorspace = AVCOL_SPC_UNSPECIFIED;
>>>> +        }
>>>>      }
>>>>      w = get_bits(&s->gb, 14);
>>>>      skip_bits(&s->gb, 1); // marker
>>>>
>>>
>>> Ping for the series.
>>
>> i have no real objection to it.
>> iam used to see these being exported unchanged though so it feels a
>> bit odd
> 
> 
> Doesn't this destroy exporting of newly added colorspaces that have no
> explicit mention yet in libavutil? I'm not 100% sure this is a good thing.

Yes. That's how it is already done in libavcodec/h264_ps.c, so it just
makes handling unknown values consistent.

Best regards,
Andreas
Ronald S. Bultje Jan. 26, 2017, 1:26 a.m.
Hi,

On Wed, Jan 25, 2017 at 8:20 PM, Andreas Cadhalpun <
andreas.cadhalpun@googlemail.com> wrote:

> On 07.01.2017 13:44, Ronald S. Bultje wrote:
> > On Fri, Jan 6, 2017 at 9:35 PM, Michael Niedermayer
> <michael@niedermayer.cc>
> > wrote:
> >
> >> On Fri, Jan 06, 2017 at 09:43:24PM +0100, Andreas Cadhalpun wrote:
> >>> On 23.12.2016 00:57, Andreas Cadhalpun wrote:
> >>>> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
> >>>> ---
> >>>>  libavcodec/mpeg12dec.c | 4 ++++
> >>>>  1 file changed, 4 insertions(+)
> >>>>
> >>>> diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c
> >>>> index 63979079c8..d3dc67ad6a 100644
> >>>> --- a/libavcodec/mpeg12dec.c
> >>>> +++ b/libavcodec/mpeg12dec.c
> >>>> @@ -1470,6 +1470,10 @@ static void mpeg_decode_sequence_display_
> extension(Mpeg1Context
> >> *s1)
> >>>>          s->avctx->color_primaries = get_bits(&s->gb, 8);
> >>>>          s->avctx->color_trc       = get_bits(&s->gb, 8);
> >>>>          s->avctx->colorspace      = get_bits(&s->gb, 8);
> >>>> +        if (!av_color_space_name(s->avctx->colorspace)) {
> >>>> +            av_log(s->avctx, AV_LOG_WARNING, "Invalid color space %d,
> >> setting to unspecified\n", s->avctx->colorspace);
> >>>> +            s->avctx->colorspace = AVCOL_SPC_UNSPECIFIED;
> >>>> +        }
> >>>>      }
> >>>>      w = get_bits(&s->gb, 14);
> >>>>      skip_bits(&s->gb, 1); // marker
> >>>>
> >>>
> >>> Ping for the series.
> >>
> >> i have no real objection to it.
> >> iam used to see these being exported unchanged though so it feels a
> >> bit odd
> >
> >
> > Doesn't this destroy exporting of newly added colorspaces that have no
> > explicit mention yet in libavutil? I'm not 100% sure this is a good
> thing.
>
> Yes. That's how it is already done in libavcodec/h264_ps.c, so it just
> makes handling unknown values consistent.


Changing h264_ps.c would also make things consistent.

The question is not whether changing A or B towards the other makes the
combination consistent. The question is which form of consistency is
better...

Ronald
Andreas Cadhalpun Jan. 26, 2017, 1:56 a.m.
On 26.01.2017 02:26, Ronald S. Bultje wrote:
> Hi,
> 
> On Wed, Jan 25, 2017 at 8:20 PM, Andreas Cadhalpun <
> andreas.cadhalpun@googlemail.com> wrote:
> 
>> On 07.01.2017 13:44, Ronald S. Bultje wrote:
>>> Doesn't this destroy exporting of newly added colorspaces that have no
>>> explicit mention yet in libavutil? I'm not 100% sure this is a good
>> thing.
>>
>> Yes. That's how it is already done in libavcodec/h264_ps.c, so it just
>> makes handling unknown values consistent.
> 
> 
> Changing h264_ps.c would also make things consistent.

No, because also libavcodec/options_table.h limits the colorspace to known values.

> The question is not whether changing A or B towards the other makes the
> combination consistent. The question is which form of consistency is
> better...

As new values don't get added that often, I don't see a problem with requiring
to explicitly add them to libavutil before being able to use them.

Best regards,
Andreas
Ronald S. Bultje Jan. 26, 2017, 12:59 p.m.
Hi,

On Wed, Jan 25, 2017 at 8:56 PM, Andreas Cadhalpun <
andreas.cadhalpun@googlemail.com> wrote:

> On 26.01.2017 02:26, Ronald S. Bultje wrote:
> > Hi,
> >
> > On Wed, Jan 25, 2017 at 8:20 PM, Andreas Cadhalpun <
> > andreas.cadhalpun@googlemail.com> wrote:
> >
> >> On 07.01.2017 13:44, Ronald S. Bultje wrote:
> >>> Doesn't this destroy exporting of newly added colorspaces that have no
> >>> explicit mention yet in libavutil? I'm not 100% sure this is a good
> >> thing.
> >>
> >> Yes. That's how it is already done in libavcodec/h264_ps.c, so it just
> >> makes handling unknown values consistent.
> >
> >
> > Changing h264_ps.c would also make things consistent.
>
> No, because also libavcodec/options_table.h limits the colorspace to known
> values.


That is input only, this is output.

> The question is not whether changing A or B towards the other makes the
> > combination consistent. The question is which form of consistency is
> > better...
>
> As new values don't get added that often, I don't see a problem with
> requiring
> to explicitly add them to libavutil before being able to use them.


That is because your use case for libavcodec is constrained, you use it
only for decoding videos and fuzzing. There are others in this community
that do a lot more than that with libavcodec.

Look, Andreas, I appreciate the work you're doing, I really do, but you
always pick a fight with every review I put out on your code. I don't
understand why. My reviews are not difficult to address, they really are
not. My reviews are actionable, and if the action is taken, I'm happy and
the code can be committed. Why pick a fight? What is the point? Do you
think I'm an idiot that has no clue what he's doing and should be shot down
because of that? Please appreciate that I do have some clue of what I'm
doing, and I am looking out for the health of the project, just like you,
but with a slightly different perspective to some things. If I'm wrong,
please point it out, I make mistakes also, but in cases like these, it can
also help to just drop it and move on. Resolving an issue is not losing, it
is winning.

Ronald
Andreas Cadhalpun Jan. 28, 2017, 12:19 a.m.
On 26.01.2017 13:59, Ronald S. Bultje wrote:
> On Wed, Jan 25, 2017 at 8:56 PM, Andreas Cadhalpun <
> andreas.cadhalpun@googlemail.com> wrote:
>> On 26.01.2017 02:26, Ronald S. Bultje wrote:
>>> The question is not whether changing A or B towards the other makes the
>>> combination consistent. The question is which form of consistency is
>>> better...
>>
>> As new values don't get added that often, I don't see a problem with
>> requiring
>> to explicitly add them to libavutil before being able to use them.
> 
> 
> That is because your use case for libavcodec is constrained, you use it
> only for decoding videos and fuzzing.

Please refrain from making such presumptuous, false claims in the future.

> There are others in this community
> that do a lot more than that with libavcodec.
> 
> Look, Andreas, I appreciate the work you're doing, I really do, but you
> always pick a fight with every review I put out on your code.

That's a grossly distorted picture of reality [1][2][3].

> I don't
> understand why. My reviews are not difficult to address, they really are
> not. My reviews are actionable, and if the action is taken, I'm happy and
> the code can be committed.

I appreciate your reviews, as long as they remain technical.

> Why pick a fight? What is the point? Do you
> think I'm an idiot that has no clue what he's doing and should be shot down
> because of that?

Such polemic phrases are inappropriate in any technical discussion.

> Please appreciate that I do have some clue of what I'm
> doing, and I am looking out for the health of the project, just like you,
> but with a slightly different perspective to some things. If I'm wrong,
> please point it out, I make mistakes also, but in cases like these, it can
> also help to just drop it and move on. Resolving an issue is not losing, it
> is winning.

Technical issues should be resolved with convincing arguments and not by
asking anyone who disagrees with you to move on.

For example, you could explain why you think that allowing unknown values
here is important, even though new ones are added only rarely.

Best regards,
Andreas


1: https://ffmpeg.org/pipermail/ffmpeg-devel/2017-January/205192.html
2: https://ffmpeg.org/pipermail/ffmpeg-devel/2017-January/205406.html
3: https://ffmpeg.org/pipermail/ffmpeg-devel/2017-January/205414.html

Patch hide | download patch | download mbox

diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c
index 63979079c8..d3dc67ad6a 100644
--- a/libavcodec/mpeg12dec.c
+++ b/libavcodec/mpeg12dec.c
@@ -1470,6 +1470,10 @@  static void mpeg_decode_sequence_display_extension(Mpeg1Context *s1)
         s->avctx->color_primaries = get_bits(&s->gb, 8);
         s->avctx->color_trc       = get_bits(&s->gb, 8);
         s->avctx->colorspace      = get_bits(&s->gb, 8);
+        if (!av_color_space_name(s->avctx->colorspace)) {
+            av_log(s->avctx, AV_LOG_WARNING, "Invalid color space %d, setting to unspecified\n", s->avctx->colorspace);
+            s->avctx->colorspace = AVCOL_SPC_UNSPECIFIED;
+        }
     }
     w = get_bits(&s->gb, 14);
     skip_bits(&s->gb, 1); // marker