Page MenuHomePhabricator

[web] Display thumbhash in chat
ClosedPublic

Authored by bartek on May 21 2023, 8:02 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, May 23, 6:43 AM
Unknown Object (File)
Fri, May 17, 11:05 PM
Unknown Object (File)
Fri, May 10, 3:22 PM
Unknown Object (File)
Wed, May 8, 9:05 AM
Unknown Object (File)
Fri, May 3, 2:32 PM
Unknown Object (File)
Fri, May 3, 9:27 AM
Unknown Object (File)
Thu, May 2, 3:08 AM
Unknown Object (File)
Wed, May 1, 11:12 AM
Subscribers

Details

Summary

This adds the thumbhash to the chat messages while they're loading (both non-encrypted images and videos). More details and a demo video are provided in this Linear comment. Encrypted video support is added in D7900

Test Plan

Throttled network and disabled cache in Chrome Dev Tools. Switched between chats with encrypted and non-encrypted media. The thumbhash was displayed for all images and videos while they were loading/decrypting.
See demo video in Linear comment.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

bartek held this revision as a draft.
bartek published this revision for review.May 21 2023, 8:10 AM

Add missing video preload

ashoat requested changes to this revision.May 22 2023, 10:12 AM
ashoat added inline comments.
web/chat/multimedia-message.react.js
45 ↗(On Diff #26730)

Two questions about this:

  1. It appears that we check for pendingUpload?.thumbHash in two places: here, and on line 152 of multimedia.react.js. Are both checks necessary?
  2. In what scenarios do we want to show the thumbHash for a pending upload? I would guess that the pending upload will be rendering a local object URL, which should already be preloaded. But I may be missing something.
web/media/multimedia.react.js
29 ↗(On Diff #26730)

Can you add a comment to the CSS as well, to make sure that if somebody changes the CSS they also change this number?

169 ↗(On Diff #26730)

Can you explain a little bit why we can't handle this with something like object-fit: scale-down? Ideally we could avoid the manual width calculation, but I'm guessing there's some reason we need it

192 ↗(On Diff #26730)

Why do we need to wait for the video to be loaded before we show the thumbnail? It seems to me that we only need to wait for the thumbnail to be loaded. What if we await preloadImage in the effect above, and call setVideoLoaded(true) afterwards?

Besides the UX benefit of showing the thumbnail earlier, this also addresses an unlikely race here, where if the video loads before the thumbnail we might end up rendering the thumbnail before it's ready.

This revision now requires changes to proceed.May 22 2023, 10:12 AM

Address review comments, use LoadableVideo, move props refactor from D7899

web/chat/multimedia-message.react.js
45 ↗(On Diff #26730)
  1. No, it doesn't matter really. I can remove the check here as both mediaSource a.k.a singleMedia and pendingUpload are available in <Multimedia> component.
  2. That's true, the only usage of this is in chat input bar when it's available immediately.
web/media/multimedia.react.js
169 ↗(On Diff #26730)

Quoting my comment in ENG-3868:

Managed to get both encrypted and non-encrypted media render with correct size.

  • In CSS we have max-height: 200px for multimedia
  • If element or its parent has max-height specified, it takes precedence over height so having heights larger will still limit them to 200px.
  • Using the above fact, I took the original image dimensions and set width and height as following:
height: originalHeight
width: min(200, originalHeight) * (originalWidth / originalHeight)

And object-fit does not refer to background image (we'd need background-size). Anyway, to have the background dimensions exactly cover image dimensions, we need to have its dimensions already. This is especially important for encrypted media, when the browser cannot get the dimensions from src image as it is not yet available.

192 ↗(On Diff #26730)

We want to display thumbhash (blurry placeholderImage) while the video is loaded, right? And then when it is loaded and able to click play (isVideoLoaded === true), we no longer want it to be blurry, so we replace it with thumbnailURI.

What if we await preloadImage in the effect above, and call setVideoLoaded(true) afterwards?

This would cause that non-blurry thumbnailURI is displayed while the actual video is still loading (except that unlikely race)

ashoat requested changes to this revision.May 23 2023, 1:00 PM

I think either there's a disconnect about when to display the thumbhash vs. the thumbnail, or perhaps I am misunderstanding the feasibility of my suggestion. Please let me know if I'm missing something!

web/media/multimedia.react.js
30 ↗(On Diff #26899)

Might be good to add a . here so it's searchable

169 ↗(On Diff #26730)

Thanks for explaining – that makes sense, the CSS can't work if the image isn't loaded, since preserving the aspect ratio in CSS requires knowing the dimensions, and CSS is getting the dimensions from the loaded image

192 ↗(On Diff #26730)

We want to display thumbhash (blurry placeholderImage) while the video is loaded, right?

Here's the order of display I think we should do:

  1. While the thumbnail is still loading, show the thumbhash
  2. Once the thumbnail is loaded, show the thumbnail. Assuming that playing at this point is not possible (certainly for encrypted media), we may also want to do something to hide the play button if possible
  3. Once the video is loaded, continue showing the thumbnail. If we're hiding the play button in step 2, at this point we can start to show it

What do you think of this approach? I think it could be achieved by the suggestion in my previous comment:

What if we await preloadImage in the effect above, and call setVideoLoaded(true) afterwards?

This would cause that non-blurry thumbnailURI is displayed while the actual video is still loading (except that unlikely race)

I think this is exactly what we want – for the non-blurry thumbnail to be displayed while the actual video is still loading. I figure that the thumbhash is there to display while we load the thumbnail.

This revision now requires changes to proceed.May 23 2023, 1:00 PM
bartek added inline comments.
web/media/multimedia.react.js
192 ↗(On Diff #26730)

Your way seems counterintuitive to me. Why do you want to display a thumbnail while the video is still loading? In my understanding, this is against the whole idea of blurhash/thumbhash.
But let's assume that we want to display non-blurred thumbnails asap. In that case, the thumbhash for non-encrypted media is not needed at all because the browser generates thumbnails directly already and it's doing it pretty fast. In practice, decrypting thumbnails would also be so quick that the blurry thumbhash would only blink for a very short time before being replaced by an actual thumbnail (btw thumbhash also needs to be decrypted). This makes me wonder if we want to display thumbhash for videos at all. Also, this means that my whole video thumbhash work for videos is wasted: D7946 is not needed at all and encrypted-video thumbhash needs substantial rework.
Besides that, I'm not very opposed to your approach. I'd appreciate if you could explain its benefits.

ashoat requested changes to this revision.May 23 2023, 2:06 PM

Passing back to your queue, but feel free to-request review again if there's something that I'm missing. Also please let me know if this would be a difficult change to make... my impression is that changes in D7946 should hopefully be easy, but I'm not sure about the whole scope of changes that would be required.

Besides that, I'm not very opposed to your approach. I'd appreciate if you could explain its benefits.

I guess we perhaps see different goals of thumbhash:

  1. My view: the goal of thumbhash is to provide a seamless and clean transition while loading the content
  2. Your view: the goal of thumbhash is to provide a clean and attractive indicator to the user that the content is loading

I think your view is that the thumbhash should indicate to the user when the content is loading / finished loading, and so it would be confusing if the thumbhash stopped being rendered before that point.

Whereas my view is that the thumbhash is just meant to make the transition of loading the content more "seamless", but otherwise the behavior should be to aim to present the highest fidelity version of the content we can display at any given moment.

In terms of concrete benefits, presenting the highest fidelity version of the content allows the user to "see" things as quickly as possible, which I think improves the user experience.

web/media/multimedia.react.js
192 ↗(On Diff #26730)

In that case, the thumbhash for non-encrypted media is not needed at all because the browser generates thumbnails directly already and it's doing it pretty fast.

Showing a browser-generated thumbnail still requires a secondary download step, whereas the thumbhash is always delivered alongside the content metadata. I haven't measured, but I suspect there is a discernable difference in scenarios where the content isn't cached locally.

In practice, decrypting thumbnails would also be so quick that the blurry thumbhash would only blink for a very short time before being replaced by an actual thumbnail (btw thumbhash also needs to be decrypted)

The encrypted thumbnail still requires an additional download step, no? Whereas the encrypted thumbhash is available as soon as the client is aware of the message.

This revision now requires changes to proceed.May 23 2023, 2:06 PM

Whereas my view is that the thumbhash is just meant to make the transition of loading the content more "seamless", but otherwise the behavior should be to aim to present the highest fidelity version of the content we can display at any given moment.

In terms of concrete benefits, presenting the highest fidelity version of the content allows the user to "see" things as quickly as possible, which I think improves the user experience.

Fair enough, I'll try your approach

Reworked the video approach to display actual thumbnails as soon as possible. I admit this looks well, but needed to throttle network to "Slow 3G" to be able to see the effect in some cases. I'm not sure if this is a good idea, but I think it's better than nothing. I also added a placeholder for the thumbnail in case the video is not loaded yet.
Now it works especially as requested: The thumbnail is displayed as soon as it is downloaded/decrypted and replaces the thumbhash. Code for this is in D7946

I had to rework the diff tree because now every other diff depends on D7946 and encrypted media (D7900) requires all other diffs. I hope this is okay. I've added comments to the diffs with links to usages

ashoat added inline comments.
web/chat/chat-input-bar.react.js
235 ↗(On Diff #26921)

I was surprised to see thumbHash always included for pending uploads, so I took a look at InputStateContainer on web. It does appear that we always attempt to generate a thumbHash in appendFile. Meanwhile, thumbnail stuff is not set because we don't yet support video uploads on web.

One question did come up in my reading. I was reading this code, which is called when a message is sent, and does the work of optimistically creating a RawMessageInfo with the media before we receive a respond from the keyserver.

In that code, it appears that we don't pass the thumbHash into the RawMessageInfo. I guess we probably skip it because the object URL can be rendered instantly, but I found myself wondering why we don't do the same thing for the multimedia previews here. Shouldn't we be consistent?

Separately, the comment in the linked code should probably be updated. In particular, the following no longer applies:

We actually don't use the dimensions on the web side currently, but if we ever change that (for instance if we want to render a properly sized loading overlay like we do on native), 0,0 is probably a good default.

(We now use the dimensions on the web, precisely for rendering a properly size loading overlay.)

This revision is now accepted and ready to land.May 24 2023, 9:18 AM
web/chat/chat-input-bar.react.js
235 ↗(On Diff #26921)

I was surprised to see thumbHash always included for pending uploads, so I took a look at InputStateContainer on web. It does appear that we always attempt to generate a thumbHash in appendFile. Meanwhile, thumbnail stuff is not set because we don't yet support video uploads on web.

That's correct, we set anything video-related to null in pending upload based sources.

One question did come up in my reading. I was reading this code, which is called when a message is sent, and does the work of optimistically creating a RawMessageInfo with the media before we receive a respond from the keyserver.

In that code, it appears that we don't pass the thumbHash into the RawMessageInfo. I guess we probably skip it because the object URL can be rendered instantly, but I found myself wondering why we don't do the same thing for the multimedia previews here. Shouldn't we be consistent?

Yes, we should! Good catch, that was my oversight that I didn't update that part initially. Created D7965 to address this, I also removed the non-applicable comment there.