[FFmpeg-devel] [PATCH] avcodec/proresdec2: only set avctx->color* when values are specified
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>
> On Thu, Jan 17, 2019 at 7:43 PM Neil Birkbeck <neil.birkbeck at gmail.com>
> > 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;
> > - avctx->color_trc = buf;
> > - avctx->colorspace = buf;
> > + if (buf != AVCOL_PRI_UNSPECIFIED)
> > + avctx->color_primaries = buf;
> > + if (buf != AVCOL_TRC_UNSPECIFIED)
> > + avctx->color_trc = buf;
> > + if (buf != AVCOL_SPC_UNSPECIFIED)
> > + avctx->colorspace = buf;
> > avctx->color_range = AVCOL_RANGE_MPEG;
> > ptr = buf + 20;
> > --
> > 126.96.36.1991.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
> 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
> 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.
More information about the ffmpeg-devel