Bug 85942 – Assertion failure running Mozilla's WebGL performance regression tests

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Kenneth Russell]

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.

[Kenneth Russell]

Created attachment 141540 [details]
Patch

[James Robinson]

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?

[Kenneth Russell]

(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.

[Stephen White]

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.

[James Robinson]

(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?

[Kenneth Russell]

Created attachment 142030 [details]
Patch for landing

[Kenneth Russell]

(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.

[James Robinson]

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?

[Kenneth Russell]

(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).

[Stephen White]

(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.

[Kenneth Russell]

(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.

[James Robinson]

I see, I wasn't aware of that restriction.

[WebKit Review Bot]

Comment on attachment 142030 [details]
Patch for landing

Clearing flags on attachment: 142030

Committed r117191: <http://trac.webkit.org/changeset/117191>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Andy Estes]

What (if any) is the symptom of this bug in release builds?

[Kenneth Russell]

(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.

[Andy Estes]

<rdar://problem/11520387>