For the get_pixels() case yeah, you'd have to copy it and then create a new one.

Which leads to the new_from_data() case - what we want here is _new_from_bytes().  This gdk-pixbuf API long predates the existence of GBytes, and introspection isn't aware of the fact that the destroy notify applies to the array parameter.

I started looking at this a bit more, but a core issue is GdkPixbuf allows effectively casting const data to a mutable pointer.

gdk_pixbuf_new_from_data() takes a const guint8*data parameter, and that same pointer can be returned via gdk_pixbuf_get_pixels() as guchar*.

Looking at _gdk_pixbuf_new_from_resource_try_mmap which takes input from a GResource which may be a a read-only mmap store, AFAICS anyone whose code tries mutating it will just segv.

I could easily provide an API to create from generic GBytes which would have similar semantics, but should we try having something like a "readonly" property?  Then we could cause gdk_pixbuf_get_pixels() to return NULL or so.

Anyone who wanted to change it would have to gdk_pixbuf_copy().

Created attachment 279951 [details] [review]
Add _new_from_bytes() and _read_pixels() API, handle read-only pixbufs

GdkPixbuf is an old API that predates introspection and GBytes.  It
has some confusion around whether or not pixbuf data is mutable or
not.  The _new_from_data() API takes a *const* pointer, but it's not
copied, and _get_pixels() returns a non-const copy of the same
pointer.

There are several cases where we get read-only data, such as a
GResource.  For language bindings, _new_from_data() doesn't work
because the array may be a temporary copy only allocated for the call.

In order to support a clean _new_from_bytes() API, we need to add the
concept of a read-only pixbuf into the core.  The fundamental hack
here is that _get_pixels() now causes an implicit copy.

For the cases where we don't want to copy, add a new
gdk_pixbuf_read_pixels() that returns a proper const pointer.

Note I did not change _gdk_pixbuf_new_from_resource_try_mmap() yet in this patch, but it can come after.

See also: bug 721497
The patch over there takes a different approach in that it uses  gdk_pixbuf_new_from_data with a custom destroy func to do the un-ref.

Should combine the two; creating both from readonly and mutable arrays makes sense.

Review of attachment 279951 [details] [review]:

Looks good. At first I was a little thrown off by the idea of having to deal with both bytes and pixels but I think that detail was abstracted nicely with get_pixels and read_pixels. I don't know the code base very well but I take it the various IO modules (io-png.c, io-tga.c, etc...) are ok to use "->pixels" directly?

I think tests should be added to check at least a few of the rotate/scale functions that convert a Pixbuf from immutable to mutable.

::: gdk-pixbuf/gdk-pixbuf-data.c
@@ +95,3 @@
+ * @rowstride: Distance in bytes between row starts
+ * 
+ * @has_alpha: Whether the data has an opacity channel

It might also be a good idea to note that the various mutating functions (scale/rotate) will force a copy and no longer use the bytes passed in here?

@@ +109,3 @@
+	g_return_val_if_fail (bits_per_sample == 8, NULL);
+	g_return_val_if_fail (width > 0, NULL);
+ * Currently only RGB images with 8 bits per sample are supported.

What about validating the GBytes length with something like:
   g_return_val_if_fail (g_bytes_get_size (data) == width * height * (has_alpha ? 4 : 3), NULL);
(I did this with the patch for g_pixbuf_new_from_array)

Created attachment 280090 [details] [review]
Add _new_from_bytes() and _read_pixels() API, handle read-only pixbufs

* Updated for comments
* Added a test case

The I/O modules are OK, basically relying on the fact that the default
is to malloc into ->pixels.

Review of attachment 280090 [details] [review]:

Thanks for the educational test! So writing to the read-only mem-mapped buffer will cause a segfault?

This is basically accepted-commit_now as far as I'm concerned (given the spelling problem is fixed for non-unix platforms). But is that ok given I'm not a GTK+ maintainer?

