Closed Bug 363909 Opened 16 years ago Closed 16 years ago

Implement ColorMatrix filter

Categories

(Core :: SVG, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: malex, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 4 obsolete files)

 
Attached patch First Draft (obsolete) — Splinter Review
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
Attached patch update to tip and fix issues (obsolete) — Splinter Review
(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 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?
Attached patch address review comment (obsolete) — Splinter Review
(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 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+
Attached patch address final review comment (obsolete) — Splinter Review
Attachment #254143 - Attachment is obsolete: true
Attachment #254416 - Flags: superreview?(roc)
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.
(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+
Attached patch update to tipSplinter Review
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
Closed: 16 years ago
Resolution: --- → FIXED
Blocks: 311029
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.