From daa05ea1587d57c813c6dc952c78ff21ca9916ac Mon Sep 17 00:00:00 2001 From: Benjamin Bouvier Date: Tue, 22 Oct 2024 11:37:43 +0200 Subject: [PATCH] feat(media): cache thumbnails too with a sensible media request key We can't know which key is going to be used precisely for the thumbnail, so assume non-animated cropped same-size thumbnail media request. --- crates/matrix-sdk/src/room/mod.rs | 47 +++++++- .../tests/integration/room/attachment/mod.rs | 109 +++++++++++++----- 2 files changed, 129 insertions(+), 27 deletions(-) diff --git a/crates/matrix-sdk/src/room/mod.rs b/crates/matrix-sdk/src/room/mod.rs index 1978fe78fb4..349b2cb7d64 100644 --- a/crates/matrix-sdk/src/room/mod.rs +++ b/crates/matrix-sdk/src/room/mod.rs @@ -26,6 +26,7 @@ use matrix_sdk_base::{ deserialized_responses::{ RawAnySyncOrStrippedState, RawSyncOrStrippedState, SyncOrStrippedState, TimelineEvent, }, + media::{MediaThumbnailSettings, MediaThumbnailSize}, store::StateStoreExt, ComposerDraft, RoomInfoNotableUpdateReasons, RoomMemberships, StateChanges, StateStoreDataKey, StateStoreDataValue, @@ -1917,6 +1918,7 @@ impl Room { /// /// * `store_in_cache` - A boolean defining whether the uploaded media will /// be stored in the cache immediately after a successful upload. + #[instrument(skip_all)] pub(super) async fn prepare_and_send_attachment<'a>( &'a self, filename: &'a str, @@ -1933,6 +1935,20 @@ impl Room { let thumbnail = config.thumbnail.take(); + // If necessary, store caching data for the thumbnail ahead of time. + let thumbnail_cache_info = if store_in_cache { + // Use a small closure returning Option to avoid an unnecessary complicated + // chain of map/and_then. + let get_info = || { + let thumbnail = thumbnail.as_ref()?; + let info = thumbnail.info.as_ref()?; + Some((thumbnail.data.clone(), info.height?, info.width?)) + }; + get_info() + } else { + None + }; + #[cfg(feature = "e2e-encryption")] let (media_source, thumbnail_source, thumbnail_info) = if self.is_encrypted().await? { self.client @@ -1943,6 +1959,8 @@ impl Room { .media() .upload_plain_media_and_thumbnail( content_type, + // TODO: get rid of this clone; wait for Ruma to use `Bytes` or something + // similar. data.clone(), thumbnail, send_progress, @@ -1960,11 +1978,38 @@ impl Room { if store_in_cache { let cache_store = self.client.event_cache_store(); + // A failure to cache shouldn't prevent the whole upload from finishing + // properly, so only log errors during caching. + + debug!("caching the media"); let request = MediaRequest { source: media_source.clone(), format: MediaFormat::File }; - // This shouldn't prevent the whole process from finishing properly. if let Err(err) = cache_store.add_media_content(&request, data).await { warn!("unable to cache the media after uploading it: {err}"); } + + if let Some(((data, height, width), source)) = + thumbnail_cache_info.zip(thumbnail_source.as_ref()) + { + debug!("caching the thumbnail"); + + // Do a best guess at figuring the media request: not animated, cropped + // thumbnail of the original size. + let request = MediaRequest { + source: source.clone(), + format: MediaFormat::Thumbnail(MediaThumbnailSettings { + size: MediaThumbnailSize { + method: ruma::media::Method::Crop, + width, + height, + }, + animated: false, + }), + }; + + if let Err(err) = cache_store.add_media_content(&request, data).await { + warn!("unable to cache the media after uploading it: {err}"); + } + } } let msg_type = self.make_attachment_message( diff --git a/crates/matrix-sdk/tests/integration/room/attachment/mod.rs b/crates/matrix-sdk/tests/integration/room/attachment/mod.rs index c34f69b8b84..31e2d395958 100644 --- a/crates/matrix-sdk/tests/integration/room/attachment/mod.rs +++ b/crates/matrix-sdk/tests/integration/room/attachment/mod.rs @@ -1,4 +1,4 @@ -use std::time::Duration; +use std::{sync::Mutex, time::Duration}; use matrix_sdk::{ attachment::{ @@ -6,7 +6,7 @@ use matrix_sdk::{ Thumbnail, }, config::SyncSettings, - media::{MediaFormat, MediaRequest}, + media::{MediaFormat, MediaRequest, MediaThumbnailSettings, MediaThumbnailSize}, test_utils::logged_in_client_with_server, }; use matrix_sdk_test::{async_test, mocks::mock_encryption_state, test_json, DEFAULT_TEST_ROOM_ID}; @@ -65,29 +65,10 @@ async fn test_room_attachment_send() { b"Hello world".to_vec(), AttachmentConfig::new(), ) - .store_in_cache() .await .unwrap(); assert_eq!(event_id!("$h29iv0s8:example.com"), response.event_id); - - // The media is immediately cached in the cache store, so we don't need to set - // up another mock endpoint for getting the media. - let reloaded = client - .media() - .get_media_content( - &MediaRequest { - source: MediaSource::Plain(owned_mxc_uri!( - "mxc://example.com/AQwafuaFswefuhsfAFAgsw" - )), - format: MediaFormat::File, - }, - true, - ) - .await - .unwrap(); - - assert_eq!(reloaded, b"Hello world"); } #[async_test] @@ -201,6 +182,9 @@ async fn test_room_attachment_send_wrong_info() { async fn test_room_attachment_send_info_thumbnail() { let (client, server) = logged_in_client_with_server().await; + let media_mxc = owned_mxc_uri!("mxc://example.com/media"); + let thumbnail_mxc = owned_mxc_uri!("mxc://example.com/thumbnail"); + Mock::given(method("PUT")) .and(path_regex(r"^/_matrix/client/r0/rooms/.*/send/.*")) .and(header("authorization", "Bearer 1234")) @@ -215,20 +199,37 @@ async fn test_room_attachment_send_info_thumbnail() { "mimetype":"image/jpeg", "size": 3600, }, - "thumbnail_url": "mxc://example.com/AQwafuaFswefuhsfAFAgsw", + "thumbnail_url": thumbnail_mxc, } }))) .respond_with(ResponseTemplate::new(200).set_body_json(&*test_json::EVENT_ID)) .mount(&server) .await; + let counter = Mutex::new(0); Mock::given(method("POST")) .and(path("/_matrix/media/r0/upload")) .and(header("authorization", "Bearer 1234")) .and(header("content-type", "image/jpeg")) - .respond_with(ResponseTemplate::new(200).set_body_json(json!({ - "content_uri": "mxc://example.com/AQwafuaFswefuhsfAFAgsw" - }))) + .respond_with({ + // First request: return the thumbnail MXC; + // Second request: return the media MXC. + let media_mxc = media_mxc.clone(); + let thumbnail_mxc = thumbnail_mxc.clone(); + move |_: &wiremock::Request| { + let mut counter = counter.lock().unwrap(); + if *counter == 0 { + *counter += 1; + ResponseTemplate::new(200).set_body_json(json!({ + "content_uri": &thumbnail_mxc + })) + } else { + ResponseTemplate::new(200).set_body_json(json!({ + "content_uri": &media_mxc + })) + } + } + }) .expect(2) .mount(&server) .await; @@ -242,6 +243,24 @@ async fn test_room_attachment_send_info_thumbnail() { let room = client.get_room(&DEFAULT_TEST_ROOM_ID).unwrap(); + // Preconditions: nothing is found in the cache. + let media_request = + MediaRequest { source: MediaSource::Plain(media_mxc), format: MediaFormat::File }; + let thumbnail_request = MediaRequest { + source: MediaSource::Plain(thumbnail_mxc.clone()), + format: MediaFormat::Thumbnail(MediaThumbnailSettings { + size: MediaThumbnailSize { + method: ruma::media::Method::Crop, + width: uint!(480), + height: uint!(360), + }, + animated: false, + }), + }; + let _ = client.media().get_media_content(&media_request, true).await.unwrap_err(); + let _ = client.media().get_media_content(&thumbnail_request, true).await.unwrap_err(); + + // Send the attachment with a thumbnail. let config = AttachmentConfig::with_thumbnail(Thumbnail { data: b"Thumbnail".to_vec(), content_type: mime::IMAGE_JPEG, @@ -260,10 +279,48 @@ async fn test_room_attachment_send_info_thumbnail() { let response = room .send_attachment("image", &mime::IMAGE_JPEG, b"Hello world".to_vec(), config) + .store_in_cache() .await .unwrap(); - assert_eq!(event_id!("$h29iv0s8:example.com"), response.event_id) + // The event was sent. + assert_eq!(event_id!("$h29iv0s8:example.com"), response.event_id); + + // The media is immediately cached in the cache store, so we don't need to set + // up another mock endpoint for getting the media. + let reloaded = client.media().get_media_content(&media_request, true).await.unwrap(); + assert_eq!(reloaded, b"Hello world"); + + // The thumbnail is cached with sensible defaults. + let reloaded = client.media().get_media_content(&thumbnail_request, true).await.unwrap(); + assert_eq!(reloaded, b"Thumbnail"); + + // The thumbnail can't be retrieved as a file. + let _ = client + .media() + .get_media_content( + &MediaRequest { + source: MediaSource::Plain(thumbnail_mxc.clone()), + format: MediaFormat::File, + }, + true, + ) + .await + .unwrap_err(); + + // But it is not found when requesting it as a thumbnail with a different size. + let thumbnail_request = MediaRequest { + source: MediaSource::Plain(thumbnail_mxc), + format: MediaFormat::Thumbnail(MediaThumbnailSettings { + size: MediaThumbnailSize { + method: ruma::media::Method::Crop, + width: uint!(42), + height: uint!(1337), + }, + animated: false, + }), + }; + let _ = client.media().get_media_content(&thumbnail_request, true).await.unwrap_err(); } #[async_test]