LibMedia: Track Matroska master element ends with position()

We don't actually need a Vector stack of bytes read for each element
we're reading out of a Matroska file, we already have the C++ stack
in which we can store the start and end of the master elements we're
reading.

This fixes an issue where seeks while parsing master elements would not
increment m_octets_read, so the master element could continue reading
further than intended.

This could cause a BlockGroup followed by a SimpleBlock to read as if
the BlockGroup contained the SimpleBlock, meaning that SampleIterator
would skip the SimpleBlock.

A test is added to ensure this doesn't regress again.

Libraries/LibMedia/Containers/Matroska/Reader.cpp

@@ -117,9 +117,9 @@ static DecoderErrorOr<size_t> parse_master_element(Streamer& streamer, [[maybe_u
 
     bool first_element = true;
     auto first_element_position = streamer.position();
+    auto element_data_end = first_element_position + element_data_size;
 
-    streamer.push_octets_read();
-    while (streamer.octets_read() < element_data_size) {
+    while (streamer.position() < element_data_end) {
         dbgln_if(MATROSKA_TRACE_DEBUG, "====== Reading  element ======");
         auto element_id = TRY(streamer.read_variable_size_integer(false));
         dbgln_if(MATROSKA_TRACE_DEBUG, "{:s} element ID is {:#010x}", element_name, element_id);
@@ -160,14 +160,13 @@ static DecoderErrorOr<size_t> parse_master_element(Streamer& streamer, [[maybe_u
         if (result.value() == ElementIterationDecision::BreakHere)
             break;
         if (result.value() == ElementIterationDecision::BreakAtEnd) {
-            TRY(streamer.seek_to_position(first_element_position + element_data_size));
+            TRY(streamer.seek_to_position(element_data_end));
             break;
         }
 
-        dbgln_if(MATROSKA_TRACE_DEBUG, "Read {} octets of the {} so far.", streamer.octets_read(), element_name);
+        dbgln_if(MATROSKA_TRACE_DEBUG, "Read {} octets of the {} so far.", streamer.position() - first_element_position, element_name);
         first_element = false;
     }
-    streamer.pop_octets_read();
 
     return first_element_position;
 }
@@ -772,7 +771,7 @@ DecoderErrorOr<Vector<ByteBuffer>> SampleIterator::get_frames(Block block)
     Vector<ByteBuffer> frames;
 
     if (block.lacing() == Block::Lacing::EBML) {
-        auto octets_read_before_frame_sizes = streamer.octets_read();
+        auto frames_start_position = streamer.position();
         auto frame_count = TRY(streamer.read_octet()) + 1;
         Vector<u64> frame_sizes;
         frame_sizes.ensure_capacity(frame_count);
@@ -796,7 +795,7 @@ DecoderErrorOr<Vector<ByteBuffer>> SampleIterator::get_frames(Block block)
             frame_size_sum += frame_size;
             previous_frame_size = frame_size;
         }
-        frame_sizes.append(block.data_size() - frame_size_sum - (streamer.octets_read() - octets_read_before_frame_sizes));
+        frame_sizes.append(block.data_size() - frame_size_sum - (streamer.position() - frames_start_position));
 
         for (int i = 0; i < frame_count; i++) {
             // FIXME: ReadonlyBytes instead of copying the frame data?
@@ -809,7 +808,7 @@ DecoderErrorOr<Vector<ByteBuffer>> SampleIterator::get_frames(Block block)
         for (int i = 0; i < frame_count; i++)
             frames.append(TRY(streamer.read_raw_octets(individual_frame_size)));
     } else if (block.lacing() == Block::Lacing::XIPH) {
-        auto frames_start_position = streamer.octets_read();
+        auto frames_start_position = streamer.position();
 
         auto frame_count_minus_one = TRY(streamer.read_octet());
         frames.ensure_capacity(frame_count_minus_one + 1);
@@ -829,7 +828,7 @@ DecoderErrorOr<Vector<ByteBuffer>> SampleIterator::get_frames(Block block)
 
         for (auto i = 0; i < frame_count_minus_one; i++)
             frames.append(TRY(streamer.read_raw_octets(frame_sizes[i])));
-        frames.append(TRY(streamer.read_raw_octets(block.data_size() - (streamer.octets_read() - frames_start_position))));
+        frames.append(TRY(streamer.read_raw_octets(block.data_size() - (streamer.position() - frames_start_position))));
     } else {
         frames.append(TRY(streamer.read_raw_octets(block.data_size())));
     }
@@ -1250,7 +1249,6 @@ DecoderErrorOr<u8> Streamer::read_octet()
     u8 result;
     Bytes bytes { &result, 1 };
     TRY(m_stream_cursor->read_into(bytes));
-    m_octets_read.last()++;
     return bytes[0];
 }
 
@@ -1319,7 +1317,6 @@ DecoderErrorOr<ByteBuffer> Streamer::read_raw_octets(size_t num_octets)
     auto result = MUST(ByteBuffer::create_uninitialized(num_octets));
     auto bytes = result.bytes();
     TRY(m_stream_cursor->read_into(bytes));
-    m_octets_read.last() += num_octets;
     return result;
 }
 
@@ -1358,7 +1355,6 @@ DecoderErrorOr<void> Streamer::read_unknown_element()
     auto element_length = TRY(read_variable_size_integer());
     dbgln_if(MATROSKA_TRACE_DEBUG, "Skipping unknown element of size {}.", element_length);
     TRY(m_stream_cursor->seek(element_length, IncrementallyPopulatedStream::Cursor::SeekMode::FromCurrentPosition));
-    m_octets_read.last() += element_length;
     return {};
 }
 

Libraries/LibMedia/Containers/Matroska/Reader.h

@@ -128,17 +128,6 @@ public:
     {
     }
 
-    size_t octets_read() { return m_octets_read.last(); }
-
-    void push_octets_read() { m_octets_read.append(0); }
-
-    void pop_octets_read()
-    {
-        auto popped = m_octets_read.take_last();
-        if (!m_octets_read.is_empty())
-            m_octets_read.last() += popped;
-    }
-
     DecoderErrorOr<u8> read_octet();
 
     DecoderErrorOr<i16> read_i16();
@@ -161,7 +150,6 @@ public:
 
 private:
     NonnullRefPtr<IncrementallyPopulatedStream::Cursor> m_stream_cursor;
-    Vector<size_t> m_octets_read { 0 };
 };
 
 }

Tests/LibMedia/TestParseMatroska.cpp

@@ -53,3 +53,26 @@ TEST_CASE(seek_in_multi_frame_blocks)
     EXPECT(coded_frame_after_backward_seek.timestamp() > AK::Duration::zero());
     EXPECT(coded_frame_after_backward_seek.timestamp() <= backward_seek_time);
 }
