[FFmpeg-devel] lavc/h264_ps: Check chroma_location limits

Submitted by Carl Eugen Hoyos on March 24, 2017, 9:40 a.m.

Details

Message ID 201703241040.21244.cehoyos@ag.or.at
State New
Headers show

Commit Message

Carl Eugen Hoyos March 24, 2017, 9:40 a.m.
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(+)

Comments

wm4 March 24, 2017, 9:59 a.m.
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 */
>      }
>
Carl Eugen Hoyos March 24, 2017, 10:23 a.m.
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
wm4 March 24, 2017, 10:57 a.m.
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.
Ronald S. Bultje March 24, 2017, 11:40 a.m.
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
Ronald S. Bultje March 24, 2017, 11:48 a.m.
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
wm4 March 24, 2017, 11:53 a.m.
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
Clément Bœsch March 24, 2017, 2:05 p.m.
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(...)) ...
Ronald S. Bultje March 24, 2017, 2:16 p.m.
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
Michael Niedermayer March 25, 2017, 1:15 a.m.
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


[...]
Ronald S. Bultje March 25, 2017, 1:35 a.m.
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
Carl Eugen Hoyos March 27, 2017, 7:31 a.m.
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
wm4 March 27, 2017, 9:40 a.m.
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?
Paul B Mahol March 27, 2017, 9:43 a.m.
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.
Carl Eugen Hoyos March 27, 2017, 11:29 a.m.
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
wm4 March 27, 2017, 11:41 a.m.
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?
Carl Eugen Hoyos March 29, 2017, 7:54 a.m.
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
wm4 March 29, 2017, 9:42 a.m.
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.

Patch hide | download patch | download mbox

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 */
     }