668801 - New shadow drawing logic gives problems

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Bas Schouten (:bas.schouten)]

The new shadow drawing logic where we always draw shadows when shadowColor has a non-zero alpha value is proving problematic.

We end up drawing shadows needlessly a lot where developers have left the shadowColor, but just set the blur to 0 for example. A situation where this happens and significantly affects performance can be found both in the Asteroids benchmark and on Pirateslovedaisies.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We need to backout the patch for bug 656844, and also fix it for Azure.

Comment 2

[Bas Schouten (:bas.schouten)]

Attachment: Part 1: Add code to Azure for non-operator over shadows

Comment 3

[Bas Schouten (:bas.schouten)]

Attachment: Part 2: Fix a small surface clearing bug

Comment 4

[Bas Schouten (:bas.schouten)]

Comment on attachment 543837 [details] [diff] [review]
Part 2: Fix a small surface clearing bug

Uploaded wrongly here.

Comment 5

[Bas Schouten (:bas.schouten)]

Attachment: Part 2: Fix a small surface clearing bug

Comment 6

[Bas Schouten (:bas.schouten)]

Attachment: Part 3: Fix clipping to work with non-operator over shadows

This patch adds code to make shadows work correctly with non-operator over and clipping.

Comment 7

[Bas Schouten (:bas.schouten)]

Attachment: Part 3: Fix clipping to work with non-operator over shadows v2

Removed compiled shader code from the patch.

Comment 8

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 543868 [details] [diff] [review]
Part 3: Fix clipping to work with non-operator over shadows v2

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

::: gfx/2d/DrawTargetD2D.cpp
@@ +121,5 @@
>      if (FAILED(hr)) {
>        gfxWarning() << "Failed to create shared bitmap for old surface.";
>      }
>  
> +    mClippedArea = mDT->GetClippedGeometry();

Nice idea.

@@ +610,5 @@
> +  } else {
> +    mPrivateData->mEffect->GetTechniqueByName("SampleMaskedTexture")->
> +      GetPassByIndex(0)->Apply(0);
> +  }
> +

How about we reverse the condition here and above. That feels sort of better to me.

Comment 9

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/942107e5bca9
http://hg.mozilla.org/mozilla-central/rev/028e71400576
http://hg.mozilla.org/mozilla-central/rev/af10b6844d98
http://hg.mozilla.org/mozilla-central/rev/0ec8b80572a6
http://hg.mozilla.org/mozilla-central/rev/f892e78c8c6d
http://hg.mozilla.org/mozilla-central/rev/f11be63ce31c