884888 - SkiaGL has problems drawing shadows

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[James Willcox (:snorp) ([e-mail]) (he/him)]

Drawing a solid filled rect with a shadow renders incorrectly with SkiaGL. The shadow appears in the wrong location and larger than specified.

Comment 1

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Expected shadow output

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Actual output on SkiaGL/Android (ignore the scaling difference)

Comment 3

[James Willcox (:snorp) ([e-mail]) (he/him)]

Matt if you have a chance to look at this it would be appreciated. I fear it is some issue with the Skia gpu backend, though. Our stuff in DrawTargetSkia looks to be fine, and works ok with the software backend.

Comment 4

[Matt Woodrow (:mattwoodrow)]

It's definitely a problem with the Skia GPU code.

When drawing an image with a shadow, you're supposed to use the alpha channel of the source image as the mask to blur.

The skia code written for blurring doesn't do this, and just uses the path (usually a rect) for the mask. This is what chrome is using.

For a image with transparent sections, only the outside of the rect gets blurred, which is often transparent pixels. The interior of the image doesn't get blurring.

See this testcase which doesn't blur on chrome, but does on firefox: http://jsfiddle.net/8dbkb/3/

We use an SkRasterizer to convert the source image's alpha channel into a mask to be used for blurring, and the GPU backend just ignores this entirely.

We can make the slow path for this work fairly easily, doing the whole thing on the GPU would require more serious changes.

Quick solution: 

Don't bother calling drawWithGPUMaskFilter if paint.getRasterizer() returns something.

Pass the rasterizer into drawWithMaskFilter

The first call, to SkDraw::DrawToMask is generating us a mask using the path.

If we have a rasterizer, do this instead: http://mxr.mozilla.org/mozilla-central/source/gfx/skia/src/core/SkDraw.cpp#971


Ideally we'd be able to do that all on the GPU. I think we should hack up this fix, and file a skia bug and convince them to do the optimized version. Might need to convince them that their version is broken, and not to spec.

Comment 5

[Matt Woodrow (:mattwoodrow)]

I can take a look at doing this next week.

What is the state of SkiaGL on various branches? i.e. which branch should I be using to test and fix this?

Anything special I need to do to get it enabled on mac?

Comment 6

[James Willcox (:snorp) ([e-mail]) (he/him)]

The graphics branch is where we've been doing our work, but I think this bug is mostly independent of that stuff. That branch should work out of the box on mac with SkiaGL enabled.

Comment 7

[Matt Woodrow (:mattwoodrow)]

Attachment: Dont ignore the paints rasterizer

Looks like it works. It is going to make us fall off the fast path for shadows though, since this does them on the CPU.

I haven't run our tests with this, can someone do that and make sure it fixes what we expect.

We still want to file a skia bug and try get a GPU implementation for this

Comment 8

[James Willcox (:snorp) ([e-mail]) (he/him)]

Looks like it works to me, but it still fails some shadow tests in test_canvas.html. I had to mess with it a bit to build on the graphics branch, though, so maybe I messed something up.

Comment 9

[James Willcox (:snorp) ([e-mail]) (he/him)]

I guess maybe we aren't handling alpha correctly now for the shadow? Following test (and others) still fails with this patch (test_canvas.html, c501):

ctx.fillStyle = '#f00';
ctx.fillRect(0, 0, 100, 50);
ctx.fillStyle = '#f00'; // (work around broken Firefox globalAlpha caching)
ctx.shadowColor = '#00f';
ctx.shadowOffsetY = 50;
ctx.globalAlpha = 0.5;
ctx.fillRect(0, -50, 100, 50);

isPixel(ctx, 50,25, 127,0,127,255, 2);


Test expects the rect to be purple, but it is blue.

Comment 10

[Matt Woodrow (:mattwoodrow)]

Attachment: Dont ignore the paints rasterizer v2

Fixed the bug with c501.

The alpha turned out to be a red herring. The issue was a zero blur radius meant that we skipped the code I changed entirely.

I can't get test_canvas to run without crashing, so I'm not sure if this fixes all the remaining shadow tests.

Comment 11

[Matt Woodrow (:mattwoodrow)]

Attachment: Dont ignore the paints rasterizer v3

Tidied it up a bit.

Comment 12

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Modified for Skia on graphics branch

I modified this to work on the graphics branch, and I also get a crash. Again, it's possible I screwed something up to cause.

#0  0x40121630 in memcpy () from /Users/snorp/jimdb/lib/015d21d4fe181a0e/system/lib/libc.so
#1  0x6febd2aa in GrGpuGL::uploadTexData (this=0x88f10000, desc=..., isNewTexture=<optimized out>, left=0, top=0, width=16, height=25, 
    dataConfig=kAlpha_8_GrPixelConfig, data=0x1, rowBytes=1779286880) at /Volumes/Slow/source/mozilla-graphics/gfx/skia/src/gpu/gl/GrGpuGL.cpp:620
