276692 - move image->GdkPixbuf conversion code to nsImageGTK

Status: RESOLVED FIXED

(Core Graveyard :: GFX: Gtk, enhancement)

Description

[Christian :Biesinger (don't email me, ping me on IRC)]

from bug 38447 comment 105
"I'd really like it if we could share this code to convert a gfxIImage to a
GdkPixbuf.  One way we could do this would be to subclass gfxImageFrame for
gtk, and add a new interface that has a method something like:

GdkPixbuf* ConvertToPixbuf(PRBool aForceAlphaChannel);"

(and it turns out that I should have re-read that comment before writing the
implementation. anyway...)

I added this to nsImageGTK, which simplifies this code quite a bit since many
function calls can now be member variable accessors. it also avoid creating a
new class.

this implementation now has no aForceAlphaChannel argument, it expects the
caller to manually add it if desired. since this seems like a rarely used
feature, that seems ok. but if reviewers prefer it, I can add it, I don't feel
strongly about this one way or another.

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch as described

Comment 2

[Brian Ryner (not reading)]

Comment on attachment 170043 [details] [diff] [review]
patch as described

>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ gfx/src/gtk/nsIGdkPixbufImage.h	1 Jan 2005 20:22:15 -0000
>+/**
>+ * An interface that allows getting a GdkPixbuf*.
>+ */
>+class nsIGdkPixbufImage : public nsISupports {

Would it be possible to make this extend nsIImage instead of nsISupports?  That
way, we don't add extra bloat per-image (MI).

>--- gfx/src/gtk/nsImageGTK.cpp	23 Sep 2004 23:02:19 -0000	1.141
>+++ gfx/src/gtk/nsImageGTK.cpp	1 Jan 2005 20:22:15 -0000
>+NS_IMETHODIMP_(GdkPixbuf*)
>+nsImageGTK::GetGdkPixbuf() {
>+  // Init ensures that we only have 24bpp images
>+  NS_ASSERTION(mNumBytesPixel == 3, "Unexpected color depth");
>+
>+  nsresult rv = LockImagePixels(PR_FALSE);
>+  NS_ENSURE_SUCCESS(rv, nsnull);
>+
>+  // Since UnlockImagePixels potentially frees the image data (and since the
>+  // buffer might outlive this object anyway), we have to copy the data.
>+  guchar* pixels = NS_STATIC_CAST(guchar*,
>+      nsMemory::Clone(mImageBits, mRowBytes * mHeight));
>+  UnlockImagePixels(PR_FALSE);
>+  if (!pixels)
>+    return NULL;
>+
>+  GdkPixbuf *pixbuf = gdk_pixbuf_new_from_data(pixels,
>+      GDK_COLORSPACE_RGB,
>+      PR_FALSE,
>+      8,
>+      mWidth,
>+      mHeight,
>+      mRowBytes,
>+      pixbuf_free,
>+      NULL);
>+  if (!pixbuf)
>+    return nsnull;

Any reason this isn't consistent with respect to NULL and nsnull?

>--- browser/components/shell/src/nsGNOMEShellService.cpp	15 Jul 2004 22:51:18 -0000	1.4
>+++ browser/components/shell/src/nsGNOMEShellService.cpp	1 Jan 2005 20:22:15 -0000
>@@ -355,119 +357,26 @@ nsGNOMEShellService::SetShouldCheckDefau
>   return NS_OK;
> }
> 
> static nsresult
> WriteImage(const nsCString& aPath, gfxIImageFrame* aImage)
> {
>-  PRInt32 width, height;
>-  aImage->GetWidth(&width);
>-  aImage->GetHeight(&height);
>+  // We need the nsIImage, in order to get the nsIGdkPixbufImage
>+  nsCOMPtr<nsIImage> img(do_GetInterface(aImage));

We could avoid the need for that by making gfxImageFrame::GetInterface call
mImage->QueryInterface if the interface is nsIGdkPixbufImage.

r=me with those fixed.

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

>Would it be possible to make this extend nsIImage instead of nsISupports?  That
>way, we don't add extra bloat per-image (MI).

that's an excellent idea; I'll do that.

>Any reason this isn't consistent with respect to NULL and nsnull?

not really... I think I used NULL for gdkpixbuf interaction and nsnull for the
rest, but using nsnull consistently seems better. will fix.

>We could avoid the need for that by making gfxImageFrame::GetInterface call
>mImage->QueryInterface if the interface is nsIGdkPixbufImage.

hm... maybe we can make gfxImageFrame::GetInterface unconditionally call
mImage->QI? or I can just use that if.

Comment 4

[Brian Ryner (not reading)]

(In reply to comment #3)
> hm... maybe we can make gfxImageFrame::GetInterface unconditionally call
> mImage->QI? or I can just use that if.

Either way is fine with me.

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch v2

ok, I decided to unconditionally call it.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 171827 [details] [diff] [review]
patch v2

+#define NSIGDKPIXBUGIMAGE_IID \
...
+    NS_DEFINE_STATIC_IID_ACCESSOR(NSIGDKPIXBUGIMAGE_IID)

that's NSIGDKPIXBU*F*

other than that, looks good. carrying forward bryner's r+

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

heh, oops. thanks for catching the typo.

Checking in browser/components/shell/src/nsGNOMEShellService.cpp;
/cvsroot/mozilla/browser/components/shell/src/nsGNOMEShellService.cpp,v  <-- 
nsGNOMEShellService.cpp
new revision: 1.7; previous revision: 1.6
done
Checking in gfx/src/gtk/Makefile.in;
/cvsroot/mozilla/gfx/src/gtk/Makefile.in,v  <--  Makefile.in
new revision: 1.119; previous revision: 1.118
done
RCS file: /cvsroot/mozilla/gfx/src/gtk/nsIGdkPixbufImage.h,v
done
Checking in gfx/src/gtk/nsIGdkPixbufImage.h;
/cvsroot/mozilla/gfx/src/gtk/nsIGdkPixbufImage.h,v  <--  nsIGdkPixbufImage.h
initial revision: 1.1
done
Checking in gfx/src/gtk/nsImageGTK.cpp;
/cvsroot/mozilla/gfx/src/gtk/nsImageGTK.cpp,v  <--  nsImageGTK.cpp
new revision: 1.142; previous revision: 1.141
done
Checking in gfx/src/gtk/nsImageGTK.h;
/cvsroot/mozilla/gfx/src/gtk/nsImageGTK.h,v  <--  nsImageGTK.h
new revision: 1.54; previous revision: 1.53
done

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: missing part of the patch

this was meant to be part of the patch (see comment 4, comment 5) but I
apparently forgot to diff this file

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 177750 [details] [diff] [review]
missing part of the patch

I'm a little skeptical that GetInterface implementations should just
unconditionally forward to another object without some sort of IID check. 
You're then at the mercy of some other object for what your GetInterface does. 
I prefer to think of GetInterface as just a shortcut for an additional
interface on which there's a getter.  That getter would return something of a
given interface, that could then be QIed.  That said, maybe there's a good bit
of precedent for it.

So I think I actually prefer the approach I put in bug 38447.

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: get image using GI(nsIImage) followed by QI(nsIGdkPixbufImage)

OK, back to the approach as in the first version of this patch.

bryner, given comment 9, is this ok with you?

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

attachment 177988 [details] [diff] [review] checked in.