sdk: Improvements around `generate_image_thumbnail` #3415

zecakeh · 2024-05-15T11:33:14Z

The main improvement here is to be able to choose the format of the generated thumbnail to avoid errors because the image crate cannot encode to the same format as the original.

We also allow to send the attachment even if the generation fails, because thumbnails are optional after all.

See commits messages for details.

Public API changes documented in changelogs (optional)

We have already all the data for it. Also fixes an error where the thumbnail format was assumed to always be JPEG. Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>

Sending an attachment could often fail if the image crate cannot encode the thumbnail to the same format as the original. This allows to select a known supported format to always be able to generate a thumbnail. Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>

Since the thumbnail is optional, failing to generate it should not stop us from sending the attachment. Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>

codecov · 2024-05-15T11:55:15Z

Codecov Report

Attention: Patch coverage is 87.50000% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 83.16%. Comparing base (7ae0bce) to head (9c2b0a8).
Report is 9 commits behind head on main.

Files	Patch %	Lines
crates/matrix-sdk/src/room/futures.rs	75.00%	2 Missing ⚠️
crates/matrix-sdk/src/attachment.rs	93.75%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3415      +/-   ##
==========================================
+ Coverage   82.98%   83.16%   +0.17%     
==========================================
  Files         247      247              
  Lines       25019    25014       -5     
==========================================
+ Hits        20763    20802      +39     
+ Misses       4256     4212      -44

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

bnjbvr

Sweet, that makes sense, thanks!

I think a few more tests might be nice to have, but not mandatory. I've got a few nit requests below that I'd like to see addressed before merging, though.

bnjbvr · 2024-05-16T08:42:15Z

crates/matrix-sdk/src/attachment.rs

+    let content_type = mime::Mime::from_str(image_format.to_mime_type())
+        .expect("image should give a valid mimetype");


Can we avoid the expect and return an error instead?

bnjbvr · 2024-05-16T08:42:41Z

crates/matrix-sdk/src/room/futures.rs

