Commit 93b3f43e by krocard Committed by Ian Baker

Fix some extractor nullness checks

Fix Matroska, Heif, FLAC, Ogg, Opus, Vorbis
extractor nullness check.

There should be no functional change.
Every media that fail to be parsed should still fail.
Every media that parsed successfully should still succeed.

This refactor aims to push all nullness constraints up the call stack to clarify each API nullness contract. This ensures implementation and caller have to prove their respective contract close to where such logic is implemented. This also allows to fail early if an nullness contract is broken instead of deep in the call stack.

For example, by adding a requirement that all implementation of `StreamReader.readHeaders` have to initialize `setupData.format` if the return false, each overriding method is forced to prove this next to the logic initializing it. This also means the runtime check might not be needed because the nullnessChecker can prove itself the contract holds.

This is in contrast with adding a null check at the point of usage, which will not catch logic errors where they are produce, but later when they are perceived; making it harder to debug and catching the issue at run time instead of compile time.

#exofixit

PiperOrigin-RevId: 346163124
parent 42f5e53d
...@@ -138,7 +138,7 @@ public final class PsshAtomUtil { ...@@ -138,7 +138,7 @@ public final class PsshAtomUtil {
if (parsedAtom == null) { if (parsedAtom == null) {
return null; return null;
} }
if (uuid != null && !uuid.equals(parsedAtom.uuid)) { if (!uuid.equals(parsedAtom.uuid)) {
Log.w(TAG, "UUID mismatch. Expected: " + uuid + ", got: " + parsedAtom.uuid + "."); Log.w(TAG, "UUID mismatch. Expected: " + uuid + ", got: " + parsedAtom.uuid + ".");
return null; return null;
} }
......
...@@ -15,6 +15,9 @@ ...@@ -15,6 +15,9 @@
*/ */
package com.google.android.exoplayer2.extractor.ogg; package com.google.android.exoplayer2.extractor.ogg;
import static com.google.android.exoplayer2.util.Assertions.checkNotNull;
import static com.google.android.exoplayer2.util.Assertions.checkState;
import androidx.annotation.Nullable; import androidx.annotation.Nullable;
import com.google.android.exoplayer2.extractor.ExtractorInput; import com.google.android.exoplayer2.extractor.ExtractorInput;
import com.google.android.exoplayer2.extractor.FlacFrameReader; import com.google.android.exoplayer2.extractor.FlacFrameReader;
...@@ -23,11 +26,11 @@ import com.google.android.exoplayer2.extractor.FlacSeekTableSeekMap; ...@@ -23,11 +26,11 @@ import com.google.android.exoplayer2.extractor.FlacSeekTableSeekMap;
import com.google.android.exoplayer2.extractor.FlacStreamMetadata; import com.google.android.exoplayer2.extractor.FlacStreamMetadata;
import com.google.android.exoplayer2.extractor.FlacStreamMetadata.SeekTable; import com.google.android.exoplayer2.extractor.FlacStreamMetadata.SeekTable;
import com.google.android.exoplayer2.extractor.SeekMap; import com.google.android.exoplayer2.extractor.SeekMap;
import com.google.android.exoplayer2.util.Assertions;
import com.google.android.exoplayer2.util.FlacConstants; import com.google.android.exoplayer2.util.FlacConstants;
import com.google.android.exoplayer2.util.ParsableByteArray; import com.google.android.exoplayer2.util.ParsableByteArray;
import com.google.android.exoplayer2.util.Util; import com.google.android.exoplayer2.util.Util;
import java.util.Arrays; import java.util.Arrays;
import org.checkerframework.checker.nullness.qual.EnsuresNonNullIf;
/** /**
* {@link StreamReader} to extract Flac data out of Ogg byte stream. * {@link StreamReader} to extract Flac data out of Ogg byte stream.
...@@ -68,6 +71,7 @@ import java.util.Arrays; ...@@ -68,6 +71,7 @@ import java.util.Arrays;
} }
@Override @Override
@EnsuresNonNullIf(expression = "#3.format", result = false)
protected boolean readHeaders(ParsableByteArray packet, long position, SetupData setupData) { protected boolean readHeaders(ParsableByteArray packet, long position, SetupData setupData) {
byte[] data = packet.getData(); byte[] data = packet.getData();
@Nullable FlacStreamMetadata streamMetadata = this.streamMetadata; @Nullable FlacStreamMetadata streamMetadata = this.streamMetadata;
...@@ -76,18 +80,26 @@ import java.util.Arrays; ...@@ -76,18 +80,26 @@ import java.util.Arrays;
this.streamMetadata = streamMetadata; this.streamMetadata = streamMetadata;
byte[] metadata = Arrays.copyOfRange(data, 9, packet.limit()); byte[] metadata = Arrays.copyOfRange(data, 9, packet.limit());
setupData.format = streamMetadata.getFormat(metadata, /* id3Metadata= */ null); setupData.format = streamMetadata.getFormat(metadata, /* id3Metadata= */ null);
} else if ((data[0] & 0x7F) == FlacConstants.METADATA_TYPE_SEEK_TABLE) { return true;
}
if ((data[0] & 0x7F) == FlacConstants.METADATA_TYPE_SEEK_TABLE) {
SeekTable seekTable = FlacMetadataReader.readSeekTableMetadataBlock(packet); SeekTable seekTable = FlacMetadataReader.readSeekTableMetadataBlock(packet);
streamMetadata = streamMetadata.copyWithSeekTable(seekTable); streamMetadata = streamMetadata.copyWithSeekTable(seekTable);
this.streamMetadata = streamMetadata; this.streamMetadata = streamMetadata;
flacOggSeeker = new FlacOggSeeker(streamMetadata, seekTable); flacOggSeeker = new FlacOggSeeker(streamMetadata, seekTable);
} else if (isAudioPacket(data)) { return true;
}
if (isAudioPacket(data)) {
if (flacOggSeeker != null) { if (flacOggSeeker != null) {
flacOggSeeker.setFirstFrameOffset(position); flacOggSeeker.setFirstFrameOffset(position);
setupData.oggSeeker = flacOggSeeker; setupData.oggSeeker = flacOggSeeker;
} }
checkNotNull(setupData.format);
return false; return false;
} }
return true; return true;
} }
...@@ -142,7 +154,7 @@ import java.util.Arrays; ...@@ -142,7 +154,7 @@ import java.util.Arrays;
@Override @Override
public SeekMap createSeekMap() { public SeekMap createSeekMap() {
Assertions.checkState(firstFrameOffset != -1); checkState(firstFrameOffset != -1);
return new FlacSeekTableSeekMap(streamMetadata, firstFrameOffset); return new FlacSeekTableSeekMap(streamMetadata, firstFrameOffset);
} }
......
...@@ -15,6 +15,7 @@ ...@@ -15,6 +15,7 @@
*/ */
package com.google.android.exoplayer2.extractor.ogg; package com.google.android.exoplayer2.extractor.ogg;
import static com.google.android.exoplayer2.util.Assertions.checkStateNotNull;
import static java.lang.Math.min; import static java.lang.Math.min;
import com.google.android.exoplayer2.C; import com.google.android.exoplayer2.C;
...@@ -25,7 +26,6 @@ import com.google.android.exoplayer2.extractor.ExtractorOutput; ...@@ -25,7 +26,6 @@ import com.google.android.exoplayer2.extractor.ExtractorOutput;
import com.google.android.exoplayer2.extractor.ExtractorsFactory; import com.google.android.exoplayer2.extractor.ExtractorsFactory;
import com.google.android.exoplayer2.extractor.PositionHolder; import com.google.android.exoplayer2.extractor.PositionHolder;
import com.google.android.exoplayer2.extractor.TrackOutput; import com.google.android.exoplayer2.extractor.TrackOutput;
import com.google.android.exoplayer2.util.Assertions;
import com.google.android.exoplayer2.util.ParsableByteArray; import com.google.android.exoplayer2.util.ParsableByteArray;
import java.io.IOException; import java.io.IOException;
import org.checkerframework.checker.nullness.qual.EnsuresNonNullIf; import org.checkerframework.checker.nullness.qual.EnsuresNonNullIf;
...@@ -73,7 +73,7 @@ public class OggExtractor implements Extractor { ...@@ -73,7 +73,7 @@ public class OggExtractor implements Extractor {
@Override @Override
public int read(ExtractorInput input, PositionHolder seekPosition) throws IOException { public int read(ExtractorInput input, PositionHolder seekPosition) throws IOException {
Assertions.checkStateNotNull(output); // Asserts that init has been called. checkStateNotNull(output); // Check that init has been called.
if (streamReader == null) { if (streamReader == null) {
if (!sniffInternal(input)) { if (!sniffInternal(input)) {
throw new ParserException("Failed to determine bitstream type"); throw new ParserException("Failed to determine bitstream type");
......
...@@ -15,12 +15,15 @@ ...@@ -15,12 +15,15 @@
*/ */
package com.google.android.exoplayer2.extractor.ogg; package com.google.android.exoplayer2.extractor.ogg;
import static com.google.android.exoplayer2.util.Assertions.checkNotNull;
import com.google.android.exoplayer2.Format; import com.google.android.exoplayer2.Format;
import com.google.android.exoplayer2.audio.OpusUtil; import com.google.android.exoplayer2.audio.OpusUtil;
import com.google.android.exoplayer2.util.MimeTypes; import com.google.android.exoplayer2.util.MimeTypes;
import com.google.android.exoplayer2.util.ParsableByteArray; import com.google.android.exoplayer2.util.ParsableByteArray;
import java.util.Arrays; import java.util.Arrays;
import java.util.List; import java.util.List;
import org.checkerframework.checker.nullness.qual.EnsuresNonNullIf;
/** /**
* {@link StreamReader} to extract Opus data out of Ogg byte stream. * {@link StreamReader} to extract Opus data out of Ogg byte stream.
...@@ -55,6 +58,7 @@ import java.util.List; ...@@ -55,6 +58,7 @@ import java.util.List;
} }
@Override @Override
@EnsuresNonNullIf(expression = "#3.format", result = false)
protected boolean readHeaders(ParsableByteArray packet, long position, SetupData setupData) { protected boolean readHeaders(ParsableByteArray packet, long position, SetupData setupData) {
if (!headerRead) { if (!headerRead) {
byte[] headerBytes = Arrays.copyOf(packet.getData(), packet.limit()); byte[] headerBytes = Arrays.copyOf(packet.getData(), packet.limit());
...@@ -68,12 +72,13 @@ import java.util.List; ...@@ -68,12 +72,13 @@ import java.util.List;
.setInitializationData(initializationData) .setInitializationData(initializationData)
.build(); .build();
headerRead = true; headerRead = true;
return true;
} else { } else {
checkNotNull(setupData.format); // Has been set when the header was read.
boolean headerPacket = packet.readInt() == OPUS_CODE; boolean headerPacket = packet.readInt() == OPUS_CODE;
packet.setPosition(0); packet.setPosition(0);
return headerPacket; return headerPacket;
} }
return true;
} }
/** /**
......
...@@ -15,7 +15,9 @@ ...@@ -15,7 +15,9 @@
*/ */
package com.google.android.exoplayer2.extractor.ogg; package com.google.android.exoplayer2.extractor.ogg;
import androidx.annotation.Nullable; import static com.google.android.exoplayer2.util.Assertions.checkStateNotNull;
import static com.google.android.exoplayer2.util.Util.castNonNull;
import com.google.android.exoplayer2.C; import com.google.android.exoplayer2.C;
import com.google.android.exoplayer2.Format; import com.google.android.exoplayer2.Format;
import com.google.android.exoplayer2.extractor.Extractor; import com.google.android.exoplayer2.extractor.Extractor;
...@@ -24,10 +26,12 @@ import com.google.android.exoplayer2.extractor.ExtractorOutput; ...@@ -24,10 +26,12 @@ import com.google.android.exoplayer2.extractor.ExtractorOutput;
import com.google.android.exoplayer2.extractor.PositionHolder; import com.google.android.exoplayer2.extractor.PositionHolder;
import com.google.android.exoplayer2.extractor.SeekMap; import com.google.android.exoplayer2.extractor.SeekMap;
import com.google.android.exoplayer2.extractor.TrackOutput; import com.google.android.exoplayer2.extractor.TrackOutput;
import com.google.android.exoplayer2.util.Assertions;
import com.google.android.exoplayer2.util.ParsableByteArray; import com.google.android.exoplayer2.util.ParsableByteArray;
import java.io.IOException; import java.io.IOException;
import org.checkerframework.checker.nullness.qual.EnsuresNonNull;
import org.checkerframework.checker.nullness.qual.EnsuresNonNullIf;
import org.checkerframework.checker.nullness.qual.MonotonicNonNull; import org.checkerframework.checker.nullness.qual.MonotonicNonNull;
import org.checkerframework.checker.nullness.qual.RequiresNonNull;
/** StreamReader abstract class. */ /** StreamReader abstract class. */
@SuppressWarnings("UngroupedOverloads") @SuppressWarnings("UngroupedOverloads")
...@@ -39,8 +43,8 @@ import org.checkerframework.checker.nullness.qual.MonotonicNonNull; ...@@ -39,8 +43,8 @@ import org.checkerframework.checker.nullness.qual.MonotonicNonNull;
private static final int STATE_END_OF_INPUT = 3; private static final int STATE_END_OF_INPUT = 3;
static class SetupData { static class SetupData {
Format format; @MonotonicNonNull Format format;
OggSeeker oggSeeker; @MonotonicNonNull OggSeeker oggSeeker;
} }
private final OggPacket oggPacket; private final OggPacket oggPacket;
...@@ -53,13 +57,14 @@ import org.checkerframework.checker.nullness.qual.MonotonicNonNull; ...@@ -53,13 +57,14 @@ import org.checkerframework.checker.nullness.qual.MonotonicNonNull;
private long currentGranule; private long currentGranule;
private int state; private int state;
private int sampleRate; private int sampleRate;
@Nullable private SetupData setupData; private SetupData setupData;
private long lengthOfReadPacket; private long lengthOfReadPacket;
private boolean seekMapSet; private boolean seekMapSet;
private boolean formatSet; private boolean formatSet;
public StreamReader() { public StreamReader() {
oggPacket = new OggPacket(); oggPacket = new OggPacket();
setupData = new SetupData();
} }
void init(ExtractorOutput output, TrackOutput trackOutput) { void init(ExtractorOutput output, TrackOutput trackOutput) {
...@@ -95,7 +100,7 @@ import org.checkerframework.checker.nullness.qual.MonotonicNonNull; ...@@ -95,7 +100,7 @@ import org.checkerframework.checker.nullness.qual.MonotonicNonNull;
} else { } else {
if (state != STATE_READ_HEADERS) { if (state != STATE_READ_HEADERS) {
targetGranule = convertTimeToGranule(timeUs); targetGranule = convertTimeToGranule(timeUs);
oggSeeker.startSeek(targetGranule); castNonNull(oggSeeker).startSeek(targetGranule);
state = STATE_READ_PAYLOAD; state = STATE_READ_PAYLOAD;
} }
} }
...@@ -103,14 +108,16 @@ import org.checkerframework.checker.nullness.qual.MonotonicNonNull; ...@@ -103,14 +108,16 @@ import org.checkerframework.checker.nullness.qual.MonotonicNonNull;
/** @see Extractor#read(ExtractorInput, PositionHolder) */ /** @see Extractor#read(ExtractorInput, PositionHolder) */
final int read(ExtractorInput input, PositionHolder seekPosition) throws IOException { final int read(ExtractorInput input, PositionHolder seekPosition) throws IOException {
assertInitialized();
switch (state) { switch (state) {
case STATE_READ_HEADERS: case STATE_READ_HEADERS:
return readHeaders(input); return readHeadersAndUpdateState(input);
case STATE_SKIP_HEADERS: case STATE_SKIP_HEADERS:
input.skipFully((int) payloadStartPosition); input.skipFully((int) payloadStartPosition);
state = STATE_READ_PAYLOAD; state = STATE_READ_PAYLOAD;
return Extractor.RESULT_CONTINUE; return Extractor.RESULT_CONTINUE;
case STATE_READ_PAYLOAD: case STATE_READ_PAYLOAD:
castNonNull(oggSeeker);
return readPayload(input, seekPosition); return readPayload(input, seekPosition);
default: default:
// Never happens. // Never happens.
...@@ -118,20 +125,42 @@ import org.checkerframework.checker.nullness.qual.MonotonicNonNull; ...@@ -118,20 +125,42 @@ import org.checkerframework.checker.nullness.qual.MonotonicNonNull;
} }
} }
private int readHeaders(ExtractorInput input) throws IOException { @EnsuresNonNull({"trackOutput", "extractorOutput"})
boolean readingHeaders = true; private void assertInitialized() {
while (readingHeaders) { checkStateNotNull(trackOutput);
castNonNull(extractorOutput);
}
/**
* Read all header packets.
*
* @param input The {@link ExtractorInput} to read data from.
* @return {@code true} if all headers were read. {@code false} if end of the input is
* encountered.
* @throws IOException If reading from the input fails.
*/
@EnsuresNonNullIf(expression = "setupData.format", result = true)
private boolean readHeaders(ExtractorInput input) throws IOException {
while (true) {
if (!oggPacket.populate(input)) { if (!oggPacket.populate(input)) {
state = STATE_END_OF_INPUT; state = STATE_END_OF_INPUT;
return Extractor.RESULT_END_OF_INPUT; return false;
} }
lengthOfReadPacket = input.getPosition() - payloadStartPosition; lengthOfReadPacket = input.getPosition() - payloadStartPosition;
readingHeaders = readHeaders(oggPacket.getPayload(), payloadStartPosition, setupData); if (readHeaders(oggPacket.getPayload(), payloadStartPosition, setupData)) {
if (readingHeaders) {
payloadStartPosition = input.getPosition(); payloadStartPosition = input.getPosition();
} else {
return true; // Current packet is not a header, therefore all headers have been read.
} }
} }
}
@RequiresNonNull({"trackOutput"})
private int readHeadersAndUpdateState(ExtractorInput input) throws IOException {
if (!readHeaders(input)) {
return Extractor.RESULT_END_OF_INPUT;
}
sampleRate = setupData.format.sampleRate; sampleRate = setupData.format.sampleRate;
if (!formatSet) { if (!formatSet) {
...@@ -156,13 +185,13 @@ import org.checkerframework.checker.nullness.qual.MonotonicNonNull; ...@@ -156,13 +185,13 @@ import org.checkerframework.checker.nullness.qual.MonotonicNonNull;
isLastPage); isLastPage);
} }
setupData = null;
state = STATE_READ_PAYLOAD; state = STATE_READ_PAYLOAD;
// First payload packet. Trim the payload array of the ogg packet after headers have been read. // First payload packet. Trim the payload array of the ogg packet after headers have been read.
oggPacket.trimPayload(); oggPacket.trimPayload();
return Extractor.RESULT_CONTINUE; return Extractor.RESULT_CONTINUE;
} }
@RequiresNonNull({"trackOutput", "oggSeeker", "extractorOutput"})
private int readPayload(ExtractorInput input, PositionHolder seekPosition) throws IOException { private int readPayload(ExtractorInput input, PositionHolder seekPosition) throws IOException {
long position = oggSeeker.read(input); long position = oggSeeker.read(input);
if (position >= 0) { if (position >= 0) {
...@@ -173,7 +202,7 @@ import org.checkerframework.checker.nullness.qual.MonotonicNonNull; ...@@ -173,7 +202,7 @@ import org.checkerframework.checker.nullness.qual.MonotonicNonNull;
} }
if (!seekMapSet) { if (!seekMapSet) {
SeekMap seekMap = Assertions.checkStateNotNull(oggSeeker.createSeekMap()); SeekMap seekMap = checkStateNotNull(oggSeeker.createSeekMap());
extractorOutput.seekMap(seekMap); extractorOutput.seekMap(seekMap);
seekMapSet = true; seekMapSet = true;
} }
...@@ -234,6 +263,7 @@ import org.checkerframework.checker.nullness.qual.MonotonicNonNull; ...@@ -234,6 +263,7 @@ import org.checkerframework.checker.nullness.qual.MonotonicNonNull;
* @param setupData Setup data to be filled. * @param setupData Setup data to be filled.
* @return Whether the packet contains header data. * @return Whether the packet contains header data.
*/ */
@EnsuresNonNullIf(expression = "#3.format", result = false)
protected abstract boolean readHeaders( protected abstract boolean readHeaders(
ParsableByteArray packet, long position, SetupData setupData) throws IOException; ParsableByteArray packet, long position, SetupData setupData) throws IOException;
......
...@@ -15,6 +15,9 @@ ...@@ -15,6 +15,9 @@
*/ */
package com.google.android.exoplayer2.extractor.ogg; package com.google.android.exoplayer2.extractor.ogg;
import static com.google.android.exoplayer2.util.Assertions.checkNotNull;
import static com.google.android.exoplayer2.util.Assertions.checkStateNotNull;
import androidx.annotation.Nullable; import androidx.annotation.Nullable;
import androidx.annotation.VisibleForTesting; import androidx.annotation.VisibleForTesting;
import com.google.android.exoplayer2.Format; import com.google.android.exoplayer2.Format;
...@@ -26,6 +29,7 @@ import com.google.android.exoplayer2.util.ParsableByteArray; ...@@ -26,6 +29,7 @@ import com.google.android.exoplayer2.util.ParsableByteArray;
import java.io.IOException; import java.io.IOException;
import java.util.ArrayList; import java.util.ArrayList;
import java.util.Arrays; import java.util.Arrays;
import org.checkerframework.checker.nullness.qual.EnsuresNonNullIf;
/** /**
* {@link StreamReader} to extract Vorbis data out of Ogg byte stream. * {@link StreamReader} to extract Vorbis data out of Ogg byte stream.
...@@ -74,7 +78,7 @@ import java.util.Arrays; ...@@ -74,7 +78,7 @@ import java.util.Arrays;
} }
// ... we need to decode the block size // ... we need to decode the block size
int packetBlockSize = decodeBlockSize(packet.getData()[0], vorbisSetup); int packetBlockSize = decodeBlockSize(packet.getData()[0], checkStateNotNull(vorbisSetup));
// a packet contains samples produced from overlapping the previous and current frame data // a packet contains samples produced from overlapping the previous and current frame data
// (https://www.xiph.org/vorbis/doc/Vorbis_I_spec.html#x1-350001.3.2) // (https://www.xiph.org/vorbis/doc/Vorbis_I_spec.html#x1-350001.3.2)
int samplesInPacket = seenFirstAudioPacket ? (packetBlockSize + previousPacketBlockSize) / 4 int samplesInPacket = seenFirstAudioPacket ? (packetBlockSize + previousPacketBlockSize) / 4
...@@ -89,9 +93,11 @@ import java.util.Arrays; ...@@ -89,9 +93,11 @@ import java.util.Arrays;
} }
@Override @Override
@EnsuresNonNullIf(expression = "#3.format", result = false)
protected boolean readHeaders(ParsableByteArray packet, long position, SetupData setupData) protected boolean readHeaders(ParsableByteArray packet, long position, SetupData setupData)
throws IOException { throws IOException {
if (vorbisSetup != null) { if (vorbisSetup != null) {
checkNotNull(setupData.format);
return false; return false;
} }
...@@ -99,6 +105,7 @@ import java.util.Arrays; ...@@ -99,6 +105,7 @@ import java.util.Arrays;
if (vorbisSetup == null) { if (vorbisSetup == null) {
return true; return true;
} }
VorbisSetup vorbisSetup = this.vorbisSetup;
VorbisUtil.VorbisIdHeader idHeader = vorbisSetup.idHeader; VorbisUtil.VorbisIdHeader idHeader = vorbisSetup.idHeader;
...@@ -131,6 +138,8 @@ import java.util.Arrays; ...@@ -131,6 +138,8 @@ import java.util.Arrays;
commentHeader = VorbisUtil.readVorbisCommentHeader(scratch); commentHeader = VorbisUtil.readVorbisCommentHeader(scratch);
return null; return null;
} }
VorbisUtil.VorbisIdHeader vorbisIdHeader = this.vorbisIdHeader;
VorbisUtil.CommentHeader commentHeader = this.commentHeader;
// the third packet contains the setup header // the third packet contains the setup header
byte[] setupHeaderData = new byte[scratch.limit()]; byte[] setupHeaderData = new byte[scratch.limit()];
......
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