Implement Send for Image

[ids1024]

This changes the assumptions of the crate to assert that a valid pixmap::Image has a ref-count of 1, with an alpha map that has a ref-count of 1. Consequently, set_alpha_map must take ownership of its argument.

My understanding, looking at the pixmap code, is that this should ensure it is safe to Send the Image to another thread. Of course this does limit the API of this crate if a consumer wants to use multiple references (though pixman_image_ref wasn't wrapper already), or wants to use set_alpha_map while still keeping a reference to the alpha map. The first issue can just be solved by wrapping in Rc, though.

Smithay/smithay#1500

[Drakulix]

Perhaps also adding a comment that increasing the ref-count by by-passing pixman-rs is breaking Send?

[cmeissl]

Okay, I also had to re-read some stuff in the pixman sources.

Do we really have to change the signature to take ownership? Not sure this does indeed solve anything.
The most obvious reason why pixman is not thread-safe is the integer based ref-counting. While this wrapper
does make use of pixman_image_ref the ref-count might still be incremented through other functions like
for example pixman_image_set_alpha_map.
The current signature of set_alpha_map already borrows the passed alpha map until the image is either dropped
or clear_alpha_map is called. So this should already prevent sending the image to a different thread while keeping the
alpha image on a different one.
With scoped threads it would be possible to send the image to a different thread, but as this guarantees that the thread
is finished before the end of the scope it should still be valid.

So maybe just marking the ImageRef Send is already good enough?

(iirc pixman has some tls based cache, so possible that this might interfere somehow with Send)

[ids1024]

The current signature of set_alpha_map already borrows the passed alpha map until the image is either dropped
or clear_alpha_map is called. So this should already prevent sending the image to a different thread while keeping the
alpha image on a different one.

Ah, I guess I wasn't paying enough attention to the lifetimes there. That should also work.

With scoped threads it would be possible to send the image to a different thread, but as this guarantees that the thread
is finished before the end of the scope it should still be valid.

I wasn't thinking of scoped threads. We can't send &T to another thread unless T: Send. But I guess this could be a problem with the alpha map.

I think you could:

• Call set_alpha_map on an Image/ImageRef
• Send the image to a scoped thread
• Modify the alpha map image on the first thread

And this would be a thread safety issue, since the spawned thread might be be reading (in a non-thread-safe way) the same properties of the alpha map image as are being modified?

So maybe it needs to take ownership in the set_alpha_map method, or the methods that mutate state need to take an &mut

[ids1024]

So I guess at a minimum, set_alpha_map needs to take an &mut ref to the alpha map image, that will prevent it from being used (or dropped) until the image using it is dropped of replaces the alpha map. Transferring ownership is just more permanent way to get the same exclusivity (and would allow omitting any lifetime bounds).

[cmeissl]

Transferring ownership is just more permanent way to get the same exclusivity (and would allow omitting any lifetime bounds).

But at the cost of not allowing the same alpha map to be used with different images. I would really like to avoid this limitation if possible.

I think you could:

• Call set_alpha_map on an Image/ImageRef
• Send the image to a scoped thread
• Modify the alpha map image on the first thread

And this would be a thread safety issue, since the spawned thread might be be reading (in a non-thread-safe way) the > same properties of the alpha map image as are being modified?

I don't think this would be possible, the scope guarantees that all threads are finished. Moving the alpha image to a second thread in the same scope should be prevented by the borrow checker. So imo this should be safe.

[ids1024]

Here's an example of what I mean:

fn main() {
    let alpha_map_image = pixman::Image::new(pixman::FormatCode::A1, 1920, 1080, true).unwrap();

    std::thread::scope(|s| {
        let mut image1 =
            pixman::Image::new(pixman::FormatCode::R8G8B8A8, 1920, 1080, true).unwrap();
        image1 = image1.set_alpha_map(&alpha_map_image, 0, 0);

        let mut image2 =
            pixman::Image::new(pixman::FormatCode::R8G8B8A8, 1920, 1080, true).unwrap();
        image2 = image2.set_alpha_map(&alpha_map_image, 0, 0);

        s.spawn(move || {
            dbg!(image1);
        });
        s.spawn(move || {
            dbg!(image2);
        });

        alpha_map_image.add_triangles((0, 0), &[pixman::Triangle::new((0, 0), (0, 1), (1, 0))]);
    });
}

A couple things:

• Presumably methods like add_triangles mutate the image, and could conflict unsoundly with the thread trying to read the alpha image? Probably the method should take &mut self.
• Something we can't address so easily: the spawned threads here should decrement the reference could of the alpha image when they drop image1 and image2. But this is unsound if the pixman doesn't internally use thread-safe atomic reference counting?

[cmeissl]

Here's an example of what I mean:

fn main() {
    let alpha_map_image = pixman::Image::new(pixman::FormatCode::A1, 1920, 1080, true).unwrap();

    std::thread::scope(|s| {
        let mut image1 =
            pixman::Image::new(pixman::FormatCode::R8G8B8A8, 1920, 1080, true).unwrap();
        image1 = image1.set_alpha_map(&alpha_map_image, 0, 0);

        let mut image2 =
            pixman::Image::new(pixman::FormatCode::R8G8B8A8, 1920, 1080, true).unwrap();
        image2 = image2.set_alpha_map(&alpha_map_image, 0, 0);

        s.spawn(move || {
            dbg!(image1);
        });
        s.spawn(move || {
            dbg!(image2);
        });

        alpha_map_image.add_triangles((0, 0), &[pixman::Triangle::new((0, 0), (0, 1), (1, 0))]);
    });
}

A couple things:

* Presumably methods like `add_triangles` mutate the image, and could conflict unsoundly with the thread trying to read the alpha image? Probably the method should take `&mut self`.

* Something we can't address so easily: the spawned threads here should decrement the reference could of the alpha image when they drop `image1` and `image2`. But this is unsound if the pixman doesn't internally use thread-safe atomic reference counting?

Yeah, right. Changing all functions that mutate the image to take &mut should solve the thread issues, except for the ref-counted drop.

pixman_image_unref internally will unref the alpha image:

	if (common->alpha_map)
	    pixman_image_unref ((pixman_image_t *)common->alpha_map);

I also fail to see how this could be encoded in rust today other then to limit the api by taking ownership of the alpha image :(

[ids1024]

I guess if there was a single global mutex that is locked any time an image is unrefed (I guess including setting / clearing the alpha map) that would would.

[ids1024]

Closing in favor of #19.