diff --git a/android/app/src/androidTest/kotlin/com/edde746/plezy/exoplayer/FfmpegExtractorReadRetryTest.kt b/android/app/src/androidTest/kotlin/com/edde746/plezy/exoplayer/FfmpegExtractorReadRetryTest.kt new file mode 100644 index 000000000..d1e82ea23 --- /dev/null +++ b/android/app/src/androidTest/kotlin/com/edde746/plezy/exoplayer/FfmpegExtractorReadRetryTest.kt @@ -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 + 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> = inner.responseHeaders + + override fun close() = inner.close() + } + + private class CapturingOutput : ExtractorOutput { + val sampleTimestamps = mutableListOf() + + 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) : 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) + } + } +} diff --git a/android/app/src/main/cpp/media3_ffmpeg_demuxer/ffmpeg_demuxer_jni.cc b/android/app/src/main/cpp/media3_ffmpeg_demuxer/ffmpeg_demuxer_jni.cc index 3519cc1f7..03da40bf1 100644 --- a/android/app/src/main/cpp/media3_ffmpeg_demuxer/ffmpeg_demuxer_jni.cc +++ b/android/app/src/main/cpp/media3_ffmpeg_demuxer/ffmpeg_demuxer_jni.cc @@ -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(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) { diff --git a/android/app/src/main/kotlin/com/edde746/plezy/exoplayer/FfmpegExtractor.kt b/android/app/src/main/kotlin/com/edde746/plezy/exoplayer/FfmpegExtractor.kt index 76e384929..e68cbc6ad 100644 --- a/android/app/src/main/kotlin/com/edde746/plezy/exoplayer/FfmpegExtractor.kt +++ b/android/app/src/main/kotlin/com/edde746/plezy/exoplayer/FfmpegExtractor.kt @@ -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") } } diff --git a/android/app/src/main/kotlin/com/edde746/plezy/exoplayer/FfmpegRandomAccessSource.kt b/android/app/src/main/kotlin/com/edde746/plezy/exoplayer/FfmpegRandomAccessSource.kt index 2a7227ef0..d0286545e 100644 --- a/android/app/src/main/kotlin/com/edde746/plezy/exoplayer/FfmpegRandomAccessSource.kt +++ b/android/app/src/main/kotlin/com/edde746/plezy/exoplayer/FfmpegRandomAccessSource.kt @@ -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 diff --git a/android/app/src/test/kotlin/com/edde746/plezy/exoplayer/FfmpegRandomAccessSourceTest.kt b/android/app/src/test/kotlin/com/edde746/plezy/exoplayer/FfmpegRandomAccessSourceTest.kt index 7a7cd9e0e..95aeee143 100644 --- a/android/app/src/test/kotlin/com/edde746/plezy/exoplayer/FfmpegRandomAccessSourceTest.kt +++ b/android/app/src/test/kotlin/com/edde746/plezy/exoplayer/FfmpegRandomAccessSourceTest.kt @@ -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()