From e61e54f93f1538b08b40c3cdbec2a7e375535ae7 Mon Sep 17 00:00:00 2001 From: zarzet <42882290+zarzet@users.noreply.github.com> Date: Mon, 28 Sep 2026 12:21:15 +0700 Subject: [PATCH] fix(download): align native metadata preparation and ReplayGain support --- .../spotiflac/NativeDownloadContainerTest.kt | 72 +++++++++++ .../zarz/spotiflac/NativeDownloadFinalizer.kt | 10 +- .../spotiflac/NativeFinalizationPolicy.kt | 5 + .../zarz/spotiflac/NativeFinalizerMedia.kt | 4 +- .../spotiflac/NativeFinalizationPolicyTest.kt | 11 ++ lib/providers/download_queue_provider.dart | 1 + .../download_queue_provider_embedding.dart | 7 + ...download_queue_provider_native_worker.dart | 11 +- .../download_queue_provider_single_item.dart | 103 +-------------- lib/services/download_track_metadata.dart | 59 +++++++++ test/download_track_metadata_test.dart | 122 ++++++++++++++++++ 11 files changed, 288 insertions(+), 117 deletions(-) create mode 100644 lib/services/download_track_metadata.dart create mode 100644 test/download_track_metadata_test.dart diff --git a/android/app/src/androidTest/kotlin/com/zarz/spotiflac/NativeDownloadContainerTest.kt b/android/app/src/androidTest/kotlin/com/zarz/spotiflac/NativeDownloadContainerTest.kt index c8332c59..0cb845b7 100644 --- a/android/app/src/androidTest/kotlin/com/zarz/spotiflac/NativeDownloadContainerTest.kt +++ b/android/app/src/androidTest/kotlin/com/zarz/spotiflac/NativeDownloadContainerTest.kt @@ -3,6 +3,8 @@ package com.zarz.spotiflac import androidx.test.ext.junit.runners.AndroidJUnit4 import androidx.test.platform.app.InstrumentationRegistry import java.io.File +import org.junit.After +import org.junit.Before import org.json.JSONObject import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse @@ -12,6 +14,76 @@ import org.junit.runner.RunWith @RunWith(AndroidJUnit4::class) class NativeDownloadContainerTest { + private lateinit var backend: CoreBackend + private lateinit var backendRoot: File + + @Before + fun initializeBackend() { + val context = InstrumentationRegistry.getInstrumentation().targetContext + backendRoot = File(context.filesDir, "finalizer-test-${System.nanoTime()}").apply { mkdirs() } + backend = createCoreBackend(context) + backend.invokeApplication("initExtensionSystem", mapOf( + "extensions_dir" to File(backendRoot, "sources").apply { mkdirs() }.path, + "data_dir" to File(backendRoot, "data").apply { mkdirs() }.path, + "master_key" to "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=", + "allowed_directories" to listOf(context.cacheDir.path), + )) + } + + @After + fun closeBackend() { + backend.invokeApplication("cleanupExtensions", emptyMap()) + backendRoot.deleteRecursively() + } + + @Test + fun nativeFormatsPreserveTagsLyricsAndReplayGain() { + val context = InstrumentationRegistry.getInstrumentation().targetContext + val root = File(context.cacheDir, "finalizer-parity-${System.nanoTime()}").apply { mkdirs() } + try { + for ((extension, encoder) in listOf("mp3" to "libmp3lame", "opus" to "libopus", "flac" to "flac", "m4a" to "aac")) { + val input = File(root, "track.$extension") + val fixture = NativeDownloadFinalizer.runFFmpegArguments(arrayOf( + "-v", "error", "-f", "lavfi", "-i", "sine=frequency=997:sample_rate=48000", + "-t", "1.5", "-c:a", encoder, input.path, + )) + assertTrue(fixture.second, fixture.first) + val request = JSONObject() + .put("contract_version", 1).put("item_id", "example-$extension") + .put("service", "example-provider").put("track_name", "Parity test") + .put("artist_name", "Example artist").put("album_name", "Example album") + .put("album_artist", "Album Artist").put("track_number", 2).put("total_tracks", 10) + .put("quality", "LOSSLESS").put("storage_mode", "app") + .put("output_ext", ".$extension").put("embed_metadata", true) + .put("embed_lyrics", true).put("lyrics_mode", "both") + .put("embed_replaygain", true).put("duration_ms", 1500) + val result = NativeDownloadFinalizer.finalize( + context, "example-$extension", request.toString(), "{}", + JSONObject().put("success", true).put("file_path", input.path) + .put("file_name", input.name).put("lyrics_lrc", "[00:00.00]Example line"), + "{\"save_download_history\":false}", + ) + assertTrue(result.toString(), result.getBoolean("success")) + assertFalse(result.toString(), result.has("replaygain_warning")) + assertEquals(1.5, result.getJSONObject("replaygain").getDouble("duration_secs"), 0.001) + val output = File(result.getString("file_path")) + assertEquals(extension, output.extension) + assertEquals("[00:00.00]Example line", File(root, "track.lrc").readText()) + File(root, "track.lrc").delete() + val probe = NativeDownloadFinalizer.runFFmpegArguments(arrayOf( + "-hide_banner", "-i", output.path, "-map", "0:a:0", "-f", "null", "-", + )) + assertTrue(probe.second, probe.first) + val gainTag = if (extension == "opus") "r128_track_gain" else "replaygain_track_gain" + for (tag in listOf("Parity test", "Album Artist", "Example line", gainTag)) { + assertTrue("$extension: missing $tag\n${probe.second}", probe.second.contains(tag, ignoreCase = true)) + } + } + } finally { + root.deleteRecursively() + } + } + @Test fun misnamedMp4IsTaggedAndPublishedAsM4aWithoutOverwritingExistingAudio() { val context = InstrumentationRegistry.getInstrumentation().targetContext 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 a10845c3..6182e0b3 100644 --- a/android/app/src/main/kotlin/com/zarz/spotiflac/NativeDownloadFinalizer.kt +++ b/android/app/src/main/kotlin/com/zarz/spotiflac/NativeDownloadFinalizer.kt @@ -1269,11 +1269,11 @@ object NativeDownloadFinalizer { } private fun replayGainDurationSeconds(input: FinalizeInput): Double { - val duration = input.request.optInt("duration_ms", 0).let { - if (it > 0) it else trackInt(input, "duration", 0) - } - if (duration <= 0) return 1.0 - return if (duration > 10000) duration / 1000.0 else duration.toDouble() + val duration = NativeFinalizationPolicy.durationMilliseconds( + input.request.optLong("duration_ms", 0L), + trackInt(input, "duration", 0).toLong(), + ) + return if (duration > 0L) duration / 1000.0 else 1.0 } private fun buildHistoryRow(input: FinalizeInput, state: FinalizeState): ContentValues { diff --git a/android/app/src/main/kotlin/com/zarz/spotiflac/NativeFinalizationPolicy.kt b/android/app/src/main/kotlin/com/zarz/spotiflac/NativeFinalizationPolicy.kt index 0d81d2bf..c0f8317a 100644 --- a/android/app/src/main/kotlin/com/zarz/spotiflac/NativeFinalizationPolicy.kt +++ b/android/app/src/main/kotlin/com/zarz/spotiflac/NativeFinalizationPolicy.kt @@ -11,6 +11,11 @@ import kotlin.math.roundToInt * finalizer's I/O-heavy orchestration. */ internal object NativeFinalizationPolicy { + fun durationMilliseconds(requestMilliseconds: Long, trackSeconds: Long): Long { + if (requestMilliseconds > 0) return requestMilliseconds + return trackSeconds.coerceIn(0, Long.MAX_VALUE / 1000) * 1000 + } + fun resolvedAlbumRelativeDirectory( relativeDirectory: String, albumFolderTemplate: String, diff --git a/android/app/src/main/kotlin/com/zarz/spotiflac/NativeFinalizerMedia.kt b/android/app/src/main/kotlin/com/zarz/spotiflac/NativeFinalizerMedia.kt index d2eada61..731336ac 100644 --- a/android/app/src/main/kotlin/com/zarz/spotiflac/NativeFinalizerMedia.kt +++ b/android/app/src/main/kotlin/com/zarz/spotiflac/NativeFinalizerMedia.kt @@ -245,9 +245,7 @@ internal fun NativeDownloadFinalizer.resolveLyricsLrc(context: Context, input: N internal fun NativeDownloadFinalizer.lyricsDurationMs(input: NativeDownloadFinalizer.FinalizeInput): Long { val requestDuration = input.request.optLong("duration_ms", 0L) val trackDuration = trackInt(input, "duration", 0).toLong() - val duration = if (requestDuration > 0L) requestDuration else trackDuration - if (duration <= 0L) return 0L - return if (duration > 10000L) duration else duration * 1000L + return NativeFinalizationPolicy.durationMilliseconds(requestDuration, trackDuration) } internal fun nativeMetadataEditHandled(response: String): Boolean { diff --git a/android/app/src/test/kotlin/com/zarz/spotiflac/NativeFinalizationPolicyTest.kt b/android/app/src/test/kotlin/com/zarz/spotiflac/NativeFinalizationPolicyTest.kt index dd1664db..29821c49 100644 --- a/android/app/src/test/kotlin/com/zarz/spotiflac/NativeFinalizationPolicyTest.kt +++ b/android/app/src/test/kotlin/com/zarz/spotiflac/NativeFinalizationPolicyTest.kt @@ -7,6 +7,17 @@ import org.junit.Assert.assertTrue import org.junit.Test class NativeFinalizationPolicyTest { + @Test + fun durationUsesDeclaredUnitsForShortTracksAndLongPerformances() { + assertEquals(250L, NativeFinalizationPolicy.durationMilliseconds(250, 0)) + assertEquals(8000L, NativeFinalizationPolicy.durationMilliseconds(8000, 8)) + assertEquals(10000L, NativeFinalizationPolicy.durationMilliseconds(10000, 10)) + assertEquals(180000L, NativeFinalizationPolicy.durationMilliseconds(180000, 0)) + assertEquals(8000L, NativeFinalizationPolicy.durationMilliseconds(0, 8)) + assertEquals(14400000L, NativeFinalizationPolicy.durationMilliseconds(0, 14400)) + assertEquals(0L, NativeFinalizationPolicy.durationMilliseconds(0, -1)) + } + @Test fun lateAlbumMetadataResolvesOnlyThePendingFolderLeaf() { assertEquals( diff --git a/lib/providers/download_queue_provider.dart b/lib/providers/download_queue_provider.dart index e0858a0f..85bd96d6 100644 --- a/lib/providers/download_queue_provider.dart +++ b/lib/providers/download_queue_provider.dart @@ -44,6 +44,7 @@ import 'package:spotiflac_android/utils/progress_stream_poller.dart'; import 'package:spotiflac_android/providers/download_history_provider.dart'; import 'package:spotiflac_android/services/native_download_history.dart'; +import 'package:spotiflac_android/services/download_track_metadata.dart'; export 'package:spotiflac_android/providers/download_history_provider.dart'; export 'package:spotiflac_android/providers/download_queue_state.dart'; diff --git a/lib/providers/download_queue_provider_embedding.dart b/lib/providers/download_queue_provider_embedding.dart index 639313c3..80739e3d 100644 --- a/lib/providers/download_queue_provider_embedding.dart +++ b/lib/providers/download_queue_provider_embedding.dart @@ -22,6 +22,13 @@ class _DeezerExtendedMetadataFields { } extension _DownloadQueueEmbedding on DownloadQueueNotifier { + Future _prepareDownloadSourceTrack(Track track) async { + if (!track.id.startsWith('deezer:')) return track; + final id = track.id.substring('deezer:'.length); + final enriched = await enrichIncompleteDownloadTrack(track, 'deezer', id); + return identical(enriched, track) ? track : enriched.copyWith(deezerId: id); + } + Future _resolveDownloadAlbumCredit( Track track, AppSettings settings, diff --git a/lib/providers/download_queue_provider_native_worker.dart b/lib/providers/download_queue_provider_native_worker.dart index f7158158..89eb4bbd 100644 --- a/lib/providers/download_queue_provider_native_worker.dart +++ b/lib/providers/download_queue_provider_native_worker.dart @@ -877,8 +877,9 @@ extension _DownloadQueueNativeWorker on DownloadQueueNotifier { return null; } + final sourceTrack = await _prepareDownloadSourceTrack(item.track); item = item.copyWith( - track: await _resolveDownloadAlbumCredit(item.track, settings), + track: await _resolveDownloadAlbumCredit(sourceTrack, settings), ); final isSafMode = _isSafMode(settings); @@ -915,12 +916,6 @@ extension _DownloadQueueNativeWorker on DownloadQueueNotifier { } final outputExt = _determineOutputExt(quality, item.service); - if (settings.embedReplayGain && - outputExt != '.flac' && - outputExt != '.m4a') { - return null; - } - String? safFileName; final safOutputExt = isSafMode ? outputExt : ''; final baseFilenameFormat = _shouldTreatAsSingleRelease(item.track) @@ -992,7 +987,7 @@ extension _DownloadQueueNativeWorker on DownloadQueueNotifier { ).withStrategy(useExtensions: true, useFallback: state.autoFallback); return _NativeWorkerRequestContext( - item: item, + item: item.copyWith(track: trackForPayload), requestJson: jsonEncode(payload.toJson()), outputDir: outputDir, quality: quality, diff --git a/lib/providers/download_queue_provider_single_item.dart b/lib/providers/download_queue_provider_single_item.dart index 248c0ee4..04c45628 100644 --- a/lib/providers/download_queue_provider_single_item.dart +++ b/lib/providers/download_queue_provider_single_item.dart @@ -307,107 +307,8 @@ class _DownloadRun { } Future _enrichDeezerTrackIfNeeded() async { - final needsEnrichment = - trackToDownload.id.startsWith('deezer:') && - (trackToDownload.isrc == null || - trackToDownload.isrc!.isEmpty || - trackToDownload.trackNumber == null || - trackToDownload.trackNumber == 0 || - trackToDownload.totalTracks == null || - trackToDownload.totalTracks == 0 || - (trackToDownload.composer == null || - trackToDownload.composer!.isEmpty)); - - if (needsEnrichment) { - try { - _log.d( - 'Enriching incomplete metadata for Deezer track: ${trackToDownload.name}', - ); - _log.d( - 'Current ISRC: ${trackToDownload.isrc}, TrackNumber: ${trackToDownload.trackNumber}', - ); - final rawId = trackToDownload.id.split(':')[1]; - _log.d('Fetching full metadata for Deezer ID: $rawId'); - final fullData = await PlatformBridge.getProviderMetadata( - 'deezer', - 'track', - rawId, - ); - _log.d('Got response keys: ${fullData.keys.toList()}'); - - if (fullData.containsKey('track')) { - final trackData = fullData['track']; - _log.d('Track data type: ${trackData.runtimeType}'); - if (trackData is Map) { - final data = trackData; - _log.d('Track data keys: ${data.keys.toList()}'); - _log.d('ISRC from API: ${data['isrc']}'); - _log.d('album_type from API: ${data['album_type']}'); - final enrichedTotalTracks = readPositiveInt(data['total_tracks']); - final enrichedTotalDiscs = readPositiveInt(data['total_discs']); - final enrichedComposer = normalizeOptionalString( - data['composer']?.toString(), - ); - trackToDownload = trackToDownload.copyWith( - id: (data['spotify_id'] as String?) ?? trackToDownload.id, - name: (data['name'] as String?) ?? trackToDownload.name, - artistName: - (data['artists'] as String?) ?? trackToDownload.artistName, - albumName: - (data['album_name'] as String?) ?? trackToDownload.albumName, - albumArtist: data['album_artist'] as String?, - artistId: - (data['artist_id'] ?? data['artistId'])?.toString() ?? - trackToDownload.artistId, - albumId: data['album_id']?.toString() ?? trackToDownload.albumId, - coverUrl: data['images'] as String?, - duration: - ((data['duration_ms'] as int?) ?? - (trackToDownload.duration * 1000)) ~/ - 1000, - isrc: (data['isrc'] as String?) ?? trackToDownload.isrc, - trackNumber: data['track_number'] as int?, - discNumber: data['disc_number'] as int?, - totalDiscs: enrichedTotalDiscs ?? trackToDownload.totalDiscs, - releaseDate: data['release_date'] as String?, - deezerId: rawId, - albumType: - (data['album_type'] as String?) ?? trackToDownload.albumType, - totalTracks: enrichedTotalTracks ?? trackToDownload.totalTracks, - composer: enrichedComposer ?? trackToDownload.composer, - genre: data['genre']?.toString() ?? trackToDownload.genre, - label: data['label']?.toString() ?? trackToDownload.label, - copyright: - data['copyright']?.toString() ?? trackToDownload.copyright, - comment: data['comment']?.toString() ?? trackToDownload.comment, - explicit: - parseExplicitFlag(data['explicit']) ?? - trackToDownload.explicit, - upc: - (data['upc'] ?? data['barcode'])?.toString() ?? - trackToDownload.upc, - ); - _log.d( - 'Metadata enriched: Track ${trackToDownload.trackNumber}, Disc ${trackToDownload.discNumber}, ISRC ${trackToDownload.isrc}, AlbumType ${trackToDownload.albumType}', - ); - } else { - _log.w('Unexpected track data type: ${trackData.runtimeType}'); - } - } else { - _log.w('Response does not contain track key'); - } - } catch (e, stack) { - _log.w('Failed to enrich metadata: $e'); - _log.w('Stack trace: $stack'); - } - - if (await _shouldAbort('during metadata enrichment')) { - return false; - } - } - - _log.d('Track coverUrl after enrichment: ${trackToDownload.coverUrl}'); - return true; + trackToDownload = await n._prepareDownloadSourceTrack(trackToDownload); + return !await _shouldAbort('during metadata enrichment'); } Future _resolveOutputTarget() async { diff --git a/lib/services/download_track_metadata.dart b/lib/services/download_track_metadata.dart new file mode 100644 index 00000000..4889bb4f --- /dev/null +++ b/lib/services/download_track_metadata.dart @@ -0,0 +1,59 @@ +import 'package:spotiflac_android/models/track.dart'; +import 'package:spotiflac_android/services/platform_bridge.dart'; +import 'package:spotiflac_android/utils/int_utils.dart'; +import 'package:spotiflac_android/utils/logger.dart'; +import 'package:spotiflac_android/utils/string_utils.dart'; + +/// Shared preparation before either queue chooses folders, filenames or tags. +Future enrichIncompleteDownloadTrack( + Track track, + String providerId, + String providerTrackId, { + Future> Function(String, String, String)? loadMetadata, +}) async { + if (normalizeOptionalString(track.isrc) != null && + (track.trackNumber ?? 0) > 0 && + (track.totalTracks ?? 0) > 0 && + normalizeOptionalString(track.composer) != null) { + return track; + } + try { + final response = await (loadMetadata ?? PlatformBridge.getProviderMetadata)( + providerId, + 'track', + providerTrackId, + ).timeout(const Duration(seconds: 8)); + final data = response['track']; + if (data is! Map) return track; + String? text(String key) => normalizeOptionalString(data[key]?.toString()); + final durationMs = readPositiveInt(data['duration_ms']); + return track.copyWith( + id: text('spotify_id') ?? track.id, + name: text('name') ?? track.name, + artistName: text('artists') ?? track.artistName, + albumName: text('album_name') ?? track.albumName, + albumArtist: text('album_artist') ?? track.albumArtist, + artistId: text('artist_id') ?? text('artistId') ?? track.artistId, + albumId: text('album_id') ?? track.albumId, + coverUrl: text('images') ?? track.coverUrl, + duration: durationMs == null ? track.duration : durationMs ~/ 1000, + isrc: text('isrc') ?? track.isrc, + trackNumber: readPositiveInt(data['track_number']) ?? track.trackNumber, + discNumber: readPositiveInt(data['disc_number']) ?? track.discNumber, + totalDiscs: readPositiveInt(data['total_discs']) ?? track.totalDiscs, + releaseDate: text('release_date') ?? track.releaseDate, + albumType: text('album_type') ?? track.albumType, + totalTracks: readPositiveInt(data['total_tracks']) ?? track.totalTracks, + composer: text('composer') ?? track.composer, + genre: text('genre') ?? track.genre, + label: text('label') ?? track.label, + copyright: text('copyright') ?? track.copyright, + comment: text('comment') ?? track.comment, + explicit: parseExplicitFlag(data['explicit']) ?? track.explicit, + upc: text('upc') ?? text('barcode') ?? track.upc, + ); + } catch (error) { + AppLogger('DownloadMetadata').w('Track enrichment failed: $error'); + return track; + } +} diff --git a/test/download_track_metadata_test.dart b/test/download_track_metadata_test.dart new file mode 100644 index 00000000..1afe90d7 --- /dev/null +++ b/test/download_track_metadata_test.dart @@ -0,0 +1,122 @@ +import 'package:flutter_test/flutter_test.dart'; +import 'package:spotiflac_android/models/track.dart'; +import 'package:spotiflac_android/services/download_track_metadata.dart'; + +void main() { + const original = Track( + id: 'example:track', + source: 'example', + name: 'Song', + artistName: 'Artist', + albumName: 'Album', + duration: 8, + trackNumber: 0, + albumId: 'album', + previewUrl: 'https://example.test/preview', + audioQuality: 'LOSSLESS', + audioModes: 'STEREO', + ); + + test( + 'both download paths receive full credits, numbering and cover before naming', + () async { + final enriched = await enrichIncompleteDownloadTrack( + original, + 'example', + 'track', + loadMetadata: (provider, type, id) async { + expect([provider, type, id], ['example', 'track', 'track']); + return { + 'track': { + 'album_artist': 'Album Artist', + 'album_name': 'Resolved Album', + 'track_number': 3, + 'total_tracks': 12, + 'disc_number': 2, + 'total_discs': 2, + 'composer': 'Composer', + 'isrc': 'USAAA2400001', + 'duration_ms': 8000, + 'images': 'https://example.test/cover', + 'explicit': true, + 'upc': '123456789012', + }, + }; + }, + ); + expect(enriched.toJson(), { + ...original.toJson(), + 'albumArtist': 'Album Artist', + 'albumName': 'Resolved Album', + 'trackNumber': 3, + 'totalTracks': 12, + 'discNumber': 2, + 'totalDiscs': 2, + 'composer': 'Composer', + 'isrc': 'USAAA2400001', + 'coverUrl': 'https://example.test/cover', + 'explicit': true, + 'upc': '123456789012', + }); + }, + ); + + test('empty optional fields do not erase existing metadata', () async { + final enriched = await enrichIncompleteDownloadTrack( + original, + 'example', + 'track', + loadMetadata: (_, _, _) async => { + 'track': { + 'name': '', + 'album_name': null, + 'duration_ms': 0, + 'disc_number': -1, + }, + }, + ); + expect(enriched.toJson(), original.toJson()); + }); + + test( + 'complete tracks avoid an extra lookup and failed lookups keep source metadata', + () async { + var calls = 0; + Future> load( + String provider, + String type, + String id, + ) async { + calls++; + throw StateError('provider unavailable'); + } + + final complete = original.copyWith( + isrc: 'USAAA2400001', + trackNumber: 1, + totalTracks: 12, + composer: 'Composer', + ); + expect( + await enrichIncompleteDownloadTrack( + complete, + 'example', + 'track', + loadMetadata: load, + ), + same(complete), + ); + expect(calls, 0); + expect( + await enrichIncompleteDownloadTrack( + original, + 'example', + 'track', + loadMetadata: load, + ), + same(original), + ); + expect(calls, 1); + }, + ); +}