diff --git a/app/src/main/java/org/libremediaconverter/convert/ConversionViewModel.kt b/app/src/main/java/org/libremediaconverter/convert/ConversionViewModel.kt index d26adb0..3cc9352 100644 --- a/app/src/main/java/org/libremediaconverter/convert/ConversionViewModel.kt +++ b/app/src/main/java/org/libremediaconverter/convert/ConversionViewModel.kt @@ -2,7 +2,6 @@ package org.libremediaconverter.convert import android.app.Application import android.net.Uri -import android.provider.OpenableColumns import android.util.Log import androidx.lifecycle.AndroidViewModel import androidx.lifecycle.viewModelScope @@ -53,7 +52,15 @@ data class ConversionSettings( data class InputFile( val uri: Uri, val displayName: String, - val sizeBytes: Long, + /** + * How big the file is, or null when nothing could say. + * + * Nullable rather than `0L`, and that is the point of it. The two were the same value before, + * so an unmeasurable file arrived at the space check claiming to be empty. [InputQuery] owns + * how the answer is found and what it means; every reader of this has to decide what an + * unknown size does, which is exactly the decision the old default made silently. + */ + val sizeBytes: Long?, /** * What probing found. Null only while the probe is still running. * @@ -206,7 +213,10 @@ class ConversionViewModel @JvmOverloads constructor( // card its source details, and re-probing is what there is no URI for. uri = Uri.EMPTY, displayName = JobTags.displayNameOf(tags) ?: UNKNOWN_INPUT_NAME, - sizeBytes = JobTags.sizeBytesOf(tags) ?: 0L, + // No `?: 0L`. A job tagged before sizes were tagged at all, or one enqueued + // for a file nothing could measure, has no size -- and answering that with + // zero is the same conflation this whole change is about. See [InputQuery]. + sizeBytes = JobTags.sizeBytesOf(tags), ) activeWorkId = reattachment.job.id // No initial state of our own: the flow's first emission carries the job's real @@ -231,7 +241,7 @@ class ConversionViewModel @JvmOverloads constructor( 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(Dispatchers.IO) { queryFile(uri) } + val file = withContext(Dispatchers.IO) { InputQuery.describe(getApplication(), uri) } // 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. @@ -451,24 +461,6 @@ class ConversionViewModel @JvmOverloads constructor( else -> null } - private fun queryFile(uri: Uri): InputFile { - var name = "input" - var size = 0L - getApplication().contentResolver - .query(uri, null, null, null, null) - ?.use { cursor -> - if (cursor.moveToFirst()) { - cursor.getColumnIndex(OpenableColumns.DISPLAY_NAME) - .takeIf { it >= 0 } - ?.let { name = cursor.getString(it) ?: name } - cursor.getColumnIndex(OpenableColumns.SIZE) - .takeIf { it >= 0 } - ?.let { size = cursor.getLong(it) } - } - } - return InputFile(uri, name, size) - } - private companion object { /** * Shown for a reattached job whose tags predate them — work enqueued by an earlier diff --git a/app/src/main/java/org/libremediaconverter/convert/ConverterScreen.kt b/app/src/main/java/org/libremediaconverter/convert/ConverterScreen.kt index a271860..921b348 100644 --- a/app/src/main/java/org/libremediaconverter/convert/ConverterScreen.kt +++ b/app/src/main/java/org/libremediaconverter/convert/ConverterScreen.kt @@ -422,7 +422,13 @@ private fun FileCard(input: InputFile) { Card(modifier = Modifier.fillMaxWidth()) { Column(modifier = Modifier.padding(16.dp)) { Text(input.displayName, style = MaterialTheme.typography.titleMedium) - Text(formatBytes(input.sizeBytes), style = MaterialTheme.typography.bodySmall) + // The null is handled here rather than inside formatBytes, because "no provider would + // say" is not a number and a formatter that invented one -- "0 B" -- is the defect + // this card would be showing. It degrades in words, like the codec rows below it. + Text( + input.sizeBytes?.let(::formatBytes) ?: "Size unknown", + style = MaterialTheme.typography.bodySmall, + ) val probe = input.probe if (probe == null) { diff --git a/app/src/main/java/org/libremediaconverter/convert/InputQuery.kt b/app/src/main/java/org/libremediaconverter/convert/InputQuery.kt new file mode 100644 index 0000000..0dd1730 --- /dev/null +++ b/app/src/main/java/org/libremediaconverter/convert/InputQuery.kt @@ -0,0 +1,102 @@ +package org.libremediaconverter.convert + +import android.content.Context +import android.database.Cursor +import android.net.Uri +import android.provider.OpenableColumns +import android.util.Log + +/** + * What the app can find out about a picked file before an engine opens it. + * + * One place rather than two: `queryFile` existed in `ConversionViewModel` and `JoinViewModel` + * byte for byte, so a fix to either was a fix to half the app. + * + * **An unknown size is null here, never zero.** That distinction is the whole point of this file. + * The old code started at `var size = 0L` and only moved off it when a provider answered the + * `OpenableColumns.SIZE` column, so "this file is empty" and "nobody told me how big it is" + * reached `OutputPublisher.hasSpaceFor` as the same number — and `hasSpaceFor(0)` is only "is + * there 128 MB free". Confirmed live on a Pixel 10 Pro XL, where `contentResolver.query` on a + * `file://` URI returns null outright and the default survived untouched: + * `queryFile gave displayName='input' sizeBytes=0`. + * + * So the size is asked for twice, in order: + * + * 1. **What the provider says.** `OpenableColumns.SIZE`, which documents providers *may* omit. + * 2. **What the file itself says.** `openFileDescriptor(uri, "r")` and `statSize`, which needs + * no cooperation from a provider beyond being openable — and the app is going to have to + * open the input anyway, so it is not asking for anything a conversion would not need. This + * is what answers the `file://` case above. + * + * Only when both decline is the answer null, and the callers each say what they do about that. + */ +object InputQuery { + + /** + * Shown when no provider names the file. + * + * Kept exactly as it was — it is what reaches the save dialog as `input_converted.mp4` for a + * job whose input nothing described, and changing it here would rename files for reasons + * unrelated to this fix. + */ + const val FALLBACK_DISPLAY_NAME = "input" + + /** Everything the picker knows about [uri] the moment it is chosen. */ + fun describe(context: Context, uri: Uri): InputFile = InputFile( + uri = uri, + displayName = firstRow(context, uri) { it.displayNameOrNull() } ?: FALLBACK_DISPLAY_NAME, + sizeBytes = sizeOf(context, uri), + ) + + /** + * How many bytes [uri] holds, or null when nothing can say. + * + * Public because the workers need it too, and for a reason worth stating: a worker's input + * `Data` carries the size the *picker* found, which is missing for work enqueued before this + * existed and for a request built by hand. The worker holds the URI, so when the number is + * absent it can ask the file rather than assume. + */ + fun sizeOf(context: Context, uri: Uri): Long? = firstRow(context, uri) { it.sizeOrNull() } ?: measure(context, uri) + + /** + * The sum of [sizes], or null if even one of them is unknown. + * + * A join's total is only as good as its worst-known part. Adding up the ones that answered + * would produce a lower bound that reads exactly like a real total, and the space check has + * no way to tell the two apart — which is the same conflation this whole file exists to end. + */ + fun total(sizes: List): Long? = sizes.fold(0L as Long?) { running, size -> + if (running == null || size == null) null else running + size + } + + /** + * Reads [read] out of the first row of a metadata query, or null if there is no row. + * + * Guarded because a resolver call is a call into another app: a provider that has been + * uninstalled, revoked its grant, or simply crashes takes the query with it, and a file + * picker is not a place to bring the process down from. + */ + private fun firstRow(context: Context, uri: Uri, read: (Cursor) -> T): T? = runCatching { + context.contentResolver.query(uri, null, null, null, null)?.use { cursor -> + if (cursor.moveToFirst()) read(cursor) else null + } + }.onFailure { Log.w(TAG, "Could not read metadata for $uri", it) }.getOrNull() + + /** + * The size according to the file descriptor, or null if it cannot be opened. + * + * `statSize` is `-1` for anything without a fixed length — a pipe, or a provider streaming its + * answer — which is a different way of saying "unknown" and is treated as one. + */ + private fun measure(context: Context, uri: Uri): Long? = runCatching { + context.contentResolver.openFileDescriptor(uri, "r")?.use { it.statSize } + }.getOrNull()?.takeIf { it >= 0 } + + private fun Cursor.displayNameOrNull(): String? = + getColumnIndex(OpenableColumns.DISPLAY_NAME).takeIf { it >= 0 && !isNull(it) }?.let(::getString) + + private fun Cursor.sizeOrNull(): Long? = + getColumnIndex(OpenableColumns.SIZE).takeIf { it >= 0 && !isNull(it) }?.let(::getLong)?.takeIf { it >= 0 } + + private const val TAG = "InputQuery" +} diff --git a/app/src/main/java/org/libremediaconverter/convert/OutputPublisher.kt b/app/src/main/java/org/libremediaconverter/convert/OutputPublisher.kt index 8616ccd..ebb8355 100644 --- a/app/src/main/java/org/libremediaconverter/convert/OutputPublisher.kt +++ b/app/src/main/java/org/libremediaconverter/convert/OutputPublisher.kt @@ -29,13 +29,51 @@ open class OutputPublisher(private val context: Context) { open fun createStagingFile(name: String): File = File(stagingDir, name) /** - * True if there is room for a further [bytes], including headroom. + * True if staging can take a further [bytes], with [SPACE_HEADROOM_BYTES] left over. * - * Staging means peak usage is roughly input + output at once, so a job that would - * just barely fit is rejected rather than failing partway through. + * **The doc this replaces claimed peak usage was "roughly input + output at once" while the + * arithmetic reserved `input + 128 MB`.** The arithmetic is what stays, and this says why + * rather than the two continuing to disagree. + * + * [bytes] is the *input's* size standing in for the output's, because before an engine has + * run there is no other number. It is generous for the ordinary conversion, which is asked + * for precisely because it shrinks its input, and short for the ones that do not — a re-encode + * to a bulkier codec, or a stream copy into a container with more overhead. + * + * The 128 MB absorbs that error, and one more besides: [publish] copies the staged file to + * the user's destination, so while that runs the bytes exist twice on any destination sharing + * this volume. Reserving `input + output` outright would have refused jobs that fit, on a + * device where the destination is usually removable or remote. + * + * So this is a pre-flight check that stops a job which obviously cannot fit from spending + * minutes discovering it — not a guarantee. A conversion that runs out of space anyway fails + * through its engine, with a message of its own. + * + * Open so a test can force a full disk; see `FakeFailures` in the instrumented source set. */ open fun hasSpaceFor(bytes: Long): Boolean = stagingDir.usableSpace > bytes + SPACE_HEADROOM_BYTES + /** + * The same check for a job whose input size nobody could determine — see [InputQuery]. + * + * **This deliberately produces the same number the defect produced by accident**, which is + * worth stating plainly: with no size to reserve for, all that is left to check is the + * headroom. What has changed is that it is now the answer to a question that was asked. The + * old code could not tell an unmeasurable file from an empty one, so it silently made this + * the answer for *both*; now [hasSpaceFor] means "there is room for this many bytes" and + * nothing else claims it. + * + * Refusing instead was considered and rejected. It would turn "no provider answered the + * `SIZE` column" into "this file cannot be converted" — a worse defect than the one being + * fixed, and one the user could do nothing about. + * + * The default answers *through* [hasSpaceFor], which is what keeps a publisher that refuses + * on space — `FakeFailures.FullDisk`, which overrides `hasSpaceFor` and nothing else — + * refusing this too. `SpaceCheckTest` pins that delegation, because an override here that + * stopped delegating would quietly stop honouring a full disk. + */ + open fun hasSpaceForUnknownSize(): Boolean = hasSpaceFor(0L) + /** * Copies a finished staging file into a user-chosen SAF destination. * diff --git a/app/src/main/java/org/libremediaconverter/join/JoinViewModel.kt b/app/src/main/java/org/libremediaconverter/join/JoinViewModel.kt index 374d9ff..7b0dc7b 100644 --- a/app/src/main/java/org/libremediaconverter/join/JoinViewModel.kt +++ b/app/src/main/java/org/libremediaconverter/join/JoinViewModel.kt @@ -2,7 +2,6 @@ package org.libremediaconverter.join import android.app.Application import android.net.Uri -import android.provider.OpenableColumns import androidx.lifecycle.AndroidViewModel import androidx.lifecycle.viewModelScope import androidx.media3.common.util.UnstableApi @@ -18,6 +17,7 @@ import kotlinx.coroutines.launch import kotlinx.coroutines.withContext import org.libremediaconverter.convert.ConversionDependencies import org.libremediaconverter.convert.InputFile +import org.libremediaconverter.convert.InputQuery import org.libremediaconverter.model.ConcatStrategy import org.libremediaconverter.work.ConcatWorker import org.libremediaconverter.work.JobTags @@ -117,7 +117,7 @@ class JoinViewModel @JvmOverloads constructor( // render these individually and offer to join them. Anything that starts drawing // this list has to carry the names in the tags first. val inputs = List(JobTags.inputCountOf(tags) ?: MIN_JOIN_INPUTS) { - InputFile(Uri.EMPTY, "", 0L) + InputFile(Uri.EMPTY, "", sizeBytes = null) } activeWorkId = reattachment.job.id observe(reattachment.job.id, inputs, cancelled = JoinState.Idle) @@ -130,7 +130,9 @@ class JoinViewModel @JvmOverloads constructor( return } viewModelScope.launch { - val files = withContext(Dispatchers.IO) { uris.map(::queryFile) } + val files = withContext(Dispatchers.IO) { + uris.map { InputQuery.describe(getApplication(), it) } + } _state.value = JoinState.Ready(files) } } @@ -139,7 +141,10 @@ class JoinViewModel @JvmOverloads constructor( val inputs = (_state.value as? JoinState.Ready)?.inputs ?: return val request = ConcatWorker.request( inputs = inputs.map { it.uri }, - totalBytes = inputs.sumOf { it.sizeBytes }, + // Not `sumOf`, which cannot express what is being summed any more. A join's + // total is only as good as its least-known part, and adding up the inputs that + // did answer would hand the space check a lower bound it would read as a total. + totalBytes = InputQuery.total(inputs.map { it.sizeBytes }), ) activeWorkId = request.id workManager.enqueue(request) @@ -256,22 +261,6 @@ class JoinViewModel @JvmOverloads constructor( _state.value = JoinState.Idle } - private fun queryFile(uri: Uri): InputFile { - var name = "input" - var size = 0L - getApplication().contentResolver - .query(uri, null, null, null, null) - ?.use { cursor -> - if (cursor.moveToFirst()) { - cursor.getColumnIndex(OpenableColumns.DISPLAY_NAME).takeIf { it >= 0 } - ?.let { name = cursor.getString(it) ?: name } - cursor.getColumnIndex(OpenableColumns.SIZE).takeIf { it >= 0 } - ?.let { size = cursor.getLong(it) } - } - } - return InputFile(uri, name, size) - } - private companion object { /** * Used when a reattached job carries no count tag — work enqueued by an earlier version diff --git a/app/src/main/java/org/libremediaconverter/work/ConcatWorker.kt b/app/src/main/java/org/libremediaconverter/work/ConcatWorker.kt index 4ad86e6..553310b 100644 --- a/app/src/main/java/org/libremediaconverter/work/ConcatWorker.kt +++ b/app/src/main/java/org/libremediaconverter/work/ConcatWorker.kt @@ -9,9 +9,11 @@ import androidx.work.Data import androidx.work.ForegroundInfo import androidx.work.OneTimeWorkRequestBuilder import androidx.work.WorkerParameters +import androidx.work.hasKeyWithValueOfType import androidx.work.workDataOf import kotlinx.coroutines.CancellationException import org.libremediaconverter.convert.ConversionDependencies +import org.libremediaconverter.convert.InputQuery import org.libremediaconverter.convert.StagingNames import org.libremediaconverter.ffmpeg.ConcatEngine import org.libremediaconverter.model.OutputFormat @@ -39,12 +41,16 @@ class ConcatWorker(context: Context, params: WorkerParameters) : CoroutineWorker if (uris.size < 2) { return Result.failure(workDataOf(KEY_ERROR to "Pick at least two files to join.")) } - val totalBytes = inputData.getLong(KEY_TOTAL_BYTES, 0L) + // Absent, not zero, when the picker could not size every input -- see the same read in + // ConversionWorker and InputQuery for why the two are no longer one number. + val declaredTotal = inputData + .takeIf { it.hasKeyWithValueOfType(KEY_TOTAL_BYTES) } + ?.getLong(KEY_TOTAL_BYTES, 0L) val format = OutputFormat.valueOf( inputData.getString(KEY_FORMAT) ?: DEFAULT_FORMAT.name, ) - if (!publisher.hasSpaceFor(totalBytes)) { + if (!hasRoomFor(declaredTotal, uris)) { return Result.failure(workDataOf(KEY_ERROR to "Not enough free space to join these files.")) } @@ -104,6 +110,23 @@ class ConcatWorker(context: Context, params: WorkerParameters) : CoroutineWorker } } + /** + * Whether staging can take this join, measuring the inputs when nothing else has. + * + * The same shape as `ConversionWorker.hasRoomFor` and for the same reasons, with one + * difference worth naming: a join's total is [InputQuery.total], which is null the moment a + * *single* input cannot be sized. Summing the ones that answered would produce a lower bound + * indistinguishable from a real total, which is the conflation this change exists to end. + */ + private fun hasRoomFor(declared: Long?, uris: List): Boolean { + val bytes = declared ?: InputQuery.total(uris.map { InputQuery.sizeOf(applicationContext, it) }) + if (bytes == null) { + Log.i(TAG, "Nothing could size every input; checking headroom only.") + return publisher.hasSpaceForUnknownSize() + } + return publisher.hasSpaceFor(bytes) + } + override suspend fun getForegroundInfo(): ForegroundInfo = ForegroundInfo( NOTIFICATION_ID, notifications.build(id, "Joining files", 0, indeterminate = true), @@ -150,13 +173,15 @@ class ConcatWorker(context: Context, params: WorkerParameters) : CoroutineWorker * join screen says about a job in flight, and after a restart nothing else can supply * it. See [JobTags]. */ - fun request(inputs: List, totalBytes: Long, format: OutputFormat = DEFAULT_FORMAT) = + fun request(inputs: List, totalBytes: Long?, format: OutputFormat = DEFAULT_FORMAT) = OneTimeWorkRequestBuilder() .addTag(JobTags.inputCount(inputs.size)) .setInputData( Data.Builder() .putStringArray(KEY_INPUT_URIS, inputs.map(Uri::toString).toTypedArray()) - .putLong(KEY_TOTAL_BYTES, totalBytes) + // Omitted rather than zeroed when a total could not be worked out; a + // `Data` has no null, so the missing key is the unknown. + .apply { totalBytes?.let { putLong(KEY_TOTAL_BYTES, it) } } .putString(KEY_FORMAT, format.name) .build(), ) diff --git a/app/src/main/java/org/libremediaconverter/work/ConversionWorker.kt b/app/src/main/java/org/libremediaconverter/work/ConversionWorker.kt index 3b03a95..5d6994d 100644 --- a/app/src/main/java/org/libremediaconverter/work/ConversionWorker.kt +++ b/app/src/main/java/org/libremediaconverter/work/ConversionWorker.kt @@ -9,10 +9,13 @@ import androidx.work.Data import androidx.work.ForegroundInfo import androidx.work.OneTimeWorkRequestBuilder import androidx.work.WorkerParameters +import androidx.work.hasKeyWithValueOfType import androidx.work.workDataOf import com.arthenica.ffmpegkit.FFmpegKitConfig import kotlinx.coroutines.CancellationException import org.libremediaconverter.convert.ConversionDependencies +import org.libremediaconverter.convert.InputQuery +import org.libremediaconverter.convert.OutputPublisher import org.libremediaconverter.convert.StagingNames import org.libremediaconverter.model.AudioCodec import org.libremediaconverter.model.Container @@ -57,7 +60,11 @@ class ConversionWorker(context: Context, params: WorkerParameters) : CoroutineWo val inputUri = inputData.getString(KEY_INPUT_URI)?.let(Uri::parse) ?: return Result.failure(workDataOf(KEY_ERROR to "No input file.")) val displayName = inputData.getString(KEY_DISPLAY_NAME) ?: "input" - val sizeBytes = inputData.getLong(KEY_SIZE_BYTES, 0L) + // Absent, not zero, when nobody could say -- see InputQuery. `getLong(key, 0L)` is what + // made those two the same number, and `hasSpaceFor(0)` is only "is there 128 MB free". + val declaredSize = inputData + .takeIf { it.hasKeyWithValueOfType(KEY_SIZE_BYTES) } + ?.getLong(KEY_SIZE_BYTES, 0L) val spec = readSpec() val quality = QualityTier.valueOf( inputData.getString(KEY_QUALITY) ?: QualityTier.FAST.name, @@ -66,7 +73,7 @@ class ConversionWorker(context: Context, params: WorkerParameters) : CoroutineWo inputData.getString(KEY_ENGINE_PREFERENCE) ?: EnginePreference.AUTO.name, ) - if (!publisher.hasSpaceFor(sizeBytes)) { + if (!hasRoomFor(declaredSize, inputUri)) { return Result.failure(workDataOf(KEY_ERROR to "Not enough free space to convert.")) } @@ -151,6 +158,29 @@ class ConversionWorker(context: Context, params: WorkerParameters) : CoroutineWo } } + /** + * Whether staging can take this job, asking the input itself when nothing else has. + * + * [declared] is what the picker found, carried in this job's `Data`. It is absent for work + * enqueued before the size became optional, for a [request] built by hand, and for a file + * whose provider would not answer — so the fallback opens the input and asks the descriptor, + * which is one syscall on a file the conversion is about to open anyway. It runs only when + * [declared] is null, so an ordinary job pays nothing for it. + * + * When even that cannot answer, the *question* changes rather than a number being invented: + * [OutputPublisher.hasSpaceForUnknownSize] is the documented "all that is left to check is + * the headroom", and it is not a refusal. Failing every job whose provider is quiet would be + * a worse defect than the vacuous check it replaces. + */ + private fun hasRoomFor(declared: Long?, inputUri: Uri): Boolean { + val bytes = declared ?: InputQuery.sizeOf(applicationContext, inputUri) + if (bytes == null) { + Log.i(TAG, "Nothing could size $inputUri; checking headroom only.") + return publisher.hasSpaceForUnknownSize() + } + return publisher.hasSpaceFor(bytes) + } + /** * The dynamic half of the routing rules. * @@ -318,18 +348,22 @@ class ConversionWorker(context: Context, params: WorkerParameters) : CoroutineWo fun request( inputUri: Uri, displayName: String, - sizeBytes: Long, + sizeBytes: Long?, spec: OutputSpec = OutputFormat.MP4_H265.spec, quality: QualityTier = QualityTier.FAST, enginePreference: EnginePreference = EnginePreference.AUTO, ) = OneTimeWorkRequestBuilder() .addTag(JobTags.displayName(displayName)) - .addTag(JobTags.sizeBytes(sizeBytes)) + // Neither the tag nor the Data entry is written for a size nobody knows. A `Data` has + // no null, so the absence of the key *is* the unknown — and a tag reading + // `size-bytes:0` would come back through Reattachment as a confident claim that the + // user's file is empty. + .apply { sizeBytes?.let { addTag(JobTags.sizeBytes(it)) } } .setInputData( Data.Builder() .putString(KEY_INPUT_URI, inputUri.toString()) .putString(KEY_DISPLAY_NAME, displayName) - .putLong(KEY_SIZE_BYTES, sizeBytes) + .apply { sizeBytes?.let { putLong(KEY_SIZE_BYTES, it) } } .putString(KEY_CONTAINER, spec.container.name) .putString(KEY_VIDEO_CODEC, spec.videoCodec.name) .putString(KEY_AUDIO_CODEC, spec.audioCodec.name) diff --git a/app/src/test/java/org/libremediaconverter/convert/ConversionViewModelProbeFailureTest.kt b/app/src/test/java/org/libremediaconverter/convert/ConversionViewModelProbeFailureTest.kt index df59aae..dfb5079 100644 --- a/app/src/test/java/org/libremediaconverter/convert/ConversionViewModelProbeFailureTest.kt +++ b/app/src/test/java/org/libremediaconverter/convert/ConversionViewModelProbeFailureTest.kt @@ -100,7 +100,10 @@ class ConversionViewModelProbeFailureTest { // on that thread's handler rather than at this call. What must not happen is the card // filling in with an "unreadable" verdict the app would then act on. val settled = settle(viewModel) - assertEquals(ConversionState.Ready(InputFile(INPUT, "input", 0L)), settled) + // `sizeBytes = null`, not `0L`: no provider is registered for this authority, so the + // metadata query returns nothing and the descriptor cannot be opened either. That is the + // unknown, and it stopped being spelled the same way as "empty" -- see [InputQuery]. + assertEquals(ConversionState.Ready(InputFile(INPUT, "input", sizeBytes = null)), settled) assertNull("an OOM must not be reported as a probe result", (settled as ConversionState.Ready).input.probe) } diff --git a/app/src/test/java/org/libremediaconverter/convert/StagingCleanupSupport.kt b/app/src/test/java/org/libremediaconverter/convert/StagingCleanupSupport.kt index 50a33c6..3ea2462 100644 --- a/app/src/test/java/org/libremediaconverter/convert/StagingCleanupSupport.kt +++ b/app/src/test/java/org/libremediaconverter/convert/StagingCleanupSupport.kt @@ -59,28 +59,46 @@ open class RecordingPublisher(context: Context) : OutputPublisher(context) { * `SUCCEEDED` `WorkInfo` carrying an output path, and that is exactly what this produces — * through a real `WorkManager`, so the ViewModel's own observer, its `SUCCEEDED` branch and * its cleanup handle are all the production ones. + * + * It also keeps every [Data] it was handed, which is the only way back to what a ViewModel + * actually enqueued: `WorkInfo` returns a job's tags and its output and never the input `Data` + * it was built with, so a test that wants to know what `convert()` or `join()` put in a request + * has to catch it here, on its way to the worker. */ class SucceedingWorkerFactory(private val outputData: Data) : WorkerFactory() { + + /** The input `Data` of each request that has reached a worker, in order. */ + val enqueued = mutableListOf() + override fun createWorker( appContext: Context, workerClassName: String, workerParameters: WorkerParameters, - ): ListenableWorker = object : Worker(appContext, workerParameters) { - override fun doWork(): Result = Result.success(outputData) + ): ListenableWorker { + enqueued += workerParameters.inputData + return object : Worker(appContext, workerParameters) { + override fun doWork(): Result = Result.success(outputData) + } } } -/** Installs a synchronous test WorkManager whose workers succeed with [outputData]. */ -fun installTestWorkManager(context: Context, outputData: Data) { +/** + * Installs a synchronous test WorkManager whose workers succeed with [outputData]. + * + * @return the factory, so a caller that cares can read back what was enqueued. + */ +fun installTestWorkManager(context: Context, outputData: Data): SucceedingWorkerFactory { + val factory = SucceedingWorkerFactory(outputData) WorkManagerTestInitHelper.initializeTestWorkManager( context, Configuration.Builder() .setMinimumLoggingLevel(Log.ASSERT) .setExecutor(SynchronousExecutor()) .setTaskExecutor(SynchronousExecutor()) - .setWorkerFactory(SucceedingWorkerFactory(outputData)) + .setWorkerFactory(factory) .build(), ) + return factory } /** diff --git a/app/src/test/java/org/libremediaconverter/convert/UnknownInputSizeTest.kt b/app/src/test/java/org/libremediaconverter/convert/UnknownInputSizeTest.kt new file mode 100644 index 0000000..99e39c3 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/convert/UnknownInputSizeTest.kt @@ -0,0 +1,165 @@ +package org.libremediaconverter.convert + +import android.app.Application +import android.net.Uri +import androidx.media3.common.util.UnstableApi +import androidx.work.Data +import androidx.work.hasKeyWithValueOfType +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertNull +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.libremediaconverter.join.JoinState +import org.libremediaconverter.join.JoinViewModel +import org.libremediaconverter.model.InputProbe +import org.libremediaconverter.work.ConcatWorker +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment +import java.io.File + +/** + * That a size nobody reported is not the same thing as a size of zero. + * + * `queryFile` started at `var size = 0L` and only moved off it when a provider answered the + * `OpenableColumns.SIZE` column, so "the file is empty" and "nobody told me" arrived at the space + * check as the same number — and `hasSpaceFor(0)` is only "is there 128 MB free". + * + * The gap is not hypothetical. On a Pixel 10 Pro XL, `contentResolver.query` on a `file://` URI + * returns null outright, so the cursor block never runs and the default survives: + * `queryFile gave displayName='input' sizeBytes=0`. Robolectric reproduces that exactly, which is + * what makes the first test below a JVM test rather than a device one. + */ +@UnstableApi +@RunWith(RobolectricTestRunner::class) +class UnknownInputSizeTest { + + private lateinit var app: Application + private lateinit var workers: SucceedingWorkerFactory + + @Before + fun setUp() { + app = RuntimeEnvironment.getApplication() + // FFprobe's loader throws a bare java.lang.Error on the JVM, and nothing here is about + // what the probe found. + ConversionDependencies.probe = { _, _ -> InputProbe() } + // Both ViewModels reach WorkManager.getInstance() while constructing. + workers = installTestWorkManager(app, Data.EMPTY) + } + + @After + fun tearDown() { + ConversionDependencies.reset() + } + + @Test + fun `a picked file no provider describes is measured rather than reported as empty`() { + val input = fileOfSize(INPUT_BYTES, "holiday.mp4") + + val ready = pickedInto(Uri.fromFile(input)) + + // The resolver answers nothing at all for a file:// URI -- the exact device case -- so the + // only way to this number is opening the file and asking the descriptor. + assertEquals(INPUT_BYTES.toLong(), ready.input.sizeBytes) + } + + @Test + fun `a picked file nothing can measure has no size rather than a size of zero`() { + // No provider is registered for this authority, so the metadata query returns null and + // openFileDescriptor throws FileNotFoundException. Nothing can say how big it is, and + // saying "zero" would be a claim rather than an answer. + val ready = pickedInto(Uri.parse("content://test/holiday.mp4")) + + assertNull(ready.input.sizeBytes) + } + + @Test + fun `the same measurement is what the join picker gets`() { + // Not a copy of the convert test for its own sake: `queryFile` existed twice, once in each + // ViewModel, byte for byte. One of the two being fixed is the shape this would come back in. + val first = fileOfSize(FIRST_JOIN_BYTES, "one.mp4") + val second = fileOfSize(SECOND_JOIN_BYTES, "two.mp4") + val viewModel = JoinViewModel(app) + + viewModel.onInputsPicked(listOf(Uri.fromFile(first), Uri.fromFile(second))) + val ready = awaitState(viewModel.state, "Ready") { it is JoinState.Ready } as JoinState.Ready + + assertEquals( + listOf(FIRST_JOIN_BYTES.toLong(), SECOND_JOIN_BYTES.toLong()), + ready.inputs.map { it.sizeBytes }, + ) + } + + @Test + fun `a join enqueues the total it worked out, and no total at all when it could not`() { + // The wiring, which the two tests around it do not reach: `InputQuery.total` being right + // says nothing about `join()` calling it, and `ConcatWorker`'s unknown branch is reached + // by work built in that test rather than by this ViewModel. Restoring + // `inputs.sumOf { it.sizeBytes ?: 0L }` leaves both of those green. + // + // Read off the request on its way to a worker, because that is the only place it is + // legible: `WorkInfo` hands back a job's tags and its output, never the input `Data`. + val first = fileOfSize(FIRST_JOIN_BYTES, "one.mp4") + val second = fileOfSize(SECOND_JOIN_BYTES, "two.mp4") + + joined(Uri.fromFile(first), Uri.fromFile(second)) + val known = workers.enqueued.single() + assertEquals( + (FIRST_JOIN_BYTES + SECOND_JOIN_BYTES).toLong(), + known.getLong(ConcatWorker.KEY_TOTAL_BYTES, MISSING), + ) + + // And with one input nothing can size, the key is absent rather than carrying a short + // total -- a `Data` has no null, so absence is the only way to say "unknown" in one. + workers = installTestWorkManager(app, Data.EMPTY) + joined(Uri.fromFile(first), Uri.parse("content://test/two.mp4")) + val unknown = workers.enqueued.single() + assertFalse( + "a total that could not be worked out must not be enqueued as a number", + unknown.hasKeyWithValueOfType(ConcatWorker.KEY_TOTAL_BYTES), + ) + } + + @Test + fun `a total is only as good as its least-known part`() { + // The rule the join side needed that the convert side did not. `sumOf` over a list with an + // unknown in it produces a number, and a number that is short by one whole file is worse + // than no number: the space check cannot tell it from a real total, so it would reserve + // for half the job and pass. + assertEquals(3_333L, InputQuery.total(listOf(1_111L, 2_222L))) + assertNull(InputQuery.total(listOf(1_111L, null))) + assertNull(InputQuery.total(listOf(null, 2_222L))) + // A join of nothing has a known total of nothing. Both workers refuse fewer than two + // inputs long before this, so it is a statement about the fold rather than a real case. + assertEquals(0L, InputQuery.total(emptyList())) + } + + /** Drives a real [JoinViewModel] from a pick to an enqueued join. */ + private fun joined(vararg uris: Uri) { + val viewModel = JoinViewModel(app) + viewModel.onInputsPicked(uris.toList()) + awaitState(viewModel.state, "Ready") { it is JoinState.Ready } + viewModel.join() + awaitState(viewModel.state, "past Joining") { it !is JoinState.Ready } + } + + private fun pickedInto(uri: Uri): ConversionState.Ready { + val viewModel = ConversionViewModel(app) + viewModel.onInputPicked(uri) + return awaitState(viewModel.state, "Ready") { it is ConversionState.Ready } as ConversionState.Ready + } + + private fun fileOfSize(bytes: Int, name: String): File = + File(app.cacheDir, name).apply { writeBytes(ByteArray(bytes)) } + + private companion object { + const val INPUT_BYTES = 4_321 + const val FIRST_JOIN_BYTES = 1_111 + const val SECOND_JOIN_BYTES = 2_222 + + /** A `getLong` default no real total could be mistaken for. */ + const val MISSING = -1L + } +} diff --git a/app/src/test/java/org/libremediaconverter/work/SpaceCheckTest.kt b/app/src/test/java/org/libremediaconverter/work/SpaceCheckTest.kt new file mode 100644 index 0000000..cf35cdd --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/work/SpaceCheckTest.kt @@ -0,0 +1,251 @@ +package org.libremediaconverter.work + +import android.app.Application +import android.content.Context +import android.net.Uri +import androidx.media3.common.util.UnstableApi +import androidx.work.Data +import androidx.work.ListenableWorker +import androidx.work.testing.TestListenableWorkerBuilder +import androidx.work.workDataOf +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.ConversionDependencies +import org.libremediaconverter.convert.OutputPublisher +import org.libremediaconverter.convert.installTestWorkManager +import org.libremediaconverter.model.DeviceCodecs +import org.libremediaconverter.model.EnginePreference +import org.libremediaconverter.model.InputProbe +import org.libremediaconverter.model.OutputFormat +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment +import java.io.File +import java.util.UUID + +/** + * What the space check is actually asked, which is where D5 lived. + * + * The worker read its input size out of `Data` with `getLong(KEY_SIZE_BYTES, 0L)`, so a job whose + * size nobody could report asked "is there room for 0 bytes?" — which the headroom answers yes to + * on any device with 128 MB free, whatever the file turns out to weigh. + * + * These assert on the *question*, not on the verdict. A test that only checked whether the job ran + * would pass against the defect: the defect is that the guard is vacuous, not that it refuses. + */ +@UnstableApi +@RunWith(RobolectricTestRunner::class) +class SpaceCheckTest { + + private lateinit var app: Application + private lateinit var publisher: RecordingSpacePublisher + + @Before + fun setUp() { + app = RuntimeEnvironment.getApplication() + publisher = RecordingSpacePublisher(app) + ConversionDependencies.publisher = { publisher } + ConversionDependencies.probe = { _, _ -> InputProbe() } + ConversionDependencies.deviceCodecs = { DeviceCodecs.PERMISSIVE } + // The progress notification builds its cancel action from WorkManager.getInstance(). + installTestWorkManager(app, Data.EMPTY) + } + + @After + fun tearDown() { + ConversionDependencies.reset() + } + + @Test + fun `a job with no declared size measures its input rather than asking for room for nothing`() { + val input = fileOfSize(INPUT_BYTES, "holiday.mp4") + + runBlocking { conversionWorker(Uri.fromFile(input), declaredSize = null).doWork() } + + assertEquals(listOf(INPUT_BYTES.toLong()), publisher.requested) + } + + @Test + fun `a declared size is trusted rather than re-measured`() { + // The ordinary path, pinned so the measurement stays a fallback. Opening a descriptor per + // job is cheap, but doing it when the picker already answered would be work for nothing -- + // and the declared number is the one the user was shown. + val input = fileOfSize(INPUT_BYTES, "holiday.mp4") + + runBlocking { conversionWorker(Uri.fromFile(input), declaredSize = DECLARED_BYTES).doWork() } + + assertEquals(listOf(DECLARED_BYTES), publisher.requested) + } + + @Test + fun `a join with no declared total measures its inputs rather than asking for room for nothing`() { + val first = fileOfSize(FIRST_JOIN_BYTES, "one.mp4") + val second = fileOfSize(SECOND_JOIN_BYTES, "two.mp4") + + runBlocking { concatWorker(listOf(Uri.fromFile(first), Uri.fromFile(second))).doWork() } + + assertEquals(listOf(FIRST_JOIN_BYTES.toLong() + SECOND_JOIN_BYTES), publisher.requested) + } + + @Test + fun `a job whose input nothing can size asks the unknown-size question instead of claiming zero`() { + // A file:// URI at a path that does not exist: the resolver answers no metadata, and + // opening a descriptor throws. Nothing left can say how big it is. + runBlocking { conversionWorker(MISSING_INPUT, declaredSize = null).doWork() } + + // The point of the whole change, in one line. `[0L]` -- what this recorded before -- is a + // claim that the file is empty; the unknown question is the absence of a claim. + assertEquals(emptyList(), publisher.requested) + assertEquals(1, publisher.unknownQuestions) + } + + @Test + fun `a join whose inputs are not all measurable has no total, rather than the ones that answered`() { + val known = fileOfSize(FIRST_JOIN_BYTES, "one.mp4") + + runBlocking { concatWorker(listOf(Uri.fromFile(known), MISSING_INPUT)).doWork() } + + // Emphatically not `[1111]`. A lower bound is indistinguishable from a total once it + // reaches the space check, and the check would then be reserving for half the job. + assertEquals(emptyList(), publisher.requested) + assertEquals(1, publisher.unknownQuestions) + } + + @Test + fun `a size nothing can determine is not by itself a reason to refuse the conversion`() { + // The decision this defect had to make, pinned so it cannot be quietly reversed. Refusing + // an unmeasurable input would turn "no provider answered the SIZE column" into "this file + // cannot be converted", which is a worse defect than the vacuous guard it replaces -- and + // one the user could do nothing at all about. + ConversionDependencies.publisher = { AlwaysRoomPublisher(app) } + ConversionDependencies.software = { WritingTranscoder } + + val result = runBlocking { + conversionWorker(MISSING_INPUT, declaredSize = null, engine = EnginePreference.FORCE_SOFTWARE).doWork() + } + + assertTrue("an unknown size must not end the job; got $result", result is ListenableWorker.Result.Success) + } + + @Test + fun `a full disk still refuses a job whose size is unknown`() { + // The other half of that decision, and what keeps `FakeFailures.FullDisk` -- which + // overrides `hasSpaceFor` and nothing else -- still meaning what it says. An independent + // implementation of the unknown-size question could stop honouring a full disk without a + // single caller changing. + ConversionDependencies.publisher = { NoRoomPublisher(app) } + + val result = runBlocking { conversionWorker(MISSING_INPUT, declaredSize = null).doWork() } + + assertEquals( + ListenableWorker.Result.failure( + workDataOf(ConversionWorker.KEY_ERROR to "Not enough free space to convert."), + ), + result, + ) + } + + /** + * A worker whose input `Data` carries a size only when [declaredSize] is given. + * + * Built entry by entry rather than through `ConversionWorker.request`, because "the key is + * simply not there" is the shape being tested and `request` is one of the two things that + * produces it. + */ + private fun conversionWorker( + input: Uri, + declaredSize: Long?, + engine: EnginePreference = EnginePreference.AUTO, + ): ConversionWorker { + val data = mutableMapOf( + ConversionWorker.KEY_INPUT_URI to input.toString(), + ConversionWorker.KEY_DISPLAY_NAME to "holiday.mp4", + ConversionWorker.KEY_CONTAINER to SPEC.container.name, + ConversionWorker.KEY_VIDEO_CODEC to SPEC.videoCodec.name, + ConversionWorker.KEY_AUDIO_CODEC to SPEC.audioCodec.name, + ConversionWorker.KEY_ENGINE_PREFERENCE to engine.name, + ) + declaredSize?.let { data[ConversionWorker.KEY_SIZE_BYTES] = it } + return TestListenableWorkerBuilder( + context = app, + inputData = workDataOf(*data.map { it.key to it.value }.toTypedArray()), + runAttemptCount = 0, + ).setId(CONVERSION_ID).build() + } + + private fun concatWorker(inputs: List): ConcatWorker = TestListenableWorkerBuilder( + context = app, + inputData = workDataOf( + ConcatWorker.KEY_INPUT_URIS to inputs.map(Uri::toString).toTypedArray(), + ConcatWorker.KEY_FORMAT to OutputFormat.MP4_H264.name, + ), + runAttemptCount = 0, + ).setId(CONCAT_ID).build() + + private fun fileOfSize(bytes: Int, name: String): File = + File(app.cacheDir, name).apply { writeBytes(ByteArray(bytes)) } + + private companion object { + const val INPUT_BYTES = 4_321 + const val DECLARED_BYTES = 9_999L + const val FIRST_JOIN_BYTES = 1_111 + const val SECOND_JOIN_BYTES = 2_222 + + /** + * An input nothing can size. + * + * A `file://` path that does not exist, which under Robolectric behaves exactly as the + * device pass recorded for a real one: `contentResolver.query` returns null, so no SIZE + * column is ever reached, and `openFileDescriptor` throws `FileNotFoundException`. It is + * also a scheme the worker handles without the FFmpegKit SAF bridge, which is native and + * therefore unavailable here. + */ + val MISSING_INPUT: Uri = Uri.parse("file:///nonexistent/holiday.mp4") + val SPEC = OutputFormat.MP4_H265.spec + val CONVERSION_ID: UUID = UUID.fromString("00000000-0000-4000-8000-000000000011") + val CONCAT_ID: UUID = UUID.fromString("00000000-0000-4000-8000-000000000012") + } +} + +/** + * Records *which* space question was asked, and refuses either way. + * + * Both overrides, and neither calls `super`. That is what makes the two questions tell apart at + * all: the production default answers `hasSpaceForUnknownSize()` by delegating to + * `hasSpaceFor(0L)`, so a recorder that delegated would log an unknown size as a request for zero + * bytes — the exact conflation being tested. The delegation itself is pinned separately, by + * [NoRoomPublisher] and the full-disk test. + * + * Refusing keeps the worker to the one line under test: the check runs before anything is staged + * or any engine is reached, so `false` ends `doWork` immediately and no native library is asked to + * load. + */ +private class RecordingSpacePublisher(context: Context) : OutputPublisher(context) { + val requested = mutableListOf() + var unknownQuestions = 0 + private set + + override fun hasSpaceFor(bytes: Long): Boolean { + requested += bytes + return false + } + + override fun hasSpaceForUnknownSize(): Boolean { + unknownQuestions++ + return false + } +} + +/** + * A full disk expressed the only way `FakeFailures.FullDisk` expresses it. + * + * `hasSpaceFor` and nothing else, so a job refused here is a job refused *through* the + * delegation rather than by an override of its own. + */ +private class NoRoomPublisher(context: Context) : OutputPublisher(context) { + override fun hasSpaceFor(bytes: Long): Boolean = false +}