Page MenuHomePhabricator

[web] Generate thumbhash during photo upload
ClosedPublic

Authored by bartek on May 15 2023, 8:04 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Nov 4, 1:17 PM
Unknown Object (File)
Mon, Nov 4, 1:17 PM
Unknown Object (File)
Mon, Nov 4, 1:17 PM
Unknown Object (File)
Mon, Nov 4, 1:17 PM
Unknown Object (File)
Mon, Nov 4, 1:17 PM
Unknown Object (File)
Mon, Nov 4, 1:16 PM
Unknown Object (File)
Wed, Oct 30, 3:57 PM
Unknown Object (File)
Wed, Oct 30, 3:57 PM
Subscribers

Details

Summary

This diff adds thumbhash creation step when uploading photos on web. The approach here is way simpler than on native. This works for both encrypted and non-encrypted media. The canvas code is based on this example code.

In contrast to native, we can use thumbhash npm package to avoid copying their code. Adding @ashoat because of this dependency

Test Plan

Selected a photo to upload on web, and verified in TablePlus that the upload contains the thumbHash string in the extras column. For encrypted media, it's significantly longer because it includes IV and other encryption metadata.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

bartek held this revision as a draft.
bartek edited the test plan for this revision. (Show Details)
bartek added a reviewer: ashoat.
bartek published this revision for review.May 17 2023, 9:57 PM

Mostly questions

web/input/input-state-container.react.js
782 ↗(On Diff #26590)

Nit: can we be consistent about capitalization?

1054–1059 ↗(On Diff #26590)

Can we please keep all input types as read-only?

web/input/input-state.js
15–38 ↗(On Diff #26590)

If you make this type read-only, does it introduce Flow errors? If not, let's make it read-only

web/media/image-utils.js
96 ↗(On Diff #26590)

Is there a reason you check binaryThumbHash here instead of thumbHashString? Figure it's always best to check the "final" data format to determine success, but I guess we could argue binaryThumbHash is the "final" data format for encryption here

100 ↗(On Diff #26590)

Why do we need to check binaryThumbhash here if we're already checking thumbHashString?

This revision is now accepted and ready to land.May 18 2023, 6:30 AM
web/input/input-state.js
15–38 ↗(On Diff #26590)

No errors

web/media/image-utils.js
96 ↗(On Diff #26590)

No strong opinion here, my thinking was that without binaryThumbHash there's no thumbHashString. And yes, the "final" result depends on the encryption.
I'm okay either way, but checking for thumbHashString seems more correct.

100 ↗(On Diff #26590)

Flow needs both to be non-null later.