Skip to content

Commit

Permalink
Preserve limit when resetting ParsableByteArray in OggPacket#populate
Browse files Browse the repository at this point in the history
When I moved ParsableByteArray#data behind a getter I replaced some
assignments with calls to reset(byte[]):
ce2e6e2

reset(byte[]) deliberately sets `limit` to `data.length`, in order to
handle cases that were reassigning `data` but not updating `limit`.
However OggPacket was already using `limit` to track where to write
'new' data into the array, so changing `limit` to `data.length` caused
us to try and write new data beyond the end of the array.

I looked at other uses of reset(byte[]) in ce2e6e2
and condluded the only other usage in MatroskaExtractor is legit and
shouldn't be updated like this (because MatroskaExtractor previously
*wasn't* correctly updating/maintaining `limit`).

Issue: #7992
PiperOrigin-RevId: 334601586
  • Loading branch information
icbaker authored and ojw28 committed Oct 17, 2020
1 parent f3767b3 commit bd312ec
Show file tree
Hide file tree
Showing 9 changed files with 2,194 additions and 4 deletions.
2 changes: 2 additions & 0 deletions RELEASENOTES.md
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@
([#7967](https:/google/ExoPlayer/issues/7967)).
* Use TLEN ID3 tag to compute the duration in Mp3Extractor
([#7949](https:/google/ExoPlayer/issues/7949)).
* Fix regression for Ogg files with packets that span multiple pages
([#7992](https:/google/ExoPlayer/issues/7992)).
* UI
* Add the option to sort tracks by `Format` in `TrackSelectionView` and
`TrackSelectionDialogBuilder`
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,9 @@ public boolean populate(ExtractorInput input) throws IOException {
int segmentIndex = currentSegmentIndex + segmentCount;
if (size > 0) {
if (packetArray.capacity() < packetArray.limit() + size) {
packetArray.reset(Arrays.copyOf(packetArray.getData(), packetArray.limit() + size));
packetArray.reset(
Arrays.copyOf(packetArray.getData(), packetArray.limit() + size),
/* limit= */ packetArray.limit());
}
input.readFully(packetArray.getData(), packetArray.limit(), size);
packetArray.setLimit(packetArray.limit() + size);
Expand Down Expand Up @@ -131,7 +133,8 @@ public void trimPayload() {
}
packetArray.reset(
Arrays.copyOf(
packetArray.getData(), max(OggPageHeader.MAX_PAGE_PAYLOAD, packetArray.limit())));
packetArray.getData(), max(OggPageHeader.MAX_PAGE_PAYLOAD, packetArray.limit())),
/* limit= */ packetArray.limit());
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,11 +60,27 @@ public void vorbis() throws Exception {
OggExtractor::new, "media/ogg/bear_vorbis.ogg", simulationConfig);
}

// Ensure the extractor can handle non-contiguous pages by using a file with 10 bytes of garbage
// data before the start of the second page.
/**
* Ensure the extractor can handle non-contiguous pages by using a file with 10 bytes of garbage
* data before the start of the second page.
*
* <p>https:/google/ExoPlayer/issues/7230
*/
@Test
public void vorbisWithGapBeforeSecondPage() throws Exception {
ExtractorAsserts.assertBehavior(
OggExtractor::new, "media/ogg/bear_vorbis_gap.ogg", simulationConfig);
}

/**
* Use some very large Vorbis Comment metadata to create a packet that is larger than a single Ogg
* page.
*
* <p>https:/google/ExoPlayer/issues/7992
*/
@Test
public void vorbisWithPacketSpanningBetweenPages() throws Exception {
ExtractorAsserts.assertBehavior(
OggExtractor::new, "media/ogg/bear_vorbis_with_large_metadata.ogg", simulationConfig);
}
}
Loading

0 comments on commit bd312ec

Please sign in to comment.