Bug 28345 - [GTK] BitmapImage::getGdkPixbuf does not handle alpha channels properly
Summary: [GTK] BitmapImage::getGdkPixbuf does not handle alpha channels properly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 28346
  Show dependency treegraph
 
Reported: 2009-08-15 19:08 PDT by Martin Robinson
Modified: 2009-08-23 00:18 PDT (History)
2 users (show)

See Also:


Attachments
Fix conversion (4.95 KB, patch)
2009-08-15 19:17 PDT, Martin Robinson
eric: review-
Details | Formatted Diff | Diff
Patch with variable names according to conventions (4.98 KB, patch)
2009-08-17 23:02 PDT, Martin Robinson
xan.lopez: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Robinson 2009-08-15 19:08:21 PDT
Cairo surfaces store their information in CAIRO_FORMAT_ARGB32, while GdkPixbufs store their information in GDK_COLORSPACE_RGB. The current conversion process between the two types leads to transparency corruption on some images.
Comment 1 Martin Robinson 2009-08-15 19:17:56 PDT
Created attachment 34910 [details]
Fix conversion

I've attached a patch fixing this issue.
Comment 2 Martin Robinson 2009-08-15 19:33:33 PDT
Sorry to spam this bug, but I wanted to mention that this is reproducible by enabling images as cursors with the patch from #28346 and visiting: http://www.worldtimzone.com/mozilla/testcase/css3cursors.html and trying the red cursor at the bottom.
Comment 3 Eric Seidel (no email) 2009-08-17 17:17:04 PDT
Comment on attachment 34910 [details]
Fix conversion

rowstride should probably be rowStride according to webkit style.

Are you sure there aren't gtk functions which abstract this?
+#if G_BYTE_ORDER == G_LITTLE_ENDIAN
+            guchar alpha = source[3];
+            dest[0] = alpha ? ((source[2] * 255) / alpha) : 0;
+            dest[1] = alpha ? ((source[1] * 255) / alpha) : 0;
+            dest[2] = alpha ? ((source[0] * 255) / alpha) : 0;
+            dest[3] = alpha;
+#else
Seems silly to make every program check those ifdefs.
Comment 4 Martin Robinson 2009-08-17 17:29:20 PDT
Perhaps more recent versions of GTK+ or GDK have added functionality to accomplish this, but I could not find it. For reference here is an example where Abiword does something very similar: http://google.com/codesearch/p?hl=en&sa=N&cd=1&ct=rc#gQXpoyUxBIc/abiword-2.5.0/goffice-bits/goffice/utils/go-image.c&q=cairo_to_pixbuf&l=327
Comment 5 Martin Robinson 2009-08-17 23:02:23 PDT
Created attachment 35016 [details]
Patch with variable names according to conventions

This patch aligns variable names according to WebKit conventions.
Comment 6 Xan Lopez 2009-08-23 00:11:44 PDT
Comment on attachment 35016 [details]
Patch with variable names according to conventions

This looks good to me, and on top of fixing a bug it manages to do the data conversion on the client side. There's a NULL where we should use 0, but I'll fix that when landing.
Comment 7 Xan Lopez 2009-08-23 00:18:54 PDT
Committed as r47689, closing.