feat(send_queue): report progress for media uploads #5008
Conversation
Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>
There was a problem hiding this comment.
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.
| } { | ||
| let mut subscriber = progress.subscribe(); | ||
| let our_updates = updates.clone(); | ||
| spawn(async move { |
There was a problem hiding this comment.
Who controls this task? If the upload is cancelled, this task should be cancelled too right? Or maybe the subscriber.next().await may return a None at some point? If that's the case, please test that behaviour specifically, and comment that out, otherwise handle this 🙂.
There was a problem hiding this comment.
Yes, indeed next() will yield None once the observable (progress) and its clones are dropped. This happens once the final RoomSendQueueUpdate::MediaUpload is emited further down after the request has completed.
I've added a comment about this but could also add an explicit abort if that would be better.
There was a problem hiding this comment.
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!
| /// Is the media a thumbnail? | ||
| is_thumbnail: bool, |
There was a problem hiding this comment.
Out of curiosity: do you think there's lot of value in exposing this? In which cases would a consumer of the API care that an upload progress is related to a thumbnail or not? I'd imagine the interesting information is "what's the overall percentage of progress for the upload", and that this kind of details doesn't bring much value? (Also that would avoid the data migration 😈)
| is_thumbnail, | ||
| progress, | ||
| } => { | ||
| self.update_event_send_state( |
There was a problem hiding this comment.
At first sight, I was slightly worried about this approach. This will clone the item every time there's a new progress update, which will cause lots of unnecessary overhead at the FFI layer (especially on Android where the JVM may finalize the items much later after they've been dropped on the Rust side, causing spurious spikes in memory usage). Have you tried it out on a low to medium range Android device? How does it feel, in terms of latency/speed?
Overall, I imagined another API where we could have a side-channel progress stream, exposed independently of the RoomSendQueueUpdates. This way, we could channel those progress stream updates directly to the FFI consumers, in a new delegate — and not have to clone the timeline item each time.
But! Turns out we've discussed this in our team meeting, and we're curious to evaluate the performance of the current approach, so…
TLDR: no changes required here, but would be great if you had some performance feedback for this approach!
There was a problem hiding this comment.
Good point about the memory usage. I have actually only used this on iOS so far and don't even have a functional Android setup yet in the project I'm prototyping this. Overall, I'm finding it slightly odd to shove this into send queue updates, too, to be honest. It just seemed like the most straightforward way at the time. But I wouldn't necessarily be opposed to trying out a different approach if you already have concerns.
| let mut req = | ||
| room.client().media().upload(&mime, data, Some(request_config)); | ||
| if let Some(watcher) = progress_watcher { | ||
| req = req.with_send_progress_observable(watcher); |
There was a problem hiding this comment.
If I understand correctly, this will emit the following updates, if I'm sending a file with a thumbnail:
- first progress updates for the thumbnail, going from 0 to 100%
- then progress updates for the file itself, going from 0 to 100%
Is that right? I think observers would likely be more observed in the overall progress from 0 to 100%, so it would be nice if we could aggregate those into a single stream, for observers (especially for galleries which may contain many medias and thumbnails).
There was a problem hiding this comment.
I think this goes together with your other comment. I actually cannot think of a reason why a client would be interested in displaying the thumbnail and file upload progress separately. So aggregating them would probably be best.
I think a difficulty with this is determining the overall upload size ahead of time. The way the code is currently structured, we only receive the size of the file upload once the file has been encrypted and actually started uploading. To aggregate it, we would have to know the size already when starting the thumbnail upload, however.
There was a problem hiding this comment.
Maybe we could update UploadEncryptedFile so that it can either take a reader for the plain data (like today) or a vector of the encrypted data? Then we could encrypt both thumbnail and media ahead of time, to compute the overall upload size, store it on the send queue requests and use it inside RoomSendQueue::sending_task to aggregate the progress per thumbnail / media pair. That would mingle the concerns a little of course.
The only other option I can think of is to do some heuristics and translate the bytes sent for the encrypted attachments into bytes of the unencrypted attachments. That would mean the SDK wouldn't report the actually uploaded bytes though which doesn't feel great either.
🤔
There was a problem hiding this comment.
Nice, I thought about these two alternatives as well :)
- The first one would work for a thumbnail and a media, but could lead to device memory exhaustion (and getting OOM killed) if you want the same behavior for a gallery with many items.
- The second one could work nicely with a rule of 3:
fake_number_bytes_sent = number_encrypted_bytes_sent * total_bytes / total_encrypted_bytes. As you're pointing it, it wouldn't be the "perfect" information when forwarded to end users, and it would be wrong if the end user decided to show the live bandwidth usage (MB per second).
An alternative would be to restrict ourselves to "relative" progress (i.e. get a percentage, not raw values). This would make it impossible to display the live bandwidth usage, but might be sufficient to display an inline spinner that fills up over time. That could be a nice 80/20 solution, what do you think @Johennes @Hywan?
There was a problem hiding this comment.
Oh, yes good point that about the memory limitations when encrypting everything upfront.
I think the percentage should probably suffice in most cases. Maybe we could just map the internal TransmissionProgress to a new struct that contains only the current percentage then.
Co-authored-by: Ivan Enderlin <ivan@mnt.io> Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>
Co-authored-by: Ivan Enderlin <ivan@mnt.io> Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>
…ploadedMedia into EventSendProgress::MediaUpload
…iaUploadProgress into RoomSendQueueUpdate::MediaUpload
|
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.movKeep in mind though that this may also be caused by the upload process itself. I tried measuring the allocations now and I got this: 
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 |
|
@jmartinesp Thanks! Can you measure if the memory peak maps to the size of the media, or is it something else? |
I'd say it looks quite similar without these changes, the image above being a bit more stretched. |
|
@jmartinesp thanks a bunch for taking the time to test the impact of this on Android! ❤️ |
…out from handle_request
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.