Bug 56238 – Non-premultiplied-alpha canvas attribute is ignore for toDataURL, drawImage, texImage2D

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[John Bauman]

Currently WebKit treats every webgl canvas as having a premultiplied alpha, which causes a lot of problems for functions that need to either premultiply it (drawImage), or avoid converting it entirely (toDataURL, texImage2D).

[John Bauman]

Created attachment 85565 [details]
Patch

[John Bauman]

I'm not entirely sure that that's the right approach, but it works for Chromium and Safari and I'm sure it could be extended to Qt if I knew enough about Qt, so that's probably a sign.

[Kenneth Russell]

Comment on attachment 85565 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=85565&action=review

This looks like it's on the right track to me but needs some more work.

Please incorporate the test, or portion thereof, which exercises this functionality using the script "update-webgl-conformance-tests". (cd into LayoutTests/fast/canvas/webgl, and run the script pointing it to the path of the test in the Khronos repo.)

> Source/WebCore/html/HTMLCanvasElement.cpp:347
> +    RefPtr<ImageData> imData = getImageData();

Avoid abbreviations in WebKit style. Just use "imageData".

> Source/WebCore/platform/graphics/GraphicsContext3D.h:864
> +    // Read rendering results into an RGBA pixel array.

The call sites indicate this produces BGRA data. Please update the comment or the spec of this method to indicate exactly what it does; please also mention whether the alpha channel is multiplied in or not.

> Source/WebCore/platform/graphics/ImageBuffer.h:136
> +    String ImageDataToDataURL(ImageData& input, const String& mimeType, const double* quality);

It would be nicer to put this in ImageData.h/cpp (WebCore/html/) but I see the reasons for putting it here. In particular, if you can achieve better code refactoring (see below), I think it's OK to leave it here. You could also consider making this a static private method, making ImageData a friend, and adding ImageData::todataURL().

Here, ImageData& should be const.

> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:593
> +    if (!image)
> +        return "data:,";
> +        
> +    RetainPtr<CFMutableDataRef> data(AdoptCF, CFDataCreateMutable(kCFAllocatorDefault, 0));
> +    if (!data)
> +        return "data:,";
> +        
> +    RetainPtr<CFStringRef> uti = utiFromMIMEType(mimeType);
> +    ASSERT(uti);
> +        
> +    RetainPtr<CGImageDestinationRef> destination(AdoptCF, CGImageDestinationCreateWithData(data.get(), uti.get(), 1, 0));
> +    if (!destination)
> +        return "data:,";
> +        
> +    RetainPtr<CFDictionaryRef> imageProperties = 0;
> +    if (CFEqual(uti.get(), jpegUTI()) && quality && *quality >= 0.0 && *quality <= 1.0) {
> +        // Apply the compression quality to the image destination.
> +        RetainPtr<CFNumberRef> compressionQuality(AdoptCF, CFNumberCreate(kCFAllocatorDefault, kCFNumberDoubleType, quality));
> +        const void* key = kCGImageDestinationLossyCompressionQuality;
> +        const void* value = compressionQuality.get();
> +        imageProperties.adoptCF(CFDictionaryCreate(0, &key, &value, 1, &kCFTypeDictionaryKeyCallBacks, &kCFTypeDictionaryValueCallBacks));
> +    }
> +        
> +    CGImageDestinationAddImage(destination.get(), image.get(), imageProperties.get());
> +    CGImageDestinationFinalize(destination.get());
> +    
> +    Vector<char> out;
> +    base64Encode(reinterpret_cast<const char*>(CFDataGetBytePtr(data.get())), CFDataGetLength(data.get()), out);
> +
> +    return makeString("data:", mimeType, ";base64,", out);

This block of code is exactly equal to that in ImageBuffer::toDataURL. Please refactor it into a new helper function.

> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:530
> +    return 0;

Add FIXME indicating this should be implemented.

> Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:399
> +    Vector<unsigned char> encodedImage;
> +    if (mimeType == "image/jpeg") {
> +        int compressionQuality = JPEGImageEncoder::DefaultCompressionQuality;
> +        if (quality && *quality >= 0.0 && *quality <= 1.0)
> +            compressionQuality = static_cast<int>(*quality * 100 + 0.5);
> +        if (!JPEGImageEncoder::encode(source, compressionQuality, &encodedImage))
> +            return "data:,";
> +    } else {
> +        if (!PNGImageEncoder::encode(source, &encodedImage))
> +            return "data:,";
> +        ASSERT(mimeType == "image/png");
> +    }
> +
> +    Vector<char> base64Data;
> +    base64Encode(*reinterpret_cast<Vector<char>*>(&encodedImage), base64Data);
> +
> +    return makeString("data:", mimeType, ";base64,", base64Data);

