Files
LibreMediaConverter/app/src/androidTest/java/org/libremediaconverter/join/ConcatEngineTest.kt
T
JMR-devandClaude Opus 5 2a68f03134 Give every job a staging path of its own
`<cacheDir>/conversions/` is shared by the convert tab, the join tab and `ConcatEngine`, and
until now none of the three named a file that belonged to one job. A conversion derived its name
from the input's display name, so two `holiday.mp4` from different folders wrote the same file.
A join used the constant `joined.<ext>`, so any two joins of one format did. The list file was
the constant `concat_list.txt`, so any two joins at all did, and one of them would read the
other's input list.

The naming half is not a hypothesis. Two independent conversions on a Pixel each computed
`cache/conversions/input_converted.mp4`, the second silently overwrote the first, and a tag query
in a fresh process then returned **two SUCCEEDED `WorkInfo`s naming that one file** with one file
on disk. That is the collision reaching the point where it makes a *fix* ambiguous rather than
just a file: `Reattachment` can offer the bytes, because they are the user's either way, but it
cannot say which job produced them.

`StagingNames` keys the name on the WorkManager request id. That id is what stays still across a
retry -- `WorkerWrapper` builds `WorkerParameters` from the `WorkSpec` id and only increments
`runAttemptCount` -- which matters more here than uniqueness does, and matters more since the
previous commit made retries routine. A failed attempt deletes its staged file on the way out,
and that only collects the partial the *previous* attempt left when the name has not moved.

Opaque rather than sanitised, deliberately. The staged name is never shown to anyone: `save()`
recomputes a suggested name and the user picks the real one in the SAF dialog. So there was
nothing to lose by dropping the display name, and something to gain -- a provider-supplied
display name can contain a separator, be empty, or be four kilobytes long, and `File(stagingDir,
"../escape_converted.mp4")` resolves to a path outside staging. That was reachable before this
commit and is now unreachable by construction rather than by a sanitiser that has to be right
about every case. There is a test for exactly that name.

The extension stays, and is not decoration: `FFmpegConcatCommand` names no output muxer, so
FFmpeg infers it from the output path. A fully opaque name would quietly produce the wrong
container.

`ConcatEngine`'s list file is derived from the output it belongs to rather than taking another
parameter, so the two cannot drift apart, a directory listing shows which list belongs to which
join, and the sweep ages them together.

Three neighbouring comments claimed things that are no longer true, and are corrected rather than
left to mislead the next reader:

  - `Reattachment.Ambiguous` said it "resolves on its own once each job stages under a name of
    its own". It now does -- for work enqueued from here on. The case is **kept**, because the
    queue outlives the change: WorkManager holds finished work for about a week, and the jobs
    likeliest to be sitting in it when this code first runs are the ones named the old way.
    Behaviour is unchanged and `ReattachmentTest` is untouched.
  - `OutputPublisher.sweepStaging` justified its age rule partly on there being "no per-job
    namespacing". There is now, and the rule still stands on its own: per-job names stop two jobs
    from sharing a file, and say nothing about whether a file's job is still running, which is the
    question a sweep actually asks. Same for `StagingSweep` and the note in
    `LibreMediaConverterApp`.
  - Both ViewModels' `reattach()` explained aliasing as something nothing prevented. Narrowed to
    what is still true of work already in the queue.

`ConcatEngineTest` asks `StagingNames` for the list file's name instead of spelling out
`concat_list.txt`. That is the difference between a test and a tautology: a literal there would
have gone on passing after the rename while asserting that a file nothing creates does not exist.
The same trap was live in the two worker tests from the previous commits, whose staged-file
assertions computed a path of their own -- they now assert on the staging directory being empty,
which cannot go vacuous when a name moves.

`PerJobStagingTest` drives the real worker, because the naming function was never the part that
was wrong: what was wrong was which name the worker asked for. Two jobs converting one file must
leave two files; a second attempt at one job must not leave a second; and a display name that
climbs out of staging must not. The first and third fail before the change with "each job must
have staged its own file, found [input_converted.mp4] expected:<2> but was:<1>" and "the output
belongs in staging expected:<1> but was:<0>" -- the latter because the file had landed in
`cacheDir` instead.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-22 20:11:44 -05:00

184 lines
6.7 KiB
Kotlin

