Image is no longer Send in 0.3.0

[Be-ing]

trafficstars

Image now uses internal data structures that do not implement Send. This broke upgrading my application from 0.2.5 to 0.3.0 because I was generating an Image in a UI callback then moving it into an upgrade_in_event_loop callback. I hacked around this by passing a SharedPixelBuffer<Rgba8Pixel> from the UI callback to the event loop then calling Image::from_rgba8_premultiplied in the event loop.

moire on  upgrade [$!] via 🦀 v1.63.0 took 3s 
 0% ❯ cargo build
   Compiling moire-ui v0.0.0 (/home/be/sw/moire/crates/ui)
error[E0277]: `Rc<RefCell<rctree::NodeData<usvg::NodeKind>>>` cannot be sent between threads safely
   --> crates/ui/src/deck.rs:196:25
    |
196 |             main_window.upgrade_in_event_loop(move |main_window| {
    |                         ^^^^^^^^^^^^^^^^^^^^^ `Rc<RefCell<rctree::NodeData<usvg::NodeKind>>>` cannot be sent between threads safely
    |
    = help: within `i_slint_core::graphics::image::svg::ParsedSVG`, the trait `Send` is not implemented for `Rc<RefCell<rctree::NodeData<usvg::NodeKind>>>`
    = note: required because it appears within the type `rctree::Node<usvg::NodeKind>`
    = note: required because it appears within the type `usvg::Tree`
    = note: required because it appears within the type `i_slint_core::graphics::image::svg::ParsedSVG`
    = note: required because of the requirements on the impl of `Send` for `VRc<OpaqueImageVTable, i_slint_core::graphics::image::svg::ParsedSVG>`
    = note: required because it appears within the type `ImageInner`
    = note: required because it appears within the type `slint::Image`
note: required because it's used within this closure
   --> crates/ui/src/deck.rs:196:47
    |
196 |               main_window.upgrade_in_event_loop(move |main_window| {
    |  _______________________________________________^
197 | |                 // FIXME: Is the deck still supposed to load the track that has now
198 | |                 // become available, i.e. is a load operation for the corresponding
199 | |                 // track uid still pending? Otherwise we should abort this operation
...   |
241 | |                     .expect("GUI to audio channel full!");
242 | |             }).expect("Failed to load file");
    | |_____________^
note: required by a bound in `slint::Weak::<T>::upgrade_in_event_loop`
   --> /home/be/.cargo/registry/src/github.com-1ecc6299db9ec823/i-slint-core-0.3.0/api.rs:673:36
    |
673 |             func: impl FnOnce(T) + Send + 'static,
    |                                    ^^^^ required by this bound in `slint::Weak::<T>::upgrade_in_event_loop`

error[E0277]: `Rc<RefCell<rctree::NodeData<usvg::NodeKind>>>` cannot be shared between threads safely
   --> crates/ui/src/deck.rs:196:25
    |
196 |             main_window.upgrade_in_event_loop(move |main_window| {
    |                         ^^^^^^^^^^^^^^^^^^^^^ `Rc<RefCell<rctree::NodeData<usvg::NodeKind>>>` cannot be shared between threads safely
    |
    = help: within `i_slint_core::graphics::image::svg::ParsedSVG`, the trait `Sync` is not implemented for `Rc<RefCell<rctree::NodeData<usvg::NodeKind>>>`
    = note: required because it appears within the type `rctree::Node<usvg::NodeKind>`
    = note: required because it appears within the type `usvg::Tree`
    = note: required because it appears within the type `i_slint_core::graphics::image::svg::ParsedSVG`
    = note: required because of the requirements on the impl of `Send` for `VRc<OpaqueImageVTable, i_slint_core::graphics::image::svg::ParsedSVG>`
    = note: required because it appears within the type `ImageInner`
    = note: required because it appears within the type `slint::Image`
note: required because it's used within this closure
   --> crates/ui/src/deck.rs:196:47
    |
196 |               main_window.upgrade_in_event_loop(move |main_window| {
    |  _______________________________________________^
197 | |                 // FIXME: Is the deck still supposed to load the track that has now
198 | |                 // become available, i.e. is a load operation for the corresponding
199 | |                 // track uid still pending? Otherwise we should abort this operation
...   |
241 | |                     .expect("GUI to audio channel full!");
242 | |             }).expect("Failed to load file");
    | |_____________^
note: required by a bound in `slint::Weak::<T>::upgrade_in_event_loop`
   --> /home/be/.cargo/registry/src/github.com-1ecc6299db9ec823/i-slint-core-0.3.0/api.rs:673:36
    |
673 |             func: impl FnOnce(T) + Send + 'static,
    |                                    ^^^^ required by this bound in `slint::Weak::<T>::upgrade_in_event_loop`

error[E0277]: `NonNull<i_slint_core::sharedvector::SharedVectorInner<u8>>` cannot be shared between threads safely
   --> crates/ui/src/deck.rs:196:25
    |
196 |             main_window.upgrade_in_event_loop(move |main_window| {
    |                         ^^^^^^^^^^^^^^^^^^^^^ `NonNull<i_slint_core::sharedvector::SharedVectorInner<u8>>` cannot be shared between threads safely
    |
    = help: within `i_slint_core::graphics::image::svg::ParsedSVG`, the trait `Sync` is not implemented for `NonNull<i_slint_core::sharedvector::SharedVectorInner<u8>>`
    = note: required because it appears within the type `SharedVector<u8>`
    = note: required because it appears within the type `SharedString`
    = note: required because it appears within the type `ImageCacheKey`
    = note: required because it appears within the type `i_slint_core::graphics::image::svg::ParsedSVG`
    = note: required because of the requirements on the impl of `Send` for `VRc<OpaqueImageVTable, i_slint_core::graphics::image::svg::ParsedSVG>`
    = note: required because it appears within the type `ImageInner`
    = note: required because it appears within the type `slint::Image`
note: required because it's used within this closure
   --> crates/ui/src/deck.rs:196:47
    |
196 |               main_window.upgrade_in_event_loop(move |main_window| {
    |  _______________________________________________^
197 | |                 // FIXME: Is the deck still supposed to load the track that has now
198 | |                 // become available, i.e. is a load operation for the corresponding
199 | |                 // track uid still pending? Otherwise we should abort this operation
...   |
241 | |                     .expect("GUI to audio channel full!");
242 | |             }).expect("Failed to load file");
    | |_____________^
note: required by a bound in `slint::Weak::<T>::upgrade_in_event_loop`
   --> /home/be/.cargo/registry/src/github.com-1ecc6299db9ec823/i-slint-core-0.3.0/api.rs:673:36
    |
673 |             func: impl FnOnce(T) + Send + 'static,
    |                                    ^^^^ required by this bound in `slint::Weak::<T>::upgrade_in_event_loop`

For more information about this error, try `rustc --explain E0277`.
error: could not compile `moire-ui` due to 3 previous errors

Perhaps cargo-semver-checks could have caught this.

[tronical]

The semver checks are a very good idea! I recall when this made the rounds on Reddit. We should have tried to run it before the release :(

With regards to the Image type: we’ve changed the implementation to use cross-platform image decoders and provide a backend-independent cache. As usual, this cache is thread local and that’s one thing that’ll make it harder.

We could perhaps use send wrapper for the variants that aren’t send but I’m not sure this is the right path forward and it makes the Image type heavier.

I think the approach of sending the shared pixel buffer and therefore a very concrete type is better than letting a very opaque type be send. I’d say it was send by accident.

What do you think?

[Be-ing]

Perhaps add something like this to the Image documentation could help?

Note that Image does not implement Send. If you need to send an image across threads, send a Path or SharedPixelBuffer across threads instead, then construct the Image from that using one of Image's methods.

[tronical]

Excellent! Can you make a PR or shall I?

[Be-ing]

I can do that.

[ansingh]

Ran into this while trying to implement a demo app showing thumbnails in a list. I wanted to load the images in a background thread since loading them blocks the app otherwise.

I obviously can't send path, and sending SharedPixelBuffer isn't ideal either - it requires you to implement caching, decoding yourself along with adjusting for any changes in view. It seems very limiting.

[ogoffart]

Why is sending the SharedPixelBuffer not ideal?

[ansingh]

I already listed my reasons. If you go that route - you forego the convenience of letting your UI lib handle the format decoding, component resizing and caching. Loading the image in background is a fairly common use case with medium to large images, doing that should ideally be fairly straightforward.

In QML, Image has an asynchronous property to help with that.

[ogoffart]

The thing is that the Image uses internal caches that are thread-local. You can't really upload the images to the GPU in a different thread anyway. Yes, this means that the loading and decoding currently happen in the GUI thread, which is not optimal. We could try to improve that by having our code more asynchronous. I think for your use case you may want to go with the approach of loading the SharedPixelBuffer in the thread. Note that before 0.3, when image was Send, we were doing the loading lazily in the main thread anyway.

Overall, i'd say it would still be good if we can make Image Send again, and even try to offload all the image loading on a thread.

[tronical]

@ansign If Image were send, what function would you use for construction in your other thread?

I do agree with Olivier that multi-threaded, asynchronous image decoding could (and should?) be a built-in feature of Slint itself, should not be something users have to do. API wise that would mean construction from a path, but we could also add the option of constructing a slint::Image from a buffer that holds the encoded image.

[ansingh]

I would have been using path/url in my case, but a constructor with encoded data buffer is also a great idea.

Since we cannot use a non-literal path in .slint's Image component directly; we have to pass slint::image object from rust code for all dynamically loaded images. This creates another issue when we need to pass a list of images from rust code to a .slint component. A non send Image means that you cannot create the final vec in the background thread, you have to create the slint::Image and the vec containing it in the GUI thread when you need to pass a list of images.

I think in general, anything that is necessary (without alternate) to be passed to .slint files should be send for the sake of convenience. Asynchronous loading would be very nice too.

[tronical]

Thanks. I agree. API wise it makes sense that slint::Image should be Send, and internally any fields that refer to thread-local data (like the cache key) could be guarded with a std::thread::ThreadId, like we do in slint::Weak.

[uklotzde]

A non send Image means that you cannot create the final vec in the background thread, you have to create the slint::Image and the vec containing it in the GUI thread when you need to pass a list of images.

This becomes even more clumsy when using dynamically loaded image content as a member of an exported struct. It requires to create a redundant companion type that can be populated in another thread before being moved into the thread context of the event loop. The subsequent transformation in the event loop requires a reallocation when used in a Vec or any other composite type.

An example could be found here: https://codeberg.org/moire/moire/src/commit/4cc53b1e37bcf09dd4929b5dddfd3825579a446b/crates/ui/src/tasklet.rs#L159

[uklotzde]

Maybe as single, all-embracing image type is not sufficient? Separation of concerns or another level of indirection has often served me well to resolve contradictory requirements.

[uklotzde]

How about images as a property in a struct? This is still an obstacle and breaks the concept of seamless integration between generated and custom code.

I don't consider this issue as resolved. Instructions on how to handle the image type as a property in structs are still missing.

[uklotzde]

Simple example: List with items that contain both an image and a text.