There is some duplicated code between this and ImageBuffer::toDataURL above. It's less than in other places in this file but it would be good to refactor.

> Source/WebCore/platform/image-encoders/skia/JPEGImageEncoder.cpp:196
> +    imageSize.clampNegativeToZero();
> +    JPEGOutputBuffer destination;
> +    destination.output = output;
> +    Vector<JSAMPLE> row;
> +
> +    jpeg_compress_struct cinfo;
> +    jpeg_error_mgr error;
> +    cinfo.err = jpeg_std_error(&error);
> +    error.error_exit = handleError;
> +    jmp_buf jumpBuffer;
> +    cinfo.client_data = &jumpBuffer;
> +
> +    if (setjmp(jumpBuffer)) {
> +        jpeg_destroy_compress(&cinfo);
> +        return false;
> +    }
> +
> +    jpeg_create_compress(&cinfo);
> +    cinfo.dest = &destination;
> +    cinfo.dest->init_destination = prepareOutput;
> +    cinfo.dest->empty_output_buffer = writeOutput;
> +    cinfo.dest->term_destination = finishOutput;
> +    cinfo.image_height = imageSize.height();
> +    cinfo.image_width = imageSize.width();
> +    cinfo.in_color_space = JCS_RGB;
> +    cinfo.input_components = 3;
> +
> +    jpeg_set_defaults(&cinfo);
> +    jpeg_set_quality(&cinfo, quality, TRUE);
> +    jpeg_start_compress(&cinfo, TRUE);
> +
> +    const unsigned char* pixels = imageData.data()->data()->data();
> +    row.resize(cinfo.image_width * cinfo.input_components);
> +    while (cinfo.next_scanline < cinfo.image_height) {
> +        RGBAtoRGB(pixels, cinfo.image_width, row.data());
> +        jpeg_write_scanlines(&cinfo, row.dataSlot(), 1);
> +        pixels += cinfo.image_width * 4;
> +    }
> +
> +    jpeg_finish_compress(&cinfo);
> +    jpeg_destroy_compress(&cinfo);
> +    return true;

Nearly all of this code is duplicated from encode(const SkBitmap& bitmap, ...). Please refactor it to avoid the duplication.

> Source/WebCore/platform/image-encoders/skia/PNGImageEncoder.cpp:154
> +    IntSize imageSize(bitmap.width(), bitmap.height());
> +    imageSize.clampNegativeToZero();
> +    unsigned char *pixels = bitmap.data()->data()->data();
> +
> +    png_struct* png = png_create_write_struct(PNG_LIBPNG_VER_STRING, 0, 0, 0);
> +    png_info* info = png_create_info_struct(png);
> +    if (!png || !info || setjmp(png_jmpbuf(png))) {
> +        png_destroy_write_struct(png ? &png : 0, info ? &info : 0);
> +        return false;
> +    }
> +
> +    // Optimize compression for speed.
> +    // The parameters are the same as what libpng uses by default for RGB and RGBA images, except:
> +    // - the zlib compression level is 3 instead of 6, to avoid the lazy Ziv-Lempel match searching;
> +    // - the delta filter is 1 ("sub") instead of 5 ("all"), to reduce the filter computations.
> +    // The zlib memory level (8) and strategy (Z_FILTERED) will be set inside libpng.
> +    //
> +    // Avoid the zlib strategies Z_HUFFMAN_ONLY or Z_RLE.
> +    // Although they are the fastest for poorly-compressible images (e.g. photographs),
> +    // they are very slow for highly-compressible images (e.g. text, drawings or business graphics).
> +    png_set_compression_level(png, 3);
> +    png_set_filter(png, PNG_FILTER_TYPE_BASE, PNG_FILTER_SUB);
> +
> +    png_set_write_fn(png, output, writeOutput, 0);
> +    png_set_IHDR(png, info, imageSize.width(), imageSize.height(),
> +                 8, PNG_COLOR_TYPE_RGB_ALPHA, 0, 0, 0);
> +    png_write_info(png, info);
> +
> +    for (int y = 0; y < imageSize.height(); ++y) {
> +        png_write_row(png, pixels);
> +        pixels += imageSize.width() * 4;
> +    }
> +
> +    png_write_end(png, info);
> +    png_destroy_write_struct(&png, &info);
> +    return true;

Same comment about duplicated code between here and encode(const SkBitmap& bitmap, ...).

[John Bauman]

Created attachment 85767 [details]
Patch

[Kenneth Russell]

Comment on attachment 85767 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=85767&action=review

The code changes look good but there are a few issues with the test case that need to be fixed.

> LayoutTests/fast/canvas/webgl/premultiplyalpha-test.html:49
> +     console.log(gl.getContextAttributes());

This should be removed.

> LayoutTests/fast/canvas/webgl/premultiplyalpha-test.html:70
> +     img.src = png;
> +     img.onload = function() {

There are issues like this race condition that I mentioned in the upstream bug report on Khronos. Please fix those and re-convert this test case into a layout test.

> LayoutTests/fast/canvas/webgl/resources/webgl-test-utils.js:109
> -  '    gl_FragColor = texture2D(tex, texCoord);\n' +
> +  '    gl_FragData[0] = texture2D(tex, texCoord);\n' +

This change is invalid. gl_FragData is unsupported in GLSL ES 2.0.

> LayoutTests/platform/chromium/test_expectations.txt:2360
> +BUGWEBGL : fast/canvas/webgl/premultiplyalpha-test.html = TEXT

Why is this new test not passing? It seems wrong that it doesn't pass even in Chromium.

> LayoutTests/platform/mac/Skipped:306
> +fast/canvas/webgl/premultipliedalpha-test.html

Since the new test doesn't involve drawing a WebGL canvas to the screen whose premultipliedAlpha flag is set to false, it seems that we should be able to make the new test pass on Mac as well. Is that not possible?

> Source/WebCore/platform/image-encoders/skia/PNGImageEncoder.cpp:106
> +            preMultipliedBGRAtoRGBA(pixels,
> +                                    imageSize.width(), row.data());

Line break here is unnecessary.

[John Bauman]

(In reply to comment #5)
> 
> > LayoutTests/fast/canvas/webgl/resources/webgl-test-utils.js:109
> > -  '    gl_FragColor = texture2D(tex, texCoord);\n' +
> > +  '    gl_FragData[0] = texture2D(tex, texCoord);\n' +
> 
> This change is invalid. gl_FragData is unsupported in GLSL ES 2.0.

I got this straight from upstream. http://www.khronos.org/registry/gles/specs/2.0/es_full_spec_2.0.24.pdf says that gl_FragData is allowed.
> 
> > LayoutTests/platform/chromium/test_expectations.txt:2360
> > +BUGWEBGL : fast/canvas/webgl/premultiplyalpha-test.html = TEXT
> 
> Why is this new test not passing? It seems wrong that it doesn't pass even in Chromium.

Currently the test for the premultiplied case is broken, but with that fixed it should work on chromium and mac.
> 
> > LayoutTests/platform/mac/Skipped:306
> > +fast/canvas/webgl/premultipliedalpha-test.html
> 
> Since the new test doesn't involve drawing a WebGL canvas to the screen whose premultipliedAlpha flag is set to false, it seems that we should be able to make the new test pass on Mac as well. Is that not possible?

[Kenneth Russell]

(In reply to comment #6)
> (In reply to comment #5)
> > 
> > > LayoutTests/fast/canvas/webgl/resources/webgl-test-utils.js:109
> > > -  '    gl_FragColor = texture2D(tex, texCoord);\n' +
> > > +  '    gl_FragData[0] = texture2D(tex, texCoord);\n' +
> > 
> > This change is invalid. gl_FragData is unsupported in GLSL ES 2.0.
> 
> I got this straight from upstream. http://www.khronos.org/registry/gles/specs/2.0/es_full_spec_2.0.24.pdf says that gl_FragData is allowed.

I apologize; I stand corrected.

> > > LayoutTests/platform/chromium/test_expectations.txt:2360
> > > +BUGWEBGL : fast/canvas/webgl/premultiplyalpha-test.html = TEXT
> > 
> > Why is this new test not passing? It seems wrong that it doesn't pass even in Chromium.
> 
> Currently the test for the premultiplied case is broken, but with that fixed it should work on chromium and mac.

What's the plan in this area? Are you going to fix the test upstream in Khronos? In that case, let's get it fixed there and then just update this copy of the test to match it.

> > 
> > > LayoutTests/platform/mac/Skipped:306
> > > +fast/canvas/webgl/premultipliedalpha-test.html
> > 
> > Since the new test doesn't involve drawing a WebGL canvas to the screen whose premultipliedAlpha flag is set to false, it seems that we should be able to make the new test pass on Mac as well. Is that not possible?

[John Bauman]

Created attachment 85889 [details]
Patch

[John Bauman]

Created attachment 85890 [details]
Patch

[Kenneth Russell]

Comment on attachment 85890 [details]
Patch

Looks great. Excellent work.

[WebKit Commit Bot]

Comment on attachment 85890 [details]
Patch

Clearing flags on attachment: 85890

Committed r81213: <http://trac.webkit.org/changeset/81213>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[John Bauman]

*** Bug 55509 has been marked as a duplicate of this bug. ***