Add workaround for AOSP bug with decoding single channel hardware gai… #5357

falhassen · 2024-01-24T01:30:21Z

…nmaps using BitmapFactory.

Android U devices that use skiagl for hwui renderering (currently the vast majority) have a bug where uplading single channel bitmaps to hardware bitmaps fails. This bug essentially breaks single channel gainmap rendering using hardware bitmaps, which is a common use case on devices that have Ultra HDR as the default capture format.

The workaround is as follows:

Detect the presence of the bug by attempting to copy a 1x1 single channel bitmap to hardware.
Memoize the result
For BitmapFactory.decode* calls, if the input bitmap has a gainmap, copy it to a three-channel gainmap. This step seems wasteful, but in practice, the Android OS already does some copying when creating hardware bitmaps. Also, more importantly, there is an AOSP fix for this bug, and as devices are updated with this fix, this flow will no longer be exercised.

falhassen · 2024-01-24T01:34:19Z

I can try adding emulator tests, but I wanted to check on overall feedback with this approach before adding tests I might have to erase depending on reviewer feedback.

I will need to spend some time to see if the bug can be reproduced in emulators. I'm not sure if hardware bitmap flows are testable using emulators.

falhassen · 2024-01-24T18:30:28Z

I added a flag guard too, so this should not break anyone who doesn't opt-in into the bug fix.

sjudd · 2024-01-26T04:05:13Z

What if you turn off hardware Bitmaps for U until the fix goes out?

falhassen · 2024-01-26T04:19:30Z

I'm worried about the memory implications of doing that. If I understand correctly, we would double our bitmap memory allocation.

…

On Thu, Jan 25, 2024, 8:05 PM Sam ***@***.***> wrote: What if you turn off hardware Bitmaps for U until the fix goes out? — Reply to this email directly, view it on GitHub <#5357 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACSZ4GH75JQHB5RU4QFW52LYQMTQJAVCNFSM6AAAAABCH6GJ22VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMJRGQ2DOMZVG4> . You are receiving this because you authored the thread.Message ID: ***@***.***>

falhassen · 2024-01-26T04:20:45Z

Also, we will have to live with some devices being permanently in this state and I would prefer a workaround. As devidd upgrade to V, this will be a non-issue.

…

On Thu, Jan 25, 2024, 8:19 PM Fares Alhassen ***@***.***> wrote: I'm worried about the memory implications of doing that. If I understand correctly, we would double our bitmap memory allocation. On Thu, Jan 25, 2024, 8:05 PM Sam ***@***.***> wrote: > What if you turn off hardware Bitmaps for U until the fix goes out? > > — > Reply to this email directly, view it on GitHub > <#5357 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/ACSZ4GH75JQHB5RU4QFW52LYQMTQJAVCNFSM6AAAAABCH6GJ22VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMJRGQ2DOMZVG4> > . > You are receiving this because you authored the thread.Message ID: > ***@***.***> >

falhassen · 2024-01-26T04:22:12Z

