Refuse a spec that would produce an empty file, and guard the Media3 export that used to die building one #113

Merged
JMR-dev merged 4 commits from fix/empty-composition-crash into main 2026-08-26 02:17:10 +00:00
4 changed files with 263 additions and 33 deletions
Showing only changes of commit 238142d9cc - Show all commits
@@ -11,15 +11,26 @@ import kotlinx.coroutines.runBlocking
import kotlinx.coroutines.withTimeout
import org.junit.After
import org.junit.Assert.assertEquals
import org.junit.Assert.assertFalse
import org.junit.Assert.assertNull
import org.junit.Assert.assertTrue
import org.junit.Before
import org.junit.Test
import org.junit.runner.RunWith
import org.libremediaconverter.FailsOnEmulatorApi37
import org.libremediaconverter.model.AudioCodec
import org.libremediaconverter.model.AudioPlan
import org.libremediaconverter.model.Container
import org.libremediaconverter.model.ConversionRequest
import org.libremediaconverter.model.CopyPlanner
import org.libremediaconverter.model.InputKind
import org.libremediaconverter.model.InputProbe
import org.libremediaconverter.model.OutputFormat
import org.libremediaconverter.model.OutputSpec
import org.libremediaconverter.model.VideoCodec
import org.libremediaconverter.model.VideoPlan
import java.io.File
import java.util.concurrent.CancellationException
import java.util.concurrent.Executors
import java.util.concurrent.TimeUnit
@@ -170,6 +181,63 @@ class Media3EngineTest {
)
}
/**
* The builders that used to throw where nothing could catch them.
*
* `EditedMediaItem.Builder` rejects a composition with both tracks removed —
* checkState("Audio and video cannot both be removed") — and the engine builds it on its own
* HandlerThread. That build sat *between* two narrow `runCatching` blocks, one around
* `buildTransformer` and one around `start`, so the exception reached the thread's uncaught
* handler and took the process with it while the continuation was never resumed.
*
* `ContainerCapabilities.validate` now refuses the spec that gets here from the picker; this
* is the other half — the engine surviving a request that arrives without being validated.
* Deliberately not `@FailsOnEmulatorApi37`: nothing here decodes or encodes, so no emulator
* codec is involved. The builder refuses the input before any media is touched.
*/
@Test
fun aPlanThatRemovesBothTracksFailsInsteadOfKillingTheProcess() {
val request = ConversionRequest(
spec = OutputSpec(Container.MP4, VideoCodec.H265, AudioCodec.NONE),
probe = InputProbe(
videoCodec = null,
audioCodec = "mp3",
hasVideo = false,
container = Container.MP3,
kind = InputKind.AUDIO_ONLY,
),
)
// Asserted rather than assumed: ConversionRequest's default probe says hasVideo = true,
// and with it this same spec plans to (Encode, Drop) and nothing throws at all — which
// would make the whole test vacuous without a word of warning.
val plan = CopyPlanner.plan(request.spec, request.probe)
assertEquals(VideoPlan.Drop, plan.video)
assertEquals(AudioPlan.Drop, plan.audio)
val failure = runCatching {
runBlocking {
withTimeout(BUILDER_TIMEOUT_MS) {
engine.transcode(Uri.fromFile(input), output, request) {}
}
}
}.exceptionOrNull()
// Two assertions, and the second is not pedantry. withTimeout raises
// TimeoutCancellationException, and `java.util.concurrent.CancellationException` *extends*
// IllegalStateException — so testing only the type below would call an unresumed
// continuation a pass. A hang is the other half of this defect and every bit as bad as the
// crash: the worker would sit holding a foreground service forever.
assertFalse(
"the continuation was never resumed — the failure escaped instead of being reported: " +
"$failure",
failure is CancellationException,
)
assertTrue(
"the builder's refusal must surface as a failed job, not a dead process; got $failure",
failure is IllegalStateException,
)
}
private fun durationMsOf(file: File): Long {
val extractor = MediaExtractor()
return try {
@@ -211,5 +279,12 @@ class Media3EngineTest {
private companion object {
const val TIMEOUT_SECONDS = 120L
/**
* Short on purpose. Nothing is decoded or encoded on this path — the builder refuses the
* input outright — so anything approaching this is a hang, which is what the test is
* looking for.
*/
const val BUILDER_TIMEOUT_MS = 30_000L
}
}
@@ -68,42 +68,64 @@ class Media3Engine(private val context: Context) : HardwareTranscoder {
): Unit = suspendCancellableCoroutine { cont ->
val plan = CopyPlanner.plan(request.spec, request.probe)
handler.post {
val transformer = runCatching { buildTransformer(plan, cont) }
.getOrElse {
cont.resumeWithException(it)
return@post
}
// Dropping the tracks the target does not have is what stops an audio-only export
// from carrying a re-encoded video track. Without setRemoveVideo, asking for M4A
// produced an HEVC stream in a file named .m4a.
val item = EditedMediaItem.Builder(MediaItem.fromUri(input))
.setRemoveVideo(plan.video == VideoPlan.Drop)
.setRemoveAudio(plan.audio == AudioPlan.Drop)
.build()
// A Composition is the only way to ask for transmuxing; the plain
// start(EditedMediaItem, path) overload always re-encodes. This is the remux path.
val composition = Composition.Builder(EditedMediaItemSequence.Builder(item).build())
.setTransmuxVideo(plan.video == VideoPlan.Copy)
.setTransmuxAudio(plan.audio == AudioPlan.Copy)
.build()
cont.invokeOnCancellation {
// cancel() has the same single-thread requirement as start().
handler.post { runCatching { transformer.cancel() } }
}
runCatching { transformer.start(composition, output.absolutePath) }
.onFailure {
cont.resumeWithException(it)
return@post
}
pollProgress(transformer, cont, onProgress)
// One guard around the whole body, deliberately.
//
// This used to be two narrow ones — around `buildTransformer` and around
// `transformer.start` — with the two Media3 builders sitting unguarded between them.
// On this thread that is not a small gap: nothing here has a caller to throw back to,
// so an escaping exception reaches the HandlerThread's uncaught handler and takes the
// process down, while [cont] is never resumed either way. `EditedMediaItem.Builder`
// does exactly that for a plan that drops both tracks
// ("Audio and video cannot both be removed"), which a queued job can still carry.
// Widening the guard costs nothing on success and turns every such refusal into a
// failed job with a reason.
runCatching { startExport(input, output, plan, cont, onProgress) }
.onFailure { if (cont.isActive) cont.resumeWithException(it) }
}
}
/**
* Builds the export and hands it to Transformer. Runs on the HandlerThread; may throw.
*
* Everything Transformer's single-thread contract covers lives here, so that the caller has
* exactly one place to catch. Returning normally means the export is running and [cont] belongs
* to the listener; throwing means it never started and the caller owns resuming.
*/
private fun startExport(
input: Uri,
output: File,
plan: ConversionPlan,
cont: CancellableContinuation<Unit>,
onProgress: (Int) -> Unit,
) {
val transformer = buildTransformer(plan, cont)
// Dropping the tracks the target does not have is what stops an audio-only export
// from carrying a re-encoded video track. Without setRemoveVideo, asking for M4A
// produced an HEVC stream in a file named .m4a.
val item = EditedMediaItem.Builder(MediaItem.fromUri(input))
.setRemoveVideo(plan.video == VideoPlan.Drop)
.setRemoveAudio(plan.audio == AudioPlan.Drop)
.build()
// A Composition is the only way to ask for transmuxing; the plain
// start(EditedMediaItem, path) overload always re-encodes. This is the remux path.
val composition = Composition.Builder(EditedMediaItemSequence.Builder(item).build())
.setTransmuxVideo(plan.video == VideoPlan.Copy)
.setTransmuxAudio(plan.audio == AudioPlan.Copy)
.build()
// Registered before start(), so a cancellation racing the export always finds a
// transformer to cancel.
cont.invokeOnCancellation {
// cancel() has the same single-thread requirement as start().
handler.post { runCatching { transformer.cancel() } }
}
transformer.start(composition, output.absolutePath)
pollProgress(transformer, cont, onProgress)
}
/**
* @throws IllegalArgumentException if [plan] names a container Media3 cannot mux. That is a
* routing bug rather than a runtime condition — [org.libremediaconverter.model.ConversionRouter]
@@ -0,0 +1,105 @@
package org.libremediaconverter.convert
import android.net.Uri
import androidx.media3.common.util.UnstableApi
import kotlinx.coroutines.runBlocking
import kotlinx.coroutines.withTimeout
import org.junit.Assert.assertEquals
import org.junit.Assert.assertFalse
import org.junit.Assert.assertTrue
import org.junit.Test
import org.junit.runner.RunWith
import org.libremediaconverter.model.AudioCodec
import org.libremediaconverter.model.AudioPlan
import org.libremediaconverter.model.Container
import org.libremediaconverter.model.ConversionRequest
import org.libremediaconverter.model.CopyPlanner
import org.libremediaconverter.model.InputKind
import org.libremediaconverter.model.InputProbe
import org.libremediaconverter.model.OutputSpec
import org.libremediaconverter.model.VideoCodec
import org.libremediaconverter.model.VideoPlan
import org.robolectric.RobolectricTestRunner
import org.robolectric.RuntimeEnvironment
import java.io.File
import java.util.concurrent.CancellationException
/**
* What happens when Media3 refuses the export before it starts.
*
* `EditedMediaItem.Builder` rejects a composition with both tracks removed —
* checkState("Audio and video cannot both be removed") — and [Media3Engine] builds it on its own
* HandlerThread. That build used to sit *between* two narrow `runCatching` blocks, one around
* `buildTransformer` and one around `start`, so the exception escaped `handler.post`'s body: it
* reached the thread's uncaught handler, which on Android takes the process down, and the
* continuation was left unresumed either way.
*
* Robolectric runs the real [android.os.HandlerThread] and the real Media3 builders, so the whole
* sequence happens here — the engine really posts, really builds, and really throws. What it cannot
* reproduce is the *consequence* of an escaped throw: a JVM background thread dying is not process
* death. So the assertion is on the half that is observable everywhere and is the half that
* matters to the user — the suspension is resolved, with the reason, rather than left hanging.
* `Media3EngineTest.aPlanThatRemovesBothTracksFailsInsteadOfKillingTheProcess` is the same case on
* a device.
*/
@UnstableApi
@RunWith(RobolectricTestRunner::class)
class Media3EngineEmptyCompositionTest {
@Test
fun `a plan that removes both tracks fails the job instead of escaping the handler thread`() {
val context = RuntimeEnvironment.getApplication()
val engine = Media3Engine(context)
val request = ConversionRequest(
spec = OutputSpec(Container.MP4, VideoCodec.H265, AudioCodec.NONE),
probe = InputProbe(
videoCodec = null,
audioCodec = "mp3",
hasVideo = false,
container = Container.MP3,
kind = InputKind.AUDIO_ONLY,
),
)
// Asserted rather than assumed: ConversionRequest's default probe says hasVideo = true, and
// with it this same spec plans to (Encode, Drop), nothing throws, and the test would pass
// over a code path it never entered.
val plan = CopyPlanner.plan(request.spec, request.probe)
assertEquals(VideoPlan.Drop, plan.video)
assertEquals(AudioPlan.Drop, plan.audio)
val failure = try {
runCatching {
runBlocking {
withTimeout(TIMEOUT_MS) {
engine.transcode(Uri.parse("file:///dev/null"), File(context.cacheDir, "empty.mp4"), request) {}
}
}
}.exceptionOrNull()
} finally {
engine.close()
}
// Both halves are load-bearing, and the second is not pedantry: withTimeout raises
// TimeoutCancellationException, and `java.util.concurrent.CancellationException` *extends*
// IllegalStateException — so testing only the first would call an unresumed continuation a
// pass. This assertion was written that way, and the mutation is what found it.
assertFalse(
"the continuation was never resumed — the failure escaped instead of being reported: $failure",
failure is CancellationException,
)
assertTrue(
"the builder's refusal must surface as a failed job; got $failure",
failure is IllegalStateException,
)
}
private companion object {
/**
* Short on purpose. Nothing is decoded, encoded or muxed on this path — the builder refuses
* the input outright — so anything approaching this is a hang, which is the failure mode
* this test is looking for.
*/
const val TIMEOUT_MS = 10_000L
}
}
@@ -350,6 +350,34 @@ class ConversionRouterTest {
}
}
/**
* Why `Media3Engine` still needs a guard of its own.
*
* `ContainerCapabilities.validate` now refuses "a video codec with the audio off" for an input
* with no video track, so neither the picker nor `ConversionWorker` will start one. Routing is
* a separate question and still answers MEDIA3 — nothing about a dropped track makes the job
* un-hardware-able — so a request that skips validation, from a direct
* `ConversionWorker.request(...)` or a job queued before the settings changed, arrives at the
* engine with a plan Media3 cannot build. That has to fail the job, not the process.
*/
@Test
fun `a plan that drops both tracks still routes to media3`() {
val audioOnly = InputProbe(
videoCodec = null,
audioCodec = "mp3",
hasVideo = false,
container = Container.MP3,
kind = InputKind.AUDIO_ONLY,
)
val spec = OutputSpec(Container.MP4, VideoCodec.H265, AudioCodec.NONE)
val plan = CopyPlanner.plan(spec, audioOnly)
assertEquals(VideoPlan.Drop, plan.video)
assertEquals(AudioPlan.Drop, plan.audio)
assertEquals(Engine.MEDIA3, route(spec, probe = audioOnly).engine)
}
@Test
fun `audio-only formats are flagged as such`() {
assertEquals(true, OutputFormat.MP3.isAudioOnly)