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.
This bug may solve itself with Bug 89938. GraphicsContext3D::extractImageData causes strange message with specific png files.
(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.
Created attachment 149519 [details] Patch
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? :)
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.
Created attachment 149542 [details] Patch
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.
(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.
Comment on attachment 149542 [details] Patch Clearing flags on attachment: 149542 Committed r121292: <http://trac.webkit.org/changeset/121292>
All reviewed patches have been landed. Closing bug.