Skip to content
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

expose cairo_image_surface_create_for_data #252

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mildsunrise
Copy link
Contributor

node-gtk is really great, thank you 💙

AFAIK, the only current way to create an ImageSurface is from a PNG, and it has to be a file in the disk.
This PR exposes the cairo_image_surface_create_for_data function.
I would be really grateful if we could make it happen, since that would allow:

  • Retrieving the image from other sources, i.e. http, etc.
  • Working with other image formats (i.e. decode a JPEG image using a separate module, then use the resulting pixels)
  • Painting directly onto user's buffer.
  • Collecting pixel data from other surfaces (i.e. to implement screenshot functionality).
  • Rendering a user's buffer into a widget.

The function is low-level (the user is responsible of allocating a large enough buffer for their width, height & format; the user is responsible of holding that buffer alive as long as the image surface is used; etc.)

@mildsunrise
Copy link
Contributor Author

Example of use (for any user who lands here):

const width = 16, height = 16
const buf = new Uint32Array(width * height)
// set pixels to a greenish color
buf.fill(0x22AA00)
// create an ImageSurface operating on buf
const surface = Cairo.ImageSurface.createForData(buf, Cairo.Format.RGB24, width, height, width*4)
// make sure the buffer doesn't get destroyed while surface is alive
surface.buffer = buf

@romgrk
Copy link
Owner

romgrk commented Jan 6, 2021

The function is low-level (the user is responsible of allocating a large enough buffer for their width, height & format; the user is responsible of holding that buffer alive as long as the image surface is used; etc.)

Yes, that's fine, node-gtk already exposes a lot of low-level stuff, calling a C library comes with C responsabilities :) OTOH it would be possible to add an override for createForData to retain a reference to buffer, if we're pretty sure that the lifetime of buffer will always be at least as long as surface. LMK if you have an opinion on this, but I'd be ok to pass the responsability to the user.

And the use-case makes sense, I can't remember why I left out the function when I did the cairo bindings. Sorry you had to go through the JS generators btw, it's a bit messy but Cairo isn't GObject-introspected so essentially it's a second set of bindings just for cairo -_-

One thing, can you add a test for this use-case in tests/cairo/surface.js? The example with comments you have right there would do just fine, tests are a nice way to document API usage.

Travis isn't linking to GH again so here is the link for CI: https://travis-ci.org/github/romgrk/node-gtk/builds/753106331
The macOS build failures seem to be unrelated, they can be ignore. Need to switch to GHA soon anyway.

@mildsunrise
Copy link
Contributor Author

And the use-case makes sense, I can't remember why I left out the function when I did the cairo bindings. Sorry you had to go through the JS generators btw, it's a bit messy but Cairo isn't GObject-introspected so essentially it's a second set of bindings just for cairo -_-

Don't worry! Fortunately it only needed a small change :)

OTOH it would be possible to add an override for createForData to retain a reference to buffer, if we're pretty sure that the lifetime of buffer will always be at least as long as surface. LMK if you have an opinion on this, but I'd be ok to pass the responsability to the user.

Retaining a reference to the backing buffer would be a good option, yes. On the Node.js side this makes sense, backing buffers AFAIK should own their memory and manage its lifetime.

But to do this correctly (since other things can hold a reference to the surface in addition to our bindings), we should attach the reference to the surface itself. According to the docs we should use cairo_surface_set_user_data together with a destroy callback. Do you know how we would add an override on C++?

One thing, can you add a test for this use-case in tests/cairo/surface.js? The example with comments you have right there would do just fine, tests are a nice way to document API usage.

Will do!

@romgrk
Copy link
Owner

romgrk commented Jan 14, 2021

But to do this correctly (since other things can hold a reference to the surface in addition to our bindings), we should attach the reference to the surface itself. According to the docs we should use cairo_surface_set_user_data together with a destroy callback. Do you know how we would add an override on C++?

Unfortunately I haven't set up C++ overrides. For Cairo however it's possible to add an hand-written methods in the automatically-generated bindings (example). There is no mechanism in place to preserve those modifications if the generators are run again but they're rarely run and I just discard the git diff manually for those regions when I do.
Not the best setup to be frank but the cairo project is dead/dying from what I've understood so I try to not invest too much time in there. The project still does what it does well so it's fine to maintain good quality bindings but it's safe to assume they won't change too much.

mildsunrise added a commit to mildsunrise/ilo-protocol that referenced this pull request Jan 17, 2021
rendering works perfectly, keyboard input works perfectly,
it's lightweight, and I'm happy

NOTE: this depends on romgrk/node-gtk#252
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants