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

Fix animated images only autoplaying sometimes #6215

Conversation

networkException
Copy link
Contributor

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other:

Content

This pull request fixes autoplaying animated images only actually autoplaying in some cases as only thumbnail data would be fetched in the other cases.

It introduces a new ImageContentRenderer mode specifically for animated images with thumbnail sizing that always resolves to the actual image url just like stickers for example

The autoplaying feature was originally introduced in #6166

Motivation and context

This issue was brought up in the element-android matrix channel after release 1.4.18

Screenshots / GIFs

Before After
before after

Tests

  • Send an animated image in a public room
  • Check that the image actually autoplays instead of just showing the thumbnail

Tested devices

  • Physical
  • Emulator
  • OS version(s): Android 12 x86_64, Android 5 x86_64

Checklist

networkException added a commit to networkException/element-android that referenced this pull request May 31, 2022
@networkException networkException force-pushed the fix-animated-only-fetching-thumbnail branch from d74b925 to 9654c91 Compare May 31, 2022 20:49
@ouchadam ouchadam added the Z-Community-PR Issue is solved by a community member's PR label Jun 1, 2022
@bmarty bmarty requested review from a team, ouchadam and bmarty and removed request for a team June 8, 2022 11:12
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 for the PR!
2 non-blocking remarks.

networkException added a commit to networkException/element-android that referenced this pull request Jun 9, 2022
@networkException networkException force-pushed the fix-animated-only-fetching-thumbnail branch from 9654c91 to 0c13ed8 Compare June 9, 2022 12:09
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.

thanks for the update 💯

networkException added a commit to networkException/element-android that referenced this pull request Jun 15, 2022
@networkException networkException force-pushed the fix-animated-only-fetching-thumbnail branch from 0c13ed8 to 18eed00 Compare June 15, 2022 14:09
@networkException
Copy link
Contributor Author

@networkException
Copy link
Contributor Author

Is there anything blocking this from getting merged?

@ryanpeters-MSFT
Copy link

Is there anything blocking this from getting merged?

Came here to ask this as well. Would love for this to be merged!

This patch introduces a new `ImageContentRenderer` mode used for
autoplaying animated images. The mode shares url resolving semantics
with `FULL_SIZE` and `STICKER`, as such not just fetching thumbnail data
but shares sizing semantics with `THUMBNAIL` (scaling by image height).

This change fixes animated images not playing in cases in which only a
static thumbnail would be loaded.

This new mode will only be chosen if the message content is actually a
playable image, as such limiting bandwith usage to the required amount
by avoiding to load normal images fully (still fetching animated images
will increase bandwith usage as a whole of course).

Signed-off-by: networkException <[email protected]>
This patch removes the dependency on `VectorSettings` as well as only
enable animated rendering if the image is actually playable.

Signed-off-by: networkException <[email protected]>
@networkException networkException force-pushed the fix-animated-only-fetching-thumbnail branch from 18eed00 to 2bca94d Compare August 21, 2022 10:08
@networkException
Copy link
Contributor Author

@bmarty is there something missing for this to get merged?

@bmarty
Copy link
Member

bmarty commented Oct 4, 2022

@bmarty is there something missing for this to get merged?

Just time. Thanks for the update!

@bmarty bmarty merged commit 2cb16d9 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.

4 participants