diff mbox

[FFmpeg-devel,2/2] ffmpeg: Use the colour properties from the input stream when doing transcode

Message ID 20180516071945.28983-2-haihao.xiang@intel.com
State New
Headers show

Commit Message

Xiang, Haihao May 16, 2018, 7:19 a.m. UTC
In lavc/hevec_vaapi, colour properties in AVCodecContext are needed to
write the sequence header

Tested by the command below:
ffmpeg -hwaccel vaapi -hwaccel_device /dev/dri/renderD128
-hwaccel_output_format vaapi -i input-with-hdr.mkv -c:v hevc_vaapi
-profile:v main10 output.h265

Signed-off-by: Haihao Xiang <haihao.xiang@intel.com>
---
 fftools/ffmpeg.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Mark Thompson May 21, 2018, 10:07 p.m. UTC | #1
On 16/05/18 08:19, Haihao Xiang wrote:
> In lavc/hevec_vaapi, colour properties in AVCodecContext are needed to
> write the sequence header
> 
> Tested by the command below:
> ffmpeg -hwaccel vaapi -hwaccel_device /dev/dri/renderD128
> -hwaccel_output_format vaapi -i input-with-hdr.mkv -c:v hevc_vaapi
> -profile:v main10 output.h265
> 
> Signed-off-by: Haihao Xiang <haihao.xiang@intel.com>
> ---
>  fftools/ffmpeg.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> index 5a19a09d9a..80b5441f8f 100644
> --- a/fftools/ffmpeg.c
> +++ b/fftools/ffmpeg.c
> @@ -3298,6 +3298,10 @@ static int init_output_stream_encode(OutputStream *ost)
>          dec_ctx = ist->dec_ctx;
>  
>          enc_ctx->chroma_sample_location = dec_ctx->chroma_sample_location;
> +        enc_ctx->color_range            = dec_ctx->color_range;
> +        enc_ctx->color_primaries        = dec_ctx->color_primaries;
> +        enc_ctx->color_trc              = dec_ctx->color_trc;
> +        enc_ctx->colorspace             = dec_ctx->colorspace;
>      } else {
>          for (j = 0; j < oc->nb_streams; j++) {
>              AVStream *st = oc->streams[j];
> 

There are outstanding patches passing color_range through the filter chain (see <https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2018-May/229248.html>), and that would be the right solution for the rest of these properties as well, but it would require significantly more work.

I think hacking it like this is ok for now?  It's not worse than before since a change during filtering wasn't visible anyway, and the manual options do still work to override it.  I think the commit message could be a little clearer about the problem (really, that this colour information doesn't pass through filtering) and its limitations (requires a single input matched to the output; will write the wrong thing if filtering changes anything) though, or maybe that could be explained in a comment.

Does anyone else have any thoughts on this?  If there are no objections then I would apply it updated with a clearer explanation.

Thanks,

- Mark
Michael Niedermayer May 23, 2018, 1:27 a.m. UTC | #2
On Mon, May 21, 2018 at 11:07:34PM +0100, Mark Thompson wrote:
> On 16/05/18 08:19, Haihao Xiang wrote:
> > In lavc/hevec_vaapi, colour properties in AVCodecContext are needed to
> > write the sequence header
> > 
> > Tested by the command below:
> > ffmpeg -hwaccel vaapi -hwaccel_device /dev/dri/renderD128
> > -hwaccel_output_format vaapi -i input-with-hdr.mkv -c:v hevc_vaapi
> > -profile:v main10 output.h265
> > 
> > Signed-off-by: Haihao Xiang <haihao.xiang@intel.com>
> > ---
> >  fftools/ffmpeg.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> > index 5a19a09d9a..80b5441f8f 100644
> > --- a/fftools/ffmpeg.c
> > +++ b/fftools/ffmpeg.c
> > @@ -3298,6 +3298,10 @@ static int init_output_stream_encode(OutputStream *ost)
> >          dec_ctx = ist->dec_ctx;
> >  
> >          enc_ctx->chroma_sample_location = dec_ctx->chroma_sample_location;
> > +        enc_ctx->color_range            = dec_ctx->color_range;
> > +        enc_ctx->color_primaries        = dec_ctx->color_primaries;
> > +        enc_ctx->color_trc              = dec_ctx->color_trc;
> > +        enc_ctx->colorspace             = dec_ctx->colorspace;
> >      } else {
> >          for (j = 0; j < oc->nb_streams; j++) {
> >              AVStream *st = oc->streams[j];
> > 
> 
> There are outstanding patches passing color_range through the filter chain (see <https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2018-May/229248.html>), and that would be the right solution for the rest of these properties as well, but it would require significantly more work.
> 
> I think hacking it like this is ok for now?  It's not worse than before since a change during filtering wasn't visible anyway, and the manual options do still work to override it.  I think the commit message could be a little clearer about the problem (really, that this colour information doesn't pass through filtering) and its limitations (requires a single input matched to the output; will write the wrong thing if filtering changes anything) though, or maybe that could be explained in a comment.
> 
> Does anyone else have any thoughts on this?  If there are no objections then I would apply it updated with a clearer explanation.

I think the key question is
Does this change "Unknown" to incorrect values
if so it is IMHO not a good idea as we would create broken files where we did
previously just create files that lacked some metadata


[...]
Xiang, Haihao May 24, 2018, 6:29 a.m. UTC | #3
On Wed, 2018-05-23 at 03:27 +0200, Michael Niedermayer wrote:
> On Mon, May 21, 2018 at 11:07:34PM +0100, Mark Thompson wrote:

> > On 16/05/18 08:19, Haihao Xiang wrote:

> > > In lavc/hevec_vaapi, colour properties in AVCodecContext are needed to

> > > write the sequence header

> > > 

> > > Tested by the command below:

> > > ffmpeg -hwaccel vaapi -hwaccel_device /dev/dri/renderD128

> > > -hwaccel_output_format vaapi -i input-with-hdr.mkv -c:v hevc_vaapi

> > > -profile:v main10 output.h265

> > > 

> > > Signed-off-by: Haihao Xiang <haihao.xiang@intel.com>

> > > ---

> > >  fftools/ffmpeg.c | 4 ++++

> > >  1 file changed, 4 insertions(+)

> > > 

> > > diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c

> > > index 5a19a09d9a..80b5441f8f 100644

> > > --- a/fftools/ffmpeg.c

> > > +++ b/fftools/ffmpeg.c

> > > @@ -3298,6 +3298,10 @@ static int init_output_stream_encode(OutputStream

> > > *ost)

> > >          dec_ctx = ist->dec_ctx;

> > >  

> > >          enc_ctx->chroma_sample_location = dec_ctx-

> > > >chroma_sample_location;

> > > +        enc_ctx->color_range            = dec_ctx->color_range;

> > > +        enc_ctx->color_primaries        = dec_ctx->color_primaries;

> > > +        enc_ctx->color_trc              = dec_ctx->color_trc;

> > > +        enc_ctx->colorspace             = dec_ctx->colorspace;

> > >      } else {

> > >          for (j = 0; j < oc->nb_streams; j++) {

> > >              AVStream *st = oc->streams[j];

> > > 

> > 

> > There are outstanding patches passing color_range through the filter chain

> > (see <https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2018-May/229248.html>)

> > , and that would be the right solution for the rest of these properties as

> > well, but it would require significantly more work.

> > 

> > I think hacking it like this is ok for now?  It's not worse than before

> > since a change during filtering wasn't visible anyway, and the manual

> > options do still work to override it.  I think the commit message could be a

> > little clearer about the problem (really, that this colour information

> > doesn't pass through filtering) and its limitations (requires a single input

> > matched to the output; will write the wrong thing if filtering changes

> > anything) though, or maybe that could be explained in a comment.

> > 

> > Does anyone else have any thoughts on this?  If there are no objections then

> > I would apply it updated with a clearer explanation.

> 

> I think the key question is

> Does this change "Unknown" to incorrect values


Do you mean the color in dec_ctx may be incorrect? If so, we should fix the
decoder. 

BTW I didn't see error when ran fate test on my machine.
 

> if so it is IMHO not a good idea as we would create broken files where we did

> previously just create files that lacked some metadata

> 

> 

> [...]

> _______________________________________________

> ffmpeg-devel mailing list

> ffmpeg-devel@ffmpeg.org

> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Tobias Rapp May 24, 2018, 7:16 a.m. UTC | #4
On 22.05.2018 00:07, Mark Thompson wrote:
> On 16/05/18 08:19, Haihao Xiang wrote:
>> In lavc/hevec_vaapi, colour properties in AVCodecContext are needed to
>> write the sequence header
>>
>> Tested by the command below:
>> ffmpeg -hwaccel vaapi -hwaccel_device /dev/dri/renderD128
>> -hwaccel_output_format vaapi -i input-with-hdr.mkv -c:v hevc_vaapi
>> -profile:v main10 output.h265
>>
>> Signed-off-by: Haihao Xiang <haihao.xiang@intel.com>
>> ---
>>   fftools/ffmpeg.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
>> index 5a19a09d9a..80b5441f8f 100644
>> --- a/fftools/ffmpeg.c
>> +++ b/fftools/ffmpeg.c
>> @@ -3298,6 +3298,10 @@ static int init_output_stream_encode(OutputStream *ost)
>>           dec_ctx = ist->dec_ctx;
>>   
>>           enc_ctx->chroma_sample_location = dec_ctx->chroma_sample_location;
>> +        enc_ctx->color_range            = dec_ctx->color_range;
>> +        enc_ctx->color_primaries        = dec_ctx->color_primaries;
>> +        enc_ctx->color_trc              = dec_ctx->color_trc;
>> +        enc_ctx->colorspace             = dec_ctx->colorspace;
>>       } else {
>>           for (j = 0; j < oc->nb_streams; j++) {
>>               AVStream *st = oc->streams[j];
>>
> 
> There are outstanding patches passing color_range through the filter chain (see <https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2018-May/229248.html>), and that would be the right solution for the rest of these properties as well, but it would require significantly more work.
> 
> I think hacking it like this is ok for now?  It's not worse than before since a change during filtering wasn't visible anyway, and the manual options do still work to override it.  I think the commit message could be a little clearer about the problem (really, that this colour information doesn't pass through filtering) and its limitations (requires a single input matched to the output; will write the wrong thing if filtering changes anything) though, or maybe that could be explained in a comment.
> 
> Does anyone else have any thoughts on this?  If there are no objections then I would apply it updated with a clearer explanation.

Recently I checked why interlacing information is not forwarded 
correctly from the decoded+filtered frame to the encoder and noticed 
that the encoder is just updated too late to be picked up by the muxer 
when writing header information. I think something similar is happening 
here.

Initializing the encoder here will fix the situation in case the filter 
graph between decoder and encoder doesn't change one of the settings, 
else it will write wrong information -- which is worse than the status 
quo. In my opinion the best solution would be to change ffmpeg.c to 
write a header after it has fetched a frame from each output filter 
graph, so it can use the actual output frame properties. Until then a 
work-around is to initialize the encoder manually on the command-line, 
in case you are able to determine the output values in advance (e.g. by 
using ffprobe).

Regards,
Tobias
Mark Thompson May 24, 2018, 10:15 a.m. UTC | #5
On 24/05/18 07:29, Xiang, Haihao wrote:
> On Wed, 2018-05-23 at 03:27 +0200, Michael Niedermayer wrote:
>> On Mon, May 21, 2018 at 11:07:34PM +0100, Mark Thompson wrote:
>>> On 16/05/18 08:19, Haihao Xiang wrote:
>>>> In lavc/hevec_vaapi, colour properties in AVCodecContext are needed to
>>>> write the sequence header
>>>>
>>>> Tested by the command below:
>>>> ffmpeg -hwaccel vaapi -hwaccel_device /dev/dri/renderD128
>>>> -hwaccel_output_format vaapi -i input-with-hdr.mkv -c:v hevc_vaapi
>>>> -profile:v main10 output.h265
>>>>
>>>> Signed-off-by: Haihao Xiang <haihao.xiang@intel.com>
>>>> ---
>>>>  fftools/ffmpeg.c | 4 ++++
>>>>  1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
>>>> index 5a19a09d9a..80b5441f8f 100644
>>>> --- a/fftools/ffmpeg.c
>>>> +++ b/fftools/ffmpeg.c
>>>> @@ -3298,6 +3298,10 @@ static int init_output_stream_encode(OutputStream
>>>> *ost)
>>>>          dec_ctx = ist->dec_ctx;
>>>>  
>>>>          enc_ctx->chroma_sample_location = dec_ctx-
>>>>> chroma_sample_location;
>>>> +        enc_ctx->color_range            = dec_ctx->color_range;
>>>> +        enc_ctx->color_primaries        = dec_ctx->color_primaries;
>>>> +        enc_ctx->color_trc              = dec_ctx->color_trc;
>>>> +        enc_ctx->colorspace             = dec_ctx->colorspace;
>>>>      } else {
>>>>          for (j = 0; j < oc->nb_streams; j++) {
>>>>              AVStream *st = oc->streams[j];
>>>>
>>>
>>> There are outstanding patches passing color_range through the filter chain
>>> (see <https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2018-May/229248.html>)
>>> , and that would be the right solution for the rest of these properties as
>>> well, but it would require significantly more work.
>>>
>>> I think hacking it like this is ok for now?  It's not worse than before
>>> since a change during filtering wasn't visible anyway, and the manual
>>> options do still work to override it.  I think the commit message could be a
>>> little clearer about the problem (really, that this colour information
>>> doesn't pass through filtering) and its limitations (requires a single input
>>> matched to the output; will write the wrong thing if filtering changes
>>> anything) though, or maybe that could be explained in a comment.
>>>
>>> Does anyone else have any thoughts on this?  If there are no objections then
>>> I would apply it updated with a clearer explanation.
>>
>> I think the key question is
>> Does this change "Unknown" to incorrect values
> 
> Do you mean the color in dec_ctx may be incorrect? If so, we should fix the
> decoder. 

No, I mean where the input properties in the decode context are correct but don't match the output because it gets changed during filtering.

For example:

ffmpeg -i bt709_input.mkv -vf colorspace=bt2020 bt2020_output.mkv

will have the output file marked as BT.709 after this patch, where previously it was "unspecified".  (Explicitly setting -color_primaries/-color_trc/-colorspace on the output works in both cases.)

- Mark
Xiang, Haihao May 25, 2018, 5:58 a.m. UTC | #6
On Thu, 2018-05-24 at 11:15 +0100, Mark Thompson wrote:
> On 24/05/18 07:29, Xiang, Haihao wrote:

> > On Wed, 2018-05-23 at 03:27 +0200, Michael Niedermayer wrote:

> > > On Mon, May 21, 2018 at 11:07:34PM +0100, Mark Thompson wrote:

> > > > On 16/05/18 08:19, Haihao Xiang wrote:

> > > > > In lavc/hevec_vaapi, colour properties in AVCodecContext are needed to

> > > > > write the sequence header

> > > > > 

> > > > > Tested by the command below:

> > > > > ffmpeg -hwaccel vaapi -hwaccel_device /dev/dri/renderD128

> > > > > -hwaccel_output_format vaapi -i input-with-hdr.mkv -c:v hevc_vaapi

> > > > > -profile:v main10 output.h265

> > > > > 

> > > > > Signed-off-by: Haihao Xiang <haihao.xiang@intel.com>

> > > > > ---

> > > > >  fftools/ffmpeg.c | 4 ++++

> > > > >  1 file changed, 4 insertions(+)

> > > > > 

> > > > > diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c

> > > > > index 5a19a09d9a..80b5441f8f 100644

> > > > > --- a/fftools/ffmpeg.c

> > > > > +++ b/fftools/ffmpeg.c

> > > > > @@ -3298,6 +3298,10 @@ static int

> > > > > init_output_stream_encode(OutputStream

> > > > > *ost)

> > > > >          dec_ctx = ist->dec_ctx;

> > > > >  

> > > > >          enc_ctx->chroma_sample_location = dec_ctx-

> > > > > > chroma_sample_location;

> > > > > 

> > > > > +        enc_ctx->color_range            = dec_ctx->color_range;

> > > > > +        enc_ctx->color_primaries        = dec_ctx->color_primaries;

> > > > > +        enc_ctx->color_trc              = dec_ctx->color_trc;

> > > > > +        enc_ctx->colorspace             = dec_ctx->colorspace;

> > > > >      } else {

> > > > >          for (j = 0; j < oc->nb_streams; j++) {

> > > > >              AVStream *st = oc->streams[j];

> > > > > 

> > > > 

> > > > There are outstanding patches passing color_range through the filter

> > > > chain

> > > > (see <https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2018-May/229248.ht

> > > > ml>)

> > > > , and that would be the right solution for the rest of these properties

> > > > as

> > > > well, but it would require significantly more work.

> > > > 

> > > > I think hacking it like this is ok for now?  It's not worse than before

> > > > since a change during filtering wasn't visible anyway, and the manual

> > > > options do still work to override it.  I think the commit message could

> > > > be a

> > > > little clearer about the problem (really, that this colour information

> > > > doesn't pass through filtering) and its limitations (requires a single

> > > > input

> > > > matched to the output; will write the wrong thing if filtering changes

> > > > anything) though, or maybe that could be explained in a comment.

> > > > 

> > > > Does anyone else have any thoughts on this?  If there are no objections

> > > > then

> > > > I would apply it updated with a clearer explanation.

> > > 

> > > I think the key question is

> > > Does this change "Unknown" to incorrect values

> > 

> > Do you mean the color in dec_ctx may be incorrect? If so, we should fix the

> > decoder. 

> 

> No, I mean where the input properties in the decode context are correct but

> don't match the output because it gets changed during filtering.


Got it, thanks

> 

> For example:

> 

> ffmpeg -i bt709_input.mkv -vf colorspace=bt2020 bt2020_output.mkv

> 

> will have the output file marked as BT.709 after this patch, where previously

> it was "unspecified".  (Explicitly setting -color_primaries/-color_trc/-

> colorspace on the output works in both cases.)


I agree with you it's not worse than before as we don't get the expected result
in both cases. 


> 

> - Mark

> _______________________________________________

> ffmpeg-devel mailing list

> ffmpeg-devel@ffmpeg.org

> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Tobias Rapp May 25, 2018, 6:57 a.m. UTC | #7
On 25.05.2018 07:58, Xiang, Haihao wrote:
> On Thu, 2018-05-24 at 11:15 +0100, Mark Thompson wrote:
>>
>> For example:
>>
>> ffmpeg -i bt709_input.mkv -vf colorspace=bt2020 bt2020_output.mkv
>>
>> will have the output file marked as BT.709 after this patch, where previously
>> it was "unspecified".  (Explicitly setting -color_primaries/-color_trc/-
>> colorspace on the output works in both cases.)
> 
> I agree with you it's not worse than before as we don't get the expected result
> in both cases.

Not quite: When a file says "I don't know this property value" you have 
a chance to lookup the value somewhere else or use a default. When it 
says "I know the value" and gives a wrong value, you completely loose trust.

So in my opinion this patch should not be applied, as it possibly makes 
ffmpeg generate files with wrong information where it just had no 
information before. The correct approach would be to set the encoder 
properties from output frame data and only write a header once the 
encoders have been initialized completely.

Regards,
Tobias
Marton Balint May 25, 2018, 8:20 a.m. UTC | #8
On Fri, 25 May 2018, Tobias Rapp wrote:

> On 25.05.2018 07:58, Xiang, Haihao wrote:
>> On Thu, 2018-05-24 at 11:15 +0100, Mark Thompson wrote:
>>>
>>> For example:
>>>
>>> ffmpeg -i bt709_input.mkv -vf colorspace=bt2020 bt2020_output.mkv
>>>
>>> will have the output file marked as BT.709 after this patch, where 
> previously
>>> it was "unspecified".  (Explicitly setting -color_primaries/-color_trc/-
>>> colorspace on the output works in both cases.)
>> 
>> I agree with you it's not worse than before as we don't get the expected 
> result
>> in both cases.
>
> Not quite: When a file says "I don't know this property value" you have 
> a chance to lookup the value somewhere else or use a default. When it 
> says "I know the value" and gives a wrong value, you completely loose trust.
>
> So in my opinion this patch should not be applied, as it possibly makes 
> ffmpeg generate files with wrong information where it just had no 
> information before. The correct approach would be to set the encoder 
> properties from output frame data and only write a header once the 
> encoders have been initialized completely.

I agree.

Regards,
Marton
Mark Thompson May 26, 2018, 4:29 p.m. UTC | #9
On 25/05/18 07:57, Tobias Rapp wrote:
> On 25.05.2018 07:58, Xiang, Haihao wrote:
>> On Thu, 2018-05-24 at 11:15 +0100, Mark Thompson wrote:
>>>
>>> For example:
>>>
>>> ffmpeg -i bt709_input.mkv -vf colorspace=bt2020 bt2020_output.mkv
>>>
>>> will have the output file marked as BT.709 after this patch, where previously
>>> it was "unspecified".  (Explicitly setting -color_primaries/-color_trc/-
>>> colorspace on the output works in both cases.)
>>
>> I agree with you it's not worse than before as we don't get the expected result
>> in both cases.
> 
> Not quite: When a file says "I don't know this property value" you have a chance to lookup the value somewhere else or use a default. When it says "I know the value" and gives a wrong value, you completely loose trust.

Right, that is a compelling argument.  I agree with you, so I definitely won't apply the patch in this form.

> So in my opinion this patch should not be applied, as it possibly makes ffmpeg generate files with wrong information where it just had no information before. The correct approach would be to set the encoder properties from output frame data and only write a header once the encoders have been initialized completely.

Passing the information through libavfilter should be equivalent, but yeah.

Thanks,

- Mark
Xiang, Haihao May 28, 2018, 7:30 a.m. UTC | #10
On Sat, 2018-05-26 at 17:29 +0100, Mark Thompson wrote:
> On 25/05/18 07:57, Tobias Rapp wrote:

> > On 25.05.2018 07:58, Xiang, Haihao wrote:

> > > On Thu, 2018-05-24 at 11:15 +0100, Mark Thompson wrote:

> > > > 

> > > > For example:

> > > > 

> > > > ffmpeg -i bt709_input.mkv -vf colorspace=bt2020 bt2020_output.mkv

> > > > 

> > > > will have the output file marked as BT.709 after this patch, where

> > > > previously

> > > > it was "unspecified".  (Explicitly setting -color_primaries/-color_trc/-

> > > > colorspace on the output works in both cases.)

> > > 

> > > I agree with you it's not worse than before as we don't get the expected

> > > result

> > > in both cases.

> > 

> > Not quite: When a file says "I don't know this property value" you have a

> > chance to lookup the value somewhere else or use a default. When it says "I

> > know the value" and gives a wrong value, you completely loose trust.

> 

> Right, that is a compelling argument.  I agree with you, so I definitely won't

> apply the patch in this form.

> 


According the comment in avcodec.h, the color properties in AVCodecContext
should be set by user for encoding. I think ffmpeg is the user in the case
below. Where are the color properties set if we don't set the default values in
init_output_stream_encode()?

ffmpeg -hwaccel vaapi -hwaccel_device /dev/dri/renderD128 -hwaccel_output_format
vaapi -i input-with-hdr.mkv -c:v hevc_vaapi -profile:v main10 output.h265


> > So in my opinion this patch should not be applied, as it possibly makes

> > ffmpeg generate files with wrong information where it just had no

> > information before. The correct approach would be to set the encoder

> > properties from output frame data and only write a header once the encoders

> > have been initialized completely.

> 

> Passing the information through libavfilter should be equivalent, but yeah.

> 


In hevc_vaapi encoder, the color properties in AVCodecContext are used to
initialize sequence parameters and extradata is written when AVCodec.init() is
called, where AVFrame is unavailable. 

Actually I have another patch to use the properties in AVFrame to update
sequence parameters however it will result in mismatch in sps because extradata
is written in the initialization path. 

Both AVCodecContext and AVFrame have color properties, which one should be used
for encoding?

BTW I debugged ffmpeg with args set to '-i bt709_input.mkv -vf colorspace=bt2020
bt2020_output.mkv' and found the color properties have been set correctly for
input and output AVFrames when create_filtergraph() is called w/wo this patch:

(gdb) p out->color_primaries
$16 = AVCOL_PRI_BT2020
(gdb) p in->color_primaries 
$17 = AVCOL_PRI_BT709

I guess color properties in AVCodecContext are used when writing .mkv, 

Thanks
Haihao
Tobias Rapp May 29, 2018, 12:20 p.m. UTC | #11
On 28.05.2018 09:30, Xiang, Haihao wrote:
> On Sat, 2018-05-26 at 17:29 +0100, Mark Thompson wrote:
>> On 25/05/18 07:57, Tobias Rapp wrote:
>>> On 25.05.2018 07:58, Xiang, Haihao wrote:
>>>> On Thu, 2018-05-24 at 11:15 +0100, Mark Thompson wrote:
>>>>>
>>>>> For example:
>>>>>
>>>>> ffmpeg -i bt709_input.mkv -vf colorspace=bt2020 bt2020_output.mkv
>>>>>
>>>>> will have the output file marked as BT.709 after this patch, where
>>>>> previously
>>>>> it was "unspecified".  (Explicitly setting -color_primaries/-color_trc/-
>>>>> colorspace on the output works in both cases.)
>>>>
>>>> I agree with you it's not worse than before as we don't get the expected
>>>> result
>>>> in both cases.
>>>
>>> Not quite: When a file says "I don't know this property value" you have a
>>> chance to lookup the value somewhere else or use a default. When it says "I
>>> know the value" and gives a wrong value, you completely loose trust.
>>
>> Right, that is a compelling argument.  I agree with you, so I definitely won't
>> apply the patch in this form.
>>
> 
> According the comment in avcodec.h, the color properties in AVCodecContext
> should be set by user for encoding. I think ffmpeg is the user in the case
> below. Where are the color properties set if we don't set the default values in
> init_output_stream_encode()?
> 
> ffmpeg -hwaccel vaapi -hwaccel_device /dev/dri/renderD128 -hwaccel_output_format
> vaapi -i input-with-hdr.mkv -c:v hevc_vaapi -profile:v main10 output.h265

Setting color properties in init_output_stream_encode() basically looks 
OK, but the question "when" seems more important than "where". It should 
be called after some output frame is available, and use that to 
initialize the encoder. Currently the current AVFrame is not available 
in init_output_stream_encode(), only the buffersink properties.

Hopefully somebody with more knowledge about FFmpeg infrastructure will 
make a recommendation on how this should be solved: By adding color 
properties and other frame data to the buffersink interface? Or by 
making the current/last frame available in init_output_stream_encode()? 
Or ...?

It doesn't need to be auto-negotiated like in the mentioned patch series 
for color_range, it just needs to be forwarded properly from filter 
graph output to the encoder.

Regards,
Tobias
diff mbox

Patch

diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
index 5a19a09d9a..80b5441f8f 100644
--- a/fftools/ffmpeg.c
+++ b/fftools/ffmpeg.c
@@ -3298,6 +3298,10 @@  static int init_output_stream_encode(OutputStream *ost)
         dec_ctx = ist->dec_ctx;
 
         enc_ctx->chroma_sample_location = dec_ctx->chroma_sample_location;
+        enc_ctx->color_range            = dec_ctx->color_range;
+        enc_ctx->color_primaries        = dec_ctx->color_primaries;
+        enc_ctx->color_trc              = dec_ctx->color_trc;
+        enc_ctx->colorspace             = dec_ctx->colorspace;
     } else {
         for (j = 0; j < oc->nb_streams; j++) {
             AVStream *st = oc->streams[j];