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];