Bug 56476 – Canvas2D GPU acceleration should support shadows

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Stephen White]

We should support hard and soft shadows in canvas 2D GPU acceleration, as in the software path.

[Stephen White]

Created attachment 85951 [details]
Patch

[Kenneth Russell]

Comment on attachment 85951 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=85951&action=review

Overall this looks excellent. It's great that your algorithm for this is finally getting incorporated. A couple of questions and comments.

> Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:754
> +    scissorClear(rect.x(), rect.y() - width, rect.width() + width, width);

Should this be rect.x() - width?

> Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:756
> +    scissorClear(rect.x(), rect.maxY(), width, rect.height() + width);

Should this be rect.y() instead of rect.maxY()? And possibly rect.x() - width?

> Source/WebCore/platform/graphics/gpu/BicubicShader.cpp:125
> +    m_context->uniform1i(m_imageLocation, 0);

It seems pretty magic to assume that the input texture comes in on texture unit 0. Is this common in the other shaders? If so, it's fine, but otherwise, at least a comment in the header about the conventions would help.

> Source/WebCore/platform/graphics/gpu/ConvolutionShader.cpp:110
> +    m_context->uniform1i(m_imageLocation, 0);

Same comment here about texture unit 0.

> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.h:167
> +const float kMaxSigma = 4.0f;
> +const int kMaxKernelWidth = 25;

WebKit style actually doesn't use the "k" prefix for constants.

[Stephen White]

