[FFmpeg-devel] [PATCH 1/3] avformat/mxfdec: SMPTE RDD 48:2018 support

[Tomas Härdin]

mån 2022-07-11 klockan 23:44 +0200 skrev Michael Niedermayer:
> 
> +    { {
> 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x0D,0x04,0x01,0x02,0x02,0x03,0x09
> ,0x01,0x00 }, 15,       AV_CODEC_ID_FFV1 }, /*FFV1 V0 */
> +    { {
> 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x0D,0x04,0x01,0x02,0x02,0x03,0x09
> ,0x02,0x00 }, 15,       AV_CODEC_ID_FFV1 }, /*FFV1 V1 */
> +    { {

Double-checked, these are correct

> +typedef struct MXFFFV1SubDescriptor {
> +    MXFMetadataSet meta;
> +    uint8_t *extradata;
> +    int extradata_size;

Is FFV1 extradata size bounded? It so we could avoid an allocation.
Either way the local set syntax limits this to 64k, see below.

> +static const uint8_t mxf_ffv1_extradata[]                  = {
> 0x06,0x0e,0x2b,0x34,0x01,0x01,0x01,0x0e,0x04,0x01,0x06,0x0c,0x01,0x00
> ,0x00,0x00 };

Maybe add a comment // FFV1InitializationMetadata

> +static int mxf_read_ffv1_sub_descriptor(void *arg, AVIOContext *pb,
> int tag, int size, UID uid, int64_t klv_offset)
> +{
> +    MXFFFV1SubDescriptor *ffv1_sub_descriptor = arg;
> +
> +    if (IS_KLV_KEY(uid, mxf_ffv1_extradata)) {
> +        if (ffv1_sub_descriptor->extradata)
> +            av_log(NULL, AV_LOG_WARNING, "Duplicate
> ffv1_extradata\n");

Perhaps we should pass AVFormatContext* along with these functions to
enable proper logging. Doesn't need to hold up this patch though.

> +        av_free(ffv1_sub_descriptor->extradata);
> +        ffv1_sub_descriptor->extradata_size = 0;
> +        ffv1_sub_descriptor->extradata = av_malloc(size);
> +        if (!ffv1_sub_descriptor->extradata)
> +            return AVERROR(ENOMEM);
> +        ffv1_sub_descriptor->extradata_size = size;

This could be simplified with av_fast_malloc(), or no allocation at all
if we use a static sized array.

> +        avio_read(pb, ffv1_sub_descriptor->extradata, size);
> +    }

I presume the other items (GOP size, version number etc) are of no
interest and can be probed by the decoder.

> +    { {
> 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x0d,0x0d,0x01,0x03,0x01,0x02,0x23
> ,0x01,0x00 }, 14,       AV_CODEC_ID_FFV1, NULL, 14 },

matching_len and wrapping_indicator_pos appear to be correct.
mxf_get_wrapping_kind() should report the correct wrapping type AFAICT.

> +static void parse_ffv1_sub_descriptor(MXFContext *mxf, MXFTrack
> *source_track, MXFDescriptor *descriptor, AVStream *st)
> +{
> +    for (int i = 0; i < descriptor->sub_descriptors_count; i++) {
> +        MXFFFV1SubDescriptor *ffv1_sub_descriptor =
> mxf_resolve_strong_ref(mxf, &descriptor->sub_descriptors_refs[i],
> FFV1SubDescriptor);
> +        if (ffv1_sub_descriptor == NULL)
> +            continue;
> +
> +        descriptor->extradata      = ffv1_sub_descriptor->extradata;
> +        descriptor->extradata_size = ffv1_sub_descriptor-
> >extradata_size;
> +        ffv1_sub_descriptor->extradata = NULL;
> +        ffv1_sub_descriptor->extradata_size = 0;

I guess this will require an allocation either way

> +    { {
> 0x06,0x0e,0x2b,0x34,0x02,0x53,0x01,0x01,0x0d,0x01,0x01,0x01,0x01,0x01
> ,0x81,0x03 }, mxf_read_ffv1_sub_descriptor,
> sizeof(MXFFFV1SubDescriptor), FFV1SubDescriptor },

The spec says 0x7F not 0x53. 0x53 is used in groups with 2-byte tags
rather than full KLVs. The intent here seems to be to use local tags,
which fortuitously limits extradata_size to 64k. This makes me think
Amendment 1:2022 is wrong or that 0x7F is just to signal private use
until it gets rolled into the next version of RDD 48.

Tables 18 and 23 in S377m-1-2009 say that 0x7F corresponds to "Abstract
Groups" which are "never encoded as Metadata Sets".

Reading S336m-2007 it seems one can actually use various lengths and
tag sizes. 0x53 corresponds to 2 byte length and 2 byte tag. S377m says
that in addition to this, 0x13 is allowed in MXF which uses ASN.1 BER
encoded lengths. Don't know if any files in the wild use that. Probably
not.

/Tomas