Page MenuHomePhabricator

[web] Add thumbhash to encrypted media, fix spinner
ClosedPublic

Authored by bartek on May 21 2023, 7:56 AM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 23 2024, 9:45 PM
Unknown Object (File)
Feb 23 2024, 9:04 PM
Unknown Object (File)
Feb 23 2024, 8:10 PM
Unknown Object (File)
Feb 23 2024, 7:13 PM
Unknown Object (File)
Feb 23 2024, 7:01 PM
Unknown Object (File)
Feb 23 2024, 5:42 PM
Unknown Object (File)
Jan 10 2024, 3:39 AM
Unknown Object (File)
Dec 22 2023, 5:23 AM
Subscribers

Details

Summary

Also added props to the EncryptedMedia component to allow for a placeholder to be displayed when the media is being decrypted and to customize the style prop of the img / video elements. For videos, this uses the LoadableVideo component introduced in D7946

This also fixes the spinner CSS in modals for encrypted media. Without this change, it was displayed at the top instead of the center.

Depends on D7947.

Test Plan

Ensured the spinner is centered in both chat and modal.

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:07 AM
bartek added inline comments.
web/media/media.css
58–59 ↗(On Diff #26727)

Had to add another parent class instead of simply removing because otherwise it was breaking the in-chat spinners

ashoat requested changes to this revision.May 22 2023, 10:19 AM

Confused why we're ignoring thumbnailURI for encrypted videos

web/media/encrypted-multimedia.react.js
95 ↗(On Diff #26727)

For non-encrypted media, we set poster to thumbnailURI once it's available. Can you explain a little bit why we're doing it differently for encrypted media?

I'm guessing the idea is that the video is already loaded at that point, but it seems like in D7902 we wait for the video to be loaded before displaying the thumbnailURI anyways.

I guess it would also require some additional decryption work (decrypting the thumbnailURI), but that should be faster than decrypting the whole video.

web/media/media.css
58–59 ↗(On Diff #26727)

Thanks for explaining. I think this could've been a separate diff... this diff comes across as "X and Y", where X and Y could have been separated. Not worth actioning now, though

This revision now requires changes to proceed.May 22 2023, 10:19 AM
bartek added inline comments.
web/media/encrypted-multimedia.react.js
95 ↗(On Diff #26727)

Encrypted videos work totally differently.
First of all, we don't use thumbnailURI for them, but rather thumbnailHolder and thumbnailEncryptionKey.

For encrypted videos, the source is unavailable (source?.uri == null) when the placeholder is displayed. But after decryption work, this is a local blob/data URI and browser can auto-generate poster immediately when replacing the src prop.

This is not the case for non-encrypted videos, where src is available from the beginning, but the operation that takes time is downloading its content by the browser.
And when we're not replacing sources, the browser does not auto-generate the poster prop so we have to do the thumbnailURI trick

ashoat requested changes to this revision.May 23 2023, 1:03 PM
ashoat added inline comments.
web/media/encrypted-multimedia.react.js
95 ↗(On Diff #26727)

Thanks for the clarification RE thumbnailURI vs. thumbnailHolder!

I responded in more detail in D7902, but I'm still confused about why we're not showing the thumbnail at all here. I think we should try to decrypt the thumbnail, and the replace the thumbhash with the thumbnail as soon as possible. This will presumably happen before the whole video is decrypted, and will allow us to display a higher-res placeholder while the video continues to load.

This revision now requires changes to proceed.May 23 2023, 1:03 PM
bartek retitled this revision from [web] Add placeholder prop to encrypted media, fix spinner to [web] Add thumbhash to encrypted media, fix spinner.May 24 2023, 12:06 AM
bartek edited the summary of this revision. (Show Details)

Reworked the diff to the new approach. Now encrypted media also use LoadableVideo to display thumbhash and thumbnail for the video.

The whole approach and diff tree changes discussed in D7902

web/media/encrypted-multimedia.react.js
95 ↗(On Diff #26727)

w

95 ↗(On Diff #26727)

w

Whoops, accidentaly added a comment

web/media/multimedia-modal.react.js
105 ↗(On Diff #26923)

This state is defined in parent diff: D7947

ashoat requested changes to this revision.May 24 2023, 9:26 AM
ashoat added inline comments.
web/media/encrypted-multimedia.react.js
104–113 ↗(On Diff #26923)

I'm confused how we're going to render the video content here. Am I missing something? Shouldn't uri be set to source?.uri?

This revision now requires changes to proceed.May 24 2023, 9:26 AM
bartek added inline comments.
web/media/encrypted-multimedia.react.js
104–113 ↗(On Diff #26923)

Please see lines 65-67 of this file. We pass the source via videoRef once it's decrypted.

The non-null uri prop would create the <source> element inside the <video> (see loadable-video.react.js) and we don't want that because (from my past experience) it confuses the browser.

web/media/encrypted-multimedia.react.js
85 ↗(On Diff #26923)

We didn't have the src or <source> set here prior to this change.

ashoat added inline comments.
web/media/encrypted-multimedia.react.js
104–113 ↗(On Diff #26923)

Thanks, sorry I missed that!

This revision is now accepted and ready to land.May 24 2023, 10:10 AM