+
+TEST_CASE(block_group)
+{
+    auto file = MUST(Core::File::open("./test-matroska-block-group.mkv"sv, Core::File::OpenMode::Read));
+    auto stream = Media::IncrementallyPopulatedStream::create_from_buffer(MUST(file->read_until_eof()));
+    auto matroska_reader = MUST(Media::Matroska::Reader::from_stream(stream->create_cursor()));
+    u64 video_track = 0;
+    MUST(matroska_reader.for_each_track_of_type(Media::Matroska::TrackEntry::TrackType::Video, [&](Media::Matroska::TrackEntry const& track_entry) -> Media::DecoderErrorOr<IterationDecision> {
+        video_track = track_entry.track_number();
+        return IterationDecision::Break;
+    }));
+    VERIFY(video_track == 1);
+
+    auto iterator = MUST(matroska_reader.create_sample_iterator(stream->create_cursor(), video_track));
+
+    auto first_block = MUST(iterator.next_block());
+    EXPECT(first_block.duration().has_value());
+    EXPECT_EQ(first_block.duration()->to_milliseconds(), 33);
+
+    auto second_block = MUST(iterator.next_block());
+    EXPECT_EQ(second_block.timestamp().to_milliseconds(), 33);
+    EXPECT(second_block.only_keyframes());
+}