| Message ID | 20171127083648.10588-1-steven@strobe.cc |
|---|---|
| State | New |
| Headers | show |
On Mon, Nov 27, 2017 at 12:36:48AM -0800, Steven Robertson wrote: > Signed-off-by: Steven Robertson <steven@strobe.cc> > --- > libavcodec/dnxhddec.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/libavcodec/dnxhddec.c b/libavcodec/dnxhddec.c > index f46e41a456..6f8c716412 100644 > --- a/libavcodec/dnxhddec.c > +++ b/libavcodec/dnxhddec.c > @@ -93,7 +93,9 @@ static av_cold int dnxhd_decode_init(AVCodecContext *avctx) > > ctx->avctx = avctx; > ctx->cid = -1; > - avctx->colorspace = AVCOL_SPC_BT709; > + if (avctx->colorspace == AVCOL_SPC_UNSPECIFIED) { > + avctx->colorspace = AVCOL_SPC_BT709; > + } why is this sometimes but not always correct ? can one and the same dnxhd stream be 2 different colorspace depending on container or is the decoder implementation incomplete in relation to setting the colorspace ? [...]
It's... less wrong. The VC-3 DNxHR update added a 'clv' field to an existing byte in the frame header to allow for the carriage of a limited set of color info (0 = 709/709, 1=2020/2020NCL, 2=2020/2020CL, 3=container specifies), but because that field is new-ish and assigns a meaning other than 'unspecified' to 0, we see files that have correct colorimetry in a 'colr' atom or equivalent (e.g. 2020/2020NCL with PQ transfer curve) but which do not set the 'clv' value, so it defaults to 0. For these media, the most correct choice is to trust the container over the file. Because DNxHD/DNxHR is getting more popular for delivering HDR, HDR requires setting a transfer function, and 'clv' doesn't have a way to specify the transfer function, DNxHR for HDR always needs to rely on the container for metadata, and this at least gets the issue out of the way for those cases without changing existing behavior for containers that don't specify a color system. Additionally, because it's set in the frame header, we'd have to probe a frame and guard against frame changes or even reconfigure things after a frame change, and since I'm not an experienced ffmpeg dev I opted for the minimal fix to make 2020NCL for HDR files work properly. On Mon, Nov 27, 2017 at 4:35 PM, Michael Niedermayer <michael@niedermayer.cc > wrote: > On Mon, Nov 27, 2017 at 12:36:48AM -0800, Steven Robertson wrote: > > Signed-off-by: Steven Robertson <steven@strobe.cc> > > --- > > libavcodec/dnxhddec.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/libavcodec/dnxhddec.c b/libavcodec/dnxhddec.c > > index f46e41a456..6f8c716412 100644 > > --- a/libavcodec/dnxhddec.c > > +++ b/libavcodec/dnxhddec.c > > @@ -93,7 +93,9 @@ static av_cold int dnxhd_decode_init(AVCodecContext > *avctx) > > > > ctx->avctx = avctx; > > ctx->cid = -1; > > - avctx->colorspace = AVCOL_SPC_BT709; > > + if (avctx->colorspace == AVCOL_SPC_UNSPECIFIED) { > > + avctx->colorspace = AVCOL_SPC_BT709; > > + } > > why is this sometimes but not always correct ? > > can one and the same dnxhd stream be 2 different colorspace depending > on container or is the decoder implementation incomplete in relation > to setting the colorspace ? > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > When the tyrant has disposed of foreign enemies by conquest or treaty, and > there is nothing more to fear from them, then he is always stirring up > some war or other, in order that the people may require a leader. -- Plato > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > >
On Mon, Nov 27, 2017 at 06:43:32PM -0800, Steven Robertson wrote: > It's... less wrong. > > The VC-3 DNxHR update added a 'clv' field to an existing byte in the frame > header to allow for the carriage of a limited set of color info (0 = > 709/709, 1=2020/2020NCL, 2=2020/2020CL, 3=container specifies), but because > that field is new-ish and assigns a meaning other than 'unspecified' to 0, > we see files that have correct colorimetry in a 'colr' atom or equivalent > (e.g. 2020/2020NCL with PQ transfer curve) but which do not set the 'clv' > value, so it defaults to 0. For these media, the most correct choice is to > trust the container over the file. Because DNxHD/DNxHR is getting more > popular for delivering HDR, HDR requires setting a transfer function, and > 'clv' doesn't have a way to specify the transfer function, DNxHR for HDR > always needs to rely on the container for metadata, and this at least gets > the issue out of the way for those cases without changing existing behavior > for containers that don't specify a color system. > > Additionally, because it's set in the frame header, we'd have to probe a > frame and guard against frame changes or even reconfigure things after a > frame change, and since I'm not an experienced ffmpeg dev I opted for the > minimal fix to make 2020NCL for HDR files work properly. Something like this should be in the commit message thx > > > On Mon, Nov 27, 2017 at 4:35 PM, Michael Niedermayer <michael@niedermayer.cc > > wrote: > > > On Mon, Nov 27, 2017 at 12:36:48AM -0800, Steven Robertson wrote: > > > Signed-off-by: Steven Robertson <steven@strobe.cc> > > > --- > > > libavcodec/dnxhddec.c | 4 +++- > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > diff --git a/libavcodec/dnxhddec.c b/libavcodec/dnxhddec.c > > > index f46e41a456..6f8c716412 100644 > > > --- a/libavcodec/dnxhddec.c > > > +++ b/libavcodec/dnxhddec.c > > > @@ -93,7 +93,9 @@ static av_cold int dnxhd_decode_init(AVCodecContext > > *avctx) > > > > > > ctx->avctx = avctx; > > > ctx->cid = -1; > > > - avctx->colorspace = AVCOL_SPC_BT709; > > > + if (avctx->colorspace == AVCOL_SPC_UNSPECIFIED) { > > > + avctx->colorspace = AVCOL_SPC_BT709; > > > + } > > > > why is this sometimes but not always correct ? > > > > can one and the same dnxhd stream be 2 different colorspace depending > > on container or is the decoder implementation incomplete in relation > > to setting the colorspace ? > > > > [...] > > -- > > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > > > When the tyrant has disposed of foreign enemies by conquest or treaty, and > > there is nothing more to fear from them, then he is always stirring up > > some war or other, in order that the people may require a leader. -- Plato > > > > _______________________________________________ > > ffmpeg-devel mailing list > > ffmpeg-devel@ffmpeg.org > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > > > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
diff --git a/libavcodec/dnxhddec.c b/libavcodec/dnxhddec.c index f46e41a456..6f8c716412 100644 --- a/libavcodec/dnxhddec.c +++ b/libavcodec/dnxhddec.c @@ -93,7 +93,9 @@ static av_cold int dnxhd_decode_init(AVCodecContext *avctx) ctx->avctx = avctx; ctx->cid = -1; - avctx->colorspace = AVCOL_SPC_BT709; + if (avctx->colorspace == AVCOL_SPC_UNSPECIFIED) { + avctx->colorspace = AVCOL_SPC_BT709; + } avctx->coded_width = FFALIGN(avctx->width, 16); avctx->coded_height = FFALIGN(avctx->height, 16);
Signed-off-by: Steven Robertson <steven@strobe.cc> --- libavcodec/dnxhddec.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)