Bug 89937 – [Qt] Use premultiplied alpha when extracting image data in WebGL

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

[Noam Rosenthal]

In GraphicsContext3DQt, we convert the native QPixmap to QImage::Format_ARGB32, and then pass AlphaDoPremultiply as an argument to packPixels.
This means that if the image was originally premultiplied, the code path would convert it to unpremultiplied and then back to premultiplied.
Instead, we should use QImage::Format_ARGB32_Premultiplied when premultiplyAlpha is true.

[Dongseong Hwang]

This bug may solve itself with Bug 89938.

GraphicsContext3D::extractImageData causes strange message with specific png files.

[Noam Rosenthal]

(In reply to comment #1)
> This bug may solve itself with Bug 89938.
> 
> GraphicsContext3D::extractImageData causes strange message with specific png files.

hmm... not sure they're related.

[Noam Rosenthal]

Created attachment 149519 [details]
Patch

[Simon Hausmann]

Comment on attachment 149519 [details]
Patch

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

> Source/WebCore/ChangeLog:14
> +        Covered by existing WebGL pixel tests.

Any chance of naming them, for reference in the ChangeLog? :)

[Jocelyn Turcotte]

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

> Source/WebCore/ChangeLog:9
> +        Format_ARB32_Premultiplied instead of converting it to ARGB32 and then letting

ARB32 -> ARGB32

> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:503
> +    qtImage = qtImage.convertToFormat(premultiplyAlpha ? QImage::Format_ARGB32_Premultiplied : QImage::Format_ARGB32);

Won't this cause an extra conversion if the source image is, on the opposite, already in Format_ARGB32?
The cost of AlphaDoPremultiply should be pretty low as packPixel already has to convert from BGRA to ARGB and the data is in the CPU cache at the moment of the pre-multiplication.

[Noam Rosenthal]

Created attachment 149542 [details]
Patch

[Jocelyn Turcotte]

Comment on attachment 149542 [details]
Patch

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

> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:520
> +    switch (qtImage.format()) {
> +    case QImage::Format_ARGB32:
> +        if (premultiplyAlpha)
> +            alphaOp = AlphaDoPremultiply;
> +        break;
> +    case QImage::Format_ARGB32_Premultiplied:
> +        if (!premultiplyAlpha)
> +            alphaOp = AlphaDoUnmultiply;
> +        break;
> +    default:
> +        // The image has a format that is not supported in packPixels. We have to convert it here.
> +        qtImage = qtImage.convertToFormat(premultiplyAlpha ? QImage::Format_ARGB32_Premultiplied : QImage::Format_ARGB32);
> +        break;
> +    }

Wouldn't if/else look cleaner?

if (qtImage.format() == QImage::Format_ARGB32 && premultiplyAlpha)
    alphaOp = AlphaDoPremultiply;
else if (qtImage.format() == QImage::Format_ARGB32_Premultiplied && !premultiplyAlpha)
    alphaOp = AlphaDoUnmultiply;
else
    // The image has a format that is not supported in packPixels. We have to convert it here.
    qtImage = qtImage.convertToFormat(premultiplyAlpha ? QImage::Format_ARGB32_Premultiplied : QImage::Format_ARGB32);

r=me in any case.

[Jocelyn Turcotte]

(In reply to comment #7)
> Wouldn't if/else look cleaner?
> 
> if (qtImage.format() == QImage::Format_ARGB32 && premultiplyAlpha)
>     alphaOp = AlphaDoPremultiply;
> else if (qtImage.format() == QImage::Format_ARGB32_Premultiplied && !premultiplyAlpha)
>     alphaOp = AlphaDoUnmultiply;
> else
>     // The image has a format that is not supported in packPixels. We have to convert it here.
>     qtImage = qtImage.convertToFormat(premultiplyAlpha ? QImage::Format_ARGB32_Premultiplied : QImage::Format_ARGB32);

Humm that would cause a call to convertToFormat(Format_ARGB32) if qtImage.format() == QImage::Format_ARGB32 && !premultiplyAlpha. Probably a no-op but not so clean in any case, feel free to ignore.

[WebKit Review Bot]

Comment on attachment 149542 [details]
Patch

Clearing flags on attachment: 149542

Committed r121292: <http://trac.webkit.org/changeset/121292>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.