Skip to content

SDK / exoplayer

Go to a project
Commit 3c8c5a33 by olly Committed by Oliver Woodman
Options

Fix DefaultOggSeeker seeking 

- When in STATE_SEEK with targetGranule==0, seeking would exit
  without checking that the input was positioned at the correct
  place.
- Seeking could fail due to trying to read beyond the end of the
  stream.
- Seeking was not robust against IO errors during the skip phase
  that occurs after the binary search has sufficiently converged.

PiperOrigin-RevId: 261317035
parent 80bc50b6
Expand all
Inline Side-by-side
Showing with 53 additions and 153 deletions
library/core/src/main/java/com/google/android/exoplayer2/extractor/ogg/StreamReader.java
...@@ -148,9 +148,9 @@ import java.io.IOException; ...@@ -148,9 +148,9 @@ import java.io.IOException;
boolean isLastPage = (firstPayloadPageHeader.type & 0x04) != 0; // Type 4 is end of stream. boolean isLastPage = (firstPayloadPageHeader.type & 0x04) != 0; // Type 4 is end of stream.
oggSeeker = oggSeeker =
new DefaultOggSeeker( new DefaultOggSeeker(
this,
payloadStartPosition, payloadStartPosition,
input.getLength(), input.getLength(),
this,
firstPayloadPageHeader.headerSize + firstPayloadPageHeader.bodySize, firstPayloadPageHeader.headerSize + firstPayloadPageHeader.bodySize,
firstPayloadPageHeader.granulePosition, firstPayloadPageHeader.granulePosition,
isLastPage); isLastPage);
......
library/core/src/test/java/com/google/android/exoplayer2/extractor/ogg/DefaultOggSeekerTest.java
...@@ -16,7 +16,6 @@ ...@@ -16,7 +16,6 @@
package com.google.android.exoplayer2.extractor.ogg; package com.google.android.exoplayer2.extractor.ogg;
import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.Truth.assertWithMessage;
import static org.junit.Assert.fail; import static org.junit.Assert.fail;
import androidx.test.ext.junit.runners.AndroidJUnit4; import androidx.test.ext.junit.runners.AndroidJUnit4;
...@@ -36,9 +35,9 @@ public final class DefaultOggSeekerTest { ...@@ -36,9 +35,9 @@ public final class DefaultOggSeekerTest {
public void testSetupWithUnsetEndPositionFails() { public void testSetupWithUnsetEndPositionFails() {
try { try {
new DefaultOggSeeker( new DefaultOggSeeker(
/* startPosition= */ 0,
/* endPosition= */ C.LENGTH_UNSET,
/* streamReader= */ new TestStreamReader(), /* streamReader= */ new TestStreamReader(),
/* payloadStartPosition= */ 0,
/* payloadEndPosition= */ C.LENGTH_UNSET,
/* firstPayloadPageSize= */ 1, /* firstPayloadPageSize= */ 1,
/* firstPayloadPageGranulePosition= */ 1, /* firstPayloadPageGranulePosition= */ 1,
/* firstPayloadPageIsLastPage= */ false); /* firstPayloadPageIsLastPage= */ false);
...@@ -62,9 +61,9 @@ public final class DefaultOggSeekerTest { ...@@ -62,9 +61,9 @@ public final class DefaultOggSeekerTest {
TestStreamReader streamReader = new TestStreamReader(); TestStreamReader streamReader = new TestStreamReader();
DefaultOggSeeker oggSeeker = DefaultOggSeeker oggSeeker =
new DefaultOggSeeker( new DefaultOggSeeker(
/* startPosition= */ 0,
/* endPosition= */ testFile.data.length,
/* streamReader= */ streamReader, /* streamReader= */ streamReader,
/* payloadStartPosition= */ 0,
/* payloadEndPosition= */ testFile.data.length,
/* firstPayloadPageSize= */ testFile.firstPayloadPageSize, /* firstPayloadPageSize= */ testFile.firstPayloadPageSize,
/* firstPayloadPageGranulePosition= */ testFile.firstPayloadPageGranulePosition, /* firstPayloadPageGranulePosition= */ testFile.firstPayloadPageGranulePosition,
/* firstPayloadPageIsLastPage= */ false); /* firstPayloadPageIsLastPage= */ false);
...@@ -78,70 +77,56 @@ public final class DefaultOggSeekerTest { ...@@ -78,70 +77,56 @@ public final class DefaultOggSeekerTest {
input.setPosition((int) nextSeekPosition); input.setPosition((int) nextSeekPosition);
} }
// Test granule 0 from file start // Test granule 0 from file start.
assertThat(seekTo(input, oggSeeker, 0, 0)).isEqualTo(0); long granule = seekTo(input, oggSeeker, 0, 0);
assertThat(granule).isEqualTo(0);
assertThat(input.getPosition()).isEqualTo(0); assertThat(input.getPosition()).isEqualTo(0);
// Test granule 0 from file end // Test granule 0 from file end.
assertThat(seekTo(input, oggSeeker, 0, testFile.data.length - 1)).isEqualTo(0); granule = seekTo(input, oggSeeker, 0, testFile.data.length - 1);
assertThat(granule).isEqualTo(0);
assertThat(input.getPosition()).isEqualTo(0); assertThat(input.getPosition()).isEqualTo(0);
{ // Test last granule // Test last granule.
long currentGranule = seekTo(input, oggSeeker, testFile.lastGranule, 0); granule = seekTo(input, oggSeeker, testFile.lastGranule, 0);
long position = testFile.data.length; long position = testFile.data.length;
assertThat( // TODO: Simplify this.
(testFile.lastGranule > currentGranule && position > input.getPosition()) assertThat(
|| (testFile.lastGranule == currentGranule && position == input.getPosition())) (testFile.lastGranule > granule && position > input.getPosition())
.isTrue(); || (testFile.lastGranule == granule && position == input.getPosition()))
} .isTrue();
{ // Test exact granule // Test exact granule.
input.setPosition(testFile.data.length / 2); input.setPosition(testFile.data.length / 2);
oggSeeker.skipToNextPage(input); oggSeeker.skipToNextPage(input);
assertThat(pageHeader.populate(input, true)).isTrue(); assertThat(pageHeader.populate(input, true)).isTrue();
long position = input.getPosition() + pageHeader.headerSize + pageHeader.bodySize; position = input.getPosition() + pageHeader.headerSize + pageHeader.bodySize;
long currentGranule = seekTo(input, oggSeeker, pageHeader.granulePosition, 0); granule = seekTo(input, oggSeeker, pageHeader.granulePosition, 0);
assertThat( // TODO: Simplify this.
(pageHeader.granulePosition > currentGranule && position > input.getPosition()) assertThat(
|| (pageHeader.granulePosition == currentGranule (pageHeader.granulePosition > granule && position > input.getPosition())
&& position == input.getPosition())) || (pageHeader.granulePosition == granule && position == input.getPosition()))
.isTrue(); .isTrue();
}
for (int i = 0; i < 100; i += 1) { for (int i = 0; i < 100; i += 1) {
long targetGranule = (long) (random.nextDouble() * testFile.lastGranule); long targetGranule = (long) (random.nextDouble() * testFile.lastGranule);
int initialPosition = random.nextInt(testFile.data.length); int initialPosition = random.nextInt(testFile.data.length);
granule = seekTo(input, oggSeeker, targetGranule, initialPosition);
long currentGranule = seekTo(input, oggSeeker, targetGranule, initialPosition);
long currentPosition = input.getPosition(); long currentPosition = input.getPosition();
if (granule == 0) {
assertWithMessage("getNextSeekPosition() didn't leave input on a page start.")
.that(pageHeader.populate(input, true))
.isTrue();
if (currentGranule == 0) {
assertThat(currentPosition).isEqualTo(0); assertThat(currentPosition).isEqualTo(0);
} else { } else {
int previousPageStart = testFile.findPreviousPageStart(currentPosition); int previousPageStart = testFile.findPreviousPageStart(currentPosition);
input.setPosition(previousPageStart); input.setPosition(previousPageStart);
assertThat(pageHeader.populate(input, true)).isTrue(); pageHeader.populate(input, false);
assertThat(currentGranule).isEqualTo(pageHeader.granulePosition); assertThat(granule).isEqualTo(pageHeader.granulePosition);
} }
input.setPosition((int) currentPosition); input.setPosition((int) currentPosition);
oggSeeker.skipToPageOfGranule(input, targetGranule, -1); pageHeader.populate(input, false);
long positionDiff = Math.abs(input.getPosition() - currentPosition); // The target granule should be within the current page.
assertThat(granule).isAtMost(targetGranule);
long granuleDiff = currentGranule - targetGranule; assertThat(targetGranule).isLessThan(pageHeader.granulePosition);
if ((granuleDiff > DefaultOggSeeker.MATCH_RANGE || granuleDiff < 0)
&& positionDiff > DefaultOggSeeker.MATCH_BYTE_RANGE) {
fail(
"granuleDiff ("
+ granuleDiff
+ ") or positionDiff ("
+ positionDiff
+ ") is more than allowed.");
}
} }
} }
...@@ -149,18 +134,15 @@ public final class DefaultOggSeekerTest { ...@@ -149,18 +134,15 @@ public final class DefaultOggSeekerTest {
FakeExtractorInput input, DefaultOggSeeker oggSeeker, long targetGranule, int initialPosition) FakeExtractorInput input, DefaultOggSeeker oggSeeker, long targetGranule, int initialPosition)
throws IOException, InterruptedException { throws IOException, InterruptedException {
long nextSeekPosition = initialPosition; long nextSeekPosition = initialPosition;
oggSeeker.startSeek(targetGranule);
int count = 0; int count = 0;
oggSeeker.resetSeeking(); while (nextSeekPosition >= 0) {
do {
input.setPosition((int) nextSeekPosition);
nextSeekPosition = oggSeeker.getNextSeekPosition(targetGranule, input);
if (count++ > 100) { if (count++ > 100) {
fail("infinite loop?"); fail("Seek failed to converge in 100 iterations");
} }
} while (nextSeekPosition >= 0); input.setPosition((int) nextSeekPosition);
nextSeekPosition = oggSeeker.read(input);
}
return -(nextSeekPosition + 2); return -(nextSeekPosition + 2);
} }
...@@ -171,8 +153,7 @@ public final class DefaultOggSeekerTest { ...@@ -171,8 +153,7 @@ public final class DefaultOggSeekerTest {
} }
@Override @Override
protected boolean readHeaders(ParsableByteArray packet, long position, SetupData setupData) protected boolean readHeaders(ParsableByteArray packet, long position, SetupData setupData) {
throws IOException, InterruptedException {
return false; return false;
} }
} }
......
library/core/src/test/java/com/google/android/exoplayer2/extractor/ogg/DefaultOggSeekerUtilMethodsTest.java
...@@ -85,9 +85,9 @@ public final class DefaultOggSeekerUtilMethodsTest { ...@@ -85,9 +85,9 @@ public final class DefaultOggSeekerUtilMethodsTest {
throws IOException, InterruptedException { throws IOException, InterruptedException {
DefaultOggSeeker oggSeeker = DefaultOggSeeker oggSeeker =
new DefaultOggSeeker( new DefaultOggSeeker(
/* startPosition= */ 0,
/* endPosition= */ extractorInput.getLength(),
/* streamReader= */ new FlacReader(), /* streamReader= */ new FlacReader(),
/* payloadStartPosition= */ 0,
/* payloadEndPosition= */ extractorInput.getLength(),
/* firstPayloadPageSize= */ 1, /* firstPayloadPageSize= */ 1,
/* firstPayloadPageGranulePosition= */ 2, /* firstPayloadPageGranulePosition= */ 2,
/* firstPayloadPageIsLastPage= */ false); /* firstPayloadPageIsLastPage= */ false);
...@@ -100,87 +100,6 @@ public final class DefaultOggSeekerUtilMethodsTest { ...@@ -100,87 +100,6 @@ public final class DefaultOggSeekerUtilMethodsTest {
} }
@Test @Test
public void testSkipToPageOfGranule() throws IOException, InterruptedException {
byte[] packet = TestUtil.buildTestData(3 * 254, random);
byte[] data = TestUtil.joinByteArrays(
OggTestData.buildOggHeader(0x01, 20000, 1000, 0x03),
TestUtil.createByteArray(254, 254, 254), // Laces.
packet,
OggTestData.buildOggHeader(0x04, 40000, 1001, 0x03),
TestUtil.createByteArray(254, 254, 254), // Laces.
packet,
OggTestData.buildOggHeader(0x04, 60000, 1002, 0x03),
TestUtil.createByteArray(254, 254, 254), // Laces.
packet);
FakeExtractorInput input = new FakeExtractorInput.Builder().setData(data).build();
// expect to be granule of the previous page returned as elapsedSamples
skipToPageOfGranule(input, 54000, 40000);
// expect to be at the start of the third page
assertThat(input.getPosition()).isEqualTo(2 * (30 + (3 * 254)));
}
@Test
public void testSkipToPageOfGranulePreciseMatch() throws IOException, InterruptedException {
byte[] packet = TestUtil.buildTestData(3 * 254, random);
byte[] data = TestUtil.joinByteArrays(
OggTestData.buildOggHeader(0x01, 20000, 1000, 0x03),
TestUtil.createByteArray(254, 254, 254), // Laces.
packet,
OggTestData.buildOggHeader(0x04, 40000, 1001, 0x03),
TestUtil.createByteArray(254, 254, 254), // Laces.
packet,
OggTestData.buildOggHeader(0x04, 60000, 1002, 0x03),
TestUtil.createByteArray(254, 254, 254), // Laces.
packet);
FakeExtractorInput input = new FakeExtractorInput.Builder().setData(data).build();
skipToPageOfGranule(input, 40000, 20000);
// expect to be at the start of the second page
assertThat(input.getPosition()).isEqualTo(30 + (3 * 254));
}
@Test
public void testSkipToPageOfGranuleAfterTargetPage() throws IOException, InterruptedException {
byte[] packet = TestUtil.buildTestData(3 * 254, random);
byte[] data = TestUtil.joinByteArrays(
OggTestData.buildOggHeader(0x01, 20000, 1000, 0x03),
TestUtil.createByteArray(254, 254, 254), // Laces.
packet,
OggTestData.buildOggHeader(0x04, 40000, 1001, 0x03),
TestUtil.createByteArray(254, 254, 254), // Laces.
packet,
OggTestData.buildOggHeader(0x04, 60000, 1002, 0x03),
TestUtil.createByteArray(254, 254, 254), // Laces.
packet);
FakeExtractorInput input = new FakeExtractorInput.Builder().setData(data).build();
skipToPageOfGranule(input, 10000, -1);
assertThat(input.getPosition()).isEqualTo(0);
}
private void skipToPageOfGranule(ExtractorInput input, long granule,
long elapsedSamplesExpected) throws IOException, InterruptedException {
DefaultOggSeeker oggSeeker =
new DefaultOggSeeker(
/* startPosition= */ 0,
/* endPosition= */ input.getLength(),
/* streamReader= */ new FlacReader(),
/* firstPayloadPageSize= */ 1,
/* firstPayloadPageGranulePosition= */ 2,
/* firstPayloadPageIsLastPage= */ false);
while (true) {
try {
assertThat(oggSeeker.skipToPageOfGranule(input, granule, -1))
.isEqualTo(elapsedSamplesExpected);
return;
} catch (FakeExtractorInput.SimulatedIOException e) {
input.resetPeekPosition();
}
}
}
@Test
public void testReadGranuleOfLastPage() throws IOException, InterruptedException { public void testReadGranuleOfLastPage() throws IOException, InterruptedException {
FakeExtractorInput input = OggTestData.createInput(TestUtil.joinByteArrays( FakeExtractorInput input = OggTestData.createInput(TestUtil.joinByteArrays(
TestUtil.buildTestData(100, random), TestUtil.buildTestData(100, random),
...@@ -204,7 +123,7 @@ public final class DefaultOggSeekerUtilMethodsTest { ...@@ -204,7 +123,7 @@ public final class DefaultOggSeekerUtilMethodsTest {
assertReadGranuleOfLastPage(input, 60000); assertReadGranuleOfLastPage(input, 60000);
fail(); fail();
} catch (EOFException e) { } catch (EOFException e) {
// ignored // Ignored.
} }
} }
...@@ -216,7 +135,7 @@ public final class DefaultOggSeekerUtilMethodsTest { ...@@ -216,7 +135,7 @@ public final class DefaultOggSeekerUtilMethodsTest {
assertReadGranuleOfLastPage(input, 60000); assertReadGranuleOfLastPage(input, 60000);
fail(); fail();
} catch (IllegalArgumentException e) { } catch (IllegalArgumentException e) {
// ignored // Ignored.
} }
} }
...@@ -224,9 +143,9 @@ public final class DefaultOggSeekerUtilMethodsTest { ...@@ -224,9 +143,9 @@ public final class DefaultOggSeekerUtilMethodsTest {
throws IOException, InterruptedException { throws IOException, InterruptedException {
DefaultOggSeeker oggSeeker = DefaultOggSeeker oggSeeker =
new DefaultOggSeeker( new DefaultOggSeeker(
/* startPosition= */ 0,
/* endPosition= */ input.getLength(),
/* streamReader= */ new FlacReader(), /* streamReader= */ new FlacReader(),
/* payloadStartPosition= */ 0,
/* payloadEndPosition= */ input.getLength(),
/* firstPayloadPageSize= */ 1, /* firstPayloadPageSize= */ 1,
/* firstPayloadPageGranulePosition= */ 2, /* firstPayloadPageGranulePosition= */ 2,
/* firstPayloadPageIsLastPage= */ false); /* firstPayloadPageIsLastPage= */ false);
...@@ -235,7 +154,7 @@ public final class DefaultOggSeekerUtilMethodsTest { ...@@ -235,7 +154,7 @@ public final class DefaultOggSeekerUtilMethodsTest {
assertThat(oggSeeker.readGranuleOfLastPage(input)).isEqualTo(expected); assertThat(oggSeeker.readGranuleOfLastPage(input)).isEqualTo(expected);
break; break;
} catch (FakeExtractorInput.SimulatedIOException e) { } catch (FakeExtractorInput.SimulatedIOException e) {
// ignored // Ignored.
} }
} }
} }
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or sign in to comment