signalapp / Signal-Android

A private messenger for Android.
https://signal.org
GNU Affero General Public License v3.0
25.59k stars 6.14k forks source link

GIFs added via built-in picker don't stretch with over-scroll animation #12391

Open Herohtar opened 2 years ago

Herohtar commented 2 years ago

Bug description

When over-scrolling in a chat, MP4 "GIFs" don't stretch.

Steps to reproduce

  1. Send a GIF using the built-in picker
  2. Scroll to the bottom of the chat
  3. Attempt to continue scrolling the chat

Actual result: Chat contents stretch slightly, except for the GIF Expected result: The GIF should stretch along with everything else

Screenshots

In the below video, the first GIF is an actual GIF that was inserted using Gboard's GIF search. The second GIF is a MP4 "GIF" inserted by the built-in GIPHY search.

https://user-images.githubusercontent.com/10427164/184466882-93966275-24b9-429b-b3ff-1905b1b78847.mp4

Device info

Device: Pixel 5 Android version: 12 Signal version: 5.45.6

Link to debug log

https://debuglogs.org/android/5.45.6/4e73fd6413072b9af1434aa03f8068858c96d4269d583bbf7737e7c949ca2cc9

AButton117 commented 2 years ago

I have also noticed this issue.

UserX404 commented 2 years ago

@AButton117 This doesn't add anything to find the root cause. As a minimum append your logs.

cody-signal commented 2 years ago

This is easily reproducible, it's something we're tracking but not planning to address immediately.

dreambucket13 commented 2 years ago

overscroll

I find it interesting that the bottom border does do the scroll effect - but the embedded video does not.

alex-signal commented 2 years ago

Just to note what's happening here:

GIFs are played back as MP4 videos, because the video file format is so much smaller than the GIF file format.

Our video players in the chat screen are in a separate container than the recycler view, and are dynamically positioned as the user scrolls content:

[ recyclerview ]
[ video players ]

This is done because attaching and detaching a video player from the view hierarchy is very expensive (80ms or more) and all this cost is due to having to build up and tear down textures and surfaces.

As you scroll through a recycler view, views are detached and then reattached to the recycler view container and rebound with new data. This is, for normal views, very fast, and is why they made recycler view in the first place. However, once you have this extra 80ms delay because of the surface textures, etc. inside a video player, you suddenly introduce a whole bunch of jank in the form of frame drops as the user is scrolling the chat, especially if there are a lot of gifs in it.

(Furthermore, we can only show a certain number of video players on the screen at once, depending on the device, because each needs its own dedicated hardware decoder.)

Fixing this issue involves changing how the video player view is rendered, because it needs to be drawn in a separate layer, and likely needs to be drawn within a RecyclerView.ItemDecorator's onDraw. This gets super tricky, and I'm not even sure it'd end up working correctly.

So that's essentially why they don't stretch, and we've not fixed it yet because it's a fairly hard fix for a fairly small UI issue (and we have limited resources)

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Herohtar commented 1 year ago

No

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Herohtar commented 1 year ago

Still not stale

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Herohtar commented 1 year ago

Doesn't look quite as awful as it used to, but still exists.

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Herohtar commented 1 year ago

Still there.

alex-signal commented 1 year ago

Hi! This is not currently high priority but I'll have a think about it. It is a complex issue because the video view rendering the gif is not a child of the recyclerview (due to performance reasons.)