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

[Andreas Cadhalpun]

On 26.01.2017 13:59, Ronald S. Bultje wrote:
> On Wed, Jan 25, 2017 at 8:56 PM, Andreas Cadhalpun <
> andreas.cadhal...@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

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

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

[Ronald S. Bultje]

Hi,

On Wed, Jan 25, 2017 at 8:56 PM, Andreas Cadhalpun <
andreas.cadhal...@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.cadhal...@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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Andreas Cadhalpun]

On 26.01.2017 02:26, Ronald S. Bultje wrote:
> Hi,
> 
> On Wed, Jan 25, 2017 at 8:20 PM, Andreas Cadhalpun <
> andreas.cadhal...@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

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

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

[Ronald S. Bultje]

Hi,

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

> On 07.01.2017 13:44, Ronald S. Bultje wrote:
> > On Fri, Jan 6, 2017 at 9:35 PM, Michael Niedermayer
> 
> > 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 
>  ---
>   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(>gb, 8);
>   s->avctx->color_trc   = get_bits(>gb, 8);
>   s->avctx->colorspace  = get_bits(>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(>gb, 14);
>   skip_bits(>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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Andreas Cadhalpun]

On 07.01.2017 13:44, Ronald S. Bultje wrote:
> On Fri, Jan 6, 2017 at 9:35 PM, Michael Niedermayer 
> 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 
 ---
  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(>gb, 8);
  s->avctx->color_trc   = get_bits(>gb, 8);
  s->avctx->colorspace  = get_bits(>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(>gb, 14);
  skip_bits(>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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Ronald S. Bultje]

Hi,

On Fri, Jan 6, 2017 at 9:35 PM, Michael Niedermayer 
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 
> > > ---
> > >  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(>gb, 8);
> > >  s->avctx->color_trc   = get_bits(>gb, 8);
> > >  s->avctx->colorspace  = get_bits(>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(>gb, 14);
> > >  skip_bits(>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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Michael Niedermayer]

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 
> > ---
> >  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(>gb, 8);
> >  s->avctx->color_trc   = get_bits(>gb, 8);
> >  s->avctx->colorspace  = get_bits(>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(>gb, 14);
> >  skip_bits(>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


[...]
-- 
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 1/3] mpeg12dec: validate color space

[Andreas Cadhalpun]

On 23.12.2016 00:57, Andreas Cadhalpun wrote:
> Signed-off-by: Andreas Cadhalpun 
> ---
>  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(>gb, 8);
>  s->avctx->color_trc   = get_bits(>gb, 8);
>  s->avctx->colorspace  = get_bits(>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(>gb, 14);
>  skip_bits(>gb, 1); // marker
> 

Ping for the series.

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

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

[Andreas Cadhalpun]

Signed-off-by: Andreas Cadhalpun 
---
 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(>gb, 8);
 s->avctx->color_trc   = get_bits(>gb, 8);
 s->avctx->colorspace  = get_bits(>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(>gb, 14);
 skip_bits(>gb, 1); // marker
-- 
2.11.0

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