54023 2011-02-08 11:16:58 -0800 [chromium] Three texture-related webgl tests failing in DEBUG due to skia change 2011-02-17 09:08:02 -0800 1 1 1 Unclassified WebKit WebGL 528+ (Nightly build) PC OS X 10.5 RESOLVED FIXED P2 Normal --- 1 zmo reed abarth commit-queue enne eric hbono kbr reed webkit.review.bot oldest_to_newest 347552 0 zmo 2011-02-08 11:16:58 -0800 Put them to test expectations for now (http://trac.webkit.org/changeset/77959) Need to investigate. fast/canvas/webgl/gl-teximage.html fast/canvas/webgl/tex-image-with-format-and-type.html fast/canvas/webgl/texture-transparent-pixels-initialized.html 347553 1 zmo 2011-02-08 11:18:47 -0800 This happened after http://crrev.com/74103 by hbono@chromium.org 347564 2 zmo 2011-02-08 11:34:46 -0800 Sorry blamed the wrong guy and wrong CL. It is skia in r74130. Cc'ed reed@chromium.org 347695 3 kbr 2011-02-08 14:40:29 -0800 (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. 347698 4 kbr 2011-02-08 14:41:35 -0800 (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 350873 5 zmo 2011-02-14 14:09:42 -0800 [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 350887 6 reed 2011-02-14 14:27:34 -0800 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 350892 7 zmo 2011-02-14 14:37:25 -0800 Yes in WebGL sometimes we have m_premultiplyAlpha == false. It is a spec requirement. So how should we fix this? 351259 8 reed 2011-02-15 07:54:02 -0800 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. 351327 9 zmo 2011-02-15 09:05:48 -0800 (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! 352015 10 82622 reed 2011-02-16 06:11:31 -0800 Created attachment 82622 call non-asserting version of SkPackARGB32 352016 11 reed 2011-02-16 06:12:01 -0800 (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). 352018 12 reed 2011-02-16 06:12:33 -0800 *** Bug 54550 has been marked as a duplicate of this bug. *** 352131 13 zmo 2011-02-16 09:22:11 -0800 Looks good. Thanks for the quick fix. When this lands, I will remove the three tests out of test_expectations.txt. 352257 14 82622 kbr 2011-02-16 12:29:55 -0800 Comment on attachment 82622 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 352259 15 kbr 2011-02-16 12:35:03 -0800 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. 352473 16 commit-queue 2011-02-16 17:07:53 -0800 The commit-queue encountered the following flaky tests while processing attachment 82622: animations/suspend-resume-animation-events.html bug 51002 (author: cmarrin@apple.com) The commit-queue is continuing to process your patch. 352474 17 82622 commit-queue 2011-02-16 17:09:11 -0800 Comment on attachment 82622 call non-asserting version of SkPackARGB32 Clearing flags on attachment: 82622 Committed r78753: <http://trac.webkit.org/changeset/78753> 352475 18 commit-queue 2011-02-16 17:09:17 -0800 All reviewed patches have been landed. Closing bug. 352523 19 webkit.review.bot 2011-02-16 18:50:17 -0800 http://trac.webkit.org/changeset/78753 might have broken GTK Linux 64-bit Debug 352952 20 zmo 2011-02-17 09:08:02 -0800 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. 82622 2011-02-16 06:11:31 -0800 2011-02-16 17:09:11 -0800 call non-asserting version of SkPackARGB32 packargb.diff text/plain 2028 reed SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA3ODcwMSkKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTkgQEAKKzIwMTEtMDItMTYgIE1pa2UgUmVlZCAgPHJlZWRAZ29vZ2xlLmNvbT4K KworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBVc2Ugbm9u LWFzc2VydGluZyBwYWNrIGZ1bmN0aW9uIGZvciBkZWNvZGluZyBpbWFnZXMsIHNpbmNlIHdlYmds IG1heSB3YW50CisgICAgICAgIGEgbm9uLXByZW11bHRpcGxpZWQgdmVyc2lvbiBvZiB0aGUgaW1h Z2UuCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD01NDAy MworCisgICAgICAgIE5vIG5ldyB0ZXN0cy4KKyAgICAgICAgZmFzdC9jYW52YXMvd2ViZ2wvZ2wt dGV4aW1hZ2UuaHRtbAorICAgICAgICBmYXN0L2NhbnZhcy93ZWJnbC90ZXgtaW1hZ2Utd2l0aC1m b3JtYXQtYW5kLXR5cGUuaHRtbAorICAgICAgICBmYXN0L2NhbnZhcy93ZWJnbC90ZXh0dXJlLXRy YW5zcGFyZW50LXBpeGVscy1pbml0aWFsaXplZC5odG1sCisKKyAgICAgICAgKiBwbGF0Zm9ybS9p bWFnZS1kZWNvZGVycy9JbWFnZURlY29kZXIuaDoKKyAgICAgICAgKFdlYkNvcmU6OkltYWdlRnJh bWU6OnNldFJHQkEpOgorCiAyMDExLTAyLTE0ICBNaWtoYWlsIE5hZ2Fub3YgIDxtbmFnYW5vdkBj aHJvbWl1bS5vcmc+CiAKICAgICAgICAgUmV2aWV3ZWQgYnkgUGF2ZWwgRmVsZG1hbi4KSW5kZXg6 IFdlYkNvcmUvcGxhdGZvcm0vaW1hZ2UtZGVjb2RlcnMvSW1hZ2VEZWNvZGVyLmgKPT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PQotLS0gV2ViQ29yZS9wbGF0Zm9ybS9pbWFnZS1kZWNvZGVycy9JbWFnZURlY29kZXIuaAkocmV2 aXNpb24gNzg2MzUpCisrKyBXZWJDb3JlL3BsYXRmb3JtL2ltYWdlLWRlY29kZXJzL0ltYWdlRGVj b2Rlci5oCSh3b3JraW5nIGNvcHkpCkBAIC0yMyw3ICsyMyw3IEBACiAgKiBQUk9GSVRTOyBPUiBC VVNJTkVTUyBJTlRFUlJVUFRJT04pIEhPV0VWRVIgQ0FVU0VEIEFORCBPTiBBTlkgVEhFT1JZCiAg KiBPRiBMSUFCSUxJVFksIFdIRVRIRVIgSU4gQ09OVFJBQ1QsIFNUUklDVCBMSUFCSUxJVFksIE9S IFRPUlQKICAqIChJTkNMVURJTkcgTkVHTElHRU5DRSBPUiBPVEhFUldJU0UpIEFSSVNJTkcgSU4g QU5ZIFdBWSBPVVQgT0YgVEhFIFVTRQotICogT0YgVEhJUyBTT0ZUV0FSRSwgRVZFTiBJRiBBRFZJ U0VEIE9GIFRIRSBQT1NTSUJJTElUWSBPRiBTVUNIIERBTUFHRS4gCisgKiBPRiBUSElTIFNPRlRX QVJFLCBFVkVOIElGIEFEVklTRUQgT0YgVEhFIFBPU1NJQklMSVRZIE9GIFNVQ0ggREFNQUdFLgog ICovCiAKICNpZm5kZWYgSW1hZ2VEZWNvZGVyX2gKQEAgLTE3Niw3ICsxNzYsMTAgQEAgbmFtZXNw YWNlIFdlYkNvcmUgewogICAgICAgICAgICAgICAgICAgICBiID0gc3RhdGljX2Nhc3Q8dW5zaWdu ZWQ+KGIgKiBhbHBoYVBlcmNlbnQpOwogICAgICAgICAgICAgICAgIH0KICNpZiBQTEFURk9STShT S0lBKQotICAgICAgICAgICAgICAgICpkZXN0ID0gU2tQYWNrQVJHQjMyKGEsIHIsIGcsIGIpOwor ICAgICAgICAgICAgICAgIC8vIHdlIGFyZSBzdXJlIHRvIGNhbGwgdGhlIE5vQ2hlY2sgdmVyc2lv biwgc2luY2Ugd2UgbWF5CisgICAgICAgICAgICAgICAgLy8gZGVsaWJlcmF0ZWx5IHBhc3Mgbm9u LXByZW11bHRpcGxpZWQgdmFsdWVzLCBhbmQgd2UgZG9uJ3Qgd2FudAorICAgICAgICAgICAgICAg IC8vIGFuIGFzc2VydC4KKyAgICAgICAgICAgICAgICAqZGVzdCA9IFNrUGFja0FSR0IzMk5vQ2hl Y2soYSwgciwgZywgYik7CiAjZWxzZQogICAgICAgICAgICAgICAgICpkZXN0ID0gKGEgPDwgMjQg fCByIDw8IDE2IHwgZyA8PCA4IHwgYik7CiAjZW5kaWYK