[FFmpeg-devel] [RFC] [PATCH] avcodec/codec_par: Keep format channel layout if specified

[Raphaël Zumer]

Hello,

Tagging this as RFC in case there is disagreement on the correct/desired behavior.

For context:
I am working with spatial audio and noticed that FFmpeg does not honor the SA3D box metadata, which is used to signal ambisonic audio channel layouts.
FFmpeg does parse the SA3D box and applies the specified channel layout properties to the codec parameters. However, they are later reset based on codec-level parameters in avcodec_parameters_from_context(), which is called from multiple locations between SA3D being parsed and the channel layout being resolved in FFprobe.
The result is that only codecs which support ambisonic signaling (I don't know of any besides Opus) can be recognized as ambisonic, despite container-level metadata being present. Since a lot of ambisonic content is distributed in AAC or PCM/Wave format, I think that the SA3D metadata should take precedence over codec-level metadata in this case.

Obviously this means ignoring the codec-level metadata which may not match. If there are reasons to prioritize the channel layout defined at the codec level at the expense of SA3D, please let me know the rationale. Also, I am aware that at least libswresample is also ignoring the container-level metadata, so other changes may be needed to propagate the SA3D properties when decoding/filtering ambisonic tracks, but this allows for detection as a first step.

The implementation could be greatly simplified by not resetting the channel layout, but that would require removing av_channel_layout_uninit() from codec_parameters_reset() or adding an argument to skip it. I would favor the former, but this affects a few other functions in this file so let me know if you have a preference.

I assume this would be a micro version bump (or minor?) but will add that once other details are sorted out.

Thanks,
Raphaël Zumer

Signed-off-by: Raphaël Zumer <rzumer at tebako.net>
---
  libavcodec/codec_par.c | 32 +++++++++++++++++++++++++++++---
  1 file changed, 29 insertions(+), 3 deletions(-)

diff --git a/libavcodec/codec_par.c b/libavcodec/codec_par.c
index 775c187073..0cd707d431 100644
--- a/libavcodec/codec_par.c
+++ b/libavcodec/codec_par.c
@@ -100,10 +100,34 @@ int avcodec_parameters_copy(AVCodecParameters *dst, const AVCodecParameters *src
  int avcodec_parameters_from_context(AVCodecParameters *par,
                                      const AVCodecContext *codec)
  {
+    int keep_ch_layout = par->ch_layout.order != AV_CHANNEL_ORDER_UNSPEC;
+    AVChannelLayout ch_layout;
      int ret;
  
+    /*
+     * The stream codec parameters may have a channel layout set
+     * already that is not represented in the codec context.
+     * For example, spatial audio channel layouts in codecs with no
+     * signaling for them may be decoded from container-level metadata.
+     *
+     * Assume that if the channel order is specified, we should
+     * preserve the existing layout rather than let
+     * avcodec_parameters_from_context() override it.
+     */
+    if (keep_ch_layout) {
+        ret = av_channel_layout_copy(&ch_layout, &par->ch_layout);
+        if (ret < 0)
+            return ret;
+    }
+
      codec_parameters_reset(par);
  
+    if (keep_ch_layout) {
+        ret = av_channel_layout_copy(&par->ch_layout, &ch_layout);
+        if (ret < 0)
+            return ret;
+    }
+
      par->codec_type = codec->codec_type;
      par->codec_id   = codec->codec_id;
      par->codec_tag  = codec->codec_tag;
@@ -146,9 +170,11 @@ FF_DISABLE_DEPRECATION_WARNINGS
  FF_ENABLE_DEPRECATION_WARNINGS
          } else {
  #endif
-        ret = av_channel_layout_copy(&par->ch_layout, &codec->ch_layout);
-        if (ret < 0)
-            return ret;
+        if (!keep_ch_layout) {
+            ret = av_channel_layout_copy(&par->ch_layout, &codec->ch_layout);
+            if (ret < 0)
+                return ret;
+        }
  #if FF_API_OLD_CHANNEL_LAYOUT
  FF_DISABLE_DEPRECATION_WARNINGS
          }
-- 
2.41.0