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

[Tomas Härdin]

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).


> @@ -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.

> +            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.

/Tomas