From af99f9fc9000e76990795a164dd51fd2bafc34c3 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 5 Jul 2019 21:01:01 +0200 Subject: [PATCH] pcm/Volume: convert S16 to S24 to preserve quality and reduce noise Applying software volume to S16 samples means several bits of precision are lost; at 25% volume, two bits are lost. Additionally, dithering adds some noise. The problem gets worse when you apply the software volume code twice: for the software mixer volume, and again for the replay gain. This loses some more precision and adds even more dithering noise, which can become audible (see https://github.com/MusicPlayerDaemon/MPD/issues/542). By converting everything to 24 bit, we need to shift only two bits to the right instead of ten, losing nearly no precision, and dithering is not needed. Even if the output device is unable to play S24 directly, we can convert back to S16 with only one stage of dithering. Closes https://github.com/MusicPlayerDaemon/MPD/issues/542 --- NEWS | 1 + src/filter/plugins/ReplayGainFilterPlugin.cxx | 25 +++-- src/filter/plugins/ReplayGainFilterPlugin.hxx | 7 +- src/filter/plugins/VolumeFilterPlugin.cxx | 4 +- src/output/Init.cxx | 10 +- src/pcm/Volume.cxx | 92 +++++++++++++++++-- src/pcm/Volume.hxx | 10 +- test/software_volume.cxx | 2 +- test/test_pcm_volume.cxx | 45 ++++++++- 9 files changed, 174 insertions(+), 22 deletions(-) diff --git a/NEWS b/NEWS index 512f59e6b..56b307992 100644 --- a/NEWS +++ b/NEWS @@ -11,6 +11,7 @@ ver 0.22 (not yet released) * filter - ffmpeg: new plugin based on FFmpeg's libavfilter library - hdcd: new plugin based on FFmpeg's "af_hdcd" for HDCD playback + - volume: convert S16 to S24 to preserve quality and reduce dithering noise ver 0.21.12 (not yet released) * decoder diff --git a/src/filter/plugins/ReplayGainFilterPlugin.cxx b/src/filter/plugins/ReplayGainFilterPlugin.cxx index 43906fb63..83fe9d68f 100644 --- a/src/filter/plugins/ReplayGainFilterPlugin.cxx +++ b/src/filter/plugins/ReplayGainFilterPlugin.cxx @@ -69,7 +69,7 @@ class ReplayGainFilter final : public Filter { PcmVolume pv; public: - ReplayGainFilter(const ReplayGainConfig &_config, + ReplayGainFilter(const ReplayGainConfig &_config, bool allow_convert, const AudioFormat &audio_format, Mixer *_mixer, unsigned _base) :Filter(audio_format), @@ -77,7 +77,8 @@ public: mixer(_mixer), base(_base) { info.Clear(); - out_audio_format.format = pv.Open(out_audio_format.format); + out_audio_format.format = pv.Open(out_audio_format.format, + allow_convert); } void SetInfo(const ReplayGainInfo *_info) { @@ -120,6 +121,12 @@ class PreparedReplayGainFilter final : public PreparedFilter { */ Mixer *mixer = nullptr; + /** + * Allow the class to convert to a different #SampleFormat to + * preserve quality? + */ + const bool allow_convert; + /** * The base volume level for scale=1.0, between 1 and 100 * (including). @@ -127,8 +134,9 @@ class PreparedReplayGainFilter final : public PreparedFilter { unsigned base; public: - explicit PreparedReplayGainFilter(const ReplayGainConfig _config) - :config(_config) {} + explicit PreparedReplayGainFilter(const ReplayGainConfig _config, + bool _allow_convert) + :config(_config), allow_convert(_allow_convert) {} void SetMixer(Mixer *_mixer, unsigned _base) { assert(_mixer == nullptr || (_base > 0 && _base <= 100)); @@ -172,15 +180,18 @@ ReplayGainFilter::Update() } std::unique_ptr -NewReplayGainFilter(const ReplayGainConfig &config) noexcept +NewReplayGainFilter(const ReplayGainConfig &config, + bool allow_convert) noexcept { - return std::make_unique(config); + return std::make_unique(config, + allow_convert); } std::unique_ptr PreparedReplayGainFilter::Open(AudioFormat &af) { - return std::make_unique(config, af, mixer, base); + return std::make_unique(config, allow_convert, + af, mixer, base); } ConstBuffer diff --git a/src/filter/plugins/ReplayGainFilterPlugin.hxx b/src/filter/plugins/ReplayGainFilterPlugin.hxx index ef23356ca..87e835af5 100644 --- a/src/filter/plugins/ReplayGainFilterPlugin.hxx +++ b/src/filter/plugins/ReplayGainFilterPlugin.hxx @@ -30,8 +30,13 @@ class Mixer; struct ReplayGainConfig; struct ReplayGainInfo; +/** + * @param allow_convert allow the class to convert to a different + * #SampleFormat to preserve quality? + */ std::unique_ptr -NewReplayGainFilter(const ReplayGainConfig &config) noexcept; +NewReplayGainFilter(const ReplayGainConfig &config, + bool allow_convert) noexcept; /** * Enables or disables the hardware mixer for applying replay gain. diff --git a/src/filter/plugins/VolumeFilterPlugin.cxx b/src/filter/plugins/VolumeFilterPlugin.cxx index 8ebf6c97b..38dabea99 100644 --- a/src/filter/plugins/VolumeFilterPlugin.cxx +++ b/src/filter/plugins/VolumeFilterPlugin.cxx @@ -30,7 +30,8 @@ class VolumeFilter final : public Filter { public: explicit VolumeFilter(const AudioFormat &audio_format) :Filter(audio_format) { - out_audio_format.format = pv.Open(out_audio_format.format); + out_audio_format.format = pv.Open(out_audio_format.format, + true); } unsigned GetVolume() const noexcept { @@ -85,4 +86,3 @@ volume_filter_set(Filter *_filter, unsigned volume) noexcept filter->SetVolume(volume); } - diff --git a/src/output/Init.cxx b/src/output/Init.cxx index 07aa7ddd0..080cc399e 100644 --- a/src/output/Init.cxx +++ b/src/output/Init.cxx @@ -213,12 +213,18 @@ FilteredAudioOutput::Setup(EventLoop &event_loop, block.GetBlockValue("replay_gain_handler", "software"); if (strcmp(replay_gain_handler, "none") != 0) { + /* when using software volume, we lose quality by + invoking PcmVolume::Apply() twice; to avoid losing + too much precision, we allow the ReplayGainFilter + to convert 16 bit to 24 bit */ + const bool allow_convert = mixer_type == MixerType::SOFTWARE; + prepared_replay_gain_filter = - NewReplayGainFilter(replay_gain_config); + NewReplayGainFilter(replay_gain_config, allow_convert); assert(prepared_replay_gain_filter != nullptr); prepared_other_replay_gain_filter = - NewReplayGainFilter(replay_gain_config); + NewReplayGainFilter(replay_gain_config, allow_convert); assert(prepared_other_replay_gain_filter != nullptr); } diff --git a/src/pcm/Volume.cxx b/src/pcm/Volume.cxx index 2c975af67..985da0b5b 100644 --- a/src/pcm/Volume.cxx +++ b/src/pcm/Volume.cxx @@ -30,6 +30,49 @@ #include #include +#if GCC_OLDER_THAN(8,0) +/* GCC 6.3 emits this bogus warning in PcmVolumeConvert() because it + checks an unreachable branch */ +#pragma GCC diagnostic ignored "-Wshift-count-overflow" +#endif + +/** + * Apply software volume, converting to a different sample type. + */ +template, + class DTraits=SampleTraits> +#if !GCC_OLDER_THAN(8,0) +constexpr +#endif +static typename DTraits::value_type +PcmVolumeConvert(typename STraits::value_type _sample, int volume) noexcept +{ + typename STraits::long_type sample(_sample); + sample *= volume; + + static_assert(DTraits::BITS > STraits::BITS, + "Destination sample must be larger than source sample"); + + /* after multiplying with the volume value, the "sample" + variable contains this number of precision bits: source + bits plus the volume bits */ + constexpr unsigned BITS = STraits::BITS + PCM_VOLUME_BITS; + + /* .. and now we need to scale to the requested destination + bits */ + + typename DTraits::value_type result; + if (BITS > DTraits::BITS) + result = sample >> (BITS - DTraits::BITS); + else if (BITS < DTraits::BITS) + result = sample << (DTraits::BITS - BITS); + else + result = sample; + + return result; +} + template> static inline typename Traits::value_type pcm_volume_sample(PcmDither &dither, @@ -71,6 +114,16 @@ pcm_volume_change_16(PcmDither &dither, pcm_volume_change(dither, dest, src, n, volume); } +static void +PcmVolumeChange16to32(int32_t *dest, const int16_t *src, size_t n, + int volume) noexcept +{ + for (size_t i = 0; i != n; ++i) + dest[i] = PcmVolumeConvert(src[i], + volume); +} + static void pcm_volume_change_24(PcmDither &dither, int32_t *dest, const int32_t *src, size_t n, @@ -97,17 +150,31 @@ pcm_volume_change_float(float *dest, const float *src, size_t n, } SampleFormat -PcmVolume::Open(SampleFormat _format) +PcmVolume::Open(SampleFormat _format, bool allow_convert) { assert(format == SampleFormat::UNDEFINED); + convert = false; + switch (_format) { case SampleFormat::UNDEFINED: throw FormatRuntimeError("Software volume for %s is not implemented", sample_format_to_string(_format)); case SampleFormat::S8: + break; + case SampleFormat::S16: + if (allow_convert) { + /* convert S16 to S24 to avoid discarding too + many bits of precision in this stage */ + format = _format; + convert = true; + return SampleFormat::S24_P32; + } + + break; + case SampleFormat::S24_P32: case SampleFormat::S32: case SampleFormat::FLOAT: @@ -124,10 +191,17 @@ PcmVolume::Open(SampleFormat _format) ConstBuffer PcmVolume::Apply(ConstBuffer src) noexcept { - if (volume == PCM_VOLUME_1) + if (volume == PCM_VOLUME_1 && !convert) return src; size_t dest_size = src.size; + if (convert) { + assert(format == SampleFormat::S16); + + /* converting to S24_P32 */ + dest_size *= 2; + } + void *data = buffer.Get(dest_size); if (volume == 0) { @@ -149,10 +223,16 @@ PcmVolume::Apply(ConstBuffer src) noexcept break; case SampleFormat::S16: - pcm_volume_change_16(dither, (int16_t *)data, - (const int16_t *)src.data, - src.size / sizeof(int16_t), - volume); + if (convert) + PcmVolumeChange16to32((int32_t *)data, + (const int16_t *)src.data, + src.size / sizeof(int16_t), + volume); + else + pcm_volume_change_16(dither, (int16_t *)data, + (const int16_t *)src.data, + src.size / sizeof(int16_t), + volume); break; case SampleFormat::S24_P32: diff --git a/src/pcm/Volume.hxx b/src/pcm/Volume.hxx index 52f170ead..50622512e 100644 --- a/src/pcm/Volume.hxx +++ b/src/pcm/Volume.hxx @@ -63,6 +63,12 @@ pcm_volume_to_float(int volume) noexcept class PcmVolume { SampleFormat format; + /** + * Are we currently converting to a different #SampleFormat? + * This is set by Open(). + */ + bool convert; + unsigned volume; PcmBuffer buffer; @@ -95,9 +101,11 @@ public: * Throws on error. * * @param format the input sample format + * @param allow_convert allow the class to convert to a + * different #SampleFormat to preserve quality? * @return the output sample format */ - SampleFormat Open(SampleFormat format); + SampleFormat Open(SampleFormat format, bool allow_convert); /** * Closes the object. After that, you may call Open() again. diff --git a/test/software_volume.cxx b/test/software_volume.cxx index 17b9b7167..ad44b4069 100644 --- a/test/software_volume.cxx +++ b/test/software_volume.cxx @@ -50,7 +50,7 @@ try { audio_format = ParseAudioFormat(argv[1], false); PcmVolume pv; - const auto out_sample_format = pv.Open(audio_format.format); + const auto out_sample_format = pv.Open(audio_format.format, false); if (out_sample_format != audio_format.format) fprintf(stderr, "Converting to %s\n", diff --git a/test/test_pcm_volume.cxx b/test/test_pcm_volume.cxx index 536fee9e3..35ad845de 100644 --- a/test/test_pcm_volume.cxx +++ b/test/test_pcm_volume.cxx @@ -36,7 +36,7 @@ TestVolume(G g=G()) typedef typename Traits::value_type value_type; PcmVolume pv; - EXPECT_EQ(pv.Open(F), F); + EXPECT_EQ(pv.Open(F, false), F); constexpr size_t N = 509; static value_type zero[N]; @@ -77,6 +77,47 @@ TEST(PcmTest, Volume16) TestVolume(); } +TEST(PcmTest, Volume16to32) +{ + constexpr SampleFormat F = SampleFormat::S16; + typedef int16_t value_type; + RandomInt g; + + PcmVolume pv; + EXPECT_EQ(pv.Open(F, true), SampleFormat::S24_P32); + + constexpr size_t N = 509; + static value_type zero[N]; + const auto _src = TestDataBuffer(g); + const ConstBuffer src(_src, sizeof(_src)); + + pv.SetVolume(0); + auto dest = pv.Apply(src); + EXPECT_EQ(src.size * 2, dest.size); + EXPECT_EQ(0, memcmp(dest.data, zero, sizeof(zero))); + + pv.SetVolume(PCM_VOLUME_1); + dest = pv.Apply(src); + EXPECT_EQ(src.size * 2, dest.size); + auto s = ConstBuffer::FromVoid(src); + auto d = ConstBuffer::FromVoid(dest); + for (size_t i = 0; i < N; ++i) + EXPECT_EQ(d[i], s[i] << 8); + + pv.SetVolume(PCM_VOLUME_1 / 2); + dest = pv.Apply(src); + EXPECT_EQ(src.size * 2, dest.size); + + s = ConstBuffer::FromVoid(src); + d = ConstBuffer::FromVoid(dest); + for (unsigned i = 0; i < N; ++i) { + const int32_t expected = (s[i] << 8) / 2; + EXPECT_EQ(d[i], expected); + } + + pv.Close(); +} + TEST(PcmTest, Volume24) { TestVolume(RandomInt24()); @@ -90,7 +131,7 @@ TEST(PcmTest, Volume32) TEST(PcmTest, VolumeFloat) { PcmVolume pv; - pv.Open(SampleFormat::FLOAT); + pv.Open(SampleFormat::FLOAT, false); constexpr size_t N = 509; static float zero[N];