Exposing the CSS/SVG Filters as Canvas APIs

RESOLVED FIXED in mozilla35

Status

()

Core
Canvas: 2D
RESOLVED FIXED
4 years ago
4 months ago

People

(Reporter: tobytailor, Assigned: mstange)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

unspecified
mozilla35
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [shumway:m2][DocArea=Canvas])

Attachments

(12 attachments, 11 obsolete attachments)

2.78 KB, text/html
Details
230.08 KB, image/png
Details
3.23 KB, patch
roc
: review+
Details | Diff | Splinter Review
4.69 KB, patch
roc
: review+
smaug
: review+
Details | Diff | Splinter Review
7.03 KB, patch
bas
: review+
Details | Diff | Splinter Review
5.15 KB, patch
bas
: review+
Details | Diff | Splinter Review
6.15 KB, patch
Details | Diff | Splinter Review
7.77 KB, patch
Details | Diff | Splinter Review
10.64 KB, patch
Details | Diff | Splinter Review
10.82 KB, patch
Details | Diff | Splinter Review
16.46 KB, patch
Details | Diff | Splinter Review
5.28 KB, patch
roc
: review+
Details | Diff | Splinter Review
(Reporter)

Description

4 years ago
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.
(Reporter)

Updated

4 years ago
Blocks: 927828
Jet says that Markus is working on this bug.
Assignee: nobody → mstange
Whiteboard: [shumway:m2]

Updated

3 years ago
Depends on: 948265
Blocks: 1038057

Comment 2

3 years ago
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.
Flags: needinfo?(mstange)
(Assignee)

Comment 3

3 years ago
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.
Flags: needinfo?(mstange)
Blocks: 1044759
No longer blocks: 1038057
Summary: Exposing the CSS/SVG Filters as Canvas API's → Exposing the CSS/SVG Filters as Canvas APIs, including ColorTransform
Depends on: 1047134

Comment 4

3 years ago
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.
Flags: needinfo?(mstange)

Comment 5

