fix(player): retry transient stream errors instead of falling back to MPV
During Android direct play, a single dropped connection or network blip mid-stream kicked an otherwise healthy ExoPlayer session over to the MPV fallback - on a Shield this showed as random backend switches minutes into an episode (log pu4ad: "ffmpeg demuxer read failed: -5" with contentIsMalformed=true, while MPV reopened the same URL fine). Three defects lined up behind it, all in the 2.17.0 ffmpeg demux path: - The AVIO read callback turned the input proxy's stored IOException (its -1 return) into a bare AVERROR(EIO) without marking javaError, so the extractor classified the failure as a malformed container. media3 never retries a ParserException, so the designed ERR_JAVA -> IOException -> load-error-retry path was unreachable. The callback now latches javaError for a negative read and fails fast on every later read in the same native call, so matroska resync cannot clobber the stored message or skip past the failed range. - A failed refill latches AVIOContext error/eof_reached and avio never drives the callback again, so even a correctly classified retry would re-fail on the stale error. nativeReadPacket and nativeSeek now clear the latch on entry; a genuine end of file (error == 0) is left alone. - FfmpegRandomAccessSource kept a handle whose read had thrown, and its position matched the retried request, so the retry was handed the same dead handle. A failed read now drops the handle and the retry reopens. A transient failure now surfaces as a retryable IOException, media3 reopens the source, and sample delivery resumes gaplessly; persistent failures still exhaust the retry policy and reach the MPV fallback as before. close #2113
This commit is contained in:
+240
@@ -0,0 +1,240 @@
|
||||
package com.edde746.plezy.exoplayer
|
||||
|
||||
import android.content.Context
|
||||
import android.net.Uri
|
||||
import androidx.media3.common.C
|
||||
import androidx.media3.common.DataReader
|
||||
import androidx.media3.common.Format
|
||||
import androidx.media3.common.ParserException
|
||||
import androidx.media3.common.util.ParsableByteArray
|
||||
import androidx.media3.datasource.DataSource
|
||||
import androidx.media3.datasource.DataSpec
|
||||
import androidx.media3.datasource.DefaultDataSource
|
||||
import androidx.media3.datasource.TransferListener
|
||||
import androidx.media3.extractor.DefaultExtractorInput
|
||||
import androidx.media3.extractor.Extractor
|
||||
import androidx.media3.extractor.ExtractorInput
|
||||
import androidx.media3.extractor.ExtractorOutput
|
||||
import androidx.media3.extractor.PositionHolder
|
||||
import androidx.media3.extractor.SeekMap
|
||||
import androidx.media3.extractor.TrackOutput
|
||||
import androidx.media3.extractor.text.DefaultSubtitleParserFactory
|
||||
import androidx.test.ext.junit.runners.AndroidJUnit4
|
||||
import androidx.test.platform.app.InstrumentationRegistry
|
||||
import com.edde746.plezy.libass.media.AssHandler
|
||||
import java.io.File
|
||||
import java.io.IOException
|
||||
import org.junit.Assert.assertEquals
|
||||
import org.junit.Assert.assertFalse
|
||||
import org.junit.Assert.assertThrows
|
||||
import org.junit.Assert.assertTrue
|
||||
import org.junit.Test
|
||||
import org.junit.runner.RunWith
|
||||
|
||||
/**
|
||||
* Regression coverage for #2113: a transient byte-source failure mid-stream
|
||||
* must reach media3 as a plain retryable IOException, and the demuxer must
|
||||
* deliver samples again once the retried read succeeds.
|
||||
*
|
||||
* 2.17.1 got both halves wrong. The AVIO callback converted the proxy's
|
||||
* stored IOException into a bare AVERROR(EIO), which the extractor classified
|
||||
* as "malformed container" — a ParserException that
|
||||
* DefaultLoadErrorHandlingPolicy never retries — so one dropped connection
|
||||
* sent an otherwise healthy ExoPlayer session to the MPV fallback. And even
|
||||
* with the classification fixed, a failed refill latches
|
||||
* AVIOContext.error/eof_reached, so the retry would re-fail on the stale
|
||||
* error without the native side clearing the latch on re-entry.
|
||||
*/
|
||||
@RunWith(AndroidJUnit4::class)
|
||||
class FfmpegExtractorReadRetryTest {
|
||||
|
||||
private companion object {
|
||||
/** 30 s at 10 fps: video samples are exact multiples of 100 ms. */
|
||||
const val FIXTURE = "ffmpeg/seek_cued.mkv"
|
||||
const val SAMPLE_INTERVAL_US = 100_000L
|
||||
const val TOTAL_SAMPLES = 300
|
||||
}
|
||||
|
||||
@Test
|
||||
fun transientLoaderReadFailureIsRetryableAndDeliveryResumesGaplessly() {
|
||||
withHarness(FIXTURE) { harness ->
|
||||
assertTrue("no samples before the fault", harness.pumpUntil { harness.sampleTimestamps.size >= 10 })
|
||||
|
||||
harness.failNextLoaderRead()
|
||||
val thrown = assertThrows(IOException::class.java) {
|
||||
harness.pumpUntil { harness.sampleTimestamps.size >= TOTAL_SAMPLES }
|
||||
}
|
||||
// The regression: a byte-source failure classified as malformed content
|
||||
// is terminal to media3, which retries only plain IOExceptions.
|
||||
assertFalse("byte-source failure classified as malformed: $thrown", thrown is ParserException)
|
||||
assertEquals("injected loader failure", thrown.message)
|
||||
|
||||
// What ExtractingLoadable does on a load-error retry: reopen the source
|
||||
// where the input stood and call read() again.
|
||||
harness.reopenForRetry()
|
||||
assertTrue(
|
||||
"no samples after the retried read",
|
||||
harness.pumpUntil { harness.sampleTimestamps.size >= TOTAL_SAMPLES / 2 }
|
||||
)
|
||||
|
||||
// Decode order is not presentation order with B-frames; what must hold
|
||||
// is that the delivered set stays gapless across the failure point.
|
||||
val sorted = harness.sampleTimestamps.sorted()
|
||||
assertEquals(
|
||||
"sample delivery must resume gaplessly after the transient failure",
|
||||
sorted.indices.map { it * SAMPLE_INTERVAL_US },
|
||||
sorted
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
private fun withHarness(fixture: String, block: (Harness) -> Unit) {
|
||||
val instrumentation = InstrumentationRegistry.getInstrumentation()
|
||||
val context = instrumentation.targetContext
|
||||
val cacheDir = context.cacheDir.also { it.mkdirs() }
|
||||
val file = File.createTempFile("ffmpeg-retry-fixture-", ".mkv", cacheDir)
|
||||
instrumentation.context.assets.open(fixture).use { input ->
|
||||
file.outputStream().use(input::copyTo)
|
||||
}
|
||||
val harness = Harness(context, Uri.fromFile(file))
|
||||
try {
|
||||
harness.prepare()
|
||||
block(harness)
|
||||
} finally {
|
||||
harness.close()
|
||||
file.delete()
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Stand-in for ProgressiveMediaPeriod's loader, like FfmpegExtractorSeekTest's,
|
||||
* plus the piece under test here: the loader's DataSource can be made to fail
|
||||
* exactly one read, and [reopenForRetry] performs the load-error retry the
|
||||
* way ExtractingLoadable.load() does.
|
||||
*/
|
||||
private class Harness(context: Context, private val uri: Uri) {
|
||||
private val factory = DefaultDataSource.Factory(context)
|
||||
private val io = FfmpegRandomAccessSource(factory) { DataSpec(uri) }
|
||||
private val output = CapturingOutput()
|
||||
private val positionHolder = PositionHolder()
|
||||
private val extractor = requireNotNull(
|
||||
FfmpegExtractor.create(
|
||||
{ FfmpegDemuxerPolicy.Preference.FFMPEG },
|
||||
DvConversionMode.DISABLED,
|
||||
DefaultSubtitleParserFactory(),
|
||||
AssHandler(),
|
||||
io
|
||||
)
|
||||
) { "native ffmpeg demuxer unavailable" }
|
||||
|
||||
private var loaderSource: FlakySource? = null
|
||||
private var input: ExtractorInput? = null
|
||||
|
||||
val sampleTimestamps: List<Long>
|
||||
get() = output.sampleTimestamps
|
||||
|
||||
fun prepare() {
|
||||
openAt(0)
|
||||
assertTrue("fixture should be sniffed by the ffmpeg demuxer", extractor.sniff(input!!))
|
||||
input!!.resetPeekPosition()
|
||||
extractor.init(output)
|
||||
}
|
||||
|
||||
/** Arms the current loader handle; only its next read throws. */
|
||||
fun failNextLoaderRead() {
|
||||
loaderSource!!.failNextRead = true
|
||||
}
|
||||
|
||||
fun reopenForRetry() {
|
||||
openAt(input!!.position)
|
||||
}
|
||||
|
||||
fun pumpUntil(condition: () -> Boolean): Boolean {
|
||||
repeat(200_000) {
|
||||
if (condition()) return true
|
||||
when (extractor.read(input!!, positionHolder)) {
|
||||
Extractor.RESULT_SEEK -> openAt(positionHolder.position)
|
||||
Extractor.RESULT_END_OF_INPUT -> return condition()
|
||||
else -> Unit
|
||||
}
|
||||
}
|
||||
return condition()
|
||||
}
|
||||
|
||||
private fun openAt(position: Long) {
|
||||
loaderSource?.close()
|
||||
val source = FlakySource(factory.createDataSource())
|
||||
val remaining = source.open(DataSpec.Builder().setUri(uri).setPosition(position).build())
|
||||
loaderSource = source
|
||||
input = DefaultExtractorInput(
|
||||
source,
|
||||
position,
|
||||
if (remaining == C.LENGTH_UNSET.toLong()) C.LENGTH_UNSET.toLong() else position + remaining
|
||||
)
|
||||
}
|
||||
|
||||
fun close() {
|
||||
extractor.release()
|
||||
loaderSource?.close()
|
||||
}
|
||||
}
|
||||
|
||||
/** Wraps only the loader's source, so the fault never lands on an index read. */
|
||||
private class FlakySource(private val inner: DataSource) : DataSource {
|
||||
var failNextRead = false
|
||||
|
||||
override fun open(dataSpec: DataSpec): Long = inner.open(dataSpec)
|
||||
|
||||
override fun read(buffer: ByteArray, offset: Int, length: Int): Int {
|
||||
if (failNextRead) {
|
||||
failNextRead = false
|
||||
throw IOException("injected loader failure")
|
||||
}
|
||||
return inner.read(buffer, offset, length)
|
||||
}
|
||||
|
||||
override fun addTransferListener(transferListener: TransferListener) = inner.addTransferListener(transferListener)
|
||||
|
||||
override fun getUri(): Uri? = inner.uri
|
||||
|
||||
override fun getResponseHeaders(): Map<String, List<String>> = inner.responseHeaders
|
||||
|
||||
override fun close() = inner.close()
|
||||
}
|
||||
|
||||
private class CapturingOutput : ExtractorOutput {
|
||||
val sampleTimestamps = mutableListOf<Long>()
|
||||
|
||||
override fun track(id: Int, type: Int): TrackOutput = CapturingTrack(type, sampleTimestamps)
|
||||
|
||||
override fun endTracks() = Unit
|
||||
|
||||
override fun seekMap(seekMap: SeekMap) = Unit
|
||||
}
|
||||
|
||||
private class CapturingTrack(private val type: Int, private val timestamps: MutableList<Long>) : TrackOutput {
|
||||
private val scratch = ByteArray(64 * 1024)
|
||||
|
||||
override fun format(format: Format) = Unit
|
||||
|
||||
override fun sampleData(input: DataReader, length: Int, allowEndOfInput: Boolean): Int =
|
||||
sampleData(input, length, allowEndOfInput, TrackOutput.SAMPLE_DATA_PART_MAIN)
|
||||
|
||||
override fun sampleData(input: DataReader, length: Int, allowEndOfInput: Boolean, sampleDataPart: Int): Int {
|
||||
val read = input.read(scratch, 0, minOf(length, scratch.size))
|
||||
return if (read == C.RESULT_END_OF_INPUT) 0 else read
|
||||
}
|
||||
|
||||
override fun sampleData(data: ParsableByteArray, length: Int) {
|
||||
sampleData(data, length, TrackOutput.SAMPLE_DATA_PART_MAIN)
|
||||
}
|
||||
|
||||
override fun sampleData(data: ParsableByteArray, length: Int, sampleDataPart: Int) {
|
||||
data.skipBytes(length)
|
||||
}
|
||||
|
||||
override fun sampleMetadata(timeUs: Long, flags: Int, size: Int, offset: Int, cryptoData: TrackOutput.CryptoData?) {
|
||||
if (type == C.TRACK_TYPE_VIDEO) timestamps.add(timeUs)
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -204,6 +204,13 @@ int avioReadPacket(void* opaque, uint8_t* buf, int bufSize) {
|
||||
JNIEnv* env = envFor();
|
||||
if (env == nullptr) return AVERROR(EIO);
|
||||
if (bufSize <= 0) return 0;
|
||||
// After the byte source has failed once in this native call, fail every
|
||||
// further read immediately: libavformat's internal recovery (matroska
|
||||
// resync) would drive the dead source again, clobbering the stored
|
||||
// IOException message — and were the source half-alive, could silently
|
||||
// resync past the failed range. Failing fast makes the whole entry return
|
||||
// ERR_JAVA, and media3's retry resumes at the exact failed position.
|
||||
if (s->javaError) return AVERROR(EIO);
|
||||
|
||||
const jint want = bufSize < kReadChunkBytes ? bufSize : kReadChunkBytes;
|
||||
// Prefer the loader's input when it already stands here: those bytes then
|
||||
@@ -216,8 +223,15 @@ int avioReadPacket(void* opaque, uint8_t* buf, int bufSize) {
|
||||
return AVERROR(EIO);
|
||||
}
|
||||
const jint n = inputPos == s->logicalPos ? callRead(env, s, want) : callReadAt(env, s, s->logicalPos, want);
|
||||
if (s->javaError) return AVERROR(EIO);
|
||||
if (n < 0) return AVERROR(EIO);
|
||||
if (n < 0 || s->javaError) {
|
||||
// A negative count is the Input contract for a byte source that threw:
|
||||
// the Kotlin proxy caught the IOException and stored its message. Mark
|
||||
// javaError so the JNI entry point returns ERR_JAVA and media3's
|
||||
// load-error policy retries the load; a bare AVERROR would read as a
|
||||
// malformed container, which media3 never retries (#2113).
|
||||
s->javaError = true;
|
||||
return AVERROR(EIO);
|
||||
}
|
||||
if (n == 0) return AVERROR_EOF;
|
||||
|
||||
env->GetByteArrayRegion(s->readBuffer, 0, n, reinterpret_cast<jbyte*>(buf));
|
||||
@@ -264,6 +278,21 @@ int64_t avioSeek(void* opaque, int64_t offset, int whence) {
|
||||
return target;
|
||||
}
|
||||
|
||||
// A failed byte-source read latches AVIOContext error state: fill_buffer
|
||||
// stores the AVERROR in pb->error, sets pb->eof_reached, and never drives the
|
||||
// read callback again while either is set (aviobuf.c). The Kotlin side maps
|
||||
// ERR_JAVA to an IOException that media3's load-error policy retries with the
|
||||
// source reopened, so the latch must be dropped when the retry re-enters —
|
||||
// otherwise the first retry re-fails on the stale error without ever reaching
|
||||
// the byte source (#2113). A genuine end of file latches eof_reached with
|
||||
// error == 0 and is deliberately left alone.
|
||||
void clearLatchedIoError(DemuxState* s) {
|
||||
if (s->format == nullptr || s->format->pb == nullptr) return;
|
||||
if (s->format->pb->error >= 0) return;
|
||||
s->format->pb->error = 0;
|
||||
s->format->pb->eof_reached = 0;
|
||||
}
|
||||
|
||||
// Probing (ffio_ensure_seekback) can swap the AVIO buffer for a larger
|
||||
// allocation and free the original, so the pointer handed to
|
||||
// avio_alloc_context may be stale. Free whichever buffer the context
|
||||
@@ -830,6 +859,7 @@ JNIEXPORT jint JNICALL Java_com_edde746_plezy_exoplayer_FfmpegDemuxerJni_nativeR
|
||||
jint capacity = env->GetArrayLength(buffer);
|
||||
jlong result[7] = {};
|
||||
s->javaError = false;
|
||||
clearLatchedIoError(s);
|
||||
while (true) {
|
||||
if (s->packetPending) {
|
||||
// Oversized packet held across a CODE_GROW round trip: deliver it now
|
||||
@@ -931,6 +961,7 @@ Java_com_edde746_plezy_exoplayer_FfmpegDemuxerJni_nativeSeek(JNIEnv* env, jobjec
|
||||
DemuxState* s = gState;
|
||||
if (s == nullptr || !s->opened || s->format == nullptr) return ERR_NOT_OPEN;
|
||||
s->javaError = false;
|
||||
clearLatchedIoError(s);
|
||||
// Any packet parked for a CODE_GROW retry belongs to the pre-seek stream.
|
||||
s->packetPending = false;
|
||||
if (s->bsf != nullptr) {
|
||||
|
||||
@@ -548,6 +548,9 @@ internal class FfmpegExtractor private constructor(
|
||||
}
|
||||
FfmpegDemuxerJni.ERR_JAVA ->
|
||||
throw IOException(inputProxy.lastError ?: "demuxer input failed")
|
||||
// Byte-source failures surface as ERR_JAVA above and become retryable
|
||||
// IOExceptions; a raw AVERROR here is libavformat's verdict on the
|
||||
// bytes themselves, which media3 rightly treats as terminal (#2113).
|
||||
else -> throw malformed("ffmpeg demuxer read failed: $code")
|
||||
}
|
||||
}
|
||||
|
||||
@@ -62,7 +62,15 @@ internal class FfmpegRandomAccessSource(
|
||||
fun readAt(position: Long, buffer: ByteArray, length: Int): Int = synchronized(this) {
|
||||
if (length <= 0) return 0
|
||||
val handle = handleAt(position)
|
||||
val read = handle.read(buffer, 0, length)
|
||||
val read = try {
|
||||
handle.read(buffer, 0, length)
|
||||
} catch (e: Throwable) {
|
||||
// The handle is dead but handlePosition still matches the request, so a
|
||||
// load-error retry of the same read would be handed the same broken
|
||||
// handle forever. Drop it; the retry reopens at the position (#2113).
|
||||
close()
|
||||
throw e
|
||||
}
|
||||
if (read == C.RESULT_END_OF_INPUT) return 0
|
||||
handlePosition = position + read
|
||||
read
|
||||
|
||||
+23
@@ -33,6 +33,7 @@ class FfmpegRandomAccessSourceTest {
|
||||
private class FakeDataSource(private val content: ByteArray) : DataSource {
|
||||
var openedAt: Long = -1
|
||||
var closed = false
|
||||
var failNextRead = false
|
||||
private var position = 0
|
||||
|
||||
override fun open(dataSpec: DataSpec): Long {
|
||||
@@ -42,6 +43,10 @@ class FfmpegRandomAccessSourceTest {
|
||||
}
|
||||
|
||||
override fun read(buffer: ByteArray, offset: Int, length: Int): Int {
|
||||
if (failNextRead) {
|
||||
failNextRead = false
|
||||
throw IOException("injected read failure")
|
||||
}
|
||||
if (position >= content.size) return C.RESULT_END_OF_INPUT
|
||||
val count = minOf(length, content.size - position)
|
||||
content.copyInto(buffer, offset, position, position + count)
|
||||
@@ -109,6 +114,24 @@ class FfmpegRandomAccessSourceTest {
|
||||
assertTrue("abandoned handle must be closed", factory.created.first().closed)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun failedReadDropsTheHandleSoTheRetryReopens() {
|
||||
val factory = FakeFactory()
|
||||
val source = sourceFor(factory)
|
||||
val buffer = ByteArray(32)
|
||||
|
||||
source.readAt(0, buffer, 32)
|
||||
factory.created.single().failNextRead = true
|
||||
assertThrows(IOException::class.java) { source.readAt(32, buffer, 32) }
|
||||
assertTrue("failed handle must be closed", factory.created.single().closed)
|
||||
|
||||
// media3's load-error policy retries the read with the source reopened; a
|
||||
// dead handle whose position happens to match must not serve it (#2113).
|
||||
assertEquals(32, source.readAt(32, buffer, 32))
|
||||
assertArrayEquals(content.copyOfRange(32, 64), buffer)
|
||||
assertEquals(2, source.openCount)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun endOfInputReportsZeroWithoutAdvancing() {
|
||||
val factory = FakeFactory()
|
||||
|
||||
Reference in New Issue
Block a user