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

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

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

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

[Vittorio Giovara]

On Sat, Jul 15, 2023 at 4:51 PM Raphaël Zumer <raphael.zumer@vimeo.com>
wrote:

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

Any comments on this patch? If no objections I'd like to push it at the end
of the week
-- 
Vittorio
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

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

[Anton Khirnov]

Quoting Vittorio Giovara
> 
> Any comments on this patch? If no objections I'd like to push it at the end
> of the week

Sorry, not acceptable. This is the wrong place to do it.

AVCodecParameters is a dumb container for parameters. It MUST NOT make
any assumptions about who calls it or for what purpose. The caller tells
it to copy data - it copies data.

Sadly I don't have the time to think about this in depth right now (ask
me again in 3 weeks or so), but some potential alternatives:
* handle this explicitly in the caller
* add a new function, say avcodec_parameters_update(), perhaps with a
  flags parameter controlling how exactly is the update to be performed;
  not entirely sure this generalizes well enough

-- 
Anton Khirnov
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

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

[Raphaël Zumer]

Thanks for the reply; I wasn't looking to merge the RFC as-is, but rather to figure out the preferred approach, and most importantly make sure that there is no fundamental disagreement on suppressing codec-level metadata in situations where it conflicts with format-level.

I will see which of the alternative implementations comes out cleaner (*probably* a separate function due to the multiple call locations) and either send a v2 RFC patch or separate set later.

Raphaël Zumer

On 7/25/23 14:53, Anton Khirnov wrote:
> Quoting Vittorio Giovara
>> Any comments on this patch? If no objections I'd like to push it at the end
>> of the week
> Sorry, not acceptable. This is the wrong place to do it.
>
> AVCodecParameters is a dumb container for parameters. It MUST NOT make
> any assumptions about who calls it or for what purpose. The caller tells
> it to copy data - it copies data.
>
> Sadly I don't have the time to think about this in depth right now (ask
> me again in 3 weeks or so), but some potential alternatives:
> * handle this explicitly in the caller
> * add a new function, say avcodec_parameters_update(), perhaps with a
>    flags parameter controlling how exactly is the update to be performed;
>    not entirely sure this generalizes well enough
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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