3 years ago
(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.
(Assignee)

Comment 6

3 years ago
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.
Flags: needinfo?(mstange)
(Assignee)

Comment 7

3 years ago
Created attachment 8471619 [details]
testcase
(Assignee)

Comment 8

3 years ago
Created attachment 8471621 [details]
expected rendering of testcase
(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?
(Assignee)

Comment 10

3 years ago
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.
(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.
(Assignee)

Comment 12

3 years ago
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.
(Assignee)

Comment 14

3 years ago
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?
(Assignee)

Comment 15

3 years ago
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.

Comment 16

3 years ago
(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?
(Assignee)

Comment 17

3 years ago
(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.
Blocks: 1038057
No longer blocks: 1044759
(Assignee)

Updated

3 years ago
Depends on: 1062832
(Assignee)

Updated

3 years ago
Depends on: 1065344
(Assignee)

Updated

3 years ago
Depends on: 1066270
(Assignee)

Comment 19

3 years ago
Created attachment 8488187 [details] [diff] [review]
part 1: Add a DrawOptions argument to FilterSupport::RenderFilterDescription.

We need this to support the global canvas composition operator during filter drawing.
Attachment #8471792 - Attachment is obsolete: true
Attachment #8488187 - Flags: review?(roc)
(Assignee)

Comment 20

3 years ago
Created attachment 8488189 [details] [diff] [review]
part 2: Add the CanvasRenderingContext2D.filter property, preffed off by default under canvas.filters.enabled.
Attachment #8488189 - Flags: review?(roc)
(Assignee)

Comment 21

3 years ago
Created attachment 8488194 [details] [diff] [review]
part 3: Add parsing support for canvas filters.

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.
Attachment #8488194 - Flags: review?(roc)
(Assignee)

Comment 22

3 years ago
Created attachment 8488204 [details] [diff] [review]
part 4: Resolve the parsed canvas filter to a FilterDescription.
Attachment #8488204 - Flags: review?(roc)
(Assignee)

Comment 23

3 years ago
Created attachment 8488206 [details] [diff] [review]
part 5: Keep the filter description up-to-date when something in the referenced filter changes.
Attachment #8488206 - Flags: review?(roc)
(Assignee)

Comment 24

3 years ago
Created attachment 8488208 [details] [diff] [review]
part 6: Also calculate bounds for canvas operations when a filter is used.

When AdjustedTarget also does filter drawing, its performance benefits from accurate source bounds.
Attachment #8488208 - Flags: review?(bas)
(Assignee)

Comment 25

3 years ago
Created attachment 8488209 [details] [diff] [review]
part 7: Split shadow drawing out of AdjustedTarget.
Attachment #8488209 - Flags: review?(bas)
(Assignee)

Comment 26

3 years ago
Created attachment 8488210 [details] [diff] [review]
part 8: Canvas filter drawing support
Attachment #8488210 - Flags: review?(bas)
(Assignee)

Comment 27

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=acff72957f45
Attachment #8488208 - Flags: review?(bas) → review+
Attachment #8488209 - Flags: review?(bas) → review+
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..
(Assignee)

Comment 29

3 years ago
Created attachment 8488226 [details] [diff] [review]
part 9: Use aDestPoint in dt->DrawFilter instead of a translation

We still need to reset the transform to identity, so it probably doesn't change much. You can decide whether this patch improves things. :-)
Attachment #8488226 - Flags: review?(bas)
(Assignee)

Comment 30

3 years ago
Created attachment 8488248 [details] [diff] [review]
part 9: Use aDestPoint in dt->DrawFilter instead of a translation

using AutoSaveTransform
Attachment #8488226 - Attachment is obsolete: true
Attachment #8488226 - Flags: review?(bas)
Attachment #8488248 - Flags: review?(bas)
Attachment #8488248 - Flags: review?(bas) → review+
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
Attachment #8488210 - Flags: review?(bas) → review+
Attachment #8488187 - Flags: review?(roc) → review+
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.
Attachment #8488189 - Flags: review?(roc)
Attachment #8488189 - Flags: review?(bugs)
Attachment #8488189 - Flags: 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.
Attachment #8488194 - Flags: review?(roc) → review?(cam)
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?
Attachment #8488204 - Flags: review?(roc) → review-
Attachment #8488206 - Flags: review?(roc) → review+
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.)
Attachment #8488189 - Flags: review?(bugs) → review+
(Assignee)

Comment 36

3 years ago
(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.
(Assignee)

Comment 37

3 years ago
Created attachment 8488730 [details] [diff] [review]
part 10: Put the default font size in a shared place.

(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?
Attachment #8488730 - Flags: review?(roc)
(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"
Attachment #8488730 - Flags: review?(roc) → review+
(Assignee)

Comment 40

3 years ago
(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.
(Assignee)

Comment 41

3 years ago
Created attachment 8491046 [details] [diff] [review]
part 3: Add parsing support for canvas filters.

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.
Attachment #8488194 - Attachment is obsolete: true
Attachment #8488194 - Flags: review?(cam)
Attachment #8491046 - Flags: review?(cam)
(Assignee)

Comment 42

3 years ago
Created attachment 8491049 [details] [diff] [review]
part 4: Resolve the parsed canvas filter to a FilterDescription. r=roc
Attachment #8488204 - Attachment is obsolete: true
(Assignee)

Comment 43

3 years ago
Created attachment 8491050 [details] [diff] [review]
part 5: Keep the filter description up-to-date when something in the referenced filter changes. r=roc
Attachment #8488206 - Attachment is obsolete: true
(Assignee)

Comment 44

3 years ago
Created attachment 8491053 [details] [diff] [review]
part 7: Split shadow drawing out of AdjustedTarget. r=Bas
Attachment #8488209 - Attachment is obsolete: true
(Assignee)

Comment 45

3 years ago
Created attachment 8491054 [details] [diff] [review]
part 8: Canvas filter drawing support. r=Bas
Attachment #8488210 - Attachment is obsolete: true
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.
Attachment #8491046 - Flags: review?(cam) → review-
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.
(Assignee)

Comment 48

3 years ago
Created attachment 8492135 [details] [diff] [review]
part 3: Add parsing support for canvas filters.

Thanks for the very thorough review, Cameron! I agree with you on all points, and this patch should address your comments.
Attachment #8491046 - Attachment is obsolete: true
Attachment #8492135 - Flags: review?(cam)
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?
Attachment #8492135 - Flags: review?(cam) → review+
(Assignee)

Comment 50

3 years ago
(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.
Status: NEW → ASSIGNED
(Assignee)

Comment 51

3 years ago
Created attachment 8494043 [details] [diff] [review]
part 3: Add parsing support for canvas filters. r=heycam
Attachment #8492135 - Attachment is obsolete: true
(Assignee)

Comment 52

3 years ago
Created attachment 8494045 [details] [diff] [review]
part 10: Correctly handle em/ex units in SVG filter primitive region length values.
Attachment #8488730 - Attachment is obsolete: true
Attachment #8494045 - Flags: review?(roc)
Attachment #8494045 - Flags: review?(roc) → review+
(Assignee)

Comment 53

3 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Keywords: dev-doc-needed

Comment 55

3 years ago
Unless I missed something, there is no spec for this yet – who will drive this?
(Assignee)

Comment 56

3 years ago
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
Whiteboard: [shumway:m2] → [shumway:m2][DocArea=Canvas]
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.
Keywords: dev-doc-needed → dev-doc-complete
Summary: Exposing the CSS/SVG Filters as Canvas APIs, including ColorTransform → Exposing the CSS/SVG Filters as Canvas APIs

Updated

2 years ago
Blocks: 1152918
(Reporter)

Updated

2 years ago
Depends on: 1163105, 1163107
(Reporter)

Comment 58

2 years ago
Created attachment 8603515 [details] [diff] [review]
tests

Unit and reference tests covering edge cases outlined in https://lists.w3.org/Archives/Public/public-whatwg-archive/2014Sep/0112.html.
Attachment #8603515 - Flags: review?(mstange)
(Reporter)

Updated

2 years ago
Depends on: 1163124

Updated

2 years ago
Depends on: 1164766
(Assignee)

Comment 59

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=986008b8fe45
(Reporter)

Updated

2 years ago
Attachment #8603515 - Flags: review?(mstange)
(Reporter)

Updated

2 years ago
Depends on: 1173544
(Reporter)

Updated

2 years ago
Attachment #8603515 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Blocks: 1173545
(Assignee)

Updated

2 years ago
No longer depends on: 1163124
(Assignee)

Updated

2 years ago
No longer depends on: 1163105
(Assignee)

Comment 60

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f5077249a362
See Also: → bug 1318283
You need to log in before you can comment on or make changes to this bug.