From b79538f040d1cc4181a32168f5d4b3382d3f7d19 Mon Sep 17 00:00:00 2001 From: revtex Date: Wed, 2 Sep 2026 17:55:42 -0400 Subject: [PATCH 1/2] Type the cover art as the front cover, and settle three profile differences it should not adopt MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The encoding profiles were audited flag by flag against another Windows Spotify recorder's, on the working assumption that any difference was a gap in ours. One was, and it was worse than it first looked. The other three turned out to be decisions this project had already taken for reasons that were nowhere written down, which is the same as not having taken them. A picture inside an audio file carries a type — front cover, back cover, artist photo, and a dozen more — and a description of its own. Offstream set neither. `-disposition:v attached_pic` marks the stream as artwork and settles nothing else, leaving the type at 0, which both ID3's APIC frame and FLAC's METADATA_BLOCK_PICTURE spell "Other". Every file this app has ever written carries its sleeve under that type, so software that goes looking for a front cover specifically passes over a file that has one: the art is in there, correct and complete, and the sleeve is still a grey box. Two `-metadata:s:v` arguments fix it, and neither is the free text it resembles. "comment" is read by the muxer as the picture *type* and only takes the spellings the format defines — "Cover (front)" selects type 3, and anything unrecognised falls back to "Other" without complaint; encoding with `comment=Sleeve test` produced a file reporting "Other" with that string nowhere in it. "title" is the description, and that one is genuinely free text. ffprobe cannot tell those two cases apart, because it reports the type back through the same `comment` key the argument uses, so a file where the string was stored verbatim and one where it was interpreted read identically — which is why the new integration test asserts through TagLib#, the way a player sees it, and why the isolation run behind it encoded with each argument alone and searched the output bytes. Removing either argument fails that test. M4A takes both and stores neither: the mov muxer keeps a cover as a bare atom with nowhere to put a type or a description. They are written in the shared attached-picture branch anyway, because the alternative is a per-format exception list that has to stay right about a muxer detail nobody re-checks. The three differences that stay differences, each with the fact that decides it: MP3 keeps constant bitrate. The other ladder passes `-abr 1` at every rung and reserves plain `-b:a 320k` for a top rung of its own — so Offstream's default *is* that top rung, and taking the ABR flag without also adding a constant-bitrate rung would move the default down a notch on the most-used format. Adding the rung means the bitrate stops being a plain kbps number, which is the LAMEPreset-shaped setting §5.1 rejected deliberately. WAV keeps `-c:a pcm_s16le`, and §5.1's offer of a stream copy "when the temp already matches" is struck: it never matches. WasapiLoopbackCapture reports the endpoint's shared-mode mix format, which on Windows is 32-bit float — confirmed with ffprobe, `codec_name=pcm_f32le`. Copying the temp file would make the WAV output float32, twice the size, in the one format people choose because they are handing it to something else. That is a different file, not a cheaper way to make the same one. Cover art keeps `-c:v mjpeg` over a stream copy of the downloaded bytes. `CoverArtFetcher.TempFileFor` preserves a `.png` extension on purpose, because `CoverArtWriter` reads the picture's MIME type off it, so the source is not reliably JPEG and a copy would put a PNG into an APIC frame. The re-encode is normalisation, not waste. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 10 ++++ docs/MODERNIZATION-PLAN.md | 56 ++++++++++++++++++- .../Encoding/FFmpegArguments.cs | 20 +++++++ .../Encoding/CoverArtIntegrationTests.cs | 40 +++++++++++++ .../Encoding/FFmpegArgumentsTests.cs | 27 +++++++++ 5 files changed, 152 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d1d7229..47d7678 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -51,6 +51,16 @@ phase plan these entries follow. or Advanced left the Record page describing the file the previous settings would have produced, until something was recorded. It now follows a saved setting straight away. +- **Embedded cover art was filed as "Other", not as the front cover.** A picture inside an audio + file carries a type — front cover, back cover, artist photo, and a dozen more — and a + description of its own. Offstream set neither. It marked the stream as attached artwork and + left the type at 0, which both ID3's `APIC` frame and FLAC's `METADATA_BLOCK_PICTURE` spell + `Other`, so software that goes looking for a front cover specifically passed over files that + had one: the sleeve was in there, correct and complete, and still showed as a grey box. The + picture is now typed as the front cover and described as one. M4A is the exception and always + was — its container has nowhere to keep either field, so the sleeve goes in bare there whatever + is asked for. + ## [0.2.0] - 2026-08-31 **Upgrading from 0.1.0 resets one setting.** The three advertisement switches diff --git a/docs/MODERNIZATION-PLAN.md b/docs/MODERNIZATION-PLAN.md index be6c927..3684bb0 100644 --- a/docs/MODERNIZATION-PLAN.md +++ b/docs/MODERNIZATION-PLAN.md @@ -173,7 +173,7 @@ All conversion goes through ffmpeg. Capture writes raw PCM WAV to a temp file; f | Format | Args | Notes | | --- | --- | --- | | MP3 | `-c:a libmp3lame -b:a {rate}k` | CBR; VBR (`-q:a`) can be exposed later | -| WAV | `-c:a pcm_s16le` | Or stream-copy when the temp already matches | +| WAV | `-c:a pcm_s16le` | Never a stream copy — the temp is float32, see the 2026-09-02 finding | | Opus | `-c:a libopus -b:a {rate}k` | Ogg container, `.opus` | | **FLAC** | `-c:a flac -compression_level 8` | New — near-free once ffmpeg owns conversion | | **AAC/M4A** | `-c:a aac -b:a {rate}k` | New | @@ -1042,6 +1042,60 @@ The tempting fix is worse than the bug: splitting the box on commas the way the The never-erase rule from the day before became one helper, `LibraryLookup`, rather than two hand-written copies, and grew from genre and year to all seven fields a lookup can leave empty. There are still two call sites, and both are still load-bearing — `MetadataViewModel.FetchOneAsync` for every automatic lookup and `SpotifyCatalogEnricher` for the manual **Use this** path, which does not pass through it — but the rule itself now exists once, so the next field cannot be added to one copy and forgotten in the other. That is precisely how the bug survived its first fix. +### Finding: three of the four ways the encoding profiles differ from a comparable recorder are ours to keep (2026-09-02) + +The profiles were audited flag by flag against another Windows Spotify recorder's, on the working +assumption that any difference was a gap. One was. The other three are decisions this project had +already taken, and they are written down here rather than left to be rediscovered, because each +one reads from the outside as an obvious improvement that nobody has got round to. + +**The real gap: the attached picture was typed `Other`.** `-disposition:v attached_pic` marks a +stream as cover art and settles nothing else about it — in particular it leaves the picture type +at 0, which is `Other` in both ID3's `APIC` frame and FLAC's `METADATA_BLOCK_PICTURE`. Every file +Offstream had ever written carried its sleeve under that type, and software that looks for a +front cover specifically skips it. + +Two `-metadata:s:v` arguments fix it, and the surprise is that **neither is free text in the way +it looks**. `comment` is read by the muxer as the picture *type*, matched against the spellings +the format defines: `Cover (front)` selects type 3, and anything unrecognised falls back to +`Other` without complaint — encoding with `comment=Sleeve test` produced a file reporting +`Other`, and the string was nowhere in it. `title` is the description and is genuinely free text. +Reading the result back with ffprobe is not enough to tell these apart, because ffprobe reports +the *type* through the same `comment` key the argument uses, so a file where the string was +stored verbatim and a file where it was interpreted look identical; the isolation run — each +argument alone, then a byte search of the output — is what separated them, and +`Encode_TypesTheCoverArtAsTheFrontCover` asserts it through TagLib# for that reason. M4A takes +both arguments and stores neither: the mov muxer keeps a cover as a bare atom with nowhere to put +a type or a description. + +**MP3 stays constant-bitrate.** The other ladder passes `-abr 1` at every rung and keeps plain +`-b:a 320k` for a top rung of its own. Offstream's `-b:a {rate}k` with no `-abr` *is* that top +rung — the default output is already the best MP3 that ladder can produce. Taking the `-abr 1` +without also adding a constant-bitrate rung would move the default down a notch on the +most-used format; adding the rung means the bitrate stops being a plain kbps number, which is the +`LAMEPreset`-shaped setting §5.1 rejected on purpose. ABR is a genuine quality win at 96 and 128 +kbps, so this is worth reopening if those rungs turn out to be the ones people use. It buys +nothing at a default of 320. + +**WAV stays `-c:a pcm_s16le`, and the stream-copy §5.1 used to offer is off the table.** The +other implementation copies the captured WAV instead of encoding it, which is the same idea. The +temp file never matches: `WasapiLoopbackCapture` reports the endpoint's shared-mode mix format, +which on Windows is 32-bit float — `codec_name=pcm_f32le`, confirmed with ffprobe. Copying it +would make Offstream's WAV output float32, twice the size of the 16-bit file it writes today, in +the one format people pick precisely because they are handing it to something else. The copy is +not a cheaper way to produce the same file; it produces a different file. + +**Cover art stays `-c:v mjpeg`.** The other implementation pipes the downloaded bytes through +`-c:v copy`, which is a free generation of quality whenever the source is already JPEG. It is not +always JPEG here: `CoverArtFetcher.TempFileFor` keeps a `.png` extension deliberately, because +`CoverArtWriter` derives the picture's MIME type from it, and `copy` would put a PNG into an APIC +frame. The re-encode is normalisation, not waste. + +One thing the audit turned up that is neither: `WaveFormatExtensions.GetMp3Restrictions` has no +caller outside its own tests. It is a LAME-era guard — ffmpeg resamples and downmixes without +being asked — kept because it came across with the ported suite. Worth deleting when something +else touches that file, not on its own. + --- ## 12. Risks diff --git a/src/Offstream.Core/Encoding/FFmpegArguments.cs b/src/Offstream.Core/Encoding/FFmpegArguments.cs index 6319ac3..df22316 100644 --- a/src/Offstream.Core/Encoding/FFmpegArguments.cs +++ b/src/Offstream.Core/Encoding/FFmpegArguments.cs @@ -70,6 +70,26 @@ public static IReadOnlyList Build(EncodeRequest request) // With two inputs the mapping must be explicit, or ffmpeg picks one stream per type. args.AddRange(["-map", "0:a", "-map", "1:v"]); args.AddRange(["-c:v", "mjpeg", "-disposition:v", "attached_pic"]); + + // Types the picture and names it. -disposition:v attached_pic only says "this + // stream is cover art" — it leaves the picture type at 0, which both ID3's APIC + // frame and FLAC's METADATA_BLOCK_PICTURE spell "Other", and software looking for a + // front cover specifically then passes over a file that has one. + // + // The two arguments do different jobs, and neither is free text. "comment" is read + // by the muxer as the picture type and only takes the spellings the format defines: + // "Cover (front)" selects type 3, and anything it does not recognise silently falls + // back to "Other" — so this string is a constant, not a caption. "title" is the + // description, and that one is genuinely free text. Both verified by encoding with + // each argument alone and reading the result back. + // + // M4A takes them and stores neither: the mov muxer keeps a cover as a bare atom with + // nowhere to put a type or a description. They are still written in the shared branch + // rather than per format, because the alternative is an exception list that has to + // stay right about a muxer detail nobody re-checks. English on purpose: this is tag + // content other software matches on, not a string the user reads. + args.AddRange(["-metadata:s:v", "title=Album cover"]); + args.AddRange(["-metadata:s:v", "comment=Cover (front)"]); } foreach (var argument in profile.CodecArguments) diff --git a/tests/Offstream.Core.Tests/Encoding/CoverArtIntegrationTests.cs b/tests/Offstream.Core.Tests/Encoding/CoverArtIntegrationTests.cs index 894de2e..ecf9a55 100644 --- a/tests/Offstream.Core.Tests/Encoding/CoverArtIntegrationTests.cs +++ b/tests/Offstream.Core.Tests/Encoding/CoverArtIntegrationTests.cs @@ -54,6 +54,46 @@ public async Task Encode_AttachesCoverArtInTheSamePass(MediaFormat format) Assert.Contains("attached_pic=1", streams, StringComparison.Ordinal); } + /// + /// The attached picture is typed as the front cover, and described. + /// + /// + /// + /// -disposition:v attached_pic does not settle this. It marks the stream as cover art + /// and leaves the picture type at 0 — Other — which is what every file written before + /// 2026-09-02 carries, and what makes software hunting for a front cover skip a file that + /// has one. The type comes from the stream's comment tag, which the muxer matches + /// against the spellings the format defines rather than storing verbatim. + /// + /// + /// Asserted through TagLib# rather than ffprobe because that is what a player sees, and + /// because ffprobe reports the type through the same comment key the argument uses, + /// so it would pass on a file where the string had been stored and never interpreted. + /// M4A is excluded: the mov muxer has nowhere to keep either field. + /// + /// + [Theory] + [InlineData(MediaFormat.Mp3)] + [InlineData(MediaFormat.Flac)] + public async Task Encode_TypesTheCoverArtAsTheFrontCover(MediaFormat format) + { + var source = await _workspace.CreateSourceWavAsync(); + var cover = await _workspace.CreateCoverArtAsync(); + var output = _workspace.PathFor(format, "typed-cover"); + + var outcome = await new AudioEncoder(_workspace.Runner).EncodeAsync( + new EncodeRequest(source, output, format, 192, SampleTrack(), cover)); + + Assert.False(outcome.HasWarning); + + using var tagged = TagLib.File.Create(output); + + var picture = Assert.Single(tagged.Tag.Pictures); + + Assert.Equal(TagLib.PictureType.FrontCover, picture.Type); + Assert.Equal("Album cover", picture.Description); + } + /// /// The §5.2 fallback. ffmpeg's METADATA_BLOCK_PICTURE support for Ogg is weak, /// so the profile routes Opus through TagLib# after the encode. The picture must survive diff --git a/tests/Offstream.Core.Tests/Encoding/FFmpegArgumentsTests.cs b/tests/Offstream.Core.Tests/Encoding/FFmpegArgumentsTests.cs index ef80602..d2dc027 100644 --- a/tests/Offstream.Core.Tests/Encoding/FFmpegArgumentsTests.cs +++ b/tests/Offstream.Core.Tests/Encoding/FFmpegArgumentsTests.cs @@ -122,12 +122,39 @@ public void CoverArt_ForMp3_AddsSecondInputAndExplicitMaps() "-i", @"C:\temp\cover.jpg", "-map", "0:a", "-map", "1:v", "-c:v", "mjpeg", "-disposition:v", "attached_pic", + "-metadata:s:v", "title=Album cover", + "-metadata:s:v", "comment=Cover (front)", "-c:a", "libmp3lame", "-b:a", "320k", "-id3v2_version", "3", @"C:\music\out.mp3", ], argv); } + /// + /// Every container that takes an attached picture also gets the picture's description. + /// + /// + /// The description is a separate field from the disposition, and it is the one a player + /// shows when it lists what pictures a file holds. Pinned for all three formats rather than + /// for MP3 alone, because it is written once in the shared branch and a format added later + /// picks it up silently — this is what says that is intended. + /// + [Theory] + [InlineData(MediaFormat.Mp3)] + [InlineData(MediaFormat.Flac)] + [InlineData(MediaFormat.Aac)] + public void CoverArt_NamesThePictureItAttaches(MediaFormat format) + { + var argv = FFmpegArguments.Build(Request(format, cover: @"C:\temp\cover.jpg")).ToList(); + + var disposition = argv.IndexOf("attached_pic"); + Assert.True(disposition >= 0); + + Assert.Equal( + ["-metadata:s:v", "title=Album cover", "-metadata:s:v", "comment=Cover (front)"], + argv.GetRange(disposition + 1, 4)); + } + /// /// The whole point of the ID3 version flag: it travels with the cover art. /// From d3a6f3844a56b827e956ba876e4bbe7331d25247 Mon Sep 17 00:00:00 2001 From: revtex Date: Wed, 2 Sep 2026 18:14:14 -0400 Subject: [PATCH 2/2] Delete the MP3 limits check, which ffmpeg has been answering on its own MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `WaveFormatExtensions.GetMp3Restrictions` reported which MP3 limits a capture format exceeded — more than two channels, or above 48 kHz — and nothing has ever called it. It came across with the ported suite because LAME had to be told: the reference implementation resampled and reduced channels by hand before handing it the buffer, so the answer had somewhere to go. ffmpeg does both unasked, and the method, its `Mp3Restriction` enum and its four tests have been sitting there answering a question nobody was still asking. Checked before deleting rather than after, because libmp3lame really does refuse more than two channels and really is capped at 48 kHz. Had ffmpeg passed those constraints through instead of resolving them, the dead guard would have been pointing at a live crash on any 5.1 or high-rate endpoint — which a WASAPI mix format can be. Encoding a 6-channel 48 kHz float32 WAV and a stereo 96 kHz one through the exact profiles this app ships: MP3 downmixes to stereo and resamples to 48 kHz, FLAC, AAC and Opus each keep all six channels, and every case exits zero. That last part is worth knowing for its own sake — a multichannel endpoint survives as multichannel in every format except MP3 — so it is recorded in the plan beside the deletion rather than lost with the code that hinted at it. No changelog entry: nothing called this, so nothing observable changes. Co-Authored-By: Claude Opus 5 --- docs/MODERNIZATION-PLAN.md | 27 +++++++++--- .../Audio/WaveFormatExtensions.cs | 29 ------------- .../Recording/RecordingEnums.cs | 7 --- .../Audio/WaveFormatExtensionsTests.cs | 43 ------------------- 4 files changed, 22 insertions(+), 84 deletions(-) delete mode 100644 src/Offstream.Core/Audio/WaveFormatExtensions.cs delete mode 100644 tests/Offstream.Core.Tests/Audio/WaveFormatExtensionsTests.cs diff --git a/docs/MODERNIZATION-PLAN.md b/docs/MODERNIZATION-PLAN.md index 3684bb0..473962d 100644 --- a/docs/MODERNIZATION-PLAN.md +++ b/docs/MODERNIZATION-PLAN.md @@ -373,7 +373,7 @@ Prove the risky parts survive the move before restructuring anything. | Enums (media format, provider, cover size, restrictions, policies) | ✅ ported | | `Text/StringExtensions`, `Text/EnumerableExtensions` | ✅ ported | | `Spotify/SpotifyWindowTitles` (idle/ad detection) | ✅ ported | -| `Audio/WaveFormatExtensions` (MP3 limits) | ✅ ported | +| ~~`Audio/WaveFormatExtensions` (MP3 limits)~~ | **Deleted 2026-09-02** — ported, never called, and ffmpeg makes the check unnecessary; see the encoding-profile finding | | `Naming/PathText` (diacritics, segment cleaning, tidy) | ✅ ported | | `Naming/FileNameTemplate` | ✅ ported, syntax byte-identical | | `Metadata/Track` | ✅ ported, with its own tests | @@ -1091,10 +1091,27 @@ always JPEG here: `CoverArtFetcher.TempFileFor` keeps a `.png` extension deliber `CoverArtWriter` derives the picture's MIME type from it, and `copy` would put a PNG into an APIC frame. The re-encode is normalisation, not waste. -One thing the audit turned up that is neither: `WaveFormatExtensions.GetMp3Restrictions` has no -caller outside its own tests. It is a LAME-era guard — ffmpeg resamples and downmixes without -being asked — kept because it came across with the ported suite. Worth deleting when something -else touches that file, not on its own. +One thing the audit turned up that is neither, and it is now gone: +`WaveFormatExtensions.GetMp3Restrictions` had no caller outside its own tests. It answered "which +MP3 limits does this capture format exceed" — more than two channels, or above 48 kHz — because +LAME had to be told, and the reference implementation resampled and reduced channels by hand +before handing it the buffer. ffmpeg does both unasked, so the answer had nowhere to go and the +method, its `Mp3Restriction` enum and its four ported tests are deleted. + +That was checked rather than assumed, because libmp3lame really does refuse more than two +channels and really is capped at 48 kHz — if ffmpeg had passed the constraint through instead of +resolving it, the dead guard would have been pointing at a live crash on any 5.1 or high-rate +endpoint. Encoding a 6-channel 48 kHz float32 WAV and a stereo 96 kHz one through the exact +profiles this app ships: + +| Input | MP3 | FLAC | AAC | Opus | +| --- | --- | --- | --- | --- | +| 5.1 @ 48 kHz | downmixed to stereo | 6 channels kept | 6 channels kept | 6 channels kept | +| stereo @ 96 kHz | resampled to 48 kHz | — | — | — | + +Every case exits 0. Worth knowing for its own sake: **a multichannel endpoint survives as +multichannel in every format except MP3**, which is a real difference between the formats on the +Settings page and not something the app says anywhere. --- diff --git a/src/Offstream.Core/Audio/WaveFormatExtensions.cs b/src/Offstream.Core/Audio/WaveFormatExtensions.cs deleted file mode 100644 index b95d12c..0000000 --- a/src/Offstream.Core/Audio/WaveFormatExtensions.cs +++ /dev/null @@ -1,29 +0,0 @@ -using NAudio.Wave; -using Offstream.Core.Recording; - -namespace Offstream.Core.Audio; - -/// Limits the MP3 encoder imposes on an input wave format. -public static class WaveFormatExtensions -{ - /// MP3 supports at most stereo. - public const int Mp3MaxChannels = 2; - - /// MP3 supports at most 48 kHz. - public const int Mp3MaxSampleRate = 48000; - - /// - /// Which MP3 limits exceeds; empty when it can be encoded as-is. - /// - public static IReadOnlyList GetMp3Restrictions(this WaveFormat waveFormat) - { - ArgumentNullException.ThrowIfNull(waveFormat); - - var restrictions = new List(); - - if (waveFormat.Channels > Mp3MaxChannels) restrictions.Add(Mp3Restriction.Channel); - if (waveFormat.SampleRate > Mp3MaxSampleRate) restrictions.Add(Mp3Restriction.SampleRate); - - return restrictions; - } -} diff --git a/src/Offstream.Core/Recording/RecordingEnums.cs b/src/Offstream.Core/Recording/RecordingEnums.cs index ecbac05..dadb6b3 100644 --- a/src/Offstream.Core/Recording/RecordingEnums.cs +++ b/src/Offstream.Core/Recording/RecordingEnums.cs @@ -62,13 +62,6 @@ public enum SilenceTrim TrimStart, } -/// Why a wave format cannot be encoded to MP3 without conversion. -public enum Mp3Restriction -{ - Channel, - SampleRate, -} - /// UI language. public enum LanguageType { diff --git a/tests/Offstream.Core.Tests/Audio/WaveFormatExtensionsTests.cs b/tests/Offstream.Core.Tests/Audio/WaveFormatExtensionsTests.cs deleted file mode 100644 index 8b8acdf..0000000 --- a/tests/Offstream.Core.Tests/Audio/WaveFormatExtensionsTests.cs +++ /dev/null @@ -1,43 +0,0 @@ -using NAudio.Wave; -using Offstream.Core.Audio; -using Offstream.Core.Recording; -using Xunit; - -namespace Offstream.Core.Tests.Audio; - -/// Ported from the reference suite's WaveFormatExtensionsTest. -public sealed class WaveFormatExtensionsTests -{ - [Fact] - public void ReportsChannelRestrictionAboveStereo() - { - var waveFormat = WaveFormat.CreateIeeeFloatWaveFormat(WaveFormatExtensions.Mp3MaxSampleRate, 6); - - Assert.Contains(Mp3Restriction.Channel, waveFormat.GetMp3Restrictions()); - } - - [Fact] - public void ReportsSampleRateRestrictionAbove48k() - { - var waveFormat = WaveFormat.CreateIeeeFloatWaveFormat(96000, WaveFormatExtensions.Mp3MaxChannels); - - Assert.Contains(Mp3Restriction.SampleRate, waveFormat.GetMp3Restrictions()); - } - - [Fact] - public void ReportsBothRestrictionsWhenBothExceeded() - { - var waveFormat = WaveFormat.CreateIeeeFloatWaveFormat(96000, 6); - - Assert.Equal([Mp3Restriction.Channel, Mp3Restriction.SampleRate], waveFormat.GetMp3Restrictions()); - } - - [Fact] - public void ReportsNoRestrictionsWithinLimits() - { - var waveFormat = WaveFormat.CreateIeeeFloatWaveFormat( - WaveFormatExtensions.Mp3MaxSampleRate, WaveFormatExtensions.Mp3MaxChannels); - - Assert.Empty(waveFormat.GetMp3Restrictions()); - } -}