package org.libremediaconverter.join
import android.media.MediaExtractor
import android.media.MediaFormat
import android.net.Uri
import androidx.test.ext.junit.runners.AndroidJUnit4
import androidx.test.platform.app.InstrumentationRegistry
import kotlinx.coroutines.runBlocking
import org.junit.After
import org.junit.Assert.assertEquals
import org.junit.Assert.assertTrue
import org.junit.Before
import org.junit.Test
import org.junit.runner.RunWith
import org.libremediaconverter.convert.MediaProbe
import org.libremediaconverter.convert.StagingNames
import org.libremediaconverter.ffmpeg.ConcatEngine
import org.libremediaconverter.model.ConcatStrategy
import java.io.File
/**
* The join path, end to end on a device.
*
* The point of these tests is the strategy decision, not merely that a file appears.
* FFmpeg's `concat` demuxer does not reliably reject mismatched inputs -- it can emit a
* file whose later segments are garbled -- so "it produced output" is not evidence of
* correctness. Each test therefore checks which strategy ran *and* that the result is
* long enough to contain both inputs.
*/
@RunWith(AndroidJUnit4::class)
class ConcatEngineTest {
private val context = InstrumentationRegistry.getInstrumentation().targetContext
private val engine = ConcatEngine(context)
private val staged = mutableListOf<File>()
private lateinit var clipA: File
private lateinit var clipB: File
private lateinit var clipMismatched: File
@Before
fun setUp() {
clipA = copyAsset("clip_a.mp4")
clipB = copyAsset("clip_b.mp4")
clipMismatched = copyAsset("clip_c_mismatched.mp4")
}
@After
fun tearDown() {
(staged + listOf(clipA, clipB, clipMismatched)).forEach { it.delete() }
}
private fun copyAsset(name: String): File {
val out = File(context.cacheDir, name)
InstrumentationRegistry.getInstrumentation().context.assets
.open(name)
.use { asset -> out.outputStream().use { asset.copyTo(it) } }
return out
}
private fun output(name: String) = File(context.cacheDir, name).also {
it.delete()
staged += it
}
private fun durationMs(file: File): Long {
val extractor = MediaExtractor()
return try {
extractor.setDataSource(file.absolutePath)
(0 until extractor.trackCount)
.map { extractor.getTrackFormat(it) }
.filter { it.containsKey(MediaFormat.KEY_DURATION) }
.maxOfOrNull { it.getLong(MediaFormat.KEY_DURATION) / 1000 } ?: 0L
} finally {
extractor.release()
}
}
// --- the fast path ------------------------------------------------------
@Test
fun matchingClipsAreJoinedByStreamCopy(): Unit = runBlocking {
val out = output("joined_matching.mp4")
val result = engine.join(listOf(Uri.fromFile(clipA), Uri.fromFile(clipB)), out)
assertEquals(
"identical inputs should not need re-encoding",
ConcatStrategy.STREAM_COPY,
result.strategy,
)
assertTrue("no output produced", out.exists() && out.length() > 0)
// Both 2 s inputs must be present, not just the first.
assertTrue(
"joined duration ${durationMs(out)}ms is too short to hold both clips",
durationMs(out) >= 3_500,
)
}
// --- the correctness path ----------------------------------------------
@Test
fun mismatchedClipsAreReEncodedRatherThanStreamCopied(): Unit = runBlocking {
val out = output("joined_mismatched.mp4")
val result = engine.join(listOf(Uri.fromFile(clipA), Uri.fromFile(clipMismatched)), out)
// This is the case a naive implementation gets wrong: the demuxer would accept
// these and produce a corrupt second half.
assertEquals(
"differing resolution must force a re-encode",
ConcatStrategy.REENCODE,
result.strategy,
)
assertTrue("no output produced", out.exists() && out.length() > 0)
assertTrue(
"joined duration ${durationMs(out)}ms is too short to hold both clips",
durationMs(out) >= 3_500,
)
}
@Test
fun reEncodedOutputIsPlayableAndCarriesBothTracks(): Unit = runBlocking {
val out = output("joined_playable.mp4")
engine.join(listOf(Uri.fromFile(clipA), Uri.fromFile(clipMismatched)), out)
val extractor = MediaExtractor()
try {
extractor.setDataSource(out.absolutePath)
val mimes = (0 until extractor.trackCount).map {
extractor.getTrackFormat(it).getString(MediaFormat.KEY_MIME).orEmpty()
}
assertTrue("no video track in $mimes", mimes.any { it.startsWith("video/") })
assertTrue("no audio track in $mimes", mimes.any { it.startsWith("audio/") })
} finally {
extractor.release()
}
}
// --- guards -------------------------------------------------------------
@Test
fun joiningRefusesFewerThanTwoInputs() {
val out = output("joined_single.mp4")
val failure = runCatching {
runBlocking { engine.join(listOf(Uri.fromFile(clipA)), out) }
}.exceptionOrNull()
assertTrue(
"expected an IllegalArgumentException, got $failure",
failure is IllegalArgumentException,
)
}
@Test
fun theListFileIsCleanedUpAfterJoining(): Unit = runBlocking {
val out = output("joined_cleanup.mp4")
engine.join(listOf(Uri.fromFile(clipA), Uri.fromFile(clipB)), out)
// Asked of StagingNames rather than spelled out: the list file used to be the constant
// concat_list.txt, and a literal here would have gone on passing vacuously once the name
// moved -- it would be asserting that a file nothing creates does not exist.
assertTrue(
"the concat list file was left behind",
!File(out.parentFile, StagingNames.concatListFor(out.name)).exists(),
)
}
// --- the probe the planner depends on ----------------------------------
@Test
fun probeReadsThePropertiesTheStrategyDependsOn() {
val a = MediaProbe.probeForConcat(context, Uri.fromFile(clipA))
val mismatched = MediaProbe.probeForConcat(context, Uri.fromFile(clipMismatched))
assertEquals("h264", a.videoCodec)
assertEquals(320, a.width)
assertEquals(240, a.height)
assertEquals(640, mismatched.width)
assertEquals(480, mismatched.height)
assertTrue(
"the probe must actually distinguish these clips, or the planner cannot",
a.width != mismatched.width || a.height != mismatched.height,
)
}
}