[FFmpeg-devel] [PATCH 1/3] avformat/mxfdec: Read Mastering Display Colour Volume from MXF

[Harry Mallon]

> On 7 Sep 2020, at 10:40, Tomas Härdin <tjoppen at acc.umu.se> wrote:
> 
> mån 2020-08-31 klockan 20:07 +0100 skrev Harry Mallon:
>> +static const uint8_t mxf_mastering_display_primaries[]                = { 0x06,0x0e,0x2b,0x34,0x01,0x01,0x01,0x0e,0x04,0x20,0x04,0x01,0x01,0x01,0x00,0x00 };
>> +static const uint8_t mxf_mastering_display_white_point_chromaticity[] = { 0x06,0x0e,0x2b,0x34,0x01,0x01,0x01,0x0e,0x04,0x20,0x04,0x01,0x01,0x02,0x00,0x00 };
>> +static const uint8_t mxf_mastering_display_maximum_luminance[]        = { 0x06,0x0e,0x2b,0x34,0x01,0x01,0x01,0x0e,0x04,0x20,0x04,0x01,0x01,0x03,0x00,0x00 };
>> +static const uint8_t mxf_mastering_display_minimum_luminance[]        = { 0x06,0x0e,0x2b,0x34,0x01,0x01,0x01,0x0e,0x04,0x20,0x04,0x01,0x01,0x04,0x00,0x00 };
> 
> I can't find any of these ULs in any of the specs I have on hand. Are
> they all defined in 2067-21? ULs for for example primaries are already
> defined in RP210 (06.0E.2B.34.01.01.01.09.04.01.02.01.01.06.00.00) and
> RP224 (06.0E.2B.34.04.01.01.06.04.01.01.01.03.00.00.00).

SMPTEST 2067-21:2020 (available free atm https://ieeexplore.ieee.org/stamp/stamp.jsp?tp=&arnumber=9097487), Annex B. The UIDs are split over multiple lines, preventing easy grepping. :)

> 
> 
>> @@ -1272,6 +1280,60 @@ static int mxf_read_generic_descriptor(void *arg, AVIOContext *pb, int tag, int
>>                 rsiz == FF_PROFILE_JPEG2000_DCINEMA_4K)
>>                 descriptor->pix_fmt = AV_PIX_FMT_XYZ12;
>>         }
>> +        if (IS_KLV_KEY(uid, mxf_mastering_display_primaries)) {
>> +            const int chroma_den = 50000;
> 
> I see this and luma_den used both here and in the mxfenc patch. Since
> they're the same values, consider defining them as macros in mxf.h
> instead.

Done in v2.

> 
>> +            int i;
>> +            if (!descriptor->mastering) {
>> +                descriptor->mastering = av_mastering_display_metadata_alloc();
>> +                if (!descriptor->mastering)
>> +                    return AVERROR(ENOMEM);
>> +            }
> 
> This exact code appears four times, should be de-duplicated. You could
> for example do IS_KLV_KEY() with a prefix of all four keys
> (0x06,0x0e,0x2b,0x34,0x01,0x01,0x01,0x0e,0x04,0x20,0x04,0x01,0x01)
> before these four IS_KLV_KEY() ifs.

Done in v2.

Best,
Harry