Implement ColorMatrix filter

RESOLVED FIXED

Status

()

RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: malex, Unassigned)

Tracking

(Blocks: 1 bug)

Trunk
x86
Windows XP
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

12 years ago
 
(Reporter)

Comment 1

12 years ago
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.
Saturate does not take percentage values. C.f. http://issues.apache.org/bugzilla/show_bug.cgi?id=36743#c3
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.
Attachment #248718 - Attachment is obsolete: true
Attachment #253763 - Flags: review?(tor)

Comment 4

12 years ago
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?
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.
Attachment #253763 - Attachment is obsolete: true
Attachment #254143 - Flags: review?(tor)
Attachment #253763 - Flags: review?(tor)

Comment 6

12 years ago
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.
Attachment #254143 - Flags: review?(tor) → review+
Created attachment 254416 [details] [diff] [review]
address final review comment
Attachment #254143 - Attachment is obsolete: true
Attachment #254416 - Flags: superreview?(roc)

Comment 8

12 years ago
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?
(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

12 years ago
(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.
Attachment #254416 - Flags: superreview?(roc) → superreview+
Created attachment 255758 [details] [diff] [review]
update to tip

integrate with bug 370556

Change one line to use use CopySourceImage rather than memcpy
Attachment #254416 - Attachment is obsolete: true
The update to tip patch addresses comment 8.

checked in.

Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Updated

12 years ago
Blocks: 311029

Comment 13

11 years ago
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)
(In reply to comment #13)
> 

Do raise a new bug for this please.
You need to log in before you can comment on or make changes to this bug.