Type the cover art as the front cover, settle three profile differences, and delete a guard ffmpeg makes unnecessary - #53
Merged
Conversation
…rences it should not adopt 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 <noreply@anthropic.com>
`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 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Audited the encoding profiles flag by flag against another Windows Spotify recorder's, on the
working assumption that any difference was a gap in ours. One was. 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, since each reads from the outside as an obvious improvement
nobody has got round to.
The gap: every cover Offstream has written is filed as "Other"
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_picmarks the stream as artwork and settles nothing else, leaving the type at 0, which both ID3's
APICframe and FLAC'sMETADATA_BLOCK_PICTUREspellOther. Software that goes looking for afront cover specifically then 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:varguments fix it, and neither is the free text it resembles:comment=Cover (front)Otherwithout complaint.title=Album coverEncoding with
comment=Sleeve testproduced a file reportingOther, with that string nowherein its bytes. That matters for how this is tested: ffprobe cannot tell the two cases apart,
because it reports the type back through the same
commentkey the argument uses, so a filewhere the string was stored verbatim and one where it was interpreted read identically. The new
integration test asserts through TagLib# instead — the way a player sees it — and encoding with
each argument alone, then searching the output bytes, is what separated them in the first place.
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. 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 that stay differences
MP3 keeps constant bitrate. The other ladder passes
-abr 1at every rung and reserves plain-b:a 320kfor a top rung of its own — so Offstream's default is that top rung, and its outputis already the best MP3 that ladder can produce. 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 on purpose. ABR is a real win at 96 and 128 kbps, so it is worth reopening if those
rungs turn out to be the ones people use.
WAV keeps
-c:a pcm_s16le. §5.1 used to offer a stream copy "when the temp already matches",and the other implementation does exactly that. The temp never matches:
WasapiLoopbackCapturereports the endpoint's shared-mode mix format, which on Windows is 32-bit float — confirmed with
ffprobe,
codec_name=pcm_f32le. Copying it would make the WAV output float32, twice the size, inthe one format people pick precisely because they are handing it to something else. That note in
the plan is now struck.
Cover art keeps
-c:v mjpegrather than a stream copy of the downloaded bytes.CoverArtFetcher.TempFileForpreserves a.pngextension deliberately, becauseCoverArtWriterreads 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.
And one deletion the audit turned up
WaveFormatExtensions.GetMp3Restrictionsreported 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. ffmpeg does both unasked. The method, its
Mp3Restrictionenum and its four tests are gone.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:
Every case exits zero. The right-hand columns are worth keeping for their own sake: a
multichannel endpoint survives as multichannel in every format except MP3, which is a real
difference between the entries in the format picker that the app does not mention anywhere. That
is recorded in the plan beside the deletion rather than lost with the code that hinted at it.
No changelog entry for this half — nothing called it, so nothing observable changes.
Verification
.\build.ps1 -Test— 1,286 pass, 0 fail (271 UI + 1,015 Core), 0 warnings. 5 new, 4 deleted with the guard they covered.Encode_TypesTheCoverArtAsTheFrontCoverreally guards: with the two arguments removed, both itscases fail.
that nothing leaked to a file-level
commenttag —format_tagsshows onlyencoder, andthe raw frames show an
APICwith noCOMMbeside it. That mattered: a comment the recorderwrites and the Metadata page cannot reach is the exact failure the editable-tag rule exists to
prevent.
Docs:
CHANGELOG.md, and a plan finding under §5 recording the four-way split. No README change —the picture's type is invisible in Offstream's own UI, and the README's promise about the sleeve
is unchanged.