Bug 83973

... or else create a specific function, cairoImageSurfaceToGdkPixbufPremultiplied() say, for the purpose. Martin, let me know which you'd prefer and I'll prepare a patch.

Given bug 49532, seems cairoImageSurfaceToGdkPixbufPremultiplied() is the way to go.

Created attachment 137350 [details] Patch

Update.

Created attachment 137494 [details] Patch

I have no means to run GTK tests in my setup. So maybe we submit and check the webkit GTK bots, or you could patch it locally and see if the toDataURL.jpeg.alpha test passes.

I guess this should be handled by GdkPixbuf jpeg encoder, no? gdk_pixbuf_get_from_surface() returns a valid GdkPixbuf (using an alpha channel or not depending on the cairo surface image format). So gdk_pixbuf_save should work with that pixbuf.

(In reply to comment #8) > I guess this should be handled by GdkPixbuf jpeg encoder, no? Nope. JPEG encoders ignore alpha channel and encode the RGB channels only. The encoder used by GTK looks right to me; see real_save_jpeg() here: http://gdk-pixbuf.sourcearchive.com/documentation/2.22.0-1/io-jpeg_8c-source.html I believe the JPEG encoder should remain dumb and encode whatever RGB it's given. That RGB can be in one of two states: premultiplied or not.

(In reply to comment #8) > gdk_pixbuf_get_from_surface() returns a valid GdkPixbuf (using an alpha channel or not depending on the cairo surface image format). So gdk_pixbuf_save should work with that pixbuf. The cairo surface of an ImageBuffer is CAIRO_FORMAT_ARGB32 premultiplied. gdk_pixbuf_get_from_surface() converts the cairo surface pixels to un-premultiplied color. Thus, gdk_pixbuf_save() would send un-premultiplied color to the JPEG encoder if gdk_pixbuf_get_from_surface() was used to create the GdkPixbuf. The <canvas> spec states that toDataURL() must encode a Porter-Duff composite source-over black for image types that do not support alpha, JPEG for example. The toDataURL.jpeg.alpha.html checks for that. The test has been vetted for accuracy (bug 40147) and approved by the W3C http://w3c-test.org/html/tests/approved/canvas/toDataURL.jpeg.alpha.html The pass this test, you must send premultiplied color to the JPEG encoder. The reasons are documented in ChangeLog https://bugs.webkit.org/attachment.cgi?id=76964&action=prettypatch

(In reply to comment #10) > (In reply to comment #8) > > gdk_pixbuf_get_from_surface() returns a valid GdkPixbuf (using an alpha channel or not depending on the cairo surface image format). So gdk_pixbuf_save should work with that pixbuf. > > The cairo surface of an ImageBuffer is CAIRO_FORMAT_ARGB32 premultiplied. gdk_pixbuf_get_from_surface() converts the cairo surface pixels to un-premultiplied color. Thus, gdk_pixbuf_save() would send un-premultiplied color to the JPEG encoder if gdk_pixbuf_get_from_surface() was used to create the GdkPixbuf. > > The <canvas> spec states that toDataURL() must encode a Porter-Duff composite source-over black for image types that do not support alpha, JPEG for example. The toDataURL.jpeg.alpha.html checks for that. The test has been vetted for accuracy (bug 40147) and approved by the W3C > > http://w3c-test.org/html/tests/approved/canvas/toDataURL.jpeg.alpha.html > > The pass this test, you must send premultiplied color to the JPEG encoder. The reasons are documented in ChangeLog https://bugs.webkit.org/attachment.cgi?id=76964&action=prettypatch In that case I think we should do exactly that, something like this should work: GRefPtr<GdkPixbuf> pixbuf; if (type == "jpeg" && cairo_image_surface_get_format(surface) == CAIRO_FORMAT_ARGB32) { // JPEG doesn't support alpha channel. The <canvas> spec states that toDataURL() must encode a Porter-Duff // composite source-over black for image types that do not support alpha. RefPtr<cairo_surface_t> newSurface = adoptRef(cairo_image_surface_create(CAIRO_FORMAT_RGB24, cairo_image_surface_get_width(surface), cairo_image_surface_get_height(surface))); RefPtr<cairo_t> cr = adoptRef(cairo_create(newSurface.get())); cairo_set_source_surface(cr.get(), surface, 0, 0); cairo_set_operator(cr.get(), CAIRO_OPERATOR_SOURCE); cairo_paint(cr.get()); pixbuf = adoptGRef(cairoImageSurfaceToGdkPixbuf(newSurface.get())); } else pixbuf = adoptGRef(cairoImageSurfaceToGdkPixbuf(surface)); if (!pixbuf) return false;

> RefPtr<cairo_t> cr = adoptRef(cairo_create(newSurface.get())); > cairo_set_source_surface(cr.get(), surface, 0, 0); > cairo_set_operator(cr.get(), CAIRO_OPERATOR_SOURCE); > cairo_paint(cr.get()); > pixbuf = adoptGRef(cairoImageSurfaceToGdkPixbuf(newSurface.get())); Does cairoImageSurfaceToGdkPixbuf produce un-premultiplied colors here?

(In reply to comment #12) > > RefPtr<cairo_t> cr = adoptRef(cairo_create(newSurface.get())); > > cairo_set_source_surface(cr.get(), surface, 0, 0); > > cairo_set_operator(cr.get(), CAIRO_OPERATOR_SOURCE); > > cairo_paint(cr.get()); > > pixbuf = adoptGRef(cairoImageSurfaceToGdkPixbuf(newSurface.get())); > > Does cairoImageSurfaceToGdkPixbuf produce un-premultiplied colors here? It doesn't un-premultiply because there's no alpha in the source image.

Ok and when cairo paints a CAIRO_FORMAT_ARGB32 source into a CAIRO_FORMAT_RGB24 surface, the alpha channel is just ignored?

Because maybe it's not ignored given that the another plausible answer to my question is that since CAIRO_FORMAT_ARGB32 is premultipled data, it gets un-premultipled on write to a CAIRO_FORMAT_RGB24 target. I done a bunch of web searching trying to work out the correct answer, but so far I've found no canonical reference to the fact, nor in the cairo docs, or by reading through the cairo code trying to understand what it does. I guess I'm stuck here. What's a reasonable way for us to proceed here?

Data is not un-premultiplied, you can test the code I pasted and check that test toDataURL.jpeg.alpha.html passes.

Was comment #6 unclear?

(In reply to comment #17) > Was comment #6 unclear? You an test it with GtkLauncher and loading http://w3c-test.org/html/tests/approved/canvas/toDataURL.jpeg.alpha.html you don't need to run the tests

Was is the GtkLauncher ?

And whatever is is, I doubt I can build on on windows box.

Sorry, whatever GtkLauncher is, I doubt I can build it on a windows box.

(In reply to comment #21) > Sorry, whatever GtkLauncher is, I doubt I can build it on a windows box. Ok, I thought you could build webkit but not run the tests. GtkLauncher is a small web browser for testing that is built with the GTK+ port. I can simply submit a patch with the code I pasted if you want.

(In reply to comment #22) > (In reply to comment #21) > > Sorry, whatever GtkLauncher is, I doubt I can build it on a windows box. > > Ok, I thought you could build webkit but not run the tests. GtkLauncher is a small web browser for testing that is built with the GTK+ port. Appreciate the explanation. And no I can't build webkit gtk and run tests with my current setup (win32). > I can simply submit a patch with the code I pasted if you want. Yes please, that sounds good to me, if you like to do it.

One more thought was that Martin's original code, now modified, is much like a single memcpy. The code in comment #11 seems to cost two memcpy operations (one for the paint and one for cairoImageSurfaceToGdkPixbuf step). Should I worry about performance?

(In reply to comment #24) > One more thought was that Martin's original code, now modified, is much like a single memcpy. The code in comment #11 seems to cost two memcpy operations (one for the paint and one for cairoImageSurfaceToGdkPixbuf step). Should I worry about performance? Indeed it might be nice to do this in a single pass instead of two. How can you be sure your original patch fixes the test without testing it though? If necessary, I can do that locally.

Indeed. Testing that the test works means we either 1) submit and watch the GTK+ build bot, or > If necessary, I can do that locally. do as you suggest. If you have time, that'd sure help.

Created attachment 138311 [details] Patch

(In reply to comment #24) > One more thought was that Martin's original code, now modified, is much like a single memcpy. The code in comment #11 seems to cost two memcpy operations (one for the paint and one for cairoImageSurfaceToGdkPixbuf step). Should I worry about performance? We can actually use the same data to avoid the memory allocation since cairo use the same 32 bits also for RGB24.

Comment on attachment 138311 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=138311&action=review > Source/WebCore/platform/graphics/gtk/ImageBufferGtk.cpp:45 > + // JPEG doesn't support alpha channel. The <canvas> spec states that toDataURL() must encode a Porter-Duff Patch looks fine to me. nit: CAIRO_FORMAT_ARGB32 is a given, right? We could just assert if we're concerned about it. if (type == "jpeg") { ASSERT(cairo_image_surface_get_format(surface) == CAIRO_FORMAT_ARGB32); ...

Comment on attachment 138311 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=138311&action=review r=me but please consider this question before landing: >> Source/WebCore/platform/graphics/gtk/ImageBufferGtk.cpp:45 >> + // JPEG doesn't support alpha channel. The <canvas> spec states that toDataURL() must encode a Porter-Duff > > Patch looks fine to me. > > nit: CAIRO_FORMAT_ARGB32 is a given, right? We could just assert if we're concerned about it. > > if (type == "jpeg") { > ASSERT(cairo_image_surface_get_format(surface) == CAIRO_FORMAT_ARGB32); > ... Yes I think it'd be ok to proceed as Noel advises. Carlos?

(In reply to comment #30) > (From update of attachment 138311 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=138311&action=review > > r=me but please consider this question before landing: > > >> Source/WebCore/platform/graphics/gtk/ImageBufferGtk.cpp:45 > >> + // JPEG doesn't support alpha channel. The <canvas> spec states that toDataURL() must encode a Porter-Duff > > > > Patch looks fine to me. > > > > nit: CAIRO_FORMAT_ARGB32 is a given, right? We could just assert if we're concerned about it. > > > > if (type == "jpeg") { > > ASSERT(cairo_image_surface_get_format(surface) == CAIRO_FORMAT_ARGB32); > > ... > > Yes I think it'd be ok to proceed as Noel advises. Carlos? es, or even remove the check since surface is created unconditionally as ARGB32 in ImageBuffer constructor

Committed r115058: <http://trac.webkit.org/changeset/115058>

(In reply to comment #31) > or even remove the check since surface is created unconditionally as ARGB32 in ImageBuffer constructor Exactly. Thanks for helping everybody, test passes.