-
Notifications
You must be signed in to change notification settings - Fork 2k
Android: constrain oversized image decodes to display bounds to prevent runtime bitmap draw crashes #35606
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Android: constrain oversized image decodes to display bounds to prevent runtime bitmap draw crashes #35606
Changes from all commits
17e8a5e
7a6690f
d647ccf
7702c62
fe546f5
8a0483e
f129408
efbb3c8
bcf3853
f93775c
a33b70b
a36a181
14b21e0
6e63183
fe7f429
424f89d
ecdab27
db0d955
54456ba
db059b8
117fdb3
bf0ade8
8b4f94f
e6dd195
383217e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -46,6 +46,7 @@ | |
| import com.bumptech.glide.RequestBuilder; | ||
| import com.bumptech.glide.RequestManager; | ||
| import com.bumptech.glide.load.engine.DiskCacheStrategy; | ||
| import com.bumptech.glide.load.resource.bitmap.DownsampleStrategy; | ||
| import com.bumptech.glide.request.target.Target; | ||
|
|
||
| import com.google.android.material.appbar.AppBarLayout; | ||
|
|
@@ -336,10 +337,177 @@ private static void load(RequestBuilder<Drawable> builder, Context context, bool | |
| prepare(builder, target, cachingEnabled, callback); | ||
| } | ||
|
|
||
| private static RequestBuilder<Drawable> limitToDisplaySize(RequestBuilder<Drawable> builder, Context context) { | ||
| if (context == null) { | ||
| return builder; | ||
| } | ||
|
|
||
| DisplayMetrics metrics = context.getResources().getDisplayMetrics(); | ||
| if (metrics == null) { | ||
| return builder; | ||
| } | ||
|
|
||
| int width = metrics.widthPixels; | ||
| int height = metrics.heightPixels; | ||
| if (width <= 0 || height <= 0) { | ||
| return builder; | ||
| } | ||
|
|
||
| return builder | ||
| .downsample(DownsampleStrategy.CENTER_INSIDE) | ||
| .override(width, height); | ||
| } | ||
|
|
||
| private static RequestBuilder<Drawable> limitToTargetSize(RequestBuilder<Drawable> builder, ImageView imageView) { | ||
| // Cap the decode so an oversized bitmap can never reach the canvas draw (the crash this fixes), | ||
| // choosing a downsample strategy that matches the view's ScaleType (see AspectExtensions.cs for | ||
| // the Aspect -> ScaleType mapping): | ||
| // * CENTER (Aspect.Center) draws the source 1:1 without scaling, so decoding it down to the view | ||
| // size would visibly change the result for any image larger than its view. Preserve native | ||
| // resolution but still guard against oversized bitmaps by capping the decode at the display | ||
| // size instead of the much smaller view size. | ||
| // * CENTER_CROP (Aspect.AspectFill) and FIT_XY (Aspect.Fill) fill the view on every axis, so a | ||
| // fit-inside decode would shrink an extreme-aspect source to its short axis and then upscale it | ||
| // to fill, losing detail. Cover the view instead, but clamp the decode so neither axis can ever | ||
| // exceed the display bounds (the crash invariant). See DisplayBoundedFillStrategy: it targets the | ||
| // covering ratio yet caps the scale at the display ratio and rounds toward the smaller bitmap, so | ||
| // the decode is provably <= display on both axes while staying as sharp as the display allows. | ||
| // * FitCenter (Aspect.AspectFit) and the default only need to fit WITHIN the view, so honor the | ||
| // view's own size when it declares one and fall back to an explicit display ceiling when the view | ||
| // is WRAP_CONTENT / not yet measured, otherwise Glide's target-size negotiation can still decode | ||
| // an extreme-aspect source above the display bounds. | ||
| ImageView.ScaleType scaleType = imageView.getScaleType(); | ||
| if (scaleType == ImageView.ScaleType.CENTER) { | ||
| return limitToDisplaySize(builder, imageView.getContext()); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
[major] Performance-Critical Path Optimization — |
||
| } | ||
|
|
||
| if (scaleType == ImageView.ScaleType.CENTER_CROP || scaleType == ImageView.ScaleType.FIT_XY) { | ||
| return limitToViewCoveringDisplaySize(builder, imageView); | ||
| } | ||
|
|
||
| return limitToViewOrDisplaySize(builder, imageView); | ||
| } | ||
|
|
||
| private static RequestBuilder<Drawable> limitToViewOrDisplaySize(RequestBuilder<Drawable> builder, ImageView imageView) { | ||
| // FitCenter/AspectFit (MAUI's default Android scale type) only has to fit the source inside the | ||
| // view. A WRAP_CONTENT or not-yet-measured ImageView, however, gives Glide no bounded target, so an | ||
| // extreme-aspect source can still decode above the display bounds and crash. Prefer the view's own | ||
| // fixed/measured size when it has one (so small thumbnails stay cheap) and fall back to the display | ||
| // size as a hard ceiling otherwise. | ||
| builder = builder.downsample(DownsampleStrategy.CENTER_INSIDE); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
[major] Android image decode sizing — |
||
|
|
||
| Context context = imageView.getContext(); | ||
| if (context == null) { | ||
| return builder; | ||
| } | ||
|
|
||
| DisplayMetrics metrics = context.getResources().getDisplayMetrics(); | ||
| if (metrics == null) { | ||
| return builder; | ||
| } | ||
|
|
||
| int displayWidth = metrics.widthPixels; | ||
| int displayHeight = metrics.heightPixels; | ||
| if (displayWidth <= 0 || displayHeight <= 0) { | ||
| return builder; | ||
| } | ||
|
|
||
| int targetWidth = boundedDimension(imageView, true, displayWidth); | ||
| int targetHeight = boundedDimension(imageView, false, displayHeight); | ||
| return builder.override(targetWidth, targetHeight); | ||
| } | ||
|
|
||
| private static int boundedDimension(ImageView imageView, boolean horizontal, int displayLimit) { | ||
| // Honor an already-measured or fixed positive dimension (clamped to the display so it can never | ||
| // exceed it); WRAP_CONTENT / MATCH_PARENT / unmeasured axes fall back to the display limit. | ||
| int measured = horizontal ? imageView.getWidth() : imageView.getHeight(); | ||
| if (measured > 0) { | ||
| return Math.min(measured, displayLimit); | ||
| } | ||
|
|
||
| ViewGroup.LayoutParams layoutParams = imageView.getLayoutParams(); | ||
| if (layoutParams != null) { | ||
| int declared = horizontal ? layoutParams.width : layoutParams.height; | ||
| if (declared > 0) { | ||
| return Math.min(declared, displayLimit); | ||
| } | ||
| } | ||
|
|
||
| return displayLimit; | ||
| } | ||
|
|
||
| private static RequestBuilder<Drawable> limitToViewCoveringDisplaySize(RequestBuilder<Drawable> builder, ImageView imageView) { | ||
| // CENTER_CROP (Aspect.AspectFill) and FIT_XY (Aspect.Fill) fill the view on every axis. Decoding | ||
| // fit-inside would shrink a wide/tall source to the view's short axis and then upscale it to fill, | ||
| // losing detail, so cover the view box instead. DisplayBoundedFillStrategy caps the covering scale at | ||
| // the display ratio and rounds toward the smaller bitmap, so the decode stays <= display on both axes | ||
| // (never re-opening the oversized-bitmap crash) while remaining as sharp as the display allows. | ||
| Context context = imageView.getContext(); | ||
| if (context == null) { | ||
| return builder.downsample(DownsampleStrategy.CENTER_INSIDE); | ||
| } | ||
|
|
||
| DisplayMetrics metrics = context.getResources().getDisplayMetrics(); | ||
| if (metrics == null) { | ||
| return builder.downsample(DownsampleStrategy.CENTER_INSIDE); | ||
| } | ||
|
|
||
| int displayWidth = metrics.widthPixels; | ||
| int displayHeight = metrics.heightPixels; | ||
| if (displayWidth <= 0 || displayHeight <= 0) { | ||
| return builder.downsample(DownsampleStrategy.CENTER_INSIDE); | ||
| } | ||
|
|
||
| int targetWidth = boundedDimension(imageView, true, displayWidth); | ||
| int targetHeight = boundedDimension(imageView, false, displayHeight); | ||
| return builder | ||
| .downsample(new DisplayBoundedFillStrategy(displayWidth, displayHeight)) | ||
| .override(targetWidth, targetHeight); | ||
| } | ||
|
|
||
| // A cover downsample strategy for CENTER_CROP / FIT_XY that can never decode above the display bounds. | ||
| // getScaleFactor targets the covering (larger) axis ratio for sharpness but clamps it to the display | ||
| // (smaller) axis ratio; MEMORY rounding keeps the decoded bitmap <= that display-capped target, so the | ||
| // decoded size is provably <= display on both axes. This is the crash invariant the whole cap enforces. | ||
| private static final class DisplayBoundedFillStrategy extends DownsampleStrategy { | ||
| private final int maxWidth; | ||
| private final int maxHeight; | ||
|
|
||
| DisplayBoundedFillStrategy(int maxWidth, int maxHeight) { | ||
| this.maxWidth = maxWidth; | ||
| this.maxHeight = maxHeight; | ||
| } | ||
|
|
||
| @Override | ||
| public float getScaleFactor(int sourceWidth, int sourceHeight, int requestedWidth, int requestedHeight) { | ||
| if (sourceWidth <= 0 || sourceHeight <= 0) { | ||
| return 1f; | ||
| } | ||
|
|
||
| // Cover the requested box: the larger of the two axis ratios. | ||
| float cover = Math.max( | ||
| (float) requestedWidth / sourceWidth, | ||
| (float) requestedHeight / sourceHeight); | ||
| // Never let either decoded axis exceed the display: the smaller of the two display ratios. | ||
| float displayCap = Math.min( | ||
| (float) maxWidth / sourceWidth, | ||
| (float) maxHeight / sourceHeight); | ||
| // Never upscale during decode; the ImageView matrix handles any remaining fill. | ||
| return Math.min(1f, Math.min(cover, displayCap)); | ||
| } | ||
|
|
||
| @Override | ||
| public SampleSizeRounding getSampleSizeRounding(int sourceWidth, int sourceHeight, int requestedWidth, int requestedHeight) { | ||
| // MEMORY keeps the decoded bitmap <= the display-capped target, preserving the crash invariant. | ||
| return SampleSizeRounding.MEMORY; | ||
| } | ||
| } | ||
|
|
||
| public static void loadImageFromFile(ImageView imageView, String file, ImageLoaderCallback callback) { | ||
| RequestBuilder<Drawable> builder = Glide | ||
| .with(imageView) | ||
| .load(file); | ||
| builder = limitToTargetSize(builder, imageView); | ||
| loadInto(builder, imageView, true, callback, file); | ||
| } | ||
|
|
||
|
|
@@ -352,17 +520,26 @@ public static void loadImageFromUri(ImageView imageView, String uri, boolean cac | |
| RequestBuilder<Drawable> builder = Glide | ||
| .with(imageView) | ||
| .load(androidUri); | ||
| builder = limitToTargetSize(builder, imageView); | ||
| loadInto(builder, imageView, cachingEnabled, callback, androidUri); | ||
| } | ||
|
|
||
| public static void loadImageFromStream(ImageView imageView, InputStream inputStream, ImageLoaderCallback callback) { | ||
| RequestBuilder<Drawable> builder = Glide | ||
| .with(imageView) | ||
| .load(inputStream) | ||
| .override(Target.SIZE_ORIGINAL, Target.SIZE_ORIGINAL); | ||
| .load(inputStream); | ||
| builder = limitToTargetSize(builder, imageView); | ||
| loadInto(builder, imageView, false, callback, inputStream); | ||
| } | ||
|
|
||
| public static void loadImageFromResource(ImageView imageView, int resourceId, ImageLoaderCallback callback) { | ||
| RequestBuilder<Drawable> builder = Glide | ||
| .with(imageView) | ||
| .load(resourceId); | ||
| builder = limitToTargetSize(builder, imageView); | ||
| loadInto(builder, imageView, true, callback, resourceId); | ||
| } | ||
|
|
||
| public static void loadImageFromFont(ImageView imageView, @ColorInt int color, String glyph, Typeface typeface, float textSize, ImageLoaderCallback callback) { | ||
| FontModel fontModel = new FontModel(color, glyph, textSize, typeface); | ||
| RequestBuilder<Drawable> builder = Glide | ||
|
|
@@ -380,9 +557,22 @@ public static void loadImageFromFile(Context context, String file, ImageLoaderCa | |
| RequestBuilder<Drawable> builder = Glide | ||
| .with(context) | ||
| .load(file); | ||
| builder = limitToDisplaySize(builder, context); | ||
| load(builder, context, true, callback, file); | ||
| } | ||
|
|
||
| public static void loadImageFromResource(Context context, int resourceId, ImageLoaderCallback callback) { | ||
| if (isContextDestroyed(context)) { | ||
| callback.onComplete(false, null, null); | ||
| return; | ||
| } | ||
| RequestBuilder<Drawable> builder = Glide | ||
| .with(context) | ||
| .load(resourceId); | ||
| builder = limitToDisplaySize(builder, context); | ||
| load(builder, context, true, callback, resourceId); | ||
| } | ||
|
|
||
| public static void loadImageFromUri(Context context, String uri, boolean cachingEnabled, ImageLoaderCallback callback) { | ||
| if (isContextDestroyed(context)) { | ||
| callback.onComplete(false, null, null); | ||
|
|
@@ -396,6 +586,7 @@ public static void loadImageFromUri(Context context, String uri, boolean caching | |
| RequestBuilder<Drawable> builder = Glide | ||
| .with(context) | ||
| .load(androidUri); | ||
| builder = limitToDisplaySize(builder, context); | ||
| load(builder, context, cachingEnabled, callback, androidUri); | ||
| } | ||
|
|
||
|
|
@@ -406,8 +597,8 @@ public static void loadImageFromStream(Context context, InputStream inputStream, | |
| } | ||
| RequestBuilder<Drawable> builder = Glide | ||
| .with(context) | ||
| .load(inputStream) | ||
| .override(Target.SIZE_ORIGINAL, Target.SIZE_ORIGINAL); | ||
| .load(inputStream); | ||
| builder = limitToDisplaySize(builder, context); | ||
| load(builder, context, false, callback, inputStream); | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,8 +27,9 @@ public partial class FileImageSourceService | |
| var id = imageView.Context?.GetDrawableId(file) ?? -1; | ||
| if (id > 0) | ||
| { | ||
| imageView.SetImageResource(id); | ||
| return Task.FromResult<IImageSourceServiceResult?>(new ImageSourceServiceLoadResult()); | ||
| var resourceCallback = new ImageLoaderCallback(); | ||
| PlatformInterop.LoadImageFromResource(imageView, id, resourceCallback); | ||
|
kubaflo marked this conversation as resolved.
Comment on lines
27
to
+31
|
||
| return resourceCallback.Result; | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -48,7 +49,7 @@ public partial class FileImageSourceService | |
| return Task.FromResult<IImageSourceServiceResult?>(null); | ||
| } | ||
|
|
||
| public override Task<IImageSourceServiceResult<Drawable>?> GetDrawableAsync(IImageSource imageSource, Context context, CancellationToken cancellationToken = default) | ||
| public override async Task<IImageSourceServiceResult<Drawable>?> GetDrawableAsync(IImageSource imageSource, Context context, CancellationToken cancellationToken = default) | ||
| { | ||
| var fileImageSource = (IFileImageSource)imageSource; | ||
| if (!fileImageSource.IsEmpty) | ||
|
|
@@ -62,17 +63,55 @@ public partial class FileImageSourceService | |
| var id = context?.GetDrawableId(file) ?? -1; | ||
| if (id > 0) | ||
| { | ||
| var d = context?.GetDrawable(id); | ||
| if (d is not null) | ||
| return Task.FromResult<IImageSourceServiceResult<Drawable>?>(new ImageSourceServiceResult(d)); | ||
| var resourceCallback = new ImageLoaderResultCallback(); | ||
| PlatformInterop.LoadImageFromResource(context, id, resourceCallback); | ||
|
|
||
| var result = await resourceCallback.Result.ConfigureAwait(false); | ||
|
|
||
| // The async Glide callback is not itself cancelable, so recheck the token before | ||
| // inspecting the result. A superseded (canceled) resource load must propagate | ||
| // cancellation even when the callback returned null, otherwise the non-ImageView | ||
| // consumer (ImageSourcePartExtensions.UpdateSourceAsync) treats null as a Glide | ||
| // failure and calls setImage(null), clearing the newer source it already applied. | ||
| // Dispose any drawable we are dropping and propagate cancellation so the caller's | ||
| // OperationCanceledException path leaves the newer image intact. | ||
| if (cancellationToken.IsCancellationRequested) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
[major] Async/Threading Safety & Regression Prevention — This new cancellation-recheck (throw |
||
| { | ||
| result?.Dispose(); | ||
| throw new OperationCanceledException(cancellationToken); | ||
| } | ||
|
|
||
| if (result is null) | ||
| { | ||
| Logger?.LogWarning("Unable to load image resource '{File}'.", file); | ||
| return null; | ||
| } | ||
|
|
||
| return result; | ||
| } | ||
| } | ||
|
|
||
| var callback = new ImageLoaderResultCallback(); | ||
|
|
||
| PlatformInterop.LoadImageFromFile(context, file, callback); | ||
|
|
||
| return callback.Result; | ||
| var fileResult = await callback.Result.ConfigureAwait(false); | ||
|
|
||
| // Same cancellation handling as the resource path above: propagate cancellation (disposing | ||
| // the dropped drawable) instead of returning a null that a superseded load would turn into | ||
| // setImage(null), clearing a newer source on the non-ImageView consumer. | ||
| if (cancellationToken.IsCancellationRequested) | ||
| { | ||
| fileResult?.Dispose(); | ||
| throw new OperationCanceledException(cancellationToken); | ||
| } | ||
|
|
||
| return fileResult; | ||
| } | ||
| catch (OperationCanceledException) | ||
| { | ||
| // Normal cancellation of a superseded load — propagate without logging it as a failure. | ||
| throw; | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
|
|
@@ -81,7 +120,7 @@ public partial class FileImageSourceService | |
| } | ||
| } | ||
|
|
||
| return Task.FromResult<IImageSourceServiceResult<Drawable>?>(null); | ||
| return null; | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[major] Performance-Critical Path Optimization — Routing
CENTER_CROPandFIT_XYthroughlimitToDisplaySize()forces.override(screenWidth, screenHeight)and bypasses Glide's ImageView target sizing. A small AspectFill/Fill image in a scrolling list (for example a 48dp thumbnail backed by a 4K photo) will now decode near screen size for every cell instead of the target/cover size, greatly increasing bitmap memory and scroll-path decode work. Use the target dimensions when available (FIT_XY can decode exactly to the target; CENTER_CROP can compute a cover size capped by display bounds) and fall back to display size only when the target size is unavailable or for CENTER's 1:1 semantics.