Re: [FFmpeg-devel] [PATCH] avcodec/proresdec2: only set avctx->color* when values are specified
On Fri, Jan 18, 2019 at 7:48 AM Vittorio Giovara wrote: > On Fri, Jan 18, 2019 at 6:43 AM Carl Eugen Hoyos > wrote: > > > 2019-01-18 4:48 GMT+01:00, Neil Birkbeck : > > > On Thu, Jan 17, 2019 at 7:43 PM Neil Birkbeck > > > > 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 > > >> --- > > >> 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). > > > > Isn't this even more of a reason to prefer the container value over > > the bitstream value? > > > > Absolutely not > I was initially going to check and see if preferring the container (when set) was a valid alternative, since that was what the old behavior was. But I couldn't find precedent for that elsewhere in the code (except when the codec was just assuming some fixed constant not specified in the bitstream). If a library user wants to prefer/detect container metadata, I take it they have to avformat_open_input, inspect metadata before find_stream_info, and use their application-specific logic to do add any bitstream filters or adjust parameters of the filter graph? For posterity, there are a couple more slightly relevant links I found when looking for an authoritative reference on such inconsistencies in prores. I only found [1] that confirms the tooling/workflows were inconsistent at some point, and [2] which suggested that the colr atom took precedence over headers on whatever viewer they were talking about (presumably quicktime, on mac). [1] https://github.com/bbc/qtff-parameter-editor [2] https://www.drastic.tv/support-59/supporttipstechnical/202-prores-colour-shifts-in-post-production From ab77fa3035bace9dfff3b9f83e623e823cac80cf Mon Sep 17 00:00:00 2001 From: Neil Birkbeck Date: Tue, 15 Jan 2019 17:19:44 -0800 Subject: [PATCH] avcodec/proresdec2: only set avctx->color* when values are specified 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). 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). A bit more info on that topic related to HDR workflows here: https://github.com/bbc/qtff-parameter-editor 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). Signed-off-by: Neil Birkbeck --- 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,
Re: [FFmpeg-devel] [PATCH] avcodec/proresdec2: only set avctx->color* when values are specified
On Fri, Jan 18, 2019 at 6:43 AM Carl Eugen Hoyos wrote: > 2019-01-18 4:48 GMT+01:00, Neil Birkbeck : > > On Thu, Jan 17, 2019 at 7:43 PM Neil Birkbeck > > 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 > >> --- > >> 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). > > Isn't this even more of a reason to prefer the container value over > the bitstream value? > Absolutely not -- Vittorio ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/proresdec2: only set avctx->color* when values are specified
On Thu, Jan 17, 2019 at 10:57 PM Neil Birkbeck wrote: > On Thu, Jan 17, 2019 at 7:43 PM Neil Birkbeck > 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 > > --- > > 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
Re: [FFmpeg-devel] [PATCH] avcodec/proresdec2: only set avctx->color* when values are specified
2019-01-18 4:48 GMT+01:00, Neil Birkbeck : > On Thu, Jan 17, 2019 at 7:43 PM Neil Birkbeck > 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 >> --- >> 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). Isn't this even more of a reason to prefer the container value over the bitstream value? Carl Eugen ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/proresdec2: only set avctx->color* when values are specified
On Thu, Jan 17, 2019 at 7:43 PM Neil Birkbeck 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 > --- > 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 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] avcodec/proresdec2: only set avctx->color* when values are specified
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 --- 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 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel