Closed
Bug 363909
Opened 16 years ago
Closed 16 years ago
Implement ColorMatrix filter
Categories
(Core :: SVG, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: malex, Unassigned)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 4 obsolete files)
22.45 KB,
patch
|
Details | Diff | Splinter Review |
Reporter | ||
Comment 1•16 years ago
|
||
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•16 years ago
|
||
Saturate does not take percentage values. C.f. http://issues.apache.org/bugzilla/show_bug.cgi?id=36743#c3
Comment 3•16 years ago
|
||
(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?
Comment 5•16 years ago
|
||
(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+
Comment 7•16 years ago
|
||
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?
Comment 9•16 years ago
|
||
(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•16 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+
Comment 11•16 years ago
|
||
integrate with bug 370556 Change one line to use use CopySourceImage rather than memcpy
Attachment #254416 -
Attachment is obsolete: true
Comment 12•16 years ago
|
||
The update to tip patch addresses comment 8. checked in.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Comment 13•15 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)
Comment 14•15 years ago
|
||
(In reply to comment #13) > Do raise a new bug for this please.
Comment 15•15 years ago
|
||
Opened at https://bugzilla.mozilla.org/show_bug.cgi?id=399133
You need to log in
before you can comment on or make changes to this bug.
Description
•