Update TransformationUtils#rotateImageExif to preserve gainmap and co… #5334
Conversation
|
I need to also bump the Robolectric version to use API 34 in tests. |
library/src/main/java/com/bumptech/glide/load/resource/bitmap/TransformationUtils.java
Outdated
Show resolved
Hide resolved
|
I think it would be better if BitmapPool is just removed universally. This would need to be done behind a Glide flag. That would solve the issue and also simplify Glide. |
| Bitmap result; | ||
| if (VERSION.SDK_INT >= VERSION_CODES.UPSIDE_DOWN_CAKE) { | ||
| // BitmapPool doesn't support gainmaps (and colorspace), so use Bitmap.createBitmap to apply | ||
| // the matrix. Bitmap.createBitmap will reuse bitmaps if needed on M+. We could also make this |
There was a problem hiding this comment.
I don't think this comment is accurate, Bitmap.createBItmap will only return the original Bitmap if it thinks there's no requested transformation. Whereas we normally say "reuse" as in "modify and reuse"
There was a problem hiding this comment.
Thanks for the correction. I have removed this comment and did not mention "reuse" in the BitmapCopier class where the new logic lives.
library/src/main/java/com/bumptech/glide/load/resource/bitmap/TransformationUtils.java
Show resolved
Hide resolved
|
I don't have any objection to submitting this as is if it's helpful. |
|
Note that I also took out the version guard. Users of the flag can decide which sdk versions to test this bug fix against. |
| import android.graphics.Bitmap; | ||
| import android.graphics.Matrix; | ||
| import androidx.annotation.Nullable; | ||
| /** |
There was a problem hiding this comment.
please can you run this file through google-java-format, the whitespace in the header is not consistent
| * associated gainmap pixels if non-null. | ||
| * | ||
| */ | ||
| public static Bitmap copy(Bitmap inBitmap, @Nullable Matrix matrix) { |
There was a problem hiding this comment.
this is an awful lot of boilerplate for a simple proxy method, what is the point in this?
There was a problem hiding this comment.
I was addressing this comment: #5334 (comment). Quoting here:
"I'm totally on board with ditching the BitmapPooll. But you're going to need to write this in more places (at least all other Transformations in this class) to have it work consistently, especially for local images.
Might be worth thinking through earlier rather than later how we'd go about it. A static helper? A BitmapHelper interface that is like the BitmapPool but only ever provides new instances?"
The idea is that we would use this class to replace other usages of BitmapPool. Of course, I could just remove the boilerplate as you suggest, but I would need to put all the javadoc somewhere.
There was a problem hiding this comment.
If we're planning on removing the bitmap pool entirely you don't need this documentation at all, you'd just remove it and replace it with Bitmap.createBitmap calls right?
There was a problem hiding this comment.
I removed the boilerplate.
| * @param exifOrientation the exif orientation [1-8]. | ||
| * @param preserveGainmapAndColorSpace whether to preserve gainmap and color space information | ||
| * post-transformation. | ||
| * @return The rotated and/or flipped image or toOrient if no rotation or flip was necessary. |
There was a problem hiding this comment.
Done. Also removed the documentation mentioned below.
| * associated gainmap pixels if non-null. | ||
| * | ||
| */ | ||
| public static Bitmap copy(Bitmap inBitmap, @Nullable Matrix matrix) { |
There was a problem hiding this comment.
If we're planning on removing the bitmap pool entirely you don't need this documentation at all, you'd just remove it and replace it with Bitmap.createBitmap calls right?
…lorspace on Android U+. Also bump Robolectric Version to support testing on Android U.
…lorspace on Android U+
This is only done on Android U+ to minimize risk with not using BitmapPool anymore when an image is rotated due to an EXIF orientation metadata.
I was going to add a unit test for gainmaps too, but unfortunately, there needs to be one more change to Robolectric's ShadowGainmap that will allow us to exercise the flows on Android U. My own testing with the required change in Robolectric shows that this change will preserve gainmaps.
fix #5333