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

@@ -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<Audio::FlacLoaderPlugin>(flac_data.bytes());
+    auto flac_or_error = Audio::FlacLoaderPlugin::try_create(flac_data.bytes());
 
-    if (flac->initialize().is_error())
+    if (flac_or_error.is_error())
-        return 1;
+        return 0;
+
+    auto flac = flac_or_error.release_value();
 
     for (;;) {
         auto samples = flac->get_more_samples();

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<Audio::MP3LoaderPlugin>(flac_data.bytes());
+    auto mp3_or_error = Audio::MP3LoaderPlugin::try_create(flac_data.bytes());
 
-    if (mp3->initialize().is_error())
+    if (mp3_or_error.is_error())
-        return 1;
+        return 0;
+
+    auto mp3 = mp3_or_error.release_value();
 
     for (;;) {
         auto samples = mp3->get_more_samples();

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<Audio::WavLoaderPlugin>(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();

Tests/LibAudio/TestFLACSpec.cpp

@@ -21,14 +21,16 @@ struct FlacTest : Test::TestCase {
 
     void run() const
     {
-        auto loader = Audio::FlacLoaderPlugin { m_path.string() };
+        auto result = Audio::FlacLoaderPlugin::try_create(m_path.string());
-        if (auto result = loader.initialize(); result.is_error()) {
+        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;

Userland/Libraries/LibAudio/FlacLoader.cpp

@@ -25,20 +25,33 @@
 
 namespace Audio {
 
-FlacLoaderPlugin::FlacLoaderPlugin(StringView path)
+FlacLoaderPlugin::FlacLoaderPlugin(OwnPtr<Core::Stream::SeekableStream> stream)
-    : LoaderPlugin(path)
+    : LoaderPlugin(move(stream))
 {
 }
 
-FlacLoaderPlugin::FlacLoaderPlugin(Bytes buffer)
+Result<NonnullOwnPtr<FlacLoaderPlugin>, LoaderError> FlacLoaderPlugin::try_create(StringView path)
-    : LoaderPlugin(buffer)
 {
+    auto stream = LOADER_TRY(Core::Stream::BufferedFile::create(LOADER_TRY(Core::Stream::File::open(path, Core::Stream::OpenMode::Read))));
+    auto loader = make<FlacLoaderPlugin>(move(stream));
+
+    LOADER_TRY(loader->initialize());
+
+    return loader;
+}
+
+Result<NonnullOwnPtr<FlacLoaderPlugin>, LoaderError> FlacLoaderPlugin::try_create(Bytes buffer)
+{
+    auto stream = LOADER_TRY(Core::Stream::MemoryStream::construct(buffer));
+    auto loader = make<FlacLoaderPlugin>(move(stream));
+
+    LOADER_TRY(loader->initialize());
+
+    return loader;
 }
 
 MaybeLoaderError FlacLoaderPlugin::initialize()
 {
-    LOADER_TRY(LoaderPlugin::initialize());
-
     TRY(parse_header());
     TRY(reset());
     return {};

Userland/Libraries/LibAudio/FlacLoader.h

@@ -47,11 +47,11 @@ ALWAYS_INLINE ErrorOr<i32> 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(OwnPtr<Core::Stream::SeekableStream> stream);
-    explicit FlacLoaderPlugin(Bytes buffer);
     virtual ~FlacLoaderPlugin() override = default;
 
-    virtual MaybeLoaderError initialize() override;
+    static Result<NonnullOwnPtr<FlacLoaderPlugin>, LoaderError> try_create(StringView path);
+    static Result<NonnullOwnPtr<FlacLoaderPlugin>, 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.

Userland/Libraries/LibAudio/Loader.cpp

@@ -11,26 +11,11 @@
 
 namespace Audio {
 
-LoaderPlugin::LoaderPlugin(StringView path)
+LoaderPlugin::LoaderPlugin(OwnPtr<Core::Stream::SeekableStream> stream)
-    : m_path(path)
+    : 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<LoaderPlugin> plugin)
     : m_plugin(move(plugin))
 {
@@ -38,35 +23,47 @@ Loader::Loader(NonnullOwnPtr<LoaderPlugin> plugin)
 
 Result<NonnullOwnPtr<LoaderPlugin>, LoaderError> Loader::try_create(StringView path)
 {
-    NonnullOwnPtr<LoaderPlugin> plugin = adopt_own(*new WavLoaderPlugin(path));
+    {
-    auto initstate0 = plugin->initialize();
+        auto plugin = WavLoaderPlugin::try_create(path);
-    if (!initstate0.is_error())
+        if (!plugin.is_error())
-        return plugin;
+            return NonnullOwnPtr<LoaderPlugin>(plugin.release_value());
+    }
 
-    plugin = adopt_own(*new FlacLoaderPlugin(path));
+    {
-    auto initstate1 = plugin->initialize();
+        auto plugin = FlacLoaderPlugin::try_create(path);
-    if (!initstate1.is_error())
+        if (!plugin.is_error())
-        return plugin;
+            return NonnullOwnPtr<LoaderPlugin>(plugin.release_value());
+    }
 
-    plugin = adopt_own(*new MP3LoaderPlugin(path));
+    {
-    auto initstate2 = plugin->initialize();
+        auto plugin = MP3LoaderPlugin::try_create(path);
-    if (!initstate2.is_error())
+        if (!plugin.is_error())
-        return plugin;
+            return NonnullOwnPtr<LoaderPlugin>(plugin.release_value());
+    }
 
     return LoaderError { "No loader plugin available" };
 }
 
 Result<NonnullOwnPtr<LoaderPlugin>, LoaderError> Loader::try_create(Bytes buffer)
 {
-    NonnullOwnPtr<LoaderPlugin> plugin = adopt_own(*new WavLoaderPlugin(buffer));
+    {
-    if (auto initstate = plugin->initialize(); !initstate.is_error())
+        auto plugin = WavLoaderPlugin::try_create(buffer);
-        return plugin;
+        if (!plugin.is_error())
-    plugin = adopt_own(*new FlacLoaderPlugin(buffer));
+            return NonnullOwnPtr<LoaderPlugin>(plugin.release_value());
-    if (auto initstate = plugin->initialize(); !initstate.is_error())
+    }
-        return plugin;
+
-    plugin = adopt_own(*new MP3LoaderPlugin(buffer));
+    {
-    if (auto initstate = plugin->initialize(); !initstate.is_error())
+        auto plugin = FlacLoaderPlugin::try_create(buffer);
-        return plugin;
+        if (!plugin.is_error())
+            return NonnullOwnPtr<LoaderPlugin>(plugin.release_value());
+    }
+
+    {
+        auto plugin = MP3LoaderPlugin::try_create(buffer);
+        if (!plugin.is_error())
+            return NonnullOwnPtr<LoaderPlugin>(plugin.release_value());
+    }
+
     return LoaderError { "No loader plugin available" };
 }
 

Userland/Libraries/LibAudio/Loader.h

@@ -30,12 +30,9 @@ using MaybeLoaderError = Result<void, LoaderError>;
 
 class LoaderPlugin {
 public:
-    explicit LoaderPlugin(StringView path);
+    explicit LoaderPlugin(OwnPtr<Core::Stream::SeekableStream> stream);
-    explicit LoaderPlugin(Bytes buffer);
     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<PictureData> const& pictures() const { return m_pictures; };
 
 protected:
-    StringView m_path;
     OwnPtr<Core::Stream::SeekableStream> m_stream;
-    // The constructor might set this so that we can initialize the data stream later.
-    Optional<Bytes> m_backing_memory;
 
     Vector<PictureData> m_pictures;
 };

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)
+MP3LoaderPlugin::MP3LoaderPlugin(OwnPtr<Core::Stream::SeekableStream> stream)
-    : LoaderPlugin(path)
+    : LoaderPlugin(move(stream))
 {
 }
 
-MP3LoaderPlugin::MP3LoaderPlugin(Bytes buffer)
+Result<NonnullOwnPtr<MP3LoaderPlugin>, LoaderError> MP3LoaderPlugin::try_create(StringView path)
-    : LoaderPlugin(buffer)
 {
+    auto stream = LOADER_TRY(Core::Stream::BufferedFile::create(LOADER_TRY(Core::Stream::File::open(path, Core::Stream::OpenMode::Read))));
+    auto loader = make<MP3LoaderPlugin>(move(stream));
+
+    LOADER_TRY(loader->initialize());
+
+    return loader;
+}
+
+Result<NonnullOwnPtr<MP3LoaderPlugin>, LoaderError> MP3LoaderPlugin::try_create(Bytes buffer)
+{
+    auto stream = LOADER_TRY(Core::Stream::MemoryStream::construct(buffer));
+    auto loader = make<MP3LoaderPlugin>(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());

Userland/Libraries/LibAudio/MP3Loader.h

@@ -23,11 +23,12 @@ struct ScaleFactorBand;
 
 class MP3LoaderPlugin : public LoaderPlugin {
 public:
-    explicit MP3LoaderPlugin(StringView path);
+    explicit MP3LoaderPlugin(OwnPtr<Core::Stream::SeekableStream> stream);
-    explicit MP3LoaderPlugin(Bytes buffer);
     virtual ~MP3LoaderPlugin() = default;
 
-    virtual MaybeLoaderError initialize() override;
+    static Result<NonnullOwnPtr<MP3LoaderPlugin>, LoaderError> try_create(StringView path);
+    static Result<NonnullOwnPtr<MP3LoaderPlugin>, 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<MP3::Header, LoaderError> read_header();

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)
+WavLoaderPlugin::WavLoaderPlugin(OwnPtr<Core::Stream::SeekableStream> stream)
-    : LoaderPlugin(path)
+    : LoaderPlugin(move(stream))
 {
 }
 
+Result<NonnullOwnPtr<WavLoaderPlugin>, 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<WavLoaderPlugin>(move(stream));
+
+    LOADER_TRY(loader->initialize());
+
+    return loader;
+}
+
+Result<NonnullOwnPtr<WavLoaderPlugin>, LoaderError> WavLoaderPlugin::try_create(Bytes buffer)
+{
+    auto stream = LOADER_TRY(Core::Stream::MemoryStream::construct(buffer));
+    auto loader = make<WavLoaderPlugin>(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<typename SampleReader>
 MaybeLoaderError WavLoaderPlugin::read_samples_from_stream(Core::Stream::Stream& stream, SampleReader read_sample, FixedArray<Sample>& samples) const
 {

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(OwnPtr<Core::Stream::SeekableStream> stream);
-    explicit WavLoaderPlugin(Bytes buffer);
+    static Result<NonnullOwnPtr<WavLoaderPlugin>, LoaderError> try_create(StringView path);
-
+    static Result<NonnullOwnPtr<WavLoaderPlugin>, LoaderError> try_create(Bytes buffer);
-    virtual MaybeLoaderError initialize() override;
 
     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;