Re: [FFmpeg-devel] [PATCH] [dnxhddec] Do not overwrite colorspace if the container has set it.

[Michael Niedermayer]

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  > wrote:
> 
> > On Mon, Nov 27, 2017 at 12:36:48AM -0800, Steven Robertson wrote:
> > > Signed-off-by: Steven Robertson 
> > > ---
> > >  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

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

"You are 36 times more likely to die in a bathtub than at the hands of a
terrorist. Also, you are 2.5 times more likely to become a president and
2 times more likely to become an astronaut, than to die in a terrorist
attack." -- Thoughty2



signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] [dnxhddec] Do not overwrite colorspace if the container has set it.

[Steven Robertson]

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  wrote:

> On Mon, Nov 27, 2017 at 12:36:48AM -0800, Steven Robertson wrote:
> > Signed-off-by: Steven Robertson 
> > ---
> >  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

Re: [FFmpeg-devel] [PATCH] [dnxhddec] Do not overwrite colorspace if the container has set it.

[Michael Niedermayer]

On Mon, Nov 27, 2017 at 12:36:48AM -0800, Steven Robertson wrote:
> Signed-off-by: Steven Robertson 
> ---
>  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


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH] [dnxhddec] Do not overwrite colorspace if the container has set it.

[Steven Robertson]

Signed-off-by: Steven Robertson 
---
 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;
+}
 
 avctx->coded_width  = FFALIGN(avctx->width,  16);
 avctx->coded_height = FFALIGN(avctx->height, 16);
-- 
2.15.0.417.g466bffb3ac-goog

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel