From c57be0f47496d2bb23da25224833f9080f9cbcc4 Mon Sep 17 00:00:00 2001 From: Tim Schumacher Date: Mon, 5 Dec 2022 00:41:23 +0100 Subject: [PATCH] LibAudio: Switch LoaderPlugin to a more traditional constructor pattern This now prepares all the needed (fallible) components before actually constructing a LoaderPlugin object, so we are no longer filling them in at an arbitrary later point in time. --- Meta/Lagom/Fuzzers/FuzzFlacLoader.cpp | 8 ++- Meta/Lagom/Fuzzers/FuzzMP3Loader.cpp | 8 ++- Meta/Lagom/Fuzzers/FuzzWAVLoader.cpp | 7 ++- Tests/LibAudio/TestFLACSpec.cpp | 8 ++- Userland/Libraries/LibAudio/FlacLoader.cpp | 25 ++++++-- Userland/Libraries/LibAudio/FlacLoader.h | 7 ++- Userland/Libraries/LibAudio/Loader.cpp | 73 +++++++++++----------- Userland/Libraries/LibAudio/Loader.h | 8 +-- Userland/Libraries/LibAudio/MP3Loader.cpp | 25 ++++++-- Userland/Libraries/LibAudio/MP3Loader.h | 8 ++- Userland/Libraries/LibAudio/WavLoader.cpp | 32 +++++++--- Userland/Libraries/LibAudio/WavLoader.h | 9 +-- 12 files changed, 132 insertions(+), 86 deletions(-) diff --git a/Meta/Lagom/Fuzzers/FuzzFlacLoader.cpp b/Meta/Lagom/Fuzzers/FuzzFlacLoader.cpp index 5056f0899e..2f393e0c3b 100644 --- a/Meta/Lagom/Fuzzers/FuzzFlacLoader.cpp +++ b/Meta/Lagom/Fuzzers/FuzzFlacLoader.cpp @@ -11,10 +11,12 @@ extern "C" int LLVMFuzzerTestOneInput(uint8_t const* data, size_t size) { auto flac_data = ByteBuffer::copy(data, size).release_value(); - auto flac = make(flac_data.bytes()); + auto flac_or_error = Audio::FlacLoaderPlugin::try_create(flac_data.bytes()); - if (flac->initialize().is_error()) - return 1; + if (flac_or_error.is_error()) + return 0; + + auto flac = flac_or_error.release_value(); for (;;) { auto samples = flac->get_more_samples(); diff --git a/Meta/Lagom/Fuzzers/FuzzMP3Loader.cpp b/Meta/Lagom/Fuzzers/FuzzMP3Loader.cpp index 4d1b751e57..a3f9ba90cb 100644 --- a/Meta/Lagom/Fuzzers/FuzzMP3Loader.cpp +++ b/Meta/Lagom/Fuzzers/FuzzMP3Loader.cpp @@ -11,10 +11,12 @@ extern "C" int LLVMFuzzerTestOneInput(uint8_t const* data, size_t size) { auto flac_data = ByteBuffer::copy(data, size).release_value(); - auto mp3 = make(flac_data.bytes()); + auto mp3_or_error = Audio::MP3LoaderPlugin::try_create(flac_data.bytes()); - if (mp3->initialize().is_error()) - return 1; + if (mp3_or_error.is_error()) + return 0; + + auto mp3 = mp3_or_error.release_value(); for (;;) { auto samples = mp3->get_more_samples(); diff --git a/Meta/Lagom/Fuzzers/FuzzWAVLoader.cpp b/Meta/Lagom/Fuzzers/FuzzWAVLoader.cpp index e668ff317b..8e589b21b7 100644 --- a/Meta/Lagom/Fuzzers/FuzzWAVLoader.cpp +++ b/Meta/Lagom/Fuzzers/FuzzWAVLoader.cpp @@ -13,7 +13,12 @@ extern "C" int LLVMFuzzerTestOneInput(uint8_t const* data, size_t size) if (!data) return 0; auto wav_data = ReadonlyBytes { data, size }; - auto wav = make(wav_data); + auto wav_or_error = Audio::WavLoaderPlugin::try_create(wav_data); + + if (wav_or_error.is_error()) + return 0; + + auto wav = wav_or_error.release_value(); for (;;) { auto samples = wav->get_more_samples(); diff --git a/Tests/LibAudio/TestFLACSpec.cpp b/Tests/LibAudio/TestFLACSpec.cpp index 23a040d1e1..487d3d9aa4 100644 --- a/Tests/LibAudio/TestFLACSpec.cpp +++ b/Tests/LibAudio/TestFLACSpec.cpp @@ -21,14 +21,16 @@ struct FlacTest : Test::TestCase { void run() const { - auto loader = Audio::FlacLoaderPlugin { m_path.string() }; - if (auto result = loader.initialize(); result.is_error()) { + auto result = Audio::FlacLoaderPlugin::try_create(m_path.string()); + if (result.is_error()) { FAIL(String::formatted("{} (at {})", result.error().description, result.error().index)); return; } + auto loader = result.release_value(); + while (true) { - auto maybe_samples = loader.get_more_samples(2 * MiB); + auto maybe_samples = loader->get_more_samples(2 * MiB); if (maybe_samples.is_error()) { FAIL(String::formatted("{} (at {})", maybe_samples.error().description, maybe_samples.error().index)); return; diff --git a/Userland/Libraries/LibAudio/FlacLoader.cpp b/Userland/Libraries/LibAudio/FlacLoader.cpp index 580c5d8fce..6c6d7f464b 100644 --- a/Userland/Libraries/LibAudio/FlacLoader.cpp +++ b/Userland/Libraries/LibAudio/FlacLoader.cpp @@ -25,20 +25,33 @@ namespace Audio { -FlacLoaderPlugin::FlacLoaderPlugin(StringView path) - : LoaderPlugin(path) +FlacLoaderPlugin::FlacLoaderPlugin(OwnPtr stream) + : LoaderPlugin(move(stream)) { } -FlacLoaderPlugin::FlacLoaderPlugin(Bytes buffer) - : LoaderPlugin(buffer) +Result, LoaderError> FlacLoaderPlugin::try_create(StringView path) { + auto stream = LOADER_TRY(Core::Stream::BufferedFile::create(LOADER_TRY(Core::Stream::File::open(path, Core::Stream::OpenMode::Read)))); + auto loader = make(move(stream)); + + LOADER_TRY(loader->initialize()); + + return loader; +} + +Result, LoaderError> FlacLoaderPlugin::try_create(Bytes buffer) +{ + auto stream = LOADER_TRY(Core::Stream::MemoryStream::construct(buffer)); + auto loader = make(move(stream)); + + LOADER_TRY(loader->initialize()); + + return loader; } MaybeLoaderError FlacLoaderPlugin::initialize() { - LOADER_TRY(LoaderPlugin::initialize()); - TRY(parse_header()); TRY(reset()); return {}; diff --git a/Userland/Libraries/LibAudio/FlacLoader.h b/Userland/Libraries/LibAudio/FlacLoader.h index 6e3a6dd585..4271b43f29 100644 --- a/Userland/Libraries/LibAudio/FlacLoader.h +++ b/Userland/Libraries/LibAudio/FlacLoader.h @@ -47,11 +47,11 @@ ALWAYS_INLINE ErrorOr decode_unsigned_exp_golomb(u8 order, BigEndianInputBi // https://datatracker.ietf.org/doc/html/draft-ietf-cellar-flac-03 (newer IETF draft that uses incompatible numberings and names) class FlacLoaderPlugin : public LoaderPlugin { public: - explicit FlacLoaderPlugin(StringView path); - explicit FlacLoaderPlugin(Bytes buffer); + explicit FlacLoaderPlugin(OwnPtr stream); virtual ~FlacLoaderPlugin() override = default; - virtual MaybeLoaderError initialize() override; + static Result, LoaderError> try_create(StringView path); + static Result, LoaderError> try_create(Bytes buffer); virtual LoaderSamples get_more_samples(size_t max_bytes_to_read_from_input = 128 * KiB) override; @@ -69,6 +69,7 @@ public: bool sample_count_unknown() const { return m_total_samples == 0; } private: + MaybeLoaderError initialize(); MaybeLoaderError parse_header(); // Either returns the metadata block or sets error message. // Additionally, increments m_data_start_location past the read meta block. diff --git a/Userland/Libraries/LibAudio/Loader.cpp b/Userland/Libraries/LibAudio/Loader.cpp index 2554378c2b..7b662326ef 100644 --- a/Userland/Libraries/LibAudio/Loader.cpp +++ b/Userland/Libraries/LibAudio/Loader.cpp @@ -11,26 +11,11 @@ namespace Audio { -LoaderPlugin::LoaderPlugin(StringView path) - : m_path(path) +LoaderPlugin::LoaderPlugin(OwnPtr stream) + : m_stream(move(stream)) { } -LoaderPlugin::LoaderPlugin(Bytes buffer) - : m_backing_memory(buffer) -{ -} - -MaybeLoaderError LoaderPlugin::initialize() -{ - if (m_backing_memory.has_value()) - m_stream = LOADER_TRY(Core::Stream::MemoryStream::construct(m_backing_memory.value())); - else - m_stream = LOADER_TRY(Core::Stream::BufferedFile::create(LOADER_TRY(Core::Stream::File::open(m_path, Core::Stream::OpenMode::Read)))); - - return {}; -} - Loader::Loader(NonnullOwnPtr plugin) : m_plugin(move(plugin)) { @@ -38,35 +23,47 @@ Loader::Loader(NonnullOwnPtr plugin) Result, LoaderError> Loader::try_create(StringView path) { - NonnullOwnPtr plugin = adopt_own(*new WavLoaderPlugin(path)); - auto initstate0 = plugin->initialize(); - if (!initstate0.is_error()) - return plugin; + { + auto plugin = WavLoaderPlugin::try_create(path); + if (!plugin.is_error()) + return NonnullOwnPtr(plugin.release_value()); + } - plugin = adopt_own(*new FlacLoaderPlugin(path)); - auto initstate1 = plugin->initialize(); - if (!initstate1.is_error()) - return plugin; + { + auto plugin = FlacLoaderPlugin::try_create(path); + if (!plugin.is_error()) + return NonnullOwnPtr(plugin.release_value()); + } - plugin = adopt_own(*new MP3LoaderPlugin(path)); - auto initstate2 = plugin->initialize(); - if (!initstate2.is_error()) - return plugin; + { + auto plugin = MP3LoaderPlugin::try_create(path); + if (!plugin.is_error()) + return NonnullOwnPtr(plugin.release_value()); + } return LoaderError { "No loader plugin available" }; } Result, LoaderError> Loader::try_create(Bytes buffer) { - NonnullOwnPtr plugin = adopt_own(*new WavLoaderPlugin(buffer)); - if (auto initstate = plugin->initialize(); !initstate.is_error()) - return plugin; - plugin = adopt_own(*new FlacLoaderPlugin(buffer)); - if (auto initstate = plugin->initialize(); !initstate.is_error()) - return plugin; - plugin = adopt_own(*new MP3LoaderPlugin(buffer)); - if (auto initstate = plugin->initialize(); !initstate.is_error()) - return plugin; + { + auto plugin = WavLoaderPlugin::try_create(buffer); + if (!plugin.is_error()) + return NonnullOwnPtr(plugin.release_value()); + } + + { + auto plugin = FlacLoaderPlugin::try_create(buffer); + if (!plugin.is_error()) + return NonnullOwnPtr(plugin.release_value()); + } + + { + auto plugin = MP3LoaderPlugin::try_create(buffer); + if (!plugin.is_error()) + return NonnullOwnPtr(plugin.release_value()); + } + return LoaderError { "No loader plugin available" }; } diff --git a/Userland/Libraries/LibAudio/Loader.h b/Userland/Libraries/LibAudio/Loader.h index 2afc078573..9a5d381eb3 100644 --- a/Userland/Libraries/LibAudio/Loader.h +++ b/Userland/Libraries/LibAudio/Loader.h @@ -30,12 +30,9 @@ using MaybeLoaderError = Result; class LoaderPlugin { public: - explicit LoaderPlugin(StringView path); - explicit LoaderPlugin(Bytes buffer); + explicit LoaderPlugin(OwnPtr stream); virtual ~LoaderPlugin() = default; - virtual MaybeLoaderError initialize() = 0; - virtual LoaderSamples get_more_samples(size_t max_bytes_to_read_from_input = 128 * KiB) = 0; virtual MaybeLoaderError reset() = 0; @@ -61,10 +58,7 @@ public: Vector const& pictures() const { return m_pictures; }; protected: - StringView m_path; OwnPtr m_stream; - // The constructor might set this so that we can initialize the data stream later. - Optional m_backing_memory; Vector m_pictures; }; diff --git a/Userland/Libraries/LibAudio/MP3Loader.cpp b/Userland/Libraries/LibAudio/MP3Loader.cpp index 61340c00a7..3cbb014f27 100644 --- a/Userland/Libraries/LibAudio/MP3Loader.cpp +++ b/Userland/Libraries/LibAudio/MP3Loader.cpp @@ -14,20 +14,33 @@ namespace Audio { DSP::MDCT<12> MP3LoaderPlugin::s_mdct_12; DSP::MDCT<36> MP3LoaderPlugin::s_mdct_36; -MP3LoaderPlugin::MP3LoaderPlugin(StringView path) - : LoaderPlugin(path) +MP3LoaderPlugin::MP3LoaderPlugin(OwnPtr stream) + : LoaderPlugin(move(stream)) { } -MP3LoaderPlugin::MP3LoaderPlugin(Bytes buffer) - : LoaderPlugin(buffer) +Result, LoaderError> MP3LoaderPlugin::try_create(StringView path) { + auto stream = LOADER_TRY(Core::Stream::BufferedFile::create(LOADER_TRY(Core::Stream::File::open(path, Core::Stream::OpenMode::Read)))); + auto loader = make(move(stream)); + + LOADER_TRY(loader->initialize()); + + return loader; +} + +Result, LoaderError> MP3LoaderPlugin::try_create(Bytes buffer) +{ + auto stream = LOADER_TRY(Core::Stream::MemoryStream::construct(buffer)); + auto loader = make(move(stream)); + + LOADER_TRY(loader->initialize()); + + return loader; } MaybeLoaderError MP3LoaderPlugin::initialize() { - LOADER_TRY(LoaderPlugin::initialize()); - m_bitstream = LOADER_TRY(Core::Stream::BigEndianInputBitStream::construct(*m_stream)); TRY(synchronize()); diff --git a/Userland/Libraries/LibAudio/MP3Loader.h b/Userland/Libraries/LibAudio/MP3Loader.h index a0ef5ea8eb..1bf5cbf3b3 100644 --- a/Userland/Libraries/LibAudio/MP3Loader.h +++ b/Userland/Libraries/LibAudio/MP3Loader.h @@ -23,11 +23,12 @@ struct ScaleFactorBand; class MP3LoaderPlugin : public LoaderPlugin { public: - explicit MP3LoaderPlugin(StringView path); - explicit MP3LoaderPlugin(Bytes buffer); + explicit MP3LoaderPlugin(OwnPtr stream); virtual ~MP3LoaderPlugin() = default; - virtual MaybeLoaderError initialize() override; + static Result, LoaderError> try_create(StringView path); + static Result, LoaderError> try_create(Bytes buffer); + virtual LoaderSamples get_more_samples(size_t max_bytes_to_read_from_input = 128 * KiB) override; virtual MaybeLoaderError reset() override; @@ -41,6 +42,7 @@ public: virtual String format_name() override { return "MP3 (.mp3)"; } private: + MaybeLoaderError initialize(); MaybeLoaderError synchronize(); MaybeLoaderError build_seek_table(); ErrorOr read_header(); diff --git a/Userland/Libraries/LibAudio/WavLoader.cpp b/Userland/Libraries/LibAudio/WavLoader.cpp index 62d0d06753..4018fe9122 100644 --- a/Userland/Libraries/LibAudio/WavLoader.cpp +++ b/Userland/Libraries/LibAudio/WavLoader.cpp @@ -18,24 +18,38 @@ namespace Audio { static constexpr size_t const maximum_wav_size = 1 * GiB; // FIXME: is there a more appropriate size limit? -WavLoaderPlugin::WavLoaderPlugin(StringView path) - : LoaderPlugin(path) +WavLoaderPlugin::WavLoaderPlugin(OwnPtr stream) + : LoaderPlugin(move(stream)) { } +Result, LoaderError> WavLoaderPlugin::try_create(StringView path) +{ + auto stream = LOADER_TRY(Core::Stream::BufferedFile::create(LOADER_TRY(Core::Stream::File::open(path, Core::Stream::OpenMode::Read)))); + auto loader = make(move(stream)); + + LOADER_TRY(loader->initialize()); + + return loader; +} + +Result, LoaderError> WavLoaderPlugin::try_create(Bytes buffer) +{ + auto stream = LOADER_TRY(Core::Stream::MemoryStream::construct(buffer)); + auto loader = make(move(stream)); + + LOADER_TRY(loader->initialize()); + + return loader; +} + MaybeLoaderError WavLoaderPlugin::initialize() { - LOADER_TRY(LoaderPlugin::initialize()); + LOADER_TRY(parse_header()); - TRY(parse_header()); return {}; } -WavLoaderPlugin::WavLoaderPlugin(Bytes buffer) - : LoaderPlugin(buffer) -{ -} - template MaybeLoaderError WavLoaderPlugin::read_samples_from_stream(Core::Stream::Stream& stream, SampleReader read_sample, FixedArray& samples) const { diff --git a/Userland/Libraries/LibAudio/WavLoader.h b/Userland/Libraries/LibAudio/WavLoader.h index 437f7eb54f..6eb2d936e7 100644 --- a/Userland/Libraries/LibAudio/WavLoader.h +++ b/Userland/Libraries/LibAudio/WavLoader.h @@ -29,10 +29,9 @@ static constexpr unsigned const WAVE_FORMAT_EXTENSIBLE = 0xFFFE; // Determined b // Parses and reads audio data from a WAV file. class WavLoaderPlugin : public LoaderPlugin { public: - explicit WavLoaderPlugin(StringView path); - explicit WavLoaderPlugin(Bytes buffer); - - virtual MaybeLoaderError initialize() override; + explicit WavLoaderPlugin(OwnPtr stream); + static Result, LoaderError> try_create(StringView path); + static Result, LoaderError> try_create(Bytes buffer); virtual LoaderSamples get_more_samples(size_t max_samples_to_read_from_input = 128 * KiB) override; @@ -50,6 +49,8 @@ public: virtual PcmSampleFormat pcm_format() override { return m_sample_format; } private: + MaybeLoaderError initialize(); + MaybeLoaderError parse_header(); LoaderSamples samples_from_pcm_data(Bytes const& data, size_t samples_to_read) const;