diff --git a/listenarr.application/Downloads/Import/DownloadImportService.cs b/listenarr.application/Downloads/Import/DownloadImportService.cs index d93309848..58fb8cfba 100644 --- a/listenarr.application/Downloads/Import/DownloadImportService.cs +++ b/listenarr.application/Downloads/Import/DownloadImportService.cs @@ -90,31 +90,20 @@ public async Task> ImportDownloadFilesAsync(Audiobook audiobo try { // Precompute audiobook and best existing quality to avoid import-order races - string? bestExisting = null; - QualityProfile? abProfile = null; - - abProfile = audiobook.QualityProfile; + AudioQualityInput? bestExisting = null; + var abProfile = audiobook.QualityProfile; if (audiobook.Files != null && audiobook.Files.Count != 0) { - foreach (var f in audiobook.Files) + foreach (var existingFile in audiobook.Files) { - string q = string.Empty; - if (!string.IsNullOrEmpty(f.Format)) q = f.Format; - if (f.Bitrate.HasValue) - { - var kb = f.Bitrate.Value / 1000; - if (kb >= 320) q = "MP3 320kbps"; - else if (kb >= 256) q = "MP3 256kbps"; - else if (kb >= 192) q = "MP3 192kbps"; - else if (kb >= 128) q = "MP3 128kbps"; - } - if (string.IsNullOrEmpty(q) && !string.IsNullOrEmpty(f.Path)) q = ImportQualityEvaluator.Determine(null, f.Path); + var existingInput = ImportQualityEvaluator.FromFile(existingFile); - if (string.IsNullOrEmpty(bestExisting)) bestExisting = q; - else if (!string.IsNullOrEmpty(q) && !string.IsNullOrEmpty(bestExisting) && abProfile != null && ImportQualityEvaluator.IsAcceptable(q, bestExisting, abProfile)) + // "Not worse than the current best" climbs the list to the hardest quality to beat. + if (bestExisting is null + || ImportQualityEvaluator.IsAcceptable(existingInput, bestExisting.Value, abProfile)) { - bestExisting = q; + bestExisting = existingInput; } } } @@ -184,14 +173,18 @@ public async Task> ImportDownloadFilesAsync(Audiobook audiobo candidateMetadata = await metadataService.ExtractFileMetadataAsync(file); } - var candidateQuality = ImportQualityEvaluator.Determine(candidateMetadata, file); + var candidateInput = ImportQualityEvaluator.FromMetadata(candidateMetadata, file); try { - if (audiobook.Files != null && audiobook.Files.Count != 0 && !ImportQualityEvaluator.IsAcceptable(candidateQuality, bestExisting, abProfile)) + if (bestExisting is not null + && !ImportQualityEvaluator.IsAcceptable(candidateInput, bestExisting.Value, abProfile)) { - results.Add(ImportResult.Skipped($"candidate quality '{candidateQuality}' is not better than existing '{bestExisting}'")); - logger.LogInformation($"Skipping import of file {file} for audiobook {audiobook.Id} because candidate quality '{candidateQuality}' is not better than existing '{bestExisting}'"); + var candidateQuality = ImportQualityEvaluator.Describe(candidateInput, abProfile); + var existingQuality = ImportQualityEvaluator.Describe(bestExisting.Value, abProfile); + + results.Add(ImportResult.Skipped($"candidate quality '{candidateQuality}' is not better than existing '{existingQuality}'")); + logger.LogInformation($"Skipping import of file {file} for audiobook {audiobook.Id} because candidate quality '{candidateQuality}' is not better than existing '{existingQuality}'"); continue; } } diff --git a/listenarr.application/Downloads/Import/ImportQualityEvaluator.cs b/listenarr.application/Downloads/Import/ImportQualityEvaluator.cs index 9e222133b..8c165e6c7 100644 --- a/listenarr.application/Downloads/Import/ImportQualityEvaluator.cs +++ b/listenarr.application/Downloads/Import/ImportQualityEvaluator.cs @@ -1,3 +1,5 @@ +using Listenarr.Domain.Common; + namespace Listenarr.Application.Downloads.Import; public static class ImportQualityEvaluator @@ -31,22 +33,72 @@ public static string Determine(AudioMetadata? metadata, string path) }; } - public static bool IsAcceptable(string? candidate, string? existing, QualityProfile? profile) + /// Project an already-imported library file onto the shared quality vocabulary. + public static AudioQualityInput FromFile(AudiobookFile file) => new() { - if (string.IsNullOrWhiteSpace(candidate) || string.IsNullOrWhiteSpace(existing) || profile == null) - { - return true; - } + Codec = file.Codec, + Container = file.Container, + Format = file.Format, + BitrateBitsPerSecond = file.Bitrate, + Path = file.Path + }; + + /// Project an incoming download's extracted metadata onto the shared quality vocabulary. + public static AudioQualityInput FromMetadata(AudioMetadata? metadata, string path) => new() + { + Codec = metadata?.Codec, + Container = metadata?.Container, + Format = metadata?.Format, + BitrateBitsPerSecond = metadata?.BitRate, + Path = path + }; - return !TryParseBitrate(candidate, out var candidateBitrate) - || !TryParseBitrate(existing, out var existingBitrate) - || candidateBitrate >= existingBitrate; + /// Human-readable quality label for import logs: the matched profile rung when the + /// profile can rank the file, otherwise the file's own format/extension. + public static string Describe(AudioQualityInput input, QualityProfile? profile) + { + var rung = QualityMatcher.MatchLabel(input, profile); + if (!string.IsNullOrWhiteSpace(rung)) return rung!; + if (!string.IsNullOrWhiteSpace(input.Format)) return input.Format!; + return Determine(null, input.Path ?? string.Empty); } - private static bool TryParseBitrate(string quality, out int bitrate) + /// + /// Whether is not worse than and may + /// therefore be imported. Equal quality is acceptable on purpose: a multi-file audiobook imports + /// parts that all share the same quality as the parts already on disk, and requiring a strict + /// upgrade would skip every one of them. (Automatic search wants the stricter + /// instead.) + /// + public static bool IsAcceptable(AudioQualityInput candidate, AudioQualityInput existing, QualityProfile? profile) { - bitrate = 0; - var match = System.Text.RegularExpressions.Regex.Match(quality, @"\d{2,}"); - return match.Success && int.TryParse(match.Value, out bitrate); + if (profile?.Qualities is { Count: > 0 }) + { + var candidateMatch = QualityMatcher.Match(candidate, profile); + var existingMatch = QualityMatcher.Match(existing, profile); + + // Both sides land on a profile rung, so the profile's own ordering decides. + if (candidateMatch.IsMatch && existingMatch.IsMatch) + { + return candidateMatch.Rung!.Priority <= existingMatch.Rung!.Priority; + } + } + + // No profile, an empty profile, or a side the profile cannot rank. Fall back to the codec and + // bitrate facts so gating still holds for unconfigured/partial profiles. + if (QualityMatcher.IsLossless(existing) && !QualityMatcher.IsLossless(candidate)) + { + return false; + } + + if (candidate.BitrateBitsPerSecond is int candidateBitrate and > 0 + && existing.BitrateBitsPerSecond is int existingBitrate and > 0) + { + return candidateBitrate >= existingBitrate; + } + + // Quality is genuinely unknown on at least one side: allow the import rather than silently + // dropping a file we cannot reason about. + return true; } } diff --git a/listenarr.domain/Common/QualityMatcher.cs b/listenarr.domain/Common/QualityMatcher.cs index ed4238cba..b54f9929a 100644 --- a/listenarr.domain/Common/QualityMatcher.cs +++ b/listenarr.domain/Common/QualityMatcher.cs @@ -178,6 +178,13 @@ static QualityMatchResult ToAllowedMatch(QualityDefinition rung) public static string? MatchLabel(AudioQualityInput file, QualityProfile? profile) => Match(file, profile).Rung?.Quality; + /// + /// Whether a file is lossless, derived from the same mapped codec groups used for matching. + /// Exposed so profile-agnostic callers (e.g. the import gate) can honour lossless-vs-lossy + /// even when no profile rung is available. + /// + public static bool IsLossless(AudioQualityInput file) => IsLosslessFile(file); + /// /// Whether a file meets or exceeds the profile cutoff. A blank cutoff is always met; /// a missing/codec-mismatched file is not. diff --git a/tests/Features/Application/Downloads/Import/DownloadImportServiceTests.cs b/tests/Features/Application/Downloads/Import/DownloadImportServiceTests.cs index 0aac0af18..d3765d546 100644 --- a/tests/Features/Application/Downloads/Import/DownloadImportServiceTests.cs +++ b/tests/Features/Application/Downloads/Import/DownloadImportServiceTests.cs @@ -297,6 +297,209 @@ await _audiobookFileRepository.AddAsync(new AudiobookFileBuilder() Assert.Single(files); } + // Reproduction for issue #582 - "Quality filters: Flaws in the current logic". + // The numeric path (MP3 320 vs MP3 128) above happens to work, but DownloadImportService.IsQualityBetter + // ignores the QualityProfile entirely and only compares regex-extracted bitrates. For a non-numeric + // format such as FLAC, the parse fails and the method falls through to `return true`, so a lossy MP3 + // download is treated as "better" than an existing lossless FLAC and gets imported on top of it. + // + // This test asserts the INTENDED behaviour (the lossy download is skipped, leaving only the existing + // FLAC) and therefore FAILS against the current logic - that failure is the reproduction of #582. + [Fact] + public async Task QualityGating_LosslessExisting_SkipsLossyDownload() + { + var library = FileService.GetTempDirectory("library"); + var existingFlac = await FileService.GetFileAsync(library, "existing.flac"); + + // Profile that clearly ranks lossless FLAC above lossy MP3. + var profile = new QualityProfileBuilder() + .WithName("Lossless Preferred") + .Build(); + profile.Qualities = + [ + new QualityDefinition { Quality = "flac", Codec = "FLAC", IsLossless = true, Priority = 1 }, + new QualityDefinition { Quality = "mp3", Codec = "MP3", Bitrate = 320, Priority = 2 }, + ]; + var qualityProfile = await _qualityProfileRepository.AddAsync(profile); + + var audiobook = await _audiobookRepository.AddAsync(new AudiobookBuilder() + .WithTitle("The Lossless Book") + .WithBasePath(library) + .WithQualityProfile(qualityProfile) + .Build()); + + // Existing lossless FLAC file in the library. No bitrate is set so that "best existing quality" + // stays the format string "flac" (a non-numeric quality) - which is what triggers the flaw. + await _audiobookFileRepository.AddAsync(new AudiobookFileBuilder() + .WithAudiobook(audiobook) + .WithPath(existingFlac) + .WithFormat("flac") + .Build()); + + // Incoming completed download is a lower-quality lossy MP3. + var lossyMp3 = await FileService.GetTempFileAsync("lossy.mp3"); + metadataServiceMock.AddMetadata(@"lossy\.mp3$", new AudioMetadata { Title = "Lossy Download", Format = "mp3", BitRate = 320000 }); + + await _applicationSettingsRepository.SaveAsync(new ApplicationSettings { OutputPath = Path.GetTempPath(), EnableMetadataProcessing = true, CompletedFileAction = FileAction.Move }); + + // Act - process the lossy completed download against the audiobook that already has a FLAC. + var downloadImportService = _provider.GetRequiredService(); + await downloadImportService.ImportDownloadFilesAsync(audiobook, [lossyMp3]); + + // Assert: the lossy MP3 must NOT be imported over the existing lossless FLAC. + var files = await _audiobookFileRepository.GetByAudiobookIdAsync(audiobook.Id); + Assert.Single(files); + Assert.Equal("flac", files.First().Format); + } + + // A higher-bitrate file of the same codec ranks onto a better profile rung, so it imports. + [Fact] + public async Task QualityGating_NumericUpgrade_ImportsHigherBitrate() + { + var library = FileService.GetTempDirectory("library"); + var existingFile = await FileService.GetFileAsync(library, "existing.mp3"); + + var qualityProfile = await _qualityProfileRepository.AddAsync(new QualityProfileBuilder() + .WithName("Structured") + .WithStructuredDefaults() + .Build()); + + var audiobook = await _audiobookRepository.AddAsync(new AudiobookBuilder() + .WithTitle("The Upgradable Book") + .WithBasePath(library) + .WithQualityProfile(qualityProfile) + .Build()); + + await _audiobookFileRepository.AddAsync(new AudiobookFileBuilder() + .WithAudiobook(audiobook) + .WithPath(existingFile) + .WithFormat("mp3") + .WithBitrate(128000) + .Build()); + + var upgrade = await FileService.GetTempFileAsync("upgrade.mp3"); + metadataServiceMock.AddMetadata(@"upgrade\.mp3$", new AudioMetadata { Title = "Upgrade", Format = "mp3", BitRate = 320000 }); + + await _applicationSettingsRepository.SaveAsync(new ApplicationSettings { OutputPath = Path.GetTempPath(), EnableMetadataProcessing = true, CompletedFileAction = FileAction.Move }); + + var downloadImportService = _provider.GetRequiredService(); + await downloadImportService.ImportDownloadFilesAsync(audiobook, [upgrade]); + + var files = await _audiobookFileRepository.GetByAudiobookIdAsync(audiobook.Id); + Assert.Equal(2, files.Count()); + } + + // Cross-codec upgrade: the profile ranks lossless FLAC above MP3, so a FLAC download imports + // over an existing MP3. This is the mirror of the #582 repro and only works because the + // comparison is profile-driven rather than bitrate-driven (FLAC carries no comparable bitrate). + [Fact] + public async Task QualityGating_CrossFormatUpgrade_ImportsLosslessOverLossy() + { + var library = FileService.GetTempDirectory("library"); + var existingFile = await FileService.GetFileAsync(library, "existing.mp3"); + + var qualityProfile = await _qualityProfileRepository.AddAsync(new QualityProfileBuilder() + .WithName("Structured") + .WithStructuredDefaults() + .Build()); + + var audiobook = await _audiobookRepository.AddAsync(new AudiobookBuilder() + .WithTitle("The Lossy Book") + .WithBasePath(library) + .WithQualityProfile(qualityProfile) + .Build()); + + await _audiobookFileRepository.AddAsync(new AudiobookFileBuilder() + .WithAudiobook(audiobook) + .WithPath(existingFile) + .WithFormat("mp3") + .WithBitrate(320000) + .Build()); + + var upgrade = await FileService.GetTempFileAsync("upgrade.flac"); + metadataServiceMock.AddMetadata(@"upgrade\.flac$", new AudioMetadata { Title = "Lossless Upgrade", Format = "flac" }); + + await _applicationSettingsRepository.SaveAsync(new ApplicationSettings { OutputPath = Path.GetTempPath(), EnableMetadataProcessing = true, CompletedFileAction = FileAction.Move }); + + var downloadImportService = _provider.GetRequiredService(); + await downloadImportService.ImportDownloadFilesAsync(audiobook, [upgrade]); + + var files = await _audiobookFileRepository.GetByAudiobookIdAsync(audiobook.Id); + Assert.Equal(2, files.Count()); + } + + // Equal quality must be ACCEPTED, not skipped. A multi-file audiobook imports parts whose + // quality merely matches what is already on disk; a strictly-better rule would skip every one + // of them. This is why import compares "not worse" while automatic search compares "better". + [Fact] + public async Task QualityGating_EqualQuality_IsImported() + { + var library = FileService.GetTempDirectory("library"); + var existingFile = await FileService.GetFileAsync(library, "existing.mp3"); + + var qualityProfile = await _qualityProfileRepository.AddAsync(new QualityProfileBuilder() + .WithName("Structured") + .WithStructuredDefaults() + .Build()); + + var audiobook = await _audiobookRepository.AddAsync(new AudiobookBuilder() + .WithTitle("The Multi Part Book") + .WithBasePath(library) + .WithQualityProfile(qualityProfile) + .Build()); + + await _audiobookFileRepository.AddAsync(new AudiobookFileBuilder() + .WithAudiobook(audiobook) + .WithPath(existingFile) + .WithFormat("mp3") + .WithBitrate(320000) + .Build()); + + // Same codec, same bitrate as the file already on disk - i.e. the next chapter part. + var sameQuality = await FileService.GetTempFileAsync("part2.mp3"); + metadataServiceMock.AddMetadata(@"part2\.mp3$", new AudioMetadata { Title = "Part Two", Format = "mp3", BitRate = 320000 }); + + await _applicationSettingsRepository.SaveAsync(new ApplicationSettings { OutputPath = Path.GetTempPath(), EnableMetadataProcessing = true, CompletedFileAction = FileAction.Move }); + + var downloadImportService = _provider.GetRequiredService(); + await downloadImportService.ImportDownloadFilesAsync(audiobook, [sameQuality]); + + var files = await _audiobookFileRepository.GetByAudiobookIdAsync(audiobook.Id); + Assert.Equal(2, files.Count()); + } + + // No profile and no usable codec/bitrate on either side: the quality is genuinely unknown, so + // the import is allowed. Gating must never silently drop a file it cannot reason about. + [Fact] + public async Task QualityGating_UnknownQualityAndNoProfile_AllowsImport() + { + var library = FileService.GetTempDirectory("library"); + var existingFile = await FileService.GetFileAsync(library, "existing.mp3"); + + // Audiobook deliberately has no quality profile assigned. + var audiobook = await _audiobookRepository.AddAsync(new AudiobookBuilder() + .WithTitle("The Unprofiled Book") + .WithBasePath(library) + .Build()); + + // Existing file with no format and no bitrate - nothing to rank it by. + await _audiobookFileRepository.AddAsync(new AudiobookFileBuilder() + .WithAudiobook(audiobook) + .WithPath(existingFile) + .Build()); + + var unknown = await FileService.GetTempFileAsync("unknown.mp3"); + metadataServiceMock.AddMetadata(@"unknown\.mp3$", new AudioMetadata { Title = "Unknown Quality" }); + + await _applicationSettingsRepository.SaveAsync(new ApplicationSettings { OutputPath = Path.GetTempPath(), EnableMetadataProcessing = true, CompletedFileAction = FileAction.Move }); + + var downloadImportService = _provider.GetRequiredService(); + await downloadImportService.ImportDownloadFilesAsync(audiobook, [unknown]); + + var files = await _audiobookFileRepository.GetByAudiobookIdAsync(audiobook.Id); + Assert.Equal(2, files.Count()); + } + [Fact] public async Task MultiFileImport_ImportsAllFiles_WithUniqueNames() {