[FFmpeg-devel] [PATCH] avcodec/proresdec2: only set avctx->color* when values are specified

Vittorio Giovara vittorio.giovara at gmail.com
Fri Jan 18 17:42:08 EET 2019


On Thu, Jan 17, 2019 at 10:57 PM Neil Birkbeck <neil.birkbeck at gmail.com>
wrote:

> On Thu, Jan 17, 2019 at 7:43 PM Neil Birkbeck <neil.birkbeck at gmail.com>
> wrote:
>
> > This allows preservation of color values set from the container,
> > while still letting the bitstream take precedent when its values
> > are specified to some actual value (e.g., not *UNSPECIFIED).
> >
> > Signed-off-by: Neil Birkbeck <neil.birkbeck at gmail.com>
> > ---
> >  libavcodec/proresdec2.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/libavcodec/proresdec2.c b/libavcodec/proresdec2.c
> > index 6209c229c9..662ca7d48b 100644
> > --- a/libavcodec/proresdec2.c
> > +++ b/libavcodec/proresdec2.c
> > @@ -262,9 +262,12 @@ static int decode_frame_header(ProresContext *ctx,
> > const uint8_t *buf,
> >          }
> >      }
> >
> > -    avctx->color_primaries = buf[14];
> > -    avctx->color_trc       = buf[15];
> > -    avctx->colorspace      = buf[16];
> > +    if (buf[14] != AVCOL_PRI_UNSPECIFIED)
> > +        avctx->color_primaries = buf[14];
> > +    if (buf[15] != AVCOL_TRC_UNSPECIFIED)
> > +        avctx->color_trc       = buf[15];
> > +    if (buf[16] != AVCOL_SPC_UNSPECIFIED)
> > +        avctx->colorspace      = buf[16];
> >      avctx->color_range     = AVCOL_RANGE_MPEG;
> >
> >      ptr   = buf + 20;
> > --
> > 2.20.1.321.g9e740568ce-goog
> >
> >
> To add a bit more context. The patch is a fix for the case when prores
> bitstream code points are explicitly set to unspecified and are overriding
> what may have been in the container (unlike h264/h265 where such values can
> behind the color_description flag, these fields always must be present in
> the prores header). Of course, ideally these should be the same.
>
> We noticed this inconsistency on some HDR content, as prior to
>
> https://github.com/FFmpeg/FFmpeg/commit/81d78fe13bc1fd94845c002f3439fe44d9e9e0d9
> the color information in the prores bitstream (which may have been
> unspecified) was simply ignored.
>
> In practice, I guess some workflows may not have known about the new code
> points and put unspecified in the bitstream (or worse where some headers
> will contain non-HDR code points).
>
> The patch seemed like a situation where we could resolve the inconsistency
> in favor of the container (given my understanding, and from what I see in
> the code, I'm assuming the codec is typically given preference). But I'm
> interested in hearing ffmpeg-devel's opinions on whether this is most
> consistent (actually, for the HDR files that I've seen, the container is
> correct--although I'm sure there are cases where the opposite is true).
>
> I guess the most closely related discussion about codecs overriding these
> types of values is here
> https://patchwork.ffmpeg.org/patch/11279/
> but this case seemed a bit different.
>

This makes a lot of sense, but it should be part of the commit message
instead of the attached thread.
So ok with me.
-- 
Vittorio


More information about the ffmpeg-devel mailing list