Render CSS Filters using Moz2D

VERIFIED FIXED in mozilla34

Status

()

defect
VERIFIED FIXED
6 years ago
3 years ago

People

(Reporter: mvujovic, Assigned: mvujovic)

Tracking

(Depends on 4 bugs, Blocks 1 bug, {dev-doc-complete})

unspecified
mozilla34
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(relnote-firefox -)

Details

Attachments

(33 attachments, 33 obsolete attachments)

35.78 KB, patch
RyanVM
: checkin+
Details | Diff | Splinter Review
27.58 KB, patch
RyanVM
: checkin+
Details | Diff | Splinter Review
17.61 KB, patch
roc
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
33.44 KB, patch
RyanVM
: checkin+
Details | Diff | Splinter Review
95.75 KB, patch
RyanVM
: checkin+
Details | Diff | Splinter Review
13.73 KB, patch
RyanVM
: checkin+
Details | Diff | Splinter Review
41.63 KB, patch
hsivonen
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
17.48 KB, patch
RyanVM
: checkin+
Details | Diff | Splinter Review
6.56 KB, patch
mstange
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
9.34 KB, patch
Details | Diff | Splinter Review
29.52 KB, patch
RyanVM
: checkin+
Details | Diff | Splinter Review
3.95 KB, patch
mstange
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
16.38 KB, patch
mstange
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
14.08 KB, patch
Details | Diff | Splinter Review
9.00 KB, patch
longsonr
: review+
Details | Diff | Splinter Review
5.45 KB, patch
mstange
: review+
Details | Diff | Splinter Review
736 bytes, patch
longsonr
: review+
Details | Diff | Splinter Review
40.37 KB, patch
mstange
: review+
mvujovic
: checkin+
Details | Diff | Splinter Review
1.75 KB, patch
mstange
: review+
mvujovic
: checkin+
Details | Diff | Splinter Review
44.74 KB, patch
mstange
: review+
mvujovic
: checkin+
Details | Diff | Splinter Review
15.06 KB, text/html
Details
24.95 KB, text/html
Details
11.81 KB, image/png
Details
19.27 KB, patch
mvujovic
: checkin+
Details | Diff | Splinter Review
1.57 KB, patch
longsonr
: review+
mvujovic
: checkin+
Details | Diff | Splinter Review
22.98 KB, patch
mvujovic
: checkin+
Details | Diff | Splinter Review
17.09 KB, patch
mvujovic
: checkin+
Details | Diff | Splinter Review
15.64 KB, patch
mstange
: review+
mvujovic
: checkin+
Details | Diff | Splinter Review
15.12 KB, patch
mvujovic
: checkin+
Details | Diff | Splinter Review
16.64 KB, patch
mstange
: review+
mvujovic
: checkin+
Details | Diff | Splinter Review
16.75 KB, patch
mstange
: review+
mvujovic
: checkin+
Details | Diff | Splinter Review
17.25 KB, patch
mstange
: review+
mvujovic
: checkin+
Details | Diff | Splinter Review
17.73 KB, patch
mvujovic
: checkin+
Details | Diff | Splinter Review
Assignee

Description

6 years ago
Right now, we have CSS filter information stored in an nsStyleFilter chain. We need to turn it into a FilterDescription and render it. This will probably involve generalizing the logic in nsSVGFilterInstance for both CSS and SVG filters.
Assignee

Updated

6 years ago
Blocks: 869828
Assignee

Updated

6 years ago
Assignee: nobody → mvujovic
Assignee

Comment 1

6 years ago
Posted patch Big Patch (obsolete) — Splinter Review
Hi Markus,

I’ve implemented CSS Filters support in a local branch of mine. For brevity, I only support CSS blur(), but adding the rest of the CSS shorthands is easy with the new architecture. I also don’t support filterRes for simplicity and because it’s removed from the spec. The changes pass all of the existing SVG filters reftests (layout/reftests/svg/filters/).

I’ve attached the patch on this bug. I haven’t had a chance to rebase it yet (i.e. I branched early this month).

The patch is a little hard to read since I’ve rearranged so much code. I recommend looking at the new classes:
https://github.com/mvujovic/gecko-dev/blob/css-filters-rendering/layout/svg/nsFilterInstance.h
https://github.com/mvujovic/gecko-dev/blob/css-filters-rendering/layout/svg/nsFilterInstance.cpp
https://github.com/mvujovic/gecko-dev/blob/css-filters-rendering/layout/svg/nsCSSFilterInstance.h
https://github.com/mvujovic/gecko-dev/blob/css-filters-rendering/layout/svg/nsCSSFilterInstance.cpp
https://github.com/mvujovic/gecko-dev/blob/css-filters-rendering/layout/svg/nsSVGFilterInstance.h
https://github.com/mvujovic/gecko-dev/blob/css-filters-rendering/layout/svg/nsSVGFilterInstance.cpp

The whole branch is up on GitHub:
https://github.com/mvujovic/gecko-dev/tree/css-filters-rendering

(By the way, I tried my previous plan of creating a nsFilterInstance base class with nsSVGFilterInstance, nsCSSFilterInstance subclasses, but it didn’t look good, so I scrapped it.) 

Here’s a walkthrough of the architectural changes:

Before the patch, nsSVGFilterInstance both:
- Built the FilterDescription graph from an SVG <filter> element.
- Rendered the FilterDescription graph.

I’ve separated these concerns across some new classes:

nsFilterInstance:
- Creates nsSVGFilterInstance(s) and nsCSSFilterInstance(s) as needed, which will build up the FilterDescription graph. 
- Renders the FilterDescription graph.

nsSVGFilterInstance:
- Adds to the FilterDescription graph from an SVG <filter> element.

nsCSSFilterInstance:
- Adds to the FilterDescription graph from a CSS filter (nsStyleFilter).

Some other changes include:
- In general, we used to create an nsSVGFilterInstance with a URL (and the corresponding nsSVGFilterFrame). Now, we create an nsFilterInstance with an nsStyleFilter chain.
- I removed the nsAutoFilterInstance helper class and pushed its logic into nsSVGFilterInstance. The logic computes the SVG filter region and various transforms.
- I removed the following functions from nsSVGFilterFrame: PaintFilteredFrame, GetPostFilterDirtyArea, GetPreFilterNeededArea, GetPostFilterBounds. Before, these built an nsAutoFilterInstance (which built an nsSVGFilterInstance), and called a related function on the nsSVGFilterInstance. Now, these functions are now called directly from nsFilterInstance. (See call stacks below)

In general, the previous call stacks looked like:

- nsSVGIntegrationUtils function call
  - nsSVGFilterFrame function call
    - nsAutoFilterInstance temporarily created 
      - nsSVGFilterInstance temporarily created  
        - nsSVGFilterFrame function call to help compute something

The new call stacks look like:

- nsSVGIntegrationUtils function call 
  - nsFilterInstance temporarily created
    - nsSVGFilterInstance temporarily created (if needed for an SVG filter)
      - nsSVGFilterFrame function call to help compute something
        - nsCSSFilterInstance temporarily created (if needed for a CSS filter)

As you can see, nsSVGIntegrationUtils doesn’t call into nsSVGFilterFrame anymore. This is important because when we have a CSS filter chain, there is no SVG <filter> element and thus, no nsSVGFilterFrame. Also, the rest of the system (layout, rendering, etc.) can treat multiple SVG and CSS filters chained together as one nsFilterInstance (with one FilterDescription graph under the hood). Note that nsFilterInstance uses the FilterSupport functions to compute the overall filter region across the CSS / SVG filter chain.

Does this sound good overall? If so, I can start cutting up the patch and pushing it in much more reviewable / manageable segments.

Thanks!
Max
Attachment #8368938 - Flags: feedback?(mstange)
Comment on attachment 8368938 [details] [diff] [review]
Big Patch

Very nice! I'll take a look next week, but this looks really promising.

Roc will want to look at this, too.
Attachment #8368938 - Flags: feedback?(roc)
Assignee

Comment 3

6 years ago
(In reply to Markus Stange [:mstange] from comment #2)
> Comment on attachment 8368938 [details] [diff] [review]
> Big Patch
> 
> Very nice! I'll take a look next week, but this looks really promising.
> 
> Roc will want to look at this, too.

Sounds great. Thanks Markus!

I noticed one mistake in my first comment:

(In reply to Max Vujovic from comment #1)
> The new call stacks look like:
> 
> - nsSVGIntegrationUtils function call 
>   - nsFilterInstance temporarily created
>     - nsSVGFilterInstance temporarily created (if needed for an SVG filter)
>       - nsSVGFilterFrame function call to help compute something
>         - nsCSSFilterInstance temporarily created (if needed for a CSS filter)

The new call stack should look like:

- nsSVGIntegrationUtils function call 
  - nsFilterInstance temporarily created
    - nsSVGFilterInstance temporarily created (if needed for an SVG filter)
      - nsSVGFilterFrame function call to help compute something
    - nsCSSFilterInstance temporarily created (if needed for a CSS filter)

That is, "nsCSSFilterInstance temporarily created" should be at the same level as "nsSVGFilterInstance temporarily created" (not under "nsSVGFilterFrame function call...").
Any way to break the patch down into smaller pieces? Separating refactoring from the new feature would be a very good thing.
Assignee

Comment 5

6 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #4)
> Any way to break the patch down into smaller pieces? Separating refactoring
> from the new feature would be a very good thing.

Absolutely. I'll start breaking down the patch into smaller pieces for review.

The big patch and my first comment show the design that the smaller patches will be working toward :)
Assignee

Comment 6

6 years ago
In the new architecture, nsSVGFilterInstance is responsible for computing its own filter region. This patch moves the filter region calculations and transform calculations from nsAutoFilterInstance to nsSVGFilterInstance, and removes the nsAutoFilterInstance class.

Callers now create an nsSVGFilterInstance where they used to create an nsAutoFilterInstance. Eventually, callers will create an nsFilterInstance, which may internally create an nsSVGFilterInstance if there is an SVG reference filter in a CSS filter chain.
Attachment #8369731 - Flags: review?(roc)
Assignee

Comment 7

6 years ago
This patch moves MapFrameRectToFilterSpace() and GetUserToFrameSpaceInCSSPxTransform() from nsSVGFilterFrame to nsSVGFilterInstance. The filter region calculations from the previous patch call these functions, so Part 1 and Part 2 should land together. (I just realized the previous patch built on Mac, but not on Linux.)

These functions must move anyway because the nsSVGFilterInstance public functions (GetPostFilterDirtyArea, GetPreFilterNeededArea, GetPostFilterBounds) will change from providing results in filter space to providing results in frame space.
Attachment #8370479 - Flags: review?(roc)
Assignee

Comment 8

6 years ago
Single patch combining parts 1 and 2, since these must be landed together.

Try results look good: https://tbpl.mozilla.org/?tree=Try&rev=329fee998611
Attachment #8368938 - Attachment is obsolete: true
Attachment #8369731 - Attachment is obsolete: true
Attachment #8370479 - Attachment is obsolete: true
Attachment #8368938 - Flags: feedback?(roc)
Attachment #8368938 - Flags: feedback?(mstange)
Assignee

Comment 9

6 years ago
Checkin-needed for the patch "Parts 1 & 2 for landing" please.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/fb4ffbea406d
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [leave open]
Status: REOPENED → ASSIGNED
Assignee

Comment 12

6 years ago
This patch moves the following nsSVGFilterFrame methods to nsSVGFilterInstance as static methods:
- PaintFilteredFrame
- GetPostFilterDirtyArea
- GetPreFilterNeededArea
- GetPostFilterBounds

These are the primary connection points with the rest of the system (nsSVGUtils and nsSVGIntegrationUtils).

These functions take in an nsSVGFilterFrame and pass it to the nsSVGFilterInstance they create. Soon, they will take in an nsStyleFilter chain instead.

Also, I've changed the instance methods that GetPostFilterDirtyArea, GetPreFilterNeededArea, and GetPostFilterBounds   call to return rects in frame space instead of filter space. This makes the transformation code appear more encapsulated inside nsSVGFilterInstance.
Attachment #8370892 - Attachment is obsolete: true
Attachment #8371809 - Flags: review?(roc)
Assignee

Comment 13

6 years ago
Checkin-needed for the patch "Part 3: Call nsSVGFilterInstance directly instead of nsSVGFilterFrame from nsSVGIntegrationUtils and nsSVGUtils".

Try results look good: https://tbpl.mozilla.org/?tree=Try&rev=7e86ad513732

Thanks!
Keywords: checkin-needed
Attachment #8370892 - Attachment is obsolete: false
Attachment #8370892 - Flags: checkin+
Comment on attachment 8371809 [details] [diff] [review]
Part 3: Call nsSVGFilterInstance directly instead of nsSVGFilterFrame from nsSVGIntegrationUtils and nsSVGUtils

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

Please make sure you have Hg configured to include your name & email in your patches :)
Attachment #8371809 - Flags: checkin+
Comment on attachment 8371809 [details] [diff] [review]
Part 3: Call nsSVGFilterInstance directly instead of nsSVGFilterFrame from nsSVGIntegrationUtils and nsSVGUtils

Backed out for bustage. Should TransformFilterSpaceToFrameSpace have been left in nsSVGFilterFrame.cpp?
https://hg.mozilla.org/integration/mozilla-inbound/rev/dad17d2dcf28
Attachment #8371809 - Attachment is obsolete: true
Attachment #8371809 - Flags: checkin+ → checkin-
Assignee

Comment 17

6 years ago
Oops! I posted a slightly out of date version of the patch with that unused function accidentally included (TransformFilterSpaceToFrameSpace).

