Bug 67503

Created attachment 106151 [details] Patch

Created attachment 106152 [details] Demonstration of motion jitter. The attachment shows a rotating blurred circle. A strong degree of motion jitter is evident. Applying the proposed patch removes this jitter. [A rotating, non-blurred circle and a blurred, non-rotating circle are shown for comparison, as well as a rotating, non-circular object.] However, this patch changes the output for svg/dynamic-updates/SVGFEImageElement-dom-preserveAspectRatio-attr.html Looking at the source for this test, I'm not sure I agree with the expected output, but I am unsure if my back-of-the-envelope calculation is correct. I'd appreciate if someone can check me on this. The patch also produces a shifted image with aspect ratio preserved, but the amount of vertical shift is different. If the expected output is correct, I'd like some guidance on how to plumb the shift incurred by xM??YMin to apply it to the source graphic, but not the overall filter.

Oh, no layout test yet, but will provide one before final review. Where do I clear the "review?" flag?

(In reply to comment #2) > Created an attachment (id=106152) [details] > Demonstration of motion jitter. > > The attachment shows a rotating blurred circle. A strong degree of motion jitter is evident. Applying the proposed patch removes this jitter. [A rotating, non-blurred circle and a blurred, non-rotating circle are shown for comparison, as well as a rotating, non-circular object.] > > However, this patch changes the output for svg/dynamic-updates/SVGFEImageElement-dom-preserveAspectRatio-attr.html > > Looking at the source for this test, I'm not sure I agree with the expected output, but I am unsure if my back-of-the-envelope calculation is correct. I'd appreciate if someone can check me on this. The patch also produces a shifted image with aspect ratio preserved, but the amount of vertical shift is different. > > If the expected output is correct, I'd like some guidance on how to plumb the shift incurred by xM??YMin to apply it to the source graphic, but not the overall filter. I'm not sure how your output looks like, but the current output in trunk is indeed wrong. Can you compare the results with other SVG viewers? It might be interesting as well how http://dev.w3.org/SVG/profiles/1.1F2/test/svg/filters-image-05-f.svg looks like with your patch. It is wrong as well right now. Can you explain why you ignore the translation of the matrix? What is wrong with the current code (why is it wrong)?

Comment on attachment 106151 [details] Patch Attachment 106151 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9578924 New failing tests: svg/dynamic-updates/SVGFEImageElement-svgdom-preserveAspectRatio-prop.html svg/W3C-SVG-1.1/filters-morph-01-f.svg svg/dynamic-updates/SVGFEImageElement-dom-preserveAspectRatio-attr.html svg/batik/text/textFeatures.svg svg/filters/filterRes.svg

(In reply to comment #4) > > I'm not sure how your output looks like, but the current output in trunk is indeed wrong. Can you compare the results with other SVG viewers? I'll see if I can reconstruct the SVG for the final state of the test, and try it elsewhere (since the test is dynamic, and requires a test harness, other viewers won't run the dom update portion ...). > It might be interesting as well how http://dev.w3.org/SVG/profiles/1.1F2/test/svg/filters-image-05-f.svg looks like with your patch. It is wrong as well right now. > As you can see from the new failing tests in comment #5, it still seems to pass. The other new failures (excluding the aspect ratio ones), have pixel diffs too small to see by eye, and so I'm guessing it's related to the small jitter error. > Can you explain why you ignore the translation of the matrix? What is wrong with the current code (why is it wrong)? I see it like this: RenderSVGResourceFilter.cpp: 178 FloatRect drawingRegion = object->strokeBoundingBox(); 179 drawingRegion.intersect(filterData->boundaries); 180 FloatRect absoluteDrawingRegion = filterData->shearFreeAbsoluteTransform.mapRect(drawingRegion); means we're applying shearFreeAbsoluteTransform to the drawing region, which is based on the object bounding box (which doesn't move). For a rotation around the centre of the box, this should really just be the identity matrix, since the BB should neither change size nor location. So really, we're just interested in the scale change of the BB here. Note my example uses non-unit scale, so this still works for scaled objects. When we include the translation, which changes *every step of the animation* and can be 400 - 700 pixels in this example, I guess some small error creeps in? I must admit, I couldn't see the rationale is for including the translation in the shear-free transform.

(In reply to comment #6) > (In reply to comment #4) > > > > I'm not sure how your output looks like, but the current output in trunk is indeed wrong. Can you compare the results with other SVG viewers? > > I'll see if I can reconstruct the SVG for the final state of the test, and try it elsewhere (since the test is dynamic, and requires a test harness, other viewers won't run the dom update portion ...). Just open the test in FF or Opera and click on the image.

Created attachment 106179 [details] Comparison of test results: FF vs. WK with patch

(In reply to comment #7) > (In reply to comment #6) > > (In reply to comment #4) > > > > > > I'm not sure how your output looks like, but the current output in trunk is indeed wrong. Can you compare the results with other SVG viewers? > > > > I'll see if I can reconstruct the SVG for the final state of the test, and try it elsewhere (since the test is dynamic, and requires a test harness, other viewers won't run the dom update portion ...). > > Just open the test in FF or Opera and click on the image. I've attached a comparison of FF and WK with this patch applied. They're not identical, although this patch (IMO) brings us closer to FF.

(In reply to comment #9) > (In reply to comment #7) > > (In reply to comment #6) > > > (In reply to comment #4) > > > > > > > > I'm not sure how your output looks like, but the current output in trunk is indeed wrong. Can you compare the results with other SVG viewers? > > > > > > I'll see if I can reconstruct the SVG for the final state of the test, and try it elsewhere (since the test is dynamic, and requires a test harness, other viewers won't run the dom update portion ...). > > > > Just open the test in FF or Opera and click on the image. > > I've attached a comparison of FF and WK with this patch applied. They're not identical, although this patch (IMO) brings us closer to FF. Have you tried http://dev.w3.org/SVG/profiles/1.1F2/test/svg/filters-image-05-f.svg ?

(In reply to comment #9) > (In reply to comment #7) > > (In reply to comment #6) > > > (In reply to comment #4) > > > > > > > > I'm not sure how your output looks like, but the current output in trunk is indeed wrong. Can you compare the results with other SVG viewers? > > > > > > I'll see if I can reconstruct the SVG for the final state of the test, and try it elsewhere (since the test is dynamic, and requires a test harness, other viewers won't run the dom update portion ...). > > > > Just open the test in FF or Opera and click on the image. > > I've attached a comparison of FF and WK with this patch applied. They're not identical, although this patch (IMO) brings us closer to FF. With the patch, we seem to match FF pretty well on this one. Will upload images in a second ...

Created attachment 106191 [details] Filters Image 05 Comparison - FF

Created attachment 106192 [details] Filters Image 05 Comparison - WebKit (with patch)

Great. Can you add the explanation about bounding box and the identity matrix to the change log please? As a new layout test you can use http://dev.w3.org/SVG/profiles/1.1F2/test/svg/filters-image-05-f.svg. Move it to W3C-SVG-1.1-SE and modify the path to the image if necessary.

Created attachment 106478 [details] Patch

Comment on attachment 106478 [details] Patch Attachment 106478 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9601410 New failing tests: svg/dynamic-updates/SVGFEImageElement-svgdom-preserveAspectRatio-prop.html svg/W3C-SVG-1.1/filters-morph-01-f.svg svg/dynamic-updates/SVGFEImageElement-dom-preserveAspectRatio-attr.html svg/batik/text/textFeatures.svg svg/filters/filterRes.svg

Comment on attachment 106478 [details] Patch r=me with comments. We already should have the smiley in the sources. Can you look at this again please? Should be W3C-SVG-1.1/resources. Please also check that you don't have regressions on chromium. Maybe the results should just be updated.

(In reply to comment #17) > (From update of attachment 106478 [details]) > r=me with comments. We already should have the smiley in the sources. Can you look at this again please? Should be W3C-SVG-1.1/resources. Updated. > Please also check that you don't have regressions on chromium. Maybe the results should just be updated. Yes, I wasn't sure if we should re-baseline those tests after the patch went through or not. I've generated the new baselines, but I have a question: some (all?) of the original expected results were platform non-specific ... should I replace the expected output the same way, or should I just update the chromium expected output? I'm assuming the former, but in case I'm wrong would like your opinion. I'll re-post the revised patch when I hear from you.

(In reply to comment #18) > (In reply to comment #17) > > (From update of attachment 106478 [details] [details]) > > r=me with comments. We already should have the smiley in the sources. Can you look at this again please? Should be W3C-SVG-1.1/resources. > > Updated. > > > Please also check that you don't have regressions on chromium. Maybe the results should just be updated. > > Yes, I wasn't sure if we should re-baseline those tests after the patch went through or not. > > I've generated the new baselines, but I have a question: some (all?) of the original expected results were platform non-specific ... should I replace the expected output the same way, or should I just update the chromium expected output? I'm assuming the former, but in case I'm wrong would like your opinion. > > I'll re-post the revised patch when I hear from you. Just re-checked this ... apparently many of the platforms have specific pixel tests. I have a Linux and a Mac workstation (but it needs a new WebKit repo), but not Windows. For the expected output, can I just temporarily set the expectations on Win (and Mac) to fail, and grab the expected pixel output from the bots? The .txt expectations seem to be the same (as expected).

(In reply to comment #19) > (In reply to comment #18) > > (In reply to comment #17) > > > (From update of attachment 106478 [details] [details] [details]) > > > r=me with comments. We already should have the smiley in the sources. Can you look at this again please? Should be W3C-SVG-1.1/resources. > > > > Updated. > > > > > Please also check that you don't have regressions on chromium. Maybe the results should just be updated. > > > > Yes, I wasn't sure if we should re-baseline those tests after the patch went through or not. > > > > I've generated the new baselines, but I have a question: some (all?) of the original expected results were platform non-specific ... should I replace the expected output the same way, or should I just update the chromium expected output? I'm assuming the former, but in case I'm wrong would like your opinion. > > > > I'll re-post the revised patch when I hear from you. > > Just re-checked this ... apparently many of the platforms have specific pixel tests. I have a Linux and a Mac workstation (but it needs a new WebKit repo), but not Windows. For the expected output, can I just temporarily set the expectations on Win (and Mac) to fail, and grab the expected pixel output from the bots? Sure!

Created attachment 106583 [details] Patch

Comment on attachment 106583 [details] Patch Clearing flags on attachment: 106583 Committed r94697: <http://trac.webkit.org/changeset/94697>

All reviewed patches have been landed. Closing bug.

svg/W3C-SVG-1.1-SE/filters-image-05-f.svg is failing on Mac: http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r94699%20(32896)/results.html Are you sure this test is platform-independent?

(In reply to comment #24) > svg/W3C-SVG-1.1-SE/filters-image-05-f.svg is failing on Mac: > http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r94699%20(32896)/results.html > > Are you sure this test is platform-independent? I think we certainly expected it to be, I'm quite surprised they're different in the text results. It may be we'll need to re-baseline it in some cases.

(In reply to comment #25) > (In reply to comment #24) > > svg/W3C-SVG-1.1-SE/filters-image-05-f.svg is failing on Mac: > > http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r94699%20(32896)/results.html > > > > Are you sure this test is platform-independent? > > I think we certainly expected it to be, I'm quite surprised they're different in the text results. > > It may be we'll need to re-baseline it in some cases.

(In reply to comment #25) > I think we certainly expected it to be, I'm quite surprised they're different in the text results. > > It may be we'll need to re-baseline it in some cases. Could you take care of the failure please?

(In reply to comment #27) > (In reply to comment #25) > > I think we certainly expected it to be, I'm quite surprised they're different in the text results. > > > > It may be we'll need to re-baseline it in some cases. > > Could you take care of the failure please? I've filed this as https://bugs.webkit.org/show_bug.cgi?id=67781 Without having access to a Snow Leopard machine, I'm wondering if it's related to changes in font metrics?

(In reply to comment #28) > (In reply to comment #27) > > (In reply to comment #25) > > > I think we certainly expected it to be, I'm quite surprised they're different in the text results. > > > > > > It may be we'll need to re-baseline it in some cases. > > > > Could you take care of the failure please? > > I've filed this as https://bugs.webkit.org/show_bug.cgi?id=67781 > > Without having access to a Snow Leopard machine, I'm wondering if it's related to changes in font metrics? Have one expectation now in platform/mac - verified it matches for Leopard and Lion.