New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mark `ImageSurface` and `PDFSurface` as `Send` #153

Merged
merged 1 commit into from Sep 23, 2017

Conversation

Projects
None yet
4 participants
@fengalin
Contributor

fengalin commented Sep 7, 2017

While following this guide, I found that Surface wasn't Send. I assume that if it is possible to share it between threads in C, it should also be possible in Rust.

@fengalin

This comment has been minimized.

Show comment
Hide comment
@fengalin

fengalin Sep 7, 2017

Contributor

Question: shouldn't cairo-sys-rs be part of gtk-rs/sys? gtk-rs/sys/gdk-sys and gtk-rs/sys/gtk-sys, depend on it. So when I modified cairo, I had to modify gtk-rs/sys to use my repository and ended up editing dependencies in: gio, glib, gtk, gdk, gdk-pixbuf, pango (and gstreamer and gstreamer-sys since my project also depends on gstreamer).

Contributor

fengalin commented Sep 7, 2017

Question: shouldn't cairo-sys-rs be part of gtk-rs/sys? gtk-rs/sys/gdk-sys and gtk-rs/sys/gtk-sys, depend on it. So when I modified cairo, I had to modify gtk-rs/sys to use my repository and ended up editing dependencies in: gio, glib, gtk, gdk, gdk-pixbuf, pango (and gstreamer and gstreamer-sys since my project also depends on gstreamer).

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Sep 7, 2017

Member

Question: shouldn't cairo-sys-rs be part of gtk-rs/sys?

No, it doesn't have a gir file so it's on its own.

Member

GuillaumeGomez commented Sep 7, 2017

Question: shouldn't cairo-sys-rs be part of gtk-rs/sys?

No, it doesn't have a gir file so it's on its own.

@fengalin

This comment has been minimized.

Show comment
Hide comment
@fengalin

fengalin Sep 11, 2017

Contributor

All right! What about a separate crate then? ;) IMHO, it feels strange to modify glib and all the crates that depend on it for a simple modification in cairo-rs.

Anyway, that was just a mere comment. What do you think about the PR?

Contributor

fengalin commented Sep 11, 2017

All right! What about a separate crate then? ;) IMHO, it feels strange to modify glib and all the crates that depend on it for a simple modification in cairo-rs.

Anyway, that was just a mere comment. What do you think about the PR?

fengalin added a commit to fengalin/media-toc that referenced this pull request Sep 15, 2017

@fengalin

This comment has been minimized.

Show comment
Hide comment
@fengalin

fengalin Sep 22, 2017

Contributor

@GuillaumeGomez what do you think about this PR?

Contributor

fengalin commented Sep 22, 2017

@GuillaumeGomez what do you think about this PR?

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Sep 23, 2017

Member

For me it seems fine but I'd prefer to have the confirmation from @EPashkin and @sdroege. :)

Member

GuillaumeGomez commented Sep 23, 2017

For me it seems fine but I'd prefer to have the confirmation from @EPashkin and @sdroege. :)

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Sep 23, 2017

Member

I don't think this is safe, and the Cairo documentation there is at least misleading and outdated. See also https://developer.gnome.org/gdk3/stable/gdk3-Threads.html#gdk-threads-enter , those functions are deprecated since years.

