| Message ID | 201703241040.21244.cehoyos@ag.or.at |
|---|---|
| State | New |
| Headers | show |
On Fri, 24 Mar 2017 10:40:21 +0100 Carl Eugen Hoyos <cehoyos@ag.or.at> wrote: > From 1c249440c62271565be12112f321ad9aa6a922fb Mon Sep 17 00:00:00 2001 > From: Carl Eugen Hoyos <cehoyos@ag.or.at> > Date: Fri, 24 Mar 2017 10:38:22 +0100 > Subject: [PATCH] lavc/h264_ps: Check that chroma_location is within limits. > > Fixes ticket #6255. > --- > libavcodec/h264_ps.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/libavcodec/h264_ps.c b/libavcodec/h264_ps.c > index b78ad25..55e6a6e 100644 > --- a/libavcodec/h264_ps.c > +++ b/libavcodec/h264_ps.c > @@ -181,6 +181,8 @@ static inline int decode_vui_parameters(GetBitContext *gb, AVCodecContext *avctx > if (get_bits1(gb)) { > /* chroma_sample_location_type_top_field */ > avctx->chroma_sample_location = get_ue_golomb(gb) + 1; > + if (avctx->chroma_sample_location >= AVCHROMA_LOC_NB) > + avctx->chroma_sample_location = AVCHROMA_LOC_UNSPECIFIED; AVCHROMA_LOC_NB is not part of the libavutil ABI, so libavcodec can in theory not use it. > get_ue_golomb(gb); /* chroma_sample_location_type_bottom_field */ > } >
2017-03-24 10:59 GMT+01:00 wm4 <nfxjfg@googlemail.com>: > On Fri, 24 Mar 2017 10:40:21 +0100 > Carl Eugen Hoyos <cehoyos@ag.or.at> wrote: > >> From 1c249440c62271565be12112f321ad9aa6a922fb Mon Sep 17 00:00:00 2001 >> From: Carl Eugen Hoyos <cehoyos@ag.or.at> >> Date: Fri, 24 Mar 2017 10:38:22 +0100 >> Subject: [PATCH] lavc/h264_ps: Check that chroma_location is within limits. >> >> Fixes ticket #6255. >> --- >> libavcodec/h264_ps.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/libavcodec/h264_ps.c b/libavcodec/h264_ps.c >> index b78ad25..55e6a6e 100644 >> --- a/libavcodec/h264_ps.c >> +++ b/libavcodec/h264_ps.c >> @@ -181,6 +181,8 @@ static inline int decode_vui_parameters(GetBitContext *gb, AVCodecContext *avctx >> if (get_bits1(gb)) { >> /* chroma_sample_location_type_top_field */ >> avctx->chroma_sample_location = get_ue_golomb(gb) + 1; >> + if (avctx->chroma_sample_location >= AVCHROMA_LOC_NB) >> + avctx->chroma_sample_location = AVCHROMA_LOC_UNSPECIFIED; > > AVCHROMA_LOC_NB is not part of the libavutil ABI, so libavcodec can in > theory not use it. I suspect you misread this rule (we cannot make _NB smaller), in any case please look a few lines up from the patch, there are several similar cases there. Carl Eugen
On Fri, 24 Mar 2017 11:23:44 +0100 Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > 2017-03-24 10:59 GMT+01:00 wm4 <nfxjfg@googlemail.com>: > > On Fri, 24 Mar 2017 10:40:21 +0100 > > Carl Eugen Hoyos <cehoyos@ag.or.at> wrote: > > > >> From 1c249440c62271565be12112f321ad9aa6a922fb Mon Sep 17 00:00:00 2001 > >> From: Carl Eugen Hoyos <cehoyos@ag.or.at> > >> Date: Fri, 24 Mar 2017 10:38:22 +0100 > >> Subject: [PATCH] lavc/h264_ps: Check that chroma_location is within limits. > >> > >> Fixes ticket #6255. > >> --- > >> libavcodec/h264_ps.c | 2 ++ > >> 1 file changed, 2 insertions(+) > >> > >> diff --git a/libavcodec/h264_ps.c b/libavcodec/h264_ps.c > >> index b78ad25..55e6a6e 100644 > >> --- a/libavcodec/h264_ps.c > >> +++ b/libavcodec/h264_ps.c > >> @@ -181,6 +181,8 @@ static inline int decode_vui_parameters(GetBitContext *gb, AVCodecContext *avctx > >> if (get_bits1(gb)) { > >> /* chroma_sample_location_type_top_field */ > >> avctx->chroma_sample_location = get_ue_golomb(gb) + 1; > >> + if (avctx->chroma_sample_location >= AVCHROMA_LOC_NB) > >> + avctx->chroma_sample_location = AVCHROMA_LOC_UNSPECIFIED; > > > > AVCHROMA_LOC_NB is not part of the libavutil ABI, so libavcodec can in > > theory not use it. > > I suspect you misread this rule (we cannot make _NB smaller), in any case > please look a few lines up from the patch, there are several similar > cases there. Quoting the source code: AVCHROMA_LOC_NB ///< Not part of ABI Seems pretty clear.
Hi,
On Fri, Mar 24, 2017 at 6:23 AM, Carl Eugen Hoyos <ceffmpeg@gmail.com>
wrote:
> there are several similar cases there.
That is classically how ff_ symbols became public API. Please don't use
that argument ever again.
Ronald
Hi, On Fri, Mar 24, 2017 at 7:40 AM, Ronald S. Bultje <rsbultje@gmail.com> wrote: > Hi, > > On Fri, Mar 24, 2017 at 6:23 AM, Carl Eugen Hoyos <ceffmpeg@gmail.com> > wrote: > >> there are several similar cases there. > > > That is classically how ff_ symbols became public API. Please don't use > that argument ever again. > Btw, I'm not arguing with your suggestion that maybe _NB should be redefined to be part of our API but the value in runtime libavutil may be bigger. It may or may not be a good idea, I don't know. I'm merely arguing with the point that since we violate the API already, it's OK to violate it some more. Ronald
On Fri, 24 Mar 2017 07:48:53 -0400 "Ronald S. Bultje" <rsbultje@gmail.com> wrote: > Hi, > > On Fri, Mar 24, 2017 at 7:40 AM, Ronald S. Bultje <rsbultje@gmail.com> > wrote: > > > Hi, > > > > On Fri, Mar 24, 2017 at 6:23 AM, Carl Eugen Hoyos <ceffmpeg@gmail.com> > > wrote: > > > >> there are several similar cases there. > > > > > > That is classically how ff_ symbols became public API. Please don't use > > that argument ever again. > > > > Btw, I'm not arguing with your suggestion that maybe _NB should be > redefined to be part of our API but the value in runtime libavutil may be > bigger. It may or may not be a good idea, I don't know. > > I'm merely arguing with the point that since we violate the API already, > it's OK to violate it some more. I don't think this is even needed here: - the chroma loc values obviously mirror the h264 standard, so you simply get the bitstream value (including new extension that might be added in the future) - everything outside libavutil has to handle the situation that there are unknown chroma loc values (that can be higher than AVCHROMA_LOC_NB at the time of compilation) - libavutil itself handles out of bounds chroma loc values where it matters
On Fri, Mar 24, 2017 at 10:40:21AM +0100, Carl Eugen Hoyos wrote: > Hi! > > Attached patch fixes #6255. > > Please comment, Carl Eugen > From 1c249440c62271565be12112f321ad9aa6a922fb Mon Sep 17 00:00:00 2001 > From: Carl Eugen Hoyos <cehoyos@ag.or.at> > Date: Fri, 24 Mar 2017 10:38:22 +0100 > Subject: [PATCH] lavc/h264_ps: Check that chroma_location is within limits. > > Fixes ticket #6255. > --- > libavcodec/h264_ps.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/libavcodec/h264_ps.c b/libavcodec/h264_ps.c > index b78ad25..55e6a6e 100644 > --- a/libavcodec/h264_ps.c > +++ b/libavcodec/h264_ps.c > @@ -181,6 +181,8 @@ static inline int decode_vui_parameters(GetBitContext *gb, AVCodecContext *avctx > if (get_bits1(gb)) { > /* chroma_sample_location_type_top_field */ > avctx->chroma_sample_location = get_ue_golomb(gb) + 1; > + if (avctx->chroma_sample_location >= AVCHROMA_LOC_NB) > + avctx->chroma_sample_location = AVCHROMA_LOC_UNSPECIFIED; if (av_chroma_location_name(...)) ...
Hi, On Fri, Mar 24, 2017 at 10:05 AM, Clément Bœsch <u@pkh.me> wrote: > On Fri, Mar 24, 2017 at 10:40:21AM +0100, Carl Eugen Hoyos wrote: > > Hi! > > > > Attached patch fixes #6255. > > > > Please comment, Carl Eugen > > > From 1c249440c62271565be12112f321ad9aa6a922fb Mon Sep 17 00:00:00 2001 > > From: Carl Eugen Hoyos <cehoyos@ag.or.at> > > Date: Fri, 24 Mar 2017 10:38:22 +0100 > > Subject: [PATCH] lavc/h264_ps: Check that chroma_location is within > limits. > > > > Fixes ticket #6255. > > --- > > libavcodec/h264_ps.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/libavcodec/h264_ps.c b/libavcodec/h264_ps.c > > index b78ad25..55e6a6e 100644 > > --- a/libavcodec/h264_ps.c > > +++ b/libavcodec/h264_ps.c > > @@ -181,6 +181,8 @@ static inline int decode_vui_parameters(GetBitContext > *gb, AVCodecContext *avctx > > if (get_bits1(gb)) { > > /* chroma_sample_location_type_top_field */ > > avctx->chroma_sample_location = get_ue_golomb(gb) + 1; > > + if (avctx->chroma_sample_location >= AVCHROMA_LOC_NB) > > + avctx->chroma_sample_location = AVCHROMA_LOC_UNSPECIFIED; > > if (av_chroma_location_name(...)) ... Looking at the bug report mentioned, I wonder why we have this inconsistency in our code: s = av_get_colorspace_name(par->color_space); if (s) print_str ("color_space", s); else print_str_opt("color_space", "unknown"); [..] if (par->chroma_location != AVCHROMA_LOC_UNSPECIFIED) print_str("chroma_location", av_chroma_location_name(par->chroma_location)); else print_str_opt("chroma_location", av_chroma_location_name(par->chroma_location)); This seems inconsistent. Making the second (chroma location printing) behave like the first (colorspace printing: print "unknown" for invalid entries) would make the above unnecessary. There is also the risk with the initial patch that compiling libavcodec against a newer libavutil but then runtime linking it with another (older) one would lead to crashes. Clement's idea prevents that, but leads to loss of information. (I'm not saying the current approach of returning a potentially out-of-range value and expecting the end user to check/clip is any better, since some are likely to forget leading to crashes, but we need to think this through and make behaviour of the above code consistent with whatever we come up with.) Ronald
On Fri, Mar 24, 2017 at 10:16:42AM -0400, Ronald S. Bultje wrote: [...] > There is also the risk with the initial patch that compiling libavcodec > against a newer libavutil but then runtime linking it with another (older) > one would lead to crashes. This should not be possible the minor version should always be bumped when a new colorspace or other is added. Building against that should not end up being linked to older, it in fact could fail linking hard if it was from missing symbols > Clement's idea prevents that, but leads to loss > of information. (I'm not saying the current approach of returning a > potentially out-of-range value and expecting the end user to check/clip is > any better, since some are likely to forget leading to crashes, but we need > to think this through and make behaviour of the above code consistent with > whatever we come up with.) I think we are missing a clear documentation if out of the enum range values are allowed in the field or not. We could allow code to set any value in the field it likes and require users to deal with this or Forbid code to ever set a value not supported by the current compile time enum. or Forbid code to ever set a value not supported by the current link time enum. Code reading still has to deal with new values as libs can be updated and new values can be added to the enum. I belive ubitux and carl implemented 2 of these options with their patches Each has its own advantages and disadvanatges A: Export whatever is in the file as is B: Never set a value that isnt known at the time a human updated it no possible conflicts with future adde enum values C: Never have a value set that isnt supported by the libs functions like av_get_colorspace_name() But what i really feel is relevant here is a different aspect Andreas worked on cleaning this up previously, and carl and ubitux now with these patches here continued this. And we previously and now have near identical discussions. I think we should either make a decission in FFmpeg on which way it should be and document it. Or leave the people working on making things consistent to do it as they prefer, they too should document it of course. Repeatly starting a discussion when some patch comes up on a subject but then failing to really reach a decission and failing to document it and repeating the cycle over and over is not good [...]
Hi, On Fri, Mar 24, 2017 at 9:15 PM, Michael Niedermayer <michael@niedermayer.cc > wrote: > Repeatly starting a discussion when some patch comes up on a subject > but then failing to really reach a decission and failing to document > it and repeating the cycle over and over is not good +1. So... Vote? Ronald
2017-03-24 15:16 GMT+01:00 Ronald S. Bultje <rsbultje@gmail.com>: > There is also the risk with the initial patch that compiling libavcodec > against a newer libavutil but then runtime linking it with another (older) > one would lead to crashes. This is never supported (and is of course expected to either not start or crash). Sorry, but everything you have written in this thread so far is either plain wrong or unrelated and everything makes contributing to this project very difficult. In addition, giving the fact that you seem (?) to have stopped contributing may lead an innocent reader to the conclusion that these comments aren't meant as reviews. Carl Eugen
On Mon, 27 Mar 2017 09:31:39 +0200 Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > 2017-03-24 15:16 GMT+01:00 Ronald S. Bultje <rsbultje@gmail.com>: > > There is also the risk with the initial patch that compiling libavcodec > > against a newer libavutil but then runtime linking it with another (older) > > one would lead to crashes. > > This is never supported (and is of course expected to either not > start or crash). > > Sorry, but everything you have written in this thread so far is > either plain wrong or unrelated and everything makes > contributing to this project very difficult. I know, contributing to this project can be hard, but I see other reasons for this conclusions. > In addition, giving the fact that you seem (?) to have stopped > contributing may lead an innocent reader to the conclusion > that these comments aren't meant as reviews. You must be joking. An active member of the community and principal author of our vp9 decoder has "stopped contributing" according to your opinion?
On 3/27/17, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > 2017-03-24 15:16 GMT+01:00 Ronald S. Bultje <rsbultje@gmail.com>: >> There is also the risk with the initial patch that compiling libavcodec >> against a newer libavutil but then runtime linking it with another (older) >> one would lead to crashes. > > This is never supported (and is of course expected to either not > start or crash). > > Sorry, but everything you have written in this thread so far is > either plain wrong or unrelated and everything makes > contributing to this project very difficult. > In addition, giving the fact that you seem (?) to have stopped > contributing may lead an innocent reader to the conclusion > that these comments aren't meant as reviews. I stopped contributing but Carl come to the rescue. Thanks Carl, you saved a day.
2017-03-27 11:40 GMT+02:00 wm4 <nfxjfg@googlemail.com>: > On Mon, 27 Mar 2017 09:31:39 +0200 > Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > >> 2017-03-24 15:16 GMT+01:00 Ronald S. Bultje <rsbultje@gmail.com>: >> In addition, giving the fact that you seem (?) to have stopped >> contributing may lead an innocent reader to the conclusion >> that these comments aren't meant as reviews. > > You must be joking. Sure? Carl Eugen
On Mon, 27 Mar 2017 13:29:35 +0200 Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > 2017-03-27 11:40 GMT+02:00 wm4 <nfxjfg@googlemail.com>: > > On Mon, 27 Mar 2017 09:31:39 +0200 > > Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > > > >> 2017-03-24 15:16 GMT+01:00 Ronald S. Bultje <rsbultje@gmail.com>: > > >> In addition, giving the fact that you seem (?) to have stopped > >> contributing may lead an innocent reader to the conclusion > >> that these comments aren't meant as reviews. > > > > You must be joking. > > Sure? So you're not?
2017-03-27 13:41 GMT+02:00 wm4 <nfxjfg@googlemail.com>: > On Mon, 27 Mar 2017 13:29:35 +0200 > Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > >> 2017-03-27 11:40 GMT+02:00 wm4 <nfxjfg@googlemail.com>: >> > On Mon, 27 Mar 2017 09:31:39 +0200 >> > Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: >> > >> >> 2017-03-24 15:16 GMT+01:00 Ronald S. Bultje <rsbultje@gmail.com>: >> >> >> In addition, giving the fact that you seem (?) to have stopped >> >> contributing may lead an innocent reader to the conclusion >> >> that these comments aren't meant as reviews. >> > >> > You must be joking. >> >> Sure? > > So you're not? I definitely wasn't joking. Maybe you had access to private information that I had not? Carl Eugen
On Wed, 29 Mar 2017 09:54:27 +0200 Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > 2017-03-27 13:41 GMT+02:00 wm4 <nfxjfg@googlemail.com>: > > On Mon, 27 Mar 2017 13:29:35 +0200 > > Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > > > >> 2017-03-27 11:40 GMT+02:00 wm4 <nfxjfg@googlemail.com>: > >> > On Mon, 27 Mar 2017 09:31:39 +0200 > >> > Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > >> > > >> >> 2017-03-24 15:16 GMT+01:00 Ronald S. Bultje <rsbultje@gmail.com>: > >> > >> >> In addition, giving the fact that you seem (?) to have stopped > >> >> contributing may lead an innocent reader to the conclusion > >> >> that these comments aren't meant as reviews. > >> > > >> > You must be joking. > >> > >> Sure? > > > > So you're not? > > I definitely wasn't joking. > Maybe you had access to private information that I had not? So what else is the reason that you claim he isn't an active contributor, even though he obviously is, and even worse, suggest that his reviews aren't reviews and don't matter? This isn't very nice of you.
diff --git a/libavcodec/h264_ps.c b/libavcodec/h264_ps.c index b78ad25..55e6a6e 100644 --- a/libavcodec/h264_ps.c +++ b/libavcodec/h264_ps.c @@ -181,6 +181,8 @@ static inline int decode_vui_parameters(GetBitContext *gb, AVCodecContext *avctx if (get_bits1(gb)) { /* chroma_sample_location_type_top_field */ avctx->chroma_sample_location = get_ue_golomb(gb) + 1; + if (avctx->chroma_sample_location >= AVCHROMA_LOC_NB) + avctx->chroma_sample_location = AVCHROMA_LOC_UNSPECIFIED; get_ue_golomb(gb); /* chroma_sample_location_type_bottom_field */ }
Hi! Attached patch fixes #6255. Please comment, Carl Eugen From 1c249440c62271565be12112f321ad9aa6a922fb Mon Sep 17 00:00:00 2001 From: Carl Eugen Hoyos <cehoyos@ag.or.at> Date: Fri, 24 Mar 2017 10:38:22 +0100 Subject: [PATCH] lavc/h264_ps: Check that chroma_location is within limits. Fixes ticket #6255. --- libavcodec/h264_ps.c | 2 ++ 1 file changed, 2 insertions(+)