Closed Bug 668801 Opened 8 years ago Closed 8 years ago

New shadow drawing logic gives problems

Categories

(Core :: Canvas: 2D, defect)

x86
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla7

People

(Reporter: bas.schouten, Assigned: bas.schouten)

Details

Attachments

(3 files, 2 obsolete files)

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.
We need to backout the patch for bug 656844, and also fix it for Azure.
Assignee: nobody → bas.schouten
Status: NEW → ASSIGNED
Attachment #543827 - Flags: review?(jmuizelaar)
Attachment #543837 - Flags: review?(jmuizelaar)
Comment on attachment 543837 [details] [diff] [review]
Part 2: Fix a small surface clearing bug

Uploaded wrongly here.
Attachment #543837 - Attachment is obsolete: true
Attachment #543837 - Flags: review?(jmuizelaar)
Attachment #543866 - Flags: review?(jmuizelaar)
This patch adds code to make shadows work correctly with non-operator over and clipping.
Attachment #543867 - Flags: review?(jmuizelaar)
Removed compiled shader code from the patch.
Attachment #543867 - Attachment is obsolete: true
Attachment #543868 - Flags: review?(jmuizelaar)
Attachment #543867 - Flags: review?(jmuizelaar)
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.
Attachment #543868 - Flags: review?(jmuizelaar) → review+
Attachment #543866 - Flags: review?(jmuizelaar) → review+
Attachment #543827 - Flags: review?(jmuizelaar)
You need to log in before you can comment on or make changes to this bug.