Bug 85942

Summary: Assertion failure running Mozilla's WebGL performance regression tests
Product: WebKit Reporter: Kenneth Russell <kbr>
Component: WebGLAssignee: Kenneth Russell <kbr>
Status: RESOLVED FIXED    
Severity: Normal CC: aestes, dino, gman, jamesr, senorblanco, webkit.review.bot, zmo
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 86515    
Attachments:
Description Flags
Patch
none
Patch for landing none

Description Kenneth Russell 2012-05-08 18:28:47 PDT
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.
Comment 1 Kenneth Russell 2012-05-11 18:20:57 PDT
Created attachment 141540 [details]
Patch
Comment 2 James Robinson 2012-05-11 18:28:18 PDT
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?
Comment 3 Kenneth Russell 2012-05-12 17:01:52 PDT
(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 4 Stephen White 2012-05-14 07:07:47 PDT
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.
Comment 5 James Robinson 2012-05-14 10:10:40 PDT
(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?
Comment 6 Kenneth Russell 2012-05-15 12:42:46 PDT
Created attachment 142030 [details]
Patch for landing
Comment 7 Kenneth Russell 2012-05-15 12:43:11 PDT
(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 8 James Robinson 2012-05-15 12:44:39 PDT
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?
Comment 9 Kenneth Russell 2012-05-15 12:48:43 PDT
(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).
Comment 10 Stephen White 2012-05-15 12:57:02 PDT
(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.
Comment 11 Kenneth Russell 2012-05-15 13:18:32 PDT
(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.
Comment 12 James Robinson 2012-05-15 13:23:13 PDT
I see, I wasn't aware of that restriction.
Comment 13 WebKit Review Bot 2012-05-15 17:00:10 PDT
Comment on attachment 142030 [details]
Patch for landing

Clearing flags on attachment: 142030

Committed r117191: <http://trac.webkit.org/changeset/117191>
Comment 14 WebKit Review Bot 2012-05-15 17:00:14 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Andy Estes 2012-05-23 16:13:03 PDT
What (if any) is the symptom of this bug in release builds?
Comment 16 Kenneth Russell 2012-05-23 16:28:18 PDT
(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.
Comment 17 Andy Estes 2012-05-23 16:45:29 PDT
<rdar://problem/11520387>