From 0dd1a9ac82200ab191314585cb4cc936045c45d9 Mon Sep 17 00:00:00 2001 From: zarzet <42882290+zarzet@users.noreply.github.com> Date: Thu, 17 Sep 2026 02:59:29 +0700 Subject: [PATCH] perf(audio): validate FLAC remuxes before encoder fallback --- .../zarz/spotiflac/NativeDownloadFinalizer.kt | 14 +- .../zarz/spotiflac/NativeFlacConversion.kt | 64 +++++++ .../spotiflac/NativeFlacConversionTest.kt | 165 ++++++++++++++++++ .../download_queue_provider_single_item.dart | 6 +- lib/services/ffmpeg_service.dart | 60 ++++++- test/ffmpeg_conversion_output_test.dart | 96 +++++++++- 6 files changed, 394 insertions(+), 11 deletions(-) create mode 100644 android/app/src/main/kotlin/com/zarz/spotiflac/NativeFlacConversion.kt create mode 100644 android/app/src/test/kotlin/com/zarz/spotiflac/NativeFlacConversionTest.kt diff --git a/android/app/src/main/kotlin/com/zarz/spotiflac/NativeDownloadFinalizer.kt b/android/app/src/main/kotlin/com/zarz/spotiflac/NativeDownloadFinalizer.kt index b70c8ee0..c2c538e2 100644 --- a/android/app/src/main/kotlin/com/zarz/spotiflac/NativeDownloadFinalizer.kt +++ b/android/app/src/main/kotlin/com/zarz/spotiflac/NativeDownloadFinalizer.kt @@ -778,16 +778,18 @@ object NativeDownloadFinalizer { adoptedOutput = true return } - val result = runFFmpeg( - "-v error -xerror -i ${q(localInput)} -c:a flac -compression_level 8 ${q(stagedOutput)} -y", - shouldCancel, + convertToStagedFlac( + input = localInput, + stagedOutput = stagedOutput, + codec = codec, + execute = { arguments -> runFFmpegArguments(arguments, shouldCancel) }, + checkCancelled = { checkCancelled(shouldCancel) }, ) - if (!result.first || !File(stagedOutput).exists()) { - throw IllegalStateException("container conversion failed: ${result.second}") - } if (!promoteStagedConversion(stagedOutput, output)) { throw IllegalStateException("failed to publish container conversion output") } + // Keep metadata failures before adoption so the source survives + // and the unsuccessful output is removed by the local cleanup. embedBasicMetadata(context, output, input, "flac") replaceStatePath(context, input, state, output, deleteOld = true) adoptedOutput = true diff --git a/android/app/src/main/kotlin/com/zarz/spotiflac/NativeFlacConversion.kt b/android/app/src/main/kotlin/com/zarz/spotiflac/NativeFlacConversion.kt new file mode 100644 index 00000000..1ea41fbf --- /dev/null +++ b/android/app/src/main/kotlin/com/zarz/spotiflac/NativeFlacConversion.kt @@ -0,0 +1,64 @@ +package com.zarz.spotiflac + +import java.io.File + +/** Creates a validated staged FLAC; publication and source ownership stay with the caller. */ +internal fun convertToStagedFlac( + input: String, + stagedOutput: String, + codec: String, + execute: (Array) -> Pair, + checkCancelled: () -> Unit, +) { + val staged = File(stagedOutput) + var complete = false + var diagnostic = "" + val encoders = if (NativeFinalizationPolicy.normalizeAudioCodec(codec) == "flac") { + listOf(arrayOf("-c:a", "copy"), arrayOf("-c:a", "flac", "-compression_level", "8")) + } else { + listOf(arrayOf("-c:a", "flac", "-compression_level", "8")) + } + try { + for (encoder in encoders) { + checkCancelled() + check(!staged.exists() || staged.delete()) { "failed to remove staged FLAC output" } + val result = execute( + arrayOf("-v", "error", "-xerror", "-i", input) + + encoder + arrayOf("-f", "flac", stagedOutput, "-y"), + ) + checkCancelled() + diagnostic = result.second + val hasHeader = result.first && staged.isFile && staged.length() > 42L && + staged.inputStream().use { stream -> + val header = ByteArray(4) + stream.read(header) == 4 && header.contentEquals(byteArrayOf(0x66, 0x4c, 0x61, 0x43)) + } + if (!hasHeader) { + if (result.first) diagnostic = "conversion produced no valid FLAC header" + continue + } + if ("copy" !in encoder) { + complete = true + return + } + // A remux does not decode audio, so verify the whole stream to + // reject corrupt later frames just as the encoder did previously. + // Cancellation must never trigger an encoding retry. + val validation = execute( + arrayOf( + "-v", "error", "-xerror", "-err_detect", "crccheck+explode", "-i", stagedOutput, + "-map", "0:a:0", "-f", "null", "-", + ), + ) + checkCancelled() + diagnostic = validation.second + if (validation.first) { + complete = true + return + } + } + throw IllegalStateException("container conversion failed: $diagnostic") + } finally { + if (!complete) staged.delete() + } +} diff --git a/android/app/src/test/kotlin/com/zarz/spotiflac/NativeFlacConversionTest.kt b/android/app/src/test/kotlin/com/zarz/spotiflac/NativeFlacConversionTest.kt new file mode 100644 index 00000000..f2e4e6cb --- /dev/null +++ b/android/app/src/test/kotlin/com/zarz/spotiflac/NativeFlacConversionTest.kt @@ -0,0 +1,165 @@ +package com.zarz.spotiflac + +import java.io.File +import java.nio.file.Files +import java.util.concurrent.CancellationException +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertThrows +import org.junit.Assert.assertTrue +import org.junit.Assume.assumeTrue +import org.junit.Test + +class NativeFlacConversionTest { + private fun withFiles(block: (File, File) -> Unit) { + val directory = Files.createTempDirectory("native-flac-conversion-").toFile() + try { + val input = File(directory, "input.m4a").apply { writeText("original audio") } + block(input, File(directory, "output.partial.flac")) + assertEquals("original audio", input.readText()) + } finally { + directory.deleteRecursively() + } + } + + private fun writeFlacHeader(file: File) { + file.writeBytes(byteArrayOf(0x66, 0x4c, 0x61, 0x43) + ByteArray(64)) + } + + @Test + fun validRemuxSkipsEncoderAndLeavesSourceUntouched() = withFiles { input, output -> + val commands = mutableListOf>() + convertToStagedFlac(input.path, output.path, "flac", { arguments -> + commands.add(arguments.toList()) + if ("copy" in arguments) writeFlacHeader(output) + true to "" + }, {}) + assertEquals(2, commands.size) + assertTrue("copy" in commands.first()) + assertFalse("-map" in commands.first()) + assertTrue("null" in commands.last()) + assertFalse("-frames:a" in commands.last()) + assertFalse(commands.any { "-compression_level" in it }) + assertTrue(output.exists()) + } + + @Test + fun failedRemuxOrValidationRetriesEncoderWithCleanStage() { + for (failure in listOf("command", "header", "decode")) { + withFiles { input, output -> + var encoded = false + convertToStagedFlac(input.path, output.path, "flac", { arguments -> + when { + "copy" in arguments -> { + if (failure == "header") output.writeText("invalid FLAC") else writeFlacHeader(output) + (failure != "command") to "remux diagnostic" + } + "-compression_level" in arguments -> { + assertFalse(output.exists()) + encoded = true + writeFlacHeader(output) + true to "" + } + else -> (encoded || failure != "decode") to "decode diagnostic" + } + }, {}) + assertTrue(encoded) + assertTrue(output.exists()) + } + } + } + + @Test + fun nonFlacSkipsRemuxAndFailedEncodeRemovesPartialOutput() = withFiles { input, output -> + var calls = 0 + assertThrows(IllegalStateException::class.java) { + convertToStagedFlac(input.path, output.path, "alac", { arguments -> + calls++ + assertFalse("copy" in arguments) + output.writeText("partial") + false to "encoder failed" + }, {}) + } + assertEquals(1, calls) + assertFalse(output.exists()) + } + + @Test + fun cancellationAfterRemuxDoesNotFallbackAndRemovesPartialOutput() = withFiles { input, output -> + var cancelled = false + var calls = 0 + assertThrows(CancellationException::class.java) { + convertToStagedFlac(input.path, output.path, "flac", { + calls++ + writeFlacHeader(output) + cancelled = true + false to "cancelled" + }, { + if (cancelled) throw CancellationException() + }) + } + assertEquals(1, calls) + assertFalse(output.exists()) + } + + @Test + fun hostFlacInMp4RemuxAndFallbackPreserveDecodedPcmAndArtwork() { + val ffmpeg = System.getenv("SPOTIFLAC_TEST_FFMPEG").orEmpty() + assumeTrue("Set SPOTIFLAC_TEST_FFMPEG for host media fixtures", File(ffmpeg).canExecute()) + fun execute(arguments: Array): Pair { + val process = ProcessBuilder(listOf(ffmpeg) + arguments).redirectErrorStream(true).start() + val output = process.inputStream.bufferedReader().use { it.readText() } + return (process.waitFor() == 0) to output + } + fun run(vararg arguments: String): String { + val result = execute(arrayOf(*arguments)) + assertTrue(result.second, result.first) + return result.second.trim() + } + val directory = Files.createTempDirectory("native-flac-fixture-").toFile() + try { + val cover = File(directory, "cover.png") + run("-v", "error", "-f", "lavfi", "-i", "color=c=red:s=32x32", "-frames:v", "1", cover.path) + val coverHash = run("-v", "error", "-i", cover.path, "-map", "0:v:0", "-f", "hash", "-hash", "sha256", "-") + for ((sampleRate, sampleFormat) in listOf(44100 to "s16", 96000 to "s32")) { + val source = File(directory, "source-$sampleRate.flac") + val input = File(directory, "input-$sampleRate.m4a") + run( + "-v", "error", "-f", "lavfi", "-i", "aevalsrc=0.1*sin(2*PI*997*t):s=$sampleRate", + "-t", "0.2", "-c:a", "flac", "-sample_fmt", sampleFormat, source.path, + ) + run( + "-v", "error", "-i", source.path, "-i", cover.path, "-map", "0:a:0", "-map", "1:v:0", + "-c", "copy", "-disposition:v", "attached_pic", "-strict", "-2", "-f", "mp4", input.path, + ) + val sourceHash = run("-v", "error", "-i", source.path, "-map", "0:a:0", "-c:a", "pcm_s32le", "-f", "hash", "-hash", "sha256", "-") + for (mode in listOf("remux", "failed-remux", "corrupt-remux")) { + val output = File(directory, "output-$sampleRate-$mode.partial.flac") + var encoded = false + convertToStagedFlac(input.path, output.path, "flac", { arguments -> + if ("-compression_level" in arguments) encoded = true + if (mode == "failed-remux" && "copy" in arguments) { + false to "forced remux failure" + } else { + execute(arguments).also { result -> + if (mode == "corrupt-remux" && "copy" in arguments && result.first) { + val bytes = output.readBytes() + for (index in bytes.size - 50 until bytes.size - 34) { + bytes[index] = (bytes[index].toInt() xor 0x55).toByte() + } + output.writeBytes(bytes) + } + } + } + }, {}) + assertEquals(mode != "remux", encoded) + assertEquals(sourceHash, run("-v", "error", "-i", output.path, "-map", "0:a:0", "-c:a", "pcm_s32le", "-f", "hash", "-hash", "sha256", "-")) + assertEquals(coverHash, run("-v", "error", "-i", output.path, "-map", "0:v:0", "-f", "hash", "-hash", "sha256", "-")) + assertTrue(input.exists()) + } + } + } finally { + directory.deleteRecursively() + } + } +} diff --git a/lib/providers/download_queue_provider_single_item.dart b/lib/providers/download_queue_provider_single_item.dart index 8c214890..e64d28e2 100644 --- a/lib/providers/download_queue_provider_single_item.dart +++ b/lib/providers/download_queue_provider_single_item.dart @@ -1236,7 +1236,10 @@ class _DownloadRun { DownloadStatus.finalizing, progress: 0.95, ); - final flacPath = await FFmpegService.convertM4aToFlac(tempPath); + final flacPath = await FFmpegService.convertM4aToFlac( + tempPath, + sourceCodec: codec, + ); if (flacPath == null) { _log.w('FFmpeg conversion returned null, keeping M4A file'); branch = 'convertFailed'; @@ -1364,6 +1367,7 @@ class _DownloadRun { ); final flacPath = await FFmpegService.convertM4aToFlac( currentFilePath, + sourceCodec: codec, ); if (flacPath != null) { diff --git a/lib/services/ffmpeg_service.dart b/lib/services/ffmpeg_service.dart index 0a553caa..482ba6ba 100644 --- a/lib/services/ffmpeg_service.dart +++ b/lib/services/ffmpeg_service.dart @@ -664,6 +664,7 @@ class FFmpegService { static Future convertM4aToFlac( String inputPath, { + String? sourceCodec, @visibleForTesting Future Function(List)? execute, }) async { final plan = await _conversionOutputPlan( @@ -672,7 +673,48 @@ class FFmpegService { deleteOriginal: true, ); try { - final result = await (execute ?? _executeWithArguments)([ + final run = execute ?? _executeWithArguments; + final codec = sourceCodec ?? await probePrimaryAudioCodec(inputPath); + if (codec?.trim().toLowerCase() == 'flac') { + final remux = await run([ + '-v', + 'error', + '-xerror', + '-i', + inputPath, + '-c:a', + 'copy', + '-f', + 'flac', + plan.workingPath, + '-y', + ]); + if (remux.success && await _hasNativeFlacHeader(plan.workingPath)) { + final validation = await run([ + '-v', + 'error', + '-xerror', + '-err_detect', + 'crccheck+explode', + '-i', + plan.workingPath, + '-map', + '0:a:0', + '-f', + 'null', + '-', + ]); + if (validation.success) { + return await _finalizeConversionOutput( + plan: plan, + inputPath: inputPath, + deleteOriginal: true, + ); + } + } + _log.w('FLAC remux validation failed; retrying with the encoder'); + } + final result = await run([ '-v', 'error', '-xerror', @@ -700,6 +742,22 @@ class FFmpegService { return null; } + static Future _hasNativeFlacHeader(String path) async { + final file = File(path); + if (!await file.exists() || await file.length() <= 42) return false; + final input = await file.open(); + try { + final magic = await input.read(4); + return magic.length == 4 && + magic[0] == 0x66 && + magic[1] == 0x4c && + magic[2] == 0x61 && + magic[3] == 0x43; + } finally { + await input.close(); + } + } + /// Corrects a native FLAC payload's suffix without replacing a sibling file. static Future ensureNativeFlacExtension(String inputPath) async { if (inputPath.toLowerCase().endsWith('.flac')) return inputPath; diff --git a/test/ffmpeg_conversion_output_test.dart b/test/ffmpeg_conversion_output_test.dart index d214531f..741ac4de 100644 --- a/test/ffmpeg_conversion_output_test.dart +++ b/test/ffmpeg_conversion_output_test.dart @@ -28,6 +28,7 @@ void main() { ).writeAsString('existing'); final result = await FFmpegService.convertM4aToFlac( source.path, + sourceCodec: 'alac', execute: (arguments) async { expect(arguments[arguments.indexOf('-i') + 1], source.path); expect(await source.exists(), isTrue); @@ -45,6 +46,7 @@ void main() { final source = await input('Song.m4a'); final result = await FFmpegService.convertM4aToFlac( source.path, + sourceCodec: 'alac', execute: (arguments) async { await File(arguments[arguments.length - 2]).writeAsString('partial'); return FFmpegResult(success: false, returnCode: 1, output: 'failure'); @@ -64,7 +66,11 @@ void main() { (_) async => FFmpegResult(success: true, returnCode: 0, output: ''), ]) { expect( - await FFmpegService.convertM4aToFlac(source.path, execute: execute), + await FFmpegService.convertM4aToFlac( + source.path, + sourceCodec: 'alac', + execute: execute, + ), isNull, ); expect(await source.readAsString(), 'source'); @@ -77,6 +83,7 @@ void main() { final source = await input('Song.flac'); final result = await FFmpegService.convertM4aToFlac( source.path, + sourceCodec: 'alac', execute: (arguments) async { expect(arguments[arguments.length - 2], isNot(source.path)); expect(await source.readAsString(), 'source'); @@ -101,14 +108,97 @@ void main() { } final results = await Future.wait([ - FFmpegService.convertM4aToFlac(first.path, execute: execute), - FFmpegService.convertM4aToFlac(second.path, execute: execute), + FFmpegService.convertM4aToFlac( + first.path, + sourceCodec: 'alac', + execute: execute, + ), + FFmpegService.convertM4aToFlac( + second.path, + sourceCodec: 'alac', + execute: execute, + ), ]); expect(results.toSet(), hasLength(2)); expect(results, everyElement(isNotNull)); expect(await directory.list().length, 2); }); + test( + 'FLAC payload is remuxed and decoded before deleting its source', + () async { + final source = await input('Song.m4a'); + final commands = >[]; + final result = await FFmpegService.convertM4aToFlac( + source.path, + sourceCodec: 'flac', + execute: (arguments) async { + commands.add(arguments); + expect(await source.exists(), isTrue); + if (arguments.contains('copy')) { + // Preserve FFmpeg's existing attached-artwork stream selection. + expect(arguments, isNot(contains('-map'))); + await File(arguments[arguments.length - 2]).writeAsBytes([ + 0x66, + 0x4c, + 0x61, + 0x43, + ...List.filled(64, 0), + ]); + } else { + expect(arguments, containsAllInOrder(['-f', 'null', '-'])); + expect( + arguments, + containsAllInOrder(['-err_detect', 'crccheck+explode', '-i']), + ); + expect(arguments, isNot(contains('-frames:a'))); + } + return FFmpegResult(success: true, returnCode: 0, output: ''); + }, + ); + expect(result, isNotNull); + expect(commands, hasLength(2)); + expect(await source.exists(), isFalse); + }, + ); + + for (final failure in ['copy', 'header', 'decode']) { + test( + 'invalid FLAC $failure falls back to encoding the intact source', + () async { + final source = await input('Song.m4a'); + var encoded = false; + final result = await FFmpegService.convertM4aToFlac( + source.path, + sourceCodec: 'flac', + execute: (arguments) async { + expect(await source.readAsString(), 'source'); + if (arguments.contains('copy')) { + await File(arguments[arguments.length - 2]).writeAsBytes([ + if (failure != 'header') ...[0x66, 0x4c, 0x61, 0x43], + ...List.filled(64, 0), + ]); + return FFmpegResult( + success: failure != 'copy', + returnCode: failure == 'copy' ? 1 : 0, + output: '', + ); + } + if (arguments.last == '-') { + return FFmpegResult(success: false, returnCode: 1, output: 'bad'); + } + encoded = true; + expect(arguments, containsAllInOrder(['-c:a', 'flac'])); + return succeed(arguments); + }, + ); + expect(encoded, isTrue); + expect(await File(result!).readAsString(), 'converted'); + expect(await source.exists(), isFalse); + }, + ); + } + test( 'native FLAC rename preserves sibling files and adds missing suffix', () async {