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
This commit is contained in:
olly 2019-08-02 15:20:35 +01:00 committed by Oliver Woodman
parent 90ab05c574
commit 91c62ea26f
4 changed files with 129 additions and 252 deletions

View File

@ -16,6 +16,7 @@
package com.google.android.exoplayer2.extractor.ogg;
import androidx.annotation.VisibleForTesting;
import com.google.android.exoplayer2.C;
import com.google.android.exoplayer2.ParserException;
import com.google.android.exoplayer2.extractor.ExtractorInput;
import com.google.android.exoplayer2.extractor.SeekMap;
@ -35,11 +36,12 @@ import java.io.IOException;
private static final int STATE_SEEK_TO_END = 0;
private static final int STATE_READ_LAST_PAGE = 1;
private static final int STATE_SEEK = 2;
private static final int STATE_IDLE = 3;
private static final int STATE_SKIP = 3;
private static final int STATE_IDLE = 4;
private final OggPageHeader pageHeader = new OggPageHeader();
private final long startPosition;
private final long endPosition;
private final long payloadStartPosition;
private final long payloadEndPosition;
private final StreamReader streamReader;
private int state;
@ -55,26 +57,27 @@ import java.io.IOException;
/**
* Constructs an OggSeeker.
*
* @param startPosition Start position of the payload (inclusive).
* @param endPosition End position of the payload (exclusive).
* @param streamReader The {@link StreamReader} that owns this seeker.
* @param payloadStartPosition Start position of the payload (inclusive).
* @param payloadEndPosition End position of the payload (exclusive).
* @param firstPayloadPageSize The total size of the first payload page, in bytes.
* @param firstPayloadPageGranulePosition The granule position of the first payload page.
* @param firstPayloadPageIsLastPage Whether the first payload page is also the last page in the
* ogg stream.
* @param firstPayloadPageIsLastPage Whether the first payload page is also the last page.
*/
public DefaultOggSeeker(
long startPosition,
long endPosition,
StreamReader streamReader,
long payloadStartPosition,
long payloadEndPosition,
long firstPayloadPageSize,
long firstPayloadPageGranulePosition,
boolean firstPayloadPageIsLastPage) {
Assertions.checkArgument(startPosition >= 0 && endPosition > startPosition);
Assertions.checkArgument(
payloadStartPosition >= 0 && payloadEndPosition > payloadStartPosition);
this.streamReader = streamReader;
this.startPosition = startPosition;
this.endPosition = endPosition;
if (firstPayloadPageSize == endPosition - startPosition || firstPayloadPageIsLastPage) {
this.payloadStartPosition = payloadStartPosition;
this.payloadEndPosition = payloadEndPosition;
if (firstPayloadPageSize == payloadEndPosition - payloadStartPosition
|| firstPayloadPageIsLastPage) {
totalGranules = firstPayloadPageGranulePosition;
state = STATE_IDLE;
} else {
@ -91,7 +94,7 @@ import java.io.IOException;
positionBeforeSeekToEnd = input.getPosition();
state = STATE_READ_LAST_PAGE;
// Seek to the end just before the last page of stream to get the duration.
long lastPageSearchPosition = endPosition - OggPageHeader.MAX_PAGE_SIZE;
long lastPageSearchPosition = payloadEndPosition - OggPageHeader.MAX_PAGE_SIZE;
if (lastPageSearchPosition > positionBeforeSeekToEnd) {
return lastPageSearchPosition;
}
@ -101,137 +104,110 @@ import java.io.IOException;
state = STATE_IDLE;
return positionBeforeSeekToEnd;
case STATE_SEEK:
long currentGranule;
if (targetGranule == 0) {
currentGranule = 0;
} else {
long position = getNextSeekPosition(targetGranule, input);
if (position >= 0) {
return position;
}
currentGranule = skipToPageOfGranule(input, targetGranule, -(position + 2));
long position = getNextSeekPosition(input);
if (position != C.POSITION_UNSET) {
return position;
}
state = STATE_SKIP;
// Fall through.
case STATE_SKIP:
skipToPageOfTargetGranule(input);
state = STATE_IDLE;
return -(currentGranule + 2);
return -(startGranule + 2);
default:
// Never happens.
throw new IllegalStateException();
}
}
@Override
public void startSeek(long targetGranule) {
Assertions.checkArgument(state == STATE_IDLE || state == STATE_SEEK);
this.targetGranule = targetGranule;
state = STATE_SEEK;
resetSeeking();
}
@Override
public OggSeekMap createSeekMap() {
return totalGranules != 0 ? new OggSeekMap() : null;
}
@VisibleForTesting
public void resetSeeking() {
start = startPosition;
end = endPosition;
@Override
public void startSeek(long targetGranule) {
this.targetGranule = targetGranule;
state = STATE_SEEK;
start = payloadStartPosition;
end = payloadEndPosition;
startGranule = 0;
endGranule = totalGranules;
}
/**
* Returns a position converging to the {@code targetGranule} to which the {@link ExtractorInput}
* has to seek and then be passed for another call until a negative number is returned. If a
* negative number is returned the input is at a position which is before the target page and at
* which it is sensible to just skip pages to the target granule and pre-roll instead of doing
* another seek request.
* Performs a single step of a seeking binary search, returning the byte position from which data
* should be provided for the next step, or {@link C#POSITION_UNSET} if the search has converged.
* If the search has converged then {@link #skipToPageOfTargetGranule(ExtractorInput)} should be
* called to skip to the target page.
*
* @param targetGranule The target granule position to seek to.
* @param input The {@link ExtractorInput} to read from.
* @return The position to seek the {@link ExtractorInput} to for a next call or -(currentGranule
* + 2) if it's close enough to skip to the target page.
* @return The byte position from which data should be provided for the next step, or {@link
* C#POSITION_UNSET} if the search has converged.
* @throws IOException If reading from the input fails.
* @throws InterruptedException If interrupted while reading from the input.
*/
@VisibleForTesting
public long getNextSeekPosition(long targetGranule, ExtractorInput input)
throws IOException, InterruptedException {
private long getNextSeekPosition(ExtractorInput input) throws IOException, InterruptedException {
if (start == end) {
return -(startGranule + 2);
return C.POSITION_UNSET;
}
long initialPosition = input.getPosition();
long currentPosition = input.getPosition();
if (!skipToNextPage(input, end)) {
if (start == initialPosition) {
if (start == currentPosition) {
throw new IOException("No ogg page can be found.");
}
return start;
}
pageHeader.populate(input, false);
pageHeader.populate(input, /* quiet= */ false);
input.resetPeekPosition();
long granuleDistance = targetGranule - pageHeader.granulePosition;
int pageSize = pageHeader.headerSize + pageHeader.bodySize;
if (granuleDistance < 0 || granuleDistance > MATCH_RANGE) {
if (granuleDistance < 0) {
end = initialPosition;
endGranule = pageHeader.granulePosition;
} else {
start = input.getPosition() + pageSize;
startGranule = pageHeader.granulePosition;
if (end - start + pageSize < MATCH_BYTE_RANGE) {
input.skipFully(pageSize);
return -(startGranule + 2);
}
}
if (end - start < MATCH_BYTE_RANGE) {
end = start;
return start;
}
long offset = pageSize * (granuleDistance <= 0 ? 2L : 1L);
long nextPosition = input.getPosition() - offset
+ (granuleDistance * (end - start) / (endGranule - startGranule));
nextPosition = Math.max(nextPosition, start);
nextPosition = Math.min(nextPosition, end - 1);
return nextPosition;
if (0 <= granuleDistance && granuleDistance < MATCH_RANGE) {
return C.POSITION_UNSET;
}
// position accepted (before target granule and within MATCH_RANGE)
input.skipFully(pageSize);
return -(pageHeader.granulePosition + 2);
if (granuleDistance < 0) {
end = currentPosition;
endGranule = pageHeader.granulePosition;
} else {
start = input.getPosition() + pageSize;
startGranule = pageHeader.granulePosition;
}
if (end - start < MATCH_BYTE_RANGE) {
end = start;
return start;
}
long offset = pageSize * (granuleDistance <= 0 ? 2L : 1L);
long nextPosition =
input.getPosition()
- offset
+ (granuleDistance * (end - start) / (endGranule - startGranule));
return Util.constrainValue(nextPosition, start, end - 1);
}
/**
* Skips to the position of the start of the page containing the {@code targetGranule} and returns
* the granule of the page previous to the target page.
* Skips forward to the start of the page containing the {@code targetGranule}.
*
* @param input The {@link ExtractorInput} to read from.
* @param targetGranule The target granule.
* @param currentGranule The current granule or -1 if it's unknown.
* @return The granule of the prior page or the {@code currentGranule} if there isn't a prior
* page.
* @throws ParserException If populating the page header fails.
* @throws IOException If reading from the input fails.
* @throws InterruptedException If interrupted while reading from the input.
*/
@VisibleForTesting
long skipToPageOfGranule(ExtractorInput input, long targetGranule, long currentGranule)
private void skipToPageOfTargetGranule(ExtractorInput input)
throws IOException, InterruptedException {
pageHeader.populate(input, false);
pageHeader.populate(input, /* quiet= */ false);
while (pageHeader.granulePosition < targetGranule) {
input.skipFully(pageHeader.headerSize + pageHeader.bodySize);
// Store in a member field to be able to resume after IOExceptions.
currentGranule = pageHeader.granulePosition;
// Peek next header.
pageHeader.populate(input, false);
start = input.getPosition();
startGranule = pageHeader.granulePosition;
pageHeader.populate(input, /* quiet= */ false);
}
input.resetPeekPosition();
return currentGranule;
}
/**
@ -244,7 +220,7 @@ import java.io.IOException;
*/
@VisibleForTesting
void skipToNextPage(ExtractorInput input) throws IOException, InterruptedException {
if (!skipToNextPage(input, endPosition)) {
if (!skipToNextPage(input, payloadEndPosition)) {
// Not found until eof.
throw new EOFException();
}
@ -261,7 +237,7 @@ import java.io.IOException;
*/
private boolean skipToNextPage(ExtractorInput input, long limit)
throws IOException, InterruptedException {
limit = Math.min(limit + 3, endPosition);
limit = Math.min(limit + 3, payloadEndPosition);
byte[] buffer = new byte[2048];
int peekLength = buffer.length;
while (true) {
@ -302,8 +278,8 @@ import java.io.IOException;
long readGranuleOfLastPage(ExtractorInput input) throws IOException, InterruptedException {
skipToNextPage(input);
pageHeader.reset();
while ((pageHeader.type & 0x04) != 0x04 && input.getPosition() < endPosition) {
pageHeader.populate(input, false);
while ((pageHeader.type & 0x04) != 0x04 && input.getPosition() < payloadEndPosition) {
pageHeader.populate(input, /* quiet= */ false);
input.skipFully(pageHeader.headerSize + pageHeader.bodySize);
}
return pageHeader.granulePosition;
@ -320,10 +296,11 @@ import java.io.IOException;
public SeekPoints getSeekPoints(long timeUs) {
long targetGranule = streamReader.convertTimeToGranule(timeUs);
long estimatedPosition =
startPosition
+ (targetGranule * (endPosition - startPosition) / totalGranules)
payloadStartPosition
+ (targetGranule * (payloadEndPosition - payloadStartPosition) / totalGranules)
- DEFAULT_OFFSET;
estimatedPosition = Util.constrainValue(estimatedPosition, startPosition, endPosition - 1);
estimatedPosition =
Util.constrainValue(estimatedPosition, payloadStartPosition, payloadEndPosition - 1);
return new SeekPoints(new SeekPoint(timeUs, estimatedPosition));
}

View File

@ -148,9 +148,9 @@ import java.io.IOException;
boolean isLastPage = (firstPayloadPageHeader.type & 0x04) != 0; // Type 4 is end of stream.
oggSeeker =
new DefaultOggSeeker(
this,
payloadStartPosition,
input.getLength(),
this,
firstPayloadPageHeader.headerSize + firstPayloadPageHeader.bodySize,
firstPayloadPageHeader.granulePosition,
isLastPage);

View File

@ -16,7 +16,6 @@
package com.google.android.exoplayer2.extractor.ogg;
import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.Truth.assertWithMessage;
import static org.junit.Assert.fail;
import androidx.test.ext.junit.runners.AndroidJUnit4;
@ -36,9 +35,9 @@ public final class DefaultOggSeekerTest {
public void testSetupWithUnsetEndPositionFails() {
try {
new DefaultOggSeeker(
/* startPosition= */ 0,
/* endPosition= */ C.LENGTH_UNSET,
/* streamReader= */ new TestStreamReader(),
/* payloadStartPosition= */ 0,
/* payloadEndPosition= */ C.LENGTH_UNSET,
/* firstPayloadPageSize= */ 1,
/* firstPayloadPageGranulePosition= */ 1,
/* firstPayloadPageIsLastPage= */ false);
@ -62,9 +61,9 @@ public final class DefaultOggSeekerTest {
TestStreamReader streamReader = new TestStreamReader();
DefaultOggSeeker oggSeeker =
new DefaultOggSeeker(
/* startPosition= */ 0,
/* endPosition= */ testFile.data.length,
/* streamReader= */ streamReader,
/* payloadStartPosition= */ 0,
/* payloadEndPosition= */ testFile.data.length,
/* firstPayloadPageSize= */ testFile.firstPayloadPageSize,
/* firstPayloadPageGranulePosition= */ testFile.firstPayloadPageGranulePosition,
/* firstPayloadPageIsLastPage= */ false);
@ -78,70 +77,56 @@ public final class DefaultOggSeekerTest {
input.setPosition((int) nextSeekPosition);
}
// Test granule 0 from file start
assertThat(seekTo(input, oggSeeker, 0, 0)).isEqualTo(0);
// Test granule 0 from file start.
long granule = seekTo(input, oggSeeker, 0, 0);
assertThat(granule).isEqualTo(0);
assertThat(input.getPosition()).isEqualTo(0);
// Test granule 0 from file end
assertThat(seekTo(input, oggSeeker, 0, testFile.data.length - 1)).isEqualTo(0);
// Test granule 0 from file end.
granule = seekTo(input, oggSeeker, 0, testFile.data.length - 1);
assertThat(granule).isEqualTo(0);
assertThat(input.getPosition()).isEqualTo(0);
{ // Test last granule
long currentGranule = seekTo(input, oggSeeker, testFile.lastGranule, 0);
long position = testFile.data.length;
assertThat(
(testFile.lastGranule > currentGranule && position > input.getPosition())
|| (testFile.lastGranule == currentGranule && position == input.getPosition()))
.isTrue();
}
// Test last granule.
granule = seekTo(input, oggSeeker, testFile.lastGranule, 0);
long position = testFile.data.length;
// TODO: Simplify this.
assertThat(
(testFile.lastGranule > granule && position > input.getPosition())
|| (testFile.lastGranule == granule && position == input.getPosition()))
.isTrue();
{ // Test exact granule
input.setPosition(testFile.data.length / 2);
oggSeeker.skipToNextPage(input);
assertThat(pageHeader.populate(input, true)).isTrue();
long position = input.getPosition() + pageHeader.headerSize + pageHeader.bodySize;
long currentGranule = seekTo(input, oggSeeker, pageHeader.granulePosition, 0);
assertThat(
(pageHeader.granulePosition > currentGranule && position > input.getPosition())
|| (pageHeader.granulePosition == currentGranule
&& position == input.getPosition()))
.isTrue();
}
// Test exact granule.
input.setPosition(testFile.data.length / 2);
oggSeeker.skipToNextPage(input);
assertThat(pageHeader.populate(input, true)).isTrue();
position = input.getPosition() + pageHeader.headerSize + pageHeader.bodySize;
granule = seekTo(input, oggSeeker, pageHeader.granulePosition, 0);
// TODO: Simplify this.
assertThat(
(pageHeader.granulePosition > granule && position > input.getPosition())
|| (pageHeader.granulePosition == granule && position == input.getPosition()))
.isTrue();
for (int i = 0; i < 100; i += 1) {
long targetGranule = (long) (random.nextDouble() * testFile.lastGranule);
int initialPosition = random.nextInt(testFile.data.length);
long currentGranule = seekTo(input, oggSeeker, targetGranule, initialPosition);
granule = seekTo(input, oggSeeker, targetGranule, initialPosition);
long currentPosition = input.getPosition();
assertWithMessage("getNextSeekPosition() didn't leave input on a page start.")
.that(pageHeader.populate(input, true))
.isTrue();
if (currentGranule == 0) {
if (granule == 0) {
assertThat(currentPosition).isEqualTo(0);
} else {
int previousPageStart = testFile.findPreviousPageStart(currentPosition);
input.setPosition(previousPageStart);
assertThat(pageHeader.populate(input, true)).isTrue();
assertThat(currentGranule).isEqualTo(pageHeader.granulePosition);
pageHeader.populate(input, false);
assertThat(granule).isEqualTo(pageHeader.granulePosition);
}
input.setPosition((int) currentPosition);
oggSeeker.skipToPageOfGranule(input, targetGranule, -1);
long positionDiff = Math.abs(input.getPosition() - currentPosition);
long granuleDiff = currentGranule - targetGranule;
if ((granuleDiff > DefaultOggSeeker.MATCH_RANGE || granuleDiff < 0)
&& positionDiff > DefaultOggSeeker.MATCH_BYTE_RANGE) {
fail(
"granuleDiff ("
+ granuleDiff
+ ") or positionDiff ("
+ positionDiff
+ ") is more than allowed.");
}
pageHeader.populate(input, false);
// The target granule should be within the current page.
assertThat(granule).isAtMost(targetGranule);
assertThat(targetGranule).isLessThan(pageHeader.granulePosition);
}
}
@ -149,18 +134,15 @@ public final class DefaultOggSeekerTest {
FakeExtractorInput input, DefaultOggSeeker oggSeeker, long targetGranule, int initialPosition)
throws IOException, InterruptedException {
long nextSeekPosition = initialPosition;
oggSeeker.startSeek(targetGranule);
int count = 0;
oggSeeker.resetSeeking();
do {
input.setPosition((int) nextSeekPosition);
nextSeekPosition = oggSeeker.getNextSeekPosition(targetGranule, input);
while (nextSeekPosition >= 0) {
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);
}
@ -171,8 +153,7 @@ public final class DefaultOggSeekerTest {
}
@Override
protected boolean readHeaders(ParsableByteArray packet, long position, SetupData setupData)
throws IOException, InterruptedException {
protected boolean readHeaders(ParsableByteArray packet, long position, SetupData setupData) {
return false;
}
}

View File

@ -85,9 +85,9 @@ public final class DefaultOggSeekerUtilMethodsTest {
throws IOException, InterruptedException {
DefaultOggSeeker oggSeeker =
new DefaultOggSeeker(
/* startPosition= */ 0,
/* endPosition= */ extractorInput.getLength(),
/* streamReader= */ new FlacReader(),
/* payloadStartPosition= */ 0,
/* payloadEndPosition= */ extractorInput.getLength(),
/* firstPayloadPageSize= */ 1,
/* firstPayloadPageGranulePosition= */ 2,
/* firstPayloadPageIsLastPage= */ false);
@ -99,87 +99,6 @@ public final class DefaultOggSeekerUtilMethodsTest {
}
}
@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 {
FakeExtractorInput input = OggTestData.createInput(TestUtil.joinByteArrays(
@ -204,7 +123,7 @@ public final class DefaultOggSeekerUtilMethodsTest {
assertReadGranuleOfLastPage(input, 60000);
fail();
} catch (EOFException e) {
// ignored
// Ignored.
}
}
@ -216,7 +135,7 @@ public final class DefaultOggSeekerUtilMethodsTest {
assertReadGranuleOfLastPage(input, 60000);
fail();
} catch (IllegalArgumentException e) {
// ignored
// Ignored.
}
}
@ -224,9 +143,9 @@ public final class DefaultOggSeekerUtilMethodsTest {
throws IOException, InterruptedException {
DefaultOggSeeker oggSeeker =
new DefaultOggSeeker(
/* startPosition= */ 0,
/* endPosition= */ input.getLength(),
/* streamReader= */ new FlacReader(),
/* payloadStartPosition= */ 0,
/* payloadEndPosition= */ input.getLength(),
/* firstPayloadPageSize= */ 1,
/* firstPayloadPageGranulePosition= */ 2,
/* firstPayloadPageIsLastPage= */ false);
@ -235,7 +154,7 @@ public final class DefaultOggSeekerUtilMethodsTest {
assertThat(oggSeeker.readGranuleOfLastPage(input)).isEqualTo(expected);
break;
} catch (FakeExtractorInput.SimulatedIOException e) {
// ignored
// Ignored.
}
}
}