Re: [FFmpeg-devel] [PATCH] Set native order for wav channel layouts up until 8 channels.
Le sam. 24 févr. 2024 à 19:27, Michael Niedermayer a écrit : > > On Fri, Feb 23, 2024 at 02:41:06PM -0600, Romain Beauxis wrote: > > The new default channel layout for the various RIFF/WAV decoders is not > > backward compatible. > > > > Historically, most decoders will expect the channel layouts to follow > > the native layout up-to a reasonable number of channels. > > > > Additionally, non-native layouts are causing troubles with filters > > chaining. > > > > This PR changes the default channel layout reported by RIFF/WAV decoders > > to default to the native layout when the number of channels is up-to 8. > > > > The logic for these changes is the same as the logic for the vorbis/opus > > decoders. > > > > Romain > > breaks fate > make -j32 fate-flcl1905 > TESTflcl1905 > --- ./tests/ref/fate/flcl1905 2024-02-09 03:32:32.540199565 +0100 > +++ tests/data/fate/flcl19052024-02-25 02:26:51.079111678 +0100 > @@ -1,192 +1,192 @@ > > packet|codec_type=audio|stream_index=0|pts=0|pts_time=0.00|dts=0|dts_time=0.00|duration=22528|duration_time=0.510839|size=4092|pos=56|flags=K__ > -frame|media_type=audio|stream_index=0|key_frame=1|pts=N/A|pts_time=N/A|pkt_dts=N/A|pkt_dts_time=N/A|best_effort_timestamp=N/A|best_effort_timestamp_time=N/A|pkt_duration=22528|pkt_duration_time=0.510839|duration=22528|duration_time=0.510839|pkt_pos=56|pkt_size=4092|sample_fmt=fltp|nb_samples=2048|channels=2|channel_layout=unknown > -frame|media_type=audio|stream_index=0|key_frame=1|pts=N/A|pts_time=N/A|pkt_dts=N/A|pkt_dts_time=N/A|best_effort_timestamp=N/A|best_effort_timestamp_time=N/A|pkt_duration=22528|pkt_duration_time=0.510839|duration=22528|duration_time=0.510839|pkt_pos=56|pkt_size=4092|sample_fmt=fltp|nb_samples=2048|channels=2|channel_layout=unknown > -frame|media_type=audio|stream_index=0|key_frame=1|pts=N/A|pts_time=N/A|pkt_dts=N/A|pkt_dts_time=N/A|best_effort_timestamp=N/A|best_effort_timestamp_time=N/A|pkt_duration=22528|pkt_duration_time=0.510839|duration=22528|duration_time=0.510839|pkt_pos=56|pkt_size=4092|sample_fmt=fltp|nb_samples=2048|channels=2|channel_layout=unknown > ... Thanks. I did more backward compatibility tests and I might have jumped the gun on it as well. I'll check again and will come back to it if I can confirm anything. -- Romain ___ 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] [PATCH] Set native order for wav channel layouts up until 8 channels.
On Fri, Feb 23, 2024 at 02:41:06PM -0600, Romain Beauxis wrote: > The new default channel layout for the various RIFF/WAV decoders is not > backward compatible. > > Historically, most decoders will expect the channel layouts to follow > the native layout up-to a reasonable number of channels. > > Additionally, non-native layouts are causing troubles with filters > chaining. > > This PR changes the default channel layout reported by RIFF/WAV decoders > to default to the native layout when the number of channels is up-to 8. > > The logic for these changes is the same as the logic for the vorbis/opus > decoders. > > Romain breaks fate make -j32 fate-flcl1905 TESTflcl1905 --- ./tests/ref/fate/flcl1905 2024-02-09 03:32:32.540199565 +0100 +++ tests/data/fate/flcl19052024-02-25 02:26:51.079111678 +0100 @@ -1,192 +1,192 @@ packet|codec_type=audio|stream_index=0|pts=0|pts_time=0.00|dts=0|dts_time=0.00|duration=22528|duration_time=0.510839|size=4092|pos=56|flags=K__ -frame|media_type=audio|stream_index=0|key_frame=1|pts=N/A|pts_time=N/A|pkt_dts=N/A|pkt_dts_time=N/A|best_effort_timestamp=N/A|best_effort_timestamp_time=N/A|pkt_duration=22528|pkt_duration_time=0.510839|duration=22528|duration_time=0.510839|pkt_pos=56|pkt_size=4092|sample_fmt=fltp|nb_samples=2048|channels=2|channel_layout=unknown -frame|media_type=audio|stream_index=0|key_frame=1|pts=N/A|pts_time=N/A|pkt_dts=N/A|pkt_dts_time=N/A|best_effort_timestamp=N/A|best_effort_timestamp_time=N/A|pkt_duration=22528|pkt_duration_time=0.510839|duration=22528|duration_time=0.510839|pkt_pos=56|pkt_size=4092|sample_fmt=fltp|nb_samples=2048|channels=2|channel_layout=unknown -frame|media_type=audio|stream_index=0|key_frame=1|pts=N/A|pts_time=N/A|pkt_dts=N/A|pkt_dts_time=N/A|best_effort_timestamp=N/A|best_effort_timestamp_time=N/A|pkt_duration=22528|pkt_duration_time=0.510839|duration=22528|duration_time=0.510839|pkt_pos=56|pkt_size=4092|sample_fmt=fltp|nb_samples=2048|channels=2|channel_layout=unknown ... [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The real ebay dictionary, page 1 "Used only once"- "Some unspecified defect prevented a second use" "In good condition" - "Can be repaird by experienced expert" "As is" - "You wouldnt want it even if you were payed for it, if you knew ..." signature.asc Description: PGP signature ___ 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] [PATCH] Set native order for wav channel layouts up until 8 channels.
On 2/23/2024 7:32 PM, Romain Beauxis wrote:
Le ven. 23 févr. 2024 à 15:11, Marton Balint a écrit :
On Fri, 23 Feb 2024, Romain Beauxis wrote:
The new default channel layout for the various RIFF/WAV decoders is not
backward compatible.
Historically, most decoders will expect the channel layouts to follow
the native layout up-to a reasonable number of channels.
Additionally, non-native layouts are causing troubles with filters
chaining.
This PR changes the default channel layout reported by RIFF/WAV decoders
to default to the native layout when the number of channels is up-to 8.
The logic for these changes is the same as the logic for the vorbis/opus
decoders.
For Vorbis the channel layout is in the actual Vorbis specification. So
you should follow the specification, simple guessing in the demuxer likely
won't be acceptable.
I would argue that even though there is no official specification on
channel layout for wav/riff, the de-facto assumption that _most_ users
of the library would expect is the native layout.
Typically, 1 and 2 channels would be assumed to be mono and stereo by
most users.
It's great that the API does provide flexibility but the default
should be set to satisfy most users and, in that regard, it seems that
assuming a native layout is what the vast majority of library's users
will expect.
This choice also implies at least two ABI breakage for applications
using the deprecated API but running on a the new ABI:
1: With the updated API, the library is not able to provide a backward
compatible `channel_layout` for AVFrame when the new channel order is
AV_CHANNEL_ORDER_UNSPEC which breaks ABI compatibility as the field is
reported as `0`.
2. AV_CHANNEL_ORDER_UNSPEC channel order also breaks filters chaining.
The issue appears to be that the unspec channel order implicitly sets
the the filter to accept all channel order, which breaks compatibility
here:
if (link->incfg.channel_layouts->all_layouts) {
av_log(link->src, AV_LOG_ERROR, "Cannot select channel layout for"
" the link between filters %s and %s.\n", link->src->name,
link->dst->name);
if (!link->incfg.channel_layouts->all_counts)
av_log(link->src, AV_LOG_ERROR, "Unknown channel layouts not "
"supported, try specifying a channel layout using "
"'aformat=channel_layouts=something'.\n");
return AVERROR(EINVAL);
}
It's not the demuxer's job to guess what the caller would like or
expect. It's meant to export what the input contains.
If you want to fix the CLI behavior, then you need to do it in the CLI.
Remember that ffmpeg.c is not the only lavf user.
___
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] [PATCH] Set native order for wav channel layouts up until 8 channels.
On Fri, 23 Feb 2024, Romain Beauxis wrote:
Le ven. 23 févr. 2024 à 15:11, Marton Balint a écrit :
On Fri, 23 Feb 2024, Romain Beauxis wrote:
> The new default channel layout for the various RIFF/WAV decoders is not
> backward compatible.
>
> Historically, most decoders will expect the channel layouts to follow
> the native layout up-to a reasonable number of channels.
>
> Additionally, non-native layouts are causing troubles with filters
> chaining.
>
> This PR changes the default channel layout reported by RIFF/WAV decoders
> to default to the native layout when the number of channels is up-to 8.
>
> The logic for these changes is the same as the logic for the vorbis/opus
> decoders.
For Vorbis the channel layout is in the actual Vorbis specification. So
you should follow the specification, simple guessing in the demuxer likely
won't be acceptable.
I would argue that even though there is no official specification on
channel layout for wav/riff, the de-facto assumption that _most_ users
of the library would expect is the native layout.
I think this a good starting point:
https://www.mmsp.ece.mcgill.ca/Documents/AudioFormats/WAVE/WAVE.html
Typically, 1 and 2 channels would be assumed to be mono and stereo by
most users.
It's great that the API does provide flexibility but the default
should be set to satisfy most users and, in that regard, it seems that
assuming a native layout is what the vast majority of library's users
will expect.
Actually ffmpeg.c used to have some channel layout guessing code for
exactly this purpose. Maybe you should check why that does not kick in.
This choice also implies at least two ABI breakage for applications
using the deprecated API but running on a the new ABI:
1: With the updated API, the library is not able to provide a backward
compatible `channel_layout` for AVFrame when the new channel order is
AV_CHANNEL_ORDER_UNSPEC which breaks ABI compatibility as the field is
reported as `0`.
I believe this was valid even before the new channel layout API for
unknown channel layouts, so I don't quite see the API break.
2. AV_CHANNEL_ORDER_UNSPEC channel order also breaks filters chaining.
The issue appears to be that the unspec channel order implicitly sets
the the filter to accept all channel order, which breaks compatibility
here:
if (link->incfg.channel_layouts->all_layouts) {
av_log(link->src, AV_LOG_ERROR, "Cannot select channel layout for"
" the link between filters %s and %s.\n", link->src->name,
link->dst->name);
if (!link->incfg.channel_layouts->all_counts)
av_log(link->src, AV_LOG_ERROR, "Unknown channel layouts not "
"supported, try specifying a channel layout using "
"'aformat=channel_layouts=something'.\n");
return AVERROR(EINVAL);
}
A specific ffmpeg command line which shows this error would be useful.
Maybe this can be fixed, if this is a regression.
Regards,
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".
Re: [FFmpeg-devel] [PATCH] Set native order for wav channel layouts up until 8 channels.
Le ven. 23 févr. 2024 à 15:11, Marton Balint a écrit :
>
>
>
> On Fri, 23 Feb 2024, Romain Beauxis wrote:
>
> > The new default channel layout for the various RIFF/WAV decoders is not
> > backward compatible.
> >
> > Historically, most decoders will expect the channel layouts to follow
> > the native layout up-to a reasonable number of channels.
> >
> > Additionally, non-native layouts are causing troubles with filters
> > chaining.
> >
> > This PR changes the default channel layout reported by RIFF/WAV decoders
> > to default to the native layout when the number of channels is up-to 8.
> >
> > The logic for these changes is the same as the logic for the vorbis/opus
> > decoders.
>
> For Vorbis the channel layout is in the actual Vorbis specification. So
> you should follow the specification, simple guessing in the demuxer likely
> won't be acceptable.
I would argue that even though there is no official specification on
channel layout for wav/riff, the de-facto assumption that _most_ users
of the library would expect is the native layout.
Typically, 1 and 2 channels would be assumed to be mono and stereo by
most users.
It's great that the API does provide flexibility but the default
should be set to satisfy most users and, in that regard, it seems that
assuming a native layout is what the vast majority of library's users
will expect.
This choice also implies at least two ABI breakage for applications
using the deprecated API but running on a the new ABI:
1: With the updated API, the library is not able to provide a backward
compatible `channel_layout` for AVFrame when the new channel order is
AV_CHANNEL_ORDER_UNSPEC which breaks ABI compatibility as the field is
reported as `0`.
2. AV_CHANNEL_ORDER_UNSPEC channel order also breaks filters chaining.
The issue appears to be that the unspec channel order implicitly sets
the the filter to accept all channel order, which breaks compatibility
here:
if (link->incfg.channel_layouts->all_layouts) {
av_log(link->src, AV_LOG_ERROR, "Cannot select channel layout for"
" the link between filters %s and %s.\n", link->src->name,
link->dst->name);
if (!link->incfg.channel_layouts->all_counts)
av_log(link->src, AV_LOG_ERROR, "Unknown channel layouts not "
"supported, try specifying a channel layout using "
"'aformat=channel_layouts=something'.\n");
return AVERROR(EINVAL);
}
-- Romain
___
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] [PATCH] Set native order for wav channel layouts up until 8 channels.
On Fri, 23 Feb 2024, Romain Beauxis wrote: The new default channel layout for the various RIFF/WAV decoders is not backward compatible. Historically, most decoders will expect the channel layouts to follow the native layout up-to a reasonable number of channels. Additionally, non-native layouts are causing troubles with filters chaining. This PR changes the default channel layout reported by RIFF/WAV decoders to default to the native layout when the number of channels is up-to 8. The logic for these changes is the same as the logic for the vorbis/opus decoders. For Vorbis the channel layout is in the actual Vorbis specification. So you should follow the specification, simple guessing in the demuxer likely won't be acceptable. Regards, 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".
[FFmpeg-devel] [PATCH] Set native order for wav channel layouts up until 8 channels.
The new default channel layout for the various RIFF/WAV decoders is not
backward compatible.
Historically, most decoders will expect the channel layouts to follow
the native layout up-to a reasonable number of channels.
Additionally, non-native layouts are causing troubles with filters
chaining.
This PR changes the default channel layout reported by RIFF/WAV decoders
to default to the native layout when the number of channels is up-to 8.
The logic for these changes is the same as the logic for the vorbis/opus
decoders.
Romain
---
libavformat/riff.h| 1 +
libavformat/riffdec.c | 31 ---
libavformat/wavdec.c | 4 +---
3 files changed, 30 insertions(+), 6 deletions(-)
diff --git a/libavformat/riff.h b/libavformat/riff.h
index a93eadfeca..f474efdce9 100644
--- a/libavformat/riff.h
+++ b/libavformat/riff.h
@@ -67,6 +67,7 @@ void ff_put_bmp_header(AVIOContext *pb, AVCodecParameters
*par, int for_asf, int
int ff_put_wav_header(AVFormatContext *s, AVIOContext *pb, AVCodecParameters
*par, int flags);
enum AVCodecID ff_wav_codec_get_id(unsigned int tag, int bps);
+void ff_get_wav_ch_layout(AVChannelLayout *ch_layout, int channels);
int ff_get_wav_header(void *logctx, AVIOContext *pb, AVCodecParameters *par,
int size, int big_endian);
diff --git a/libavformat/riffdec.c b/libavformat/riffdec.c
index 0fe4e02b7b..bb7c01c5f5 100644
--- a/libavformat/riffdec.c
+++ b/libavformat/riffdec.c
@@ -90,6 +90,33 @@ static void parse_waveformatex(void *logctx, AVIOContext
*pb, AVCodecParameters
}
}
+const AVChannelLayout ff_wav_demux_ch_layouts[9] = {
+AV_CHANNEL_LAYOUT_MONO,
+AV_CHANNEL_LAYOUT_STEREO,
+AV_CHANNEL_LAYOUT_SURROUND,
+AV_CHANNEL_LAYOUT_QUAD,
+AV_CHANNEL_LAYOUT_5POINT0_BACK,
+AV_CHANNEL_LAYOUT_5POINT1_BACK,
+{
+.nb_channels = 7,
+.order = AV_CHANNEL_ORDER_NATIVE,
+.u.mask = AV_CH_LAYOUT_5POINT1 | AV_CH_BACK_CENTER,
+},
+AV_CHANNEL_LAYOUT_7POINT1,
+{ 0 }
+};
+
+void ff_get_wav_ch_layout(AVChannelLayout *ch_layout, int channels)
+{
+av_channel_layout_uninit(ch_layout);
+if (channels > 8) {
+ch_layout->order = AV_CHANNEL_ORDER_UNSPEC;
+ch_layout->nb_channels = channels;
+} else {
+av_channel_layout_copy(ch_layout, _wav_demux_ch_layouts[channels -
1]);
+}
+}
+
/* "big_endian" values are needed for RIFX file format */
int ff_get_wav_header(void *logctx, AVIOContext *pb,
AVCodecParameters *par, int size, int big_endian)
@@ -195,9 +222,7 @@ int ff_get_wav_header(void *logctx, AVIOContext *pb,
/* ignore WAVEFORMATEXTENSIBLE layout if different from channel count */
if (channels != par->ch_layout.nb_channels) {
-av_channel_layout_uninit(>ch_layout);
-par->ch_layout.order = AV_CHANNEL_ORDER_UNSPEC;
-par->ch_layout.nb_channels = channels;
+ff_get_wav_ch_layout(>ch_layout, channels);
}
return 0;
diff --git a/libavformat/wavdec.c b/libavformat/wavdec.c
index 0c6629b157..282e1ae017 100644
--- a/libavformat/wavdec.c
+++ b/libavformat/wavdec.c
@@ -222,9 +222,7 @@ static int wav_parse_xma2_tag(AVFormatContext *s, int64_t
size, AVStream *st)
channels += avio_r8(pb);
avio_skip(pb, 3);
}
-av_channel_layout_uninit(>codecpar->ch_layout);
-st->codecpar->ch_layout.order = AV_CHANNEL_ORDER_UNSPEC;
-st->codecpar->ch_layout.nb_channels = channels;
+ff_get_wav_ch_layout(>codecpar->ch_layout, channels);
if (st->codecpar->ch_layout.nb_channels <= 0 || st->codecpar->sample_rate
<= 0)
return AVERROR_INVALIDDATA;
--
2.39.3 (Apple Git-145)
___
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".
