diff mbox series

[FFmpeg-devel,2/2] avcodec/dnxhdenc: properly store colorspace info

Message ID 20200124141430.8675-1-onemda@gmail.com
State New
Headers show
Series Untitled series #222
Related show

Commit Message

Paul B Mahol Jan. 24, 2020, 2:14 p.m. UTC
Signed-off-by: Paul B Mahol <onemda@gmail.com>
---
 libavcodec/dnxhdenc.c | 11 ++++++++++-
 libavcodec/dnxhdenc.h |  1 +
 2 files changed, 11 insertions(+), 1 deletion(-)

Comments

James Almer Jan. 24, 2020, 3:54 p.m. UTC | #1
On 1/24/2020 11:14 AM, Paul B Mahol wrote:
> Signed-off-by: Paul B Mahol <onemda@gmail.com>
> ---
>  libavcodec/dnxhdenc.c | 11 ++++++++++-
>  libavcodec/dnxhdenc.h |  1 +
>  2 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/libavcodec/dnxhdenc.c b/libavcodec/dnxhdenc.c
> index 2feb8baf21..03835fa794 100644
> --- a/libavcodec/dnxhdenc.c
> +++ b/libavcodec/dnxhdenc.c
> @@ -406,6 +406,15 @@ static av_cold int dnxhd_encode_init(AVCodecContext *avctx)
>      }
>  
>      ctx->is_444 = ctx->profile == FF_PROFILE_DNXHR_444;
> +
> +    switch (avctx->colorspace) {
> +    case AVCOL_SPC_UNSPECIFIED:

Isn't 3 meant to be unspecified? If not, what is it for?

> +    case AVCOL_SPC_BT709:      ctx->colorspace = 0; break;
> +    case AVCOL_SPC_BT2020_NCL: ctx->colorspace = 1; break;
> +    case AVCOL_SPC_BT2020_CL:  ctx->colorspace = 2; break;
> +    default:                   ctx->colorspace = 3; break;
> +    }
> +
>      avctx->profile = ctx->profile;
>      ctx->cid = ff_dnxhd_find_cid(avctx, ctx->bit_depth);
>      if (!ctx->cid) {
> @@ -576,7 +585,7 @@ static int dnxhd_write_header(AVCodecContext *avctx, uint8_t *buf)
>      buf[0x21] = ctx->bit_depth == 10 ? 0x58 : 0x38;
>      buf[0x22] = 0x88 + (ctx->interlaced << 2);
>      AV_WB32(buf + 0x28, ctx->cid); // CID
> -    buf[0x2c] = (!ctx->interlaced << 7) | (ctx->is_444 << 6) | (avctx->pix_fmt == AV_PIX_FMT_YUV444P10);
> +    buf[0x2c] = (!ctx->interlaced << 7) | (ctx->is_444 << 6) | (ctx->colorspace << 1) | (avctx->pix_fmt == AV_PIX_FMT_YUV444P10);
>  
>      buf[0x5f] = 0x01; // UDL
>  
> diff --git a/libavcodec/dnxhdenc.h b/libavcodec/dnxhdenc.h
> index 7b0d862e28..4d5f457e33 100644
> --- a/libavcodec/dnxhdenc.h
> +++ b/libavcodec/dnxhdenc.h
> @@ -50,6 +50,7 @@ typedef struct DNXHDEncContext {
>      int profile;
>      int bit_depth;
>      int is_444;
> +    int colorspace;
>      const CIDEntry *cid_table;
>      uint8_t *msip; ///< Macroblock Scan Indexes Payload
>      uint32_t *slice_size;
>
Paul B Mahol Jan. 24, 2020, 3:59 p.m. UTC | #2
On 1/24/20, James Almer <jamrial@gmail.com> wrote:
> On 1/24/2020 11:14 AM, Paul B Mahol wrote:
>> Signed-off-by: Paul B Mahol <onemda@gmail.com>
>> ---
>>  libavcodec/dnxhdenc.c | 11 ++++++++++-
>>  libavcodec/dnxhdenc.h |  1 +
>>  2 files changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/libavcodec/dnxhdenc.c b/libavcodec/dnxhdenc.c
>> index 2feb8baf21..03835fa794 100644
>> --- a/libavcodec/dnxhdenc.c
>> +++ b/libavcodec/dnxhdenc.c
>> @@ -406,6 +406,15 @@ static av_cold int dnxhd_encode_init(AVCodecContext
>> *avctx)
>>      }
>>
>>      ctx->is_444 = ctx->profile == FF_PROFILE_DNXHR_444;
>> +
>> +    switch (avctx->colorspace) {
>> +    case AVCOL_SPC_UNSPECIFIED:
>
> Isn't 3 meant to be unspecified? If not, what is it for?

This is to keep old behavior same. Where encoder would store
unspecified as bt709.
Mainly to not need to modify fate tests.

>
>> +    case AVCOL_SPC_BT709:      ctx->colorspace = 0; break;
>> +    case AVCOL_SPC_BT2020_NCL: ctx->colorspace = 1; break;
>> +    case AVCOL_SPC_BT2020_CL:  ctx->colorspace = 2; break;
>> +    default:                   ctx->colorspace = 3; break;
>> +    }
>> +
>>      avctx->profile = ctx->profile;
>>      ctx->cid = ff_dnxhd_find_cid(avctx, ctx->bit_depth);
>>      if (!ctx->cid) {
>> @@ -576,7 +585,7 @@ static int dnxhd_write_header(AVCodecContext *avctx,
>> uint8_t *buf)
>>      buf[0x21] = ctx->bit_depth == 10 ? 0x58 : 0x38;
>>      buf[0x22] = 0x88 + (ctx->interlaced << 2);
>>      AV_WB32(buf + 0x28, ctx->cid); // CID
>> -    buf[0x2c] = (!ctx->interlaced << 7) | (ctx->is_444 << 6) |
>> (avctx->pix_fmt == AV_PIX_FMT_YUV444P10);
>> +    buf[0x2c] = (!ctx->interlaced << 7) | (ctx->is_444 << 6) |
>> (ctx->colorspace << 1) | (avctx->pix_fmt == AV_PIX_FMT_YUV444P10);
>>
>>      buf[0x5f] = 0x01; // UDL
>>
>> diff --git a/libavcodec/dnxhdenc.h b/libavcodec/dnxhdenc.h
>> index 7b0d862e28..4d5f457e33 100644
>> --- a/libavcodec/dnxhdenc.h
>> +++ b/libavcodec/dnxhdenc.h
>> @@ -50,6 +50,7 @@ typedef struct DNXHDEncContext {
>>      int profile;
>>      int bit_depth;
>>      int is_444;
>> +    int colorspace;
>>      const CIDEntry *cid_table;
>>      uint8_t *msip; ///< Macroblock Scan Indexes Payload
>>      uint32_t *slice_size;
>>
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
James Almer Jan. 24, 2020, 4:04 p.m. UTC | #3
On 1/24/2020 12:59 PM, Paul B Mahol wrote:
> On 1/24/20, James Almer <jamrial@gmail.com> wrote:
>> On 1/24/2020 11:14 AM, Paul B Mahol wrote:
>>> Signed-off-by: Paul B Mahol <onemda@gmail.com>
>>> ---
>>>  libavcodec/dnxhdenc.c | 11 ++++++++++-
>>>  libavcodec/dnxhdenc.h |  1 +
>>>  2 files changed, 11 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/libavcodec/dnxhdenc.c b/libavcodec/dnxhdenc.c
>>> index 2feb8baf21..03835fa794 100644
>>> --- a/libavcodec/dnxhdenc.c
>>> +++ b/libavcodec/dnxhdenc.c
>>> @@ -406,6 +406,15 @@ static av_cold int dnxhd_encode_init(AVCodecContext
>>> *avctx)
>>>      }
>>>
>>>      ctx->is_444 = ctx->profile == FF_PROFILE_DNXHR_444;
>>> +
>>> +    switch (avctx->colorspace) {
>>> +    case AVCOL_SPC_UNSPECIFIED:
>>
>> Isn't 3 meant to be unspecified? If not, what is it for?
> 
> This is to keep old behavior same. Where encoder would store
> unspecified as bt709.
> Mainly to not need to modify fate tests.

The proper thing to do is adapt the tests to the new correct behavior,
not keeping an encoder doing things technically wrong just to not update
the tests.

> 
>>
>>> +    case AVCOL_SPC_BT709:      ctx->colorspace = 0; break;
>>> +    case AVCOL_SPC_BT2020_NCL: ctx->colorspace = 1; break;
>>> +    case AVCOL_SPC_BT2020_CL:  ctx->colorspace = 2; break;
>>> +    default:                   ctx->colorspace = 3; break;
>>> +    }
>>> +
>>>      avctx->profile = ctx->profile;
>>>      ctx->cid = ff_dnxhd_find_cid(avctx, ctx->bit_depth);
>>>      if (!ctx->cid) {
>>> @@ -576,7 +585,7 @@ static int dnxhd_write_header(AVCodecContext *avctx,
>>> uint8_t *buf)
>>>      buf[0x21] = ctx->bit_depth == 10 ? 0x58 : 0x38;
>>>      buf[0x22] = 0x88 + (ctx->interlaced << 2);
>>>      AV_WB32(buf + 0x28, ctx->cid); // CID
>>> -    buf[0x2c] = (!ctx->interlaced << 7) | (ctx->is_444 << 6) |
>>> (avctx->pix_fmt == AV_PIX_FMT_YUV444P10);
>>> +    buf[0x2c] = (!ctx->interlaced << 7) | (ctx->is_444 << 6) |
>>> (ctx->colorspace << 1) | (avctx->pix_fmt == AV_PIX_FMT_YUV444P10);
>>>
>>>      buf[0x5f] = 0x01; // UDL
>>>
>>> diff --git a/libavcodec/dnxhdenc.h b/libavcodec/dnxhdenc.h
>>> index 7b0d862e28..4d5f457e33 100644
>>> --- a/libavcodec/dnxhdenc.h
>>> +++ b/libavcodec/dnxhdenc.h
>>> @@ -50,6 +50,7 @@ typedef struct DNXHDEncContext {
>>>      int profile;
>>>      int bit_depth;
>>>      int is_444;
>>> +    int colorspace;
>>>      const CIDEntry *cid_table;
>>>      uint8_t *msip; ///< Macroblock Scan Indexes Payload
>>>      uint32_t *slice_size;
>>>
>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
Carl Eugen Hoyos Jan. 24, 2020, 4:09 p.m. UTC | #4
Am Fr., 24. Jan. 2020 um 17:05 Uhr schrieb James Almer <jamrial@gmail.com>:
>
> On 1/24/2020 12:59 PM, Paul B Mahol wrote:
> > On 1/24/20, James Almer <jamrial@gmail.com> wrote:
> >> On 1/24/2020 11:14 AM, Paul B Mahol wrote:
> >>> Signed-off-by: Paul B Mahol <onemda@gmail.com>
> >>> ---
> >>>  libavcodec/dnxhdenc.c | 11 ++++++++++-
> >>>  libavcodec/dnxhdenc.h |  1 +
> >>>  2 files changed, 11 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/libavcodec/dnxhdenc.c b/libavcodec/dnxhdenc.c
> >>> index 2feb8baf21..03835fa794 100644
> >>> --- a/libavcodec/dnxhdenc.c
> >>> +++ b/libavcodec/dnxhdenc.c
> >>> @@ -406,6 +406,15 @@ static av_cold int dnxhd_encode_init(AVCodecContext
> >>> *avctx)
> >>>      }
> >>>
> >>>      ctx->is_444 = ctx->profile == FF_PROFILE_DNXHR_444;
> >>> +
> >>> +    switch (avctx->colorspace) {
> >>> +    case AVCOL_SPC_UNSPECIFIED:
> >>
> >> Isn't 3 meant to be unspecified? If not, what is it for?
> >
> > This is to keep old behavior same. Where encoder would store
> > unspecified as bt709.
> > Mainly to not need to modify fate tests.
>
> The proper thing to do is adapt the tests to the new correct behavior,
> not keeping an encoder doing things technically wrong just to not update
> the tests.

But this could be an independent patch which would be better.

Carl Eugen
diff mbox series

Patch

diff --git a/libavcodec/dnxhdenc.c b/libavcodec/dnxhdenc.c
index 2feb8baf21..03835fa794 100644
--- a/libavcodec/dnxhdenc.c
+++ b/libavcodec/dnxhdenc.c
@@ -406,6 +406,15 @@  static av_cold int dnxhd_encode_init(AVCodecContext *avctx)
     }
 
     ctx->is_444 = ctx->profile == FF_PROFILE_DNXHR_444;
+
+    switch (avctx->colorspace) {
+    case AVCOL_SPC_UNSPECIFIED:
+    case AVCOL_SPC_BT709:      ctx->colorspace = 0; break;
+    case AVCOL_SPC_BT2020_NCL: ctx->colorspace = 1; break;
+    case AVCOL_SPC_BT2020_CL:  ctx->colorspace = 2; break;
+    default:                   ctx->colorspace = 3; break;
+    }
+
     avctx->profile = ctx->profile;
     ctx->cid = ff_dnxhd_find_cid(avctx, ctx->bit_depth);
     if (!ctx->cid) {
@@ -576,7 +585,7 @@  static int dnxhd_write_header(AVCodecContext *avctx, uint8_t *buf)
     buf[0x21] = ctx->bit_depth == 10 ? 0x58 : 0x38;
     buf[0x22] = 0x88 + (ctx->interlaced << 2);
     AV_WB32(buf + 0x28, ctx->cid); // CID
-    buf[0x2c] = (!ctx->interlaced << 7) | (ctx->is_444 << 6) | (avctx->pix_fmt == AV_PIX_FMT_YUV444P10);
+    buf[0x2c] = (!ctx->interlaced << 7) | (ctx->is_444 << 6) | (ctx->colorspace << 1) | (avctx->pix_fmt == AV_PIX_FMT_YUV444P10);
 
     buf[0x5f] = 0x01; // UDL
 
diff --git a/libavcodec/dnxhdenc.h b/libavcodec/dnxhdenc.h
index 7b0d862e28..4d5f457e33 100644
--- a/libavcodec/dnxhdenc.h
+++ b/libavcodec/dnxhdenc.h
@@ -50,6 +50,7 @@  typedef struct DNXHDEncContext {
     int profile;
     int bit_depth;
     int is_444;
+    int colorspace;
     const CIDEntry *cid_table;
     uint8_t *msip; ///< Macroblock Scan Indexes Payload
     uint32_t *slice_size;