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.

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

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.

(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.

(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 :)

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.

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.

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

Checkin-needed for the patch "Parts 1 & 2 for landing" please.

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.

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!

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 :)

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

Whoops, forgot the log link: https://tbpl.mozilla.org/php/getParsedLog.php?id=34289353&tree=Mozilla-Inbound

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!

Checkin-needed for "Part 3 [v2]...". Thanks Ryan!

(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

(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 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 :)

Comment on attachment 8370892 [details] [diff] [review] Parts 1 & 2 for landing This isn't obsolete just because it already landed.

(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 :)

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

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.

(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!

Checkin-needed for "Part 4: Track an nsStyleFilter chain..." Try results look good: https://tbpl.mozilla.org/?tree=Try&rev=c1d003424598

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

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.

(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

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

Added reviewer to commit message in patch.

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).

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

(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.

(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!

(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!

Addressed review comment and rebased patch.

Rebased patch.

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/7913b3e61acb https://hg.mozilla.org/integration/mozilla-inbound/rev/926a5f6d263c

https://hg.mozilla.org/mozilla-central/rev/7913b3e61acb https://hg.mozilla.org/mozilla-central/rev/926a5f6d263c

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

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

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.

Comment on attachment 8382598 [details] [diff] [review] Part 8: Remove deprecated filterRes attribute. Review of attachment 8382598 [details] [diff] [review]: ----------------------------------------------------------------- Excellent idea!

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.

Addressed review comments and rebased patch.

Rebased patch.

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

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

Comment on attachment 8383804 [details] [diff] [review] Part 8 [v2]: Remove deprecated filterRes attribute. https://hg.mozilla.org/integration/mozilla-inbound/rev/c14980a16210

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.

(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.

(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 :)

(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

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

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).

Addressed longsonr's feedback. Added a SMIL animation test on the "order" attribute of feConvolveMatrix (See anim-feConvolveMatrix-order-01.svg).

(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.)

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?

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.

Checkin-needed for "Part 8 [v5]: Remove deprecated filterRes attribute. (Full Patch)". Try results look good: https://tbpl.mozilla.org/?tree=Try&rev=2375361fab06

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

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.

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?

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.)

Addressed review comments and rebased patch.

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.)

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

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 :)

(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.

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.

You can have a zoomed reftest by adding a reftest-zoom attribute on the root reftest element.

(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.

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?

(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. :-)

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! :)

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

(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.

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.

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.

(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!

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

Checkin-needed for "Parts 11 & 12 for landing". Try results look good: https://tbpl.mozilla.org/?tree=Try&rev=4caedd1ddd8c

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.

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.

This patch moves the SVG filter chain tests into a new folder: /layout/reftests/svg/filters/svg-filter-chains

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!

(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!

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

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.

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.

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.

(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.

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

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

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.

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

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.

(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.

(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.

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.

This patch adds the missing copyright headers to the tests I wrote in the svg-filter-chains folder.

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.

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 :)

Comment on attachment 8456328 [details] [diff] [review] Part 15 [v2]: Support SourceAlpha keyword in SVG filter chains. Add [v2] to patch name.

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/b208f5144753 https://hg.mozilla.org/integration/mozilla-inbound/rev/8ab800159525 https://hg.mozilla.org/integration/mozilla-inbound/rev/a5d4d7450d02

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

(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.

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

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.)

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.)

mSourceAlphaAvailable should be a bool

(In reply to Robert Longson from comment #144) > mSourceAlphaAvailable should be a bool Oops- thanks! Here's a tiny patch for review please.

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.

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.