Fix animated webp playback #8120
Conversation
Signed-off-by: Alex Maras <[email protected]>
| @@ -509,7 +509,7 @@ class MessageItemFactory @Inject constructor( | |||
| allowNonMxcUrls = informationData.sendState.isSending() | |||
| ) | |||
|
|
|||
| val playable = messageContent.mimeType == MimeTypes.Gif | |||
| val playable = messageContent.mimeType == MimeTypes.Gif || messageContent.mimeType == MimeTypes.Webp | |||
There was a problem hiding this comment.
This is a relatively poor check, but it's the best we can do without being able to read the file headers.
In theory, we could also use
| val playable = messageContent.mimeType == MimeTypes.Gif || messageContent.mimeType == MimeTypes.Webp | |
| val playable = messageContent.mimeType == MimeTypes.Gif || (messageContent.mimeType == MimeTypes.Webp && Build.VERSION.SDK_INT >= Build.VERSION_CODES.P) |
to not show the "play" button on older versions of Android, but it doesn't really make much of an appreciable difference - they're broken either way, and were before.
There was a problem hiding this comment.
I don't have any statistics, but I don't think animated webps are commonly used compared to static webp images, so making all webp images 'playable' might not be the best approach.
The playable attribute is only used to display the 'play' button above the image, but if you have the autoplay animated images option enabled the image will already be playing in the timeline, so given these 2 possible cases:
- Users not seeing the play button in an animated webp image and not having autoplay images enabled, thus not knowing that it's an animated image.
- Users seeing play buttons over every single webp, most of them being static.
I think it would be better to go with the first option, that is: remove the || messageContent.mimeType == MimeTypes.Webp.
There was a problem hiding this comment.
Thanks, fair call. I suspect you're right that the majority are static images.
In theory, I could work through to try to read the header of the WebP and actually know whether it's playable, but it's just a URL at this point, so I'd need to stream some of the file down until I have sufficient data to know. The check isn't particularly complex.
This would give a sensible place to add an equivalent check for .gif files too. I recognise that the vast majority of gifs are going to be animated, I'd be intrigued to know the stats on webp files.
I'll test removing the playable marker for webp and see whether it plays when autoplay is enabled. If it does, I reckon that's a really good starting point, and I could submit a secondary PR down the road to check properly whether it's animated if that makes the most sense to you.
There was a problem hiding this comment.
Sadly, autoplay in thumbnail doesn't work unless playable is enabled.
It does work when tapping into it, which is still a better result than it not working at all.
If we want to actually have it autoplay in preview mode, we'd need to know beforehand that it's actually an animated webp.
There was a problem hiding this comment.
Ah, for some reason the test image I was using was always being displayed as a thumbnail (size maybe?) so I didn't notice this issue.
There was a problem hiding this comment.
I'm actually seeing the opposite issue. playable for WebP is disabled, autoplay is disabled too, but thumbnails for animated webp images are autoplayed anyway. I don't think it's directly related to your changes, but I also have no idea why in your case it seems like the opposite is happening.
See recording
Top and bottom items are WebP, middle is a gif. Tested on an Android 13 emulator. One of the images is this one: https://mathiasbynens.be/demo/animated-webp-supported.webp.Screen_recording_20230222_181549.webm
There was a problem hiding this comment.
I've been able to replicate both behaviors now. I'm going to do some testing to figure out what causes it to autoplay sometimes when autoplay is turned off. In your recording, you can see that the nyancat gif has squared corners, and I'm currently looking at my emulator on API 33 and it has rounded corners and doesn't play until I click into it, even if I tap in and then go back out, and if I turn autoplay on, go back, then go turn it off again.
However, my laptop is now showing the same behavior as yours is.
I'll let you know when I have an explanation for this. Thanks for spotting!
EDIT: I've actually managed to replicate this within a single running emulator with 2 rooms. My old Facebook-bridged test room is showing me the intended behavior with autoplay off, while my empty room that I'm sending myself webp files from is animating while autoplay is off. Perhaps it's because the Facebook room is not E2E encrypted.
EDIT2: Yep, I created a new non-E2E room and it's paused with rounded corners prior to tapping into it, while the other (otherwise identical) room is autoplaying. This is because it's requesting a thumbnail during createGlideRequest, but it can obviously only do this is if the server is capable of providing a thumbnail, which it can't with E2E.
The use of playable and dontAnimate doesn't work for the Webp because Glide doesn't fully support Webp. dontAnimate is used for Gifs. I'll try to see if there's a way around this.
There was a problem hiding this comment.
I think most of the code makes sense except for the playable attribute being true for all webp images.
Also, to have the same behaviour in the attachment screen as you do, I had to add the || it.mimeType == MimeTypes.Webp to both DataAttachmentRoomProvider.getAttachmentInfoAt and RoomEventsAttachmentProvider.getAttachmentInfoAt, otherwise the webp images didn't animate in the attachment viewer screen.
…into fix/animated-webp-playback
|
Thanks @jmartinesp - apologies, I had those previously. I must have checked them out during my pre-commit clean-up. I'll add them back in and revert WebP being marked as playable and re-test. Edit: Ah - I realised why I took them out. I thought they weren't needed, I had forgotten that the attachments exist in the room settings too. Fixing that now. |
| @@ -528,6 +528,8 @@ class MessageItemFactory @Inject constructor( | |||
| ) | |||
|
|
|||
| val playable = messageContent.mimeType == MimeTypes.Gif | |||
| // don't show play button because detecting animated webp isn't possible via mimetype | |||
| val playableIfAutoplay = playable || messageContent.mimeType == MimeTypes.Webp | |||
There was a problem hiding this comment.
This works to autoplay animated WebP in the timeline if autoplay is enabled. If it isn't enabled, no play button shows (because playable is false) which will prevent confusion with static WebP. Static WebP functions fine in autoplay mode still.
This workaround is fairly effective. I've added a comment to try to explain what's going on here.
Ideally I'd like to look at doing another PR to determine conclusively whether a Gif or a WebP is actually animated via headers, therefore allowing the play button to appear on animated WebP too, and take away the play button on static Gifs, but that would be a longer exercise.
Type of change
Content
Allowed Animated WebP to play correctly.
This involves making Glide transformations optional in order to prevent mimetypes that Glide doesn't support from throwing exceptions. This has a very minor impact - making rounded corners not work for animated WebP files in the timeline. However, I see this as an improvement over it not working at all.
Motivation and context
Animated WebP files are delivered by default from the Facebook Bridge (https://github.com/mautrix/facebook) so this has been a pain point for me.
Element Android issues:
#2695
#4199
Animated WebP is supported on android versions natively. The only issue is that the Glide library doesn't support it outside of rendering:
bumptech/glide#571
This could alternatively potentially be fixed by using https://github.com/zjupure/GlideWebpDecoder
However, this feels like a change that could potentially introduce more issues, as that library only supports a couple of recent versions of Glide, which would necessitate keeping those in-line. I'm not sure how well maintained the library is, and this could be worse than just accepting the minor differences in missing rounded corners for animated WebP files.
Screenshots / GIFs
There is a minor annoyance introduced by this - non-animated webp files will have a "play" button on them. This is unavoidable without some major changes to the image viewer. We'd need to pull the image and read the VP8X header for the ANIM bit to see if it's an animated webp. The same issue is already present with non-animated Gifs.
Tests
playable)Tested devices
The changes don't do anything particularly useful on older versions of Android - but it's not more broken than it was. Android requires API v28 to play Animated WebP by the looks of it. At the moment, it would just be still images, whereas with this change they can at least see that they're supposed to be animated.
Checklist