Bug 83973 – [GTK] Should pass canvas/philip/tests/toDataURL.jpeg.alpha.html

• Component: Canvas
• Status: RESOLVED FIXED

[noel gordon]

To pass this test, pass premultiplied color to the JPEG encoder. Modify the cairoImageSurfaceToGdkPixbuf() function added in

  https://bugs.webkit.org/attachment.cgi?id=62763&action=prettypatch

to have an default second argument (default false) that requests premultiplied color output when creating the GdkPixbuf.

[noel gordon]

... 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.

[noel gordon]

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

[noel gordon]

Created attachment 137350 [details]
Patch

[noel gordon]

Update.

[noel gordon]

Created attachment 137494 [details]
Patch

[noel gordon]

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.

[noel gordon]

https://www.w3.org/Bugs/Public/show_bug.cgi?id=11431

[Carlos Garcia Campos]

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.

[noel gordon]

(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.

[noel gordon]

(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

[Carlos Garcia Campos]

(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;

[noel gordon]

>     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?

[Carlos Garcia Campos]

(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.

[noel gordon]

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

[noel gordon]

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?

[Carlos Garcia Campos]

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

[noel gordon]

Was comment #6 unclear?

[Carlos Garcia Campos]

(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

[noel gordon]

Was is the GtkLauncher ?

[noel gordon]

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

[noel gordon]

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

[Carlos Garcia Campos]

(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.

[noel gordon]

(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.

[noel gordon]

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?

[Martin Robinson]

(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.

[noel gordon]

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.

[Carlos Garcia Campos]

Created attachment 138311 [details]
Patch

[Carlos Garcia Campos]

(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.

[noel gordon]

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);
     ...

[Philippe Normand]

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?

[Carlos Garcia Campos]

(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

[Carlos Garcia Campos]

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

[noel gordon]

(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.