363909 - Implement ColorMatrix filter

Status: RESOLVED FIXED

Description

[Alex Menzies]

Comment 1

[Alex Menzies]

Created attachment 248718 [details] [diff] [review]
First Draft

This patch provides a mostly complete implementation of the ColorMatrix filter.  Work remaining to be done includes:

* Add error checking to return NS_ERROR_FAILURE in case more the 20 values are specified.

* Add code to correctly parse the value parameter when it is given as a percentage when type="saturate".

* Filtering with type="matrix" behaves incorrectly if the last value parameter is not 0.  This is demonstrated in the test case.

Comment 2

[Robert Longson [:longsonr]]

Saturate does not take percentage values. C.f. http://issues.apache.org/bugzilla/show_bug.cgi?id=36743#c3

Comment 3

[Robert Longson [:longsonr]]

Created attachment 253763 [details] [diff] [review]
update to tip and fix issues

(In reply to comment #1)
> 
>  Work remaining to be done includes:
> 
> * Add error checking to return NS_ERROR_FAILURE in case more the 20 values are
> specified.

Implementation now insists on either no values attribute or exactly 20 values in order to work. Will not crash if more specified.

> 
> * Add code to correctly parse the value parameter when it is given as a
> percentage when type="saturate".

Not done. See comment 2.
http://www.w3.org/TR/SVG/images/filters/feColorMatrix.svg has an invalid saturate value.

> 
> * Filtering with type="matrix" behaves incorrectly if the last value parameter
> is not 0.  This is demonstrated in the test case.
> 

Implementation of filter processing changed to reflect linearRGB changes and ensure correct output if values attribute not specified. Not sure which test case Alex is referring to.

Comment 4

[tor]

Comment on attachment 253763 [details] [diff] [review]
update to tip and fix issues

One thing that calls attention to itself by the long initialization of an identity matrix is the handling of cases when "value" isn't specified.  Since all of those except for luminanceToAlpha are identity transforms, why not do a check for that and do a straight copy?

Comment 5

[Robert Longson [:longsonr]]

Created attachment 254143 [details] [diff] [review]
address review comment

(In reply to comment #4)
> (From update of attachment 253763 [details] [diff] [review])
> One thing that calls attention to itself by the long initialization of an
> identity matrix is the handling of cases when "value" isn't specified.  Since
> all of those except for luminanceToAlpha are identity transforms, why not do a
> check for that and do a straight copy?
> 

Done.

Comment 6

[tor]

Comment on attachment 254143 [details] [diff] [review]
address review comment

>+  switch (type) {
>+  case nsSVGFEColorMatrixElement::SVG_FECOLORMATRIX_TYPE_MATRIX:
>+
>+    if (HasAttr(kNameSpaceID_None, nsGkAtoms::values)) {
...
>+    } else {
>+      memcpy(targetData, sourceData, rect.height * stride);
>+      return NS_OK;
>+    }

Instead of duplicating this for three cases, I'd like something like this before the switch:

if (!HasAttr(kNameSpaceID_None, nsGkAtoms::values) &&
    (type == nsSVGFEColorMatrixElement::SVG_FECOLORMATRIX_TYPE_MATRIX ||
     type == nsSVGFEColorMatrixElement::SVG_FECOLORMATRIX_TYPE_SATURATE ||
     type == nsSVGFEColorMatrixElement::SVG_FECOLORMATRIX_TYPE_HUE_ROTATE)) {
  memcpy(targetData, sourceData, rect.height * stride);
  return NS_OK;
}

With that, r=tor.

Comment 7

[Robert Longson [:longsonr]]

Created attachment 254416 [details] [diff] [review]
address final review comment

Comment 8

[tor]

Comment on attachment 254416 [details] [diff] [review]
address final review comment

>+  if (!HasAttr(kNameSpaceID_None, nsGkAtoms::values) &&
>+      (type == nsSVGFEColorMatrixElement::SVG_FECOLORMATRIX_TYPE_MATRIX ||
>+       type == nsSVGFEColorMatrixElement::SVG_FECOLORMATRIX_TYPE_SATURATE ||
>+       type == nsSVGFEColorMatrixElement::SVG_FECOLORMATRIX_TYPE_HUE_ROTATE)) {
>+    // identity matrix filter
>+    memcpy(targetData, sourceData, rect.height * stride);
>+    return NS_OK;
>+  }

Something that occurred to me this morning - shouldn't this only be copying the data inside the filter element subregion?

Comment 9

[Robert Longson [:longsonr]]

(In reply to comment #8)
> (From update of attachment 254416 [details] [diff] [review])
> Something that occurred to me this morning - shouldn't this only be copying the
> data inside the filter element subregion?
> 

Even if I changed this, doesn't FixupTarget copy the data outside the subregion anyway afterwards?

Also this kind of memcpy occurs in nsSVGFEBlendElement::Filter, nsSVGFECompositeElement::Filter, nsSVGFEMorphologyElement::Filter (if rx == ry == 0). Are all those cases wrong?

I could write a CopySource method in nsSVGFilterResource and call that instead of all of those memcpy calls. I'd implement CopySource to only copy things in the subregion. Could do that here or as a follow up depending on your preference.

Comment 10

[tor]

(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 254416 [details] [diff] [review] [details])
> > Something that occurred to me this morning - shouldn't this only be copying the
> > data inside the filter element subregion?
> 
> Even if I changed this, doesn't FixupTarget copy the data outside the subregion
> anyway afterwards?

Yes, but we currently have an issue open with the SVG WG regarding whether we should be doing that copying in FixupTarget.  It's over a year old, but I've poked a WG member and it should be on their agenda soon.

  http://lists.w3.org/Archives/Public/www-svg/2006Jan/0432.html

> Also this kind of memcpy occurs in nsSVGFEBlendElement::Filter,
> nsSVGFECompositeElement::Filter, nsSVGFEMorphologyElement::Filter (if rx == ry
> == 0). Are all those cases wrong?

Err, yes - redundant if the WG decides that FixupTarget is right, wrong result if not.  Who reviewed that code?  Oh, right, me.  Oops.

> I could write a CopySource method in nsSVGFilterResource and call that instead
> of all of those memcpy calls. I'd implement CopySource to only copy things in
> the subregion. Could do that here or as a follow up depending on your
> preference.

Probably best as a followup.

Comment 11

[Robert Longson [:longsonr]]

Created attachment 255758 [details] [diff] [review]
update to tip

integrate with bug 370556

Change one line to use use CopySourceImage rather than memcpy

Comment 12

[Robert Longson [:longsonr]]

The update to tip patch addresses comment 8.

checked in.

Comment 13

[Drew Perttula]

http://lxr.mozilla.org/seamonkey/source/content/svg/content/src/nsSVGFilters.cpp#1346 looks wrong:

1346     colorMatrix[12] = 0.715f - 0.715f * s;

I think that should be  0.072+0.928s  according to http://www.w3.org/TR/SVG11/filters.html#feColorMatrix

The error result is that saturate with values="1" looks way yellow, and values="0" looks somewhat blue.

Here a screenshot of values="0" in minefield 3.0a9pre 2007100704:
http://bigasterisk.com/post/bugzilla363909-compare.jpg
(The lower image is in batik and looks right to me)

Comment 14

[Robert Longson [:longsonr]]

(In reply to comment #13)
> 

Do raise a new bug for this please.

Comment 15

[Drew Perttula]

Opened at https://bugzilla.mozilla.org/show_bug.cgi?id=399133