-                            data_slot = thumbnail_data;
-                            Some(Thumbnail {
-                                data: data_slot,
-                                content_type: mime::IMAGE_JPEG,


Nice catch. Is it worth adding a regression test, or too much hassle?

I added tests that check this.

bnjbvr · 2024-05-16T08:43:13Z

crates/matrix-sdk/CHANGELOG.md

@@ -12,6 +12,9 @@ Breaking changes:
 - A custom sliding sync proxy set with `ClientBuilder::sliding_sync_proxy` now takes precedence over a discovered proxy.
 - `Client::get_profile` was moved to `Account` and renamed to `Account::fetch_user_profile_of`. `Account::get_profile` was renamed to `Account::fetch_user_profile`.
 - `generate_image_thumbnail` now returns a `Thumbnail`.
+- It is know possible to select the format of a generated thumbnail.


nit: now, not know

Right, thanks

bnjbvr · 2024-05-16T08:45:10Z

crates/matrix-sdk/src/attachment.rs

+    let thumbnail_format = match format {
+        ThumbnailFormat::Always(format) => format,
+        ThumbnailFormat::Fallback(format) if !image_format.writing_enabled() => format,
+        _ => image_format,


Can you use the explicit last variant instead of _ here, so that if we add a new variant to ThumbnailFormat, the compiler warns us here, and we can then decide what's the best thing to do?

bnjbvr · 2024-05-16T08:47:01Z

crates/matrix-sdk/src/room/futures.rs

+                        Err(error) => {
+                            tracing::error!("Failed to generate thumbnail: {error}");
+                            None
+                        }


I like this; can we remove the previous match arm that handles two ImageError variants, so they're also logged?

Also, can we use warn! instead of error!, since these are non-critical?

I am not sure that ThumbnailBiggerThanOriginal should be logged, or at least not at the warn level.

It could allow the users to be lazy and always use generate_thumbnail without having to check the image's size. It is more an optimization of the code than a real error so it's not something users should care to fix.

Ok. Would it make sense to log the other error at the warn level, though?

Yes, definitely.

Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>

zecakeh · 2024-05-16T11:28:19Z

I took care of the reviews, merged main to solve conflicts, and added integration tests.

For the integration tests I needed a real image so I took the avatar from #matrix-rust-sdk:matrix.org. It's also difficult to trigger the ImageFormat::Fallback(format) path because there are only 2 formats that are not writable and they generate really big images (~5MB for the same avatar).

poljar · 2024-05-16T11:40:17Z

For the integration tests I needed a real image so I took the avatar from #matrix-rust-sdk:matrix.org.

Nice 🦀.

bnjbvr

Thanks for the tests!

bnjbvr · 2024-05-17T13:13:26Z

crates/matrix-sdk/tests/integration/room/attachment/mod.rs

+        .await
+        .unwrap();
+
+    assert_eq!(event_id!("$h29iv0s8:example.com"), response.event_id)


Here and in the 3 other tests: can we replace the raw event_id with test_json::EVENT_ID too?

test_json::EVENT_ID is a serde_json::Value containing a JSON object, so we can't use it for asserting the event ID, afaiu.

We would probably need a matrix_sdk_test::DEFAULT_TEST_EVENT_ID, like we have for the room ID for that.

bnjbvr · 2024-05-17T13:15:27Z

testing/matrix-sdk-test/src/lib.rs

@@ -93,7 +93,7 @@ macro_rules! init_tracing_for_tests {
                    // Since tracing_subscriber does prefix matching, the `matrix_sdk=` directive
                    // takes effect for all the main crates (`matrix_sdk_base`, `matrix_sdk_crypto`
                    // and so on).
-                    "info,matrix_sdk=debug".into()
+                    "info,wiremock=debug,matrix_sdk=debug".into()


Can we revert this change? wiremock can be verbose in the output…

Ah right sorry. I had enabled it locally to understand why an event wasn't matching for an endpoint.

Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>

zecakeh requested a review from a team as a code owner May 15, 2024 11:33

zecakeh requested review from bnjbvr and removed request for a team May 15, 2024 11:33

zecakeh force-pushed the generate-thumbnail branch from 116193a to c2152ad Compare May 15, 2024 11:42

zecakeh added 3 commits May 15, 2024 13:52

sdk: Return a Thumbnail from generate_image_thumbnail

5433eeb

We have already all the data for it. Also fixes an error where the thumbnail format was assumed to always be JPEG. Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>

sdk: Do not return error of thumbnail generation for SendAttachment

01fe95a

Since the thumbnail is optional, failing to generate it should not stop us from sending the attachment. Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>

zecakeh force-pushed the generate-thumbnail branch from c2152ad to 01fe95a Compare May 15, 2024 11:54

bnjbvr requested changes May 16, 2024

View reviewed changes

zecakeh added 4 commits May 16, 2024 11:50

Apply code review fixes

ef7a464

Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>

Merge branch 'main' into generate-thumbnail

6e4d085

Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>

sdk: Split attachment tests in separate file

a13620d

Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>

sdk: Add integration tests for generating thumbnails

c58a0cf

Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>

bnjbvr self-requested a review May 16, 2024 12:15

bnjbvr approved these changes May 17, 2024

View reviewed changes

Revert wiremock debug log level

9c2b0a8

Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>

bnjbvr merged commit 6c18bcf into matrix-org:main May 17, 2024
36 checks passed

zecakeh deleted the generate-thumbnail branch May 17, 2024 15:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sdk: Improvements around `generate_image_thumbnail` #3415

sdk: Improvements around `generate_image_thumbnail` #3415

zecakeh commented May 15, 2024

codecov bot commented May 15, 2024 •

edited

bnjbvr left a comment

bnjbvr May 16, 2024

zecakeh May 16, 2024

bnjbvr May 16, 2024

zecakeh May 16, 2024

bnjbvr May 16, 2024

zecakeh May 16, 2024

bnjbvr May 16, 2024

zecakeh May 16, 2024

bnjbvr May 16, 2024

zecakeh May 16, 2024 •

edited

bnjbvr May 16, 2024

zecakeh May 16, 2024

zecakeh May 16, 2024

zecakeh commented May 16, 2024 •

edited

poljar commented May 16, 2024

bnjbvr left a comment

bnjbvr May 17, 2024

zecakeh May 17, 2024

bnjbvr May 17, 2024

zecakeh May 17, 2024

zecakeh May 17, 2024

		let content_type = mime::Mime::from_str(image_format.to_mime_type())
		.expect("image should give a valid mimetype");

sdk: Improvements around generate_image_thumbnail #3415

sdk: Improvements around generate_image_thumbnail #3415

Conversation

zecakeh commented May 15, 2024

codecov bot commented May 15, 2024 • edited

Codecov Report

bnjbvr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zecakeh May 16, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zecakeh commented May 16, 2024 • edited

poljar commented May 16, 2024

bnjbvr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdk: Improvements around `generate_image_thumbnail` #3415

sdk: Improvements around `generate_image_thumbnail` #3415

codecov bot commented May 15, 2024 •

edited

zecakeh May 16, 2024 •

edited

zecakeh commented May 16, 2024 •

edited