Repository navigation
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 22 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,109 @@ 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_CROP (Aspect.AspectFill) and FIT_XY (Aspect.Fill) fill the view on every axis, so a | ||
| // COVER decode (CENTER_OUTSIDE) would keep the long axis of an extreme-aspect source above the | ||
| // display bounds (e.g. a 4*displayWidth x displayHeight source still "covers" a display-sized | ||
| // view at ~4*displayWidth). Cover quality cannot take priority over the safety invariant for a | ||
| // crash fix, so cap these at the display size too; the ImageView matrix still crops/scales the | ||
| // bounded bitmap to fill the view. | ||
| // * 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. | ||
| // * FitCenter (Aspect.AspectFit) and the default only need to fit WITHIN the view, so honor the | ||
| // view's own size when it declares one (keeps small/thumbnail loads cheap) but still 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 | ||
| || scaleType == ImageView.ScaleType.CENTER_CROP | ||
| || scaleType == ImageView.ScaleType.FIT_XY) { | ||
| 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 — |
||
| } | ||
|
|
||
| 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; | ||
| } | ||
|
|
||
| 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 +452,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 +489,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 +518,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 +529,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.
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,53 @@ 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); | ||
| if (result is null) | ||
|
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 and Threading Safety / Logic and Correctness — The resource branch returns
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 and Threading Safety — The resource path handles a null Glide result before rechecking |
||
| { | ||
| Logger?.LogWarning("Unable to load image resource '{File}'.", file); | ||
| return null; | ||
| } | ||
|
|
||
| // The async Glide callback is not itself cancelable, so recheck the token before | ||
| // returning. A superseded (canceled) resource load must NOT surface as a null result: | ||
| // the non-ImageView consumer (ImageSourcePartExtensions.UpdateSourceAsync) treats null as | ||
| // a Glide failure and calls setImage(null), clearing the newer source it already applied. | ||
| // Dispose the now-unneeded drawable 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); | ||
| } | ||
|
|
||
| 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 +118,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.