While it is safe to send Cairo surfaces backed by a threadsafe underlying surface to another thread, this is not generally the case. With X11 you can in theory make it work (but it's not supported by GDK/GTK anymore, see above). The image surface is also always fine, being backed by just some memory. With Windows you can completely forget it and can't make it work (and the code on the Cairo website never worked on Windows).

Member

sdroege commented Sep 23, 2017

I don't think this is safe, and the Cairo documentation there is at least misleading and outdated. See also https://developer.gnome.org/gdk3/stable/gdk3-Threads.html#gdk-threads-enter , those functions are deprecated since years.

While it is safe to send Cairo surfaces backed by a threadsafe underlying surface to another thread, this is not generally the case. With X11 you can in theory make it work (but it's not supported by GDK/GTK anymore, see above). The image surface is also always fine, being backed by just some memory. With Windows you can completely forget it and can't make it work (and the code on the Cairo website never worked on Windows).

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Sep 23, 2017

Member

tl;dr: This PR is wrong, but you can do something like this for image surfaces (as long as they are the only ones having access to the underlying memory!). And don't use GTK/GDK from multiple threads, ever :)

Member

sdroege commented Sep 23, 2017

tl;dr: This PR is wrong, but you can do something like this for image surfaces (as long as they are the only ones having access to the underlying memory!). And don't use GTK/GDK from multiple threads, ever :)

@fengalin

This comment has been minimized.

Show comment
Hide comment
@fengalin

fengalin Sep 23, 2017

Contributor

I don't use the deprecated functions you mentioned, the guide was just a source of inspiration. I'm implementing a double buffer mechanism, which by design must be multithreaded. Everything GTK related is done in the GTK thread. The double buffer mechanism is shared between threads, one doing the rendering using Cairo, the other being the GTK thread which handles drawing to a drawing area.

If this PR is wrong, how am I supposed to send my Cairo surface between the 2 threads?

Contributor

fengalin commented Sep 23, 2017

I don't use the deprecated functions you mentioned, the guide was just a source of inspiration. I'm implementing a double buffer mechanism, which by design must be multithreaded. Everything GTK related is done in the GTK thread. The double buffer mechanism is shared between threads, one doing the rendering using Cairo, the other being the GTK thread which handles drawing to a drawing area.

If this PR is wrong, how am I supposed to send my Cairo surface between the 2 threads?

@fengalin

This comment has been minimized.

Show comment
Hide comment
@fengalin

fengalin Sep 23, 2017

Contributor

Or are you saying that I should find a different design to work this around because this modification could be unsafe in an uncontrolled environment?

Contributor

fengalin commented Sep 23, 2017

Or are you saying that I should find a different design to work this around because this modification could be unsafe in an uncontrolled environment?

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Sep 23, 2017

Member

What kind of surface are you using for your double buffering implementation?

Member

sdroege commented Sep 23, 2017

What kind of surface are you using for your double buffering implementation?

@fengalin

This comment has been minimized.

Show comment
Hide comment
@fengalin

fengalin Sep 23, 2017

Contributor

I'm using cairo::ImageSurface. I think I finally got your first comment. Can I safely set the Send trait on cairo::ImageSurface instead of the general cairo::Surface?

Contributor

fengalin commented Sep 23, 2017

I'm using cairo::ImageSurface. I think I finally got your first comment. Can I safely set the Send trait on cairo::ImageSurface instead of the general cairo::Surface?

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Sep 23, 2017

Member

Yes, also the PNG, PDF, SVG surfaces should be safe to mark Send (but not Sync).

Member

sdroege commented Sep 23, 2017

Yes, also the PNG, PDF, SVG surfaces should be safe to mark Send (but not Sync).

@fengalin

This comment has been minimized.

Show comment
Hide comment
@fengalin

fengalin Sep 23, 2017

Contributor

Ok! Thanks

Contributor

fengalin commented Sep 23, 2017

Ok! Thanks

@fengalin fengalin changed the title from Mark `Surface` as `Send` to Mark `ImageSurface` and `PDFSurface` as `Send` Sep 23, 2017

@fengalin

This comment has been minimized.

Show comment
Hide comment
@fengalin

fengalin Sep 23, 2017

Contributor

Update since last commit:

  • Send removed from Surface
  • Send added to ImageSurface and PDFSurface

PNGSurface isn't a dedicated type, there is a convenience function PNGSurface::create_from_png that builds an ImageSurface using cairo_image_surface_create_from_png_stream.

SVGSurface is not available in gtk-rs/cairo yet.

Contributor

fengalin commented Sep 23, 2017

Update since last commit:

  • Send removed from Surface
  • Send added to ImageSurface and PDFSurface

PNGSurface isn't a dedicated type, there is a convenience function PNGSurface::create_from_png that builds an ImageSurface using cairo_image_surface_create_from_png_stream.

SVGSurface is not available in gtk-rs/cairo yet.

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Sep 23, 2017

Member

👍 for this now.

Member

EPashkin commented Sep 23, 2017

👍 for this now.

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Sep 23, 2017

Member

@sdroege: Good for you as well?

Member

GuillaumeGomez commented Sep 23, 2017

@sdroege: Good for you as well?

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Sep 23, 2017

Member

Sure, that's exactly what I wrote before :)

Member

sdroege commented Sep 23, 2017

Sure, that's exactly what I wrote before :)

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Sep 23, 2017

Member

Then let's merge. Thanks!

Member

GuillaumeGomez commented Sep 23, 2017

Then let's merge. Thanks!

@GuillaumeGomez GuillaumeGomez merged commit 911c1fe into gtk-rs:master Sep 23, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@fengalin fengalin deleted the fengalin:surface_send branch Sep 23, 2017

fengalin added a commit to fengalin/media-toc that referenced this pull request Sep 26, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment