feat(send_queue): report progress for media uploads

[Johennes]

This makes it possible to listen to the media upload progress when using the send queue. The progress is communicated via EventSendState::NotSentYet.

I haven't yet fixed / enhanced the tests or added a changelog because I'm not 100% sure about this approach and would like to get some preliminary feedback first.

• Public API changes documented in changelogs (optional)

---

• To see the specific tasks where the Asana app for GitHub is being used, see below:
  • https://app.asana.com/0/0/1210187425391164

---

Edit by @Hywan:

• Address Progress bars on media upload/download element-hq/element-x-ios#4055

[Hywan]

A really nice PR! I've a bit of feedback, but otherwise we are on the right direction.

We (the Matrix Rust team) were discussing the fact the timeline item is going to be recreated for each transmission progress update. Maybe we want a bit of throttle when broadcasting the transmission progress so that we reduce the amount of data sent to the apps. People at Element will start playing with it inside Element X, let's see how it goes.

[bnjbvr]

Interesting, and nice that we could in theory have this! I've got a few questions about the approach below. We'd like to try it out before validating the overall approach, because of the remark related to memory usage across the FFI, but other than that, it would be a great addition!

[jmartinesp]

I just tested this on EXA and it seems like there's a memory peak when receiving the upload progress:

Grabacion.de.pantalla.2025-05-28.a.las.9.54.21.mov

Keep in mind though that this may also be caused by the upload process itself.

---

I tried measuring the allocations now and I got this:

EventSendProgress$MediaUpload is the binding version of the SDK record, MediaUploadProgress is what we map it to. I think we have 2x the SDK items because we don't map them in the pinned events timeline, so they're discarded.

The range in the image shows an spike in general allocations, specially in the Java Heap. It's a shame we can't filter out items for the graph and just get the values for these allocations, we have no easy way of knowing whether these allocations come mainly from the EventSendProgress objects and their mapping process or there's something else happening at the same time.

[Hywan]

@jmartinesp Thanks! Can you measure if the memory peak maps to the size of the media, or is it something else?

[jmartinesp]

I'd say it looks quite similar without these changes, the image above being a bit more stretched.

[Johennes]

@jmartinesp thanks a bunch for taking the time to test the impact of this on Android! ❤️

[bnjbvr]

I wonder if we could avoid storing the sizes in persistent storage; after all, we're only interested in the aggregate of the remaining files to upload, so keeping this information in memory ought to be sufficient (plus, all the previously uploaded medias should still be in the event cache store until the media event is sent, at the end of the process). Let's try to make it more stateless instead.

This PR is a bit hard to review; if you introduce renamings, please keep the commit history tidy (no merge commits, only rebase/fixup! commits) and put renamings into individual commits. Also, would be sweet to group changes to the same layer (send queue vs timeline vs FFI) in commits, so as to give me a "direction" where to start from, and where to look at, in the end. You don't have to do it for this very specific PR, but for what it's worth, this is the kind of empathetic changes that usually helps (thus speeds up) reviews :)

I'd like to give this more thought and another look. Also, please add at least one end-to-end test so as to validate the approach :)

[codecov]

Codecov Report

Attention: Patch coverage is 90.21277% with 23 lines in your changes missing coverage. Please review.

Project coverage is 88.80%. Comparing base (18b169c) to head (7cba439).

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
crates/matrix-sdk/src/send_queue/mod.rs	91.34%	17 Missing and 1 partial ⚠️
...rates/matrix-sdk-ui/src/timeline/controller/mod.rs	64.28%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5008      +/-   ##
==========================================
+ Coverage   88.78%   88.80%   +0.01%     
==========================================
  Files         334      334              
  Lines       91236    91414     +178     
  Branches    91236    91414     +178     
==========================================
+ Hits        81007    81178     +171     
- Misses       6400     6406       +6     
- Partials     3829     3830       +1     

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

[Johennes]

@bnjbvr this is now ready for review again. I've addressed all previous comments other than #5008 (comment). Since I had previously made a mess out of the git history I also rebased everything into 10 self-contained commits. I will use only fix-ups from here on but wanted to try and make the base as easy to review as possible.

Regarding the approach, I went for a mix of using the event cache, info from dependent requests and an in-memory cache for thumbnail sizes.

[bnjbvr]

Thanks A LOT for breaking this into digestible commits; this made the review so much simpler. Overall this starts to look great, I've got a few non-trivial improvement requests that warrant another look, sorry about this. But we're reaching a nice state now. Really appreciate the test changes too 👏

Usually at this point, I'd try to help with the merging of this PR, by addressing the comments myself, but turns out I'll be away starting today and for a week or so, so I don't have much time to do this. If you're bored with this PR, I'll be happy to address my own comments when I'm back. Alternatively, another reviewer can make sure that my comments have been addressed in the meanwhile (although it'd be a bit of a stretch for them to read all the prior work).

Thanks a bunch, though; excited to get this soon!

[bnjbvr]

Super nice!

[bnjbvr]

1. Could we explain in the code comment the oddities about the value:
   1. Why do we need to store multiple thumbnail sizes for a single media transaction? As far as I understand, the transaction id is for the media event (either a regular (single) media event or a gallery event)
   2. Why can it be None? Could we store a Vec<usize> instead, and filter out the cases where there's no thumbnails? (and not even insert into this map, in this case)
2. Could we use a sync mutex here, aka matrix_sdk_common::locks::Mutex? we might not need the async part, if the lock is only held across sync portions of the code
3. Out of curiosity, would it make sense to have this in send_queue::StoreLock, which already contains some information about the current request? (in the form of BeingSent)

[Johennes]

1. Fixed in cf51ac9.
2. Fixed in 97a553f.
3. I will have to look into this.

[Johennes]

I addressed 3 with 971dcda and ad963c7. I had to switch back to the async Mutex though because that has lock_owned and I couldn't figure out the lifetimes without it. 🙈

[bnjbvr]

nit: looks like client can be inlined here, since it's only used once?

[Johennes]

I actually thought that, too, but the compiler won't let me. It looks like upload_encrypted_file borrows the client. If I inline room.client(), the client is dropped after this statement but then later needed again in req = req.with_send_progress_observable(progress);.

error[E0716]: temporary value dropped while borrowed
   --> crates/matrix-sdk/src/send_queue/mod.rs:944:39
    |
944 |                         let mut req = room.client()
    |                                       ^^^^^^^^^^^^^ creates a temporary value which is freed while still in use
945 |                             .upload_encrypted_file(&mut cursor)
946 |                             .with_request_config(RequestConfig::short_retry());
    |                                                                               - temporary value is freed at the end of this statement
...
949 |                             req = req.with_send_progress_observable(progress);
    |                                   --- borrow later used here
    |
    = note: consider using a `let` binding to create a longer lived value

[bnjbvr]

Right now, in the worst case, we'll have to take the event cache store lock twice, and the state store lock once, only to retrieve data that was in the stores. Since we already duplicate some of this data in the in-memory cache, we might make more use of this in-memory cache, by preemptively storing all the useful information in there, such that we don't need extra store accesses here?

[Johennes]

I could also store the other needed sizes there, yes. I think we would still need the current look-ups from persisted data though because in the case of galleries, we would otherwise not have anything to report progress for the remaining gallery items?

[bnjbvr]

Nice!

[Johennes]

Usually at this point, I'd try to help with the merging of this PR, by addressing the comments myself, but turns out I'll be away starting today and for a week or so, so I don't have much time to do this. If you're bored with this PR, I'll be happy to address my own comments when I'm back. Alternatively, another reviewer can make sure that my comments have been addressed in the meanwhile (although it'd be a bit of a stretch for them to read all the prior work).

Thanks for the review. 🙏 Letting this sit until you return seems fine from my perspective. I have addressed all comments apart from those I left unresolved.

PS: There was a conflict from a change on main. I wasn't sure how to resolve this but the folks in #matrix-rust-sdk-dev:flipdot.org said a merge should be ok.