Closed Bug 1092634 Opened 7 years ago Closed 7 years ago

SVG inset drop-shadow filter example does not look right

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: heycam, Assigned: mstange)

References

(Blocks 1 open bug, )

Details

Attachments

(1 file, 3 obsolete files)

http://www.xanthir.com/b4Yv0 looks good in Chrome and wrong in Firefox.
It looks like the filter region attributes aren't being used.  The FF rendering is the same as what you get in Chrome if you remove x/y/width/height from the <filter>.
Oh. I missed that the component alpha primitive can produce output outside the bounds of its input if you have a <feFuncA> transfer function that turns alpha = 0 into alpha > 0.
Assignee: nobody → mstange
Status: NEW → ASSIGNED
Er, I meant "component transfer primitive".

If you want to work around the Firefox bug you can replace the <feComponentTransfer> primitive with this:
<feFlood fill-color="black"/>
<feComposite in2="SourceAlpha" operator="out"/>
That also makes it work in Safari.

And I just noticed that the color matrix primitive has the same bug.
<feColorMatrix in="SourceAlpha" type="matrix" values="0 0 0 0 0  0 0 0 0 0  0 0 0 0 0  0 0 0 -1 1"/>

The lighting filters also have it, but that's already tracked in bug 1051185.
Ah, so you're optimizing by finding a bounding box of the non-transparent content and implicitly using that as the filter region instead.  That makes sense in this context, since the normal drop-shadow does pay attention to the filter region attributes correctly.
Attached patch patch (obsolete) — Splinter Review
Attachment #8521026 - Flags: review?(bas)
Attachment #8521026 - Flags: review?(bas) → review+
Blocks: 1102014
Attached patch v2 (obsolete) — Splinter Review
This adds D2D filter support using a custom ExtendInput effect.
Attachment #8521026 - Attachment is obsolete: true
Flags: needinfo?(mstange)
Attachment #8529916 - Flags: review?(bas)
Comment on attachment 8529916 [details] [diff] [review]
v2

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

As a first quick look.

::: gfx/2d/DrawTargetD2D1.cpp
@@ +846,5 @@
>      return nullptr;
>    }
>  
>    RadialGradientEffectD2D1::Register(mFactory);
> +  ExtendInputEffectD2D1::Register(mFactory);

Can you add the unregister too?
Huh, must have forgotten to qrefresh.
Attached patch v3 (obsolete) — Splinter Review
Attachment #8529916 - Attachment is obsolete: true
Attachment #8529916 - Flags: review?(bas)
Attachment #8530435 - Flags: review?(bas)
Looks like this completely breaks filter drawing in reftests... :-(
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6f2f1170e82d
Attached patch v4Splinter Review
I had to fix up the D2D1_VECTOR_4F -> D2D1_RECT_L conversion which was overflowing. I also moved the Register/Unregister calls to DrawTargetD2D so that they're also called when D2D1 filters are used with DrawTargetD2D.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=734b1842cddd
Attachment #8530435 - Attachment is obsolete: true
Attachment #8530435 - Flags: review?(bas)
Attachment #8533446 - Flags: review?(bas)
Comment on attachment 8533446 [details] [diff] [review]
v4

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

This looks great.

::: gfx/2d/ExtendInputEffectD2D1.cpp
@@ +21,5 @@
> +        <Effect>
> +            <!-- System Properties -->
> +            <Property name='DisplayName' type='string' value='ExtendInputEffect'/>
> +            <Property name='Author' type='string' value='Mozilla'/>
> +            <Property name='Category' type='string' value='Pattern effects'/>

nit: This was copy-pasted from the RadialGradientEffect, you may want to rename this :).

::: gfx/2d/ExtendInputEffectD2D1.h
@@ +13,5 @@
> +#include "2D.h"
> +#include "mozilla/Attributes.h"
> +
> +// {97143DC6-CBC4-4DD4-A8BA-13342B0BA46D}
> +DEFINE_GUID(CLSID_ExtendInputEffect, 

nit: some whitespace in this file.
Attachment #8533446 - Flags: review?(bas) → review+
https://hg.mozilla.org/mozilla-central/rev/c0d3d80eb41d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Depends on: 1134103
You need to log in before you can comment on or make changes to this bug.