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

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