smithay icon indicating copy to clipboard operation
smithay copied to clipboard

`PixmanTexture` is not `Send`

Open ids1024 opened this issue 1 year ago • 2 comments

Might as well have an issue for this.

PixmanTexture not being Send makes it impossible to use with MemoryRenderBuffer (as I noticed in https://github.com/Smithay/smithay/pull/1497), and is otherwise awkward.

Looking at pixman's implementation, pixman_image_t is not thread safe since it is ref-counted. In a way that isn't thread-safe. pixman_image_ref isn't currently bound by the pixman crate. But it is used once internally in pixman: for pixman_image_set_alpha_map.

I think it should be safe to Send a pixman image that has a reference count of 1, and if it has an alpha map, that also has a reference count of 1.

I see a couple ways to achieve this:

  • PixmanTexture could unsafe impl Send. It can no longer implement From<pixman::Image<'static, 'static>>, but could offer an unsafe function that mentions the reference count requirement.
    • pixman doesn't provide accessors for the reference count or alpha map, so we can't assert those, to make it safe.
  • The pixman crate could do the same thing internally, instead. pixman::Image::set_alpha_map will have to be changed to take ownership of the alpha map (so the ref count goes up to two, then it drops the first reference). pixman::Image::from_ptr will have to mention the same restriction.
    • Undesirable if anything using pixman would want to make use of the reference counting. Though other than set_alpha_map, it could just wrap in an Rc, so it's not that bad.
      • It could also define a Send type and a cloneable non-Send type, but that gets a bit complicated.

ids1024 avatar Aug 09 '24 01:08 ids1024

It seems much cleaner to implement this in the pixman crate, if it's okay to have such a restriction there. So https://github.com/cmeissl/pixman-rs/pull/18 makes the changes I think are necessary for this.

ids1024 avatar Aug 09 '24 02:08 ids1024

Since PixmanRenderer internally contains a solid PixmanImage this also makes DrmCompositor non-Send, if renderer_pixman is enabled, as it prefers pixman for cursor images.

Drakulix avatar Nov 01 '24 15:11 Drakulix