GNOME Bugzilla – Bug 491507
Provide cairo convenience functions inside libgdkpixbuf
Last modified: 2018-05-22 13:06:34 UTC
There are a number of missing cairo/gdk-pixbuf convenience functions: - Convert GdkPixbuf to cairo surface - Convert cairo surface to GdkPixbuf - Load file as cairo surface - Save cairo surface to file I think the best place to put these is in gdk-pixbuf. They (especially the load/save bits) are definitely useful without initializing GTK+, so I don't think putting them inside libgtk or libgdk is a good idea. (People are or should be trained to initialize GTK+ before using functions from GTK+ and GDK) There is clearly some downside to linking libgdkpixbuf against cairo ... you get an implicit dependency on libX11 and so forth, but because gdk-pixbuf is packaged inside the GTK+ package on most systems, that isn't as big as deal as it might be otherwise. A separate library doesn't make any sense to me, nor does linking cairo against GdkPixbuf.
See also http://mail.gnome.org/archives/gtk-devel-list/2007-October/msg00034.html and bug 395578
I started a thread on this: http://mail.gnome.org/archives/gtk-devel-list/2010-September/msg00022.html Lots of questions and rambling in that email, but also I proposed a concrete pixel-format-convert API for gdk-pixbuf that's simple and easy to add right away.
Created attachment 169404 [details] [review] Make GdkPixbuf switch on demand between cairo and classic format Here's an initial patch. This patch does not deprecate get_pixels, colorspace, bits per sample, n_channels, rowstride; however all of those probably should be deprecated at some point. It might be more friendly to give it a little time first. This patch leaves most of gdk-pixbuf using get_pixels internally. Separate patches could optimize any desired codepaths for cairo surface mode (to avoid the convert). It was necessary to make gdk_pixbuf_new_subpixbuf aware of the new cairo stuff though or it wouldn't work properly because it assumed pixbuf->pixels would never become a different block of memory over the lifetime of the pixbuf. This is the main backward incompatibility of this patch, I think: now the pixel buffer can get a new memory address. The fix is pretty obvious (always keep both pixbuf and surface around, once created). However, it makes pixbufs all use 2x memory. So maybe it's worth seeing if anything relies on this in practice.
Created attachment 169405 [details] [review] trivial gtk patch to use new gdk_pixbuf_get_cairo_surface() Here's the GTK patch, trivial but handy if anyone wants to test the gdk-pixbuf patch. I don't have anything running on GTK 3 besides gtk-demo (which seems to work fine) so my testing is limited. If you don't apply this GTK patch then most of the new gdk-pixbuf code would not be tested.
I have a few tweaks to the patch, I'll upload them later on, nothing major. Some larger possible tweaks: * I'm thinking there should be A8 and A1 format support. These would convert to classic pixels as the alpha channel, with the RGB pixels all black, perhaps. Would address the occasional need for a mask. This may need to be done now because it wouldn't be compatible to suddenly increase the possible cairo formats in the future. Possibly the API docs should say that the cairo format from get_cairo_surface() can be anything, even if new_from_cairo_surface() remains limited in what it accepts. Just go ahead and document that we're allowed to support more cairo formats. * I wonder if an adequate solution to the backward incompatibility problem would be that if we create from classic format, we always keep classic format. If we create from cairo surface, we only keep the most-recent representation as in the patch now. That way the classic constructors remain compatible. The only trick is that as APIs (such as the loaders, or third party APIs) switch to creating from cairo surface, apps could get a pixbuf with a movable-in-memory pixels array, when they might not be expecting it. However, surely the main sort of code that might need the pixels to stay put would be code that's pointing the pixels at a buffer inside another object, or the like, and so that code would almost always be using its own gdk_pixbuf_new_from_data(). i.e. the goal would be, if you use new_from_data(), keep that pixel buffer forever. If you just get some pixbuf from a loader, it might be a surface-based pixbuf that's allowed to toss its pixel buffer. To make things more predictable, we could always set the "pixel buffer can be tossed" flag on stuff from GdkPixbufLoader, even if it didn't come from a surface.
Created attachment 169464 [details] [review] Add cairo image surface as a second storage format for GdkPixbuf Add APIs: gdk_pixbuf_new_from_cairo_surface() gdk_pixbuf_get_cairo_surface() Historically, GdkPixbuf had only two formats, one with and one without alpha. The format is always big-endian, uses 3 bytes per pixel without alpha, and does not premultiply alpha. Cairo (and most video hardware) uses native endianness, always 4 bytes per pixel even without alpha, and premultiplies alpha. This patch changes GdkPixbuf such that it has two representations internally. At any time, a pixbuf can be either a buffer in the "classic" pixbuf format, or it can be a cairo_surface_t. As a small special case, a subpixbuf can also have neither representation, and create the desired representation on demand from its source pixbuf. In this initial patch, pixbuf->pixels is renamed to pixbuf->maybe_pixels to break source compatibility for anyone accessing it directly. All code in gdk-pixbuf is updated to access it with gdk_pixbuf_get_pixels(). Essentially all uses of gdk_pixbuf_get_pixels() are now suboptimal. However, porting away from get_pixels() is a task for another day. Pixel format conversion code is from gdk_cairo_set_source_pixbuf() and implemented originally by Owen Taylor. Fixes https://bugzilla.gnome.org/show_bug.cgi?id=491507 Fixes https://bugzilla.gnome.org/show_bug.cgi?id=395578
Created attachment 169465 [details] [review] Add "keep_pixels" flag to avoid moving pixel data to another place in memory An occasional use of GdkPixbuf is to point the data at some external pixel buffer, or point some external pixel buffer at the data. When the pixels are shared with some external code, our optimization to keep only the cairo surface or only the classic-format pixels around fails, because when we free and later recreate the classic-format pixels, they move around in memory. The keep_pixels flag designates pixbufs created with gdk_pixbuf_new() or gdk_pixbuf_new_from_data() as special, so they will never reallocate the classic-format pixel buffer. The theory is that these two constructors should cover cases where someone was sharing the pixel buffer with external code, and that these two constructors can be deprecated eventually. (They are annoying anyhow since they have two useless arguments - colorspace and bits per sample.) We do not set keep_pixels on pixbufs we create by copying, scaling, and especially image loading. This allows us to avoid keeping two representations in these pixbufs. There is still a possible incompatibility, but we will try to manage that incompatibility rather than avoid it, so we don't have to double the size of every pixbuf in every application.
Created attachment 169466 [details] [review] add gdk_pixbuf_new_from_rgb_data() as a porting aid to cairo-centric world This allows people to avoid gdk_pixbuf_new_from_data() without having to write a bunch of new code or cut-and-paste format conversion code. The function may be useful long-term anyway because premultiplied alpha is kind of annoying to work with so maybe people like to create images in the old format rather than in GPU-friendly Cairo format. We'll see.
Created attachment 169467 [details] [review] Deprecate API that assumes the old non-cairo pixel format GdkColorspace, n_channels, bits_per_sample, rowstride, gdk_pixbuf_get_pixels(), gdk_pixbuf_new(), gdk_pixbuf_new_from_data() gdk_pixbuf_get_pixels() is the one that's actively a problem, of course, because it creates the extra data copy. gdk_pixbuf_new() and gdk_pixbuf_new_from_data() use colorspace and bits per sample in their APIs, and have the special "keep pixels" hack that we'd like to avoid. So encourage people to drop these as well. The library no longer builds with GDK_PIXBUF_DISABLE_DEPRECATED (not even close), so the deprecated stuff is still declared if GDK_PIXBUF_COMPILATION. This isn't good enough to make gdk-pixbuf-xlib build so also move GDK_PIXBUF_DISABLE_DEPRECATED down into the gdk-pixbuf subdir Makefile.am.
I kept the following split up for pick-and-choose fun. not sure which you want. * baseline "add cairo" patch * "keep pixels" patch makes gdk_pixbuf_new() and new_from_data() have special semantics where they never destroy the classic pixels. intent is that this avoids almost all the back compat headaches, while only penalizing to-be-deprecated API. * "new_from_rgb_data" patch lets you create a "cairo based" pixbuf from classic-style data, which is probably pretty useful in quickly removing use of deprecated API. * "deprecate API" patch actually marks stuff deprecated. this requires removing GDK_PIXBUF_DISABLE_DEPRECATED from gtk+ configure.ac and will generally break the crap out of the whole gnome build most likely. These should be ready for testing and review.
In http://mail.gnome.org/archives/gtk-devel-list/2010-September/msg00117.html I suggested a third possible variant of this patch in which we always keep_pixels. So three options: a) toss pixels anytime we get surface b) toss pixels anytime we get surface, unless pixels came from the app c) never toss pixels once they exist even in c), once apps are ported to avoid get_pixels(), eventually we wouldn't have the pixels around. If cairo adds "classic pixbuf" format, then I think the API in this patch is unchanged, but you could just internally keep a surface instead of the byte buffer, for the classic-format pixels. In this case, conversion could be one-time and one-way: if get_pixels() was called, you'd flip to classic format and stay there forever. My opinion is that assuming a cairo dependency is desired, the from_cairo_surface and get_cairo_surface API is pretty darn obvious, so may as well just stick that in. If future cairo changes allow the implementation to be cleaned up, great.
Is it inappropriate to ask what the status of this patch/bug is here? I am working on porting my GTK based library and applications to GTK 3.0 and I really need gdk_pixbuf_get_cairo_surface() to complete the implementation in a clean way. Seems like the conversation stopped about it 6 months ago.
Just as a note: My pictures work provide this functionality out of the box (just because pictures are based around Cairo). And pictures are intended to land in GTK 3.2.
so.. what's the status here? Are the patches obsolete, because the functionality has landed somewhere else?
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gdk-pixbuf/issues/13.