From 5f1dbbaaa6dae7343faa9b9d486e397be3794fd5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?kleines=20Filmr=C3=B6llchen?= Date: Sat, 24 Jun 2023 17:04:38 +0200 Subject: [PATCH] LibAudio: Extract loader stream creation from the plugins This removes a lot of duplicated stream creation code from the plugins, and also simplifies the way that the appropriate plugin is found. This mirrors the ImageDecoderPlugin design and necessitates new sniffing methods on the loaders. --- Meta/Lagom/Fuzzers/FuzzFlacLoader.cpp | 6 +- Meta/Lagom/Fuzzers/FuzzMP3Loader.cpp | 5 +- Meta/Lagom/Fuzzers/FuzzQOALoader.cpp | 6 +- Meta/Lagom/Fuzzers/FuzzWAVLoader.cpp | 9 ++- Tests/LibAudio/TestFLACSpec.cpp | 12 +++- Userland/Libraries/LibAudio/FlacLoader.cpp | 25 +++---- Userland/Libraries/LibAudio/FlacLoader.h | 4 +- Userland/Libraries/LibAudio/Loader.cpp | 83 +++++++++------------- Userland/Libraries/LibAudio/Loader.h | 11 ++- Userland/Libraries/LibAudio/MP3Loader.cpp | 30 +++++--- Userland/Libraries/LibAudio/MP3Loader.h | 4 +- Userland/Libraries/LibAudio/QOALoader.cpp | 16 ++--- Userland/Libraries/LibAudio/QOALoader.h | 5 +- Userland/Libraries/LibAudio/WavLoader.cpp | 32 ++++----- Userland/Libraries/LibAudio/WavLoader.h | 7 +- 15 files changed, 124 insertions(+), 131 deletions(-) diff --git a/Meta/Lagom/Fuzzers/FuzzFlacLoader.cpp b/Meta/Lagom/Fuzzers/FuzzFlacLoader.cpp index cdf9ad1f795..271fbd9a799 100644 --- a/Meta/Lagom/Fuzzers/FuzzFlacLoader.cpp +++ b/Meta/Lagom/Fuzzers/FuzzFlacLoader.cpp @@ -4,14 +4,16 @@ * SPDX-License-Identifier: BSD-2-Clause */ +#include #include #include #include extern "C" int LLVMFuzzerTestOneInput(uint8_t const* data, size_t size) { - auto flac_data = ByteBuffer::copy(data, size).release_value(); - auto flac_or_error = Audio::FlacLoaderPlugin::create(flac_data.bytes()); + auto const flac_bytes = ByteBuffer::copy(data, size).release_value(); + auto flac_data = try_make(flac_bytes).release_value(); + auto flac_or_error = Audio::FlacLoaderPlugin::create(move(flac_data)); if (flac_or_error.is_error()) return 0; diff --git a/Meta/Lagom/Fuzzers/FuzzMP3Loader.cpp b/Meta/Lagom/Fuzzers/FuzzMP3Loader.cpp index a04d1391d0d..0edcc0b9e6b 100644 --- a/Meta/Lagom/Fuzzers/FuzzMP3Loader.cpp +++ b/Meta/Lagom/Fuzzers/FuzzMP3Loader.cpp @@ -10,8 +10,9 @@ extern "C" int LLVMFuzzerTestOneInput(uint8_t const* data, size_t size) { - auto flac_data = ByteBuffer::copy(data, size).release_value(); - auto mp3_or_error = Audio::MP3LoaderPlugin::create(flac_data.bytes()); + auto const mp3_bytes = ByteBuffer::copy(data, size).release_value(); + auto mp3_data = try_make(mp3_bytes).release_value(); + auto mp3_or_error = Audio::MP3LoaderPlugin::create(move(mp3_data)); if (mp3_or_error.is_error()) return 0; diff --git a/Meta/Lagom/Fuzzers/FuzzQOALoader.cpp b/Meta/Lagom/Fuzzers/FuzzQOALoader.cpp index a0c28cb5bb6..ef250afdc7e 100644 --- a/Meta/Lagom/Fuzzers/FuzzQOALoader.cpp +++ b/Meta/Lagom/Fuzzers/FuzzQOALoader.cpp @@ -4,14 +4,16 @@ * SPDX-License-Identifier: BSD-2-Clause */ +#include #include #include #include extern "C" int LLVMFuzzerTestOneInput(uint8_t const* data, size_t size) { - auto qoa_data = ByteBuffer::copy(data, size).release_value(); - auto qoa_or_error = Audio::QOALoaderPlugin::create(qoa_data.bytes()); + auto const qoa_bytes = ByteBuffer::copy(data, size).release_value(); + auto qoa_data = try_make(qoa_bytes).release_value(); + auto qoa_or_error = Audio::QOALoaderPlugin::create(move(qoa_data)); if (qoa_or_error.is_error()) return 0; diff --git a/Meta/Lagom/Fuzzers/FuzzWAVLoader.cpp b/Meta/Lagom/Fuzzers/FuzzWAVLoader.cpp index ce910d26900..ad892681efa 100644 --- a/Meta/Lagom/Fuzzers/FuzzWAVLoader.cpp +++ b/Meta/Lagom/Fuzzers/FuzzWAVLoader.cpp @@ -4,17 +4,16 @@ * SPDX-License-Identifier: BSD-2-Clause */ -#include +#include #include #include #include extern "C" int LLVMFuzzerTestOneInput(uint8_t const* data, size_t size) { - if (!data) - return 0; - auto wav_data = ReadonlyBytes { data, size }; - auto wav_or_error = Audio::WavLoaderPlugin::create(wav_data); + auto const wav_bytes = ByteBuffer::copy(data, size).release_value(); + auto wav_data = try_make(wav_bytes).release_value(); + auto wav_or_error = Audio::WavLoaderPlugin::create(move(wav_data)); if (wav_or_error.is_error()) return 0; diff --git a/Tests/LibAudio/TestFLACSpec.cpp b/Tests/LibAudio/TestFLACSpec.cpp index 7d3545fc47b..34c8dd06cff 100644 --- a/Tests/LibAudio/TestFLACSpec.cpp +++ b/Tests/LibAudio/TestFLACSpec.cpp @@ -19,7 +19,17 @@ struct DiscoverFLACTestsHack { Test::add_test_case_to_suite(adopt_ref(*new ::Test::TestCase( DeprecatedString::formatted("flac_spec_test_{}", path.basename()), [path = move(path)]() { - auto result = Audio::FlacLoaderPlugin::create(path.string()); + auto file = Core::File::open(path.string(), Core::File::OpenMode::Read); + if (file.is_error()) { + FAIL(DeprecatedString::formatted("{}", file.error())); + return; + } + auto buffered_file = Core::InputBufferedFile::create(file.release_value()); + if (buffered_file.is_error()) { + FAIL(DeprecatedString::formatted("{}", buffered_file.error())); + return; + } + auto result = Audio::FlacLoaderPlugin::create(buffered_file.release_value()); if (result.is_error()) { FAIL(DeprecatedString::formatted("{}", result.error())); return; diff --git a/Userland/Libraries/LibAudio/FlacLoader.cpp b/Userland/Libraries/LibAudio/FlacLoader.cpp index c5e20f656b0..1ab709791e7 100644 --- a/Userland/Libraries/LibAudio/FlacLoader.cpp +++ b/Userland/Libraries/LibAudio/FlacLoader.cpp @@ -12,6 +12,7 @@ #include #include #include +#include #include #include #include @@ -34,23 +35,10 @@ FlacLoaderPlugin::FlacLoaderPlugin(NonnullOwnPtr stream) { } -Result, LoaderError> FlacLoaderPlugin::create(StringView path) +ErrorOr, LoaderError> FlacLoaderPlugin::create(NonnullOwnPtr stream) { - auto stream = LOADER_TRY(Core::InputBufferedFile::create(LOADER_TRY(Core::File::open(path, Core::File::OpenMode::Read)))); auto loader = make(move(stream)); - - LOADER_TRY(loader->initialize()); - - return loader; -} - -Result, LoaderError> FlacLoaderPlugin::create(Bytes buffer) -{ - auto stream = LOADER_TRY(try_make(buffer)); - auto loader = make(move(stream)); - - LOADER_TRY(loader->initialize()); - + TRY(loader->initialize()); return loader; } @@ -61,6 +49,13 @@ MaybeLoaderError FlacLoaderPlugin::initialize() return {}; } +bool FlacLoaderPlugin::sniff(SeekableStream& stream) +{ + BigEndianInputBitStream bit_input { MaybeOwned(stream) }; + auto maybe_flac = bit_input.read_bits(32); + return !maybe_flac.is_error() && maybe_flac.value() == 0x664C6143; // "flaC" +} + // 11.5 STREAM MaybeLoaderError FlacLoaderPlugin::parse_header() { diff --git a/Userland/Libraries/LibAudio/FlacLoader.h b/Userland/Libraries/LibAudio/FlacLoader.h index 72e51334a75..82a7276b049 100644 --- a/Userland/Libraries/LibAudio/FlacLoader.h +++ b/Userland/Libraries/LibAudio/FlacLoader.h @@ -40,8 +40,8 @@ public: explicit FlacLoaderPlugin(NonnullOwnPtr stream); virtual ~FlacLoaderPlugin() override = default; - static Result, LoaderError> create(StringView path); - static Result, LoaderError> create(Bytes buffer); + static bool sniff(SeekableStream& stream); + static ErrorOr, LoaderError> create(NonnullOwnPtr); virtual ErrorOr>, LoaderError> load_chunks(size_t samples_to_read_from_input) override; diff --git a/Userland/Libraries/LibAudio/Loader.cpp b/Userland/Libraries/LibAudio/Loader.cpp index 5904cfb2549..1e63fee1e54 100644 --- a/Userland/Libraries/LibAudio/Loader.cpp +++ b/Userland/Libraries/LibAudio/Loader.cpp @@ -10,6 +10,7 @@ #include #include #include +#include namespace Audio { @@ -23,59 +24,43 @@ Loader::Loader(NonnullOwnPtr plugin) { } -Result, LoaderError> Loader::create_plugin(StringView path) +struct LoaderPluginInitializer { + bool (*sniff)(SeekableStream&); + ErrorOr, LoaderError> (*create)(NonnullOwnPtr); +}; + +#define ENUMERATE_LOADER_PLUGINS \ + __ENUMERATE_LOADER_PLUGIN(Wav) \ + __ENUMERATE_LOADER_PLUGIN(Flac) \ + __ENUMERATE_LOADER_PLUGIN(QOA) \ + __ENUMERATE_LOADER_PLUGIN(MP3) + +static constexpr LoaderPluginInitializer s_initializers[] = { +#define __ENUMERATE_LOADER_PLUGIN(Type) \ + { Type##LoaderPlugin::sniff, Type##LoaderPlugin::create }, + ENUMERATE_LOADER_PLUGINS +#undef __ENUMERATE_LOADER_PLUGIN +}; + +ErrorOr, LoaderError> Loader::create(StringView path) { - { - auto plugin = WavLoaderPlugin::create(path); - if (!plugin.is_error()) - return NonnullOwnPtr(plugin.release_value()); - } - - { - auto plugin = FlacLoaderPlugin::create(path); - if (!plugin.is_error()) - return NonnullOwnPtr(plugin.release_value()); - } - - { - auto plugin = MP3LoaderPlugin::create(path); - if (!plugin.is_error()) - return NonnullOwnPtr(plugin.release_value()); - } - - { - auto plugin = QOALoaderPlugin::create(path); - if (!plugin.is_error()) - return NonnullOwnPtr(plugin.release_value()); - } - - return LoaderError { "No loader plugin available" }; + auto stream = LOADER_TRY(Core::InputBufferedFile::create(LOADER_TRY(Core::File::open(path, Core::File::OpenMode::Read)))); + return adopt_ref(*new (nothrow) Loader(TRY(Loader::create_plugin(move(stream))))); +} +ErrorOr, LoaderError> Loader::create(Bytes buffer) +{ + auto stream = LOADER_TRY(try_make(buffer)); + return adopt_ref(*new (nothrow) Loader(TRY(Loader::create_plugin(move(stream))))); } -Result, LoaderError> Loader::create_plugin(Bytes buffer) +ErrorOr, LoaderError> Loader::create_plugin(NonnullOwnPtr stream) { - { - auto plugin = WavLoaderPlugin::create(buffer); - if (!plugin.is_error()) - return NonnullOwnPtr(plugin.release_value()); - } - - { - auto plugin = FlacLoaderPlugin::create(buffer); - if (!plugin.is_error()) - return NonnullOwnPtr(plugin.release_value()); - } - - { - auto plugin = MP3LoaderPlugin::create(buffer); - if (!plugin.is_error()) - return NonnullOwnPtr(plugin.release_value()); - } - - { - auto plugin = QOALoaderPlugin::create(buffer); - if (!plugin.is_error()) - return NonnullOwnPtr(plugin.release_value()); + for (auto const& loader : s_initializers) { + if (loader.sniff(*stream)) { + TRY(stream->seek(0, SeekMode::SetPosition)); + return loader.create(move(stream)); + } + TRY(stream->seek(0, SeekMode::SetPosition)); } return LoaderError { "No loader plugin available" }; diff --git a/Userland/Libraries/LibAudio/Loader.h b/Userland/Libraries/LibAudio/Loader.h index 3dde6471c11..3acde4fcc71 100644 --- a/Userland/Libraries/LibAudio/Loader.h +++ b/Userland/Libraries/LibAudio/Loader.h @@ -6,12 +6,12 @@ #pragma once +#include #include #include #include #include #include -#include #include #include #include @@ -24,8 +24,6 @@ namespace Audio { -static constexpr StringView no_plugin_error = "No loader plugin available"sv; - // Experimentally determined to be a decent buffer size on i686: // 4K (the default) is slightly worse, and 64K is much worse. // At sufficiently large buffer sizes, the advantage of infrequent read() calls is outweighed by the memmove() overhead. @@ -87,8 +85,8 @@ protected: class Loader : public RefCounted { public: - static Result, LoaderError> create(StringView path) { return adopt_ref(*new Loader(TRY(create_plugin(path)))); } - static Result, LoaderError> create(Bytes buffer) { return adopt_ref(*new Loader(TRY(create_plugin(buffer)))); } + static ErrorOr, LoaderError> create(StringView path); + static ErrorOr, LoaderError> create(Bytes buffer); // Will only read less samples if we're at the end of the stream. LoaderSamples get_more_samples(size_t samples_to_read_from_input = 128 * KiB); @@ -111,8 +109,7 @@ public: Vector const& pictures() const { return m_plugin->pictures(); }; private: - static Result, LoaderError> create_plugin(StringView path); - static Result, LoaderError> create_plugin(Bytes buffer); + static ErrorOr, LoaderError> create_plugin(NonnullOwnPtr stream); explicit Loader(NonnullOwnPtr); diff --git a/Userland/Libraries/LibAudio/MP3Loader.cpp b/Userland/Libraries/LibAudio/MP3Loader.cpp index 2867e68a5c9..5d18ece8c56 100644 --- a/Userland/Libraries/LibAudio/MP3Loader.cpp +++ b/Userland/Libraries/LibAudio/MP3Loader.cpp @@ -8,6 +8,7 @@ #include "MP3HuffmanTables.h" #include "MP3Tables.h" #include "MP3Types.h" +#include #include #include @@ -21,23 +22,34 @@ MP3LoaderPlugin::MP3LoaderPlugin(NonnullOwnPtr stream) { } -Result, LoaderError> MP3LoaderPlugin::create(StringView path) +bool MP3LoaderPlugin::sniff(SeekableStream& stream) { - auto stream = LOADER_TRY(Core::InputBufferedFile::create(LOADER_TRY(Core::File::open(path, Core::File::OpenMode::Read)))); - auto loader = make(move(stream)); + auto maybe_bit_stream = try_make(MaybeOwned(stream)); + if (maybe_bit_stream.is_error()) + return false; + auto bit_stream = maybe_bit_stream.release_value(); - LOADER_TRY(loader->initialize()); + auto synchronization_result = synchronize(*bit_stream, 0); + if (synchronization_result.is_error()) + return false; + auto maybe_mp3 = stream.read_value>(); + if (maybe_mp3.is_error()) + return false; - return loader; + ErrorOr id = bit_stream->read_bit(); + if (id.is_error() || id.value() != 1) + return false; + auto raw_layer = bit_stream->read_bits(2); + if (raw_layer.is_error()) + return false; + auto layer = MP3::Tables::LayerNumberLookup[raw_layer.value()]; + return layer == 3; } -Result, LoaderError> MP3LoaderPlugin::create(Bytes buffer) +ErrorOr, LoaderError> MP3LoaderPlugin::create(NonnullOwnPtr stream) { - auto stream = LOADER_TRY(try_make(buffer)); auto loader = make(move(stream)); - LOADER_TRY(loader->initialize()); - return loader; } diff --git a/Userland/Libraries/LibAudio/MP3Loader.h b/Userland/Libraries/LibAudio/MP3Loader.h index af01bf679e1..f6e5421f457 100644 --- a/Userland/Libraries/LibAudio/MP3Loader.h +++ b/Userland/Libraries/LibAudio/MP3Loader.h @@ -24,8 +24,8 @@ public: explicit MP3LoaderPlugin(NonnullOwnPtr stream); virtual ~MP3LoaderPlugin() = default; - static Result, LoaderError> create(StringView path); - static Result, LoaderError> create(Bytes buffer); + static bool sniff(SeekableStream& stream); + static ErrorOr, LoaderError> create(NonnullOwnPtr); virtual ErrorOr>, LoaderError> load_chunks(size_t samples_to_read_from_input) override; diff --git a/Userland/Libraries/LibAudio/QOALoader.cpp b/Userland/Libraries/LibAudio/QOALoader.cpp index bca3cb26018..7eaa12ec948 100644 --- a/Userland/Libraries/LibAudio/QOALoader.cpp +++ b/Userland/Libraries/LibAudio/QOALoader.cpp @@ -24,22 +24,16 @@ QOALoaderPlugin::QOALoaderPlugin(NonnullOwnPtr stream) { } -Result, LoaderError> QOALoaderPlugin::create(StringView path) +bool QOALoaderPlugin::sniff(SeekableStream& stream) { - auto stream = LOADER_TRY(Core::InputBufferedFile::create(LOADER_TRY(Core::File::open(path, Core::File::OpenMode::Read)))); - auto loader = make(move(stream)); - - LOADER_TRY(loader->initialize()); - - return loader; + auto maybe_qoa = stream.read_value>(); + return !maybe_qoa.is_error() && maybe_qoa.value() == QOA::magic; } -Result, LoaderError> QOALoaderPlugin::create(Bytes buffer) +ErrorOr, LoaderError> QOALoaderPlugin::create(NonnullOwnPtr stream) { - auto loader = make(make(buffer)); - + auto loader = make(move(stream)); LOADER_TRY(loader->initialize()); - return loader; } diff --git a/Userland/Libraries/LibAudio/QOALoader.h b/Userland/Libraries/LibAudio/QOALoader.h index d7fd80fe293..385cb0abcfe 100644 --- a/Userland/Libraries/LibAudio/QOALoader.h +++ b/Userland/Libraries/LibAudio/QOALoader.h @@ -7,6 +7,7 @@ #pragma once #include +#include #include #include #include @@ -24,8 +25,8 @@ public: explicit QOALoaderPlugin(NonnullOwnPtr stream); virtual ~QOALoaderPlugin() override = default; - static Result, LoaderError> create(StringView path); - static Result, LoaderError> create(Bytes buffer); + static bool sniff(SeekableStream& stream); + static ErrorOr, LoaderError> create(NonnullOwnPtr); virtual ErrorOr>, LoaderError> load_chunks(size_t samples_to_read_from_input) override; diff --git a/Userland/Libraries/LibAudio/WavLoader.cpp b/Userland/Libraries/LibAudio/WavLoader.cpp index e337b72aef0..639e276f7fa 100644 --- a/Userland/Libraries/LibAudio/WavLoader.cpp +++ b/Userland/Libraries/LibAudio/WavLoader.cpp @@ -12,9 +12,9 @@ #include #include #include +#include #include #include -#include namespace Audio { @@ -23,33 +23,29 @@ WavLoaderPlugin::WavLoaderPlugin(NonnullOwnPtr stream) { } -Result, LoaderError> WavLoaderPlugin::create(StringView path) +bool WavLoaderPlugin::sniff(SeekableStream& stream) { - auto stream = LOADER_TRY(Core::InputBufferedFile::create(LOADER_TRY(Core::File::open(path, Core::File::OpenMode::Read)))); - auto loader = make(move(stream)); + auto riff = stream.read_value(); + if (riff.is_error()) + return false; + if (riff.value() != RIFF::riff_magic) + return false; - LOADER_TRY(loader->initialize()); + auto size = stream.read_value>(); + if (size.is_error()) + return false; - return loader; + auto wave = stream.read_value(); + return !wave.is_error() && wave.value() == RIFF::wave_subformat_id; } -Result, LoaderError> WavLoaderPlugin::create(Bytes buffer) +ErrorOr, LoaderError> WavLoaderPlugin::create(NonnullOwnPtr stream) { - auto stream = LOADER_TRY(try_make(buffer)); auto loader = make(move(stream)); - - LOADER_TRY(loader->initialize()); - + LOADER_TRY(loader->parse_header()); return loader; } -MaybeLoaderError WavLoaderPlugin::initialize() -{ - LOADER_TRY(parse_header()); - - return {}; -} - template MaybeLoaderError WavLoaderPlugin::read_samples_from_stream(Stream& stream, SampleReader read_sample, FixedArray& samples) const { diff --git a/Userland/Libraries/LibAudio/WavLoader.h b/Userland/Libraries/LibAudio/WavLoader.h index b920ce9da7b..3d19d5cfbad 100644 --- a/Userland/Libraries/LibAudio/WavLoader.h +++ b/Userland/Libraries/LibAudio/WavLoader.h @@ -25,8 +25,9 @@ namespace Audio { class WavLoaderPlugin : public LoaderPlugin { public: explicit WavLoaderPlugin(NonnullOwnPtr stream); - static Result, LoaderError> create(StringView path); - static Result, LoaderError> create(Bytes buffer); + + static bool sniff(SeekableStream& stream); + static ErrorOr, LoaderError> create(NonnullOwnPtr); virtual ErrorOr>, LoaderError> load_chunks(size_t samples_to_read_from_input) override; @@ -44,8 +45,6 @@ public: virtual PcmSampleFormat pcm_format() override { return m_sample_format; } private: - MaybeLoaderError initialize(); - MaybeLoaderError parse_header(); MaybeLoaderError load_wav_info_block(Vector info_chunks);