On Thu, Jan 17, 2019 at 10:57 PM Neil Birkbeck <neil.birkb...@gmail.com> wrote:
> On Thu, Jan 17, 2019 at 7:43 PM Neil Birkbeck <neil.birkb...@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.birkb...@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 _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel