[FFmpeg-devel] [PATCH] avformat/mxf: support MCA audio information

[Marton Balint]

On Fri, 5 Nov 2021, Marc-Antoine Arnaud wrote:

> ---
> libavformat/mxf.h    |   1 +
> libavformat/mxfdec.c | 280 ++++++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 275 insertions(+), 6 deletions(-)
>

Could you mention in the commit description which 
standards/recommendations cover MCA in mxf?

[...]

> @@ -2328,7 +2480,9 @@ static int mxf_parse_structural_metadata(MXFContext *mxf)
>         AVStream *st;
>         FFStream *sti;
>         AVTimecode tc;
> +        enum AVAudioServiceType *ast;

This declaration can be pushed down to where it is used.

[...]

> +                // Soundfield group
> +                if (IS_KLV_KEY(mca_sub_descriptor->mca_label_dictionary_id, mxf_soundfield_group)) {
> +                    MXFSoundfieldGroupUL* group_ptr = (MXFSoundfieldGroupUL*)&mxf_soundfield_groups[0];

const MXFSoundfieldGroupUL *group_ptr = mxf_soundfield_groups;

> +
> +                    while (group_ptr->uid[0]) {
> +                        if (IS_KLV_KEY(group_ptr->uid, mca_sub_descriptor->mca_label_dictionary_id)) {
> +                            st->codecpar->channel_layout = group_ptr->id;
> +                            break;
> +                        }
> +                        group_ptr++;
> +                    }
> +                }
> +
> +                // Audio channel
> +                if (IS_KLV_KEY(mca_sub_descriptor->mca_label_dictionary_id, mxf_audio_channel)) {
> +                    MXFChannelOrderingUL* channel_ordering_ptr = (MXFChannelOrderingUL*)&mxf_channel_ordering[0];

const MXFChannelOrderingUL *channel_ordering_ptr = mxf_channel_ordering;

> +
> +                    while (channel_ordering_ptr->uid[0]) {
> +                        if (IS_KLV_KEY(channel_ordering_ptr->uid, mca_sub_descriptor->mca_label_dictionary_id)) {

You should check if current_channel < desciptor->channels here, and if 
not, then warn the user and break out of the loop. Otherwise 
current_channel can grow out of array limits.

It should also be checked that channel_ordering_ptr->index < 
descriptor->channels, and if not, then similarly, warn the user and break 
out.

Maybe a hard failure (returning AVERROR_INVALIDDATA) is not necessary, to 
allow some slightly invalid metadata?

> +                            source_track->channel_ordering[current_channel] = channel_ordering_ptr->index;
> +
> +                            if(channel_ordering_ptr->service_type != AV_AUDIO_SERVICE_TYPE_NB) {
> +                                uint8_t* side_data = av_stream_new_side_data(st, AV_PKT_DATA_AUDIO_SERVICE_TYPE, sizeof(*ast));
> +                                if (!side_data)
> +                                    goto fail_and_free;
> +                                ast = (enum AVAudioServiceType*)side_data;
> +                                *ast = channel_ordering_ptr->service_type;
> +                            }
> +
> +                            current_channel += 1;
> +                            break;
> +                        }
> +                        channel_ordering_ptr++;
> +                    }
> +                }
> +

[...]

> +            source_track->require_reordering = 0;
> +            for (j = 0; j < descriptor->channels; ++j) {
> +                if (source_track->channel_ordering[j] != j) {
> +                    source_track->require_reordering = 1;
> +                    break;
> +                }
> +            }

If require_reordering == 1 here but either codec is not PCM or 
skip_audio_reordering is set then channel_layout should be reset to 0 
(unknown). Maybe you should also set require_reordering to 0 in either of 
these cases, so further along you no longer have to check codec and 
skip_audio_reordering, only require_reordering.

> +
> +            if (source_track->require_reordering && is_pcm(st->codecpar->codec_id)) {
> +                current_channel = 0;
> +                av_log(mxf->fc, AV_LOG_DEBUG, "MCA Audio mapping (");
> +                for(j = 0; j < descriptor->channels; ++j) {
> +                    for(int k = 0; k < descriptor->channels; ++k) {
> +                        if(source_track->channel_ordering[k] == current_channel) {
> +                            av_log(mxf->fc, AV_LOG_DEBUG, "%d -> %d", source_track->channel_ordering[k], k);
> +                            if (current_channel != descriptor->channels - 1)
> +                                av_log(mxf->fc, AV_LOG_DEBUG, ", ");
> +                            current_channel += 1;
> +                        }
> +                    }
> +                }
> +                av_log(mxf->fc, AV_LOG_DEBUG, ")\n");
> +            }

[...]

> @@ -3920,6 +4185,9 @@ static const AVOption options[] = {
>     { "eia608_extract", "extract eia 608 captions from s436m track",
>       offsetof(MXFContext, eia608_extract), AV_OPT_TYPE_BOOL, {.i64 = 0}, 0, 1,
>       AV_OPT_FLAG_DECODING_PARAM },
> +    { "skip_audio_reordering", "skip audio reordering based on Multi-Channel Audio labelling",
> +      offsetof(MXFContext, skip_audio_reordering), AV_OPT_TYPE_BOOL, {.i64 = 0}, 0, 1,
> +      AV_OPT_FLAG_DECODING_PARAM },

Docs update (doc/demuxers.texi) and libavformat version micro bump is 
missing for the new option.

Thanks,
Marton