Re: [FFmpeg-devel] [RFC] [PATCH] avcodec/codec_par: Keep format channel layout if specified
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-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [RFC] [PATCH] avcodec/codec_par: Keep format channel layout if specified
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-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [RFC] [PATCH] avcodec/codec_par: Keep format channel layout if specified
On Sat, Jul 15, 2023 at 4:51 PM Raphaël Zumer
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
> ---
> 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(_layout, >ch_layout);
> +if (ret < 0)
> +return ret;
> +}
> +
> codec_parameters_reset(par);
>
> +if (keep_ch_layout) {
> +ret = av_channel_layout_copy(>ch_layout, _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(>ch_layout, >ch_layout);
> -if (ret < 0)
> -return ret;
> +if (!keep_ch_layout) {
> +ret = av_channel_layout_copy(>ch_layout,
> >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-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [RFC] [PATCH] avcodec/codec_par: Keep format channel layout if specified
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
---
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(_layout, >ch_layout);
+if (ret < 0)
+return ret;
+}
+
codec_parameters_reset(par);
+if (keep_ch_layout) {
+ret = av_channel_layout_copy(>ch_layout, _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(>ch_layout, >ch_layout);
-if (ret < 0)
-return ret;
+if (!keep_ch_layout) {
+ret = av_channel_layout_copy(>ch_layout, >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-requ...@ffmpeg.org with subject "unsubscribe".
