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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Reply via email to