310682 - Implement shadows for <html:canvas>

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect, P4)

Description

[Takeshi Kurosawa :takenspc]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20051001 Firefox/1.6a1
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20051001 Firefox/1.6a1

Mozilla's <html:canvas> implementation is not support shadows.



Reproducible: Always

Comment 1

[Takeshi Kurosawa :takenspc]

sorry, s/is/does/.

Comment 2

[(not currently active) Ted Mielczarek]

*** Bug 311747 has been marked as a duplicate of this bug. ***

Comment 3

[Philip Taylor]

Attachment: image for demo

Comment 4

[Philip Taylor]

Attachment: shadow demonstration

Comment 5

[Philip Taylor]

Attachment: attempt at patch

Comment 6

[Philip Taylor]

A first attempt at adding shadow support. It's currently lacking error reporting in out-of-memory conditions, since I'm not sure what is the proper way to handle that. It also doesn't attempt to clamp the maximum blur, so you can use up loads of time/memory or cause overflows easily.

The behaviour is based on Safari, as described in <http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2007-June/011799.html>. The last three cases in the attached demonstration (for gradient and pattern fills) are different to Safari (at least on Windows), since I believe Safari is incorrect there.

Comment 7

[Philip Taylor]

Attachment: updated patch

Fixed the error handling, and added a (fairly arbitrary) blur limit, and other minor changes.

Comment 8

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

