| Message ID | 73b49b76-c0bd-be78-b9a0-2b95dffc0f2a@googlemail.com |
|---|---|
| State | New |
| Headers | show |
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
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 [...]
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
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
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
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
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
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
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
Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> --- libavcodec/mpeg12dec.c | 4 ++++ 1 file changed, 4 insertions(+)