Skip to content
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

Merged
merged 2 commits into from
Oct 4, 2022

Conversation

atpamat
Copy link
Contributor

@atpamat atpamat commented Sep 20, 2022

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

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

  1. Sending images from within Element

    • select "send images and videos" from attachment picker
    • select image that caused a native crash on develop branch
  2. Sharing images to Element from external app

    • share an image from gallery app to Element

Tested devices

  • Physical
  • Emulator
  • OS version(s): Android 9

Checklist

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>
@bmarty bmarty added the Z-Community-PR Issue is solved by a community member's PR label Sep 21, 2022
Copy link
Member

@bmarty bmarty left a 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)
Copy link
Member

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

Suggested change
decoder.setAllocator(ImageDecoder.ALLOCATOR_SOFTWARE)
decoder.allocator = ImageDecoder.ALLOCATOR_SOFTWARE

Copy link
Contributor Author

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) {
Copy link
Member

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

Suggested change
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.

Copy link
Contributor Author

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.

@atpamat
Copy link
Contributor Author

atpamat commented Sep 21, 2022

I can rebase and squash the commits it it's ready to be merged

Copy link
Member

@bmarty bmarty left a 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.

@bmarty bmarty enabled auto-merge (squash) September 22, 2022 19:30
@bmarty bmarty disabled auto-merge October 4, 2022 13:33
@bmarty bmarty merged commit d205202 into element-hq:develop Oct 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Z-Community-PR Issue is solved by a community member's PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't share photos Cannot send images Camera Images / Photo Upload Fails
2 participants