JMR-dev/LibreMediaConverter
1
0
Fork 0
Code Issues 16 Pull Requests Actions Packages Projects Releases Wiki Activity

Merge remote-tracking branch 'origin/main' into m-117-tmp

Browse Source
This commit is contained in:
JMR-dev
2026-08-25 23:04:11 -05:00
parent 3f731d8ea7 cd77aceff4
commit b93ef79931
8 changed files with 801 additions and 19 deletions
Download Patch File Download Diff File Expand all files Collapse all files
app/src/main/java/org/libremediaconverter/convert/ConversionViewModel.kt
+76 -13
View File
@@ -193,6 +193,25 @@ class ConversionViewModel @JvmOverloads constructor(
private var observer: Job? = null
private var activeWorkId: UUID? = null
/**
* Who is allowed to write to this screen — see [ScreenOwnership] for the rule and why
* cancelling the superseded coroutine is not one.
*
* Every write below that lands after a suspension point is guarded by it: the two in
* [onInputPicked] and the one in [observe].
*
* [save] is the one left out, deliberately — and not because it is safe in both directions.
* Nothing can overwrite what it writes: it is reachable only from [ConversionState.Converted]
* or a [ConversionState.Failed] carrying its file, so the only observation that could belongs
* to a job already in a terminal state, which will not emit again. What it can still do is
* land on top of a [reset] taken while its copy was in flight, putting `Saved` on a screen the
* user has just cleared. Guarding it would drop that write instead, reporting nothing for a
* file that may genuinely have reached the user's destination. Which of those two is right is
* a question about what the screen should offer during a save, not about this race, so it is
* filed as issue #123 rather than decided here in passing.
*/
private val ownership = ScreenOwnership()
/**
* The staged output this ViewModel is responsible for deleting.
*
@@ -244,6 +263,10 @@ class ConversionViewModel @JvmOverloads constructor(
* `Data` — see [ConversionState.Converted].
*/
private fun reattach() {
// Read before the launch, and before the query it is about to suspend in. This is the
// claim the answer will belong to: anything the user does from here on supersedes it, and
// reading it on the far side of the query would read whatever superseded it instead.
val token = ownership.current
viewModelScope.launch {
val reattachment = Reattachment.choose(
workManager.jobSnapshots(
@@ -254,8 +277,17 @@ class ConversionViewModel @JvmOverloads constructor(
// The query suspends, so by now the user may have picked a file or started a
// conversion of their own. Either owns the screen; reattaching over it would throw
// away what they just did. Both this check and the assignment below run on the main
// dispatcher with no suspension point between them, so nothing can interleave.
// away what they just did.
//
// This catches a pick that has already *landed*, and only that. It used to claim that
// "both this check and the assignment below run on the main dispatcher with no
// suspension point between them, so nothing can interleave" — which was the exact
// opposite of what happens. There is no assignment below. There is observe(), which
// launches a *separate* coroutine that must suspend on `collect` before it can write
// anything, so the check happens at one moment and the write lands at another with a
// whole pick able to fit in between. That was issue #49, and believing this comment is
// why it read as flaky CI for two days. What actually holds the line is the token
// observe() carries: see [ScreenOwnership].
if (_state.value !is ConversionState.Idle || activeWorkId != null) return@launch
// Only a job that is the sole explanation for its staged file gets to name the input.
@@ -281,7 +313,7 @@ class ConversionViewModel @JvmOverloads constructor(
activeWorkId = reattachment.job.id
// No initial state of our own: the flow's first emission carries the job's real
// state, so observe() maps it exactly as it would for a conversion started here.
observe(reattachment.job.id, input, cancelled = ConversionState.Idle)
observe(reattachment.job.id, input, cancelled = ConversionState.Idle, token = token)
}
}
@@ -297,25 +329,34 @@ class ConversionViewModel @JvmOverloads constructor(
fun setQuality(quality: QualityTier) = _settings.update { it.copy(quality = quality) }
fun setEnginePreference(preference: EnginePreference) = _settings.update { it.copy(enginePreference = preference) }
/**
* The tap is the claim, which is why [ScreenOwnership.claim] is called here and not inside the
* `launch`. A claim made in the coroutine would only be immediate for as long as
* `Dispatchers.Main.immediate` happened to run it inline, and a deferred claim leaves the same
* gap this closes: it is the difference between the user owning the screen from the moment
* they tapped and owning it from whenever their coroutine got around to running.
*/
fun onInputPicked(uri: Uri) {
val token = ownership.claim()
viewModelScope.launch {
// Both the metadata query and the probe touch disk, and the probe spawns FFprobe.
// Neither belongs on the main thread.
val file = withContext(pickDispatcher) { InputQuery.describe(getApplication(), uri) }
// Every write below the hop above is guarded, this one included: two picks in quick
// succession suspend here together, and without this the slower one would land last
// and put the file the user did not choose on screen.
if (!ownership.stillHeldBy(token)) return@launch
// Show the file as soon as its name and size are known. Probing now runs FFprobe on
// every pick, which is a native process spawn, and making the whole screen wait on it
// would read as the app having ignored the tap.
_state.value = ConversionState.Ready(file)
val probe = withContext(pickDispatcher) { probeOrUnreadable(uri) }
// Only fill in the probe if the user has not moved on in the meantime.
_state.update { current ->
if (current is ConversionState.Ready && current.input.uri == uri) {
ConversionState.Ready(file.copy(probe = probe))
} else {
current
}
}
// Only fill in the probe if the user has not moved on in the meantime. The claim is
// what says whether they have -- it covers a second pick of the same URI, which a
// comparison of URIs cannot, and every state a later claim could have written.
if (!ownership.stillHeldBy(token)) return@launch
_state.value = ConversionState.Ready(file.copy(probe = probe))
}
}
@@ -366,10 +407,13 @@ class ConversionViewModel @JvmOverloads constructor(
quality = settings.quality,
enginePreference = settings.enginePreference,
)
// Tapping Convert claims the screen for this job, which is what supersedes the pick's
// still-in-flight probe and any reattachment that has not finished asking.
val token = ownership.claim()
activeWorkId = request.id
workManager.enqueue(request)
_state.value = ConversionState.Converting(input, 0)
observe(request.id, input)
observe(request.id, input, token = token)
}
/**
@@ -377,12 +421,27 @@ class ConversionViewModel @JvmOverloads constructor(
* picked file, ready to convert again. For one picked up by [reattach] there is no picked
* file — the URI that job holds belongs to a process that no longer exists — so it lands
* on Idle instead, rather than offering a Convert button over a file nothing can open.
* @param token the claim this observation belongs to. Nothing here can write until `collect`
* has resumed with a `WorkInfo`, which is some time after the caller decided to observe, so
* the claim is checked again at the last possible moment rather than trusted from then. This
* is issue #49's fix and the only thing standing between a superseded observation and the
* user's screen — see [ScreenOwnership].
*/
private fun observe(id: UUID, input: InputFile, cancelled: ConversionState = ConversionState.Ready(input)) {
private fun observe(
id: UUID,
input: InputFile,
cancelled: ConversionState = ConversionState.Ready(input),
token: Long,
) {
observer?.cancel()
observer = viewModelScope.launch {
workManager.getWorkInfoByIdFlow(id).collect { info ->
if (info == null) return@collect
// Ahead of the `when`, not merely ahead of the assignment: the SUCCEEDED branch
// takes ownership of the staged file, and a superseded observation must not do
// that either. The state and `pendingStaged` are meant to refer to the same file
// or to no file, and this is where that stays true.
if (!ownership.stillHeldBy(token)) return@collect
_state.value = when (info.state) {
WorkInfo.State.RUNNING -> ConversionState.Converting(
input,
@@ -528,6 +587,10 @@ class ConversionViewModel @JvmOverloads constructor(
* existed, this delete was the only thing a failed save could lead to — which was the defect.
*/
fun reset() {
// Start over is a claim like any other. The cancel below is a request honoured at the next
// suspension point, so a collector already on its way to a write has nothing left to
// honour it at; the claim is what actually stops that write landing on top of Idle.
ownership.claim()
observer?.cancel()
observer = null
activeWorkId = null
app/src/main/java/org/libremediaconverter/convert/ScreenOwnership.kt
+57
View File
@@ -0,0 +1,57 @@
package org.libremediaconverter.convert
/**
* Which of the things writing to a screen is still allowed to.
*
* Both ViewModels are a state machine written to from several coroutines that each suspend before
* they write: a pick hops to a dispatcher for the metadata query, a reattachment hops for the tag
* query, and an observation of a WorkManager job cannot write at all until its `collect` has
* resumed with a `WorkInfo`. Whoever resumes last wins, which is how issue #49 let a finished job
* from an earlier session take a screen the user had already picked a file on.
*
* The rule this makes enforceable is one line: **every write that lands after a suspension point
* checks the claim it was made under, and drops itself if that claim has been superseded.** The
* claim is taken synchronously, when the user acts; the check happens immediately before the
* write. Superseded work is *dropped*, not reordered — a dropped write cannot come back later.
*
* Cancelling the superseded coroutine is not a substitute and was never going to be. `Job.cancel`
* is a request, honoured at the next suspension point; a collector that has already resumed and is
* on its way to `_state.value = …` has no suspension point left to honour it at, so the write
* lands anyway. Cancellation also cannot help at all in the case #49 actually reported, where
* nothing supersedes the observation until after it has been launched. Both ViewModels still
* cancel their old observer, because leaving a collector running is a leak — but the guarantee
* does not rest on it.
*
* **Confined to the main dispatcher, and that confinement is the atomicity argument.** Every
* claim and every check runs there, with no suspension point between a check and the write it
* guards, so a claim can never land between the two. Nothing here is synchronized and nothing is
* `@Volatile`: making the field visible across threads would invite exactly the off-main use this
* cannot support, and would replace an argument that holds with one that only looks like it does.
*/
internal class ScreenOwnership {
private var claims = 0L
/**
* The claim in force now.
*
* Read by work that is about to suspend and will want to know, when it comes back, whether
* the screen it was reading is still the screen it is writing to. Read it *before* the
* suspension, not after — reading it afterwards would return whatever claim superseded it,
* which is the bug rather than the check for it.
*/
val current: Long get() = claims
/**
* Takes the screen, invalidating every write still in flight under an older claim.
*
* Called synchronously from the user's action rather than from inside the coroutine it
* starts. A claim made inside a `launch` is only immediate while the dispatcher happens to
* run it inline, and a deferred claim is no claim at all: it would leave the same gap this
* exists to close.
*/
fun claim(): Long = ++claims
/** Whether [token] is still the claim in force, and may therefore write. */
fun stillHeldBy(token: Long): Boolean = token == claims
}
app/src/main/java/org/libremediaconverter/join/JoinViewModel.kt
+71 -6
View File
@@ -20,6 +20,7 @@ import org.libremediaconverter.convert.InputFile
import org.libremediaconverter.convert.InputQuery
import org.libremediaconverter.convert.PendingSave
import org.libremediaconverter.convert.STAGED_FILE_GONE_MESSAGE
import org.libremediaconverter.convert.ScreenOwnership
import org.libremediaconverter.model.ConcatStrategy
import org.libremediaconverter.work.ConcatWorker
import org.libremediaconverter.work.JobTags
@@ -76,6 +77,15 @@ class JoinViewModel @JvmOverloads constructor(
app: Application,
/** Where [reset] runs its delete. See the same parameter on `ConversionViewModel`. */
private val cleanupDispatcher: CoroutineDispatcher = Dispatchers.IO,
/**
* Where the metadata query behind a pick runs. See the same parameter on `ConversionViewModel`.
*
* The join side had no such seam, and the gap was not cosmetic: the one write `onInputsPicked`
* makes lands *after* this hop, so a test that wants to ask what happens while a pick is still
* in flight had no way to hold one there. Issue #49's race is exactly that question, and it
* went unasked on this side for as long as the dispatcher was a literal.
*/
private val pickDispatcher: CoroutineDispatcher = Dispatchers.IO,
) : AndroidViewModel(app) {
private val workManager = WorkManager.getInstance(app)
@@ -90,6 +100,20 @@ class JoinViewModel @JvmOverloads constructor(
private var observer: Job? = null
private var activeWorkId: UUID? = null
/**
* Who is allowed to write to this screen -- see [ScreenOwnership], which carries the rule and
* the reason cancelling the superseded coroutine is not one.
*
* The convert side had issue #49 reported against it four times in two days; this side has the
* identical shape and was never reported, because nothing was watching. Every write below that
* lands after a suspension point is guarded: the one in [onInputsPicked] and the one in
* [observe]. [save] is the one left out, deliberately and with the same limit its counterpart
* in `ConversionViewModel` spells out: nothing can overwrite what it writes, but it can still
* land on top of a [reset] taken while its copy was in flight. Which way that should go is a
* question about the save screen rather than about this race -- issue #123.
*/
private val ownership = ScreenOwnership()
/**
* The staged output this ViewModel is responsible for deleting.
*
@@ -116,6 +140,10 @@ class JoinViewModel @JvmOverloads constructor(
* the rules about which job and why.
*/
private fun reattach() {
// Read before the launch, and before the query it is about to suspend in: this is the
// claim the answer belongs to. Reading it on the far side of the query would read whatever
// superseded it, which is the bug rather than the check for it.
val token = ownership.current
viewModelScope.launch {
val reattachment = Reattachment.choose(
workManager.jobSnapshots(
@@ -125,8 +153,15 @@ class JoinViewModel @JvmOverloads constructor(
) ?: return@launch
// The query suspends, so the user may have picked files or started a join in the
// meantime. Theirs wins. No suspension point between this check and the assignment
// below, and both run on the main dispatcher, so nothing can interleave.
// meantime. Theirs wins.
//
// This catches a pick that has already *landed*, and only that. It used to claim there
// was "no suspension point between this check and the assignment below", which was the
// opposite of what happens: there is no assignment below, only observe(), which
// launches a separate coroutine that cannot write until its `collect` resumes. The
// check happens at one moment and the write lands at another, with a whole pick able
// to fit in between -- issue #49. The token observe() carries is what holds that line;
// see [ScreenOwnership].
if (_state.value !is JoinState.Idle || activeWorkId != null) return@launch
// Joins used to stage under one constant name, so two finished joins always reported
@@ -146,19 +181,29 @@ class JoinViewModel @JvmOverloads constructor(
InputFile(Uri.EMPTY, "", sizeBytes = null)
}
activeWorkId = reattachment.job.id
observe(reattachment.job.id, inputs, cancelled = JoinState.Idle)
observe(reattachment.job.id, inputs, cancelled = JoinState.Idle, token = token)
}
}
/**
* The tap is the claim, which is why it is taken here rather than inside the `launch` -- and
* above the early return, so the refusal below is covered by it too. A claim made in the
* coroutine is only immediate while `Dispatchers.Main.immediate` happens to run it inline, and
* a deferred claim leaves exactly the gap this closes.
*/
fun onInputsPicked(uris: List<Uri>) {
val token = ownership.claim()
if (uris.size < 2) {
_state.value = JoinState.Failed("Pick at least two files to join.")
return
}
viewModelScope.launch {
val files = withContext(Dispatchers.IO) {
val files = withContext(pickDispatcher) {
uris.map { InputQuery.describe(getApplication(), it) }
}
// Guarded like every other write that lands after a hop: two picks in quick succession
// suspend here together, and the slower one would otherwise land last.
if (!ownership.stillHeldBy(token)) return@launch
_state.value = JoinState.Ready(files)
}
}
@@ -172,10 +217,13 @@ class JoinViewModel @JvmOverloads constructor(
// did answer would hand the space check a lower bound it would read as a total.
totalBytes = InputQuery.total(inputs.map { it.sizeBytes }),
)
// Tapping Join claims the screen for this job, superseding any reattachment that has not
// finished asking.
val token = ownership.claim()
activeWorkId = request.id
workManager.enqueue(request)
_state.value = JoinState.Joining(inputs)
observe(request.id, inputs)
observe(request.id, inputs, token = token)
}
/**
@@ -183,12 +231,25 @@ class JoinViewModel @JvmOverloads constructor(
* files, ready to join again. For one picked up by [reattach] there are no picked files —
* what that job holds are URIs granted to a process that no longer exists — so it lands on
* Idle rather than offering to re-join files nothing can open.
* @param token the claim this observation belongs to. Nothing here can write until `collect`
* has resumed with a `WorkInfo`, which is some time after the caller decided to observe, so
* the claim is checked again at the last possible moment rather than trusted from then. See
* [ScreenOwnership], and issue #49.
*/
private fun observe(id: UUID, inputs: List<InputFile>, cancelled: JoinState = JoinState.Ready(inputs)) {
private fun observe(
id: UUID,
inputs: List<InputFile>,
cancelled: JoinState = JoinState.Ready(inputs),
token: Long,
) {
observer?.cancel()
observer = viewModelScope.launch {
workManager.getWorkInfoByIdFlow(id).collect { info ->
if (info == null) return@collect
// Ahead of the `when`, not merely ahead of the assignment: the SUCCEEDED branch
// takes ownership of the staged file, and a superseded observation must not do
// that either.
if (!ownership.stillHeldBy(token)) return@collect
_state.value = when (info.state) {
WorkInfo.State.RUNNING, WorkInfo.State.BLOCKED -> JoinState.Joining(inputs)
WorkInfo.State.ENQUEUED ->
@@ -301,6 +362,10 @@ class JoinViewModel @JvmOverloads constructor(
* this button, so deletion is what the user chose rather than all this state could do.
*/
fun reset() {
// Start over is a claim like any other. The cancel below is a request honoured at the next
// suspension point, so a collector already on its way to a write has nothing left to
// honour it at; the claim is what stops that write landing on top of Idle.
ownership.claim()
observer?.cancel()
observer = null
activeWorkId = null