While running Mozilla's WebGL performance regression tests from http://hg.mozilla.org/users/bjacob_mozilla.com/webgl-perf-tests/raw-file/tip/webgl-performance-tests.html in a debug build of WebKit (reproduced here in Chromium), the following assertion is hit: ASSERT(sourceDataFormat == SourceFormatRGBA32F || sourceDataFormat == SourceFormatRGB32F... in: #0 0x00007f12af6201e3 in WebCore::GraphicsContext3D::packPixels ( this=0x7f129d75c5c0, sourceData=0x7f12988f3000 "4(", sourceDataFormat=WebCore::GraphicsContext3D::SourceFormatBGRA8, width=1024, height=1024, sourceUnpackAlignment=0, destinationFormat=6407, destinationType=5126, alphaOp=WebCore::GraphicsContext3D::AlphaDoUnmultiply, destinationData=0x7f129b0b0000) at ../../third_party/WebKit/Source/WebCore/platform/graphics/GraphicsContext3D.cpp:1545 #1 0x00007f12af6c5d2a in WebCore::GraphicsContext3D::getImageData ( this=0x7f129d75c5c0, image=0x7f129e03d780, format=6407, type=5126, premultiplyAlpha=false, ignoreGammaAndColorProfile=false, outputVector=...) at ../../third_party/WebKit/Source/WebCore/platform/graphics/skia/GraphicsContext3DSkia.cpp:86 #2 0x00007f12af61cbdc in WebCore::GraphicsContext3D::extractImageData ( this=0x7f129d75c5c0, image=0x7f129e03d780, format=6407, type=5126, flipY=false, premultiplyAlpha=false, ignoreGammaAndColorProfile=false, data=...) at ../../third_party/WebKit/Source/WebCore/platform/graphics/GraphicsContext3D.cpp:173 #3 0x00007f12ad9a4685 in WebCore::WebGLRenderingContext::texSubImage2DImpl ( this=0x7f129dfe8580, target=3553, level=0, xoffset=0, yoffset=0, format=6407, type=5126, image=0x7f129e03d780, flipY=false, premultiplyAlpha=false, ec=@0x7fff26b6181c: 0) at ../../third_party/WebKit/Source/WebCore/html/canvas/WebGLRenderingContext.cpp:3725 #4 0x00007f12ad9a4d93 in WebCore::WebGLRenderingContext::texSubImage2D ( this=0x7f129dfe8580, target=3553, level=0, xoffset=0, yoffset=0, format=6407, type=5126, canvas=0x7f129e041400, ec=@0x7fff26b6181c: 0) at ../../third_party/WebKit/Source/WebCore/html/canvas/WebGLRenderingContext.cpp:3809 This test suite is testing operations such as uploading an HTMLCanvasElement via texImage2D to a format and type of RGB and FLOAT. These code paths haven't been tested and are missing in the current code.
Created attachment 141540 [details] Patch
Comment on attachment 141540 [details] Patch Can we add the tests that verify this behavior at the same time as the code change, or is that infeasible?
(In reply to comment #2) > (From update of attachment 141540 [details]) > Can we add the tests that verify this behavior at the same time as the code change, or is that infeasible? It's not feasible. These tests run only on bots with real GPUS, not virtual machines or (generally) headless bots. They will be run on Chromium's GPU waterfall so there will be some automated coverage, though downstream rather than on webkit.org.
Comment on attachment 141540 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=141540&action=review OK. r=me > Source/WebCore/platform/graphics/GraphicsContext3D.cpp:715 > + const float scaleFactor = 1.0f / 255.0f; Nit: this could be hoisted out of the loop. > Source/WebCore/platform/graphics/GraphicsContext3D.cpp:728 > + const float scaleFactor = 1.0f / 255.0f; Same here. > Source/WebCore/platform/graphics/GraphicsContext3D.cpp:1099 > + float scaleFactor = 1.0f / (source[3] ? source[3] : 1.0f); Might be clearer as float scaleFactor = source[3] ? 1.0f / source[3] : 1.0f; > Source/WebCore/platform/graphics/GraphicsContext3D.cpp:1137 > + float scaleFactor = 1.0f / (source[3] ? source[3] : 1.0f); Same here. > Source/WebCore/platform/graphics/GraphicsContext3D.cpp:1178 > + float scaleFactor = 1.0f / (source[3] ? source[3] : 1.0f); Same here. > Source/WebCore/platform/graphics/GraphicsContext3D.cpp:1209 > + float scaleFactor = 1.0f / (source[3] ? source[3] : 1.0f); Same here.
(In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 141540 [details] [details]) > > Can we add the tests that verify this behavior at the same time as the code change, or is that infeasible? > > It's not feasible. These tests run only on bots with real GPUS, not virtual machines or (generally) headless bots. They will be run on Chromium's GPU waterfall so there will be some automated coverage, though downstream rather than on webkit.org. If we can't get that test, can we at least add a test in this patch?
Created attachment 142030 [details] Patch for landing
(In reply to comment #4) > (From update of attachment 141540 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=141540&action=review > > OK. r=me > > > Source/WebCore/platform/graphics/GraphicsContext3D.cpp:715 > > + const float scaleFactor = 1.0f / 255.0f; > > Nit: this could be hoisted out of the loop. > > > Source/WebCore/platform/graphics/GraphicsContext3D.cpp:728 > > + const float scaleFactor = 1.0f / 255.0f; > > Same here. > > > Source/WebCore/platform/graphics/GraphicsContext3D.cpp:1099 > > + float scaleFactor = 1.0f / (source[3] ? source[3] : 1.0f); > > Might be clearer as > float scaleFactor = source[3] ? 1.0f / source[3] : 1.0f; > > > Source/WebCore/platform/graphics/GraphicsContext3D.cpp:1137 > > + float scaleFactor = 1.0f / (source[3] ? source[3] : 1.0f); > > Same here. > > > Source/WebCore/platform/graphics/GraphicsContext3D.cpp:1178 > > + float scaleFactor = 1.0f / (source[3] ? source[3] : 1.0f); > > Same here. > > > Source/WebCore/platform/graphics/GraphicsContext3D.cpp:1209 > > + float scaleFactor = 1.0f / (source[3] ? source[3] : 1.0f); > > Same here. Thanks for your review. Cleanups applied in patch for landing.
Comment on attachment 142030 [details] Patch for landing Absolutely no test at all? How is anyone else supposed to patch this code without regressing this behavior?
(In reply to comment #8) > (From update of attachment 142030 [details]) > Absolutely no test at all? How is anyone else supposed to patch this code without regressing this behavior? Regressions will be caught by developers running the WebGL conformance tests locally on their machine, or in automated fashion on the Chromium GPU waterfall. We could add GPU bots to test these code paths on build.webkit.org. This is a lot of work however and should not block fixing of an issue for which tests exist in a canonical place (even if they aren't, unfortunately, layout tests).
(In reply to comment #9) > (In reply to comment #8) > > (From update of attachment 142030 [details] [details]) > > Absolutely no test at all? How is anyone else supposed to patch this code without regressing this behavior? > > Regressions will be caught by developers running the WebGL conformance tests locally on their machine, or in automated fashion on the Chromium GPU waterfall. We could add GPU bots to test these code paths on build.webkit.org. This is a lot of work however and should not block fixing of an issue for which tests exist in a canonical place (even if they aren't, unfortunately, layout tests). Is the issue lack of (guaranteed) support for FP32 texture formats? Does the current version of Mesa on Chrome's DRT support them? If so, could we turn them into layout tests, and simply mark them as Skipped on non-Chrome platforms? Or just put the tests in platform/chromium? If not, presumably a newer version of Mesa supports FP32, and we can just chuck it in the "do this after Mesa upgrade" bucket.
(In reply to comment #10) > (In reply to comment #9) > > (In reply to comment #8) > > > (From update of attachment 142030 [details] [details] [details]) > > > Absolutely no test at all? How is anyone else supposed to patch this code without regressing this behavior? > > > > Regressions will be caught by developers running the WebGL conformance tests locally on their machine, or in automated fashion on the Chromium GPU waterfall. We could add GPU bots to test these code paths on build.webkit.org. This is a lot of work however and should not block fixing of an issue for which tests exist in a canonical place (even if they aren't, unfortunately, layout tests). > > Is the issue lack of (guaranteed) support for FP32 texture formats? Does the current version of Mesa on Chrome's DRT support them? If so, could we turn them into layout tests, and simply mark them as Skipped on non-Chrome platforms? Or just put the tests in platform/chromium? If not, presumably a newer version of Mesa supports FP32, and we can just chuck it in the "do this after Mesa upgrade" bucket. Yes, the primary issue is that there are no webkit.org bots (even the Chromium bots) that would be able to reliably run these tests. I do agree that we should aim to run them, and have filed https://bugs.webkit.org/show_bug.cgi?id=86516 to track this issue.
I see, I wasn't aware of that restriction.
Comment on attachment 142030 [details] Patch for landing Clearing flags on attachment: 142030 Committed r117191: <http://trac.webkit.org/changeset/117191>
All reviewed patches have been landed. Closing bug.
What (if any) is the symptom of this bug in release builds?
(In reply to comment #15) > What (if any) is the symptom of this bug in release builds? In Chrome the renderer process crashes -- the heap is corrupted by writing beyond the end of a WTF::Vector, TCMalloc catches the corruption and crashes. I didn't test in Safari.
<rdar://problem/11520387>