Bug 54023

This happened after http://crrev.com/74103 by hbono@chromium.org

Sorry blamed the wrong guy and wrong CL. It is skia in r74130. Cc'ed reed@chromium.org

(In reply to comment #2) > Sorry blamed the wrong guy and wrong CL. It is skia in r74130. Cc'ed reed@chromium.org That sounds like the wrong revision.

(In reply to comment #3) > (In reply to comment #2) > > Sorry blamed the wrong guy and wrong CL. It is skia in r74130. Cc'ed reed@chromium.org > > That sounds like the wrong revision. To be clear, that's Chromium revision 74130. http://src.chromium.org/viewvc/chrome?view=rev&revision=74130

[31664:31664:2059838021045:FATAL:SkColorPriv.h(216)] third_party/skia/include/core/SkColorPriv.h:216: failed assertion "r <= a" Backtrace: base::debug::StackTrace::StackTrace() [0x127580e] logging::LogMessage::~LogMessage() [0x128a4e2] SkDebugf_FileLine() [0x15724ff] SkPackARGB32() [0x269036d] WebCore::ImageFrame::setRGBA() [0x2692d5e] WebCore::ImageFrame::setRGBA() [0x2692bee] WebCore::PNGImageDecoder::rowAvailable() [0x269d71d] WebCore::rowAvailable() [0x269c9fa] 0x7f90db042383 0x7f90db04287c 0x7f90db042af3 0x7f90db043c6b WebCore::PNGImageReader::decode() [0x269dbab] WebCore::PNGImageDecoder::decode() [0x269d88c] WebCore::PNGImageDecoder::frameBufferAtIndex() [0x269ccfd] WebCore::ImageSource::frameIsCompleteAtIndex() [0x2683591] WebCore::GraphicsContext3D::getImageData() [0x26e6bcd] WebCore::GraphicsContext3D::extractImageData() [0x26449da] WebCore::WebGLRenderingContext::texImage2DImpl() [0x2395969] WebCore::WebGLRenderingContext::texImage2D() [0x2395f6f] WebCore::WebGLRenderingContextInternal::texImage2D3Callback() [0x24c2d1c] WebCore::WebGLRenderingContextInternal::texImage2DCallback() [0x24c38df] v8::internal::HandleApiCallHelper<>() [0x18b47c3] v8::internal::Builtin_Impl_HandleApiCall() [0x18b2280] v8::internal::Builtin_HandleApiCall() [0x18b2259] 0x7f90af5ce24a

In debug mode (SK_DEBUG defined for Skia), SkPackARGB32() will assert that the specified color is, in fact, legal premultiplied. i.e. a >= r a >= g a >= b I presume that this is the assert that is firing in the stack-trace. The call site is (I think) this: inline void setRGBA(PixelData* dest, unsigned r, unsigned g, unsigned b, unsigned a) { if (m_premultiplyAlpha && !a) *dest = 0; else { if (m_premultiplyAlpha && a < 255) { float alphaPercent = a / 255.0f; r = static_cast<unsigned>(r * alphaPercent); g = static_cast<unsigned>(g * alphaPercent); b = static_cast<unsigned>(b * alphaPercent); } #if PLATFORM(SKIA) *dest = SkPackARGB32(a, r, g, b); #else *dest = (a << 24 | r << 16 | g << 8 | b); #endif } } The caller will apply the alpha before calling, but only if m_premultiplyAlpha is true. I don't know when this is not true (maybe it is always true), but if that is ever false, then we will skip the multiplies, which could make the assert fire. Another cause could be that one of a,r,g,b is just out of range (doubtful), since SkPackARGB32() also asserts that each one is <= 0xFF

Yes in WebGL sometimes we have m_premultiplyAlpha == false. It is a spec requirement. So how should we fix this?

I am testing a roll to Skia 796, which has a version of that macro that skips the premultiply check. When that lands, I will create a webkit patch to use it.

(In reply to comment #8) > I am testing a roll to Skia 796, which has a version of that macro that skips the premultiply check. When that lands, I will create a webkit patch to use it. Great! Thank you!

Created attachment 82622 [details] call non-asserting version of SkPackARGB32

(more notes on patch) The standard way to create a 32bit pixel in skia is to call SkPackARGB32(a,r,g,b). This puts the components in the correct (compile-time) order, and also (in the Debug build) asserts that the components are legal for premultiplied. i.e. r <= a, g <= a, b <= a. This test makes sense for Skia in general, as it only supports premultiplied bitmaps today. However, WebGL is using the same decoder and bitmap to store images that will be passed directly to opengl, and opengl supports premultiplied and non-premultiplied. This change calls a new version of the pack macro: SkPackARGB32NoCheck(a,r,g,b) which skips the asserts on the component values (since any values can be valid for opengl).

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

Looks good. Thanks for the quick fix. When this lands, I will remove the three tests out of test_expectations.txt.

Comment on attachment 82622 [details] call non-asserting version of SkPackARGB32 Looks fine. I patched my workspace locally to still call SkPackARGB32 if m_premultiplyAlpha is true, but it's another if-test in an inner loop that isn't really necessary. r=me

BTW, is the chromium_rev in Source/WebKit/chromium/DEPS at or later than the revision where you incorporated the Skia roll? If not, that is going to need to be updated. I have an unfortunate feeling that the commit queue won't catch this error, so please file another bug ASAP updating DEPS if not.

The commit-queue encountered the following flaky tests while processing attachment 82622 [details]: animations/suspend-resume-animation-events.html bug 51002 (author: cmarrin@apple.com) The commit-queue is continuing to process your patch.

Comment on attachment 82622 [details] call non-asserting version of SkPackARGB32 Clearing flags on attachment: 82622 Committed r78753: <http://trac.webkit.org/changeset/78753>

All reviewed patches have been landed. Closing bug.

http://trac.webkit.org/changeset/78753 might have broken GTK Linux 64-bit Debug

Updated the test expectations for these three tests. Committed r78836: <http://trac.webkit.org/changeset/78836> Note that fast/canvas/webgl/tex-image-with-format-and-type.html still fail from time to time in Linux Debug, but I think that's another issue.