The patch looks good (though I haven't done a full review on it), but as we're feature frozen I need to figure out if we have a chance of getting this in for 1.9 -- if not, I need to figure out where you can land it, as it's not clear what we do for bugs that are not for 1.9 :)

Comment 9

[Philip Taylor]

http://philip.html5.org/tests/canvas/suite/tests/index.2d.shadow.html has tests for this behaviour.

http://philip.html5.org/tests/canvas/suite/tests/results.html includes results for those tests in WebKit. The causes of the differences are:

* pattern.*, gradient.*: WebKit ignores the pattern/gradient alpha, and draws the shadow as if the shape were solid.
* composite.{3,4}: WebKit doesn't render shadows unless either shadowBlur or shadowOffset{X,Y} is set.
* others: random glitches that go away if you reload the page while feeling lucky.

which all sound like implementation bugs, so I think the patch here is more correct than WebKit and otherwise compatible with it.

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

Vlad: as canvas/svg become a bigger deal for us, do you have any better answers to your questions in comment #8? Not sure what sort of testsuites we have for canvas, but I must sadly admit that drop-shadows are a pretty important UI "zim-zowie" that developers like to be able to use.

Comment 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Yeah, though the drop shadows you want can't/shouldn't be provided by canvas (since you're not building your UIs using drawings on canvas)...

Comment 12

[Eric Butler [:ebutler]]

Attachment: ver 0.1

Rough draft of patch, not ready for full review. Meets specification as defined with a slight variation. Uses shadowColor=(0,0,0,0),shadowOffset=(0,0) as a "don't draw shadows" mode. Working with all items, including text and images.

This patch basically rewrites all the drawing code and, in addition to shadows, takes care of a lot of platform parity issues, particularly with composite operators. It also fixes some draw problems such as global alpha with patterns.

Some improvements remain before it will be ready. These include optimizing to avoid draw calls that would do nothing and cleaning up the code, as well as updating all of the now "failing" todo mochitests.

Comment 13

[Michael Ventnor]

+    // XXX figure out WTF this does

I'll help :)

Basically it transforms all X or Y operations done on that context by that specified amount. So say you set a device offset of -40, -40. If you draw a shape at (42, 42), then on the context it will actually be drawn at (2, 2). The reason we wanted this is so that we didn't need to transform coordinates ourself.

Roc can explain this better than I can, but basically its a way of automatically transforming coordinates. In this case, we use it to transform drawing coordinates from an absolute position to the position on the temporary surface.

Comment 14

[Eric Butler [:ebutler]]

Attachment: ver 1.0

Completed patch, test updates not included. Implements the shadow spec and corrects several other spec issues with rendering. The shadow drawing code also doesn't assume that the shadow style is a color, so it will be trivial to add support for arbitrary patterns (all that needs to be written is a setter/getter).

Also cleans up some of the code to be cleaner, such as using RAII when saving/restoring the context/path. There are other minor optimizations that could be applied, but those are probably better left for another patch.

Comment 15

[Eric Butler [:ebutler]]

Attachment: ver 1.1

Whoops, fixed typo with NS_RGBA instead of NS_RGB.

Comment 16

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 333455 [details] [diff] [review]
ver 1.1

This looks great!  Thanks for wading through all this stuff, will be great to have not just shadows working but a bunch of the other issues fixed.  A few issues before it should land, though, hopefully nothing too large..

>+    /**
>+     * Returns true iff the the given operator should affect areas of the
>+     * destination where the source is transparent. Among other things, this
>+     * implies that a fully transparent source would still affect the canvas.
>+     */
>+    PRBool OperatorAffectsUncoveredAreas(gfxContext::GraphicsOperator op) const
>+    {
>+        return op == gfxContext::OPERATOR_IN ||
>+               op == gfxContext::OPERATOR_OUT ||
>+               op == gfxContext::OPERATOR_DEST_IN ||
>+               op == gfxContext::OPERATOR_DEST_ATOP ||
>+               op == gfxContext::OPERATOR_SOURCE;
>+    }

Let's not do this quite yet, as we discussed on irc -- I would just #if 0 the return and just return PR_FALSE always for now,
and ideally file another bug to figure out these operators for canvas (and reference that bug # in a comment).


> void
>-nsCanvasRenderingContext2D::ApplyStyle(Style aWhichStyle)
>+nsCanvasRenderingContext2D::ApplyStyle(Style aWhichStyle,
>+                                       PRBool aUseGlobalAlpha)
>....
>-    SetThebesColor(CurrentState().colorStyles[aWhichStyle]);
>+    gfxRGBA color(CurrentState().colorStyles[aWhichStyle]);
>+    if (aUseGlobalAlpha)
>+        color.a *= CurrentState().globalAlpha;
>+
>+    mThebes->SetDeviceColor(color);

Hm, I thought we were going to use SetColor here, to get at least semi-correct CMS behaviour?  (that is, a color specified through CSS should match one painted with canvas).  In fact, SetThebesColor used SetColor... so change this back to SetColor.

>+static const gfxFloat SQRT_2 = sqrt(2.0);

A standard constant exists for this, M_SQRT2 -- should already be available at this point.  But just in case, at the start of the file, there's an #ifndef M_PI block.. add #define M_SQRT2 1.41421356237309504880168872420969808 to that block. (For older win32, I believe.)

Ooor.. since it's not actually used anywhere, just get rid of it :)

>+
>+gfxContext*
>+nsCanvasRenderingContext2D::ShadowInitialize(const gfxRect& extents, gfxAlphaBoxBlur& blur)
>+{
>+    gfxPoint shadowOffset = gfxPoint(CurrentState().shadowOffsetX, CurrentState().shadowOffsetY);
>+    gfxIntSize blurRadius;
>+
>+    gfxFloat sigma = CurrentState().shadowBlur > 8 ? sqrt(CurrentState().shadowBlur) : CurrentState().shadowBlur / 2;
>+    // limit to avoid overly huge temp images
>+    if (sigma > SIGMA_MAX)
>+        sigma = SIGMA_MAX;
>+    // XXX may have to scale this by something, like sqrt(2)
>+    blurRadius = gfxAlphaBoxBlur::CalculateBlurRadius(gfxPoint(sigma, sigma));
>+
>+    // calculate extents
>+    gfxRect drawExtents = extents + shadowOffset;
>+    // outest by blur radius so that shadows can be blurred even when it's
>+    // fully outside the clipping region
>+    drawExtents.Outset(blurRadius.height, blurRadius.width,
>+                       blurRadius.height, blurRadius.width);
>+
>+    // clip to avoid making overly huge temp images
>+    gfxMatrix matrix = mThebes->CurrentMatrix();
>+    mThebes->IdentityMatrix();
>+    drawExtents = drawExtents.Intersect(mThebes->GetClipExtents());
>+    mThebes->SetMatrix(matrix);
>+
>+    gfxContext* ctx = blur.Init(drawExtents, blurRadius);
>+
>+    if (!ctx)
>+        return nsnull;
>+
>+    ctx->Translate(shadowOffset);

Hmm.  I don't think this isn't quite right, though it will probably work in most cases.  Why do you need to offset by shadowOffset at all?  It seems like you should just apply that offset when you composite the shadow into the canvas buffer, and not when you draw the contents of the shadowBuffer.  That is, I would just use drawExtents = extents, and not do this translate.  You still would need to modify the device offsets though, to compensate for gfxSize(blurRadius.width, blurRadius.height) -- should be able to grab CurrentTarget from ctx, get its device offsets, and then add that gfxSize to it and reset.  That'll be the most transparent drawing into the shadow buffer for any code.

Actually!  blur.Init will already outset the extents by the blurRadius, so you shouldn't need any of this stuff here.  But there's a bug there, I just put it in bug 450178.

>+    return ctx;
>+}
>+
>+void
>+nsCanvasRenderingContext2D::ShadowFinalize(gfxAlphaBoxBlur& blur)
>+{
>+    ApplyStyle(STYLE_SHADOW);
>+    // matrix was already applied, don't apply it twice

Hrm, was it?  What happens when I have a 2.0 scale and I draw shadows?

>+    gfxMatrix matrix = mThebes->CurrentMatrix();
>+    mThebes->IdentityMatrix();

This is where you'd want to translate by the shadowOffset, before painting.

>+    blur.Paint(mThebes);
>+    mThebes->SetMatrix(matrix);
>+}
>+


>+    gfxContextPathAutoSaveRestore pathSR(mThebes);
>+    gfxContextAutoSaveRestore autoSR(mThebes);

Would having this be something like: 

gfxContextAutoSaveRestore autoSR(mThebes, STATE_AND_PATH);

vs. STATE_ONLY (default) and PATH_ONLY

be cleaner?  I think it might, but I'm not sure; up to you.

Comment 17

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Eric, something I was thinking of -- you could structure the painting like this:

ctx->PushGroup(CONTENT_ALPHA)
render content
src = ctx->PopGroup()

// src now has some type of surface with the alpha content

img = gfxImageSurface(extents + blursize*2, A8)
img->SetDeviceOffsets(-clip_extents.xy + blursize.wh)
ctx2 = gfxContext(img)
ctx2->SetSource(src)
ctx2->Paint()
blur(img)

Now you have a blurred A8 image surface you can use to mask through in img.  In the case of no blurring, just a sharp shadow, you can even optimize and skip the copying to an image surface (since you won't need to blur).  It's not as ideal as rendering directly to an image surface, but it does preserve platform native surfaces, which we could optimize to do faster blurring on if needed.  Also note that in some cases, e.g. win32, you can grab an image surface directly from a win32 surface and modify the pixels directly... though you'd still need to expand out the surface, so maybe that part won't help.

Comment 18

[Eric Butler [:ebutler]]

Attachment: ver 2.0 (different clipping behavior than spec)

Corrections made, and also implements clipping using Vlad's proposed way instead of the spec. Now the drawing model is such that clipping is taken into account before the shadow is blurred and offset, so you can do things such as draw a shape using a clip and get a shadow of it.

One behavior that is altered by implementing shadows this way is that shadows are not cast by off-screen objects. So if an object is halfway off screen, it gets this weird half-shadow cast instead of what would have been the full shadow. This, among other things, causes the majority of Phil Taylor's shadow tests to fail since they count on this behavior for correctness (even though they are testing unrelated things, such as alpha blending).

Vlad, I am still unconvinced that this new way is better than the spec's way of clipping. It's easy enough to emulate the new behavior using the spec's behavior and a second canvas, but it is much harder to emulate shadows being cast from off-screen objects using the new implementation (for one, you couldn't use a same-sized canvas). Additionally, since the new implementation requires two temporary surfaces for every shadow anyway, it's not actually much cheaper than emulating it using the spec behavior and a second canvas. Exposing push/pop group would also make it possible to easily emulate the new behavior using the old, and it would be just as efficient in most cases since that's exactly what canvas is doing internally anyway.

I suppose it's a matter of which use case will show up more: the spec's way makes casting shadows from objects outside the clip simple, whereas the new one makes casting shadows from a shape created via clipping simple. I really have no idea which will be more common, though I can definitely see both being used.

>Would having this be something like: 
>gfxContextAutoSaveRestore autoSR(mThebes, STATE_AND_PATH);
>vs. STATE_ONLY (default) and PATH_ONLY
>be cleaner?  I think it might, but I'm not sure; up to you.

I'm not sure yet either. I left it alone for now.

On another note, I tried to optimize away the gfxAlphaBoxBlur if blur radius was zero and just mask directly. However, on one of my test cases (a lot a fill/stroke text), this optimization was visibly slower by a good 300%. Apparently masking is expensive enough that it's worth drawing onto a temporary to avoid masking the full canvas and only mask a small portion instead.

Comment 19

[Eric Butler [:ebutler]]

Attachment: ver 3.0 (follows spec)

Patch that follows the spec, but includes the fixes from the earlier review. This version will allow the canvas shadow mochitests to work correctly.

Comment 20

[Eric Butler [:ebutler]]

Attachment: canvas test updates

update to the canvas mochitests. With this and the ver 3.0 patch, the tests pass on Windows and Linux. Not sure about Mac since I don't have one on which to test.

Comment 21

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 335211 [details] [diff] [review]
ver 3.0 (follows spec)

Man, the new bugzilla's interdiff viewer actually works!

This looks great, sorry it took so long to get there.  I still cringe a little at what the spec says, but let's see how it goes.

Comment 22

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

http://hg.mozilla.org/mozilla-central/rev/1e8d2532d137
http://hg.mozilla.org/mozilla-central/rev/23739714ed41

Checked in!