LibVideo: Propagate decoder errors in the Matroska Reader

Matroska::Reader functions now return DecoderErrorOr instead of values
being declared Optional. Useful errors can be handled by the users of
the parser, similarly to the VP9 decoder. A lot of the error checking
in the reader is a lot cleaner thanks to this change, since all reads
can be range checked in Streamer::read_octet() now.

Most functions for the Streamer class are now also out-of-line in
Reader.cpp now instead of residing in the header.

Tests/LibVideo/TestVP9Decode.cpp

@@ -11,8 +11,7 @@
 
 static void decode_video(StringView path, size_t expected_frame_count)
 {
-    auto matroska_document = Video::Matroska::Reader::parse_matroska_from_file(path);
-    VERIFY(matroska_document);
+    auto matroska_document = MUST(Video::Matroska::Reader::parse_matroska_from_file(path));
     auto video_track_optional = matroska_document->track_for_track_type(Video::Matroska::TrackEntry::TrackType::Video);
     VERIFY(video_track_optional.has_value());
     auto video_track_entry = video_track_optional.value();

Userland/Libraries/LibVideo/Containers/Matroska/MatroskaDemuxer.cpp

@@ -10,22 +10,12 @@ namespace Video::Matroska {
 
 DecoderErrorOr<NonnullOwnPtr<MatroskaDemuxer>> MatroskaDemuxer::from_file(StringView filename)
 {
-    // FIXME: MatroskaReader should return errors.
-    auto nullable_document = Reader::parse_matroska_from_file(filename);
-    if (!nullable_document)
-        return DecoderError::format(DecoderErrorCategory::IO, "Failed to open matroska from file '{}'", filename);
-    auto document = nullable_document.release_nonnull();
-    return make<MatroskaDemuxer>(document);
+    return make<MatroskaDemuxer>(TRY(Reader::parse_matroska_from_file(filename)));
 }
 
 DecoderErrorOr<NonnullOwnPtr<MatroskaDemuxer>> MatroskaDemuxer::from_data(ReadonlyBytes data)
 {
-    // FIXME: MatroskaReader should return errors.
-    auto nullable_document = Reader::parse_matroska_from_data(data.data(), data.size());
-    if (!nullable_document)
-        return DecoderError::format(DecoderErrorCategory::IO, "Failed to open matroska from data");
-    auto document = nullable_document.release_nonnull();
-    return make<MatroskaDemuxer>(document);
+    return make<MatroskaDemuxer>(TRY(Reader::parse_matroska_from_data(data.data(), data.size())));
 }
 
 Vector<Track> MatroskaDemuxer::get_tracks_for_type(TrackType type)

Userland/Libraries/LibVideo/Containers/Matroska/MatroskaDemuxer.h

@@ -20,7 +20,7 @@ public:
     static DecoderErrorOr<NonnullOwnPtr<MatroskaDemuxer>> from_file(StringView filename);
     static DecoderErrorOr<NonnullOwnPtr<MatroskaDemuxer>> from_data(ReadonlyBytes data);
 
-    MatroskaDemuxer(NonnullOwnPtr<MatroskaDocument>& document)
+    MatroskaDemuxer(NonnullOwnPtr<MatroskaDocument>&& document)
         : m_document(move(document))
     {
     }

Userland/Libraries/LibVideo/Containers/Matroska/Reader.cpp

@@ -1,5 +1,6 @@
 /*
  * Copyright (c) 2021, Hunter Salyer <thefalsehonesty@gmail.com>
+ * Copyright (c) 2022, Gregory Bertilson <Zaggy1024@gmail.com>
  *
  * SPDX-License-Identifier: BSD-2-Clause
  */
@@ -13,9 +14,7 @@
 
 namespace Video::Matroska {
 
-#define CHECK_HAS_VALUE(x) \
-    if (!(x).has_value())  \
-    return false
+#define TRY_READ(expression) DECODER_TRY(DecoderErrorCategory::Corrupted, expression)
 
 constexpr u32 EBML_MASTER_ELEMENT_ID = 0x1A45DFA3;
 constexpr u32 SEGMENT_ELEMENT_ID = 0x18538067;
@@ -56,415 +55,335 @@ constexpr u32 BIT_DEPTH_ID = 0x6264;
 constexpr u32 SIMPLE_BLOCK_ID = 0xA3;
 constexpr u32 TIMESTAMP_ID = 0xE7;
 
-OwnPtr<MatroskaDocument> Reader::parse_matroska_from_file(StringView path)
+DecoderErrorOr<NonnullOwnPtr<MatroskaDocument>> Reader::parse_matroska_from_file(StringView path)
 {
-    auto mapped_file_result = Core::MappedFile::map(path);
-    if (mapped_file_result.is_error())
-        return {};
-
-    auto mapped_file = mapped_file_result.release_value();
+    auto mapped_file = DECODER_TRY(DecoderErrorCategory::IO, Core::MappedFile::map(path));
     return parse_matroska_from_data((u8*)mapped_file->data(), mapped_file->size());
 }
 
-OwnPtr<MatroskaDocument> Reader::parse_matroska_from_data(u8 const* data, size_t size)
+DecoderErrorOr<NonnullOwnPtr<MatroskaDocument>> Reader::parse_matroska_from_data(u8 const* data, size_t size)
 {
     Reader reader(data, size);
     return reader.parse();
 }
 
-OwnPtr<MatroskaDocument> Reader::parse()
+DecoderErrorOr<NonnullOwnPtr<MatroskaDocument>> Reader::parse()
 {
-    auto first_element_id = m_streamer.read_variable_size_integer(false);
-    dbgln_if(MATROSKA_TRACE_DEBUG, "First element ID is {:#010x}\n", first_element_id.value());
-    if (!first_element_id.has_value() || first_element_id.value() != EBML_MASTER_ELEMENT_ID)
-        return {};
-
-    auto header = parse_ebml_header();
-    if (!header.has_value())
-        return {};
+    auto first_element_id = TRY_READ(m_streamer.read_variable_size_integer(false));
+    dbgln_if(MATROSKA_TRACE_DEBUG, "First element ID is {:#010x}\n", first_element_id);
+    if (first_element_id != EBML_MASTER_ELEMENT_ID)
+        return DecoderError::corrupted("First element was not an EBML header"sv);
 
+    auto header = TRY(parse_ebml_header());
     dbgln_if(MATROSKA_DEBUG, "Parsed EBML header");
 
-    auto root_element_id = m_streamer.read_variable_size_integer(false);
-    if (!root_element_id.has_value() || root_element_id.value() != SEGMENT_ELEMENT_ID)
-        return {};
-
-    auto matroska_document = make<MatroskaDocument>(header.value());
-
-    auto segment_parse_success = parse_segment_elements(*matroska_document);
-    if (!segment_parse_success)
-        return {};
+    auto root_element_id = TRY_READ(m_streamer.read_variable_size_integer(false));
+    if (root_element_id != SEGMENT_ELEMENT_ID)
+        return DecoderError::corrupted("Second element was not a segment element"sv);
 
+    auto matroska_document = make<MatroskaDocument>(header);
+    TRY(parse_segment_elements(*matroska_document));
     return matroska_document;
 }
 
-bool Reader::parse_master_element([[maybe_unused]] StringView element_name, Function<bool(u64)> element_consumer)
+DecoderErrorOr<void> Reader::parse_master_element([[maybe_unused]] StringView element_name, Function<DecoderErrorOr<void>(u64)> element_consumer)
 {
-    auto element_data_size = m_streamer.read_variable_size_integer();
-    CHECK_HAS_VALUE(element_data_size);
-    dbgln_if(MATROSKA_DEBUG, "{} has {} octets of data.", element_name, element_data_size.value());
+    auto element_data_size = TRY_READ(m_streamer.read_variable_size_integer());
+    dbgln_if(MATROSKA_DEBUG, "{} has {} octets of data.", element_name, element_data_size);
 
     m_streamer.push_octets_read();
-    while (m_streamer.octets_read() < element_data_size.value()) {
+    while (m_streamer.octets_read() < element_data_size) {
         dbgln_if(MATROSKA_TRACE_DEBUG, "====== Reading  element ======");
-        auto optional_element_id = m_streamer.read_variable_size_integer(false);
-        CHECK_HAS_VALUE(optional_element_id);
-
-        auto element_id = optional_element_id.value();
+        auto element_id = TRY_READ(m_streamer.read_variable_size_integer(false));
         dbgln_if(MATROSKA_TRACE_DEBUG, "{:s} element ID is {:#010x}\n", element_name, element_id);
 
-        if (!element_consumer(element_id)) {
-            dbgln_if(MATROSKA_DEBUG, "{:s} consumer failed on ID {:#010x}\n", element_name.to_string().characters(), element_id);
-            return false;
-        }
-
+        TRY(element_consumer(element_id));
         dbgln_if(MATROSKA_TRACE_DEBUG, "Read {} octets of the {} so far.", m_streamer.octets_read(), element_name);
     }
     m_streamer.pop_octets_read();
 
-    return true;
+    return {};
 }
 
-Optional<EBMLHeader> Reader::parse_ebml_header()
+DecoderErrorOr<EBMLHeader> Reader::parse_ebml_header()
 {
     EBMLHeader header;
-    auto success = parse_master_element("Header"sv, [&](u64 element_id) {
-        if (element_id == DOCTYPE_ELEMENT_ID) {
-            auto doc_type = read_string_element();
-            CHECK_HAS_VALUE(doc_type);
-            header.doc_type = doc_type.value();
-            dbgln_if(MATROSKA_DEBUG, "Read DocType attribute: {}", doc_type.value());
-        } else if (element_id == DOCTYPE_VERSION_ELEMENT_ID) {
-            auto doc_type_version = read_u64_element();
-            CHECK_HAS_VALUE(doc_type_version);
-            header.doc_type_version = doc_type_version.value();
-            dbgln_if(MATROSKA_DEBUG, "Read DocTypeVersion attribute: {}", doc_type_version.value());
-        } else {
-            return read_unknown_element();
+    TRY(parse_master_element("Header"sv, [&](u64 element_id) -> DecoderErrorOr<void> {
+        switch (element_id) {
+        case DOCTYPE_ELEMENT_ID:
+            header.doc_type = TRY_READ(m_streamer.read_string());
+            dbgln_if(MATROSKA_DEBUG, "Read DocType attribute: {}", header.doc_type);
+            break;
+        case DOCTYPE_VERSION_ELEMENT_ID:
+            header.doc_type_version = TRY_READ(m_streamer.read_u64());
+            dbgln_if(MATROSKA_DEBUG, "Read DocTypeVersion attribute: {}", header.doc_type_version);
+            break;
+        default:
+            TRY_READ(m_streamer.read_unknown_element());
         }
 
-        return true;
-    });
-
-    if (!success)
         return {};
+    }));
+
     return header;
 }
 
-bool Reader::parse_segment_elements(MatroskaDocument& matroska_document)
+DecoderErrorOr<void> Reader::parse_segment_elements(MatroskaDocument& matroska_document)
 {
     dbgln_if(MATROSKA_DEBUG, "Parsing segment elements");
-    auto success = parse_master_element("Segment"sv, [&](u64 element_id) {
-        if (element_id == SEGMENT_INFORMATION_ELEMENT_ID) {
-            auto segment_information = parse_information();
-            if (!segment_information)
-                return false;
-            matroska_document.set_segment_information(move(segment_information));
-        } else if (element_id == TRACK_ELEMENT_ID) {
-            return parse_tracks(matroska_document);
-        } else if (element_id == CLUSTER_ELEMENT_ID) {
-            auto cluster = parse_cluster();
-            if (!cluster)
-                return false;
-            matroska_document.clusters().append(cluster.release_nonnull());
-        } else {
-            return read_unknown_element();
+    return parse_master_element("Segment"sv, [&](u64 element_id) -> DecoderErrorOr<void> {
+        switch (element_id) {
+        case SEGMENT_INFORMATION_ELEMENT_ID:
+            matroska_document.set_segment_information(TRY(parse_information()));
+            break;
+        case TRACK_ELEMENT_ID:
+            TRY(parse_tracks(matroska_document));
+            break;
+        case CLUSTER_ELEMENT_ID:
+            matroska_document.clusters().append(TRY(parse_cluster()));
+            break;
+        default:
+            TRY_READ(m_streamer.read_unknown_element());
         }
 
-        return true;
+        return {};
     });
-
-    dbgln_if(MATROSKA_DEBUG, "Success {}", success);
-    return success;
 }
 
-OwnPtr<SegmentInformation> Reader::parse_information()
+DecoderErrorOr<NonnullOwnPtr<SegmentInformation>> Reader::parse_information()
 {
     auto segment_information = make<SegmentInformation>();
-    auto success = parse_master_element("Segment Information"sv, [&](u64 element_id) {
-        if (element_id == TIMESTAMP_SCALE_ID) {
-            auto timestamp_scale = read_u64_element();
-            CHECK_HAS_VALUE(timestamp_scale);
-            segment_information->set_timestamp_scale(timestamp_scale.value());
-            dbgln_if(MATROSKA_DEBUG, "Read TimestampScale attribute: {}", timestamp_scale.value());
-        } else if (element_id == MUXING_APP_ID) {
-            auto muxing_app = read_string_element();
-            CHECK_HAS_VALUE(muxing_app);
-            segment_information->set_muxing_app(muxing_app.value());
-            dbgln_if(MATROSKA_DEBUG, "Read MuxingApp attribute: {}", muxing_app.value());
-        } else if (element_id == WRITING_APP_ID) {
-            auto writing_app = read_string_element();
-            CHECK_HAS_VALUE(writing_app);
-            segment_information->set_writing_app(writing_app.value());
-            dbgln_if(MATROSKA_DEBUG, "Read WritingApp attribute: {}", writing_app.value());
-        } else if (element_id == DURATION_ID) {
-            auto duration = read_float_element();
-            CHECK_HAS_VALUE(duration);
-            segment_information->set_duration(duration.value());
-            dbgln_if(MATROSKA_DEBUG, "Read Duration attribute: {}", duration.value());
-        } else {
-            return read_unknown_element();
+    TRY(parse_master_element("Segment Information"sv, [&](u64 element_id) -> DecoderErrorOr<void> {
+        switch (element_id) {
+        case TIMESTAMP_SCALE_ID:
+            segment_information->set_timestamp_scale(TRY_READ(m_streamer.read_u64()));
+            dbgln_if(MATROSKA_DEBUG, "Read TimestampScale attribute: {}", segment_information->timestamp_scale());
+            break;
+        case MUXING_APP_ID:
+            segment_information->set_muxing_app(TRY_READ(m_streamer.read_string()));
+            dbgln_if(MATROSKA_DEBUG, "Read MuxingApp attribute: {}", segment_information->muxing_app().as_string());
+            break;
+        case WRITING_APP_ID:
+            segment_information->set_writing_app(TRY_READ(m_streamer.read_string()));
+            dbgln_if(MATROSKA_DEBUG, "Read WritingApp attribute: {}", segment_information->writing_app().as_string());
+            break;
+        case DURATION_ID:
+            segment_information->set_duration(TRY_READ(m_streamer.read_float()));
+            dbgln_if(MATROSKA_DEBUG, "Read Duration attribute: {}", segment_information->duration().value());
+            break;
+        default:
+            TRY_READ(m_streamer.read_unknown_element());
         }
 
-        return true;
-    });
-
-    if (!success)
         return {};
+    }));
+
     return segment_information;
 }
 
-bool Reader::parse_tracks(MatroskaDocument& matroska_document)
+DecoderErrorOr<void> Reader::parse_tracks(MatroskaDocument& matroska_document)
 {
-    auto success = parse_master_element("Tracks"sv, [&](u64 element_id) {
+    return parse_master_element("Tracks"sv, [&](u64 element_id) -> DecoderErrorOr<void> {
         if (element_id == TRACK_ENTRY_ID) {
             dbgln_if(MATROSKA_DEBUG, "Parsing track");
-            auto track_entry = parse_track_entry();
-            if (!track_entry)
-                return false;
+            auto track_entry = TRY(parse_track_entry());
             auto track_number = track_entry->track_number();
-            matroska_document.add_track(track_number, track_entry.release_nonnull());
-            dbgln_if(MATROSKA_DEBUG, "Track {} added to document", track_number);
+            dbgln_if(MATROSKA_DEBUG, "Adding track {} to document", track_number);
+            matroska_document.add_track(track_number, track_entry.release_nonnull<TrackEntry>());
         } else {
-            return read_unknown_element();
+            TRY_READ(m_streamer.read_unknown_element());
         }
 
-        return true;
+        return {};
     });
-
-    return success;
 }
 
-OwnPtr<TrackEntry> Reader::parse_track_entry()
+DecoderErrorOr<NonnullOwnPtr<TrackEntry>> Reader::parse_track_entry()
 {
     auto track_entry = make<TrackEntry>();
-    auto success = parse_master_element("Track"sv, [&](u64 element_id) {
-        if (element_id == TRACK_NUMBER_ID) {
-            auto track_number = read_u64_element();
-            CHECK_HAS_VALUE(track_number);
-            track_entry->set_track_number(track_number.value());
-            dbgln_if(MATROSKA_TRACE_DEBUG, "Read TrackNumber attribute: {}", track_number.value());
-        } else if (element_id == TRACK_UID_ID) {
-            auto track_uid = read_u64_element();
-            CHECK_HAS_VALUE(track_uid);
-            track_entry->set_track_uid(track_uid.value());
-            dbgln_if(MATROSKA_TRACE_DEBUG, "Read TrackUID attribute: {}", track_uid.value());
-        } else if (element_id == TRACK_TYPE_ID) {
-            auto track_type = read_u64_element();
-            CHECK_HAS_VALUE(track_type);
-            track_entry->set_track_type(static_cast<TrackEntry::TrackType>(track_type.value()));
-            dbgln_if(MATROSKA_TRACE_DEBUG, "Read TrackType attribute: {}", track_type.value());
-        } else if (element_id == TRACK_LANGUAGE_ID) {
-            auto language = read_string_element();
-            CHECK_HAS_VALUE(language);
-            track_entry->set_language(language.value());
-            dbgln_if(MATROSKA_TRACE_DEBUG, "Read Track's Language attribute: {}", language.value());
-        } else if (element_id == TRACK_CODEC_ID) {
-            auto codec_id = read_string_element();
-            CHECK_HAS_VALUE(codec_id);
-            track_entry->set_codec_id(codec_id.value());
-            dbgln_if(MATROSKA_TRACE_DEBUG, "Read Track's CodecID attribute: {}", codec_id.value());
-        } else if (element_id == TRACK_VIDEO_ID) {
-            auto video_track = parse_video_track_information();
-            CHECK_HAS_VALUE(video_track);
-            track_entry->set_video_track(video_track.value());
-        } else if (element_id == TRACK_AUDIO_ID) {
-            auto audio_track = parse_audio_track_information();
-            CHECK_HAS_VALUE(audio_track);
-            track_entry->set_audio_track(audio_track.value());
-        } else {
-            return read_unknown_element();
+    TRY(parse_master_element("Track"sv, [&](u64 element_id) -> DecoderErrorOr<void> {
+        switch (element_id) {
+        case TRACK_NUMBER_ID:
+            track_entry->set_track_number(TRY_READ(m_streamer.read_u64()));
+            dbgln_if(MATROSKA_TRACE_DEBUG, "Read TrackNumber attribute: {}", track_entry->track_number());
+            break;
+        case TRACK_UID_ID:
+            track_entry->set_track_uid(TRY_READ(m_streamer.read_u64()));
+            dbgln_if(MATROSKA_TRACE_DEBUG, "Read TrackUID attribute: {}", track_entry->track_uid());
+            break;
+        case TRACK_TYPE_ID:
+            track_entry->set_track_type(static_cast<TrackEntry::TrackType>(TRY_READ(m_streamer.read_u64())));
+            dbgln_if(MATROSKA_TRACE_DEBUG, "Read TrackType attribute: {}", track_entry->track_type());
+            break;
+        case TRACK_LANGUAGE_ID:
+            track_entry->set_language(TRY_READ(m_streamer.read_string()));
+            dbgln_if(MATROSKA_TRACE_DEBUG, "Read Track's Language attribute: {}", track_entry->language());
+            break;
+        case TRACK_CODEC_ID:
+            track_entry->set_codec_id(TRY_READ(m_streamer.read_string()));
+            dbgln_if(MATROSKA_TRACE_DEBUG, "Read Track's CodecID attribute: {}", track_entry->codec_id());
+            break;
+        case TRACK_VIDEO_ID:
+            track_entry->set_video_track(TRY(parse_video_track_information()));
+            break;
+        case TRACK_AUDIO_ID:
+            track_entry->set_audio_track(TRY(parse_audio_track_information()));
+            break;
+        default:
+            TRY_READ(m_streamer.read_unknown_element());
         }
 
-        return true;
-    });
-
-    if (!success)
         return {};
+    }));
+
     return track_entry;
 }
 
-Optional<TrackEntry::ColorFormat> Reader::parse_video_color_information()
+DecoderErrorOr<TrackEntry::ColorFormat> Reader::parse_video_color_information()
 {
     TrackEntry::ColorFormat color_format {};
 
-    auto success = parse_master_element("Colour"sv, [&](u64 element_id) {
+    TRY(parse_master_element("Colour"sv, [&](u64 element_id) -> DecoderErrorOr<void> {
         switch (element_id) {
-        case PRIMARIES_ID: {
-            auto primaries_result = read_u64_element();
-            CHECK_HAS_VALUE(primaries_result);
-            color_format.color_primaries = static_cast<ColorPrimaries>(primaries_result.value());
+        case PRIMARIES_ID:
+            color_format.color_primaries = static_cast<ColorPrimaries>(TRY_READ(m_streamer.read_u64()));
             dbgln_if(MATROSKA_TRACE_DEBUG, "Read Colour's Primaries attribute: {}", color_primaries_to_string(color_format.color_primaries));
             break;
-        }
-        case TRANSFER_CHARACTERISTICS_ID: {
-            auto transfer_characteristics_result = read_u64_element();
-            CHECK_HAS_VALUE(transfer_characteristics_result);
-            color_format.transfer_characteristics = static_cast<TransferCharacteristics>(transfer_characteristics_result.value());
+        case TRANSFER_CHARACTERISTICS_ID:
+            color_format.transfer_characteristics = static_cast<TransferCharacteristics>(TRY_READ(m_streamer.read_u64()));
             dbgln_if(MATROSKA_TRACE_DEBUG, "Read Colour's TransferCharacteristics attribute: {}", transfer_characteristics_to_string(color_format.transfer_characteristics));
             break;
-        }
-        case MATRIX_COEFFICIENTS_ID: {
-            auto matrix_coefficients_result = read_u64_element();
-            CHECK_HAS_VALUE(matrix_coefficients_result);
-            color_format.matrix_coefficients = static_cast<MatrixCoefficients>(matrix_coefficients_result.value());
+        case MATRIX_COEFFICIENTS_ID:
+            color_format.matrix_coefficients = static_cast<MatrixCoefficients>(TRY_READ(m_streamer.read_u64()));
             dbgln_if(MATROSKA_TRACE_DEBUG, "Read Colour's MatrixCoefficients attribute: {}", matrix_coefficients_to_string(color_format.matrix_coefficients));
             break;
-        }
-        case BITS_PER_CHANNEL_ID: {
-            auto bits_per_channel_result = read_u64_element();
-            CHECK_HAS_VALUE(bits_per_channel_result);
-            color_format.bits_per_channel = bits_per_channel_result.value();
+        case BITS_PER_CHANNEL_ID:
+            color_format.bits_per_channel = TRY_READ(m_streamer.read_u64());
             dbgln_if(MATROSKA_TRACE_DEBUG, "Read Colour's BitsPerChannel attribute: {}", color_format.bits_per_channel);
             break;
-        }
         default:
-            return read_unknown_element();
+            TRY_READ(m_streamer.read_unknown_element());
         }
-        return true;
-    });
 
-    if (!success)
         return {};
+    }));
+
     return color_format;
 }
 
-Optional<TrackEntry::VideoTrack> Reader::parse_video_track_information()
+DecoderErrorOr<TrackEntry::VideoTrack> Reader::parse_video_track_information()
 {
     TrackEntry::VideoTrack video_track {};
 
-    auto success = parse_master_element("VideoTrack"sv, [&](u64 element_id) {
-        if (element_id == PIXEL_WIDTH_ID) {
-            auto pixel_width = read_u64_element();
-            CHECK_HAS_VALUE(pixel_width);
-            video_track.pixel_width = pixel_width.value();
-            dbgln_if(MATROSKA_TRACE_DEBUG, "Read VideoTrack's PixelWidth attribute: {}", pixel_width.value());
-        } else if (element_id == PIXEL_HEIGHT_ID) {
-            auto pixel_height = read_u64_element();
-            CHECK_HAS_VALUE(pixel_height);
-            video_track.pixel_height = pixel_height.value();
-            dbgln_if(MATROSKA_TRACE_DEBUG, "Read VideoTrack's PixelHeight attribute: {}", pixel_height.value());
-        } else if (element_id == COLOR_ENTRY_ID) {
-            auto color_information_result = parse_video_color_information();
-            CHECK_HAS_VALUE(color_information_result);
-            video_track.color_format = color_information_result.value();
-        } else {
-            return read_unknown_element();
+    TRY(parse_master_element("VideoTrack"sv, [&](u64 element_id) -> DecoderErrorOr<void> {
+        switch (element_id) {
+        case PIXEL_WIDTH_ID:
+            video_track.pixel_width = TRY_READ(m_streamer.read_u64());
+            dbgln_if(MATROSKA_TRACE_DEBUG, "Read VideoTrack's PixelWidth attribute: {}", video_track.pixel_width);
+            break;
+        case PIXEL_HEIGHT_ID:
+            video_track.pixel_height = TRY_READ(m_streamer.read_u64());
+            dbgln_if(MATROSKA_TRACE_DEBUG, "Read VideoTrack's PixelHeight attribute: {}", video_track.pixel_height);
+            break;
+        case COLOR_ENTRY_ID:
+            video_track.color_format = TRY(parse_video_color_information());
+            break;
+        default:
+            TRY_READ(m_streamer.read_unknown_element());
         }
 
-        return true;
-    });
-
-    if (!success)
         return {};
+    }));
+
     return video_track;
 }
 
-Optional<TrackEntry::AudioTrack> Reader::parse_audio_track_information()
+DecoderErrorOr<TrackEntry::AudioTrack> Reader::parse_audio_track_information()
 {
     TrackEntry::AudioTrack audio_track {};
 
-    auto success = parse_master_element("AudioTrack"sv, [&](u64 element_id) {
-        if (element_id == CHANNELS_ID) {
-            auto channels = read_u64_element();
-            CHECK_HAS_VALUE(channels);
-            audio_track.channels = channels.value();
-            dbgln_if(MATROSKA_TRACE_DEBUG, "Read AudioTrack's Channels attribute: {}", channels.value());
-        } else if (element_id == BIT_DEPTH_ID) {
-            auto bit_depth = read_u64_element();
-            CHECK_HAS_VALUE(bit_depth);
-            audio_track.bit_depth = bit_depth.value();
-            dbgln_if(MATROSKA_TRACE_DEBUG, "Read AudioTrack's BitDepth attribute: {}", bit_depth.value());
-        } else {
-            return read_unknown_element();
+    TRY(parse_master_element("AudioTrack"sv, [&](u64 element_id) -> DecoderErrorOr<void> {
+        switch (element_id) {
+        case CHANNELS_ID:
+            audio_track.channels = TRY_READ(m_streamer.read_u64());
+            dbgln_if(MATROSKA_TRACE_DEBUG, "Read AudioTrack's Channels attribute: {}", audio_track.channels);
+            break;
+        case BIT_DEPTH_ID:
+            audio_track.bit_depth = TRY_READ(m_streamer.read_u64());
+            dbgln_if(MATROSKA_TRACE_DEBUG, "Read AudioTrack's BitDepth attribute: {}", audio_track.bit_depth);
+            break;
+        default:
+            TRY_READ(m_streamer.read_unknown_element());
         }
 
-        return true;
-    });
-
-    if (!success)
         return {};
+    }));
+
     return audio_track;
 }
 
-OwnPtr<Cluster> Reader::parse_cluster()
+DecoderErrorOr<NonnullOwnPtr<Cluster>> Reader::parse_cluster()
 {
     auto cluster = make<Cluster>();
 
-    auto success = parse_master_element("Cluster"sv, [&](u64 element_id) {
-        if (element_id == SIMPLE_BLOCK_ID) {
-            auto simple_block = parse_simple_block();
-            if (!simple_block)
-                return false;
-            cluster->blocks().append(simple_block.release_nonnull());
-        } else if (element_id == TIMESTAMP_ID) {
-            auto timestamp = read_u64_element();
-            if (!timestamp.has_value())
-                return false;
-            cluster->set_timestamp(timestamp.value());
-        } else {
-            auto success = read_unknown_element();
-            if (!success)
-                return false;
+    TRY(parse_master_element("Cluster"sv, [&](u64 element_id) -> DecoderErrorOr<void> {
+        switch (element_id) {
+        case SIMPLE_BLOCK_ID:
+            cluster->blocks().append(TRY(parse_simple_block()));
+            break;
+        case TIMESTAMP_ID:
+            cluster->set_timestamp(TRY_READ(m_streamer.read_u64()));
+            break;
+        default:
+            TRY_READ(m_streamer.read_unknown_element());
         }
 
-        return true;
-    });
-
-    if (!success)
         return {};
+    }));
+
     return cluster;
 }
 
-OwnPtr<Block> Reader::parse_simple_block()
+DecoderErrorOr<NonnullOwnPtr<Block>> Reader::parse_simple_block()
 {
     auto block = make<Block>();
 
-    auto content_size = m_streamer.read_variable_size_integer();
-    if (!content_size.has_value())
-        return {};
+    auto content_size = TRY_READ(m_streamer.read_variable_size_integer());
 
     auto octets_read_before_track_number = m_streamer.octets_read();
-    auto track_number = m_streamer.read_variable_size_integer();
-    if (!track_number.has_value())
-        return {};
-    block->set_track_number(track_number.value());
+    auto track_number = TRY_READ(m_streamer.read_variable_size_integer());
+    block->set_track_number(track_number);
 
-    if (m_streamer.remaining() < 3)
-        return {};
-    block->set_timestamp(m_streamer.read_i16());
+    block->set_timestamp(TRY_READ(m_streamer.read_i16()));
 
-    auto flags = m_streamer.read_octet();
+    auto flags = TRY_READ(m_streamer.read_octet());
     block->set_only_keyframes(flags & (1u << 7u));
     block->set_invisible(flags & (1u << 3u));
     block->set_lacing(static_cast<Block::Lacing>((flags & 0b110u) >> 1u));
     block->set_discardable(flags & 1u);
 
-    auto total_frame_content_size = content_size.value() - (m_streamer.octets_read() - octets_read_before_track_number);
+    auto total_frame_content_size = content_size - (m_streamer.octets_read() - octets_read_before_track_number);
     if (block->lacing() == Block::Lacing::EBML) {
         auto octets_read_before_frame_sizes = m_streamer.octets_read();
-        auto frame_count = m_streamer.read_octet() + 1;
+        auto frame_count = TRY_READ(m_streamer.read_octet()) + 1;
         Vector<u64> frame_sizes;
         frame_sizes.ensure_capacity(frame_count);
 
         u64 frame_size_sum = 0;
         u64 previous_frame_size;
-        auto first_frame_size = m_streamer.read_variable_size_integer();
-        if (!first_frame_size.has_value())
-            return {};
-        frame_sizes.append(first_frame_size.value());
-        frame_size_sum += first_frame_size.value();
-        previous_frame_size = first_frame_size.value();
+        auto first_frame_size = TRY_READ(m_streamer.read_variable_size_integer());
+        frame_sizes.append(first_frame_size);
+        frame_size_sum += first_frame_size;
+        previous_frame_size = first_frame_size;
 
         for (int i = 0; i < frame_count - 2; i++) {
-            auto frame_size_difference = m_streamer.read_variable_sized_signed_integer();
-            if (!frame_size_difference.has_value())
-                return {};
+            auto frame_size_difference = TRY_READ(m_streamer.read_variable_size_signed_integer());
             u64 frame_size;
-            if (frame_size_difference.value() < 0)
-                frame_size = previous_frame_size - (-frame_size_difference.value());
+            // FIXME: x - (-y) == x + y??
+            if (frame_size_difference < 0)
+                frame_size = previous_frame_size - (-frame_size_difference);
             else
-                frame_size = previous_frame_size + frame_size_difference.value();
+                frame_size = previous_frame_size + frame_size_difference;
             frame_sizes.append(frame_size);
             frame_size_sum += frame_size;
             previous_frame_size = frame_size;
@@ -472,64 +391,131 @@ OwnPtr<Block> Reader::parse_simple_block()
         frame_sizes.append(total_frame_content_size - frame_size_sum - (m_streamer.octets_read() - octets_read_before_frame_sizes));
 
         for (int i = 0; i < frame_count; i++) {
+            // FIXME: ReadonlyBytes instead of copying the frame data?
             auto current_frame_size = frame_sizes.at(i);
-            auto frame_result = ByteBuffer::copy(m_streamer.data(), current_frame_size);
-            if (frame_result.is_error())
-                return {};
-            block->add_frame(frame_result.release_value());
-            m_streamer.drop_octets(current_frame_size);
+            block->add_frame(DECODER_TRY_ALLOC(ByteBuffer::copy(m_streamer.data(), current_frame_size)));
+            TRY_READ(m_streamer.drop_octets(current_frame_size));
         }
     } else if (block->lacing() == Block::Lacing::FixedSize) {
-        auto frame_count = m_streamer.read_octet() + 1;
+        auto frame_count = TRY_READ(m_streamer.read_octet()) + 1;
         auto individual_frame_size = total_frame_content_size / frame_count;
         for (int i = 0; i < frame_count; i++) {
-            auto frame_result = ByteBuffer::copy(m_streamer.data(), individual_frame_size);
-            if (frame_result.is_error())
-                return {};
-            block->add_frame(frame_result.release_value());
-            m_streamer.drop_octets(individual_frame_size);
+            block->add_frame(DECODER_TRY_ALLOC(ByteBuffer::copy(m_streamer.data(), individual_frame_size)));
+            TRY_READ(m_streamer.drop_octets(individual_frame_size));
         }
     } else {
-        auto frame_result = ByteBuffer::copy(m_streamer.data(), total_frame_content_size);
-        if (frame_result.is_error())
-            return {};
-        block->add_frame(frame_result.release_value());
-        m_streamer.drop_octets(total_frame_content_size);
+        block->add_frame(DECODER_TRY_ALLOC(ByteBuffer::copy(m_streamer.data(), total_frame_content_size)));
+        TRY_READ(m_streamer.drop_octets(total_frame_content_size));
     }
     return block;
 }
 
-Optional<String> Reader::read_string_element()
+ErrorOr<String> Reader::Streamer::read_string()
 {
-    auto string_length = m_streamer.read_variable_size_integer();
-    if (!string_length.has_value() || m_streamer.remaining() < string_length.value())
-        return {};
-    auto string_value = String(m_streamer.data_as_chars(), string_length.value());
-    m_streamer.drop_octets(string_length.value());
+    auto string_length = TRY(read_variable_size_integer());
+    if (remaining() < string_length)
+        return Error::from_string_literal("String length extends past the end of the stream");
+    auto string_value = String(data_as_chars(), string_length);
+    TRY(drop_octets(string_length));
     return string_value;
 }
 
-Optional<u64> Reader::read_u64_element()
+ErrorOr<u8> Reader::Streamer::read_octet()
 {
-    auto integer_length = m_streamer.read_variable_size_integer();
-    if (!integer_length.has_value() || m_streamer.remaining() < integer_length.value())
-        return {};
+    if (!has_octet()) {
+        dbgln_if(MATROSKA_TRACE_DEBUG, "Ran out of stream data");
+        return Error::from_string_literal("Stream is out of data");
+    }
+    m_size_remaining--;
+    m_octets_read.last()++;
+    return *(m_data_ptr++);
+}
+
+ErrorOr<i16> Reader::Streamer::read_i16()
+{
+    return (TRY(read_octet()) << 8) | TRY(read_octet());
+}
+
+ErrorOr<u64> Reader::Streamer::read_variable_size_integer(bool mask_length)
+{
+    dbgln_if(MATROSKA_TRACE_DEBUG, "Reading from offset {:p}", m_data_ptr);
+    auto length_descriptor = TRY(read_octet());
+    dbgln_if(MATROSKA_TRACE_DEBUG, "Reading VINT, first byte is {:#02x}", length_descriptor);
+    if (length_descriptor == 0)
+        return Error::from_string_literal("read_variable_size_integer: Length descriptor has no terminating set bit");
+    size_t length = 0;
+    while (length < 8) {
+        if (((length_descriptor >> (8 - length)) & 1) == 1)
+            break;
+        length++;
+    }
+    dbgln_if(MATROSKA_TRACE_DEBUG, "Reading VINT of total length {}", length);
+    if (length > 8)
+        return Error::from_string_literal("read_variable_size_integer: Length is too large");
+
+    u64 result;
+    if (mask_length)
+        result = length_descriptor & ~(1u << (8 - length));
+    else
+        result = length_descriptor;
+    dbgln_if(MATROSKA_TRACE_DEBUG, "Beginning of VINT is {:#02x}", result);
+    for (size_t i = 1; i < length; i++) {
+        u8 next_octet = TRY(read_octet());
+        dbgln_if(MATROSKA_TRACE_DEBUG, "Read octet of {:#02x}", next_octet);
+        result = (result << 8u) | next_octet;
+        dbgln_if(MATROSKA_TRACE_DEBUG, "New result is {:#010x}", result);
+    }
+    return result;
+}
+
+ErrorOr<i64> Reader::Streamer::read_variable_size_signed_integer()
+{
+    auto length_descriptor = TRY(read_octet());
+    if (length_descriptor == 0)
+        return Error::from_string_literal("read_variable_sized_signed_integer: Length descriptor has no terminating set bit");
+    i64 length = 0;
+    while (length < 8) {
+        if (((length_descriptor >> (8 - length)) & 1) == 1)
+            break;
+        length++;
+    }
+    if (length > 8)
+        return Error::from_string_literal("read_variable_size_integer: Length is too large");
+
+    i64 result = length_descriptor & ~(1u << (8 - length));
+    for (i64 i = 1; i < length; i++) {
+        u8 next_octet = TRY(read_octet());
+        result = (result << 8u) | next_octet;
+    }
+    result -= AK::exp2<i64>(length * 7 - 1) - 1;
+    return result;
+}
+
+ErrorOr<void> Reader::Streamer::drop_octets(size_t num_octets)
+{
+    if (remaining() < num_octets)
+        return Error::from_string_literal("Tried to drop octets past the end of the stream");
+    m_size_remaining -= num_octets;
+    m_octets_read.last() += num_octets;
+    m_data_ptr += num_octets;
+    return {};
+}
+
+ErrorOr<u64> Reader::Streamer::read_u64()
+{
+    auto integer_length = TRY(read_variable_size_integer());
     u64 result = 0;
-    for (size_t i = 0; i < integer_length.value(); i++) {
-        if (!m_streamer.has_octet())
-            return {};
-        result = (result << 8u) + m_streamer.read_octet();
+    for (size_t i = 0; i < integer_length; i++) {
+        result = (result << 8u) + TRY(read_octet());
     }
     return result;
 }
 
-Optional<double> Reader::read_float_element()
+ErrorOr<double> Reader::Streamer::read_float()
 {
-    auto length = m_streamer.read_variable_size_integer();
-    if (!length.has_value() || m_streamer.remaining() < length.value())
-        return {};
+    auto length = TRY(read_variable_size_integer());
     if (length != 4u && length != 8u)
-        return {};
+        return Error::from_string_literal("Float size must be 4 or 8 bytes");
 
     union {
         u64 value;
@@ -537,24 +523,18 @@ Optional<double> Reader::read_float_element()
         double double_value;
     } read_data;
     read_data.value = 0;
-    for (size_t i = 0; i < length.value(); i++) {
-        if (!m_streamer.has_octet())
-            return {};
-        read_data.value = (read_data.value << 8u) + m_streamer.read_octet();
+    for (size_t i = 0; i < length; i++) {
+        read_data.value = (read_data.value << 8u) + TRY(read_octet());
     }
     if (length == 4u)
         return read_data.float_value;
     return read_data.double_value;
 }
 
-bool Reader::read_unknown_element()
+ErrorOr<void> Reader::Streamer::read_unknown_element()
 {
-    auto element_length = m_streamer.read_variable_size_integer();
-    if (!element_length.has_value() || m_streamer.remaining() < element_length.value())
-        return false;
-
-    m_streamer.drop_octets(element_length.value());
-    return true;
+    auto element_length = TRY(read_variable_size_integer());
+    return drop_octets(element_length);
 }
 
 }

Userland/Libraries/LibVideo/Containers/Matroska/Reader.h

@@ -1,17 +1,20 @@
 /*
  * Copyright (c) 2021, Hunter Salyer <thefalsehonesty@gmail.com>
+ * Copyright (c) 2022, Gregory Bertilson <Zaggy1024@gmail.com>
  *
  * SPDX-License-Identifier: BSD-2-Clause
  */
 
 #pragma once
 
-#include "Document.h"
 #include <AK/Debug.h>
 #include <AK/IntegralMath.h>
 #include <AK/NonnullOwnPtrVector.h>
 #include <AK/Optional.h>
 #include <AK/OwnPtr.h>
+#include <LibVideo/DecoderError.h>
+
+#include "Document.h"
 
 namespace Video::Matroska {
 
@@ -22,10 +25,10 @@ public:
     {
     }
 
-    static OwnPtr<MatroskaDocument> parse_matroska_from_file(StringView path);
-    static OwnPtr<MatroskaDocument> parse_matroska_from_data(u8 const*, size_t);
+    static DecoderErrorOr<NonnullOwnPtr<MatroskaDocument>> parse_matroska_from_file(StringView path);
+    static DecoderErrorOr<NonnullOwnPtr<MatroskaDocument>> parse_matroska_from_data(u8 const*, size_t);
 
-    OwnPtr<MatroskaDocument> parse();
+    DecoderErrorOr<NonnullOwnPtr<MatroskaDocument>> parse();
 
 private:
     class Streamer {
@@ -40,19 +43,6 @@ private:
 
         char const* data_as_chars() { return reinterpret_cast<char const*>(m_data_ptr); }
 
-        u8 read_octet()
-        {
-            VERIFY(m_size_remaining >= 1);
-            m_size_remaining--;
-            m_octets_read.last()++;
-            return *(m_data_ptr++);
-        }
-
-        i16 read_i16()
-        {
-            return (read_octet() << 8) | read_octet();
-        }
-
         size_t octets_read() { return m_octets_read.last(); }
 
         void push_octets_read() { m_octets_read.append(0); }
@@ -64,81 +54,23 @@ private:
                 m_octets_read.last() += popped;
         }
 
-        Optional<u64> read_variable_size_integer(bool mask_length = true)
-        {
-            dbgln_if(MATROSKA_TRACE_DEBUG, "Reading from offset {:p}", m_data_ptr);
-            if (!has_octet()) {
-                dbgln_if(MATROSKA_TRACE_DEBUG, "Ran out of stream data");
-                return {};
-            }
-            auto length_descriptor = read_octet();
-            dbgln_if(MATROSKA_TRACE_DEBUG, "Reading VINT, first byte is {:#02x}", length_descriptor);
-            if (length_descriptor == 0)
-                return {};
-            size_t length = 0;
-            while (length < 8) {
-                if (length_descriptor & (1u << (8 - length)))
-                    break;
-                length++;
-            }
-            dbgln_if(MATROSKA_TRACE_DEBUG, "Reading VINT of total length {}", length);
-            if (length > 8)
-                return {};
-
-            u64 result;
-            if (mask_length)
-                result = length_descriptor & ~(1u << (8 - length));
-            else
-                result = length_descriptor;
-            dbgln_if(MATROSKA_TRACE_DEBUG, "Beginning of VINT is {:#02x}", result);
-            for (size_t i = 1; i < length; i++) {
-                if (!has_octet()) {
-                    dbgln_if(MATROSKA_TRACE_DEBUG, "Ran out of stream data");
-                    return {};
-                }
-                u8 next_octet = read_octet();
-                dbgln_if(MATROSKA_TRACE_DEBUG, "Read octet of {:#02x}", next_octet);
-                result = (result << 8u) | next_octet;
-                dbgln_if(MATROSKA_TRACE_DEBUG, "New result is {:#010x}", result);
-            }
-            return result;
-        }
+        ErrorOr<u8> read_octet();
 
-        Optional<i64> read_variable_sized_signed_integer()
-        {
-            auto length_descriptor = read_octet();
-            if (length_descriptor == 0)
-                return {};
-            size_t length = 0;
-            while (length < 8) {
-                if (length_descriptor & (1u << (8 - length)))
-                    break;
-                length++;
-            }
-            if (length > 8)
-                return {};
-
-            i64 result = length_descriptor & ~(1u << (8 - length));
-            for (size_t i = 1; i < length; i++) {
-                if (!has_octet()) {
-                    return {};
-                }
-                u8 next_octet = read_octet();
-                result = (result << 8u) | next_octet;
-            }
-            result -= AK::exp2<i64>(length * 7 - 1) - 1;
-            return result;
-        }
+        ErrorOr<i16> read_i16();
 
-        void drop_octets(size_t num_octets)
-        {
-            VERIFY(m_size_remaining >= num_octets);
-            m_size_remaining -= num_octets;
-            m_octets_read.last() += num_octets;
-            m_data_ptr += num_octets;
-        }
+        ErrorOr<u64> read_variable_size_integer(bool mask_length = true);
+        ErrorOr<i64> read_variable_size_signed_integer();
 
-        bool at_end() const { return !m_size_remaining; }
+        ErrorOr<u64> read_u64();
+        ErrorOr<double> read_float();
+
+        ErrorOr<String> read_string();
+
+        ErrorOr<void> read_unknown_element();
+
+        ErrorOr<void> drop_octets(size_t num_octets);
+
+        bool at_end() const { return m_size_remaining == 0; }
         bool has_octet() const { return m_size_remaining >= 1; }
 
         size_t remaining() const { return m_size_remaining; }
@@ -150,24 +82,19 @@ private:
         Vector<size_t> m_octets_read { 0 };
     };
 
-    bool parse_master_element(StringView element_name, Function<bool(u64 element_id)> element_consumer);
-    Optional<EBMLHeader> parse_ebml_header();
-
-    bool parse_segment_elements(MatroskaDocument&);
-    OwnPtr<SegmentInformation> parse_information();
+    DecoderErrorOr<void> parse_master_element(StringView element_name, Function<DecoderErrorOr<void>(u64 element_id)> element_consumer);
+    DecoderErrorOr<EBMLHeader> parse_ebml_header();
 
-    bool parse_tracks(MatroskaDocument&);
-    OwnPtr<TrackEntry> parse_track_entry();
-    Optional<TrackEntry::VideoTrack> parse_video_track_information();
-    Optional<TrackEntry::ColorFormat> parse_video_color_information();
-    Optional<TrackEntry::AudioTrack> parse_audio_track_information();
-    OwnPtr<Cluster> parse_cluster();
-    OwnPtr<Block> parse_simple_block();
+    DecoderErrorOr<void> parse_segment_elements(MatroskaDocument&);
+    DecoderErrorOr<NonnullOwnPtr<SegmentInformation>> parse_information();
 
-    Optional<String> read_string_element();
-    Optional<u64> read_u64_element();
-    Optional<double> read_float_element();
-    bool read_unknown_element();
+    DecoderErrorOr<void> parse_tracks(MatroskaDocument&);
+    DecoderErrorOr<NonnullOwnPtr<TrackEntry>> parse_track_entry();
+    DecoderErrorOr<TrackEntry::VideoTrack> parse_video_track_information();
+    DecoderErrorOr<TrackEntry::ColorFormat> parse_video_color_information();
+    DecoderErrorOr<TrackEntry::AudioTrack> parse_audio_track_information();
+    DecoderErrorOr<NonnullOwnPtr<Cluster>> parse_cluster();
+    DecoderErrorOr<NonnullOwnPtr<Block>> parse_simple_block();
 
     Streamer m_streamer;
 };

Userland/Utilities/matroska.cpp

@@ -10,10 +10,12 @@
 
 ErrorOr<int> serenity_main(Main::Arguments)
 {
-    auto document = Video::Matroska::Reader::parse_matroska_from_file("/home/anon/Videos/test-webm.webm"sv);
-    if (!document) {
+    auto document_result = Video::Matroska::Reader::parse_matroska_from_file("/home/anon/Videos/test-webm.webm"sv);
+    if (document_result.is_error()) {
+        outln("Encountered an error during parsing: {}", document_result.release_error().string_literal());
         return Error::from_string_literal("Failed to parse :(");
     }
+    auto document = document_result.release_value();
 
     outln("DocType is {}", document->header().doc_type.characters());
     outln("DocTypeVersion is {}", document->header().doc_type_version);