From cde8002df4a86a2a6efd4aa0b5c174b2f39f7bf7 Mon Sep 17 00:00:00 2001 From: jacqueline Date: Thu, 22 Jun 2023 09:40:46 +1000 Subject: [PATCH] Fix (i think?) mysterious overly large reads in libmad --- src/audio/audio_task.cpp | 10 ++++---- src/audio/fatfs_audio_input.cpp | 20 +++++++++++++-- src/codecs/include/mad.hpp | 2 +- src/codecs/mad.cpp | 44 +++++++++++++++++++++------------ 4 files changed, 52 insertions(+), 24 deletions(-) diff --git a/src/audio/audio_task.cpp b/src/audio/audio_task.cpp index d4c1d27a..24bc7be7 100644 --- a/src/audio/audio_task.cpp +++ b/src/audio/audio_task.cpp @@ -173,11 +173,11 @@ void AudioTaskMain(std::unique_ptr pipeline, IAudioSink* sink) { float samples_sunk = bytes_sunk; samples_sunk /= pcm.channels; - int8_t bps = pcm.bits_per_sample; - if (bps == 24) { - bps = 32; - } - samples_sunk /= (bps / 8); + // Samples must be aligned to 16 bits. The number of actual bytes per + // sample is therefore the bps divided by 16, rounded up (align to word), + // times two (convert to bytes). + uint8_t bytes_per_sample = ((pcm.bits_per_sample + 16 - 1) / 16) * 2; + samples_sunk /= bytes_per_sample; current_sample_in_second += samples_sunk; while (current_sample_in_second >= pcm.sample_rate) { diff --git a/src/audio/fatfs_audio_input.cpp b/src/audio/fatfs_audio_input.cpp index 86b455f0..ca5b02a1 100644 --- a/src/audio/fatfs_audio_input.cpp +++ b/src/audio/fatfs_audio_input.cpp @@ -9,6 +9,7 @@ #include #include +#include #include #include #include @@ -71,8 +72,7 @@ auto FatfsAudioInput::OpenFile(const std::string& path) -> bool { database::TrackTags tags; if (!tag_parser.ReadAndParseTags(path, &tags)) { ESP_LOGE(kTag, "failed to read tags"); - tags.encoding = database::Encoding::kFlac; - // return false; + return false; } auto stream_type = ContainerToStreamType(tags.encoding); @@ -115,6 +115,8 @@ auto FatfsAudioInput::NeedsToProcess() const -> bool { auto FatfsAudioInput::Process(const std::vector& inputs, OutputStream* output) -> void { + // If the next path is being given to us asynchronously, then we need to check + // in regularly to see if it's available yet. if (pending_path_) { if (!pending_path_->valid()) { pending_path_ = {}; @@ -133,11 +135,15 @@ auto FatfsAudioInput::Process(const std::vector& inputs, return; } + // If the output buffer isn't ready for a new stream, then we need to wait. if (!has_prepared_output_ && !output->prepare(*current_format_)) { return; } has_prepared_output_ = true; + // Performing many small reads is inefficient; it's better to do fewer, larger + // reads. Try to achieve this by only reading in new bytes if the output + // buffer has been mostly drained. std::size_t max_size = output->data().size_bytes(); if (max_size < output->data().size_bytes() / 2) { return; @@ -148,6 +154,7 @@ auto FatfsAudioInput::Process(const std::vector& inputs, f_read(¤t_file_, output->data().data(), max_size, &size); if (result != FR_OK) { ESP_LOGE(kTag, "file I/O error %d", result); + output->mark_producer_finished(); // TODO(jacqueline): Handle errors. return; } @@ -155,6 +162,15 @@ auto FatfsAudioInput::Process(const std::vector& inputs, output->add(size); if (size < max_size || f_eof(¤t_file_)) { + // HACK: In order to decode the last frame of a file, libmad requires 8 + // 0-bytes ( == MAD_GUARD_BYTES) to be appended to the end of the stream. + // It would be better to do this within mad.cpp, but so far it's the only + // decoder that has such a requirement. + if (current_container_ == database::Encoding::kMp3) { + std::fill_n(output->data().begin(), 8, std::byte(0)); + output->add(8); + } + f_close(¤t_file_); is_file_open_ = false; has_prepared_output_ = false; diff --git a/src/codecs/include/mad.hpp b/src/codecs/include/mad.hpp index e1c479bf..d2643230 100644 --- a/src/codecs/include/mad.hpp +++ b/src/codecs/include/mad.hpp @@ -48,7 +48,7 @@ class MadMp3Decoder : public ICodec { int current_sample_; - auto GetInputPosition() -> std::size_t; + auto GetBytesUsed(std::size_t) -> std::size_t; }; } // namespace codecs diff --git a/src/codecs/mad.cpp b/src/codecs/mad.cpp index f3f3cffe..23b4ccf6 100644 --- a/src/codecs/mad.cpp +++ b/src/codecs/mad.cpp @@ -13,6 +13,7 @@ #include "mad.h" #include "codec.hpp" +#include "esp_log.h" #include "result.hpp" #include "types.hpp" @@ -43,9 +44,13 @@ MadMp3Decoder::~MadMp3Decoder() { mad_synth_finish(&synth_); } -auto MadMp3Decoder::GetInputPosition() -> std::size_t { - assert(stream_.next_frame >= stream_.buffer); - return stream_.next_frame - stream_.buffer; +auto MadMp3Decoder::GetBytesUsed(std::size_t buffer_size) -> std::size_t { + if (stream_.next_frame) { + std::size_t remaining = stream_.bufend - stream_.next_frame; + return buffer_size - remaining; + } else { + return stream_.bufend - stream_.buffer; + } } auto MadMp3Decoder::BeginStream(const cpp::span input) @@ -68,13 +73,13 @@ auto MadMp3Decoder::BeginStream(const cpp::span input) continue; } if (stream_.error == MAD_ERROR_BUFLEN) { - return {GetInputPosition(), cpp::fail(Error::kOutOfInput)}; + return {GetBytesUsed(input.size_bytes()), cpp::fail(Error::kOutOfInput)}; } - return {GetInputPosition(), cpp::fail(Error::kMalformedData)}; + return {GetBytesUsed(input.size_bytes()), cpp::fail(Error::kMalformedData)}; } uint8_t channels = MAD_NCHANNELS(&header); - return {GetInputPosition(), + return {GetBytesUsed(input.size_bytes()), OutputFormat{ .num_channels = channels, .bits_per_sample = 24, // We always scale to 24 bits @@ -85,6 +90,7 @@ auto MadMp3Decoder::BeginStream(const cpp::span input) auto MadMp3Decoder::ContinueStream(cpp::span input, cpp::span output) -> Result { + std::size_t bytes_read = 0; if (current_sample_ < 0) { mad_stream_buffer(&stream_, reinterpret_cast(input.data()), @@ -101,15 +107,18 @@ auto MadMp3Decoder::ContinueStream(cpp::span input, if (stream_.error == MAD_ERROR_BUFLEN) { // The decoder ran out of bytes before it completed a frame. We // need to return back to the caller to give us more data. - return {GetInputPosition(), cpp::fail(Error::kOutOfInput)}; + return {GetBytesUsed(input.size_bytes()), + cpp::fail(Error::kOutOfInput)}; } // The error is unrecoverable. Give up. - return {GetInputPosition(), cpp::fail(Error::kMalformedData)}; + return {GetBytesUsed(input.size_bytes()), + cpp::fail(Error::kMalformedData)}; } // We've successfully decoded a frame! Now synthesize samples to write out. mad_synth_frame(&synth_, &frame_); current_sample_ = 0; + bytes_read = GetBytesUsed(input.size_bytes()); } size_t output_byte = 0; @@ -117,8 +126,8 @@ auto MadMp3Decoder::ContinueStream(cpp::span input, if (output_byte + (4 * synth_.pcm.channels) >= output.size()) { // We can't fit the next sample into the buffer. Stop now, and also avoid // writing the sample for only half the channels. - return {GetInputPosition(), OutputInfo{.bytes_written = output_byte, - .is_finished_writing = false}}; + return {bytes_read, OutputInfo{.bytes_written = output_byte, + .is_finished_writing = false}}; } for (int channel = 0; channel < synth_.pcm.channels; channel++) { @@ -135,8 +144,8 @@ auto MadMp3Decoder::ContinueStream(cpp::span input, // We wrote everything! Reset, ready for the next frame. current_sample_ = -1; - return {GetInputPosition(), OutputInfo{.bytes_written = output_byte, - .is_finished_writing = true}}; + return {bytes_read, OutputInfo{.bytes_written = output_byte, + .is_finished_writing = true}}; } auto MadMp3Decoder::SeekStream(cpp::span input, @@ -160,7 +169,8 @@ auto MadMp3Decoder::SeekStream(cpp::span input, } else { // Don't bother checking for other errors; if the first part of the // stream doesn't even contain a header then something's gone wrong. - return {GetInputPosition(), cpp::fail(Error::kMalformedData)}; + return {GetBytesUsed(input.size_bytes()), + cpp::fail(Error::kMalformedData)}; } } @@ -184,10 +194,12 @@ auto MadMp3Decoder::SeekStream(cpp::span input, continue; } if (stream_.error == MAD_ERROR_BUFLEN) { - return {GetInputPosition(), cpp::fail(Error::kOutOfInput)}; + return {GetBytesUsed(input.size_bytes()), + cpp::fail(Error::kOutOfInput)}; } // The error is unrecoverable. Give up. - return {GetInputPosition(), cpp::fail(Error::kMalformedData)}; + return {GetBytesUsed(input.size_bytes()), + cpp::fail(Error::kMalformedData)}; } if (frames_to_go <= 1) { @@ -202,7 +214,7 @@ auto MadMp3Decoder::SeekStream(cpp::span input, // call. current_sample_ = (target_sample > current_sample) ? target_sample - current_sample : 0; - return {GetInputPosition(), {}}; + return {GetBytesUsed(input.size_bytes()), {}}; } } }