Add experimental support for MSC4274 (inline media galleries)

[Johennes]

This is a proof of concept implementation of MSC4274. It works in my testing but it's quite hacky. I'd like to use this as a starting point to discuss what a proper implementation could look like.

Only the send queue path is implemented at the moment and it duplicates and generalizes the existing code for sending normal media events. The main challenge is the way send queue requests work. For the gallery we need to hold sending the actual event until all the different chains of media uploads have completed. The hack I went for to implement this is to introduce a dependent request kind FinishGalleryItemUpload to hold the result of a single media upload. These requests are never processed on their own in try_apply_single_dependent_request. Instead they are processed and removed in bulk in a separate function process_gallery_requests, called after canonicalize_dependent_requests.

This obviously isn't a great solution because it doesn't really integrate well with the remaining send queue handling. I'm curious if anyone has thoughts for how to do this properly?

• Public API changes documented in changelogs (optional)

[codecov]

Codecov Report

Attention: Patch coverage is 6.76329% with 386 lines in your changes missing coverage. Please review.

Project coverage is 85.49%. Comparing base (f8236a8) to head (9371971).

Files with missing lines	Patch %	Lines
crates/matrix-sdk/src/send_queue/upload.rs	4.62%	165 Missing ⚠️
crates/matrix-sdk/src/send_queue/mod.rs	13.98%	123 Missing ⚠️
crates/matrix-sdk/src/room/mod.rs	0.00%	48 Missing ⚠️
crates/matrix-sdk-ui/src/timeline/futures.rs	0.00%	17 Missing ⚠️
crates/matrix-sdk/src/attachment.rs	0.00%	14 Missing ⚠️
crates/matrix-sdk-ui/src/timeline/mod.rs	0.00%	9 Missing ⚠️
crates/matrix-sdk-base/src/store/send_queue.rs	0.00%	5 Missing ⚠️
crates/matrix-sdk/src/room/edit.rs	0.00%	4 Missing ⚠️
...rates/matrix-sdk-ui/src/timeline/event_item/mod.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4838      +/-   ##
==========================================
- Coverage   86.45%   85.49%   -0.97%     
==========================================
  Files         297      297              
  Lines       34625    35033     +408     
==========================================
+ Hits        29936    29950      +14     
- Misses       4689     5083     +394     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[bnjbvr]

Hi, send queue author here :)

The main challenge is the way send queue requests work. For the gallery we need to hold sending the actual event until all the different chains of media uploads have completed. The hack I went for to implement this is to introduce a dependent request kind FinishGalleryItemUpload to hold the result of a single media upload. These requests are never processed on their own in try_apply_single_dependent_request. Instead they are processed and removed in bulk in a separate function process_gallery_requests, called after canonicalize_dependent_requests.

So you can simulate a dependency tree by flattening it into a list of dependencies that fold into each other. If request R depends on dependent requests R0, R1, R2 then:

• R depends on R2 being done
• R2 depends on R1 being done
• R1 depends on R0 being done

Each subsequent request will accumulate the result of sending the previous one. In fact, this is how the dependency tree for a media upload is done:

• first we push a media upload for the thumbnail here
• then we have a dependent request for the actual file upload, that depends on the thumbnail being uploaded here
• then we have a dependent request for sending the event here

I think that for a gallery, this could be generalized: instead of having 2 requests to send a media (one for the thumbnail, one for the media), we'd have 2*N where N is the number of gallery items. Then, we don't need any special processing for galleries in the send queue (and likely we can share more code with the sending of medias).

Does it make sense?

[Johennes]

Thanks a ton for the input! 👌

Ok, I think I see what you mean. I had actually briefly considered something similar in the beginning but then abandoned it because the accumulation felt a bit scary. But I guess my PoC code turned out even more scary. 😅

IIUC we'll need a new SentRequestKey variant for accumulating the media sources and then handing it through the chain?

[bnjbvr]

Yeah I think so.Message ID: ***@***.***>

…

[Johennes]

This is now updated to use a flattened dependency tree. It actually turned out to not be as complicated as I had feared and indeed fits into the existing code a lot better. In a nutshell:

• Added SentRequestKey::GalleryMedia to accumulate the uploaded media sources
• Added QueuedRequestKind::GalleryMediaUpload to perform the upload (and produce SentRequestKey::GalleryMedia)
• Added DependentQueuedRequestKind::UploadGalleryFileOrThumbnail for a queued file / thumbnail upload and DependentQueuedRequestKind::FinishGallery for the queued final event

This works as far as I could test but still duplicates quite a bit of code. I think the amount of duplication could be reduced if we merged some of types:

• SentRequestKey::GalleryMedia could be merged into SentRequestKey::Media and we'd only actually use the accumulated for gallery uploads, not for normal media. Similarly for QueuedRequestKind::GalleryMediaUpload being merged into QueuedRequestKind::MediaUpload. This would then spare us the extra match arm in handle_request.
• DependentQueuedRequestKind::UploadGalleryFileOrThumbnail could be merged into DependentQueuedRequestKind::UploadFileWithThumbnail. Then we could use handle_dependent_gallery_file_or_thumbnail_upload for both normal media and galleries. For normal media uploads, we'd just always be in the depends_on_thumbnail == true case and the accumulation would only kick in for galleries.

That being said, I'm mindful that this is an unstable MSC and might go away in the future. So I'm curious whether you'd actually prefer the separation / duplication or not?

[bnjbvr]

Thanks! The design makes sense, and I think it would be nice to have fewer types and fold them wherever possible as you're suggesting. Let's not forget to add #[serde(default)] annotations on top of the new fields introduced in this PR, so it's backwards-compatible and we don't have to clear the send queue's store tables or do a fancy migration :)

(Also I'm wondering if the new future for sending a gallery could also be folded into the ones we already had, likewise?)

Then, maybe we could make use of a Cargo feature to guard all this code, and make it clear it's experimental. We've had experimental features in the past, and having the cfg guards in the code will make it clear what's related to galleries, and what's not. Maybe it's overkill.

In terms of review process: I think it would help a lot to split this PR up into smaller ones, which would include some substeps that can be merged atomatically. That would help process the whole PR, since the current one is a bit overwhelming already :)

And finally, as I won't be able to review this week or the next one, anyone can steal the review(s) from me, otherwise I'm happy to take care of these when I'm back.

[Johennes]

Sounds good on everything, thank you. 👍