Page MenuHomePhabricator

[lib] Introduce `MediaMessageServerDBContent` type
ClosedPublic

Authored by atul on Aug 30 2022, 1:45 PM.
Tags
None
Referenced Files
F3342106: D4985.diff
Fri, Nov 22, 12:34 AM
Unknown Object (File)
Mon, Nov 11, 2:51 AM
Unknown Object (File)
Sat, Nov 9, 6:57 AM
Unknown Object (File)
Sat, Nov 2, 1:14 PM
Unknown Object (File)
Sat, Nov 2, 1:14 PM
Unknown Object (File)
Sat, Nov 2, 1:13 PM
Unknown Object (File)
Sat, Nov 2, 1:11 PM
Unknown Object (File)
Sat, Oct 26, 3:23 AM
Subscribers

Details

Summary

In the past we had a 1:many relationship between messages and media and a 1:1 relationship between media and uploads.

Now that we've introduced thumbnails for videos, the relationship between media and uploads is 1:many (well 1:1 or 1:2 for now).

We need some way to encode that multiple uploads correspond to the same media. One way to do this would be to introduce a media table where each entry corresponds to a Media and where the columns look something like | id | container | uploadID | thumbnailID |.

Personally I think that would be the cleanest approach since the database schema matches the relationship between messages, media, and uploads in the code.

However, what I previously discussed with @ashoat (and which he previously signed off on) was to encode the relationship between uploads for each piece of media in an object and serialize and store those in the content column of the messages database.

Here's an example of what the content column of the messages table will look like in the future:

Screen Shot 2022-08-30 at 4.27.36 PM.png (100×1 px, 43 KB)

Instead of a list of IDs, we have a list of MediaMessageServerDBContents which encode the relationship between entries in the uploads table.


Depends on D4982

Test Plan

NA, just introducing a type we have yet to use anywhere

Diff Detail

Repository
rCOMM Comm
Branch
master
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

atul published this revision for review.Aug 30 2022, 1:47 PM
atul edited the summary of this revision. (Show Details)
atul edited the summary of this revision. (Show Details)

In the future, I'd also move to include a media table (discussed this with @atul offline) but for now this approach should work, and won't involve having to migrate queries, just modifying them to parse the new content column.

This revision is now accepted and ready to land.Aug 30 2022, 2:00 PM
This revision now requires review to proceed.Aug 30 2022, 2:00 PM

Also maybe you should show an example of the case when there are both images and videos in the same message, instead of just one video in the diff summary.

tomek added inline comments.
lib/types/messages/media.js
31–34

At some point we can consider introducing thumbnails also for images. I think we could e.g. download only a thumbnail when displaying in a thread and download the whole image only after user clicked the image.

31–39

We should use readonly everywhere

This revision is now accepted and ready to land.Aug 31 2022, 4:29 AM
lib/types/messages/media.js
31–39

Ahhh, I should have this down by now. Looks like I got it right in the summary here: https://phab.comm.dev/D4982 so not sure what happened here… thanks for catching this

lib/types/messages/media.js
31–34

This would be interesting and would also support using an entire thumbnails table (perhaps even along with a media table). However, how much faster would this make things? I know of other apps that do this like WhatsApp, which displays a blurry thumbnail of an image with a download button over it, and only once the thumbnail is clicked does the full image display. But I'm not sure how much more lightweight or efficient this is, so if someone more experienced has thoughts that'd be cool.

lib/types/messages/media.js
31–34

Definitely agree that thumbnails for images would be great down the line. We could maybe make the type for that photo_with_thumbnail and add it to MediaMessageServerDBContent? Haven't thought about it too much.

As an aside, instead of storing an image, we could maybe try an approach like https://github.com/woltapp/blurhash

This revision was landed with ongoing or failed builds.Aug 31 2022, 11:26 AM
This revision was automatically updated to reflect the committed changes.

Have we looked to see what places in the keyserver codebase might make an assumption about the format of the content row?

I went ahead and checked to see if there are any rows where type = 15 in the messages table on the MariaDB database on my production keyserver, and there were 0, which is a good sign for us being able to use this new format for type = 15. That said, there might still be existing code that assumes that type = 15 messages contain a simple array of numbers.

The obvious places that will need to be updated are the message-fetchers.js code in rawMessageInfoFromRows and lib/shared/messages/multimedia-message-spec.js. Wondering if there might be any other places?