#2  0x6febdafc in GrGpuGL::onWriteTexturePixels (this=0x88f10000, texture=0x883a9cc0, left=0, top=0, width=1878070516, height=25, 
    config=kAlpha_8_GrPixelConfig, buffer=0x1, rowBytes=1779286880) at /Volumes/Slow/source/mozilla-graphics/gfx/skia/src/gpu/gl/GrGpuGL.cpp:509
#3  0x6fea61e2 in GrGpu::writeTexturePixels (this=0x88f10000, texture=0x883a9cc0, left=0, top=0, width=1878070516, height=25, 
    config=kAlpha_8_GrPixelConfig, buffer=0x1, rowBytes=1779286880) at /Volumes/Slow/source/mozilla-graphics/gfx/skia/src/gpu/GrGpu.cpp:240
#4  0x6feae8dc in GrTexture::writePixels (this=0x883a9cc0, left=0, top=0, width=1878070516, height=25, config=kAlpha_8_GrPixelConfig, buffer=0x1, 
    rowBytes=1779286880, pixelOpsFlags=0) at /Volumes/Slow/source/mozilla-graphics/gfx/skia/src/gpu/GrTexture.cpp:73
#5  0x6feb0c70 in drawWithMaskFilter (style=<optimized out>, grp=0x6a0dc36c, bounder=<optimized out>, clip=..., rasterizer=<optimized out>, 
    filter=<optimized out>, devPath=..., context=0x88f38800) at /Volumes/Slow/source/mozilla-graphics/gfx/skia/src/gpu/SkGpuDevice.cpp:992
#6  SkGpuDevice::drawPath (this=0x88f50100, draw=..., origSrcPath=..., paint=..., prePathMatrix=0x0, pathIsMutable=true)
    at /Volumes/Slow/source/mozilla-graphics/gfx/skia/src/gpu/SkGpuDevice.cpp:1082
#7  0x6feb1038 in SkGpuDevice::drawRect (this=0x88f50100, draw=..., rect=..., paint=...)
    at /Volumes/Slow/source/mozilla-graphics/gfx/skia/src/gpu/SkGpuDevice.cpp:701
#8  0x6fed8f9a in SkCanvas::drawRect (this=0x86b79ef0, r=..., paint=...)
    at /Volumes/Slow/source/mozilla-graphics/gfx/skia/src/core/SkCanvas.cpp:1655
#9  0x6fe8b9aa in mozilla::gfx::DrawTargetSkia::DrawSurfaceWithShadow (this=0x883a9860, aSurface=0x88f3f600, aDest=..., aColor=..., aOffset=..., 
    aSigma=0, aOperator=mozilla::gfx::OP_OVER) at /Volumes/Slow/source/mozilla-graphics/gfx/2d/DrawTargetSkia.cpp:356

Comment 13

[James Willcox (:snorp) ([e-mail]) (he/him)]

It looks like dstM.fImage is bogus (0x1) in drawWithMaskFilter(). Not sure why yet.

Comment 14

[James Willcox (:snorp) ([e-mail]) (he/him)]

Ah, it's uninitialized. Zeroing out avoids the crash, but this still fails a bunch of tests and does not look correct in the jsfiddle either. Matt it would be very helpful if you can work on the graphics branch to take my fumbling out of the equation :)

Comment 15

[Matt Woodrow (:mattwoodrow)]

Attachment: Dont ignore the paints rasterizer v4

Rebased to graphics branch, runs with only 5 remaining failures (none of which have shadows).

Comment 16

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 768060 [details] [diff] [review]
Dont ignore the paints rasterizer v4

Review of attachment 768060 [details] [diff] [review]:
-----------------------------------------------------------------

I actually pass all of test_canvas.html with graphics branch + this patch, so I'm calling it a r+

Comment 17

[James Willcox (:snorp) ([e-mail]) (he/him)]

We should get this upstreamed ASAP. We will never be closer to mainline than we are now, and I think the Chrome folks would like this too.

Comment 18

[Matt Woodrow (:mattwoodrow)]

Attachment: Do the blurring on the GPU

This is totally not ready to land :) Passes all the shadow tests though.

I got a shutdown crash (when destroying the DrawTargetSkia) in GrResourceEntry::validate - http://pastebin.mozilla.org/2566690

It's also definitely not optimal.

When drawing shadows, our source is *always* an image (even if the canvas api calls were a fill rect), CanvasRenderingContext2D allocates a temporary image and rasterizes the command to the first.

So we're running the SkRasterizer to convert this source image (probably RGBA32) into an A8 SkMask.

Then we upload that to a texture, but most GPU's don't support render-to-texture with a8 (according to the inline comments here), so the texture is RGBA32 and we're going to be converting again during the upload.

Then we do the blur on that texture on the gpu to generate our actual mask texture.

Then we draw the source rect, solid colour source through the mask to get our shadow.

Skipping the rgba32 -> a8 -> rgba32 roundabout would be worthwhile, but it probably requires API additions to SkRasterizer.

Comment 19

[James Willcox (:snorp) ([e-mail]) (he/him)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b3d8eb251576

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b3d8eb251576