Shumway, a SWF runtime written in JavaScript, needs to implement the same filter effects as available in the Flash Player. Most of them are already available and used in our gfx backends. Would be really useful for Shumway and other Canvas programs to have them exposed as Canvas API's.

Jet says that Markus is working on this bug.

Markus: can you hack up a prototype that lets us send SVG filters to Canvas drawing operations? We're not sure how the syntax should look, but a simple way might be to start with SVG strings as input that we can chain into a filter stack.

That's what I had in mind, too: Simple expose a canvasRenderingContext2D.filter property that takes a string with what you'd assign to the CSS filter property. Joining multiple filters with spaces already builds a correct filter chain, where each item in the chain can either be a CSS filter shorthand or a url(#svgFilter) reference where you have a <filter id="svgFilter"> somewhere in your document that contains a filter graph. I have some patches lying around that got very far along that path. I'll update them and attach them here.

There is a thread at WHAT WG mailing list about activating filters for Canvas. Markus, maybe you could respond with your ideas http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2014-March/253988.html If you are interested on working on filters in Canvas, it might be a way to get the ball rolling.

(In reply to Dirk Schulze from comment #4) > If you are interested on working on filters in Canvas, it might be a way to > get the ball rolling. The way to get the ball rolling is to hack it up and see if it's going to fly. We may just ship polyfills like bug 1047134 if the rendering pipeline isn't going to be quick enough. We can bikeshed syntax too, but I'd want to first know that the rendering can keep up with our use cases.

I'll post to the WHAT WG mailing list soon, now that I've found probably all the edge cases that need to be defined. I have a stack of about 15 patches for this. I still need to clean them up, fix a leak, and write some tests.

(In reply to Markus Stange [:mstange] from comment #6) > I'll post to the WHAT WG mailing list soon, now that I've found probably all > the edge cases that need to be defined. I *think* that in some cases (solid fills, gradients) the ColorTransform filter can be applied to the stroke and fill style directly and not on each pixel. Do you know if this has been discussed on the WHAT WG mailing list? If not, I can try and propose that. In any case, it's orthogonal to the work here. > I have a stack of about 15 patches for this. I still need to clean them up, > fix a leak, and write some tests. Do you have a patch that I can experiment with?

(In reply to Michael Bebenita [:mbx] from comment #9) > Do you have a patch that I can experiment with? Sure. With this patch, for color transforms, you need to put an SVG <filter> element with a <feColorMatrix> element that implements the color transform you want into the document, and tell the canvas to use it with ctx.filter = "url(#yourFilter)". The CSS filter shorthands are not implemented yet (except for blur, which can be enabled with layout.css.filters.enabled). > (In reply to Markus Stange [:mstange] from comment #6) > I *think* that in some cases (solid fills, gradients) the ColorTransform > filter can be applied to the stroke and fill style directly and not on each > pixel. This sounds like a worthwhile optimization that's up to the browser to implement, so it may not need to be standardized.

(In reply to Markus Stange [:mstange] from comment #10) > Created attachment 8471792 [details] [diff] [review] > wip > > (In reply to Michael Bebenita [:mbx] from comment #9) > > Do you have a patch that I can experiment with? > > Sure. With this patch, for color transforms, you need to put an SVG <filter> > element with a <feColorMatrix> element that implements the color transform > you want into the document, and tell the canvas to use it with ctx.filter = > "url(#yourFilter)". The CSS filter shorthands are not implemented yet > (except for blur, which can be enabled with layout.css.filters.enabled). > > > (In reply to Markus Stange [:mstange] from comment #6) > > I *think* that in some cases (solid fills, gradients) the ColorTransform > > filter can be applied to the stroke and fill style directly and not on each > > pixel. > > This sounds like a worthwhile optimization that's up to the browser to > implement, so it may not need to be standardized. I'm not sure it is possible to implement this optimization if the specification dictates that color transforms are applied on pixels. The Flash API specifies both ColorTransforms and ColorMatrixFilters. I believe that color transforms are concatenated together before they are applied while the color matrix filters operate on rasterized images. Implementing ColorTransforms in Canvas2D would look more like: context.save(); context.transformColor(a, b, c, d, e, ....); // or context.setColorTransform(a, b, c, d, e, ....); context.restore(); while, ColorMatrixFilters would be covered by Canvas SVG filter.

Oh, I see. Then a new API sounds like the way to go. On the other hand, only transforming a single color is a much faster operation, so there wouldn't be much of a performance difference between doing the transformation in Shumway JS vs in the platform. Right?

In principle yes, but polyfilling it is kind of a mess. The polyfill needs to parse stroke and fill styles: CSS colors, gradients, pattern styles. CanvasGradients for instance don't reflect their attributes so the polyfill needs to record these and create new CanvasGradient objects for every stroke or fill operation. Having this in the platform would be much nicer.

Ok. Can you please file a new bug for ColorTransform support and include some examples where the result would look different than with a per-pixel transform?

Status update: I'm currently working on a crash that my patches cause under certain circumstances. When that's fixed I need to regroup and document the patches and submit them for review.

(In reply to Markus Stange [:mstange] from comment #15) > Status update: I'm currently working on a crash that my patches cause under > certain circumstances. When that's fixed I need to regroup and document the > patches and submit them for review. Could you go a bit more into detail how filter regions and primitive subregions are resolved for SVG Filters on Canvas please? So far it don't even know what gets filtered or how filters work. Is it following the various proposals on WHAT WG and every of the next drawing operations will be filtered in isolation?

(In reply to Dirk Schulze from comment #16) > Could you go a bit more into detail how filter regions and primitive > subregions are resolved for SVG Filters on Canvas please? So far it don't > even know what gets filtered or how filters work. Is it following the > various proposals on WHAT WG and every of the next drawing operations will > be filtered in isolation? I will. In any case, even if the WG agrees on different behavior than what I'm implementing here, it will be much easier to adjust to whatever we agree on after my patches have landed, since most of the work I did is related to generalizing the code that deals with units and coordinate spaces, and with making filters usable on elements that are not attached to the document and don't have an nsIFrame. What I'm implementing is following the proposal on the WHAT WG list. Filters are implemented like the canvas shadow feature: filters are applied to every drawing operation in isolation, coordinates are in canvas pixels and don't obey the current transform on the canvas context, and the filter region is defined to be the whole canvas regardless of the size of whatever the current drawing operation draws. User space is defined as the canvas, too, so 0,0 in user space is the top left of the canvas and one user space unit is one canvas pixel, regardless of the final rendered size and location of the canvas. For the CSS drop-shadow function, 1 CSS px is one canvas pixel, too. I'm also making the FillPaint and StrokePaint filter sources work as expected, i.e. a canvas-sized surface that's filled with the context's current strokeStyle / fillStyle. As far as error handling goes, at the moment all error cases just fail silently. That includes setting a bad filter property value, and referencing filters that don't exist or are in external documents that haven't loaded yet. (I think there are more but I don't recall them at the moment.) I haven't implemented the BackgroundImage/BackgroundAlpha filter sources yet, but I think they should refer to the current contents of the canvas.

This is a new feature, not a regression, so it does not need to block Shumway's M2 milestone.

We need this to support the global canvas composition operator during filter drawing.

This is strongly inspired by the existing font parsing code in the same file. How to deal with unparsable values, inherit, and em values are questions that I can't really answer yet, and they probably need some discussion on the list.

When AdjustedTarget also does filter drawing, its performance benefits from accurate source bounds.

Comment on attachment 8488210 [details] [diff] [review] part 8: Canvas filter drawing support Review of attachment 8488210 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/canvas/CanvasRenderingContext2D.cpp @@ +388,5 @@ > + > + Matrix transform = mFinalTarget->GetTransform(); > + > + mgfx::Point offset(mPostFilterBounds.TopLeft() - mOffset); > + mFinalTarget->SetTransform(Matrix::Translation(offset)); Can't we include this offset on the final DrawFilter call? Changing the translation is expensive for HWA DrawTargets..

We still need to reset the transform to identity, so it probably doesn't change much. You can decide whether this patch improves things. :-)

using AutoSaveTransform

Comment on attachment 8488210 [details] [diff] [review] part 8: Canvas filter drawing support Review of attachment 8488210 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/canvas/CanvasRenderingContext2D.cpp @@ +332,5 @@ > + mTarget = > + mFinalTarget->CreateSimilarDrawTarget(mSourceGraphicRect.Size(), SurfaceFormat::B8G8R8A8); > + > + if (!mTarget) { > + // XXX - Deal with the situation where our temp size is too big to File a bug please :-) @@ +342,5 @@ > + } > + > + Matrix transform = mFinalTarget->GetTransform(); > + transform.PostTranslate(-mSourceGraphicRect.TopLeft() + mOffset); > + mTarget->SetTransform(transform); mTarget->SetTransform(mFinalTarget->GetTransform().PostTranslate(etc.)); That's possible now that JWatt's altered things a bit :) @@ +362,5 @@ > + } > + > + Matrix transform = mFinalTarget->GetTransform(); > + transform.PostTranslate(-aRect.TopLeft() + mOffset); > + dt->SetTransform(transform); ditto

Comment on attachment 8488189 [details] [diff] [review] part 2: Add the CanvasRenderingContext2D.filter property, preffed off by default under canvas.filters.enabled. Review of attachment 8488189 [details] [diff] [review]: ----------------------------------------------------------------- Needs DOM peer review.

Comment on attachment 8488194 [details] [diff] [review] part 3: Add parsing support for canvas filters. Review of attachment 8488194 [details] [diff] [review]: ----------------------------------------------------------------- This needs someone more familiar with the style system... ::: dom/canvas/CanvasRenderingContext2D.cpp @@ +1632,5 @@ > + // to include the outer window ID. > + nsCSSParser parser(document->CSSLoader()); > + > + nsresult rv = parser.ParseStyleAttribute(EmptyString(), docURL, baseURL, > + principal, getter_AddRefs(rule)); I'm not sure this is the best way to do this... Seems a bit hacky @@ +1721,5 @@ > { > CurrentState().filterString = filter; > + > + nsTArray<nsStyleFilter>& filterChain = CurrentState().filterChain; > + error = ParseFilter(filter, filterChain); Pass 'error' as a parameter like we do elsewhere.

Comment on attachment 8488204 [details] [diff] [review] part 4: Resolve the parsed canvas filter to a FilterDescription. Review of attachment 8488204 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/canvas/CanvasRenderingContext2D.cpp @@ +1740,5 @@ > + > + virtual float GetEmLength() const MOZ_OVERRIDE > + { return 10.0f; } > + virtual float GetExLength() const MOZ_OVERRIDE > + { return 10.0f; } Can we move the magic "10px" value somewhere shared to ensure it stays in sync with the canvas default font size? ::: dom/canvas/CanvasRenderingContext2D.h @@ +939,5 @@ > lineCap(other.lineCap), > lineJoin(other.lineJoin), > filterString(other.filterString), > filterChain(other.filterChain), > + filter(other.filter), Do we actually need to keep filterChain around now?

Comment on attachment 8488189 [details] [diff] [review] part 2: Add the CanvasRenderingContext2D.filter property, preffed off by default under canvas.filters.enabled. (and let's not set the pref true before we have some kind of spec.)

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #34) > ::: dom/canvas/CanvasRenderingContext2D.h > @@ +939,5 @@ > > lineCap(other.lineCap), > > lineJoin(other.lineJoin), > > filterString(other.filterString), > > filterChain(other.filterChain), > > + filter(other.filter), > > Do we actually need to keep filterChain around now? filterChain is what we pass to nsFilterInstance::GetFilterDescription in UpdateFilter. We could of course parse the filter anew from filterString when that happens. Would you prefer that? Alternatively, we could revert attachment 8484114 [details] [diff] [review] and get filterChain from filterChainObserver.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #34) > Comment on attachment 8488204 [details] [diff] [review] > part 4: Resolve the parsed canvas filter to a FilterDescription. > > Review of attachment 8488204 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/canvas/CanvasRenderingContext2D.cpp > @@ +1740,5 @@ > > + > > + virtual float GetEmLength() const MOZ_OVERRIDE > > + { return 10.0f; } > > + virtual float GetExLength() const MOZ_OVERRIDE > > + { return 10.0f; } > > Can we move the magic "10px" value somewhere shared to ensure it stays in > sync with the canvas default font size? Something like this?

(In reply to Markus Stange [:mstange] from comment #36) > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #34) > > Do we actually need to keep filterChain around now? > > filterChain is what we pass to nsFilterInstance::GetFilterDescription in > UpdateFilter. We could of course parse the filter anew from filterString > when that happens. Would you prefer that? > > Alternatively, we could revert attachment 8484114 [details] [diff] [review] > and get filterChain from filterChainObserver. It's OK, just leave it as-is.

Comment on attachment 8488730 [details] [diff] [review] part 10: Put the default font size in a shared place. Review of attachment 8488730 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/canvas/CanvasRenderingContext2D.cpp @@ +1906,5 @@ > CanvasRenderingContext2D *mContext; > }; > > +#define DEFAULT_FONT_SIZE 10 > +#define DEFAULT_FONT_STYLE "10px sans-serif" #define DEFAULT_FONT_STYLE DEFAULT_FONT_SIZE "px sans-serif"

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #39) > Comment on attachment 8488730 [details] [diff] [review] > part 10: Put the default font size in a shared place. > > Review of attachment 8488730 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/canvas/CanvasRenderingContext2D.cpp > @@ +1906,5 @@ > > CanvasRenderingContext2D *mContext; > > }; > > > > +#define DEFAULT_FONT_SIZE 10 > > +#define DEFAULT_FONT_STYLE "10px sans-serif" > > #define DEFAULT_FONT_STYLE DEFAULT_FONT_SIZE "px sans-serif" I'd really like to do this but unfortunately it doesn't work when DEFAULT_FONT_STYLE is used inside NS_NAMED_LITERAL_STRING, because that uses macro magic to create an utf-16 string literal by prepending a u: u"10px sans serif" makes sense, u10 "px sans serif" does not. Here's the error message: > /Users/mstange/code/mozilla/dom/canvas/CanvasRenderingContext2D.cpp:1944:3: error: use of undeclared identifier 'u10' > NS_NAMED_LITERAL_STRING(fontStyle, DEFAULT_FONT_STYLE); > ^ > ../../dist/include/nsLiteralString.h:31:75: note: expanded from macro 'NS_NAMED_LITERAL_STRING' > #define NS_NAMED_LITERAL_STRING(n,s) const nsLiteralString n(MOZ_UTF16(s)) > ^ > ../../dist/include/mozilla/Char16.h:183:22: note: expanded from macro 'MOZ_UTF16' > #define MOZ_UTF16(s) MOZ_UTF16_HELPER(s) > ^ > ../../dist/include/mozilla/Char16.h:39:31: note: expanded from macro 'MOZ_UTF16_HELPER' > # define MOZ_UTF16_HELPER(s) u##s > ^ > <scratch space>:129:1: note: expanded from here > u10 > ^ I'll keep it the way it is.

I've addressed roc's comment about the error result, and added a comment that explains the behavior and the fact that it's probably not the final state. This is the last patch that needs to be reviewed before I can close this bug.

Comment on attachment 8491046 [details] [diff] [review] part 3: Add parsing support for canvas filters. Review of attachment 8491046 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the review delay. ::: dom/canvas/CanvasRenderingContext2D.cpp @@ +1632,5 @@ > + // to include the outer window ID. > + nsCSSParser parser(document->CSSLoader()); > + > + nsresult rv = parser.ParseStyleAttribute(EmptyString(), docURL, baseURL, > + principal, getter_AddRefs(rule)); I agree with roc that this is super hacky, but we're already doing it in CreateFontStyleRule. Can you please factor out as much of CreateFontStyleRule and CreateFilterStyleRule into a common function as you can, though? @@ +1645,5 @@ > + return rv; > + > + // Set a font style so that em values in the filter value have a defined > + // font size to be relative to. > + NS_NAMED_LITERAL_STRING(fontStyle, "10px sans-serif"); Shouldn't em units in the filter resolve against the font-size used on the <canvas>? And rem units should do something sensible too. So I think we should be doing something similar to CanvasRenderingContext2D::SetFont where we resolve the style of the <canvas> and inherit our styles from that. @@ +1712,5 @@ > + return nsTArray<nsStyleFilter>(); > + } > + > + const nsStyleSVGReset* svgStyle = sc->StyleSVGReset(); > + NS_ASSERTION(svgStyle, "Could not obtain SVG style"); No need to assert; StyleSVGReset will never return a null pointer. (The existing assertion in CanvasRenderingContext2D::SetFont isn't needed either.) @@ +1714,5 @@ > + > + const nsStyleSVGReset* svgStyle = sc->StyleSVGReset(); > + NS_ASSERTION(svgStyle, "Could not obtain SVG style"); > + > + return nsTArray<nsStyleFilter>(svgStyle->mFilters); This can just be |return svgStyle->mFilters;| I think. But will the multiple return statements of (temporary) nsTArrays defeat return value optimisation here? Might be safer just to make the nsTArray a reference argument to ParseFilter. @@ +1728,5 @@ > + // specced. Note that SetFont silently refuses to change the state for > + // invalid values. It can distinguish invalid values from inherit/initial > + // due to the effect the font shorthand has on font-size-adjust. The filter > + // property is not a shorthand, so we can't use a similar trick here, so it > + // looks like implementing the same behavior is more tricky for filter. You should be able to look at the value of |changed| to see if ParseProperty added a filter property. (And if that doesn't work, inspect the StyleRule object to see if there is a filter property after ParseProperty.) I think we should follow the same pattern as assigning to font.

Michael is working on ColorTransform polyfill for Shumway in bug 1047134, but it will be slow. This is a very common operation for Flash content. Proposed ColorTransform API needs to be spec'd before implementation.

Thanks for the very thorough review, Cameron! I agree with you on all points, and this patch should address your comments.

Comment on attachment 8492135 [details] [diff] [review] part 3: Add parsing support for canvas filters. Review of attachment 8492135 [details] [diff] [review]: ----------------------------------------------------------------- r=me with these comments addressed. ::: dom/canvas/CanvasRenderingContext2D.cpp @@ +1705,5 @@ > + // GetNormalBlock(). > + const nsCSSValue* filterVal = > + declaration->GetNormalBlock()->ValueFor(aProperty); > + return (!filterVal || (filterVal->GetUnit() == eCSSUnit_Inherit || > + filterVal->GetUnit() == eCSSUnit_Initial)); You should check for eCSSUnit_Unset as well. @@ +1754,5 @@ > + > + nsStyleSet* styleSet = presShell->StyleSet(); > + nsRefPtr<nsStyleContext> sc = > + styleSet->ResolveStyleForRules(parentContext, rules); > + if (!sc) { Looks like ResolveStyleForRules (through its call to nsStyleSet::GetContext) will never return null, so you can leave out this null check. @@ +1835,5 @@ > + } > + > + nsString usedFont; > + nsRefPtr<nsStyleContext> parentContext = > + GetFontStyleContext(mCanvasElement, GetFont(), Hmm, so here is a question. Should the font size relative units in the filter be resolved against the font size that has been assigned to the .font property of the CanvasRenderingContext2D or against the value of font-size on the <canvas> element? If it's the latter, then you should be using GetFontParentStyleContext instead. Something to send in to Ian or whoever is speccing .filter. Happy to go with what you have now if you think that makes sense. Can you also please add a test for font size relative units in a filter string?

(In reply to Cameron McCormack (:heycam) from comment #49) > @@ +1835,5 @@ > > + } > > + > > + nsString usedFont; > > + nsRefPtr<nsStyleContext> parentContext = > > + GetFontStyleContext(mCanvasElement, GetFont(), > > Hmm, so here is a question. Should the font size relative units in the > filter be resolved against the font size that has been assigned to the .font > property of the CanvasRenderingContext2D or against the value of font-size > on the <canvas> element? If it's the latter, then you should be using > GetFontParentStyleContext instead. Something to send in to Ian or whoever > is speccing .filter. Happy to go with what you have now if you think that > makes sense. I think it makes sense but I'll bring it up on the list. > Can you also please add a test for font size relative units in a filter > string? I'll do that when I write the rest of the tests. Or really, any tests at all.

https://hg.mozilla.org/integration/mozilla-inbound/rev/575915a27e3b https://hg.mozilla.org/integration/mozilla-inbound/rev/51f528191abd https://hg.mozilla.org/integration/mozilla-inbound/rev/e91710f9fa78 https://hg.mozilla.org/integration/mozilla-inbound/rev/4c8652171322 https://hg.mozilla.org/integration/mozilla-inbound/rev/47feffebaf8d https://hg.mozilla.org/integration/mozilla-inbound/rev/24a838f90b5c https://hg.mozilla.org/integration/mozilla-inbound/rev/ef9e17516cbc https://hg.mozilla.org/integration/mozilla-inbound/rev/e633da2eea48 https://hg.mozilla.org/integration/mozilla-inbound/rev/6a2f28a3e110 https://hg.mozilla.org/integration/mozilla-inbound/rev/88ed005fa742

https://hg.mozilla.org/mozilla-central/rev/575915a27e3b https://hg.mozilla.org/mozilla-central/rev/51f528191abd https://hg.mozilla.org/mozilla-central/rev/e91710f9fa78 https://hg.mozilla.org/mozilla-central/rev/4c8652171322 https://hg.mozilla.org/mozilla-central/rev/47feffebaf8d https://hg.mozilla.org/mozilla-central/rev/24a838f90b5c https://hg.mozilla.org/mozilla-central/rev/ef9e17516cbc https://hg.mozilla.org/mozilla-central/rev/e633da2eea48 https://hg.mozilla.org/mozilla-central/rev/6a2f28a3e110 https://hg.mozilla.org/mozilla-central/rev/88ed005fa742

Unless I missed something, there is no spec for this yet – who will drive this?

I don't really know, but I've now posted my thoughts to the WhatWG list: http://lists.w3.org/Archives/Public/public-whatwg-archive/2014Sep/0110.html

Reference: https://developer.mozilla.org/en-US/docs/Web/API/CanvasRenderingContext2D.filter Fx 35 for devs: https://developer.mozilla.org/en-US/Firefox/Releases/35#Interfaces.2FAPIs.2FDOM Reviews appreciated. Removing colorTransform from the summary, as I think it didn't happen.

Unit and reference tests covering edge cases outlined in https://lists.w3.org/Archives/Public/public-whatwg-archive/2014Sep/0112.html.