fix(desktop): decode artwork for cover, not for one axis (#626) - #627
Draft
TheZupZup wants to merge 1 commit into
Draft
fix(desktop): decode artwork for cover, not for one axis (#626)#627TheZupZup wants to merge 1 commit into
TheZupZup wants to merge 1 commit into
Conversation
#457 bounded artwork decode to the box, which was right, but passed the bound as cacheWidth only. Every artwork surface draws with BoxFit.cover, and cover is satisfied by the image's shorter side, so a landscape source came back short on the covering axis and was scaled back up: a 2:1 cover in a 48 px avatar decoded 48x24 and was enlarged to 96x48. Visibly soft, and worse for artist press shots than for album art. ResizeImage cannot express cover. Given one axis it derives the other from the aspect ratio; given both it either stretches (exact) or contains (fit). None of those guarantees min(decodedW, decodedH) >= target. The decode callback is handed the source's intrinsic size before the codec is instantiated, which is the piece ResizeImage's public API cannot use for this. CoverResizeImage scales by extent / min(srcW, srcH), so a square source decodes exactly as before (no memory regression for the common case) and a non-square one reaches the target on the axis that matters. Never upscales: a source already at or below the target on its shorter side is decoded untouched rather than inflated in the cache. Cache identity is the part that fails silently rather than loudly, so CoverResizeImageKey carries the wrapped provider's key and the extent, and has its own tests: same cover same extent is one entry, same cover two extents is two, two covers one extent never collide. flutter test: 5072 passing. analyze, format and the security surface scan clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QLDN6yzLpSdXoFHYJNqyyp
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.
Closes #626. Fixes a regression I introduced in #612, caught by Codex review there.
The regression
#612 bounded artwork decode to the box a cover is drawn into. That was the right fix for the real defect, but it passed the bound as
cacheWidthonly. Every artwork surface draws withBoxFit.cover, and cover is satisfied by the image's shorter side, so a landscape source came back short on the axis that mattered and was scaled back up:Worse for artist images than album art: press shots are routinely landscape, covers are usually square.
Why not just pass both axes
ResizeImagecannot express cover, and I checked the SDK rather than assuming:exactgives exactly WxH "regardless of whether it matches the source image's intrinsic aspect ratio... similar toBoxFit.fill". A squashed cover is worse than a soft one.fitis "conceptually similar toBoxFit.contain", so the longer side lands on the target. For landscape that is identical to today.Nothing there guarantees
min(decodedW, decodedH) >= target.I also rejected two cheap workarounds: a blanket 2x cover allowance quadruples pixels for every square cover in a library grid, and dropping the per-box bound re-creates the defect #612 fixed.
The fix
The decode callback is handed the source's intrinsic size before the codec is instantiated, which is the piece
ResizeImage's public API cannot use for this. SoCoverResizeImagepicks the scale from the shorter side:A square source decodes exactly as it did before, so the common case costs no extra memory. A non-square one reaches the target on the covering axis.
It never upscales: a source already at or below the target on its shorter side is left alone rather than inflated in the image cache to look exactly as soft as letting the GPU scale it.
Cache identity
This is the part that fails silently rather than loudly. Get it wrong and the image cache quietly stops working, or serves one cover for another.
CoverResizeImageKeycarries the wrapped provider's key and the extent, with tests for all three cases:obtainKeymirrorsResizeImage's synchronous-completion dance, because forcing it through aCompleterwould cost every artwork render a frame.What was validated
Locally against a real Flutter 3.44.7 SDK:
flutter test5072 passing, whole suiteflutter analyzeclean,dart format --set-exit-if-changed lib testcleanpython3 scripts/check_pr_security_surface.pyclean (ordinary-risk diff)scripts/check_secrets.shcleanThe orientation tests are regression tests by construction: the landscape case asserts a decoded height of 48 for a 1000x500 source, and the old single-axis behaviour produced 24.
Coverage added: landscape, portrait and square sources; a sweep over seven aspect ratios asserting the shorter side always reaches the target; no-upscale; degenerate sizes (zero and negative) not dividing by zero; and an extreme 10000x96 ratio where neither side may collapse to zero.
🤖 Generated with Claude Code
https://claude.ai/code/session_01QLDN6yzLpSdXoFHYJNqyyp
Generated by Claude Code