Bug 15654 - [GTK] GdkPixbuf support for ImageCairo
: [GTK] GdkPixbuf support for ImageCairo
Status: RESOLVED FIXED
: WebKit
New Bugs
: 523.x (Safari 3)
: All All
: P2 Normal
Assigned To:
:
: Cairo, Gtk
:
:
  Show dependency treegraph
 
Reported: 2007-10-23 19:28 PST by
Modified: 2009-04-23 08:40 PST (History)


Attachments
Converting Cairo's surfaces to GdkPixbuf (2.94 KB, patch)
2009-02-28 12:58 PST, Zan Dobersek
gns: review+
Review Patch | Details | Formatted Diff | Diff
Converting Cairo's surfaces to GdkPixbuf, take 2 (2.91 KB, patch)
2009-03-16 13:25 PST, Zan Dobersek
gns: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2007-10-23 19:28:57 PST
We need interoperability between GdkPixbuf and ImageCairo to implement certain features. It may even be the case that we want Image in the GTK+ port to be implemented using GdkPixbuf rather than a Cairo surface.
------- Comment #1 From 2008-10-26 04:03:28 PST -------
(In reply to comment #0)
> We need interoperability between GdkPixbuf and ImageCairo to implement certain
> features. It may even be the case that we want Image in the GTK+ port to be
> implemented using GdkPixbuf rather than a Cairo surface.

What is the benefit of GdkPixbuf? Why should we use GdkPixbuf instead of a cairo_surface? Expecially if you read http://live.gnome.org/GtkCairoIntegration. It is intented to replace GdkPixbuf in Gtk by cairo (at least in parts of it).
The biggest problem seems to be point 2 in the problems list.
We'll have to do something like in 'Workarounds & Solutions' on the same link.
------- Comment #2 From 2008-10-28 11:37:28 PST -------
(In reply to comment #1)
> (In reply to comment #0)
> > We need interoperability between GdkPixbuf and ImageCairo to implement certain
> > features. It may even be the case that we want Image in the GTK+ port to be
> > implemented using GdkPixbuf rather than a Cairo surface.
> 
> What is the benefit of GdkPixbuf? Why should we use GdkPixbuf instead of a
> cairo_surface? Expecially if you read
> http://live.gnome.org/GtkCairoIntegration. It is intented to replace GdkPixbuf
> in Gtk by cairo (at least in parts of it).
> The biggest problem seems to be point 2 in the problems list.
> We'll have to do something like in 'Workarounds & Solutions' on the same link.
> 

I don't think there's any need to switch to GdkPixbuf or stop using cairo_surface. This bug is just here to add some integration between the image classes in GTK+ and the WebCore Cairo backend so we can easily take GdkPixbufs handed to us by GTK+ and render them with Cairo, and vice-versa, generate GdkPixbufs based on Cairo surfaces if we need to expose them in the WebKit GTK+ API for example. This kind of integration could be helpful for Cursor.h and custom image cursors and I think the file chooser needed something like this too. Hope things are a little clearer now.
------- Comment #3 From 2009-02-28 12:58:31 PST -------
Created an attachment (id=28125) [details]
Converting Cairo's surfaces to GdkPixbuf

Implements a function for converting Cairo's surfaces to GdkPixbufs. There might be a more hackish way of doing this, but I vote this one for being the prettiest.
------- Comment #4 From 2009-03-07 13:12:54 PST -------
(In reply to comment #3)
> Created an attachment (id=28125) [details] [review]
> Converting Cairo's surfaces to GdkPixbuf
> 
> Implements a function for converting Cairo's surfaces to GdkPixbufs. There
> might be a more hackish way of doing this, but I vote this one for being the
> prettiest.
> 

Looks reasonable to me.
------- Comment #5 From 2009-03-14 12:10:15 PST -------
(In reply to comment #3)
> Created an attachment (id=28125) [details] [review]
> Converting Cairo's surfaces to GdkPixbuf
> 
> Implements a function for converting Cairo's surfaces to GdkPixbufs. There
> might be a more hackish way of doing this, but I vote this one for being the
> prettiest.
> 

Hi,

just a question about GdkPixmap* pixmap = gdk_pixmap_new(0, width, height, 24);
Won't we loose the alpha channel here? Isn't 32 a better choice here? (If 32 is possible).
We'll need the alpha channel for example on cursors with images.
------- Comment #6 From 2009-03-16 13:25:08 PST -------
Created an attachment (id=28658) [details]
Converting Cairo's surfaces to GdkPixbuf, take 2

24 -> 32. Alpha channel should now be preserved through the conversion.
------- Comment #7 From 2009-04-20 19:14:56 PST -------
(In reply to comment #6)
> Created an attachment (id=28658) [details] [review]
> Converting Cairo's surfaces to GdkPixbuf, take 2
> 
> 24 -> 32. Alpha channel should now be preserved through the conversion.
> 

The patch looks good to me, too, but I still fail to think of a use-case. I think we should have at least one patch using this functionality so that it gets a bit of real-world testing before we take it. Otherwise this may end up being something we have but use for nothing, for a long time.
------- Comment #8 From 2009-04-21 03:01:01 PST -------
(In reply to comment #7)
> The patch looks good to me, too, but I still fail to think of a use-case. I
> think we should have at least one patch using this functionality so that it
> gets a bit of real-world testing before we take it. Otherwise this may end up
> being something we have but use for nothing, for a long time.

A possible use-case is a cursor icon: https://bugs.webkit.org/show_bug.cgi?id=24565
------- Comment #9 From 2009-04-21 15:42:26 PST -------
(From update of attachment 28125 [details])
(In reply to comment #8)
> A possible use-case is a cursor icon:
> https://bugs.webkit.org/show_bug.cgi?id=24565

Right, thanks krit! This one is good to go then, IMO. What about you cook us a patch for those cursors now? =D
------- Comment #10 From 2009-04-22 20:22:31 PST -------
(From update of attachment 28658 [details])
hah... of course I reviewed the wrong patch; I also found that there's a commented out function in Pasteboard::writeImage (at WebKit/WebCore/platform/gtk/) that is waiting on this.
------- Comment #11 From 2009-04-23 08:40:29 PST -------
Landed as r42780. Thanks, Zan!