::: tests/pixbuf-readonly-to-mutable.c
@@ +83,3 @@
+  }
+#else
+  bytes = g_bytes_new (gdk_pixbuf_get_pixels (ref), gdk_pixbuf_get_byte_length (reference));

I think this should be "reference" not "ref".

Yep, if you want to see a simpler example, try:

#include <gio/gio.h>
#include <sys/mman.h>
#include <unistd.h>

int
main (int argc, char **argv)
{
  int pagesize;
  int r;
  guint8 *buf;

  pagesize = sysconf (_SC_PAGE_SIZE);
  g_assert_cmpint (pagesize, >, 0);
  buf = mmap (NULL, pagesize, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
  g_assert (buf != NULL);

  buf[0] = 'X';

  g_print ("Wrote %c\n", buf[0]);

  r = mprotect (buf, pagesize, PROT_READ);
  g_assert_cmpint (r, ==, 0);

  g_print ("Read %c\n", buf[0]);
  buf[0] = 'Y';

  g_print ("Wrote Y\n");

  return 0;
}

Created attachment 280098 [details] [review]
Add _new_from_bytes() and _read_pixels() API, handle read-only pixbufs

First, s/ref/reference per review.

Also, I noticed something while reading the mutation code again;
several of the scale/composite APIs allow src and dest to be the same.
In that case, we need to call _get_pixels() *before* read_pixels(),
otherwise the latter will point to a freed location.

Add a test case which triggered this with the old ordering.

Review of attachment 280090 [details] [review]:

Looks basically ok. It would be nice to have some more test coverage of readonly pixbufs and implicit copies (ie create a readonly pixbuf, scale it, observe implicit copy, etc). I also guess we should have a followup patch for GTK+ to do s/get_pixels/read_pixels/ where appropriate ?

If we land this, we should bump the version to 2.31

One thing I notice is that you add a read_pixels that doesn't return a length. We had to add a get_pixels_with_length at some point to satisfy bindings - do we not need the same here ?

(In reply to comment #12)
> Review of attachment 280090 [details] [review]:
> 
> Looks basically ok. It would be nice to have some more test coverage of
> readonly pixbufs and implicit copies (ie create a readonly pixbuf, scale it,
> observe implicit copy, etc). 

Should be in the patch in comment #11.

> I also guess we should have a followup patch for
> GTK+ to do s/get_pixels/read_pixels/ where appropriate ?

Yes.

(In reply to comment #13)
> One thing I notice is that you add a read_pixels that doesn't return a length.
> We had to add a get_pixels_with_length at some point to satisfy bindings - do
> we not need the same here ?

This is an interesting question; it comes down to whether or not bindings create language arrays which can be backed by C arrays.  gjs does support this.  Not sure what pygobject does.

Anyways so we should provide a GBytes accessor.  New patch coming.

Created attachment 280666 [details] [review]
Add gdk_pixbuf_read_pixel_bytes()

This can be convenient for language bindings to access the readonly
data in a form that includes length, and also avoids a copy (for
readonly pixbufs).

Is it confusing that get_pixels is now documented as 'makes a copy', read_pixels as 'doesn't copy', and 'read_pixel_bytes as 'does a one-time copy' ? Or is that just me ?

(In reply to comment #17)
> Is it confusing that get_pixels is now documented as 'makes a copy',
> read_pixels as 'doesn't copy', and 'read_pixel_bytes as 'does a one-time copy'
> ? Or is that just me ?

read_pixel_bytes() only does a copy if the source was not originally readonly.

That aside, yeah the whole thing is confusing, but I think this approach is the best solution for getting out of the current mess.

Attachment 280098 [details] pushed as 862e389 - Add _new_from_bytes() and _read_pixels() API, handle read-only pixbufs
Attachment 280666 [details] pushed as 3b40f1e - Add gdk_pixbuf_read_pixel_bytes()