[FFmpeg-devel] [PATCH] avcodec/dpxenc: stop hardcoding color trc/primaries

[Harry Mallon]

>> 
>>> 
>>> [..]
>>> +static int get_dpx_pri(int color_pri)
>>> +{
>>> +    switch (color_pri) {
>>> +    case AVCOL_PRI_BT709:
>>> +        return 6;
>>> +    case AVCOL_PRI_SMPTE240M:
>>> +    case AVCOL_PRI_SMPTE170M:
>>> +        return 9;
>> 
>> I think perhaps this should be 8 (ITU 601 525), rather than 9 (Composite video SMPTE 170M), but I am not sure?
> 
> The smpte170m is explicitly mention in specification, so make sure you use latest version of it.

Let me try to explain myself a little better. AVCOL_PRI_SMPTE170M does not mean SMPTE170M, it means SMPTE170M, ITU-R BT601-6 525, ITU-R BT1358 525 or ITU-R BT1700 NTSC (see pixfmt.h). We will have to pick a DPX option though, which could either be 9 (SMPTE170M) or 8 (ITU-R 601 525). My guess is that the ITU 601 standard would be the most obvious choice. I guess it probably doesn't matter as the options amount to the same thing, DPX does prefer that the two bytes match in V2.0 though.

> 
>> 
>>> +    case AVCOL_PRI_BT470BG:
>>> +        return 10;
>> 
>> Perhaps this should be 7 (ITU 601 625), rather than 10 (ITU 624-4 Composite video PAL), again not sure which is most widely used?
> 
> see first comment.

The primaries of ITU 624-4 PAL and ITU 601 625 are the same here, so choosing between 7 and 10 is difficult. I wonder whether the ITU 601 option is the least surprising. BT470BG is not explicitly mentioned in the DPX specification.

> 
>> 
>>> +    default:
>>> +        return 0;
>>> +    }
>>> +}
>> 
>> In DPX files containing colour difference information the colorspace would also be keyed off the value returned from this function. Perhaps it should be taken into account here (for files containing YCbCr)?
> 
> Sorry, this does not make sense, the color_prim/trc is meaningfull for both yuv and rgb.

In FFMPEG we have 3 things AVColorPrimaries, AVColorTransferCharacteristic and AVColorSpace. In DPX we have only two; Transfer Characteristic and Colorimetric Specification. If we were to read these values when decoding dpx files we would choose what to set for AVColorSpace (the YCbCr matrix) based on the Colorimetric Specification. I think that should mean that we should take the AVColorSpace and the AVColorPrimaries into consideration when choosing what to put in this byte, when writing YUV DPX files.

> 
>> 
>>> +
>>> +static int get_dpx_trc(int color_trc)
>>> +{
>>> +    switch (color_trc) {
>>> +    case AVCOL_TRC_LINEAR:
>>> +        return 2;
>>> +    case AVCOL_TRC_BT709:
>>> +        return 6;
>>> +    case AVCOL_TRC_SMPTE240M:
>>> +    case AVCOL_TRC_SMPTE170M:
>>> +        return 9;
>> 
>> This value could be 7, 8 or 9. From my reading of the spec the best might be to take colour_primaries into account and do something like:
>> 
>> if (AVCOL_PRI_BT470BG) return 7 (ITU 601 625)
>> else return 8 (ITU 601 525)
>> 
> 
> see first comment.

See my first comment

> 
>>> +    case AVCOL_TRC_GAMMA22:
>>> +        return 10;
>> 
>> 10 is ITU 624-4 Composite video PAL, which says it has gamma = 2.8 (AVCOL_TRC_GAMMA28). https://www.itu.int/dms_pub/itu-r/opb/rep/R-REP-BT.624-4-1990-PDF-E.pdf
>> 
> 
> Hmm i will double check.
> 
>>> +    default:
>>> +        return 0;
>>> +    }
>>> +}
>>> +
>>> 
>>> [..]
>>> 
>> [..]