Stop Glide from upscaling favicons during decode#8712
Open
LukasPaczos wants to merge 1 commit into
Open
Conversation
Glide's default CENTER_OUTSIDE downsample strategy was inflating small favicons (e.g. 32x32) up to MAX_FAVICON_SIZE_PX (512x512) before they got encoded to disk, breaking FaviconPersister's size-based dedup (width > existing skips, but every URL-fetched favicon now had width=512 regardless of source) and the iconReceived pre-check added in #8156 (which rejected real higher-res bitmaps because the on-disk file was reported as 512). Regression from #7710, which added the 512 target without specifying a downsample strategy. Switch to CENTER_INSIDE on the Glide loads in GlideFaviconDownloader: keeps the 512 px memory ceiling that #7710 wanted (large OOM-class favicons still get downsampled), but never scales smaller sources up. Centralise the shared Glide config (downsample, diskCacheStrategy, skipMemoryCache) behind a faviconRequest() helper so the policy lives in one place.
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
2 participants
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.
Task/Issue URL: https://app.asana.com/1/137249556945/project/1208671518894266/task/1214789916598888?focus=true
Description
Glide's default
CENTER_OUTSIDEdownsample strategy was inflating small favicons (e.g. 32x32) up toMAX_FAVICON_SIZE_PX(512x512) before they got encoded to disk, breakingFaviconPersister's size-based dedup (width > existingskips, but every URL-fetched favicon now had width=512 regardless of source) and theiconReceivedpre-check added in #8156 (which rejected real higher-res bitmaps because the on-disk file was reported as 512). Regression from #7710, which added the 512 target without specifying a downsample strategy.Switch to
CENTER_INSIDEon the Glide loads inGlideFaviconDownloader: keeps the 512 px memory ceiling that #7710 wanted (large OOM-class favicons still get downsampled), but never scales smaller sources up.Steps to test this PR
Note
Low Risk
Scoped favicon decode/persist behavior with a remote kill switch; no auth or sensitive data paths.
Overview
Fixes favicons being decoded at a fake 512×512 size by routing all
GlideFaviconDownloaderloads through a sharedfaviconRequest()that appliesDownsampleStrategy.CENTER_INSIDEwhen the new remote flag is on (default). That keeps the existing 512px cap for large icons but stops Glide from upscaling small sources before they are written to disk, soFaviconPersistercan again compare real dimensions and higher-res icons can win in the tab switcher.Adds
FaviconDownloaderGlideDownsamplingStrategyFixFeatureas a kill switch (toggle off restores Glide’s prior downsampling) and wires it throughFaviconModuleintoGlideFaviconDownloader.Reviewed by Cursor Bugbot for commit 220bdd3. Bugbot is set up for automated code reviews on this repo. Configure here.