From ed5777eb0a1c7598ac4cf0f39580d4861d8c1b62 Mon Sep 17 00:00:00 2001 From: Nick Miller Date: Sat, 5 Jun 2021 10:14:03 -0700 Subject: [PATCH] LibAudio: WavLoader: Avoid reading partial samples When samples are requested in `Audio::Loader::get_more_samples`, the request comes in as a max number of bytes to read. However, the requested number of bytes may not be an even multiple of the bytes per sample of the loaded file. If this is the case, and the bytes are read from the file/stream, then the last sample will be a partial/runt sample, which then offsets the remainder of the stream, causing white noise in playback. This bug was discovered when trying to play 24-bit Wave files, which happened to have a sample size that never aligned with the number of requested bytes. This commit fixes the bug by only reading a multiple of "bytes per sample" for the loaded file. --- Userland/Libraries/LibAudio/WavLoader.cpp | 77 ++++++++++++++--------- Userland/Libraries/LibAudio/WavLoader.h | 6 +- 2 files changed, 51 insertions(+), 32 deletions(-) diff --git a/Userland/Libraries/LibAudio/WavLoader.cpp b/Userland/Libraries/LibAudio/WavLoader.cpp index 893eedb386..c65a39c536 100644 --- a/Userland/Libraries/LibAudio/WavLoader.cpp +++ b/Userland/Libraries/LibAudio/WavLoader.cpp @@ -24,6 +24,7 @@ WavLoaderPlugin::WavLoaderPlugin(const StringView& path) m_error_string = String::formatted("Can't open file: {}", m_file->error_string()); return; } + m_stream = make(*m_file); valid = parse_header(); if (!valid) @@ -39,6 +40,7 @@ WavLoaderPlugin::WavLoaderPlugin(const ByteBuffer& buffer) m_error_string = String::formatted("Can't open memory stream"); return; } + m_memory_stream = static_cast(m_stream.ptr()); valid = parse_header(); if (!valid) @@ -49,22 +51,33 @@ WavLoaderPlugin::WavLoaderPlugin(const ByteBuffer& buffer) RefPtr WavLoaderPlugin::get_more_samples(size_t max_bytes_to_read_from_input) { - dbgln_if(AWAVLOADER_DEBUG, "Read {} bytes WAV with num_channels {} sample rate {}, " + if (!m_stream) + return nullptr; + + size_t bytes_per_sample = (m_num_channels * (pcm_bits_per_sample(m_sample_format) / 8)); + + // Might truncate if not evenly divisible + size_t samples_to_read = static_cast(max_bytes_to_read_from_input) / bytes_per_sample; + size_t bytes_to_read = samples_to_read * bytes_per_sample; + + dbgln_if(AWAVLOADER_DEBUG, "Read {} bytes ({} samples) WAV with num_channels {} sample rate {}, " "bits per sample {}, sample format {}", - max_bytes_to_read_from_input, m_num_channels, - m_sample_rate, pcm_bits_per_sample(m_sample_format), sample_format_name(m_sample_format)); - size_t samples_to_read = static_cast(max_bytes_to_read_from_input) / (m_num_channels * (pcm_bits_per_sample(m_sample_format) / 8)); - RefPtr buffer; - if (m_file) { - auto raw_samples = m_file->read(max_bytes_to_read_from_input); - if (raw_samples.is_empty()) { - return nullptr; - } - buffer = Buffer::from_pcm_data(raw_samples, *m_resampler, m_num_channels, m_sample_format); - } else { - buffer = Buffer::from_pcm_stream(*m_stream, *m_resampler, m_num_channels, m_sample_format, samples_to_read); + bytes_to_read, samples_to_read, m_num_channels, m_sample_rate, + pcm_bits_per_sample(m_sample_format), sample_format_name(m_sample_format)); + + ByteBuffer sample_data = ByteBuffer::create_zeroed(bytes_to_read); + m_stream->read_or_error(sample_data.bytes()); + if (m_stream->handle_any_error()) { + return nullptr; } - //Buffer contains normalized samples, but m_loaded_samples should contain the amount of actually loaded samples + + RefPtr buffer = Buffer::from_pcm_data( + sample_data.bytes(), + *m_resampler, + m_num_channels, + m_sample_format); + + // m_loaded_samples should contain the amount of actually loaded samples m_loaded_samples += samples_to_read; m_loaded_samples = min(m_total_samples, m_loaded_samples); return buffer; @@ -78,45 +91,41 @@ void WavLoaderPlugin::seek(const int position) m_loaded_samples = position; size_t byte_position = position * m_num_channels * (pcm_bits_per_sample(m_sample_format) / 8); - if (m_file) + // AK::InputStream does not define seek. + if (m_file) { m_file->seek(byte_position); - else - m_stream->seek(byte_position); + } else { + m_memory_stream->seek(byte_position); + } } bool WavLoaderPlugin::parse_header() { - OwnPtr file_stream; - if (m_file) - file_stream = make(*m_file); - - AK::InputStream* const stream = - (m_file ? - file_stream.ptr() : - dynamic_cast(m_stream.ptr())); + if (!m_stream) + return false; bool ok = true; auto read_u8 = [&]() -> u8 { u8 value; - *stream >> value; - if (stream->handle_any_error()) + *m_stream >> value; + if (m_stream->handle_any_error()) ok = false; return value; }; auto read_u16 = [&]() -> u16 { u16 value; - *stream >> value; - if (stream->handle_any_error()) + *m_stream >> value; + if (m_stream->handle_any_error()) ok = false; return value; }; auto read_u32 = [&]() -> u32 { u32 value; - *stream >> value; - if (stream->handle_any_error()) + *m_stream >> value; + if (m_stream->handle_any_error()) ok = false; return value; }; @@ -125,6 +134,7 @@ bool WavLoaderPlugin::parse_header() do { \ if (!ok) { \ m_error_string = String::formatted("Parsing failed: {}", msg); \ + dbgln_if(AWAVLOADER_DEBUG, m_error_string); \ return {}; \ } \ } while (0) @@ -230,6 +240,11 @@ bool WavLoaderPlugin::parse_header() int bytes_per_sample = (bits_per_sample / 8) * m_num_channels; m_total_samples = data_sz / bytes_per_sample; + dbgln_if(AWAVLOADER_DEBUG, "WAV data size {}, bytes per sample {}, total samples {}", + data_sz, + bytes_per_sample, + m_total_samples); + return true; } diff --git a/Userland/Libraries/LibAudio/WavLoader.h b/Userland/Libraries/LibAudio/WavLoader.h index 6807e4429d..9a768fdea2 100644 --- a/Userland/Libraries/LibAudio/WavLoader.h +++ b/Userland/Libraries/LibAudio/WavLoader.h @@ -11,11 +11,14 @@ #include #include #include +#include #include #include +#include #include #include #include +#include namespace Audio { class Buffer; @@ -55,7 +58,8 @@ private: bool valid { false }; RefPtr m_file; - OwnPtr m_stream; + OwnPtr m_stream; + AK::InputMemoryStream* m_memory_stream; String m_error_string; OwnPtr m_resampler;