-
Notifications
You must be signed in to change notification settings - Fork 749
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
[Bugfix] Fix crash on previewing image to upload on Android P #7184
Conversation
Using hardware bitmap allocation on Android framework versions prior to Android Q causes a crash when decoding a bitmap if GL context wasn't initialised. The issue is not documented in ImageDecoder reference but it is mentioned in the comments of glide[1] with a link to internal google discussion. [1] https://github.com/bumptech/glide/blob/f83cc274b42cf02af6611d2a8c21e4a9b1f5d592/library/src/main/java/com/bumptech/glide/load/resource/bitmap/HardwareConfigState.java#L22 Signed-off-by: Paweł Matuszewski <pamat@protonmail.com>
be52209
to
e5e6444
Compare
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.
Thanks for the fix and the Pull Request!
LGTM, I have triggered the CI to check the code.
val listener = ImageDecoder.OnHeaderDecodedListener { decoder, _, _ -> | ||
if (Build.VERSION.SDK_INT < Build.VERSION_CODES.Q) { | ||
// Allocating hardware bitmap may cause a crash on framework versions prior to Android Q | ||
decoder.setAllocator(ImageDecoder.ALLOCATOR_SOFTWARE) |
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.
There is a compilation warning, it could be replaced by
decoder.setAllocator(ImageDecoder.ALLOCATOR_SOFTWARE) | |
decoder.allocator = ImageDecoder.ALLOCATOR_SOFTWARE |
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.
done
ImageDecoder.decodeBitmap(ImageDecoder.createSource(context.contentResolver, uri)) | ||
val source = ImageDecoder.createSource(context.contentResolver, uri) | ||
val listener = ImageDecoder.OnHeaderDecodedListener { decoder, _, _ -> | ||
if (Build.VERSION.SDK_INT < Build.VERSION_CODES.Q) { |
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.
I think in this case, it could be replaced by
if (Build.VERSION.SDK_INT < Build.VERSION_CODES.Q) { | |
if (Build.VERSION.SDK_INT == Build.VERSION_CODES.P) { |
But I understand that it's more logic to keep the code like that.
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.
done, I assumed there might be something in between, like there is with O_MR1 but there indeed isn't.
I can rebase and squash the commits it it's ready to be merged |
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.
Thanks for the update. I will squash the commits when merging the PR.
Type of change
Closes #1404, closes #1851, closes #4545
Content
Use software bitmap allocation when previewing images to upload on devices with Android Pie.
Motivation and context
Using hardware bitmap allocation on Android framework versions prior to Android Q causes a crash when decoding a bitmap if GL context wasn't initialised. The issue is not documented in ImageDecoder reference but it is mentioned in the comments of glide[1] with a link to internal google discussion.
[1] https://github.com/bumptech/glide/blob/f83cc274b42cf02af6611d2a8c21e4a9b1f5d592/library/src/main/java/com/bumptech/glide/load/resource/bitmap/HardwareConfigState.java#L22
Screenshots / GIFs
Tests
Sending images from within Element
Sharing images to Element from external app
Tested devices
Checklist