| Message ID | 20220120160506.32698-3-anton@khirnov.net |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/3] lavc/encode: factor audio/video-specific parts out of ff_encode_preinit() | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
| andriy/make_ppc | success | Make finished |
| andriy/make_fate_ppc | fail | Make fate failed |
| andriy/make_armv7_RPi4 | success | Make finished |
| andriy/make_fate_armv7_RPi4 | success | Make fate finished |
| andriy/make_aarch64_jetson | success | Make finished |
| andriy/make_fate_aarch64_jetson | success | Make fate finished |
Op do 20 jan. 2022 om 17:05 schreef Anton Khirnov <anton@khirnov.net>: > Fixes #9563. > --- > libavcodec/encode.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/libavcodec/encode.c b/libavcodec/encode.c > index b6f81d1458..44ab81af3f 100644 > --- a/libavcodec/encode.c > +++ b/libavcodec/encode.c > @@ -535,6 +535,9 @@ static int encode_preinit_audio(AVCodecContext *avctx) > return AVERROR(EINVAL); > } > > + if (!avctx->bits_per_raw_sample) > + avctx->bits_per_raw_sample = 8 * > av_get_bytes_per_sample(avctx->sample_fmt); > + > return 0; > } > This creates a new regression: now 24-bit WAV files are converted to 32-bit WavPack files. I think I found the cause of the problem though: the issue is that wavpack uses sample format s32p and 24-bit wav files use s32. Therefore, a aresample filter is auto-inserted to convert s32p to s32, which causes ost->filter->graph->is_meta to be false.
Quoting Martijn van Beurden (2022-01-20 19:58:54) > Op do 20 jan. 2022 om 17:05 schreef Anton Khirnov <anton@khirnov.net>: > > > Fixes #9563. > > --- > > libavcodec/encode.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/libavcodec/encode.c b/libavcodec/encode.c > > index b6f81d1458..44ab81af3f 100644 > > --- a/libavcodec/encode.c > > +++ b/libavcodec/encode.c > > @@ -535,6 +535,9 @@ static int encode_preinit_audio(AVCodecContext *avctx) > > return AVERROR(EINVAL); > > } > > > > + if (!avctx->bits_per_raw_sample) > > + avctx->bits_per_raw_sample = 8 * > > av_get_bytes_per_sample(avctx->sample_fmt); > > + > > return 0; > > } > > > > This creates a new regression: now 24-bit WAV files are converted to 32-bit > WavPack files. I think I found the cause of the problem though: the issue > is that wavpack uses sample format s32p and 24-bit wav files use s32. > Therefore, a aresample filter is auto-inserted to convert s32p to s32, > which causes ost->filter->graph->is_meta to be false. You can work around this by using the -bits_per_raw_sample option. Handling this in a more automagic manner would be complicated - we could propagate bits_per_raw_sample through lavfi or add it to AVFrame.
Op do 20 jan. 2022 om 19:58 schreef Martijn van Beurden <mvanb1@gmail.com>: > This creates a new regression: now 24-bit WAV files are converted to > 32-bit WavPack files. I think I found the cause of the problem though: the > issue is that wavpack uses sample format s32p and 24-bit wav files use s32. > Therefore, a aresample filter is auto-inserted to convert s32p to s32, > which causes ost->filter->graph->is_meta to be false. > Also, I see an abuffer is also added to the filtergraph which too causes ost->filter->graph->is_meta to be false.
Op do 20 jan. 2022 22:18 schreef Anton Khirnov <anton@khirnov.net>: > Quoting Martijn van Beurden (2022-01-20 19:58:54) > > Op do 20 jan. 2022 om 17:05 schreef Anton Khirnov <anton@khirnov.net>: > > > > > Fixes #9563. > > > --- > > > libavcodec/encode.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/libavcodec/encode.c b/libavcodec/encode.c > > > index b6f81d1458..44ab81af3f 100644 > > > --- a/libavcodec/encode.c > > > +++ b/libavcodec/encode.c > > > @@ -535,6 +535,9 @@ static int encode_preinit_audio(AVCodecContext > *avctx) > > > return AVERROR(EINVAL); > > > } > > > > > > + if (!avctx->bits_per_raw_sample) > > > + avctx->bits_per_raw_sample = 8 * > > > av_get_bytes_per_sample(avctx->sample_fmt); > > > + > > > return 0; > > > } > > > > > > > This creates a new regression: now 24-bit WAV files are converted to > 32-bit > > WavPack files. I think I found the cause of the problem though: the issue > > is that wavpack uses sample format s32p and 24-bit wav files use s32. > > Therefore, a aresample filter is auto-inserted to convert s32p to s32, > > which causes ost->filter->graph->is_meta to be false. > > You can work around this by using the -bits_per_raw_sample option. > Handling this in a more automagic manner would be complicated - we could > propagate bits_per_raw_sample through lavfi or add it to AVFrame. > I think this patch shouldn't be applied. Without it 32 bit audio is transcoded to 24 bit audio, which is not lossless. However, with this patch 24 bit audio is transcoded to 32 bit audio which is lossless, but the resulting files are understood by few software decoder and probably no hardware decoders. Perhaps adding a warning instead of setting a default would be better? >
Quoting Martijn van Beurden (2022-01-23 14:15:53) > I think this patch shouldn't be applied. Without it 32 bit audio is > transcoded to 24 bit audio, which is not lossless. However, with this patch > 24 bit audio is transcoded to 32 bit audio which is lossless, but the > resulting files are understood by few software decoder and probably no > hardware decoders. Do you know which decoders do not support 32bit audio and how widely used they are? I'm not very familiar with wavpack, but 24bit and 32bit decoding don't seem significantly different in the decoder. > Perhaps adding a warning instead of setting a default would be better? IMO the encoder should strive to be lossless, so it should not encode as 24bit unless it was explicitly told the input data is 24bit. bits_per_raw_sample=0 is not a meaningful value, so the encoder should not treat it as 24bit.
ping any more opinions on this?
On Sun, Feb 20, 2022 at 3:19 PM Anton Khirnov <anton@khirnov.net> wrote: > ping > > any more opinions on this? > I'm OK with this patch. If 24bit is still needed it can be set in usual already known way, by setting additional flag. > > -- > 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". >
Ping for this. Current state is imho bad.
Quoting Paul B Mahol (2022-04-06 13:24:35)
> Ping for this. Current state is imho bad.
pushed
diff --git a/libavcodec/encode.c b/libavcodec/encode.c index b6f81d1458..44ab81af3f 100644 --- a/libavcodec/encode.c +++ b/libavcodec/encode.c @@ -535,6 +535,9 @@ static int encode_preinit_audio(AVCodecContext *avctx) return AVERROR(EINVAL); } + if (!avctx->bits_per_raw_sample) + avctx->bits_per_raw_sample = 8 * av_get_bytes_per_sample(avctx->sample_fmt); + return 0; }