(In reply to comment #2)
> (From update of attachment 85951 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=85951&action=review
> 
> Overall this looks excellent. It's great that your algorithm for this is finally getting incorporated. A couple of questions and comments.
> 
> > Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:754
> > +    scissorClear(rect.x(), rect.y() - width, rect.width() + width, width);
> 
> Should this be rect.x() - width?

This is what I was attempting to do:

31111
3   4
3   4
3   4
22224

So the first one starts at x, y - width.  I think that one's correct.

> > Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:756
> > +    scissorClear(rect.x(), rect.maxY(), width, rect.height() + width);
> 
> Should this be rect.y() instead of rect.maxY()? And possibly rect.x() - width?

This is supposed to be doing section 3 above, but it looks wrong to me now.  Will fix.

> > Source/WebCore/platform/graphics/gpu/BicubicShader.cpp:125
> > +    m_context->uniform1i(m_imageLocation, 0);
> 
> It seems pretty magic to assume that the input texture comes in on texture unit 0. Is this common in the other shaders? If so, it's fine, but otherwise, at least a comment in the header about the conventions would help.

I didn't see a point in exposing it from the shaders, since the only call to glActiveTexture() in the canvas context is one that sets it to unit zero (which could probably be removed).  I'll add a comment, though, thanks.

> > Source/WebCore/platform/graphics/gpu/ConvolutionShader.cpp:110
> > +    m_context->uniform1i(m_imageLocation, 0);
> 
> Same comment here about texture unit 0.
> 
> > Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.h:167
> > +const float kMaxSigma = 4.0f;
> > +const int kMaxKernelWidth = 25;
> 
> WebKit style actually doesn't use the "k" prefix for constants.

Thanks.  I can't see anything in the coding style docs, but it seems cFoo is often used in headers, so I'll use that.

[Kenneth Russell]

(In reply to comment #3)
> Thanks.  I can't see anything in the coding style docs, but it seems cFoo is often used in headers, so I'll use that.

Actually from discussions I've seen on IRC I think the conventional wisdom is to just use a capitalized first letter -- no "k" or "c" prefix.

[Stephen White]

Created attachment 85981 [details]
Patch

[Stephen White]

(In reply to comment #4)
> (In reply to comment #3)
> > Thanks.  I can't see anything in the coding style docs, but it seems cFoo is often used in headers, so I'll use that.
> 
> Actually from discussions I've seen on IRC I think the conventional wisdom is to just use a capitalized first letter -- no "k" or "c" prefix.

Really?  That looks a little strange for things that are in the WebCore namespace -- looks a lot like a class name.  I think I'll use cFoo for now, since there's precedent.

[Kenneth Russell]

Comment on attachment 85981 [details]
Patch

Looks great. clearBorders looks better now.

[Stephen White]

Committed r81350: <http://trac.webkit.org/changeset/81350>

[Alexander Pavlov (apavlov)]

Commit r81350 has broken 20 webkit_gpu_tests on the Chromium "Webkit Win" builder, so I'm going to roll out the patch. See the flakiness dashboard link:

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20GPU%20Mesa%20-%20chromium.org&tests=canvas%2Fphilip%2Ftests%2F2d.canvas.readonly.html%2Ccanvas%2Fphilip%2Ftests%2F2d.canvas.reference.html%2Ccanvas%2Fphilip%2Ftests%2F2d.clearRect%2BfillRect.alpha0.5.html%2Ccanvas%2Fphilip%2Ftests%2F2d.clearRect%2BfillRect.alpha0.html%2Ccanvas%2Fphilip%2Ftests%2F2d.clearRect%2BfillRect.basic.html%2Ccanvas%2Fphilip%2Ftests%2F2d.clearRect.basic.html%2Ccanvas%2Fphilip%2Ftests%2F2d.clearRect.clip.html%2Ccanvas%2Fphilip%2Ftests%2F2d.clearRect.globalalpha.html%2Ccanvas%2Fphilip%2Ftests%2F2d.clearRect.globalcomposite.html%2Ccanvas%2Fphilip%2Ftests%2F2d.clearRect.negative.html%2Ccanvas%2Fphilip%2Ftests%2F2d.clearRect.nonfinite.html%2Ccanvas%2Fphilip%2Ftests%2F2d.clearRect.path.html%2Ccanvas%2Fphilip%2Ftests%2F2d.clearRect.shadow.html%2Ccanvas%2Fphilip%2Ftests%2F2d.clearRect.transform.html%2Ccanvas%2Fphilip%2Ftests%2F2d.clearRect.zero.html%2Ccanvas%2Fphilip%2Ftests%2F2d.composite.canvas.copy.html%2Ccanvas%2Fphilip%2Ftests%2F2d.composite.canvas.destination-atop.html%2Ccanvas%2Fphilip%2Ftests%2F2d.composite.canvas.destination-in.html%2Ccanvas%2Fphilip%2Ftests%2F2d.composite.canvas.destination-out.html%2Ccanvas%2Fphilip%2Ftests%2F2d.composite.canvas.destination-over.html

[Stephen White]

I have no idea why these tests are timing out on the canaries.  I can't repro these timeouts on my local machine, in Debug or Release.  I tried passing the same run_webkit_tests command line that the bots use (modulo directory names), but no luck.  This is much more than the shadow tests:  it seems to be every canvas test in GPU mode.  That suggests it's part of the initialization code in SharedGraphicsContext3D.

Other than changes for shadows (which should only affect the tests with shadows, and not the large number of tests which are timing out), the main thing that affects all tests in this CL is that it compiles 2 more shaders, but one of them it compiles in 25 variants, with different loop bounds in the fragment shader.  I thought it might be the shader validator, but it doesn't seem like that would run differently on/off bot hardware.  Could it be OSMesa is doing something weird on a machine without a GPU?  Or on Windows, but not Linux?  I'm perplexed.

[Kenneth Russell]

(In reply to comment #10)
> I have no idea why these tests are timing out on the canaries.  I can't repro these timeouts on my local machine, in Debug or Release.  I tried passing the same run_webkit_tests command line that the bots use (modulo directory names), but no luck.  This is much more than the shadow tests:  it seems to be every canvas test in GPU mode.  That suggests it's part of the initialization code in SharedGraphicsContext3D.
> 
> Other than changes for shadows (which should only affect the tests with shadows, and not the large number of tests which are timing out), the main thing that affects all tests in this CL is that it compiles 2 more shaders, but one of them it compiles in 25 variants, with different loop bounds in the fragment shader.  I thought it might be the shader validator, but it doesn't seem like that would run differently on/off bot hardware.  Could it be OSMesa is doing something weird on a machine without a GPU?  Or on Windows, but not Linux?  I'm perplexed.

When you run these tests locally, are you running DRT with Mesa? If so, it really should behave the same as on the bots regardless of the kind of GPU (or lack thereof) on the system.

BTW, it could certainly be Mesa doing something wrong or bad. Earlier versions of Mesa than the one we currently use (7.9) had terrible bugs in the shader compiler including infinite loops during compilation.

[Stephen White]

Interestingly, it seems only the WinXP bots were affected.  Not only is Linux unaffected, but so were Vista and Win7 (test times went up by about 15 sec overall, but that's to be expected due to the shadow algorithm).  So it seems to be an XP problem.

[Stephen White]

(In reply to comment #11)
> When you run these tests locally, are you running DRT with Mesa? If so, it really should behave the same as on the bots regardless of the kind of GPU (or lack thereof) on the system.

Yes, DRT with Mesa.  Seems like XP is doing something weird (see previous).

> BTW, it could certainly be Mesa doing something wrong or bad. Earlier versions of Mesa than the one we currently use (7.9) had terrible bugs in the shader compiler including infinite loops during compilation.

Good to know.  It seems ok on other OS'es, though, and they're all running the same version of Mesa, AFAIK.

[Kenneth Russell]

(In reply to comment #13)
> (In reply to comment #11)
> > When you run these tests locally, are you running DRT with Mesa? If so, it really should behave the same as on the bots regardless of the kind of GPU (or lack thereof) on the system.
> 
> Yes, DRT with Mesa.  Seems like XP is doing something weird (see previous).

That's strange. I can't imagine why there would be different behavior. Is there any chance you can test on XP locally?

> > BTW, it could certainly be Mesa doing something wrong or bad. Earlier versions of Mesa than the one we currently use (7.9) had terrible bugs in the shader compiler including infinite loops during compilation.
> 
> Good to know.  It seems ok on other OS'es, though, and they're all running the same version of Mesa, AFAIK.

[Stephen White]

(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #11)
> > > When you run these tests locally, are you running DRT with Mesa? If so, it really should behave the same as on the bots regardless of the kind of GPU (or lack thereof) on the system.
> > 
> > Yes, DRT with Mesa.  Seems like XP is doing something weird (see previous).
> 
> That's strange. I can't imagine why there would be different behavior. Is there any chance you can test on XP locally?

I just installed "Windows XP Mode", a VM for Win7, and I can repro the slowdown there.  Still investigating.

[Stephen White]

(In reply to comment #15)

> I just installed "Windows XP Mode", a VM for Win7, and I can repro the slowdown there.  Still investigating.

Seems to be a bug in talloc (or a bug in MSVC's vsnprintf implementation, depending on how you look at it).  Fix up for review at http://codereview.chromium.org/6711035/.

[Stephen White]

Committed r81509: <http://trac.webkit.org/changeset/81509>