Bug 67503 - Eliminate motion jitter in animated, blurred SVG image
: Eliminate motion jitter in animated, blurred SVG image
Status: RESOLVED FIXED
: WebKit
SVG
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
: 41761
  Show dependency treegraph
 
Reported: 2011-09-02 10:38 PST by
Modified: 2011-09-08 10:19 PST (History)


Attachments
Patch (1.80 KB, patch)
2011-09-02 10:39 PST, W. James MacLean
no flags Review Patch | Details | Formatted Diff | Diff
Demonstration of motion jitter. (2.76 KB, image/svg+xml)
2011-09-02 10:46 PST, W. James MacLean
no flags Details
Comparison of test results: FF vs. WK with patch (153.49 KB, image/png)
2011-09-02 13:04 PST, W. James MacLean
no flags Details
Filters Image 05 Comparison - FF (84.13 KB, image/png)
2011-09-02 13:59 PST, W. James MacLean
no flags Details
Filters Image 05 Comparison - WebKit (with patch) (59.15 KB, image/png)
2011-09-02 14:00 PST, W. James MacLean
no flags Details
Patch (60.74 KB, patch)
2011-09-06 13:44 PST, W. James MacLean
no flags Review Patch | Details | Formatted Diff | Diff
Patch (581.56 KB, patch)
2011-09-07 07:28 PST, W. James MacLean
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-09-02 10:38:26 PST
Eliminate motion jitter in animated, blurred SVG image
------- Comment #1 From 2011-09-02 10:39:47 PST -------
Created an attachment (id=106151) [details]
Patch
------- Comment #2 From 2011-09-02 10:46:57 PST -------
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.
------- Comment #3 From 2011-09-02 10:48:41 PST -------
Oh, no layout test yet, but will provide one before final review. Where do I clear the "review?" flag?
------- Comment #4 From 2011-09-02 11:48:11 PST -------
(In reply to comment #2)
> Created an attachment (id=106152) [details] [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 From 2011-09-02 11:50:33 PST -------
(From update of attachment 106151 [details])
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 From 2011-09-02 12:09:50 PST -------
(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 From 2011-09-02 12:19:53 PST -------
(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 From 2011-09-02 13:04:15 PST -------
Created an attachment (id=106179) [details]
Comparison of test results: FF vs. WK with patch
------- Comment #9 From 2011-09-02 13:05:11 PST -------
(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 From 2011-09-02 13:16:12 PST -------
(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 From 2011-09-02 13:58:49 PST -------
(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 From 2011-09-02 13:59:53 PST -------
Created an attachment (id=106191) [details]
Filters Image 05 Comparison - FF
------- Comment #13 From 2011-09-02 14:00:46 PST -------
Created an attachment (id=106192) [details]
Filters Image 05 Comparison - WebKit (with patch)
------- Comment #14 From 2011-09-02 14:18:19 PST -------
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 From 2011-09-06 13:44:20 PST -------
Created an attachment (id=106478) [details]
Patch
------- Comment #16 From 2011-09-06 20:08:19 PST -------
(From update of attachment 106478 [details])
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 From 2011-09-06 22:43:21 PST -------
(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. Please also check that you don't have regressions on chromium. Maybe the results should just be updated.
------- Comment #18 From 2011-09-07 06:32:25 PST -------
(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.
------- Comment #19 From 2011-09-07 06:39:44 PST -------
(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?

The .txt expectations seem to be the same (as expected).
------- Comment #20 From 2011-09-07 06:42:22 PST -------
(In reply to comment #19)
> (In reply to comment #18)
> > (In reply to comment #17)
> > > (From update of attachment 106478 [details] [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 From 2011-09-07 07:28:41 PST -------
Created an attachment (id=106583) [details]
Patch
------- Comment #22 From 2011-09-07 12:19:28 PST -------
(From update of attachment 106583 [details])
Clearing flags on attachment: 106583

Committed r94697: <http://trac.webkit.org/changeset/94697>
------- Comment #23 From 2011-09-07 12:19:35 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #24 From 2011-09-07 13:33:24 PST -------
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 From 2011-09-07 13:55:05 PST -------
(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 From 2011-09-07 16:36:51 PST -------
(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 From 2011-09-07 16:37:32 PST -------
(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 From 2011-09-08 07:38:29 PST -------
(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 From 2011-09-08 10:19:52 PST -------
(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.