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

Neil Birkbeck neil.birkbeck at gmail.com
Fri Jan 18 05:48:42 EET 2019


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.

Thanks


More information about the ffmpeg-devel mailing list