GNOME Bugzilla – Bug 721497
Add binding friendly version of gdk_pixbuf_new_from_data
Last modified: 2018-06-17 14:32:57 UTC
The following python code segfaults at last line, when (W * H) is larger than 32768. This bug makes get_pixels() useless for creating modified Pixbuf. # Reproduce code from gi.repository import GdkPixbuf W = 1 H = 32769 pixbuf = GdkPixbuf.Pixbuf.new(GdkPixbuf.Colorspace.RGB, True, 8, W, H) pixels = pixbuf.get_pixels() new_pixbuf = GdkPixbuf.Pixbuf.new_from_data(pixels, GdkPixbuf.Colorspace.RGB, True, 8, pixbuf.get_width(), pixbuf.get_height(), pixbuf.get_rowstride(), None, None) new_pixels = new_pixbuf.get_pixels()
The issue is that gdk_pixbuf_new_from_data is not binding friendly. The "data" argument is marked as transfer-none, which means PyGObject passes the function a temporary array and expects gdk_pixbuf_new_from_data to make a copy (or only use the data for the duration of the call) because PyGObject will free this data after the call completes. The function itself is actually expecting the data to stay valid until it calls the "destroy_fn" which cannot work as a pure Python callback. So we need a new version of the function which accepts a GArray and manages the destroy_fn.
Created attachment 265383 [details] [review] Add gdk_pixbuf_new_from_array for GI language bindings Add a language binding friendly version of gdk_pixbuf_new_from_data. This accepts a GArray, the lifetime of which is managed by the resulting pixbuf object. Added rudimentary test and verified we do not leak with valgrind.
Also note that I opted to use GArray instead of GBytes. GArray could be used with color components greater than 8 bits (bits_per_sample) in the future if that is ever supported by using the GArray element-size. In any event bindings deal with both GArray and GBytes the same way.
(In reply to comment #2) The patch doesn't work for me. The lifetime of the data pointed by array->data is not managed by the resulting pixbuf object. PyGObject will free the data after the call completes.(The lifetime of array->data is independent from the lifetime of array.)
(In reply to comment #4) > (In reply to comment #2) > The patch doesn't work for me. > > The lifetime of the data pointed by array->data is not managed by the resulting > pixbuf object. PyGObject will free the data after the call completes.(The > lifetime of array->data is independent from the lifetime of array.) It should be managed by both PyGObject marshaling and the pixbuf via reference counting. Are you using PyGObject >= 3.11.1? https://git.gnome.org/browse/pygobject/commit/?id=fe217e0afbd6
Review of attachment 265383 [details] [review]: One question, and one minor comment. Otherwise looks good. ::: gdk-pixbuf/gdk-pixbuf-core.h @@ +316,3 @@ gpointer destroy_fn_data); +GdkPixbuf *gdk_pixbuf_new_from_array (GArray *array, Why not a GByteArray? ::: tests/pixbuf-new.c @@ +27,3 @@ + /* Fill array with numbers 0..width*height-1 */ + for (i = 0; i < image_size; i++) { + array->data[i] = i; data is "char"; I don't like implicit truncation. Can you add (char) i ?
See https://bugzilla.gnome.org/show_bug.cgi?id=732297 for the GBytes version.
(In reply to comment #6) > Review of attachment 265383 [details] [review]: > > One question, and one minor comment. Otherwise looks good. > > ::: gdk-pixbuf/gdk-pixbuf-core.h > @@ +316,3 @@ > gpointer destroy_fn_data); > > +GdkPixbuf *gdk_pixbuf_new_from_array (GArray *array, > > Why not a GByteArray? There are two reasons both of which could be argued against fairly easily: 1. These methods accept bits_per_sample which implies it supports or could eventually support larger than 8 bit per-color values. Using GArray might allow for more convenience for larger color size support in the future. If the answer to that is it'll never happen, than perhaps we should always just assume bits_per_sample is 8 and remove it from the function signature? 2. PyGObject doesn't implicitly convert to GBytes/GByteArray but it does for GArray. This just needs to be fixed in PyGObject (bug 729541).
bpp != 8 is unlikely to ever happen. Do you stil need anything here, or is gdk_pixbuf_new_from_bytes sufficient ?
GLib.Bytes.new_take(pixmap.samples) is slow like hell, please don't force it. I have to use the following function to work around a 2 years old bug with patches, damn. And the work around is slow. def pixmap2pixbuf(rgba_data, width, height): """convert RGBA data into pixbuf""" if not isinstance(rgba_data, bytearray): rgba_data = bytearray(rgba_data) del rgba_data[3::4] # strip alpha channel header = b"P6 %d %d 255\n" % (width, height) ploader = GdkPixbuf.PixbufLoader.new_with_type("pnm") ploader.write(header) ploader.write(rgba_data) ploader.close() pixbuf = ploader.get_pixbuf() return pixbuf
(In reply to Bug flys from comment #10) > GLib.Bytes.new_take(pixmap.samples) is slow like hell, please don't force it. How is it slow? Show us your code (and you probably shouldn't be using .new_take() as it makes a copy of the original buffer). > I have to use the following function to work around a 2 years old bug with > patches, damn. It's taken 2 years for somebody to comment. It seems that everybody else must be happy with using GBytes instead. > And the work around is slow. I can't imagine how that could ever be faster than using GBytes.
I had code like this: pixels = GLib.Bytes.new_take(pixmap.samples) pixbuf = GdkPixbuf.Pixbuf.new_from_bytes(pixels, GdkPixbuf.Colorspace.RGB, True, 8, pixmap.width, pixmap.height, pixmap.getSize()/pixmap.height) It's 20% slower than the above code, then again, maybe because the pixbuf loader version stripped 25% of alpha channel bytes out of the data.
nobody was happy, most of them just gave up when so much bugs with patches were ignored for years.
(In reply to Bug flys from comment #12) > I had code like this: > pixels = GLib.Bytes.new_take(pixmap.samples) > pixbuf = GdkPixbuf.Pixbuf.new_from_bytes(pixels, > GdkPixbuf.Colorspace.RGB, True, 8, > pixmap.width, pixmap.height, > pixmap.getSize()/pixmap.height) > > It's 20% slower than the above code, then again, maybe because the pixbuf > loader version stripped 25% of alpha channel bytes out of the data. Could I have the full source, or at least a test case. You likely don't want to use new_take unless that bit of data was allocated by a GLib backed function. Use new_static instead. (In reply to Bug flys from comment #13) > nobody was happy, most of them just gave up when so much bugs with patches > were ignored for years. using new_from_bytes() is functionally equivalent to what was requested in this bug. g_bytes_new_static() will put a pointer to the data and the length in a structure, and gdk_pixbuf_new_from_bytes() will read that data. It's functionally equivalent to new_from_data, except that the data and length are passed through the structure, instead of separately. (In reply to Bug flys from comment #13) > nobody was happy, most of them just gave up when so much bugs with patches > were ignored for years. There's been a fix for this bug (using new_from_bytes) for at least a year in stable. If new_from_bytes() is inadequate, please show us a test case so we can fix it. But as I said above, I don't see how this would be much slower than the code that was in the patch, unless you ended up doing extra copies. In short, please give a test case.
(In reply to Bug flys from comment #10) > GLib.Bytes.new_take(pixmap.samples) is slow like hell, please don't force it. > Thats just nonsense.
OK, after further test, it seems that the converting to GBytes was slow when the source data is in Python *bytearray*. If the source type is bytes, the overhead is small enough. In the code I wrote, the data I got was in bytearray. Here is a test code: import time data_b = b" "* 2**24 t0 = time.time() data_ba = bytearray(data_b) t1 = time.time() gbyte = GLib.Bytes.new(data_b) t2 = time.time() gbyte2 = GLib.Bytes.new(data_ba) t3 = time.time() print("data size:", len(data_b)) print("bytes -> bytearray:", t1 - t0) print("bytes -> GBytes: ", t2 - t1) print("bytearray -> GBytes: ", t3 - t2) So there are still bug some where for loading Python bytearray to GBytes. I was using Bytes.new_take() because I'd figured that since new_take() borrow the address, it might be faster. It did not help.
(In reply to Bug flys from comment #16) > OK, after further test, it seems that the converting to GBytes was slow when > the source data is in Python *bytearray*. Then that means that something in Python or the pygobject code might need fixes. Not a bug in gdk-pixbuf though. > If the source type is bytes, the > overhead is small enough. In the code I wrote, the data I got was in > bytearray. > > Here is a test code: It would be great if you could include *full* test code, not snippets. <snip> > > So there are still bug some where for loading Python bytearray to GBytes. > > I was using Bytes.new_take() because I'd figured that since new_take() borrow > the address, it might be faster. It did not help. It borrows the address, but expects the memory to be allocated with GLib memory allocation functions. https://developer.gnome.org/glib/stable/glib-Byte-Arrays.html#g-bytes-new-take Looks like it's not possible to use g_bytes_new_static() from Python, as it's skipped by the introspection. Best is to file a new bug with your test case, the original bug is fixed.
fyi, I've added a Python override using new_from_bytes() here: https://gitlab.gnome.org/GNOME/pygobject/merge_requests/74