I've posted the right patch, directly from the try bots, titled "Part 3 [v2]...".
(https://hg.mozilla.org/try/rev/0a9555e44d89)

It also includes my name thanks to the try bots. (Still figuring out how to get hg to put my name on local patches).

Sorry for the trouble!
Attachment #8370892 - Attachment is obsolete: true
Assignee

Comment 18

6 years ago
Checkin-needed for "Part 3 [v2]...". Thanks Ryan!
Keywords: checkin-needed
(In reply to Max Vujovic from comment #17)
> It also includes my name thanks to the try bots. (Still figuring out how to
> get hg to put my name on local patches).

I can recommend running ./mach mercurial-setup
Assignee

Comment 20

6 years ago
(In reply to Markus Stange [:mstange] from comment #19)
> (In reply to Max Vujovic from comment #17)
> > It also includes my name thanks to the try bots. (Still figuring out how to
> > get hg to put my name on local patches).
> 
> I can recommend running ./mach mercurial-setup

Thanks Markus! That fixed all my problems :)

Comment 21

6 years ago
landing
Comment on attachment 8372385 [details] [diff] [review]
Part 3 [v2]: Call nsSVGFilterInstance directly instead of nsSVGFilterFrame from nsSVGIntegrationUtils and nsSVGUtils

https://hg.mozilla.org/integration/mozilla-inbound/rev/5d252c7bbea4

One other thing - make sure to include the bug number in the commit message for future patches :)
Attachment #8372385 - Flags: checkin+
Comment on attachment 8370892 [details] [diff] [review]
Parts 1 & 2 for landing

This isn't obsolete just because it already landed.
Attachment #8370892 - Attachment is obsolete: false
Assignee

Comment 23

6 years ago
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #21)
> Comment on attachment 8372385 [details] [diff] [review]
> Part 3 [v2]: Call nsSVGFilterInstance directly instead of nsSVGFilterFrame
> from nsSVGIntegrationUtils and nsSVGUtils
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/5d252c7bbea4
> 
> One other thing - make sure to include the bug number in the commit message
> for future patches :)

Will do!

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #22)
> Comment on attachment 8370892 [details] [diff] [review]
> Parts 1 & 2 for landing
> 
> This isn't obsolete just because it already landed.

K. I'll stop hiding landed patches :)
Assignee

Comment 25

5 years ago
nsSVGFilterProperty is responsible for invalidating an element for painting when the element has an SVG filter and that filter's content or id changes.

Before this patch, nsSVGFilterProperty only tracked a single nsIURI, pointing to a single SVG filter.

Now, there can be multiple SVG reference filters in a filter chain (e.g. filter: url(#a) url(#b);). Thus, nsSVGFilterProperty maintains a list of nsSVGFilterReferences. nsSVGFilterReference is a new class that tracks a single nsIURI, pointing to a single SVG filter.

Try results look good: https://tbpl.mozilla.org/?tree=Try&rev=c1d003424598
Attachment #8373488 - Flags: review?(roc)
Comment on attachment 8373488 [details] [diff] [review]
Part 4: Track an nsStyleFilter chain instead of a single nsIURI in nsSVGFilterProperty

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

I like the way you're breaking this down. Thanks very much.
Attachment #8373488 - Flags: review?(roc) → review+
Assignee

Comment 27

5 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #26)
> I like the way you're breaking this down. Thanks very much.

You're welcome! Thanks for all the reviews!
Assignee

Comment 28

5 years ago
Checkin-needed for "Part 4: Track an nsStyleFilter chain..."

Try results look good: https://tbpl.mozilla.org/?tree=Try&rev=c1d003424598
Keywords: checkin-needed

Comment 29

5 years ago
landing
Comment on attachment 8373488 [details] [diff] [review]
Part 4: Track an nsStyleFilter chain instead of a single nsIURI in nsSVGFilterProperty

https://hg.mozilla.org/integration/mozilla-inbound/rev/72910da4cb77
Attachment #8373488 - Flags: checkin+
Reading through the big patch again, the only issues I've found are:

1. The interface of nsCSS/SVGFilterInstance feels strange. I think I'd prefer it if the constructor didn't mutate any of its arguments, and if the code that does that was moved out of the constructor. Maybe something that can be used like this:

nsresult
nsFilterInstance::BuildPrimitivesForFilter(const nsStyleFilter& aFilter)
{
  if (aFilter.GetType() == NS_STYLE_FILTER_URL) {
    nsSVGFilterInstance svgFilterInstance(
      mTargetFrame,
      mTargetBBox,
      mUserSpaceToIntermediateSpaceTransform,
      aFilter);
    return svgFilterInstance.BuildPrimitives(mPrimitiveDescrs,
                                             mInputImages);
  }

  nsCSSFilterInstance cssFilterInstance(aFilter);
  return cssFilterInstance.BuildPrimitives(mPrimitiveDescrs);
}

So the finalNumPrimitives != initialNumPrimitives check from the nsSVGFilterInstance constructor would be moved to the end inside of BuildPrimitives, for example.

2. BuildPrimitivesForBlur() looks all kinds of wrong, but I realize that you only included it for demonstration purposes. The color space should always be SRGB (I think), figuring out the input index should be shared between filter types, and the radius that gets set on the filter primitive description attributes needs to be in filter space units, not in CSS pixels.
Assignee

Comment 32

5 years ago
(In reply to Markus Stange [:mstange] from comment #31)
> Reading through the big patch again, the only issues I've found are:

Thanks for going though it again!

> 
> 1. The interface of nsCSS/SVGFilterInstance feels strange. I think I'd
> prefer it if the constructor didn't mutate any of its arguments, and if the
> code that does that was moved out of the constructor. Maybe something that
> can be used like this:
> 
> nsresult
> nsFilterInstance::BuildPrimitivesForFilter(const nsStyleFilter& aFilter)
> {
>   if (aFilter.GetType() == NS_STYLE_FILTER_URL) {
>     nsSVGFilterInstance svgFilterInstance(
>       mTargetFrame,
>       mTargetBBox,
>       mUserSpaceToIntermediateSpaceTransform,
>       aFilter);
>     return svgFilterInstance.BuildPrimitives(mPrimitiveDescrs,
>                                              mInputImages);
>   }
> 
>   nsCSSFilterInstance cssFilterInstance(aFilter);
>   return cssFilterInstance.BuildPrimitives(mPrimitiveDescrs);
> }
> 
> So the finalNumPrimitives != initialNumPrimitives check from the
> nsSVGFilterInstance constructor would be moved to the end inside of
> BuildPrimitives, for example.

Yes- that looks better. I'll do that.

> 
> 2. BuildPrimitivesForBlur() looks all kinds of wrong, but I realize that you
> only included it for demonstration purposes. The color space should always
> be SRGB (I think),

Yes, you're right. The spec says "Filter functions must operate in the sRGB color space." [1].

> figuring out the input index should be shared between
> filter types,

Yes, I'll factor out a function for all the CSS shorthand filters. 

> and the radius that gets set on the filter primitive
> description attributes needs to be in filter space units, not in CSS pixels.

Good point! I'll make sure to change that.

[1]: http://dev.w3.org/fxtf/filters/#FilterProperty
Assignee

Comment 33

5 years ago
This patch changes nsSVGFilterInstance to stop taking in an nsSVGFilterFrame in its constructor. Instead, nsSVGFilterInstance will grab the filtered element's nsStyleFilter chain. Then, it'll look up its own nsSVGFilterFrame based on the URL in the first filter in the nsStyleFilter chain (See the new nsSVGFilterInstance::GetFilterFrame() method).

In a later patch, I'll split nsSVGFilterInstance into nsFilterInstance and nsSVGFilterInstance. nsFilterInstance will process the nsStyleFilter chain, and nsSVGFilterInstance will process a single nsStyleFilter. nsFilterInstance will keep the constructor signature currently defined in nsSVGFilterInstance.

Try results look good: https://tbpl.mozilla.org/?tree=Try&rev=8cf1df74e627
Attachment #8377942 - Flags: review?(roc)
Assignee

Comment 34

5 years ago
Added reviewer to commit message in patch.
Attachment #8377942 - Attachment is obsolete: true
Attachment #8377942 - Flags: review?(roc)
Attachment #8377943 - Flags: review?(roc)
Assignee

Comment 35

5 years ago
This patch splits nsSVGFilterInstance into nsFilterInstance and nsSVGFilterInstance.

Generally, what’s removed from nsSVGFilterInstance in the patch is moved to nsFilterInstance.

nsFilterInstance now handles filter rendering, and nsSVGFilterInstance handles filter graph building. Later, nsCSSFilterInstance will also help with filter graph building.

Besides moving functionality, some changes were made to connect nsFilterInstance and nsSVGFilterInstance together:

- nsFilterInstance::BuildPrimitives and nsFilterInstance::BuildPrimitivesForFilter are new functions. nsFilterInstance::BuildPrimitivesForFilter calls nsSVGFilterInstance::BuildPrimitives.
- nsSVGFilterInstance::BuildPrimitives now takes in a list of FilterPrimitiveDescriptions to modify (per Markus’s suggestion).
- nsFilterInstance::BuildPrimitives is only called once in the nsFilterInstance constructor. Before this patch, nsSVGFilterInstance called its BuildPrimitives method at the beginning of each major public function (Render, ComputePostFilterDirtyRect, ComputePostFilterExtents, ComputeSourceNeededRect).
Attachment #8379135 - Flags: review?(roc)
Comment on attachment 8377943 [details] [diff] [review]
Part 5 [v2]: Look up an nsStyleFilter chain instead of passing an nsSVGFilterFrame into nsSVGFilterInstance.

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

Very nice!

::: layout/svg/nsSVGFilterInstance.cpp
@@ +304,5 @@
> +    // The URL points to an element that's not an SVG filter element.
> +    return nullptr;
> +  }
> +
> +  return static_cast<nsSVGFilterFrame*>(frame);    

trailing space
Attachment #8377943 - Flags: review?(roc) → review+
Assignee

Comment 37

5 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #36)
> Comment on attachment 8377943 [details] [diff] [review]
> Part 5 [v2]: Look up an nsStyleFilter chain instead of passing an
> nsSVGFilterFrame into nsSVGFilterInstance.
> 
> Review of attachment 8377943 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Very nice!
> 
> ::: layout/svg/nsSVGFilterInstance.cpp
> @@ +304,5 @@
> > +    // The URL points to an element that's not an SVG filter element.
> > +    return nullptr;
> > +  }
> > +
> > +  return static_cast<nsSVGFilterFrame*>(frame);    
> 
> trailing space

Thanks! I'll clean that up.
Comment on attachment 8379135 [details] [diff] [review]
Part 6: Split out rendering code from nsSVGFilterInstance into nsFilterInstance.

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

Can you use "hg copy" to make nsFilterInstance.cpp a copy of nsSVGFilterInstance before deleting the stuff you don't need? That should make this diff easier to follow and give better history.
Do you have any plans to work on hardware accelerated filter rendering on the Compositor after this?

Just thinking about how that might work:

I think we'd want to split filter rendering out from the other svg effects. So create a new display item nsDisplayFilter separate from nsDisplaySVGEffects.

If we don't support all the possible filter combinations on the compositor, then we'd need to analyze the filter chain before we create the display item, and decide which path to take.

Given that, it would be nice if we could create the nsFilterInstance outside of nsSVGIntegrationUtils so we can have the option of instead attaching it to a layer instead of going through the effects rendering code.
Assignee

Comment 40

5 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #38)
> Comment on attachment 8379135 [details] [diff] [review]
> Part 6: Split out rendering code from nsSVGFilterInstance into
> nsFilterInstance.
> 
> Review of attachment 8379135 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can you use "hg copy" to make nsFilterInstance.cpp a copy of
> nsSVGFilterInstance before deleting the stuff you don't need? That should
> make this diff easier to follow and give better history.

Yes! I've posted a patch using "hg copy". I've been looking for a command like that- thanks!
Attachment #8379135 - Attachment is obsolete: true
Attachment #8379135 - Flags: review?(roc)
Attachment #8379336 - Flags: review?(roc)
(In reply to Matt Woodrow (:mattwoodrow) from comment #39)
> Do you have any plans to work on hardware accelerated filter rendering on
> the Compositor after this?

I have. I've written the code that serializes and deserializes FilterDescription objects so that they end up on a ContainerLayerComposite, but not much more than that yet.

> I think we'd want to split filter rendering out from the other svg effects.
> So create a new display item nsDisplayFilter separate from
> nsDisplaySVGEffects.

Sounds good. And actually, creating nsDisplaySVGMask and implementing them with mask layers shouldn't be much work either.

> If we don't support all the possible filter combinations on the compositor,
> then we'd need to analyze the filter chain before we create the display
> item, and decide which path to take.

I'm trying to support all filter types on the compositor from the start, but if that doesn't work out, we can still do this, yeah.

> Given that, it would be nice if we could create the nsFilterInstance outside
> of nsSVGIntegrationUtils so we can have the option of instead attaching it
> to a layer instead of going through the effects rendering code.

We could also keep the nsFilterInstance stuff inside nsSVGIntegrationUtils but add a method to nsSVGIntegrationUtils that, instead of rendering the filter itself, returns the assembled FilterDescription. This method would then be called from the nsDisplayFilter, which would set the FilterDescription on the layer.
Comment on attachment 8379336 [details] [diff] [review]
Part 6 [v2]: Split out rendering code from nsSVGFilterInstance into nsFilterInstance.

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

Great!
Attachment #8379336 - Flags: review?(roc) → review+
Assignee

Comment 43

5 years ago
Addressed review comment and rebased patch.
Attachment #8377943 - Attachment is obsolete: true
Assignee

Comment 45

5 years ago
Checkin-needed for patches "Part 5[v3]..." and "Part 6[v3]...".

Try results look good for "Part 5[v3]...": https://tbpl.mozilla.org/?tree=Try&rev=8cf1df74e627
And "Part 6[v3]...": https://tbpl.mozilla.org/?tree=Try&rev=70fa197475d6
Keywords: checkin-needed
Assignee

Comment 48

5 years ago
This patch connects the filter graphs of chained SVG filters.
e.g. filter: url(#filter1) url(#filter2) url(#filter3);

This patch makes the SourceGraphic keyword in each SVG filter refer to the result of the previous filter in the chain, according to the spec [1]. We will handle SourceAlpha in a future patch.

In a future patch, we will also compute the overall filter region for the filter chain. Right now, we’re using the filter region of the last filter in the chain.

[1]: http://dev.w3.org/fxtf/filters/#valuedef-sourcegraphic
Attachment #8382377 - Flags: review?(roc)
Assignee

Comment 49

5 years ago
Now would be a good time to remove the deprecated filterRes attribute [1].

Maintaining different filterRes(s) across chained SVG filters would make the code fairly complicated. I imagine we'd have to use some scaling operations in the combined FilterPrimitiveDescription chain, or we'd have to use separate FilterPrimitiveDescription chains with separate backings, which would be unfortunate.

[1]: http://dev.w3.org/fxtf/filters/#element-attrdef-filterres
Attachment #8382598 - Flags: review?(roc)
Comment on attachment 8382377 [details] [diff] [review]
Part 7: Connect the filter graphs of chained SVG filters.

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

::: layout/svg/nsSVGFilterInstance.h
@@ +140,5 @@
>    gfxMatrix GetUserSpaceToFrameSpaceInCSSPxTransform() const;
>  
>    /**
> +   * Returns the source indices in the FilterPrimitiveDescription list for
> +   * the next filter primitive element.

This comment is not clear. Make it clear that the result contains the index of every source in every primitive in aPrimitiveDescrs.
Attachment #8382377 - Flags: review?(roc) → review+
Comment on attachment 8382598 [details] [diff] [review]
Part 8: Remove deprecated filterRes attribute.

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

Excellent idea!
Attachment #8382598 - Flags: review?(roc) → review+
Assignee

Comment 52

5 years ago
Comment on attachment 8382377 [details] [diff] [review]
Part 7: Connect the filter graphs of chained SVG filters.

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

Thanks for the reviews!

::: layout/svg/nsSVGFilterInstance.h
@@ +140,5 @@
>    gfxMatrix GetUserSpaceToFrameSpaceInCSSPxTransform() const;
>  
>    /**
> +   * Returns the source indices in the FilterPrimitiveDescription list for
> +   * the next filter primitive element.

The result contains the index of every source of the passed-in primitive element (not every primitive). I'll change the comment to:

/**
  * Finds the index in aPrimitiveDescrs of each input to aPrimitiveElement.
  * For example, if aPrimitiveElement is:
  *   <feGaussianBlur in="another-primitive" .../>
  * Then, the resulting aSourceIndices will contain the index of the
  * FilterPrimitiveDescription representing "another-primitive".
  */

And I'll rename the variable "aFilterElement" to "aPrimitiveElement" for clarity. aFilterElement implies a <filter> element, whereas aPrimitiveElement should imply an <fe*> element.
Assignee

Comment 53

5 years ago
Addressed review comments and rebased patch.
Attachment #8382377 - Attachment is obsolete: true
Assignee

Comment 54

5 years ago
Rebased patch.
Attachment #8382598 - Attachment is obsolete: true
Assignee

Comment 55

5 years ago
Checkin-needed for "Part 7[v2]..." and "Part 8[v2]...".

Try results look good for both:
"Part 7[v2]...": https://tbpl.mozilla.org/?tree=Try&rev=b9df2b58028e
"Part 8[v2]...": https://tbpl.mozilla.org/?tree=Try&rev=57463d22afbc
Keywords: checkin-needed

Comment 56

5 years ago
landing
Comment on attachment 8383801 [details] [diff] [review]
Part 7 [v2]: Connect the filter graphs of chained SVG filters.

https://hg.mozilla.org/integration/mozilla-inbound/rev/10b8310f8dcb
Attachment #8383801 - Flags: checkin+
Whiteboard: [leave open]
Comment on attachment 8383804 [details] [diff] [review]
Part 8 [v2]: Remove deprecated filterRes attribute.

>diff --git a/content/svg/content/test/dataTypes-helper.svg b/content/svg/content/test/dataTypes-helper.svg
>--- a/content/svg/content/test/dataTypes-helper.svg
>+++ b/content/svg/content/test/dataTypes-helper.svg
>@@ -1,12 +1,11 @@
> <?xml version="1.0"?>
> <svg xmlns="http://www.w3.org/2000/svg" width="750">
>   <defs>
>-    <!-- <integer-optional-integer> (filterRes) -->

This should be replaced, rather than removed altogether. See below...

>     <filter id="filter">
>       <!-- <boolean> (preserveAlpha) -->
>       <!-- <enum> (edgeMode) -->
>       <!-- <number> (divisor) -->
>       <!-- <integer> (targetX) -->
>       <!-- <string> (result) -->
>       <feConvolveMatrix id="convolve"/>
>       <!-- <number-optional-number> (stdDeviation) -->
>diff --git a/content/svg/content/test/test_dataTypes.html b/content/svg/content/test/test_dataTypes.html
>--- a/content/svg/content/test/test_dataTypes.html
>+++ b/content/svg/content/test/test_dataTypes.html
>@@ -117,66 +117,16 @@ function runTests()
>   convolve.targetX.baseVal = 7;
>   is(convolve.targetX.animVal, 7, "integer animVal");
>   is(convolve.getAttribute("targetX"), "7", "integer attribute");
>   convolve.setAttribute("targetX", "");
>   ok(convolve.getAttribute("targetX") === "", "empty integer attribute");
>   convolve.removeAttribute("targetX");
>   ok(convolve.getAttribute("targetX") === null, "removed integer attribute");
> 
>-  // integer-optional-integer attribute
>-
>-  filter.setAttribute("filterRes", "100");
>-  is(filter.filterResX.baseVal, 100, "integer-optional-integer first baseVal");
>-  is(filter.filterResX.animVal, 100, "integer-optional-integer first animVal");
>-  is(filter.filterResY.baseVal, 100, "integer-optional-integer second baseVal");
>-  is(filter.filterResY.animVal, 100, "integer-optional-integer second animVal");
>-
>-  filter.filterResX.baseVal = 50;
>-  is(filter.filterResX.animVal, 50, "integer-optional-integer first animVal");
>-  is(filter.filterResY.animVal, 100, "integer-optional-integer second animVal");
>-  is(filter.getAttribute("filterRes"), "50, 100", "integer-optional-integer attribute");
>-
>-  filter.filterResY.baseVal = 50;
>-  is(filter.getAttribute("filterRes"), "50", "integer-optional-integer attribute");
>-
>-  filter.setFilterRes(80, 90);
>-  is(filter.filterResX.baseVal, 80, "integer-optional-integer first baseVal");
>-  is(filter.filterResX.animVal, 80, "integer-optional-integer first animVal");
>-  is(filter.filterResY.baseVal, 90, "integer-optional-integer second baseVal");
>-  is(filter.filterResY.animVal, 90, "integer-optional-integer second animVal");
>-
>-  // 32 bit integer range
>-  filter.setFilterRes(-2147483648, 2147483647);
>-  is(filter.filterResX.baseVal, -2147483648, "integer-optional-integer first baseVal");
>-  is(filter.filterResX.animVal, -2147483648, "integer-optional-integer first animVal");
>-  is(filter.filterResY.baseVal, 2147483647, "integer-optional-integer second baseVal");
>-  is(filter.filterResY.animVal, 2147483647, "integer-optional-integer second animVal");
>-
>-  // too big, clamp
>-  filter.setAttribute("filterRes", "-2147483649, 2147483648");
>-  is(filter.filterResX.baseVal, -2147483648, "integer-optional-integer first baseVal");
>-  is(filter.filterResX.animVal, -2147483648, "integer-optional-integer first animVal");
>-  is(filter.filterResY.baseVal, 2147483647, "integer-optional-integer second baseVal");
>-  is(filter.filterResY.animVal, 2147483647, "integer-optional-integer second animVal");
>-
>-  // invalid
>-  filter.setAttribute("filterRes", "-00000000000invalid, 214748364720invalid");
>-  is(filter.filterResX.baseVal, 0, "integer-optional-integer first baseVal");
>-  is(filter.filterResX.animVal, 0, "integer-optional-integer first animVal");
>-  is(filter.filterResY.baseVal, 0, "integer-optional-integer second baseVal");
>-  is(filter.filterResY.animVal, 0, "integer-optional-integer second animVal");
>-
>-  filter.setAttribute("filterRes", "");
>-  ok(filter.getAttribute("filterRes") === "",
>-     "empty integer-optional-integer attribute");
>-  filter.removeAttribute("filterRes");
>-  ok(filter.getAttribute("filterRes") === null,
>-     "removed integer-optional-integer attribute");
>-

Now we have no test for integer-optional-integer. What really should have happened is that you found a different integer-optional-integer
attribute and used that for the tests rather than removing all the tests. The order attribute of a convolve matrix  filter is what's left I think.

>diff --git a/content/svg/content/test/test_dataTypesModEvents.html b/content/svg/content/test/test_dataTypesModEvents.html
>--- a/content/svg/content/test/test_dataTypesModEvents.html
>+++ b/content/svg/content/test/test_dataTypesModEvents.html
>@@ -122,31 +122,16 @@ function runTests()
>   eventChecker.expect("");
>   convolve.setAttribute("targetX", "8");
>   // Check redundant change when comparing attribute value to typed value
>   eventChecker.expect("remove add");
>   convolve.removeAttribute("targetX");
>   convolve.setAttribute("targetX", "8");
>   convolve.targetX.baseVal = 8;
> 
>-  // integer-optional-integer attribute

Again, all this needed to be replaced with something else and not removed.

>-
>-  eventChecker.watchAttr(filter, "filterRes");
>-  eventChecker.expect("add modify remove add");
>-  filter.setAttribute("filterRes", "60, 70");
>-  filter.filterResX.baseVal = 50;
>-  filter.removeAttribute("filterRes");
>-  filter.removeAttributeNS(null, "filterRes");
>-  filter.setAttribute("filterRes", "50, 60");
>-
>-  eventChecker.expect("");
>-  filter.filterResX.baseVal = 50;
>-  filter.setAttribute("filterRes", "50, 60");
>-  filter.filterResY.baseVal = 60;
>-
>   // angle attribute
> 
>   eventChecker.watchAttr(marker, "orient");
>   eventChecker.expect("add modify modify modify modify modify remove add");
>   marker.setAttribute("orient", "90deg");
>   marker.orientAngle.baseVal.value = 12;
>   marker.orientAngle.baseVal.valueInSpecifiedUnits = 23;
>   marker.orientAngle.baseVal.valueAsString = "34";
>diff --git a/parser/html/javasrc/AttributeName.java b/parser/html/javasrc/AttributeName.java
>--- a/parser/html/javasrc/AttributeName.java
>+++ b/parser/html/javasrc/AttributeName.java
>@@ -1003,17 +1003,16 @@ public final class AttributeName
>     public static final AttributeName AMPLITUDE = new AttributeName(ALL_NO_NS, SAME_LOCAL("amplitude"), ALL_NO_PREFIX, NCNAME_HTML | NCNAME_FOREIGN | NCNAME_LANG);
>     public static final AttributeName ARIA_LIVE = new AttributeName(ALL_NO_NS, SAME_LOCAL("aria-live"), ALL_NO_PREFIX, NCNAME_HTML | NCNAME_FOREIGN | NCNAME_LANG);
>     public static final AttributeName CLIP_RULE = new AttributeName(ALL_NO_NS, SAME_LOCAL("clip-rule"), ALL_NO_PREFIX, NCNAME_HTML | NCNAME_FOREIGN | NCNAME_LANG);
>     public static final AttributeName CLIP_PATH = new AttributeName(ALL_NO_NS, SAME_LOCAL("clip-path"), ALL_NO_PREFIX, NCNAME_HTML | NCNAME_FOREIGN | NCNAME_LANG);
>     public static final AttributeName EQUALROWS = new AttributeName(ALL_NO_NS, SAME_LOCAL("equalrows"), ALL_NO_PREFIX, NCNAME_HTML | NCNAME_FOREIGN | NCNAME_LANG);
>     public static final AttributeName ELEVATION = new AttributeName(ALL_NO_NS, SAME_LOCAL("elevation"), ALL_NO_PREFIX, NCNAME_HTML | NCNAME_FOREIGN | NCNAME_LANG);
>     public static final AttributeName DIRECTION = new AttributeName(ALL_NO_NS, SAME_LOCAL("direction"), ALL_NO_PREFIX, NCNAME_HTML | NCNAME_FOREIGN | NCNAME_LANG);
>     public static final AttributeName DRAGGABLE = new AttributeName(ALL_NO_NS, SAME_LOCAL("draggable"), ALL_NO_PREFIX, NCNAME_HTML | NCNAME_FOREIGN | NCNAME_LANG);
>-    public static final AttributeName FILTERRES = new AttributeName(ALL_NO_NS, SVG_DIFFERENT("filterres", "filterRes"), ALL_NO_PREFIX, NCNAME_HTML | NCNAME_FOREIGN | NCNAME_LANG);

This is an HTML parser change and needs to be reflected in the HTML specification. Henri Sivonen can arrange for this to happen but he needs to know about it.
removal of filterRes is an html parser specification change from the filter effects specification. It needs to be treated like feDropShadowElement except that feDropShadowElement was an element being added and filterRes is an attribute being removed.
Assignee

Comment 60

5 years ago
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #57)
> Comment on attachment 8383804 [details] [diff] [review]
> Part 8 [v2]: Remove deprecated filterRes attribute.
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/c14980a16210

@Ryan: Can we back out this change before it lands on mozilla-central? Robert added some new comments to the patch that I should address first.
(In reply to Max Vujovic from comment #60)
> (In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #57)
> > Comment on attachment 8383804 [details] [diff] [review]
> > Part 8 [v2]: Remove deprecated filterRes attribute.
> > 
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/c14980a16210
> 
> @Ryan: Can we back out this change before it lands on mozilla-central?
> Robert added some new comments to the patch that I should address first.

http://hg.mozilla.org/integration/mozilla-inbound/rev/57cd2113f044
Please get an r+ from henri sivonen (on the parser changes only) before relanding the filterRes patch.
Assignee

Comment 63

5 years ago
(In reply to Robert Longson from comment #62)
> Please get an r+ from henri sivonen (on the parser changes only) before
> relanding the filterRes patch.

Will do! Thanks for looking over the patch. I'll use another "integer-optional-integer" for the parsing tests instead of removing them. Looking at the "order" attribute now :)
Assignee

Comment 65

5 years ago
Attachment #8383804 - Attachment is obsolete: true
Assignee

Comment 67

5 years ago
(In reply to Robert Longson from comment #58)
> Comment on attachment 8383804 [details] [diff] [review]
> Part 8 [v2]: Remove deprecated filterRes attribute.
> 
> >diff --git a/content/svg/content/test/dataTypes-helper.svg b/content/svg/content/test/dataTypes-helper.svg
> >--- a/content/svg/content/test/dataTypes-helper.svg
> >+++ b/content/svg/content/test/dataTypes-helper.svg
> >@@ -1,12 +1,11 @@
> > <?xml version="1.0"?>
> > <svg xmlns="http://www.w3.org/2000/svg" width="750">
> >   <defs>
> >-    <!-- <integer-optional-integer> (filterRes) -->
> 
> This should be replaced, rather than removed altogether. See below...

Done. I moved it down to be with feConvolveMatrix, using the "order" attribute.

> 
> >     <filter id="filter">
> >       <!-- <boolean> (preserveAlpha) -->
> >       <!-- <enum> (edgeMode) -->
> >       <!-- <number> (divisor) -->
> >       <!-- <integer> (targetX) -->
> >       <!-- <string> (result) -->
> >       <feConvolveMatrix id="convolve"/>
> >       <!-- <number-optional-number> (stdDeviation) -->
> >diff --git a/content/svg/content/test/test_dataTypes.html b/content/svg/content/test/test_dataTypes.html
> >--- a/content/svg/content/test/test_dataTypes.html
> >+++ b/content/svg/content/test/test_dataTypes.html
> >@@ -117,66 +117,16 @@ function runTests()
> >   convolve.targetX.baseVal = 7;
> >   is(convolve.targetX.animVal, 7, "integer animVal");
> >   is(convolve.getAttribute("targetX"), "7", "integer attribute");
> >   convolve.setAttribute("targetX", "");
> >   ok(convolve.getAttribute("targetX") === "", "empty integer attribute");
> >   convolve.removeAttribute("targetX");
> >   ok(convolve.getAttribute("targetX") === null, "removed integer attribute");
> > 
> >-  // integer-optional-integer attribute
> >-
> >-  filter.setAttribute("filterRes", "100");
> >-  is(filter.filterResX.baseVal, 100, "integer-optional-integer first baseVal");
> >-  is(filter.filterResX.animVal, 100, "integer-optional-integer first animVal");
> >-  is(filter.filterResY.baseVal, 100, "integer-optional-integer second baseVal");
> >-  is(filter.filterResY.animVal, 100, "integer-optional-integer second animVal");
> >-
> >-  filter.filterResX.baseVal = 50;
> >-  is(filter.filterResX.animVal, 50, "integer-optional-integer first animVal");
> >-  is(filter.filterResY.animVal, 100, "integer-optional-integer second animVal");
> >-  is(filter.getAttribute("filterRes"), "50, 100", "integer-optional-integer attribute");
> >-
> >-  filter.filterResY.baseVal = 50;
> >-  is(filter.getAttribute("filterRes"), "50", "integer-optional-integer attribute");
> >-
> >-  filter.setFilterRes(80, 90);
> >-  is(filter.filterResX.baseVal, 80, "integer-optional-integer first baseVal");
> >-  is(filter.filterResX.animVal, 80, "integer-optional-integer first animVal");
> >-  is(filter.filterResY.baseVal, 90, "integer-optional-integer second baseVal");
> >-  is(filter.filterResY.animVal, 90, "integer-optional-integer second animVal");
> >-
> >-  // 32 bit integer range
> >-  filter.setFilterRes(-2147483648, 2147483647);
> >-  is(filter.filterResX.baseVal, -2147483648, "integer-optional-integer first baseVal");
> >-  is(filter.filterResX.animVal, -2147483648, "integer-optional-integer first animVal");
> >-  is(filter.filterResY.baseVal, 2147483647, "integer-optional-integer second baseVal");
> >-  is(filter.filterResY.animVal, 2147483647, "integer-optional-integer second animVal");
> >-
> >-  // too big, clamp
> >-  filter.setAttribute("filterRes", "-2147483649, 2147483648");
> >-  is(filter.filterResX.baseVal, -2147483648, "integer-optional-integer first baseVal");
> >-  is(filter.filterResX.animVal, -2147483648, "integer-optional-integer first animVal");
> >-  is(filter.filterResY.baseVal, 2147483647, "integer-optional-integer second baseVal");
> >-  is(filter.filterResY.animVal, 2147483647, "integer-optional-integer second animVal");
> >-
> >-  // invalid
> >-  filter.setAttribute("filterRes", "-00000000000invalid, 214748364720invalid");
> >-  is(filter.filterResX.baseVal, 0, "integer-optional-integer first baseVal");
> >-  is(filter.filterResX.animVal, 0, "integer-optional-integer first animVal");
> >-  is(filter.filterResY.baseVal, 0, "integer-optional-integer second baseVal");
> >-  is(filter.filterResY.animVal, 0, "integer-optional-integer second animVal");
> >-
> >-  filter.setAttribute("filterRes", "");
> >-  ok(filter.getAttribute("filterRes") === "",
> >-     "empty integer-optional-integer attribute");
> >-  filter.removeAttribute("filterRes");
> >-  ok(filter.getAttribute("filterRes") === null,
> >-     "removed integer-optional-integer attribute");
> >-
> 
> Now we have no test for integer-optional-integer. What really should have
> happened is that you found a different integer-optional-integer
> attribute and used that for the tests rather than removing all the tests.
> The order attribute of a convolve matrix  filter is what's left I think.

Done. I used the order attribute for feConvolveMatrix. I changed the values in the tests to be more sensible for kernel sizes, although it shouldn't really matter.

> 
> >diff --git a/content/svg/content/test/test_dataTypesModEvents.html b/content/svg/content/test/test_dataTypesModEvents.html
> >--- a/content/svg/content/test/test_dataTypesModEvents.html
> >+++ b/content/svg/content/test/test_dataTypesModEvents.html
> >@@ -122,31 +122,16 @@ function runTests()
> >   eventChecker.expect("");
> >   convolve.setAttribute("targetX", "8");
> >   // Check redundant change when comparing attribute value to typed value
> >   eventChecker.expect("remove add");
> >   convolve.removeAttribute("targetX");
> >   convolve.setAttribute("targetX", "8");
> >   convolve.targetX.baseVal = 8;
> > 
> >-  // integer-optional-integer attribute
> 
> Again, all this needed to be replaced with something else and not removed.

Done.

> 
> >-
> >-  eventChecker.watchAttr(filter, "filterRes");
> >-  eventChecker.expect("add modify remove add");
> >-  filter.setAttribute("filterRes", "60, 70");
> >-  filter.filterResX.baseVal = 50;
> >-  filter.removeAttribute("filterRes");
> >-  filter.removeAttributeNS(null, "filterRes");
> >-  filter.setAttribute("filterRes", "50, 60");
> >-
> >-  eventChecker.expect("");
> >-  filter.filterResX.baseVal = 50;
> >-  filter.setAttribute("filterRes", "50, 60");
> >-  filter.filterResY.baseVal = 60;
> >-
> >   // angle attribute
> > 
> >   eventChecker.watchAttr(marker, "orient");
> >   eventChecker.expect("add modify modify modify modify modify remove add");
> >   marker.setAttribute("orient", "90deg");
> >   marker.orientAngle.baseVal.value = 12;
> >   marker.orientAngle.baseVal.valueInSpecifiedUnits = 23;
> >   marker.orientAngle.baseVal.valueAsString = "34";
> >diff --git a/parser/html/javasrc/AttributeName.java b/parser/html/javasrc/AttributeName.java
> >--- a/parser/html/javasrc/AttributeName.java
> >+++ b/parser/html/javasrc/AttributeName.java
> >@@ -1003,17 +1003,16 @@ public final class AttributeName
> >     public static final AttributeName AMPLITUDE = new AttributeName(ALL_NO_NS, SAME_LOCAL("amplitude"), ALL_NO_PREFIX, NCNAME_HTML | NCNAME_FOREIGN | NCNAME_LANG);
> >     public static final AttributeName ARIA_LIVE = new AttributeName(ALL_NO_NS, SAME_LOCAL("aria-live"), ALL_NO_PREFIX, NCNAME_HTML | NCNAME_FOREIGN | NCNAME_LANG);
> >     public static final AttributeName CLIP_RULE = new AttributeName(ALL_NO_NS, SAME_LOCAL("clip-rule"), ALL_NO_PREFIX, NCNAME_HTML | NCNAME_FOREIGN | NCNAME_LANG);
> >     public static final AttributeName CLIP_PATH = new AttributeName(ALL_NO_NS, SAME_LOCAL("clip-path"), ALL_NO_PREFIX, NCNAME_HTML | NCNAME_FOREIGN | NCNAME_LANG);
> >     public static final AttributeName EQUALROWS = new AttributeName(ALL_NO_NS, SAME_LOCAL("equalrows"), ALL_NO_PREFIX, NCNAME_HTML | NCNAME_FOREIGN | NCNAME_LANG);
> >     public static final AttributeName ELEVATION = new AttributeName(ALL_NO_NS, SAME_LOCAL("elevation"), ALL_NO_PREFIX, NCNAME_HTML | NCNAME_FOREIGN | NCNAME_LANG);
> >     public static final AttributeName DIRECTION = new AttributeName(ALL_NO_NS, SAME_LOCAL("direction"), ALL_NO_PREFIX, NCNAME_HTML | NCNAME_FOREIGN | NCNAME_LANG);
> >     public static final AttributeName DRAGGABLE = new AttributeName(ALL_NO_NS, SAME_LOCAL("draggable"), ALL_NO_PREFIX, NCNAME_HTML | NCNAME_FOREIGN | NCNAME_LANG);
> >-    public static final AttributeName FILTERRES = new AttributeName(ALL_NO_NS, SVG_DIFFERENT("filterres", "filterRes"), ALL_NO_PREFIX, NCNAME_HTML | NCNAME_FOREIGN | NCNAME_LANG);
> 
> This is an HTML parser change and needs to be reflected in the HTML
> specification. Henri Sivonen can arrange for this to happen but he needs to
> know about it.

Will do. I filed:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=24900
Assignee

Comment 68

5 years ago
Comment on attachment 8384721 [details] [diff] [review]
Part 8 [v3]: Remove deprecated filterRes attribute. (Parser changes only. Do not land.)

Henri, can you review these parser changes for removing the filterRes attribute please?

I've filed a bug on the spec:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=24900

In case you want to see the full patch (with non-parser changes), it's here:
https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=948265&attachment=8384718
Attachment #8384721 - Flags: review?(hsivonen)
Much better although we've still lost the SMIL integer-optional-integer test in layout/reftests/svg/smil/reftest.list ideally you'd write a replacement test for that, a reference that has a fixed order convolve matrix and an animation that animates to that fixed value. 

Look at anim-feSpotLight-01.svg perhaps but convert that structure to feConvolveMatrix and order. We want a from attribute on the animation with one value and a to attribute with 2 values (or vice versa).
Assignee

Comment 70

5 years ago
Addressed longsonr's feedback. Added a SMIL animation test on the "order" attribute of feConvolveMatrix (See anim-feConvolveMatrix-order-01.svg).
Attachment #8384718 - Attachment is obsolete: true
Assignee

Comment 71

5 years ago
(In reply to Robert Longson from comment #69)
> Much better although we've still lost the SMIL integer-optional-integer test
> in layout/reftests/svg/smil/reftest.list ideally you'd write a replacement
> test for that, a reference that has a fixed order convolve matrix and an
> animation that animates to that fixed value. 
> 
> Look at anim-feSpotLight-01.svg perhaps but convert that structure to
> feConvolveMatrix and order. We want a from attribute on the animation with
> one value and a to attribute with 2 values (or vice versa).

Good catch! I've added a test as you suggested. In the test, I also animated the "kernelMatrix" attribute, since it depends on "order".
Comment on attachment 8384721 [details] [diff] [review]
Part 8 [v3]: Remove deprecated filterRes attribute. (Parser changes only. Do not land.)

The change to nsHtml5AttributeName.cpp (and the corresponding .java) doesn't remove the precomputed hash from the long array of magic integers. I'd expect Firefox not to work with this patch landed. Does it work?

There are SVG test cases in this patch that I'm probably not the right reviewer for.

I suggest you leave the parser and the html5lib parser test cases unchanged for now and file a separate bug to follow what happens with the parsing spec. (It doesn't hurt us if the parser camelCases something that the SVG implementation ignores, but I don't want the parser tests to diverge from upstream and with Chrome devs removing stuff unilaterally, I don't want to set the precedent of upstreaming unilateral test case changes to html5lib for them.)
Attachment #8384721 - Flags: review?(hsivonen) → review-
Assignee

Updated

5 years ago
Blocks: 979472
Assignee

Comment 73

5 years ago
New patch with Henri's suggestions.

(In reply to Henri Sivonen (:hsivonen) from comment #72)
> Comment on attachment 8384721 [details] [diff] [review]
> Part 8 [v3]: Remove deprecated filterRes attribute. (Parser changes only. Do
> not land.)
> 
> The change to nsHtml5AttributeName.cpp (and the corresponding .java) doesn't
> remove the precomputed hash from the long array of magic integers. I'd
> expect Firefox not to work with this patch landed. Does it work?

Good point. Interestingly, Firefox still works. Try results looked fine:
https://tbpl.mozilla.org/?tree=Try&rev=dbaa96236f10

> 
> There are SVG test cases in this patch that I'm probably not the right
> reviewer for.

No worries about those. roc and longsonr took a look at them.

> 
> I suggest you leave the parser and the html5lib parser test cases unchanged
> for now and file a separate bug to follow what happens with the parsing
> spec. (It doesn't hurt us if the parser camelCases something that the SVG
> implementation ignores, but I don't want the parser tests to diverge from
> upstream and with Chrome devs removing stuff unilaterally, I don't want to
> set the precedent of upstreaming unilateral test case changes to html5lib
> for them.)

That makes sense to me. I've removed the parser changes from the patch. I filed bug 979472 to track them instead.

In the new patch, I've kept the definition for nsGkAtoms::filterRes in nsGkAtomList.h so that nsTreeSanitizer.cpp can continue to use it.

Does the new patch look good wrt the HTML parser?
Attachment #8384721 - Attachment is obsolete: true
Attachment #8384970 - Attachment is obsolete: true
Attachment #8385611 - Flags: review?(hsivonen)
Comment on attachment 8385611 [details] [diff] [review]
Part 8 [v5]: Remove deprecated filterRes attribute. (Full Patch)

r+ on the parser part, i.e. the patch no longer including any parser parts.
Attachment #8385611 - Flags: review?(hsivonen) → review+
Assignee

Comment 75

5 years ago
Checkin-needed for "Part 8 [v5]: Remove deprecated filterRes attribute. (Full Patch)".

Try results look good: https://tbpl.mozilla.org/?tree=Try&rev=2375361fab06
Keywords: checkin-needed

Comment 76

5 years ago
landing
Comment on attachment 8385611 [details] [diff] [review]
Part 8 [v5]: Remove deprecated filterRes attribute. (Full Patch)

https://hg.mozilla.org/integration/mozilla-inbound/rev/f7a680ef230c
Attachment #8385611 - Flags: checkin+
Attachment #8379819 - Flags: checkin+
Attachment #8379821 - Flags: checkin+
Assignee

Comment 77

5 years ago
This patch works toward computing an overall filter region for chained SVG and CSS filters.

To compute the overall filter region for a chain, we first need to build each filter’s FilterPrimitiveDescriptions in some common space. In this patch, I introduce “intermediate space” for this purpose. This space is the same across SVG and CSS filter in a chain. It’s just user space scaled to device pixels (so that it can be stored in FilterPrimitiveDescription using integers). Intermediate space has the same scale as filter space, but filter space can be offset differently for each filter in a chain.

I also renamed “mFilterRegion" to “mUserSpaceBounds”. This is similar to the existing “mFilterSpaceBounds” variable and the new “mIntermediateSpaceBounds” variable. Using this naming makes working between the different coordinate spaces easier to follow.
Attachment #8388733 - Flags: review?(roc)
Comment on attachment 8388733 [details] [diff] [review]
Part 9: Introduce an "intermediate" coordinate space to share across chained filters.

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

::: layout/svg/nsSVGFilterInstance.h
@@ +35,5 @@
> + * This class uses several different coordinate spaces, defined as follows:
> + *
> + * "user space"
> + *   The filtered SVG element's user space or the filtered HTML element's
> + *   CSS pixel space.

Define the origin of the space for HTML elements. I think 0,0 is the top-left of the border-box?
Attachment #8388733 - Flags: review?(roc) → review+
Assignee

Comment 80

5 years ago
Comment on attachment 8388733 [details] [diff] [review]
Part 9: Introduce an "intermediate" coordinate space to share across chained filters.

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

Thanks for the review!

::: layout/svg/nsSVGFilterInstance.h
@@ +35,5 @@
> + * This class uses several different coordinate spaces, defined as follows:
> + *
> + * "user space"
> + *   The filtered SVG element's user space or the filtered HTML element's
> + *   CSS pixel space.

Good idea. Yes, (0,0) is the top-left of the border-box. (I just double-checked.)
Assignee

Comment 81

5 years ago
Addressed review comments and rebased patch.
Attachment #8388733 - Attachment is obsolete: true
Assignee

Comment 82

5 years ago
Checkin-needed for "Part 9[v2]...".

Try results looked good for v1: https://tbpl.mozilla.org/?tree=Try&rev=b5a43d0d4d0a
(I didn't rerun the try bots for the comment change in v2.)
Keywords: checkin-needed
Depends on: 818177

Comment 83

5 years ago
landing
Comment on attachment 8389454 [details] [diff] [review]
Part 9 [v2]: Introduce an "intermediate" coordinate space to share across chained filters.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7997cb5af49c
Attachment #8389454 - Flags: checkin+
Comment on attachment 8389454 [details] [diff] [review]
Part 9 [v2]: Introduce an "intermediate" coordinate space to share across chained filters.

>+ * To understand the spaces better, let's take an example filter that defines a
>+ * filter region:
>+ *   <filter id="f" x="-15" y="-15" width="130" height="130">...</filter>
>+ *
>+ * And apply the filter to a div element:
>+ *   <div style="filter: url(#f); ...">...</div>
>+ *
>+ * And let's say there are 2 device pixels for every 1 CSS pixel.
>+ *
>+ * Then:
>+ *   "user space" = the CSS pixel space of the <div>
>+ *   "intermediate space" = "user space" * 2
>+ *   "filter space" = "intermediate space" + 15

Wouldn't this be "+ 30"? I love the use of an example here, but this one confuses me :)
Assignee

Comment 86

5 years ago
(In reply to Markus Stange [:mstange] from comment #84)
> Comment on attachment 8389454 [details] [diff] [review]
> Part 9 [v2]: Introduce an "intermediate" coordinate space to share across
> chained filters.
> 
> >+ * To understand the spaces better, let's take an example filter that defines a
> >+ * filter region:
> >+ *   <filter id="f" x="-15" y="-15" width="130" height="130">...</filter>
> >+ *
> >+ * And apply the filter to a div element:
> >+ *   <div style="filter: url(#f); ...">...</div>
> >+ *
> >+ * And let's say there are 2 device pixels for every 1 CSS pixel.
> >+ *
> >+ * Then:
> >+ *   "user space" = the CSS pixel space of the <div>
> >+ *   "intermediate space" = "user space" * 2
> >+ *   "filter space" = "intermediate space" + 15
> 
> Wouldn't this be "+ 30"? I love the use of an example here, but this one
> confuses me :)

You're right! The example is wrong, so I guess it's good that you were confused :)

In the example, I incorrectly subtracted the filter region in user space (-15) instead of the filter region in intermediate space (-30) from "intermediate space". The code in the patch does the right thing.

I'll post a clarified and corrected example shortly. Please let me know what you think.
Assignee

Comment 87

5 years ago
Here is the corrected example.

I also noticed a related bug in the code, which I've fixed in this patch.

In UserSpaceToFilterSpace, I was scaling the wrong direction by calling IntermediateSpaceToUserSpace instead of UserSpaceToIntermediateSpace.

I'll look into finding a way to test this so that we can't regress it in the future. I think the tests are running on machines (like mine) where "device pixels per CSS pixel" == 1, so the bug didn't reproduce. I'm thinking that zooming in on the page should test it on those machines.
Attachment #8390061 - Flags: review?(mstange)
You can have a zoomed reftest by adding a reftest-zoom attribute on the root reftest element.
Assignee

Comment 89

5 years ago
(In reply to Robert Longson from comment #88)
> You can have a zoomed reftest by adding a reftest-zoom attribute on the root
> reftest element.

Thanks for the pointer! I've updated the patch (Part 10) with a zoomed reftest. As expected, zooming makes device pixels per CSS pixel != 1.

In the test, I used fePointLight because it calls nsSVGFilterInstance::ConvertLocation to transform the light position from user space to filter space. The test fails if this conversion is wrong.
Attachment #8390061 - Attachment is obsolete: true
Attachment #8390061 - Flags: review?(mstange)
Attachment #8390686 - Flags: review?(mstange)
Comment on attachment 8390686 [details] [diff] [review]
Part 10 [v2]: Fix user space to filter space conversion and example.

Looks good.

What would need to be done in order to make everything that's currently in filter space to be in intermediate space, so that we can rename "intermediate space" to "filter space"? Or is there a reason that that wouldn't work?
Attachment #8390686 - Flags: review?(mstange) → review+
Assignee

Comment 91

5 years ago
(In reply to Markus Stange [:mstange] from comment #90)
> Comment on attachment 8390686 [details] [diff] [review]
> Part 10 [v2]: Fix user space to filter space conversion and example.
> 
> Looks good.

Thanks for the review!

> 
> What would need to be done in order to make everything that's currently in
> filter space to be in intermediate space, so that we can rename
> "intermediate space" to "filter space"? Or is there a reason that that
> wouldn't work?

I like the idea of redefining filter space as intermediate space. I think that could work.

There are only a few places in SVG filters where we use filter space (as currently defined as the filter region):

---

1) SVGFEPointLightElement

SVGFEPointLightElement::ComputeLightAttributes does:
  map.Set(ePointLightPosition, aInstance->ConvertLocation(lightPos));

Here, ConvertLocation does a user space to filter space conversion, and aInstance is the nsSVGFilterInstance.

2) SVGFESpotLightElement

SVGFESpotLightElement::ComputeLightAttributes does:
  map.Set(eSpotLightPosition, aInstance->ConvertLocation(lightPos));
  map.Set(eSpotLightPointsAt, aInstance->ConvertLocation(pointsAt));

This is similar to fePointLight.

3) SVGFETurbulenceElement

SVGFETurbulenceElement::GetPrimitiveDescription does:
  gfxRect firstPeriodInUserSpace(0, 0, 1 / fX, 1 / fY);
  gfxRect firstPeriodInFilterSpace = aInstance->UserSpaceToFilterSpace(firstPeriodInUserSpace);
  Size frequencyInFilterSpace(1 / firstPeriodInFilterSpace.width,
                              1 / firstPeriodInFilterSpace.height);
  gfxPoint offset = firstPeriodInFilterSpace.TopLeft();
  ...
  descr.Attributes().Set(eTurbulenceOffset, IntPoint(offset.x, offset.y));
  descr.Attributes().Set(eTurbulenceBaseFrequency, frequencyInFilterSpace);

Here, frequencyInFilterSpace is the same as intermediate space, since the frequency is just scaled.
I think the eTurbulenceOffset would always be (0, 0) if we were defining it in intermediate space.

---

I think we can use intermediate space in all these places (and rename it to filter space).

As I planned before, when we're done building all of the FilterPrimitiveDescriptions for the filter chain in intermediate space, we'll compute the overall filter region. Then, we'll translate all of the filter primitive subregions so that they have positive position values in overall filter space. We'll also translate the fePointLight, feSpotLight, and feTurbulence offsets. Luckily, most values in SVG filters are relative (dx, dy, stdDev, etc.), so there's not too much translation to do :)

I'm assuming FilterPrimitiveDescription doesn't expect filter primitive subregions with negative positions. If it did, we wouldn't need the final translation.

I'll go ahead and try to remove filter space as currently defined, and rename "intermediate space" to "filter space".
(In reply to Max Vujovic from comment #91)
> SVGFETurbulenceElement::GetPrimitiveDescription does:
>   gfxRect firstPeriodInUserSpace(0, 0, 1 / fX, 1 / fY);
>   gfxRect firstPeriodInFilterSpace =
> aInstance->UserSpaceToFilterSpace(firstPeriodInUserSpace);
>   Size frequencyInFilterSpace(1 / firstPeriodInFilterSpace.width,
>                               1 / firstPeriodInFilterSpace.height);
>   gfxPoint offset = firstPeriodInFilterSpace.TopLeft();
>   ...
>   descr.Attributes().Set(eTurbulenceOffset, IntPoint(offset.x, offset.y));
>   descr.Attributes().Set(eTurbulenceBaseFrequency, frequencyInFilterSpace);
> 
> Here, frequencyInFilterSpace is the same as intermediate space, since the
> frequency is just scaled.
> I think the eTurbulenceOffset would always be (0, 0) if we were defining it
> in intermediate space.

It's not necessarily (0, 0) for filtered SVG elements; if a turbulence-covered <rect> is positioned at (100, 100) in user space, the turbulence will be offset. It's like the rect is a "window" into the infinite, user-space anchored turbulence background; repositioning the rect will reposition the window and give a view of a different part of the turbulence.
For normal HTML elements that's not the case, since their user space is anchored at the element itself, so I think you're right that eTurbulenceOffset would always be (0, 0) for those.

> I'm assuming FilterPrimitiveDescription doesn't expect filter primitive
> subregions with negative positions. If it did, we wouldn't need the final
> translation.

Actually, I'm pretty sure it can handle negative positions just fine. That's one of the big advantages bug 924103 bought us, I just didn't realize the implications back then. Before bug 924103, we were sizing all intermediate graphics surfaces to the filter space bounds, and treated all filter space positions as relative to the surface origin, so we couldn't handle negative positions. But now they're just values that we pass to Moz2D, and Moz2D filters can definitely handle them.
I have a concern about the overall filter region and how it relates to filter primitive subregion clipping.

At the moment we ignore the filter region when we calculate filter primitive subregions. The subregions are allowed to extend beyond the filter region. Only when it comes to calculating source and output rects, and to the actual drawing, we intersect the primitive subregions with the filter space bounds. There are several places in FilterSupport.cpp where such an intersection happens.

However, when there are multiple SVG filters with different filter regions in an SVG filter chain, and there's only one final filter space bounds rect that's intersected with all filters' primitive subregions, things will go wrong, won't they?
Maybe the best way to fix this is to move this intersection operation up into the primitive subregion calculation. As far as I can tell, there's no need to keep primitive subregions and the filter region separate until the last moment. (My first intuition was that it would make a difference for feTile, because I thought feTile should repeat the unclamped input primitive subregion, but that's not the case, as was discussed in bug 521207 and bug 522866.)
And when FilterSupport no longer needs the filter space bounds in order to clamp the primitive subregions, the only remaining use of them is the clipping of SourceGraphic / SourceAlpha / FillPaint / StrokePaint. And with multiple SVG filters in the chain, those inputs will need to be clipped to different filter space bounds, depending on what individual filter the respective primitive belongs to... not sure how to handle that.
My hope was that we might be able to get rid of the mFilterSpaceBounds field in the FilterDescription struct entirely, but it looks like that won't be the case. Too bad. :-)
Assignee

Comment 94

5 years ago
I've posted an experimental patch that removes filter space as currently defined (with its origin at the top left corner of the filter region). Everything uses "intermediate space" now, which shares its origin with user space (and I plan to rename "intermediate space" to "filter space"). I've tried to make the fewest amount of changes- I'll refactor things in the real patch.

The experimental patch passes all the svg/filters and svg/smil reftests.

(In reply to Markus Stange [:mstange] from comment #92)
> (In reply to Max Vujovic from comment #91)
> > SVGFETurbulenceElement::GetPrimitiveDescription does:
> >   gfxRect firstPeriodInUserSpace(0, 0, 1 / fX, 1 / fY);
> >   gfxRect firstPeriodInFilterSpace =
> > aInstance->UserSpaceToFilterSpace(firstPeriodInUserSpace);
> >   Size frequencyInFilterSpace(1 / firstPeriodInFilterSpace.width,
> >                               1 / firstPeriodInFilterSpace.height);
> >   gfxPoint offset = firstPeriodInFilterSpace.TopLeft();
> >   ...
> >   descr.Attributes().Set(eTurbulenceOffset, IntPoint(offset.x, offset.y));
> >   descr.Attributes().Set(eTurbulenceBaseFrequency, frequencyInFilterSpace);
> > 
> > Here, frequencyInFilterSpace is the same as intermediate space, since the
> > frequency is just scaled.
> > I think the eTurbulenceOffset would always be (0, 0) if we were defining it
> > in intermediate space.
> 
> It's not necessarily (0, 0) for filtered SVG elements; if a
> turbulence-covered <rect> is positioned at (100, 100) in user space, the
> turbulence will be offset. It's like the rect is a "window" into the
> infinite, user-space anchored turbulence background; repositioning the rect
> will reposition the window and give a view of a different part of the
> turbulence.
> For normal HTML elements that's not the case, since their user space is
> anchored at the element itself, so I think you're right that
> eTurbulenceOffset would always be (0, 0) for those.

Thanks for the explanation. That makes sense.

If we define filter space to have the same origin as user space, I think eTurbulenceOffset will always be (0, 0) for all elements because the feTurbulence filter primitive subregion already encodes the offset from user space. This is because the filter primitive subregion will be relative to the the new filter space origin, which is the same as the user space origin. In the current code, eTurbulenceOffset is only compensating for the difference in origin between user space and filter space.

I wrote a little reftest in the experimental patch I posted for feTurbulence, and it behaves as desired (and eTurbulenceOffset is always evaluating to 0,0 in the code).

> 
> > I'm assuming FilterPrimitiveDescription doesn't expect filter primitive
> > subregions with negative positions. If it did, we wouldn't need the final
> > translation.
> 
> Actually, I'm pretty sure it can handle negative positions just fine. That's
> one of the big advantages bug 924103 bought us, I just didn't realize the
> implications back then. Before bug 924103, we were sizing all intermediate
> graphics surfaces to the filter space bounds, and treated all filter space
> positions as relative to the surface origin, so we couldn't handle negative
> positions. But now they're just values that we pass to Moz2D, and Moz2D
> filters can definitely handle them.

That's great! I'm not seeing any problems with negative positions in the experimental patch.

(In reply to Markus Stange [:mstange] from comment #93)
> I have a concern about the overall filter region and how it relates to
> filter primitive subregion clipping.
> 
> At the moment we ignore the filter region when we calculate filter primitive
> subregions. The subregions are allowed to extend beyond the filter region.
> Only when it comes to calculating source and output rects, and to the actual
> drawing, we intersect the primitive subregions with the filter space bounds.
> There are several places in FilterSupport.cpp where such an intersection
> happens.
> 
> However, when there are multiple SVG filters with different filter regions
> in an SVG filter chain, and there's only one final filter space bounds rect
> that's intersected with all filters' primitive subregions, things will go
> wrong, won't they?
> Maybe the best way to fix this is to move this intersection operation up
> into the primitive subregion calculation. As far as I can tell, there's no
> need to keep primitive subregions and the filter region separate until the
> last moment.

That's exactly what I was thinking. In my prototype [1], as the last step of computing the filter primitive subregion, I intersected it with the parent filter region. This worked out well :)

[1]: https://github.com/mvujovic/gecko-dev/blob/css-filters-rendering/layout/svg/nsSVGFilterInstance.cpp#L527

> (My first intuition was that it would make a difference for
> feTile, because I thought feTile should repeat the unclamped input primitive
> subregion, but that's not the case, as was discussed in bug 521207 and bug
> 522866.)
> And when FilterSupport no longer needs the filter space bounds in order to
> clamp the primitive subregions, the only remaining use of them is the
> clipping of SourceGraphic / SourceAlpha / FillPaint / StrokePaint. And with
> multiple SVG filters in the chain, those inputs will need to be clipped to
> different filter space bounds, depending on what individual filter the
> respective primitive belongs to... not sure how to handle that.

Well, only the first filter in the chain can access SourceGraphic / SourceAlpha. For the next filter in the chain, SourceGraphic / SourceAlpha refer to the output of the previous filter. That might help?

Maybe we should clip the primitive subregion of the the last FilterPrimitiveDescription of a filter to the filter region of the next filter in the chain.

FillPaint and StrokePaint still refer to the filtered element for the later filters in the chain. Too bad FillPaint and StrokePaint can reference paint servers like gradients. If they were just colors, I'd insert an feFlood filter in the chain with the right primitive subregion :).

> My hope was that we might be able to get rid of the mFilterSpaceBounds field
> in the FilterDescription struct entirely, but it looks like that won't be
> the case. Too bad. :-)

Somehow I'm still hopeful! :)
Assignee

Comment 95

5 years ago
Checkin-needed for "Part 10 [v2]: Fix user space to filter space conversion and example."

Try results look good: https://tbpl.mozilla.org/?tree=Try&rev=833bdc2f10c8
Keywords: checkin-needed
(In reply to Max Vujovic from comment #94)
> Created attachment 8391622 [details] [diff] [review]
> Experimental Patch: Make filter space use same origin as user space.
> 
> I've posted an experimental patch that removes filter space as currently
> defined (with its origin at the top left corner of the filter region).
> Everything uses "intermediate space" now, which shares its origin with user
> space (and I plan to rename "intermediate space" to "filter space"). I've
> tried to make the fewest amount of changes- I'll refactor things in the real
> patch.
> 
> The experimental patch passes all the svg/filters and svg/smil reftests.

Nice!

> (In reply to Markus Stange [:mstange] from comment #92)
> If we define filter space to have the same origin as user space, I think
> eTurbulenceOffset will always be (0, 0) for all elements because the
> feTurbulence filter primitive subregion already encodes the offset from user
> space.

Of course, you're right!

> This is because the filter primitive subregion will be relative to
> the the new filter space origin, which is the same as the user space origin.
> In the current code, eTurbulenceOffset is only compensating for the
> difference in origin between user space and filter space.
> 
> I wrote a little reftest in the experimental patch I posted for
> feTurbulence, and it behaves as desired (and eTurbulenceOffset is always
> evaluating to 0,0 in the code).

Thank you for that; it's one of the tests I wanted to write for bug 924103 but somehow I never did.

> Well, only the first filter in the chain can access SourceGraphic /
> SourceAlpha. For the next filter in the chain, SourceGraphic / SourceAlpha
> refer to the output of the previous filter. That might help?
> 
> Maybe we should clip the primitive subregion of the the last
> FilterPrimitiveDescription of a filter to the filter region of the next
> filter in the chain.

Indeed, that should work.
Attachment #8390686 - Flags: checkin+
Assignee

Comment 99

5 years ago
This patch is based on the experimental patch and adds some refactoring.

The patch renames "intermediate space" to "filter space" in nsSVGFilterInstance. In nsFilterInstance, it changes the filter space origin to the user space origin instead of the filter region origin.

The patch also moves the method "ComputeUserSpaceToFilterSpaceScale" from nsSVGFilterInstance to nsFilterInstance. Both classes need the user space to filter space scale factors. Now, nsFilterInstance computes them and passes them to all of the nsSVGFilterInstance(s) it creates.
Attachment #8395786 - Flags: review?(mstange)
Comment on attachment 8395786 [details] [diff] [review]
Part 11: Rename intermediate space to filter space and change filter space origin to user space origin.

>@@ -92,17 +76,19 @@ public:
>   /**
>    * @param aFilter The SVG reference filter to process.
>    * @param aTargetFrame The frame of the filtered element under consideration.
>    * @param aTargetBBox The SVG bbox to use for the target frame, computed by
>    *   the caller. The caller may decide to override the actual SVG bbox.
>    */
>   nsSVGFilterInstance(const nsStyleFilter& aFilter,
>                       nsIFrame *aTargetFrame,
>-                      const gfxRect& aTargetBBox);
>+                      const gfxRect& aTargetBBox,
>+                      gfxSize aUserSpaceToFilterSpaceScale,
>+                      gfxSize aFilterSpaceToUserSpaceScale);

It would be more efficient to pass a gfxSize as a const reference rather than by value.
Assignee

Comment 101

5 years ago
(In reply to Robert Longson from comment #100)
> Comment on attachment 8395786 [details] [diff] [review]
> Part 11: Rename intermediate space to filter space and change filter space
> origin to user space origin.
> 
> >@@ -92,17 +76,19 @@ public:
> >   /**
> >    * @param aFilter The SVG reference filter to process.
> >    * @param aTargetFrame The frame of the filtered element under consideration.
> >    * @param aTargetBBox The SVG bbox to use for the target frame, computed by
> >    *   the caller. The caller may decide to override the actual SVG bbox.
> >    */
> >   nsSVGFilterInstance(const nsStyleFilter& aFilter,
> >                       nsIFrame *aTargetFrame,
> >-                      const gfxRect& aTargetBBox);
> >+                      const gfxRect& aTargetBBox,
> >+                      gfxSize aUserSpaceToFilterSpaceScale,
> >+                      gfxSize aFilterSpaceToUserSpaceScale);
> 
> It would be more efficient to pass a gfxSize as a const reference rather
> than by value.

Fixed. Thanks!
Attachment #8395786 - Attachment is obsolete: true
Attachment #8395786 - Flags: review?(mstange)
Attachment #8395829 - Flags: review?(mstange)
Attachment #8395829 - Flags: review?(mstange) → review+
Assignee

Comment 102

5 years ago
Posted file Try failures for "Part 11..." patch (obsolete) —
Assignee

Comment 103

5 years ago
I’ve posted a patch which makes two SVG tests a little fuzzier. This patch should land with the "Part 11…” patch.

The try bots report two test failures with the "Part 11…” patch on Windows 8 with D2D and GPU accelerated layers enabled [1]. All other platforms look good.

The difference isn’t visible to the naked eye (see the attachment for a diff [2]). The edges of the blur are different than the reference by a maximum of 3 units on the RGB channels.

- dynamic-text-02.svg fails because it’s slightly fuzzier than currently specified in reftest.list (for D2D + GPU layers).
- dynamic-rect-02.svg fails because it’s fuzzy and not currently marked as fuzzy.

These tests were added in bug 423998, and bug 776038 tracks the first one's fuzziness.

The tests are only fuzzy where there is a transformed + blurred SVG rect being compared to a non-transformed + blurred reference rect. The reference rect is positioned using x, y instead of transform.

The “Part 11…” patch changes the values that FilterPrimitiveDescription(s) use for their subregions (e.g. [-5, -5, 100, 100] instead of [0, 0, 100, 100]). I think this influences the exact values that end up in D2D, and as a result, D2D renders the blur a tiny bit differently now.

[1]: https://tbpl.mozilla.org/?tree=Try&rev=d10f2153e281
[2]: https://bug948265.bugzilla.mozilla.org/attachment.cgi?id=8397422
Attachment #8397424 - Flags: review?(longsonr)
Attachment #8397424 - Flags: review?(longsonr) → review+
Assignee

Updated

5 years ago
Attachment #8397424 - Attachment description: Patch 12: Make dynamic-text-02.svg and dynamic-rect-02.svg fuzzier with D2D and GPU layers enabled. → Part 12: Make dynamic-text-02.svg and dynamic-rect-02.svg fuzzier with D2D and GPU layers enabled.
Assignee

Comment 104

5 years ago
Assignee

Comment 105

5 years ago
Checkin-needed for "Parts 11 & 12 for landing".

Try results look good: https://tbpl.mozilla.org/?tree=Try&rev=4caedd1ddd8c
Keywords: checkin-needed
Attachment #8395829 - Attachment is obsolete: true
Attachment #8397422 - Attachment is obsolete: true
Attachment #8397424 - Attachment is obsolete: true
Attachment #8397920 - Flags: checkin+
Blocks: 927892
Assignee

Comment 108

5 years ago
I won't be available to work on this bug for a little while, so I'm posting this in-progress patch in case Markus needs to take over in the meantime. I'm hoping to get back to this bug at the end of May.

This patch is done in terms of code and tests. I'd just like to move all of the multiple-svg-filters* tests into their own folder first, since the filenames are getting too long and hard to read.
Assignee

Comment 109

5 years ago
Comment on attachment 8405167 [details] [diff] [review]
Part 14: Clip filter primitives to their filter's filter region. (In progress, not for landing)

Rename part 13 to part 14 in order to insert another patch in between.
Attachment #8405167 - Attachment description: Part 13: Clip filter primitives to their filter's filter region. (In progress, not for landing) → Part 14: Clip filter primitives to their filter's filter region. (In progress, not for landing)
Assignee

Comment 110

5 years ago
This patch moves the SVG filter chain tests into a new folder: /layout/reftests/svg/filters/svg-filter-chains
Attachment #8444671 - Flags: review?(mstange)
Comment on attachment 8444671 [details] [diff] [review]
Part 13: Move SVG filter chain tests into their own folder to make their filenames more readable.

Nice, and welcome back!
Attachment #8444671 - Flags: review?(mstange) → review+
Assignee

Comment 112

5 years ago
(In reply to Markus Stange [:mstange] from comment #111)
> Comment on attachment 8444671 [details] [diff] [review]
> Part 13: Move SVG filter chain tests into their own folder to make their
> filenames more readable.
> 
> Nice, and welcome back!

Thanks! Glad to be back!
Assignee

Comment 113

5 years ago
Checkin-needed for "Part 13: Move SVG filter chain tests into their own folder to make their filenames more readable."

Try results look good: https://tbpl.mozilla.org/?tree=Try&rev=a27d4d9cf271
Keywords: checkin-needed
Attachment #8444671 - Flags: checkin+
Assignee

Comment 116

5 years ago
In this patch, I tried to use W3C test formatting for the tests so they can be submitted to the W3C test suite.

@Dirk: Can you take a brief look at the tests and let me know if they look like properly formatted W3C tests?

Otherwise, this patch clips filter primitive subregions to their filter's filter region.

It also sets the default primitive subregion to the filter region if a primitive's input is a standard input (e.g. SourceGraphic keyword), based on this sentence from the spec:

"""
If there are no referenced nodes (e.g., for <feImage> or <feTurbulence>), or one or more of the referenced nodes is a standard input (one of SourceGraphic, SourceAlpha, BackgroundImage, BackgroundAlpha, FillPaint or StrokePaint), or for <feTile> (which is special because its principal function is to replicate the referenced node in X and Y and thereby produce a usually larger result), the default subregion is 0%, 0%, 100%, 100%, where as a special-case the percentages are relative to the dimensions of the filter region, thus making the default filter primitive subregion equal to the filter region.
"""
http://www.w3.org/TR/filter-effects/#FilterPrimitiveSubRegion

Before this patch, if a primitive used a standard input corresponding to a previous filter, the primitive subregion was being set to the primitive subregion of the last primitive of the previous filter.
Attachment #8405167 - Attachment is obsolete: true
Assignee

Comment 117

5 years ago
Patch is ready for review. There's a brief description in the previous comment.

I made some minor test formatting changes based on feedback from Dirk on IRC.
Attachment #8445978 - Attachment is obsolete: true
Attachment #8446138 - Flags: review?(mstange)
Clip filter primitives to their filter's filter region seems like it should be a separate bug rather than buried in this bugs set of patches.
We already do it, just in a different place. This is just one step of a bit of refactoring and doesn't cause any functional changes, so I think it's fine to do here.
Attachment #8446138 - Flags: review?(mstange) → review+
Assignee

Comment 120

5 years ago
(In reply to Robert Longson from comment #118)
> Clip filter primitives to their filter's filter region seems like it should
> be a separate bug rather than buried in this bugs set of patches.

(In reply to Markus Stange [:mstange] from comment #119)
> We already do it, just in a different place. This is just one step of a bit
> of refactoring and doesn't cause any functional changes, so I think it's
> fine to do here.

Thanks for the review!

To elaborate, filter primitives are already clipped to the "overall" filter region for the filter chain (see FilterSupport.cpp:1130). When there is one SVG filter in the filter chain, the "overall" filter region equals the SVG filter's filter region, so there is no functional change.

However, when there are multiple SVG filters in the filter chain, the primitives inside each filter need to be clipped to their parent filter's filter region. This patch adds that functional change, which is covered by the tests. The tests will fail without this patch.

Let me know if you'd prefer future patches to be under their own bugs. So far, this bug covers SVG and CSS filter chain functionality. Next up, I'll be adding proper support for the SourceAlpha keyword between filters in filter chains.
Assignee

Comment 121

5 years ago
Checkin-needed for "Part 14: Clip filter primitives to their filter's filter region."

Try results look good: https://tbpl.mozilla.org/?tree=Try&rev=2dfd149a7cb0
Keywords: checkin-needed
Keywords: checkin-needed
Target Milestone: mozilla30 → ---

Comment 122

5 years ago
landing
Comment on attachment 8446138 [details] [diff] [review]
Part 14: Clip filter primitives to their filter's filter region.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a368ab7e93ab
Attachment #8446138 - Flags: checkin+
Assignee

Comment 123

5 years ago
This patch is nearly ready for review. I just need to run it through the try bots.

I'll be on vacation for the next two weeks, so I'll ask for a review when I get back.
Assignee

Comment 125

5 years ago
Comment on attachment 8447446 [details] [diff] [review]
Part 15: Support SourceAlpha keyword in SVG filter chains. (In progress)

This patch is ready for review.

The try results look good:
https://tbpl.mozilla.org/?tree=Try&rev=f5291a15f490
Attachment #8447446 - Flags: review?(mstange)
Comment on attachment 8447446 [details] [diff] [review]
Part 15: Support SourceAlpha keyword in SVG filter chains. (In progress)

>@@ -1000,16 +1005,17 @@ InputAlphaModelForPrimitive(const Filter
> {
>   switch (aDescr.Type()) {
>     case PrimitiveType::Tile:
>     case PrimitiveType::Offset:
>       return aOriginalAlphaModel;
> 
>     case PrimitiveType::ColorMatrix:
>     case PrimitiveType::ComponentTransfer:
>+    case PrimitiveType::ToAlpha:
>       return AlphaModel::Unpremultiplied;

I think this should instead go to the "return aOriginalAlphaModel" case. Otherwise I think we'll insert an unnecessary premultplied-to-unpremultiplied FilterNode in front of the ToAlpha node in some cases.

---

Are you planning to remove kPrimitiveIndexSourceAlpha in a future patch by using GetOrCreateSourceAlphaIndex even for the first filter in the chain? That might save a few lines of code.
Attachment #8447446 - Flags: review?(mstange) → review+
Assignee

Comment 127

5 years ago
(In reply to Markus Stange [:mstange] from comment #126)
> Comment on attachment 8447446 [details] [diff] [review]
> Part 15: Support SourceAlpha keyword in SVG filter chains. (In progress)
> 
> >@@ -1000,16 +1005,17 @@ InputAlphaModelForPrimitive(const Filter
> > {
> >   switch (aDescr.Type()) {
> >     case PrimitiveType::Tile:
> >     case PrimitiveType::Offset:
> >       return aOriginalAlphaModel;
> > 
> >     case PrimitiveType::ColorMatrix:
> >     case PrimitiveType::ComponentTransfer:
> >+    case PrimitiveType::ToAlpha:
> >       return AlphaModel::Unpremultiplied;
> 
> I think this should instead go to the "return aOriginalAlphaModel" case.
> Otherwise I think we'll insert an unnecessary
> premultplied-to-unpremultiplied FilterNode in front of the ToAlpha node in
> some cases.

Good point! Thanks.

> 
> ---
> 
> Are you planning to remove kPrimitiveIndexSourceAlpha in a future patch by
> using GetOrCreateSourceAlphaIndex even for the first filter in the chain?
> That might save a few lines of code.

Yes, I had the same feeling. I'll do that in the next patch.
Would you mind adding 

<!--
     Any copyright is dedicated to the Public Domain.
     http://creativecommons.org/publicdomain/zero/1.0/
-->

to the reftest files, unless there's some reason you would prefer not to.
Assignee

Comment 129

5 years ago
(In reply to Robert Longson from comment #128)
> Would you mind adding 
> 
> <!--
>      Any copyright is dedicated to the Public Domain.
>      http://creativecommons.org/publicdomain/zero/1.0/
> -->
> 
> to the reftest files, unless there's some reason you would prefer not to.

Sure! I'll do that.
Assignee

Comment 130

5 years ago
Addressed review comments in this patch:
- Moved PrimitiveType::ToAlpha to the "return aOriginalAlphaModel;" case.
- Added public domain copyright notices to the test and ref file.

Running it through the bots once more to be safe.
Attachment #8447446 - Attachment is obsolete: true
Assignee

Comment 131

5 years ago
This patch adds the missing copyright headers to the tests I wrote in the svg-filter-chains folder.
Attachment #8456335 - Flags: review?(longsonr)
Assignee

Comment 132

5 years ago
Markus - I removed kPrimitiveIndexSourceAlpha in this patch. Do you think it looks better?

The code reduction isn't quite as much as I had hoped; I had to add a case for setting default values for the ToAlpha PrimitiveDescription when there is no previous filter.
Attachment #8456344 - Flags: review?(mstange)
Attachment #8456335 - Flags: review?(longsonr) → review+
Comment on attachment 8456344 [details] [diff] [review]
Part 17: Remove kPrimitiveIndexSourceAlpha since nsSVGFilterInstance creates ToAlpha filter nodes now. (Blocked by 1042864)

I think it's slightly better, yes. Not as much as I'd hoped for, but oh well :)
Attachment #8456344 - Flags: review?(mstange) → review+
Assignee

Comment 134

5 years ago
Comment on attachment 8456328 [details] [diff] [review]
Part 15 [v2]: Support SourceAlpha keyword in SVG filter chains.

Add [v2] to patch name.
Attachment #8456328 - Attachment description: Part 15: Support SourceAlpha keyword in SVG filter chains. → Part 15 [v2]: Support SourceAlpha keyword in SVG filter chains.
Assignee

Comment 135

5 years ago
Checkin-needed for:

"Part 15 [v2]: Support SourceAlpha keyword in SVG filter chains."
"Part 16: Add public domain copyright notice to SVG filter chain tests."
"Part 17: Remove kPrimitiveIndexSourceAlpha since nsSVGFilterInstance creates ToAlpha filter nodes now."

Try results look good:
Part 15: https://tbpl.mozilla.org/?tree=Try&rev=13c586d0be43
Part 15+16+17: https://tbpl.mozilla.org/?tree=Try&rev=9acff8c709d2
Keywords: checkin-needed
sorry had to backout this changes for crashes on windows 7 debug like https://tbpl.mozilla.org/php/getParsedLog.php?id=44015041&tree=Mozilla-Inbound
Assignee

Comment 138

5 years ago
(In reply to Carsten Book [:Tomcat] from comment #137)
> sorry had to backout this changes for crashes on windows 7 debug like
> https://tbpl.mozilla.org/php/getParsedLog.php?id=44015041&tree=Mozilla-
> Inbound

Thanks, Carsten. I'll look into it.
Assignee

Comment 139

5 years ago
Comment on attachment 8456328 [details] [diff] [review]
Part 15 [v2]: Support SourceAlpha keyword in SVG filter chains.

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

::: gfx/src/FilterSupport.cpp
@@ +1010,2 @@
>        return aOriginalAlphaModel;
>  

@Markus -

I debugged this on Windows a bit. I think having PrimitiveType::ToAlpha here is causing the Win7 assertion [1] occurring deep in SourceSurfaceD2DTarget.cpp. When I move PrimitiveType::ToAlpha from the aOriginalAlphaModel case to the Alpha::Unpremultiplied case, the crash goes away.

I'm thinking PrimitiveType::ToAlpha needs to be under the Unpremultiplied case since ToAlpha becomes a DISCRETE_TRANSFER FilterNode (like PrimitiveType::ComponentTransfer, which is also under this case). Does that sound right?

I'm still not quite sure why this potential incorrectness triggers the specific assertion deep in SourceSurfaceD2DTarget.cpp- I'm not as familiar with the D2D code.

[1]: https://tbpl.mozilla.org/php/getParsedLog.php?id=44015041&tree=Mozilla-Inbound
Assignee

Updated

5 years ago
Depends on: 1042864
Assignee

Comment 140

5 years ago
Please ignore my previous comment- ToAlpha really shouldn't need Unpremultiplied input. That's just a workaround that avoids the assert by constructing a different filter graph.

I figured out what's causing the assert and posted my findings in bug 1042864.

I suggest we defer the refactoring in "Part 17: Remove kPrimitiveIndexSourceAlpha..." until bug 1042864 is resolved. 

The functionality in "Part 15 [v2]: Support SourceAlpha keyword..." isn't causing the assert, so we can still land that. (And "Part 16: Add public domain copyright notice..." is harmless.)
Assignee

Comment 141

5 years ago
Checkin-needed for "Part 15 [v2]: Support SourceAlpha keyword in SVG filter chains."

New try results look good: https://tbpl.mozilla.org/?tree=Try&rev=70397f676df8

(It was Part 17 that triggered the assert on Win7. This patch should be safe.)
Keywords: checkin-needed
mSourceAlphaAvailable should be a bool
QA Whiteboard: [qa-]
Assignee

Comment 145

5 years ago
(In reply to Robert Longson from comment #144)
> mSourceAlphaAvailable should be a bool

Oops- thanks! Here's a tiny patch for review please.
Attachment #8462019 - Flags: review?(longsonr)
Assignee

Updated

5 years ago
Attachment #8456344 - Attachment description: Part 17: Remove kPrimitiveIndexSourceAlpha since nsSVGFilterInstance creates ToAlpha filter nodes now. → Part 17: Remove kPrimitiveIndexSourceAlpha since nsSVGFilterInstance creates ToAlpha filter nodes now. (Blocked by 1042864)
Assignee

Comment 146

5 years ago
Check-in needed for "Part 16: Add public domain copyright notice to SVG filter chain tests."

No try needed; this patch only adds comments to test files. Tests pass locally.
Keywords: checkin-needed
Assignee

Comment 147

5 years ago
This patch removes the filter region variable (mFilterSpaceBounds) from FilterDescription. Instead, we keep track of the filter region for each FilterPrimitiveDescription so that we can:
- Clip the original SourceGraphic to the filter region of the first filter in a filter chain. (The code before this patch clipped to the now-removed FilterDescription.mFilterSpaceBounds, which was set to last filter region as a placeholder.)
- Use the appropriate filter region for a filter primitive in FilterSupport::ComputePostFilterExtents for the FillPaint and StrokePaint inputs.
- Compute the required size for the FillPaint and StrokePaint surfaces in a new method nsFilterInstance::SourcePaintUserSpaceBounds, which considers the relevant filter regions. The FillPaint and StrokePaint surfaces need to be big enough to be reused by all of the FilterPrimitiveDescriptions that reference them.

The patch additionally:
- Removes unnecessary PrimitiveSubregion and filter region (mFilterSpaceBounds) intersection calculations in FilterSupport.cpp, since the PrimitiveSubregion is already clipped to its filter region in nsFilterInstance.cpp.

After this patch, we’ll be able to define filter regions for CSS filters, which will equal the previous output rect, expanded by the affected area of the CSS filter.
Attachment #8462095 - Flags: review?(mstange)
Attachment #8462019 - Flags: review?(longsonr) → review+
Comment on attachment 8462095 [details] [diff] [review]
Part 19: Keep track of the filter region for each FilterPrimitiveDescription.

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

Looks really good except for the missing FilterPrimitiveDescription::mFilterSpaceBounds IPDL serialization. As always I'm delighted about the number of tests you're adding.

::: gfx/ipc/GfxMessageUtils.h
@@ -1107,5 @@
>    typedef mozilla::gfx::FilterDescription paramType;
>  
>    static void Write(Message* aMsg, const paramType& aParam)
>    {
> -    WriteParam(aMsg, aParam.mFilterSpaceBounds);

You're removing the serialization/deserialization of mFilterSpaceBounds for FilterDescription, but you're not adding any for FilterPrimitiveDescription.

::: gfx/src/FilterSupport.h
@@ +322,5 @@
>    {
>      mFilterPrimitiveSubregion = aRect;
>    }
>  
> +  void SetFilterSpaceBounds(const IntRect& aRect) {

Mozilla C++ coding style requires the opening brace to be on its own line.

::: layout/reftests/svg/filters/svg-filter-chains/reftest.list
@@ +8,5 @@
>  == clip-output.svg clip-output.svg
>  == default-subregion.svg default-subregion-ref.svg
> +== different-FillPaint-filter-regions.svg different-FillPaint-filter-regions-ref.svg
> +== different-StrokePaint-filter-regions.svg different-StrokePaint-filter-regions-ref.svg
> +== dont-clip-previous-primitives.svg dont-clip-previous-primitives.svg

missing a "-ref" here

::: layout/svg/nsFilterInstance.cpp
@@ +22,5 @@
>  using namespace mozilla::dom;
>  using namespace mozilla::gfx;
>  
> +static nsIntRect
> +ToNsIntRect(const IntRect& aRect) {

use ThebesIntRect from gfx2DGlue.h

@@ +27,5 @@
> +  return nsIntRect(aRect.x, aRect.y, aRect.width, aRect.height);
> +}
> +
> +static gfxRect
> +ToGfxRect(const IntRect& aRect) {

use ThebesRect from gfx2DGlue.h

@@ +294,5 @@
>    mStrokePaint.mNeededBounds = strokePaintNeededRegion.GetBounds();
>  }
>  
> +static bool
> +HasInputIndex(const FilterPrimitiveDescription& aDescr, int32_t aInputIndex) {

line break before opening {, also applies to nsFilterInstance::SourcePaintUserSpaceBounds

@@ +349,5 @@
>      nsSVGUtils::GetCanvasTM(mTargetFrame, nsISVGChildFrame::FOR_PAINTING,
>                              mTransformRoot);
>    if (!matrix.IsSingular()) {
>      gfx->Multiply(matrix);
> +    gfx->Rectangle(SourcePaintUserSpaceBounds(aSourcePaintIndex));

Hmm, is this even necessary? The surface is sized to neededRect, which comes from aSource->mNeededBounds, which is calculated by FilterSupport::ComputeSourceNeededRegions, which already considers all the primitives that use the source paint as input. And the correct clipping of the source input is done later during filter rendering, right? Or am I missing something?
Assignee

Comment 150

5 years ago
Comment on attachment 8462095 [details] [diff] [review]
Part 19: Keep track of the filter region for each FilterPrimitiveDescription.

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

Thanks for the review Markus! And for the test appreciation :)

::: gfx/ipc/GfxMessageUtils.h
@@ -1107,5 @@
>    typedef mozilla::gfx::FilterDescription paramType;
>  
>    static void Write(Message* aMsg, const paramType& aParam)
>    {
> -    WriteParam(aMsg, aParam.mFilterSpaceBounds);

Fixed!

::: gfx/src/FilterSupport.h
@@ +322,5 @@
>    {
>      mFilterPrimitiveSubregion = aRect;
>    }
>  
> +  void SetFilterSpaceBounds(const IntRect& aRect) {

Fixed.

::: layout/reftests/svg/filters/svg-filter-chains/reftest.list
@@ +8,5 @@
>  == clip-output.svg clip-output.svg
>  == default-subregion.svg default-subregion-ref.svg
> +== different-FillPaint-filter-regions.svg different-FillPaint-filter-regions-ref.svg
> +== different-StrokePaint-filter-regions.svg different-StrokePaint-filter-regions-ref.svg
> +== dont-clip-previous-primitives.svg dont-clip-previous-primitives.svg

Good catch! I noticed a few other tests here are missing "-ref" too. I added it to clip-output and second-filter-uses-SourceAlpha.

Also, when I added "-ref" to "dont-clip-previous-primitives.svg", my shades of green didn't match, so I tweaked the reference file to pass through a single color matrix filter to get the right green (like some of the other tests).

::: layout/svg/nsFilterInstance.cpp
@@ +22,5 @@
>  using namespace mozilla::dom;
>  using namespace mozilla::gfx;
>  
> +static nsIntRect
> +ToNsIntRect(const IntRect& aRect) {

Done. Much better.

@@ +27,5 @@
> +  return nsIntRect(aRect.x, aRect.y, aRect.width, aRect.height);
> +}
> +
> +static gfxRect
> +ToGfxRect(const IntRect& aRect) {

The conversion isn't needed anymore since I removed nsFilterInstance::SourcePaintUserSpaceBounds based on your last comment.

@@ +294,5 @@
>    mStrokePaint.mNeededBounds = strokePaintNeededRegion.GetBounds();
>  }
>  
> +static bool
> +HasInputIndex(const FilterPrimitiveDescription& aDescr, int32_t aInputIndex) {

I've removed both of these functions.

@@ +349,5 @@
>      nsSVGUtils::GetCanvasTM(mTargetFrame, nsISVGChildFrame::FOR_PAINTING,
>                              mTransformRoot);
>    if (!matrix.IsSingular()) {
>      gfx->Multiply(matrix);
> +    gfx->Rectangle(SourcePaintUserSpaceBounds(aSourcePaintIndex));

Great point! Using neededRect is exactly right. I removed the new functions SourcePaintUserSpaceBounds and HasInputIndex, since we have neededRect.

Correct clipping of the original SourceGraphic to the filter region also occurs in FilterSupport::ComputeSourceNeededRegions, like this:

aSourceGraphicNeededRegion.And(aSourceGraphicNeededRegion,
                               ThebesIntRect(firstDescr.FilterSpaceBounds()));

This means we don't need to insert a Crop FilterNode to clip the original SourceGraphic, since the surface is already sized correctly. In general, we create Crop FilterNodes for PrimitiveSubregions, and the last PrimitiveSubregion of a filter (its output) is clipped to the filter region of the next filter.
Assignee

Comment 151

5 years ago
New patch, addressing review comments.
Attachment #8462095 - Attachment is obsolete: true
Attachment #8462095 - Flags: review?(mstange)
Attachment #8462866 - Flags: review?(mstange)
Assignee

Comment 152

5 years ago
Checkin-needed for "Part 18: Change mSourceAlphaAvailable from int32_t to bool."

Try results look good: https://tbpl.mozilla.org/?tree=Try&rev=ca857176c346
Keywords: checkin-needed
Comment on attachment 8462866 [details] [diff] [review]
Part 19 [v2]: Keep track of the filter region for each FilterPrimitiveDescription.

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

Great!
Attachment #8462866 - Flags: review?(mstange) → review+
(In reply to Max Vujovic from comment #150)
> Also, when I added "-ref" to "dont-clip-previous-primitives.svg", my shades
> of green didn't match

Adding color-interpolation-filters="sRGB" on the affected <filter> elements might also have fixed this, but piping both the test and the reference through similar color transforms is probably more robust.

> Correct clipping of the original SourceGraphic to the filter region also
> occurs in FilterSupport::ComputeSourceNeededRegions, like this:
> 
> aSourceGraphicNeededRegion.And(aSourceGraphicNeededRegion,
>                               
> ThebesIntRect(firstDescr.FilterSpaceBounds()));
> 
> This means we don't need to insert a Crop FilterNode to clip the original
> SourceGraphic, since the surface is already sized correctly.

I'd prefer to allow the users of FilterSupport::RenderFilterDescription to pass in source surfaces that contain more than needed and still get correct clipping.
Duplicate of this bug: 1044494
Assignee

Comment 157

5 years ago
(In reply to Markus Stange [:mstange] from comment #155)
> (In reply to Max Vujovic from comment #150)
> > Also, when I added "-ref" to "dont-clip-previous-primitives.svg", my shades
> > of green didn't match
> 
> Adding color-interpolation-filters="sRGB" on the affected <filter> elements
> might also have fixed this, but piping both the test and the reference
> through similar color transforms is probably more robust.

I tried adding color-interpolation-filters="sRGB". This makes the hueRotate results exactly match the math in the spec, but it's still challenging to get whole number results. For example, rotating full red (1, 0, 0) results in dark green (0, 0.356, 0). I would hope for full green (0, 1, 0), but I don't think any hueRotate angle can do that. Therefore, I agree piping through the same color transform is more robust (and at least easier to write and maintain).

> 
> > Correct clipping of the original SourceGraphic to the filter region also
> > occurs in FilterSupport::ComputeSourceNeededRegions, like this:
> > 
> > aSourceGraphicNeededRegion.And(aSourceGraphicNeededRegion,
> >                               
> > ThebesIntRect(firstDescr.FilterSpaceBounds()));
> > 
> > This means we don't need to insert a Crop FilterNode to clip the original
> > SourceGraphic, since the surface is already sized correctly.
> 
> I'd prefer to allow the users of FilterSupport::RenderFilterDescription to
> pass in source surfaces that contain more than needed and still get correct
> clipping.

Makes sense. In a follow-up patch, I'll insert a Crop FilterNode if the original SourceGraphic surface rect doesn't match the first filter region.
Assignee

Comment 158

5 years ago
Comment on attachment 8462866 [details] [diff] [review]
Part 19 [v2]: Keep track of the filter region for each FilterPrimitiveDescription.

Pushed to mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2960a81f4ff5

Try results look good:
https://tbpl.mozilla.org/?tree=Try&rev=69596f149002
Attachment #8462866 - Flags: checkin+
Assignee

Comment 159

5 years ago
This patch inserts a Crop FilterNode if the original SourceGraphic surface exceeds the first filter region. We shouldn't ever fall into this case, since the surface is sized using FilterSupport::ComputeSourceNeededRegions, which considers the filter region. However, handling this case makes FilterSupport::FilterNodeGraphFromDescription more robust because it'll produce the correct results even if the surface size exceeds the filter region.
Attachment #8464233 - Flags: review?(mstange)
Assignee

Comment 161

5 years ago
This patch makes the CSS blur filter function work.

Most of the code is in the new nsCSSFilterInstance class. Otherwise, I factored out a variable "mTargetBBoxInFilterSpace" in nsFilterInstance, so we can pass it to nsCSSFilterInstance and avoid computing it in multiple places.
Attachment #8465787 - Flags: review?(mstange)
Attachment #8464233 - Flags: review?(mstange) → review+
Comment on attachment 8465787 [details] [diff] [review]
Part 21: Add nsCSSFilterInstance with support for adding CSS blur filters to filter graphs.

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

::: layout/svg/nsCSSFilterInstance.cpp
@@ +103,5 @@
> +    NS_NOTREACHED("we shouldn't have parsed a negative radius in the style");
> +    return NS_ERROR_FAILURE;
> +  }
> +  radiusInFilterSpace = std::min(radiusInFilterSpace,
> +                                 gfxSize(kMaxStdDeviation, kMaxStdDeviation));

This probably doesn't do what you want. I think you really do need to write std::min twice, otherwise you'll be using gfxSize::operator<, and std::min will return the first argument if neither size is less than the other one. E.g. std::min(gfxSize(1, 3), gfxSize(2, 2)) == gfxSize(1, 3) when you really want gfxSize(1, 2).
(http://www.cplusplus.com/reference/algorithm/min/ says: "If both are equivalent, a is returned.")

@@ +134,5 @@
> +    FilterSupport::PostFilterExtentsForPrimitive(aDescr, inputExtents);
> +  IntRect outputBounds = ToIntRect(outputExtents.GetBounds());
> +
> +  aDescr.SetPrimitiveSubregion(outputBounds);
> +  aDescr.SetFilterSpaceBounds(outputBounds);

Thinking ahead: If, instead of calculating non-interfering bounds here, we had a flag on FilterPrimitiveDescription that says "don't clip", would that simplify the code? E.g. our FilterNode construction code would just not insert any Crop FilterNodes if the noclip flag is set. All those Crop FilterNodes are only necessary for correctness and don't give any performance benefits, so it seems like a waste to calculate rects that we'd like to ignore completely.
One way to express such a flag would be to turn the PrimitiveSubregion and FilterSpaceBounds fields from IntRects into Maybe<IntRect>s - I think that's the closest equivalent we have to something like None | Some(IntRect).

Hmm, actually, looking through the list of possible CSS filters, this maybe isn't worth doing after all. Drop shadow is almost the same as blur, and the color matrix ones are trivial. What do you think?

::: layout/svg/nsCSSFilterInstance.h
@@ +5,5 @@
> +
> +#ifndef __NS_CSSFILTERINSTANCE_H__
> +#define __NS_CSSFILTERINSTANCE_H__
> +
> +#include "nsStyleStruct.h"

If you can turn mFilter into a reference (see below), forward declare nsStyleFilter instead

@@ +14,5 @@
> + * FilterPrimitiveDescription connected to the filter graph.
> + */
> +class nsCSSFilterInstance
> +{
> +typedef mozilla::gfx::FilterPrimitiveDescription FilterPrimitiveDescription;

indent this

@@ +65,5 @@
> +
> +  /**
> +   * The CSS filter originally from the style system.
> +   */
> +  nsStyleFilter mFilter;

Can you make this a const reference and call out in the documentation of the constructor that it aFilter shouldn't go away during the lifetime of the created nsCSSFilterInstance?
Assignee

Comment 163

5 years ago
Comment on attachment 8465787 [details] [diff] [review]
Part 21: Add nsCSSFilterInstance with support for adding CSS blur filters to filter graphs.

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

::: layout/svg/nsCSSFilterInstance.cpp
@@ +103,5 @@
> +    NS_NOTREACHED("we shouldn't have parsed a negative radius in the style");
> +    return NS_ERROR_FAILURE;
> +  }
> +  radiusInFilterSpace = std::min(radiusInFilterSpace,
> +                                 gfxSize(kMaxStdDeviation, kMaxStdDeviation));

Good point! Thanks.

@@ +134,5 @@
> +    FilterSupport::PostFilterExtentsForPrimitive(aDescr, inputExtents);
> +  IntRect outputBounds = ToIntRect(outputExtents.GetBounds());
> +
> +  aDescr.SetPrimitiveSubregion(outputBounds);
> +  aDescr.SetFilterSpaceBounds(outputBounds);

I prototyped not setting PrimitiveSubregion and FilterSpaceBounds on FilterPrimitiveDescriptions for CSS Filters. Unfortunately, it required adding checks for whether a filter has a PrimitiveSubregion and FilterSpaceBounds to a bunch of calcuations. The extra complexity doesn't really seem worth it. I'll post my experimental patch in the next comment, so you can see how it could look.

I think a good compromise would be to focus on avoiding Crop FilterNode insertion, and keep the rest of the calculations with PrimitiveSubregion and FilterSpaceBounds unchanged. To that end, I could add an boolean FilterPrimitiveDescription::mShouldClip that's true by default. We can set it to false for CSS filters, and we can use it only for controlling the insertion of a Crop FilterNode. I like to think of it as a performance hint.

Are you ok with the compromise? Or do you think we should try something similar to the experimental patch, which avoids setting PrimitiveSubregion and FilterSpaceBounds on CSS Filters?

::: layout/svg/nsCSSFilterInstance.h
@@ +5,5 @@
> +
> +#ifndef __NS_CSSFILTERINSTANCE_H__
> +#define __NS_CSSFILTERINSTANCE_H__
> +
> +#include "nsStyleStruct.h"

Will do.

@@ +14,5 @@
> + * FilterPrimitiveDescription connected to the filter graph.
> + */
> +class nsCSSFilterInstance
> +{
> +typedef mozilla::gfx::FilterPrimitiveDescription FilterPrimitiveDescription;

Will do.

@@ +65,5 @@
> +
> +  /**
> +   * The CSS filter originally from the style system.
> +   */
> +  nsStyleFilter mFilter;

Will do.
Attachment #8465787 - Flags: review?(mstange)
Assignee

Comment 164

5 years ago
Here's the experimental patch that avoids setting PrimitiveSubregion and FilterSpaceBounds on CSS Filters. You'll notice a bunch of new descr.HasBounds() checks.

Also, I had to change nsFilterInstance::OutputFilterSpaceBounds to a new function ComputePostFilterExtentsInFilterSpace. OutputFilterSpaceBounds used to return the last PrimitiveDescription's PrimitiveSubregion, but now that's not guaranteed to exist if the last filter was a CSS filter. Instead, I calculate the post filter extents in ComputePostFilterExtentsInFilterSpace and cache them for subsequent calls.
(In reply to Max Vujovic from comment #163)
> I prototyped not setting PrimitiveSubregion and FilterSpaceBounds on
> FilterPrimitiveDescriptions for CSS Filters. Unfortunately, it required
> adding checks for whether a filter has a PrimitiveSubregion and
> FilterSpaceBounds to a bunch of calcuations. The extra complexity doesn't
> really seem worth it. I'll post my experimental patch in the next comment,
> so you can see how it could look.

Hmm, yeah, that doesn't look like it simplified anything. Then let's not worry about it now.

> I think a good compromise would be to focus on avoiding Crop FilterNode
> insertion, and keep the rest of the calculations with PrimitiveSubregion and
> FilterSpaceBounds unchanged. To that end, I could add an boolean
> FilterPrimitiveDescription::mShouldClip that's true by default. We can set
> it to false for CSS filters, and we can use it only for controlling the
> insertion of a Crop FilterNode. I like to think of it as a performance hint.

Sounds reasonable, but I think we should hold off on it until we have evidence that crop filter nodes have a cost associated with them. I like your previous approach, let's just go ahead with that. :)
Assignee

Comment 166

5 years ago
Thanks for looking again :)

Here's a patch based on the first one, with the other review comments addressed.

(In reply to Markus Stange [:mstange] from comment #165)
> (In reply to Max Vujovic from comment #163)
> > I prototyped not setting PrimitiveSubregion and FilterSpaceBounds on
> > FilterPrimitiveDescriptions for CSS Filters. Unfortunately, it required
> > adding checks for whether a filter has a PrimitiveSubregion and
> > FilterSpaceBounds to a bunch of calcuations. The extra complexity doesn't
> > really seem worth it. I'll post my experimental patch in the next comment,
> > so you can see how it could look.
> 
> Hmm, yeah, that doesn't look like it simplified anything. Then let's not
> worry about it now.
> 
> > I think a good compromise would be to focus on avoiding Crop FilterNode
> > insertion, and keep the rest of the calculations with PrimitiveSubregion and
> > FilterSpaceBounds unchanged. To that end, I could add an boolean
> > FilterPrimitiveDescription::mShouldClip that's true by default. We can set
> > it to false for CSS filters, and we can use it only for controlling the
> > insertion of a Crop FilterNode. I like to think of it as a performance hint.
> 
> Sounds reasonable, but I think we should hold off on it until we have
> evidence that crop filter nodes have a cost associated with them. I like
> your previous approach, let's just go ahead with that. :)

Sounds good!
Attachment #8465787 - Attachment is obsolete: true
Attachment #8467228 - Attachment is obsolete: true
Attachment #8467304 - Flags: review?(mstange)
Comment on attachment 8467304 [details] [diff] [review]
Part 21 [v3]: Add nsCSSFilterInstance with support for adding CSS blur filters to filter graphs.

Thanks!
Attachment #8467304 - Flags: review?(mstange) → review+
Assignee

Comment 168

5 years ago
Comment on attachment 8464233 [details] [diff] [review]
Part 20: Insert a Crop FilterNode if the original SourceGraphic surface size exceeds the filter region.

Pushed "Part 20..." to mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4634a64d20e

Try results look good:
https://tbpl.mozilla.org/?tree=Try&rev=bfd2172a1298
Attachment #8464233 - Flags: checkin+
Assignee

Comment 170

5 years ago
Markus- The previous nsCSSFilterInstance patch didn't compile on the Linux try bots. I needed to add some forward declarations and includes to nsCSSFilterInstance.h. I wanted to make sure you're ok with these:

---
#include "FilterSupport.h"
#include "gfxMatrix.h"
#include "gfxRect.h"

struct nsStyleFilter;
template<class T> class nsTArray;
---

I also added a typedef for PrimitiveType:
typedef mozilla::gfx::PrimitiveType PrimitiveType;

I wanted to forward declare FilterPrimitiveDescription and PrimitiveType to avoid the FilterSupport.h include, but I don't think we have a nice way to forward declare enums created with MOZ_BEGIN_ENUM_CLASS for both C++11 and non-C++11 compilers. At least the comment in this bug suggests that: https://bugzilla.mozilla.org/show_bug.cgi?id=886755#c16.
Attachment #8468547 - Flags: review?(mstange)
Comment on attachment 8468547 [details] [diff] [review]
Part 21 [v4]: Add nsCSSFilterInstance with support for adding CSS blur filters to filter graphs (with build fixes).

Yeah, I think this is the best you can get.
Attachment #8468547 - Flags: review?(mstange) → review+
Assignee

Comment 172

5 years ago
Comment on attachment 8468547 [details] [diff] [review]
Part 21 [v4]: Add nsCSSFilterInstance with support for adding CSS blur filters to filter graphs (with build fixes).

Pushed to mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c3fbddeb7a2

Try results look good:
https://tbpl.mozilla.org/?tree=Try&rev=44c69ccb46bb
Attachment #8468547 - Flags: checkin+
Assignee

Updated

5 years ago
Attachment #8467304 - Attachment is obsolete: true
Assignee

Comment 174

5 years ago
This patch makes CSS drop-shadow() work.

I factored out a function nsCSSFilterInstance::BlurRadiusToFilterSpace() that's used by both blur() and drop-shadow() now.

The filters spec [1] references the backgrounds spec [2] regarding the default drop-shadow() color value. Accordingly, drop-shadow() uses the CSS color property of the filtered element as the default value in this patch.

[1]: http://www.w3.org/TR/filter-effects-1/#funcdef-drop-shadow
[2]: http://www.w3.org/TR/css3-background/#the-box-shadow
Attachment #8471175 - Flags: review?(mstange)
Assignee

Comment 175

5 years ago
New patch- Forgot to put a function's opening brace on a new line.
Attachment #8471175 - Attachment is obsolete: true
Attachment #8471175 - Flags: review?(mstange)
Attachment #8471190 - Flags: review?(mstange)
Comment on attachment 8471190 [details] [diff] [review]
Part 22 [v2]: Add CSS drop-shadow filter to nsCSSFilterInstance.

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

::: layout/reftests/svg/filters/css-filters/drop-shadow-ref.html
@@ +21,5 @@
> +  <div id="target"></div>
> +  <svg width="0" height="0">
> +    <!--
> +      This equivalent SVG filter is defined in:
> +      http://www.w3.org/TR/filter-effects-1/#dropshadowEquivalent

Why isn't it defined in terms of feDropShadow?

@@ +23,5 @@
> +    <!--
> +      This equivalent SVG filter is defined in:
> +      http://www.w3.org/TR/filter-effects-1/#dropshadowEquivalent
> +    -->
> +    <filter id="drop-shadow" x="-50" y="-50" width="200" height="200">

Why doesn't this need color-interpolation-filters="sRGB"? I thought CSS filters defaulted to sRGB while SVG filters default to linearRGB.
Attachment #8471190 - Flags: review?(mstange) → review+
> Why doesn't this need color-interpolation-filters="sRGB"? I thought CSS filters defaulted to sRGB while SVG filters default to linearRGB.

Many filters default to linearRGB. Filters that use CSS colours such as lighting filters and drop shadow are sRGB always however as the CSS colour they use is always sRGB.
I was specifically concerned about the blur that happens as part of the drop shadow filter. Blurring in linearRGB will look different from blurring in sRGB, even if the input colors match.
feDropShadow operates in sRGB as it uses an sRGB CSS colour. It can't operate in linearRGB and ignores color-interpolation-filters for that reason.
Or at least if you specify linearRGB it converts to sRGB in order to do the shadow colouring which comes to the same thing.
(In reply to Robert Longson from comment #179)
> and ignores color-interpolation-filters for that reason.

I think this is the part I'm confused about. Where does that ignoring happen? SVGFEDropShadowElement doesn't override OperatesOnSRGB as far as I can tell.
(I probably knew the answer to this question at one point, because I reviewed the shadow patch, but I've forgotten about it.)
Assignee

Comment 183

5 years ago
(In reply to Markus Stange [:mstange] from comment #176)
> Comment on attachment 8471190 [details] [diff] [review]
> Part 22 [v2]: Add CSS drop-shadow filter to nsCSSFilterInstance.
> 
> Review of attachment 8471190 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/reftests/svg/filters/css-filters/drop-shadow-ref.html
> @@ +21,5 @@
> > +  <div id="target"></div>
> > +  <svg width="0" height="0">
> > +    <!--
> > +      This equivalent SVG filter is defined in:
> > +      http://www.w3.org/TR/filter-effects-1/#dropshadowEquivalent
> 
> Why isn't it defined in terms of feDropShadow?

Good idea- I'll change the patch to use feDropShadow instead. I copied from the spec, which I think referred to the expanded filter graph in case feDropShadow didn't make it into the spec.

> 
> @@ +23,5 @@
> > +    <!--
> > +      This equivalent SVG filter is defined in:
> > +      http://www.w3.org/TR/filter-effects-1/#dropshadowEquivalent
> > +    -->
> > +    <filter id="drop-shadow" x="-50" y="-50" width="200" height="200">
> 
> Why doesn't this need color-interpolation-filters="sRGB"? I thought CSS
> filters defaulted to sRGB while SVG filters default to linearRGB.

Good question. I think in this case, it happens to works because the test uses full green (0, 255, 0), which is equal in both sRGB and linearRGB.

When I change the test and ref to use half green (0, 128, 0), they no longer match without adding color-interpolation-filters="sRGB". However, they still look identical to the naked eye, which I believe is what the code Robert pointed out ensures.

I think adding color-interpolation-filters="sRGB" just ensures a similar filter graph is created for feDropShadow to the one created for drop-shadow(), so there is less accumulated error from the calculations.

I'll post two debug dumps of the intermediate surfaces for feDropShadow with color-interpolation-filters="sRGB" and without, so you can see what's going on.
Could really do with some CSS animation tests of the blur/shadow parameters. I think Chrome supports CSS animation of CSS filters.
Assignee

Comment 188

5 years ago
(In reply to Robert Longson from comment #187)
> Could really do with some CSS animation tests of the blur/shadow parameters.
> I think Chrome supports CSS animation of CSS filters.

Definitely. I'm planning on adding animation tests after I finish implementing the set of built-in filters. Do you know of a good example in the codebase of testing the intermediate renderings of CSS Animations and Transitions?

I see SMIL tests have a JS helper function setTimeAndSnapshot(), but that uses the SVG DOM to pause animations, which doesn't help for CSS animations.

If there isn't a good way to verify intermediate renderings, I suppose I could write tests that sample getComputedStyle. IIRC that's what I relied on in WebKit.
Assignee

Comment 189

5 years ago
This patch is for landing and addresses Markus's review comments.

The ref files now use an feDropShadow filter primitives with color-interpolation-filters="sRGB", which reduces differences between the drop-shadow() and feDropShadow filter graphs.

I also added a few typedefs and includes to nsCSSFilterInstance.h to satisfy the Linux try bots:

+#include "mozilla/gfx/Point.h"
+#include "mozilla/gfx/Types.h"
+#include "nsColor.h"

+class nsIFrame;

...

 class nsCSSFilterInstance
 {
+  typedef mozilla::gfx::Color Color;
   typedef mozilla::gfx::FilterPrimitiveDescription FilterPrimitiveDescription;
+  typedef mozilla::gfx::IntPoint IntPoint;
   typedef mozilla::gfx::PrimitiveType PrimitiveType;
+  typedef mozilla::gfx::Size Size;
Attachment #8471190 - Attachment is obsolete: true
Assignee

Comment 190

5 years ago
Comment on attachment 8473082 [details] [diff] [review]
Part 22 [v3]: Add CSS drop-shadow filter to nsCSSFilterInstance.

Pushed to mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/27ad2e7f6215

Try looks good (though spotty due to some downtime yesterday):
https://tbpl.mozilla.org/?tree=Try&rev=fcd6d01847ef
Attachment #8473082 - Flags: checkin+
Comment on attachment 8473082 [details] [diff] [review]
Part 22 [v3]: Add CSS drop-shadow filter to nsCSSFilterInstance.

> {
>@@ -33,16 +36,19 @@ nsCSSFilterInstance::BuildPrimitives(nsT
>   switch(mFilter.GetType()) {
>     case NS_STYLE_FILTER_BLUR:
>       descr = CreatePrimitiveDescription(PrimitiveType::GaussianBlur, aPrimitiveDescrs);
>       result = SetAttributesForBlur(descr);
>       break;
>     case NS_STYLE_FILTER_BRIGHTNESS:
>     case NS_STYLE_FILTER_CONTRAST:
>     case NS_STYLE_FILTER_DROP_SHADOW:
>+      descr = CreatePrimitiveDescription(PrimitiveType::DropShadow, aPrimitiveDescrs);
>+      result = SetAttributesForDropShadow(descr);
>+      break;
>     case NS_STYLE_FILTER_GRAYSCALE:
>     case NS_STYLE_FILTER_HUE_ROTATE:
>     case NS_STYLE_FILTER_INVERT:
>     case NS_STYLE_FILTER_OPACITY:
>     case NS_STYLE_FILTER_SATURATE:
>     case NS_STYLE_FILTER_SEPIA:
>       return NS_ERROR_NOT_IMPLEMENTED;
>     default:

Doesn't this mean that brightness an contrast filters are now drop shadows?
Assignee

Comment 192

5 years ago
(In reply to Robert Longson from comment #191)
> Comment on attachment 8473082 [details] [diff] [review]
> Part 22 [v3]: Add CSS drop-shadow filter to nsCSSFilterInstance.
> 
> > {
> >@@ -33,16 +36,19 @@ nsCSSFilterInstance::BuildPrimitives(nsT
> >   switch(mFilter.GetType()) {
> >     case NS_STYLE_FILTER_BLUR:
> >       descr = CreatePrimitiveDescription(PrimitiveType::GaussianBlur, aPrimitiveDescrs);
> >       result = SetAttributesForBlur(descr);
> >       break;
> >     case NS_STYLE_FILTER_BRIGHTNESS:
> >     case NS_STYLE_FILTER_CONTRAST:
> >     case NS_STYLE_FILTER_DROP_SHADOW:
> >+      descr = CreatePrimitiveDescription(PrimitiveType::DropShadow, aPrimitiveDescrs);
> >+      result = SetAttributesForDropShadow(descr);
> >+      break;
> >     case NS_STYLE_FILTER_GRAYSCALE:
> >     case NS_STYLE_FILTER_HUE_ROTATE:
> >     case NS_STYLE_FILTER_INVERT:
> >     case NS_STYLE_FILTER_OPACITY:
> >     case NS_STYLE_FILTER_SATURATE:
> >     case NS_STYLE_FILTER_SEPIA:
> >       return NS_ERROR_NOT_IMPLEMENTED;
> >     default:
> 
> Doesn't this mean that brightness an contrast filters are now drop shadows?

Good catch- thanks. I'll fix that right away.
Assignee

Comment 193

5 years ago
Patch to fix the switch that falls through from brightness and contrast to drop-shadow.

I like keeping the filters in alphabetical order. I added a return case to each one, so I can implement them in any order.
Attachment #8473322 - Flags: review?(longsonr)
Assignee

Comment 194

5 years ago
Attachment #8473375 - Flags: review?(mstange)
Assignee

Comment 195

5 years ago
Attachment #8473376 - Flags: review?(mstange)
Attachment #8473322 - Flags: review?(longsonr) → review-
Assignee

Comment 197

5 years ago
Comment on attachment 8473322 [details] [diff] [review]
Part 23: Add NOT_IMPLEMENTED return value for each CSS filter in nsCSSFilterInstance.

What would you like changed in this patch?
Flags: needinfo?(longsonr)
Comment on attachment 8473322 [details] [diff] [review]
Part 23: Add NOT_IMPLEMENTED return value for each CSS filter in nsCSSFilterInstance.

Nothing, just my fat fingers.
Attachment #8473322 - Flags: review- → review+
Flags: needinfo?(longsonr)
(In reply to Max Vujovic from comment #183)
Thanks for the details on the blurring color space issue. Here's what I think I was missing:
With regular blurs, the resulting interpolated colors between two *different* colors can look different. For example, if you have white text on a black background, blurring it in sRGB will the result make look subjectively darker overall, while blurring in linearRGB will keep the subjective lightness. However, with drop shadows, we don't blur between different colors; we only blur between a color and transparency. Blurring happens on premultiplied colors but the transformation between linearRGB to sRGB happens on unpremultiplied colors. And the premultiplied interpolated colors between the shadow color and transparency will all have the (roughly) same RGB values. So the interpolated colors won't look different regardless of which color space they were interpolated in.
Comment on attachment 8473375 [details] [diff] [review]
Part 24: Add CSS hue-rotate filter to nsCSSFilterInstance.

r?dbaron for the nsStyleCoord::GetAngleValueInDegrees() addition
Attachment #8473375 - Flags: review?(mstange)
Attachment #8473375 - Flags: review?(dbaron)
Attachment #8473375 - Flags: review+
Comment on attachment 8473376 [details] [diff] [review]
Part 25: Add CSS saturate filter to nsCSSFilterInstance.

r?dbaron for the nsStyleCoord::GetFactorOrPercentValue() addition
Attachment #8473376 - Flags: review?(mstange)
Attachment #8473376 - Flags: review?(dbaron)
Attachment #8473376 - Flags: review+

Updated

5 years ago
See Also: → 830089, 925025
Assignee

Comment 202

5 years ago
(In reply to Robert Longson from comment #198)
> Comment on attachment 8473322 [details] [diff] [review]
> Part 23: Add NOT_IMPLEMENTED return value for each CSS filter in
> nsCSSFilterInstance.
> 
> Nothing, just my fat fingers.

Thanks for the review!

Pushed to mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e56137ceaeb3

Try results look good:
https://tbpl.mozilla.org/?tree=Try&rev=6c96508e435e
Assignee

Comment 203

5 years ago
(In reply to Markus Stange [:mstange] from comment #199)
> (In reply to Max Vujovic from comment #183)
> Thanks for the details on the blurring color space issue. Here's what I
> think I was missing:
> With regular blurs, the resulting interpolated colors between two
> *different* colors can look different. For example, if you have white text
> on a black background, blurring it in sRGB will the result make look
> subjectively darker overall, while blurring in linearRGB will keep the
> subjective lightness. However, with drop shadows, we don't blur between
> different colors; we only blur between a color and transparency. Blurring
> happens on premultiplied colors but the transformation between linearRGB to
> sRGB happens on unpremultiplied colors. And the premultiplied interpolated
> colors between the shadow color and transparency will all have the (roughly)
> same RGB values. So the interpolated colors won't look different regardless
> of which color space they were interpolated in.

That makes a lot of sense. Thanks for the explanation!
Comment hidden (obsolete)
Comment hidden (obsolete)
Assignee

Comment 207

5 years ago
No worries, Tim. Thanks for testing!

Comment 208

5 years ago
(In reply to Markus Stange [:mstange] from comment #199)
> (In reply to Max Vujovic from comment #183)
> Thanks for the details on the blurring color space issue. Here's what I
> think I was missing:
> With regular blurs, the resulting interpolated colors between two
> *different* colors can look different. For example, if you have white text
> on a black background, blurring it in sRGB will the result make look
> subjectively darker overall, while blurring in linearRGB will keep the
> subjective lightness. However, with drop shadows, we don't blur between
> different colors; we only blur between a color and transparency. Blurring
> happens on premultiplied colors but the transformation between linearRGB to
> sRGB happens on unpremultiplied colors. And the premultiplied interpolated
> colors between the shadow color and transparency will all have the (roughly)
> same RGB values. So the interpolated colors won't look different regardless
> of which color space they were interpolated in.

I didn't thought about it yet. But the spec doesn't say that color space transformations apply on unpremultiplied colors, right?
I don't know if it does, but if not, it probably should. I don't think color space transformations on premultiplied colors make sense.
In any case, I don't think anything is wrong with how it works, I just needed a little time to wrap my head around it.
Comment on attachment 8473375 [details] [diff] [review]
Part 24: Add CSS hue-rotate filter to nsCSSFilterInstance.

> r?dbaron for the nsStyleCoord::GetAngleValueInDegrees() addition

Maybe put the 180.0 / M_PI in parentheses?  (I'm not sure if the compiler can constant-fold it otherwise, given floating point rules.)

r=dbaron
Attachment #8473375 - Flags: review?(dbaron) → review+
Comment on attachment 8473376 [details] [diff] [review]
Part 25: Add CSS saturate filter to nsCSSFilterInstance.

>r?dbaron for the nsStyleCoord::GetFactorOrPercentValue() addition

Please keep the methods in the same order in the declaration and implementation -- in other words, please move the implementation below the implementation of GetFactorValue.

r=dbaron with that
Attachment #8473376 - Flags: review?(dbaron) → review+
Assignee

Updated

5 years ago
Attachment #8476019 - Attachment description: Part 24: Add CSS hue-rotate filter to nsCSSFilterInstance. → Part 24 [v2]: Add CSS hue-rotate filter to nsCSSFilterInstance.
Assignee

Comment 214

5 years ago
Thanks for the review!

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) (away/busy Aug 27-Sep 11) from comment #210)
> Comment on attachment 8473375 [details] [diff] [review]
> Part 24: Add CSS hue-rotate filter to nsCSSFilterInstance.
> 
> > r?dbaron for the nsStyleCoord::GetAngleValueInDegrees() addition
> 
> Maybe put the 180.0 / M_PI in parentheses?  (I'm not sure if the compiler
> can constant-fold it otherwise, given floating point rules.)
> 
> r=dbaron

Done.

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) (away/busy Aug 27-Sep 11) from comment #211)
> Comment on attachment 8473376 [details] [diff] [review]
> Part 25: Add CSS saturate filter to nsCSSFilterInstance.
> 
> >r?dbaron for the nsStyleCoord::GetFactorOrPercentValue() addition
> 
> Please keep the methods in the same order in the declaration and
> implementation -- in other words, please move the implementation below the
> implementation of GetFactorValue.
> 
> r=dbaron with that

Done.
Assignee

Comment 215

5 years ago
Comment on attachment 8476019 [details] [diff] [review]
Part 24 [v2]: Add CSS hue-rotate filter to nsCSSFilterInstance.

Pushed to mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/95544f4ebf01

Try results look good:
https://tbpl.mozilla.org/?tree=Try&rev=8bad797faf56
Attachment #8476019 - Flags: checkin+
Assignee

Comment 216

5 years ago
Comment on attachment 8473322 [details] [diff] [review]
Part 23: Add NOT_IMPLEMENTED return value for each CSS filter in nsCSSFilterInstance.

This patch was missing a checkin+ flag.
Attachment #8473322 - Flags: checkin+
Assignee

Comment 217

5 years ago
Attachment #8476374 - Flags: review?(mstange)
Assignee

Comment 219

5 years ago
Comment on attachment 8476021 [details] [diff] [review]
Part 25 [v2]: Add CSS saturate filter to nsCSSFilterInstance.

Pushed to mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7453585c44e

Try results look good:
https://tbpl.mozilla.org/?tree=Try&rev=ac17b0407a6a
Attachment #8476021 - Flags: checkin+
Attachment #8476375 - Flags: review?(mstange) → review+
Comment on attachment 8476374 [details] [diff] [review]
Part 26: Add CSS grayscale filter to nsCSSFilterInstance.

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

::: layout/svg/nsCSSFilterInstance.cpp
@@ +16,5 @@
>  using namespace mozilla;
>  using namespace mozilla::gfx;
>  
> +namespace {
> +  float ClampFactor(float factor)

Why not just static float ClampFactor, instead of the unnamed namespace?
Attachment #8476374 - Flags: review?(mstange) → review+
Assignee

Comment 221

5 years ago
Thanks for the reviews!

(In reply to Markus Stange [:mstange] from comment #220)
> Comment on attachment 8476374 [details] [diff] [review]
> Part 26: Add CSS grayscale filter to nsCSSFilterInstance.
> 
> Review of attachment 8476374 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/svg/nsCSSFilterInstance.cpp
> @@ +16,5 @@
> >  using namespace mozilla;
> >  using namespace mozilla::gfx;
> >  
> > +namespace {
> > +  float ClampFactor(float factor)
> 
> Why not just static float ClampFactor, instead of the unnamed namespace?

No good reason- I now realize I misunderstood something about file static functions after reading some more. I'll change this to "static float ClampFactor", since it's shorter and more common in the source.
Assignee

Comment 223

5 years ago
This patch addresses Markus's review comment.
Attachment #8476374 - Attachment is obsolete: true
Assignee

Comment 224

5 years ago
Comment on attachment 8476794 [details] [diff] [review]
Part 26 [v2]: Add CSS grayscale filter to nsCSSFilterInstance.

Pushed to mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/da3072bdc57e

Try results look good:
https://tbpl.mozilla.org/?tree=Try&rev=70535c9cec61
Attachment #8476794 - Flags: checkin+
Assignee

Comment 225

5 years ago
Comment on attachment 8476375 [details] [diff] [review]
Part 27: Add CSS sepia filter to nsCSSFilterInstance.

Pushed to mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e09302b68abd

Try results look good:
https://tbpl.mozilla.org/?tree=Try&rev=f26c3a3419a1
Attachment #8476375 - Flags: checkin+
Assignee

Comment 226

5 years ago
Last four CSS filters coming up! (Note there's still more testing and refactoring I'd like to do after these land.)
Attachment #8476841 - Flags: review?(mstange)
Assignee

Comment 229

5 years ago
Attachment #8476847 - Flags: review?(mstange)
Attachment #8476841 - Flags: review?(mstange) → review+
Attachment #8476842 - Flags: review?(mstange) → review+
Attachment #8476845 - Flags: review?(mstange) → review+
Comment on attachment 8476847 [details] [diff] [review]
Part 31: Add CSS opacity filter to nsCSSFilterInstance.

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

::: layout/reftests/svg/filters/css-filters/opacity-over-one-translucent-source.html
@@ +14,5 @@
> +                 not change the translucency of an HTML element.">
> +  <style type="text/css">
> +    #target {
> +      filter: opacity(1000);
> +      opacity: 0.25;

Naming this test translucent "source" is actually a little misleading here, I think, because filters are applied on the opaque source and the CSS opacity applies to the result of the filter. You could use background-color: rgba(0, 255, 0, 0.25) instead.
Attachment #8476847 - Flags: review?(mstange) → review+
Also, woooo! :D
Assignee

Comment 232

5 years ago
(In reply to Markus Stange [:mstange] from comment #230)
> Comment on attachment 8476847 [details] [diff] [review]
> Part 31: Add CSS opacity filter to nsCSSFilterInstance.
> 
> Review of attachment 8476847 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> :::
> layout/reftests/svg/filters/css-filters/opacity-over-one-translucent-source.
> html
> @@ +14,5 @@
> > +                 not change the translucency of an HTML element.">
> > +  <style type="text/css">
> > +    #target {
> > +      filter: opacity(1000);
> > +      opacity: 0.25;
> 
> Naming this test translucent "source" is actually a little misleading here,
> I think, because filters are applied on the opaque source and the CSS
> opacity applies to the result of the filter. You could use background-color:
> rgba(0, 255, 0, 0.25) instead.

Ah, good point. I'll change it to rgba(0, 255, 0, 0.25) and keep the name.
Assignee

Comment 233

5 years ago
(In reply to Markus Stange [:mstange] from comment #231)
> Also, woooo! :D

Yeah! Thanks for the reviews :D
Question on the luminance.  We're using 0.2126, 0.7152, 0.0722 elsewhere in the code; here we have 0.2125, 0.7154, 0.0721.  Surely not a big difference, but I was wondering where your numbers came from - I don't think I've ever quite seen that variation (I've seen the "old school" 0.299, 0.587, 0.114).  And I'd rather we didn't have the discrepancy inside of Gecko, regardless of how imperceptible it may be.
The "here" in the luminance comment above is "luminance to alpha".  And the saturation filter uses 0.213, 0.715, 0.072 for the same type of computation, so it seems we should be consistent?
Assignee

Comment 236

5 years ago
(In reply to Milan Sreckovic [:milan] from comment #234)
> Question on the luminance.  We're using 0.2126, 0.7152, 0.0722 elsewhere in
> the code; here we have 0.2125, 0.7154, 0.0721.  Surely not a big difference,
> but I was wondering where your numbers came from - I don't think I've ever
> quite seen that variation (I've seen the "old school" 0.299, 0.587, 0.114). 
> And I'd rather we didn't have the discrepancy inside of Gecko, regardless of
> how imperceptible it may be.

Consistent would be good. I think Markus touched that section last- he might know better. We could standardize on 0.2126, 0.7152, 0.0722, since that seems to be common elsewhere in the code and matches the Filter Effects spec.

The Filter Effects spec uses 0.2126, 0.7152, 0.0722 in some places:
http://www.w3.org/TR/filter-effects/#grayscaleEquivalent

And rounds to 0.213, 0.715, 0.072 in other places:
http://www.w3.org/TR/filter-effects/#grayscaleEquivalent

It might be nice to put the values in some kind of constants used everywhere? I'm worried their scope would need to be too far reaching, though.
Assignee

Comment 237

5 years ago
(In reply to Max Vujovic from comment #236)
> (In reply to Milan Sreckovic [:milan] from comment #234)
> And rounds to 0.213, 0.715, 0.072 in other places:
> http://www.w3.org/TR/filter-effects/#grayscaleEquivalent

Sorry, that link should've been:
http://www.w3.org/TR/filter-effects/#feColorMatrixValuesAttribute
Yeah, the standard is inconsistent.  Not quite sure why we have both grayscale and saturate, when the only difference is the argument being 1-amount.  We should just standardize on 0.2126, 0.7152, 0.0722, and update the spec to reflect that.
I understand it is probably not practical to have these to live as constants in a single location, but perhaps it's good enough for now for them to just be #defines in each of the cpp files that needs them?  We can then also make it obvious that values like 0.787 are 0.7152 + 0.0722 (so, 0.7874), etc.
Assignee

Comment 239

5 years ago
(In reply to Milan Sreckovic [:milan] from comment #238)
> Yeah, the standard is inconsistent.  Not quite sure why we have both
> grayscale and saturate, when the only difference is the argument being
> 1-amount.  We should just standardize on 0.2126, 0.7152, 0.0722, and update
> the spec to reflect that.

Sounds good. I mailed public-fx about the spec:
http://lists.w3.org/Archives/Public/public-fx/2014JulSep/0108.html

I think having grayscale() is a nice reminder for authors that you can also desaturate images. With just saturate, maybe folks would forget that you can desaturate. But you're right- grayscale doesn't add any functionality over saturate.

> I understand it is probably not practical to have these to live as constants
> in a single location, but perhaps it's good enough for now for them to just
> be #defines in each of the cpp files that needs them?  We can then also make
> it obvious that values like 0.787 are 0.7152 + 0.0722 (so, 0.7874), etc.

Sounds like a good compromise to me. I'll write a patch next week to do that for FilterSupport.cpp. Also, thanks for pointing out that 0.7152 + 0.0722 = 0.7874 :)
It'd be nice to get this enabled by default when this lands, filed bug 1057180 :)

Updated

5 years ago
Blocks: 878883
(In reply to Max Vujovic from comment #239)
> ...
> Sounds like a good compromise to me. I'll write a patch next week to do that
> for FilterSupport.cpp. Also, thanks for pointing out that 0.7152 + 0.0722 =
> 0.7874 :)

To be fair, it's really (1 - 0.2126) that's relevant :)

If the spec was not written this way:

<feColorMatrix type="matrix"
  values="(0.2126 + 0.7874 * [1 - amount]) (0.7152 - 0.7152 * [1 - amount]) (0.0722 - 0.0722 * [1 - amount]) 0 0
          (0.2126 - 0.2126 * [1 - amount]) (0.7152 + 0.2848 * [1 - amount]) (0.0722 - 0.0722 * [1 - amount]) 0 0
          (0.2126 - 0.2126 * [1 - amount]) (0.7152 - 0.7152 * [1 - amount]) (0.0722 + 0.9278 * [1 - amount]) 0 0
           0 0 0 1 0"/> 

but like this instead:

<feColorMatrix type="matrix"
  values="([1 - amount] * 1 + [amount * 0.2126]) ([1 - amount) * 0 + amount * 0.7152) ([1 - amount] * 0 + [amount * 0.0722]) 0 0
          ([1 - amount] * 0 + [amount * 0.2126]) ([1 - amount) * 1 + amount * 0.7152) ([1 - amount] * 0 + [amount * 0.0722]) 0 0
          ([1 - amount] * 0 + [amount * 0.2126]) ([1 - amount) * 0 + amount * 0.7152) ([1 - amount] * 1 + [amount * 0.0722]) 0 0
           0 0 0 1 0"/> 

as silly as it is to have a zero multiplication in the formula, I think it should be easier to convince yourself that the intent is to just linearly interpolate between identity:

1 0 0 0 0
0 1 0 0 0
0 0 1 0 0
0 0 0 1 0

and luminance:

0.2126 0.7152 0.0722 0 0
0.2126 0.7152 0.0722 0 0
0.2126 0.7152 0.0722 0 0
0      0      0      1 0

Comment 244

5 years ago
dev-doc-info
Moved ddn from 878883. This bug at least adds the invert filter.
Keywords: dev-doc-needed
Assignee

Comment 245

5 years ago
(In reply to Milan Sreckovic [:milan] from comment #243)
> (In reply to Max Vujovic from comment #239)
> as silly as it is to have a zero multiplication in the formula, I think it
> should be easier to convince yourself that the intent is to just linearly
> interpolate between identity:

Thanks- that derivation makes a lot more sense than the reduced version in the spec. It's just linear interpolation! :)
Assignee

Comment 246

5 years ago
Updated patch based on Markus's review comment.

Used "background-color: rgba(0, 255, 0, 0.25)" instead of "opacity: 0.25" in opacity-over-one-translucent-source.html test.
Attachment #8476847 - Attachment is obsolete: true
Feel free to reopen if you still have patches to submit :) Thanks for the patches !
Status: ASSIGNED → RESOLVED
Closed: 6 years ago5 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Assignee