Bug 67503

Summary: Eliminate motion jitter in animated, blurred SVG image
Product: WebKit Reporter: W. James MacLean <wjmaclean>
Component: SVGAssignee: W. James MacLean <wjmaclean>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, krit, rniwa, webkit.review.bot, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 41761    
Attachments:
Description Flags
Patch
none
Demonstration of motion jitter.
none
Comparison of test results: FF vs. WK with patch
none
Filters Image 05 Comparison - FF
none
Filters Image 05 Comparison - WebKit (with patch)
none
Patch
none
Patch none

Description W. James MacLean 2011-09-02 10:38:26 PDT
Eliminate motion jitter in animated, blurred SVG image
Comment 1 W. James MacLean 2011-09-02 10:39:47 PDT
Created attachment 106151 [details]
Patch
Comment 2 W. James MacLean 2011-09-02 10:46:57 PDT
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.
Comment 3 W. James MacLean 2011-09-02 10:48:41 PDT
Oh, no layout test yet, but will provide one before final review. Where do I clear the "review?" flag?
Comment 4 Dirk Schulze 2011-09-02 11:48:11 PDT
(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 5 WebKit Review Bot 2011-09-02 11:50:33 PDT
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
Comment 6 W. James MacLean 2011-09-02 12:09:50 PDT
(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.
Comment 7 Dirk Schulze 2011-09-02 12:19:53 PDT
(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.
Comment 8 W. James MacLean 2011-09-02 13:04:15 PDT
Created attachment 106179 [details]
Comparison of test results: FF vs. WK with patch
Comment 9 W. James MacLean 2011-09-02 13:05:11 PDT
(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.
Comment 10 Dirk Schulze 2011-09-02 13:16:12 PDT
(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 ?
Comment 11 W. James MacLean 2011-09-02 13:58:49 PDT
(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 ...
Comment 12 W. James MacLean 2011-09-02 13:59:53 PDT
Created attachment 106191 [details]
Filters Image 05 Comparison - FF
Comment 13 W. James MacLean 2011-09-02 14:00:46 PDT
Created attachment 106192 [details]
Filters Image 05 Comparison - WebKit (with patch)
Comment 14 Dirk Schulze 2011-09-02 14:18:19 PDT
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.
Comment 15 W. James MacLean 2011-09-06 13:44:20 PDT
Created attachment 106478 [details]
Patch
Comment 16 WebKit Review Bot 2011-09-06 20:08:19 PDT
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 17 Dirk Schulze 2011-09-06 22:43:21 PDT
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.
Comment 18 W. James MacLean 2011-09-07 06:32:25 PDT
(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.
Comment 19 W. James MacLean 2011-09-07 06:39:44 PDT
(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).
Comment 20 Dirk Schulze 2011-09-07 06:42:22 PDT
(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!
Comment 21 W. James MacLean 2011-09-07 07:28:41 PDT
Created attachment 106583 [details]
Patch
Comment 22 WebKit Review Bot 2011-09-07 12:19:28 PDT
Comment on attachment 106583 [details]
Patch

Clearing flags on attachment: 106583

Committed r94697: <http://trac.webkit.org/changeset/94697>
Comment 23 WebKit Review Bot 2011-09-07 12:19:35 PDT
All reviewed patches have been landed.  Closing bug.
Comment 24 Ryosuke Niwa 2011-09-07 13:33:24 PDT
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?
Comment 25 W. James MacLean 2011-09-07 13:55:05 PDT
(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.
Comment 26 Ryosuke Niwa 2011-09-07 16:36:51 PDT
(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.
Comment 27 Ryosuke Niwa 2011-09-07 16:37:32 PDT
(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?
Comment 28 W. James MacLean 2011-09-08 07:38:29 PDT
(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?
Comment 29 W. James MacLean 2011-09-08 10:19:52 PDT
(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.