Finally, I have a client workaround (using BitmapRegionDecoder, which doesn't use hardware bitmaps), but that fix has some UI drawbacks in our particular application.

…

On Thu, Jan 25, 2024, 8:20 PM Fares Alhassen ***@***.***> wrote: Also, we will have to live with some devices being permanently in this state and I would prefer a workaround. As devidd upgrade to V, this will be a non-issue. On Thu, Jan 25, 2024, 8:19 PM Fares Alhassen ***@***.***> wrote: > I'm worried about the memory implications of doing that. If I understand > correctly, we would double our bitmap memory allocation. > > On Thu, Jan 25, 2024, 8:05 PM Sam ***@***.***> wrote: > >> What if you turn off hardware Bitmaps for U until the fix goes out? >> >> — >> Reply to this email directly, view it on GitHub >> <#5357 (comment)>, >> or unsubscribe >> <https://github.com/notifications/unsubscribe-auth/ACSZ4GH75JQHB5RU4QFW52LYQMTQJAVCNFSM6AAAAABCH6GJ22VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMJRGQ2DOMZVG4> >> . >> You are receiving this because you authored the thread.Message ID: >> ***@***.***> >> >

falhassen · 2024-01-26T04:26:23Z

Final finally, I don't think we can expect all affected apps to turn off hardware bitmaps. It would be nice if Glide could just have the fix built-in.

…

On Thu, Jan 25, 2024, 8:21 PM Fares Alhassen ***@***.***> wrote: Finally, I have a client workaround (using BitmapRegionDecoder, which doesn't use hardware bitmaps), but that fix has some UI drawbacks in our particular application. On Thu, Jan 25, 2024, 8:20 PM Fares Alhassen ***@***.***> wrote: > Also, we will have to live with some devices being permanently in this > state and I would prefer a workaround. As devidd upgrade to V, this will be > a non-issue. > > On Thu, Jan 25, 2024, 8:19 PM Fares Alhassen ***@***.***> > wrote: > >> I'm worried about the memory implications of doing that. If I understand >> correctly, we would double our bitmap memory allocation. >> >> On Thu, Jan 25, 2024, 8:05 PM Sam ***@***.***> wrote: >> >>> What if you turn off hardware Bitmaps for U until the fix goes out? >>> >>> — >>> Reply to this email directly, view it on GitHub >>> <#5357 (comment)>, >>> or unsubscribe >>> <https://github.com/notifications/unsubscribe-auth/ACSZ4GH75JQHB5RU4QFW52LYQMTQJAVCNFSM6AAAAABCH6GJ22VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMJRGQ2DOMZVG4> >>> . >>> You are receiving this because you authored the thread.Message ID: >>> ***@***.***> >>> >>

jreck · 2024-01-26T14:40:48Z

library/src/main/java/com/bumptech/glide/load/resource/bitmap/ImageReader.java

+        softwareBitmap =
+            BitmapFactory.decodeByteArray(bytes, /* offset= */ 0, bytes.length, options);
+        if (softwareBitmap == null) {
+          return null;


This block of code is repeated a bunch, I think you could do something like this instead:

private static Bitmap safeDecodeHardwareBitmapWithGainmap(byte[] bytes, Options options) { try { options.inPreferredConfig = Bitmap.Config.ARGB_8888; Bitmap softwareBitmap = BitmapFactory.decodeByteArray(bytes, /* offset= */ 0, bytes.length, options); return safeDecodeBitmapWithGainmapCommon(softwareBitmap); } finally { options.inPreferredConfig = Config.HARDWARE; } } private static Bitmap safeDecodeBitmapWithGainmapCommon(Bitmap softwareBitmap) { if (softwareBitmap == null) { return null; } try { Gainmap gainmap = softwareBitmap.getGainmap(); if (gainmap != null) { Bitmap gainmapContents = gainmap.getGainmapContents(); if (gainmapContents.getConfig() == Bitmap.Config.ALPHA_8) { softwareBitmap.setGainmap( GainmapCopier.convertSingleChannelGainmapToTripleChannelGainmap(gainmap)); } } return softwareBitmap.copy(Bitmap.Config.HARDWARE, /* isMutable= */ false); } finally { softwareBitmap.recycle(); } }

Thanks for the tip. Done.

falhassen · 2024-01-26T21:58:53Z

I added emulator tests to DownsamplerEmulatorTest. Unfortunately, my emulated device doesn't seem to reproduce this framework issue, but at least they can guard against regressions. Certainly, the emulated device can reproduce the non-default exif orientation bug when I disable the fix.

kanelbulle · 2024-01-29T18:09:49Z

library/src/main/java/com/bumptech/glide/GlideBuilder.java

+   * Fixes decoding of hardware gainmaps from Ultra HDR images on Android U.
+   *
+   * <p>Without this flag on, gainmaps may be dropped when decoding Ultra HDR on Android U devices
+   * using skiagl for hwui..


I think you can say more words, what's the background and why would someone not want to turn this on? Should/will it be default?

Maybe just link to the source where this is described in detail?

kanelbulle · 2024-01-29T18:46:21Z

library/src/main/java/com/bumptech/glide/load/resource/bitmap/ImageReader.java

    }

    @Nullable
    @Override
    public Bitmap decodeBitmap(Options options) {
+      if (enableHardwareGainmapFixOnU


Instead of this repeated in all places we might decode a bitmap, can you create a GlideBitmapFactory wrapper where we can do this logic once? You kind of have it in AndroidUHardwareBitmapFactoryDecoder. Or is your plan to merge this when you are cleaning up the experiment?

Otherwise it seems easy to accidentally introduce another unguarded usage of BitmapDecoding and accidentally break something on Android U, ideally we'd discourage direct usage of BitmapFactory too.

kanelbulle · 2024-01-29T18:52:37Z

library/src/main/java/com/bumptech/glide/load/resource/bitmap/ImageReader.java

+        return false;
+      }
+      Boolean requiresGainmapDecodeWorkaround = REQUIRES_GAIN_MAP_FIX.get();
+      if (requiresGainmapDecodeWorkaround == null) {


Does it ever make sense to start this computation in multiple threads? I don't think it does, so this should probably be synchronzied and use double-checked locking?

Good catch. This is a bug. I fixed it (and added a @GuardedBy)

kanelbulle · 2024-01-29T18:58:09Z

library/src/main/java/com/bumptech/glide/load/resource/bitmap/ImageReader.java

+     * @param softwareBitmap The bitmap to be decoded. Must not be a hardware bitmap..
+     */
+    public static Bitmap safeDecodeBitmapWithGainmap(Bitmap softwareBitmap) {
+      if (softwareBitmap == null) {


this isn't valid input to this method? If it is it should be @Nullable

You're right. Removed.

kanelbulle · 2024-01-29T19:03:36Z

library/src/main/java/com/bumptech/glide/load/resource/bitmap/ImageReader.java

+        if (softwareBitmap != null) {
+          softwareBitmap.recycle();
+        }
+        options.inPreferredConfig = Config.HARDWARE;


nit: It took me a second to convince myself that you're just resetting it to what it was before, but I think it would be easier to reason about this method if you add an assertion at the start of it that the options are indeed set to HARDWARE.

…nmaps using BitmapFactory. Android U devices that use skiagl for hwui renderering (currently the vast majority) have a bug where uplading single channel bitmaps to hardware bitmaps fails. This bug essentially breaks single channel gainmap rendering using hardware bitmaps, which is a common use case on devices that have Ultra HDR as the default capture format. See bumptech#5357 for more details. The workaround is as follows: 1. Detect the presence of the bug by attempting to copy a 1x1 single channel bitmap to hardware. 2. Memoize the result 3. For BitmapFactory.decode* calls, if the input bitmap has a gainmap, copy it to a three-channel gainmap. This step seems wasteful, but in practice, the Android OS already does some copying when creating hardware bitmaps. Also, more importantly, there is an AOSP fix for this bug, and as devices are updated with this fix, this flow will no longer be exercised.

falhassen · 2024-01-30T01:37:01Z

I added emulator tests to DownsamplerEmulatorTest. Unfortunately, my emulated device doesn't seem to reproduce this framework issue, but at least they can guard against regressions. Certainly, the emulated device can reproduce the non-default exif orientation bug when I disable the fix.

Actually, I found a bug in my commit where I configured the gainmap to use ARGB_8888. To reproduce the bug, I need to configure the gainmap to use ALPHA_8. Once I do that, the emulator test will fail without the workaround.

…nmaps using BitmapFactory. Android U devices that use skiagl for hwui renderering (currently the vast majority) have a bug where uplading single channel bitmaps to hardware bitmaps fails. This bug essentially breaks single channel gainmap rendering using hardware bitmaps, which is a common use case on devices that have Ultra HDR as the default capture format. See bumptech#5357 for more details. The workaround is as follows: 1. Detect the presence of the bug by attempting to copy a 1x1 single channel bitmap to hardware. 2. Memoize the result 3. For BitmapFactory.decode* calls, if the input bitmap has a gainmap, copy it to a three-channel gainmap. This step seems wasteful, but in practice, the Android OS already does some copying when creating hardware bitmaps. Also, more importantly, there is an AOSP fix for this bug, and as devices are updated with this fix, this flow will no longer be exercised.

kanelbulle · 2024-01-31T18:09:45Z

library/src/main/java/com/bumptech/glide/load/resource/bitmap/GlideBitmapFactoryWrapper.java

+ * <p>New usages of {@link BitmapFactory} APIs within Glide should be added here rather than called
+ * directly.
+ */
+final class GlideBitmapFactoryWrapper {


nit: drop the Wrapper? it's cleaner

kanelbulle · 2024-01-31T18:13:52Z

library/src/main/java/com/bumptech/glide/load/resource/bitmap/GlideBitmapFactoryWrapper.java

+  private static Bitmap safeDecodeHardwareBitmapWithGainmap(
+      InputStream inputStream, Options options) {
+    Preconditions.checkArgument(options.inPreferredConfig == Config.HARDWARE);
+    options.inPreferredConfig = Bitmap.Config.ARGB_8888;


you're setting options twice (once here and once inside the try). It should be the last statement before the try {

Applies to this and all the methods below

Whoops thanks for catching this.

kanelbulle · 2024-01-31T18:20:40Z

library/src/main/java/com/bumptech/glide/load/resource/bitmap/GlideBitmapFactoryWrapper.java

+     *
+     * @param options which will be used to decode the gainmap.
+     */
+    static synchronized boolean needsGainmapDecodeWorkaround(Options options) {


I think you can be a bit more efficient with the locking, some of the preamble here doesn't have to be synchronized, and they don't need a shared lock until REQUIRES_GAIN_MAP_FIX is found to be null?

Not sure if it matters that much, but we do know these methods are invoked from 4 threads concurrently.

Done. Sorry this had violated the Effective Java rule of avoid excessive synchronization.

I switched over to using GlideSupplier, which is the Glide way of memoizing expensive compuatiohns.

…nmaps using BitmapFactory. Android U devices that use skiagl for hwui renderering (currently the vast majority) have a bug where uplading single channel bitmaps to hardware bitmaps fails. This bug essentially breaks single channel gainmap rendering using hardware bitmaps, which is a common use case on devices that have Ultra HDR as the default capture format. See bumptech#5357 for more details. The workaround is as follows: 1. Detect the presence of the bug by attempting to copy a 1x1 single channel bitmap to hardware. 2. Memoize the result 3. For BitmapFactory.decode* calls, if the input bitmap has a gainmap, copy it to a three-channel gainmap. This step seems wasteful, but in practice, the Android OS already does some copying when creating hardware bitmaps. Also, more importantly, there is an AOSP fix for this bug, and as devices are updated with this fix, this flow will no longer be exercised.

falhassen · 2024-01-31T21:13:36Z

Sorry, I think there is a bug in my last push. Will update once I fix...

kanelbulle · 2024-02-01T00:51:16Z

library/src/main/java/com/bumptech/glide/load/resource/bitmap/ImageReader.java

    }

    @Nullable
    @Override
    public Bitmap decodeBitmap(Options options) {
-      return BitmapFactory.decodeByteArray(bytes, /* offset= */ 0, bytes.length, options);
+      return enableHardwareGainmapFixOnU


The goal here and below is that once enableHardwareGainmapFixOnU is permanently shipped, we remove enableHardwareGainmapFixOnU and only call into GlideBitmapFactory, right?

Yes, exactly. We will clean it up as we would any Glide flag.

…nmaps using BitmapFactory. Android U devices that use skiagl for hwui renderering (currently the vast majority) have a bug where uplading single channel bitmaps to hardware bitmaps fails. This bug essentially breaks single channel gainmap rendering using hardware bitmaps, which is a common use case on devices that have Ultra HDR as the default capture format. See bumptech#5357 for more details. The workaround is as follows: 1. Detect the presence of the bug by attempting to copy a 1x1 single channel bitmap to hardware. 2. Memoize the result 3. For BitmapFactory.decode* calls, if the input bitmap has a gainmap, copy it to a three-channel gainmap. This step seems wasteful, but in practice, the Android OS already does some copying when creating hardware bitmaps. Also, more importantly, there is an AOSP fix for this bug, and as devices are updated with this fix, this flow will no longer be exercised.

falhassen · 2024-02-02T03:54:56Z

I fixed the issue. The bitmap.copy operation was setting all alpha values to zero. I copied the gainmap with a filter forcing the 100% opacity (which should be working-as-intended, since ALPHA_8 images only have a single channel).

falhassen force-pushed the master branch from 54d5941 to 5d47abc Compare January 26, 2024 02:12

jreck approved these changes Jan 26, 2024

View reviewed changes

falhassen force-pushed the master branch from 5d47abc to 32ef712 Compare January 26, 2024 18:26

jreck approved these changes Jan 26, 2024

View reviewed changes

falhassen force-pushed the master branch from 32ef712 to 8bc22a4 Compare January 26, 2024 21:56

kanelbulle reviewed Jan 29, 2024

View reviewed changes

falhassen force-pushed the master branch from 7f90a70 to 2b45470 Compare January 30, 2024 00:50

falhassen force-pushed the master branch from a833aaf to 3a11463 Compare January 30, 2024 01:09

falhassen force-pushed the master branch from 3a11463 to e4a9003 Compare January 30, 2024 01:35

falhassen force-pushed the master branch from e4a9003 to 89f1260 Compare January 30, 2024 23:58

falhassen force-pushed the master branch from 89f1260 to d764b91 Compare January 31, 2024 00:52

falhassen force-pushed the master branch from d764b91 to 0e3d3df Compare January 31, 2024 01:52

falhassen force-pushed the master branch from 0e3d3df to 58830c9 Compare January 31, 2024 02:12

kanelbulle reviewed Jan 31, 2024

View reviewed changes

falhassen force-pushed the master branch from 58830c9 to d918fc1 Compare January 31, 2024 20:48

jreck mentioned this pull request Jan 31, 2024

Consider adding workaround for U + gainmap + hardware bitmap bug coil-kt/coil#2094

Open

kanelbulle reviewed Feb 1, 2024

View reviewed changes

falhassen force-pushed the master branch from d918fc1 to 56a2c0c Compare February 2, 2024 03:51

kanelbulle merged commit 8ca2db2 into bumptech:master Feb 2, 2024
4 checks passed

falhassen mentioned this pull request Apr 22, 2024

Avoid referencing resource names within Resource Loader #5388

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add workaround for AOSP bug with decoding single channel hardware gai… #5357

Add workaround for AOSP bug with decoding single channel hardware gai… #5357

falhassen commented Jan 24, 2024

falhassen commented Jan 24, 2024 •

edited

Loading

falhassen commented Jan 24, 2024 •

edited

Loading

sjudd commented Jan 26, 2024

falhassen commented Jan 26, 2024 via email

falhassen commented Jan 26, 2024 via email

falhassen commented Jan 26, 2024 via email

falhassen commented Jan 26, 2024 via email

jreck Jan 26, 2024

falhassen Jan 26, 2024 •

edited

Loading

falhassen commented Jan 26, 2024 •

edited

Loading

kanelbulle Jan 29, 2024

falhassen Jan 30, 2024

kanelbulle Jan 29, 2024

falhassen Jan 30, 2024

kanelbulle Jan 29, 2024

falhassen Jan 30, 2024

kanelbulle Jan 29, 2024

falhassen Jan 30, 2024

kanelbulle Jan 29, 2024

falhassen Jan 30, 2024

falhassen commented Jan 30, 2024

kanelbulle Jan 31, 2024

falhassen Jan 31, 2024

kanelbulle Jan 31, 2024

falhassen Jan 31, 2024

kanelbulle Jan 31, 2024

falhassen Jan 31, 2024

falhassen commented Jan 31, 2024

kanelbulle Feb 1, 2024

falhassen Feb 1, 2024

falhassen commented Feb 2, 2024

Add workaround for AOSP bug with decoding single channel hardware gai… #5357

Add workaround for AOSP bug with decoding single channel hardware gai… #5357

Conversation

falhassen commented Jan 24, 2024

falhassen commented Jan 24, 2024 • edited Loading

falhassen commented Jan 24, 2024 • edited Loading

sjudd commented Jan 26, 2024

falhassen commented Jan 26, 2024 via email

falhassen commented Jan 26, 2024 via email

falhassen commented Jan 26, 2024 via email

falhassen commented Jan 26, 2024 via email

Choose a reason for hiding this comment

falhassen Jan 26, 2024 • edited Loading

Choose a reason for hiding this comment

falhassen commented Jan 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

falhassen commented Jan 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

falhassen commented Jan 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

falhassen commented Feb 2, 2024

falhassen commented Jan 24, 2024 •

edited

Loading

falhassen commented Jan 24, 2024 •

edited

Loading

falhassen Jan 26, 2024 •

edited

Loading

falhassen commented Jan 26, 2024 •

edited

Loading