sdk: Improvements around generate_image_thumbnail (matrix-org#3415)

* sdk: Return a Thumbnail from generate_image_thumbnail

We have already all the data for it.
Also fixes an error where the thumbnail format was assumed to always be
JPEG.

* sdk: Allow to select the format of the generated thumbnail

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.

* sdk: Do not return error of thumbnail generation for SendAttachment

Since the thumbnail is optional, failing to generate it should not
stop us from sending the attachment.

* Apply code review fixes
* sdk: Split attachment tests in separate file
* sdk: Add integration tests for generating thumbnails
* Revert wiremock debug log level

---------

Signed-off-by: Kévin Commaille <[email protected]>

.github/workflows/ci.yml

@@ -41,6 +41,7 @@ jobs:
           - markdown
           - socks
           - sso-login
+          - image-proc
 
     steps:
       - name: Checkout

.github/workflows/coverage.yml

@@ -102,7 +102,7 @@ jobs:
         run: |
           rustup run stable cargo tarpaulin \
             --skip-clean --profile cov --out xml \
-            --features experimental-widgets,testing
+            --features experimental-widgets,testing,image-proc
         env:
           CARGO_PROFILE_COV_INHERITS: 'dev'
           CARGO_PROFILE_COV_DEBUG: 1

crates/matrix-sdk/CHANGELOG.md

@@ -11,6 +11,10 @@ Breaking changes:
   `Media::get_file`/`Media::remove_file`/`Media::get_thumbnail`/`Media::remove_thumbnail`
 - 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 now possible to select the format of a generated thumbnail.
+  - `generate_image_thumbnail` takes a `ThumbnailFormat`.
+  - `AttachmentConfig::generate_thumbnail` takes a `ThumbnailFormat`.
 
 Additions:
 

crates/matrix-sdk/src/attachment.rs

@@ -20,6 +20,8 @@ use std::time::Duration;
 
 #[cfg(feature = "image-proc")]
 use image::GenericImageView;
+#[cfg(feature = "image-proc")]
+pub use image::ImageFormat;
 use ruma::{
     assign,
     events::{
@@ -188,7 +190,7 @@ pub struct Thumbnail {
 }
 
 /// Configuration for sending an attachment.
-#[derive(Debug)]
+#[derive(Debug, Default)]
 pub struct AttachmentConfig {
     pub(crate) txn_id: Option<OwnedTransactionId>,
     pub(crate) info: Option<AttachmentInfo>,
@@ -200,25 +202,16 @@ pub struct AttachmentConfig {
     pub(crate) generate_thumbnail: bool,
     #[cfg(feature = "image-proc")]
     pub(crate) thumbnail_size: Option<(u32, u32)>,
+    #[cfg(feature = "image-proc")]
+    pub(crate) thumbnail_format: ThumbnailFormat,
 }
 
 impl AttachmentConfig {
     /// Create a new default `AttachmentConfig` without providing a thumbnail.
     ///
     /// To provide a thumbnail use [`AttachmentConfig::with_thumbnail()`].
     pub fn new() -> Self {
-        Self {
-            txn_id: Default::default(),
-            info: Default::default(),
-            thumbnail: None,
-            caption: None,
-            formatted_caption: None,
-            mentions: Default::default(),
-            #[cfg(feature = "image-proc")]
-            generate_thumbnail: Default::default(),
-            #[cfg(feature = "image-proc")]
-            thumbnail_size: Default::default(),
-        }
+        Self::default()
     }
 
     /// Generate the thumbnail to send for this media.
@@ -229,15 +222,21 @@ impl AttachmentConfig {
     /// more information, see the [image](https://github.com/image-rs/image)
     /// crate.
     ///
+    /// If generating the thumbnail failed, the error will be logged and sending
+    /// the attachment will proceed without a thumbnail.
+    ///
     /// # Arguments
     ///
     /// * `size` - The size of the thumbnail in pixels as a `(width, height)`
     /// tuple. If set to `None`, defaults to `(800, 600)`.
+    ///
+    /// * `format` - The image format to use to encode the thumbnail.
     #[cfg(feature = "image-proc")]
     #[must_use]
-    pub fn generate_thumbnail(mut self, size: Option<(u32, u32)>) -> Self {
+    pub fn generate_thumbnail(mut self, size: Option<(u32, u32)>, format: ThumbnailFormat) -> Self {
         self.generate_thumbnail = true;
         self.thumbnail_size = size;
+        self.thumbnail_format = format;
         self
     }
 
@@ -252,18 +251,7 @@ impl AttachmentConfig {
     /// [`AttachmentConfig::new()`] and
     /// [`AttachmentConfig::generate_thumbnail()`].
     pub fn with_thumbnail(thumbnail: Thumbnail) -> Self {
-        Self {
-            txn_id: Default::default(),
-            info: Default::default(),
-            thumbnail: Some(thumbnail),
-            caption: None,
-            formatted_caption: None,
-            mentions: Default::default(),
-            #[cfg(feature = "image-proc")]
-            generate_thumbnail: Default::default(),
-            #[cfg(feature = "image-proc")]
-            thumbnail_size: Default::default(),
-        }
+        Self { thumbnail: Some(thumbnail), ..Default::default() }
     }
 
     /// Set the transaction ID to send.
@@ -322,34 +310,30 @@ impl AttachmentConfig {
     }
 }
 
-impl Default for AttachmentConfig {
-    fn default() -> Self {
-        Self::new()
-    }
-}
-
 /// Generate a thumbnail for an image.
 ///
 /// This is a convenience method that uses the
 /// [image](https://github.com/image-rs/image) crate.
 ///
 /// # Arguments
 /// * `content_type` - The type of the media, this will be used as the
-/// content-type header.
+///   content-type header.
 ///
 /// * `reader` - A `Reader` that will be used to fetch the raw bytes of the
-/// media.
+///   media.
 ///
 /// * `size` - The size of the thumbnail in pixels as a `(width, height)` tuple.
-/// If set to `None`, defaults to `(800, 600)`.
+///   If set to `None`, defaults to `(800, 600)`.
+///
+/// * `format` - The image format to use to encode the thumbnail.
 ///
 /// # Examples
 ///
 /// ```no_run
 /// use std::{io::Cursor, path::PathBuf};
 ///
 /// use matrix_sdk::attachment::{
-///     generate_image_thumbnail, AttachmentConfig, Thumbnail,
+///     generate_image_thumbnail, AttachmentConfig, Thumbnail, ThumbnailFormat,
 /// };
 /// use mime;
 /// # use matrix_sdk::{Client, ruma::room_id };
@@ -363,13 +347,13 @@ impl Default for AttachmentConfig {
 /// let image = tokio::fs::read(path).await?;
 ///
 /// let cursor = Cursor::new(&image);
-/// let (thumbnail_data, thumbnail_info) =
-///     generate_image_thumbnail(&mime::IMAGE_JPEG, cursor, None)?;
-/// let config = AttachmentConfig::with_thumbnail(Thumbnail {
-///     data: thumbnail_data,
-///     content_type: mime::IMAGE_JPEG,
-///     info: Some(thumbnail_info),
-/// });
+/// let thumbnail = generate_image_thumbnail(
+///     &mime::IMAGE_JPEG,
+///     cursor,
+///     None,
+///     ThumbnailFormat::Original,
+/// )?;
+/// let config = AttachmentConfig::with_thumbnail(thumbnail);
 ///
 /// if let Some(room) = client.get_room(&room_id) {
 ///     room.send_attachment(
@@ -387,13 +371,13 @@ pub fn generate_image_thumbnail<R: BufRead + Seek>(
     content_type: &mime::Mime,
     reader: R,
     size: Option<(u32, u32)>,
-) -> Result<(Vec<u8>, BaseThumbnailInfo), ImageError> {
-    let image_format = image::ImageFormat::from_mime_type(content_type);
-    if image_format.is_none() {
-        return Err(ImageError::FormatNotSupported);
-    }
+    format: ThumbnailFormat,
+) -> Result<Thumbnail, ImageError> {
+    use std::str::FromStr;
 
-    let image_format = image_format.unwrap();
+    let Some(image_format) = ImageFormat::from_mime_type(content_type) else {
+        return Err(ImageError::FormatNotSupported);
+    };
 
     let image = image::load(reader, image_format)?;
     let (original_width, original_height) = image.dimensions();
@@ -409,16 +393,48 @@ pub fn generate_image_thumbnail<R: BufRead + Seek>(
     let thumbnail = image.thumbnail(width, height);
     let (thumbnail_width, thumbnail_height) = thumbnail.dimensions();
 
+    let thumbnail_format = match format {
+        ThumbnailFormat::Always(format) => format,
+        ThumbnailFormat::Fallback(format) if !image_format.writing_enabled() => format,
+        ThumbnailFormat::Fallback(_) | ThumbnailFormat::Original => image_format,
+    };
+
     let mut data: Vec<u8> = vec![];
-    thumbnail.write_to(&mut Cursor::new(&mut data), image_format)?;
+    thumbnail.write_to(&mut Cursor::new(&mut data), thumbnail_format)?;
     let data_size = data.len() as u32;
 
-    Ok((
-        data,
-        BaseThumbnailInfo {
-            width: Some(thumbnail_width.into()),
-            height: Some(thumbnail_height.into()),
-            size: Some(data_size.into()),
-        },
-    ))
+    let content_type = mime::Mime::from_str(thumbnail_format.to_mime_type())?;
+
+    let info = BaseThumbnailInfo {
+        width: Some(thumbnail_width.into()),
+        height: Some(thumbnail_height.into()),
+        size: Some(data_size.into()),
+    };
+
+    Ok(Thumbnail { data, content_type, info: Some(info) })
+}
+
+/// The format to use for encoding the thumbnail.
+#[cfg(feature = "image-proc")]
+#[derive(Debug, Default, Clone, Copy)]
+pub enum ThumbnailFormat {
+    /// Always use this format.
+    ///
+    /// Will always return an error if this format is not writable by the
+    /// `image` crate.
+    Always(ImageFormat),
+    /// Try to use the same format as the original image, and fallback to this
+    /// one if the original format is not writable.
+    ///
+    /// Will return an error if both the original format and this format are not
+    /// writable by the `image` crate.
+    Fallback(ImageFormat),
+    /// Only try to use the format of the original image.
+    ///
+    /// Will return an error if the original format is not writable by the
+    /// `image` crate.
+    ///
+    /// This is the default.
+    #[default]
+    Original,
 }

crates/matrix-sdk/src/error.rs

@@ -404,6 +404,10 @@ pub enum ImageError {
     #[error(transparent)]
     Proc(#[from] image::ImageError),
 
+    /// Error parsing the mimetype of the image.
+    #[error(transparent)]
+    Mime(#[from] mime::FromStrError),
+
     /// The image format is not supported.
     #[error("the image format is not supported")]
     FormatNotSupported,

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

@@ -35,15 +35,12 @@ use ruma::{
 use tracing::{debug, info, Instrument, Span};
 
 use super::Room;
+#[cfg(feature = "image-proc")]
+use crate::{attachment::generate_image_thumbnail, error::ImageError};
 use crate::{
     attachment::AttachmentConfig, utils::IntoRawMessageLikeEventContent, Result,
     TransmissionProgress,
 };
-#[cfg(feature = "image-proc")]
-use crate::{
-    attachment::{generate_image_thumbnail, Thumbnail},
-    error::ImageError,
-};
 
 /// Future returned by [`Room::send`].
 #[allow(missing_debug_implementations)]
@@ -275,8 +272,6 @@ impl<'a> IntoFuture for SendAttachment<'a> {
                 #[cfg(not(feature = "image-proc"))]
                 let thumbnail = None;
 
-                #[cfg(feature = "image-proc")]
-                let data_slot;
                 #[cfg(feature = "image-proc")]
                 let (data, thumbnail) = if config.generate_thumbnail {
                     let content_type = content_type.clone();
@@ -285,6 +280,7 @@ impl<'a> IntoFuture for SendAttachment<'a> {
                             &content_type,
                             Cursor::new(&data),
                             config.thumbnail_size,
+                            config.thumbnail_format,
                         );
                         (data, res)
                     };
@@ -298,19 +294,15 @@ impl<'a> IntoFuture for SendAttachment<'a> {
                     let (data, res) = make_thumbnail(data);
 
                     let thumbnail = match res {
-                        Ok((thumbnail_data, thumbnail_info)) => {
-                            data_slot = thumbnail_data;
-                            Some(Thumbnail {
-                                data: data_slot,
-                                content_type: mime::IMAGE_JPEG,
-                                info: Some(thumbnail_info),
-                            })
+                        Ok(thumbnail) => Some(thumbnail),
+                        Err(error) => {
+                            if matches!(error, ImageError::ThumbnailBiggerThanOriginal) {
+                                debug!("Not generating thumbnail: {error}");
+                            } else {
+                                tracing::warn!("Failed to generate thumbnail: {error}");
+                            }
+                            None
                         }
-                        Err(
-                            ImageError::ThumbnailBiggerThanOriginal
-                            | ImageError::FormatNotSupported,
-                        ) => None,
-                        Err(error) => return Err(error.into()),
                     };
 
                     (data, thumbnail)
@@ -329,6 +321,8 @@ impl<'a> IntoFuture for SendAttachment<'a> {
                     generate_thumbnail: false,
                     #[cfg(feature = "image-proc")]
                     thumbnail_size: None,
+                    #[cfg(feature = "image-proc")]
+                    thumbnail_format: Default::default(),
                 };
 
                 room.prepare_and_send_attachment(