Skip to content

Fixes large images crashing when opened in timeline #6290

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

Merged
merged 3 commits into from
Jun 16, 2022

Conversation

ericdecanini
Copy link
Contributor

@ericdecanini ericdecanini commented Jun 13, 2022

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

Fixes large images crashing when opened in the timeline

Motivation and context

Closes #2642
Also closes #1951
Also closes #6190
Also closes https://github.com./matrix-org/internal-config/issues/1184

Error:
java.lang.RuntimeException: Canvas: trying to draw too large(111132000bytes) bitmap.

Caused when the image is rendered in AttachmentViewerActivity. We were overriding the size to scale the bitmap to be much larger than it already is. Upon investigation, this provides no benefit in increasing the quality of the opened image.

An onlyRetrieveFromCache line was removed to as this prevented the image transition from happening and seems to give little benefit to performance.

Some unused functions and old comments were also removed

Other checks performed:

  • Image is still downloaded in full size

Screenshots / GIFs

device-2022-06-13-120754.mp4

(app used to crash when clicking the image on the timeline)

Tests

  • Send a large image to yourself (ideally over 10MB, however crashes were observed with images over 1MB)
  • Open it in the timeline
  • See no crash and that image opens up smoothly with transition
  • See smaller sized images open as normal

Tested devices

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

Checklist

@ericdecanini ericdecanini added the PR-Small PR with less than 20 updated lines label Jun 13, 2022
@ericdecanini ericdecanini marked this pull request as ready for review June 13, 2022 10:11
@ericdecanini ericdecanini requested review from fedrunov, a team and yostyle and removed request for a team June 13, 2022 10:12
})
.fitCenter()
.into(imageView)
req.fitCenter().into(target)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to double check, this line is the fix? (everything else is removed unused code)

it's interesting that we were overriding the size to be non scaled instead of the default behaviour of scaling to match the View

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed it was. I think the idea is full images get displayed with their full size, but this really doesn't have any real use since we have a limited view space and a limited zoom. The important thing is that they can still download the full image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I would have kept the previous formatting, better for git diff, or future change, but this is really a minor remark

req
    .fitCenter()
    .into(target)

)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to make the change 👍

Copy link
Contributor

@ouchadam ouchadam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great catch and clean up! 💯

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.

LGTM, thanks.

})
.fitCenter()
.into(imageView)
req.fitCenter().into(target)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I would have kept the previous formatting, better for git diff, or future change, but this is really a minor remark

req
    .fitCenter()
    .into(target)

)

@bmarty
Copy link
Member

bmarty commented Jun 13, 2022

Does it also close #1951 ?

@ericdecanini
Copy link
Contributor Author

I performed the steps to confirm this would close https://github.com./matrix-org/internal-config/issues/1184:

  • Create a private encrypted room
  • Invite other user
  • Other user sends large image
  • I can open the image with no crash

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

Copy link
Contributor

@yostyle yostyle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Copy link
Contributor

@fedrunov fedrunov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@yostyle
Copy link
Contributor

yostyle commented Aug 9, 2022

Closes #3603

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR-Small PR with less than 20 updated lines
Projects
None yet
5 participants