feat(send_queue): report progress for media uploads #5008
Conversation
Signed-off-by: Johannes Marbach <[email protected]>
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 index = { | ||
| cfg_if::cfg_if! { |
There was a problem hiding this comment.
I could also do it with a #[cfg(feature = "unstable-msc4274")] and a #[cfg(not(feature = "unstable-msc4274"))] block if that's better? I just picked cfg_if because it's already used elsewhere in the crate.
| } { | ||
| 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.
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.
Co-authored-by: Ivan Enderlin <[email protected]> Signed-off-by: Johannes Marbach <[email protected]>
Co-authored-by: Ivan Enderlin <[email protected]> Signed-off-by: Johannes Marbach <[email protected]>
…ploadedMedia into EventSendProgress::MediaUpload
…iaUploadProgress into RoomSendQueueUpdate::MediaUpload
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.