Bug 23927 – LayoutTests for svg fail after TransformationMatrix changes

• Component: Tools / Tests
• Status: RESOLVED DUPLICATE

[Chris Marrin]

Because of the change in TransformationMatrix from doing computations in floats to doubles, precision differences show up in LayoutTests as slightly different numeric values. Most of these are things like 0.00 vs -0.00 and 17s vs 172.00. Fixing DRT to get rid of these two differences will fix the problem in almost all tests

The downside is that changing how numbers print out will require many test results to be redone.

[Chris Marrin]

Created attachment 27605 [details]
Patch for getTransformToElement fix

This patch fixes one test on PPC Mac

[Chris Marrin]

Created attachment 27608 [details]
Patch to skip SVG tests until problem is resolved

This patch skips 16 svg tests until the number printing problem is resolved

[Simon Fraser (smfr)]

Comment on attachment 27608 [details]
Patch to skip SVG tests until problem is resolved

r=me, but make sure you keep this bug open, and assign it to yourself.

[Chris Marrin]

Applied getTransfrmToElement patch

Sending        LayoutTests/ChangeLog
Sending        LayoutTests/svg/custom/getTransformToElement.svg
Transmitting file data ..
Committed revision 40914.

[Chris Marrin]

Applied patch to skip some tests

Sending        LayoutTests/ChangeLog
Sending        LayoutTests/platform/mac/Skipped
Transmitting file data ..
Committed revision 40915.

[Chris Marrin]

I'm leaving this bug open. LayoutTests are now clear (WRT these issues), but some tests are being skipped, so the underlying problem still needs to be resolved.

[Eric Seidel (no email)]

Comment on attachment 27605 [details]
Patch for getTransformToElement fix

Clearing review flag to remove from commit queue.

[Eric Seidel (no email)]

Comment on attachment 27608 [details]
Patch to skip SVG tests until problem is resolved

Clearing review flag to remove from commit queue.

[Nikolas Zimmermann]

Hm, any news regarding this bug? I'd love to see it resolved.
The problem is the way Path::debugString() (which is used by DRT -> SVGRenderTreeAsText -> RenderPath -> Path::debugString()) works: it's manually traversing the path items, building up the d=".." string thatt will be dumped in the -expected.txt files (for any SVG test).
We should make that code cross-platform (why should every platform have to write it's own debugString() method, that's only used for layout tests anyways), and fix the rounding issues alltogether.

We shouldn't skip SVG tests for a long period like that :(

[David Yonge-Mallo]

I'm also running into display problems with the precision of large numbers here: https://bugs.webkit.org/show_bug.cgi?id=25645

What would it involve to change Path::debugString() to output numbers in scientific (exponential) notation for sufficiently large (or small) numbers in a consistent manner across all platforms?  Are people generally against this since it would mean rebaselining a bunch of tests?  That seems better than skipping them.

[Dirk Schulze]

(In reply to comment #10)
> I'm also running into display problems with the precision of large numbers
> here: https://bugs.webkit.org/show_bug.cgi?id=25645
> 
> What would it involve to change Path::debugString() to output numbers in
> scientific (exponential) notation for sufficiently large (or small) numbers in
> a consistent manner across all platforms?  Are people generally against this
> since it would mean rebaselining a bunch of tests?  That seems better than
> skipping them.

I don't believe that the issues here is relatet to the one in bug 25645. But to answer your question, you have to change the platform-specific debug infos at WebCore/platform/graphcis/<platform>/Path*.cpp
But some platforms, like cairo, don't support float values, they use fixed point instead. So you would break many DRT-tests by changing debugString.

[Simon Fraser (smfr)]

I agree that making a cross-platform version of Path::debugString() makes the most sense.

[David Yonge-Mallo]

(In reply to comment #11)
> I don't believe that the issues here is relatet to the one in bug 25645. But to
> answer your question, you have to change the platform-specific debug infos at
> WebCore/platform/graphcis/<platform>/Path*.cpp
> But some platforms, like cairo, don't support float values, they use fixed
> point instead. So you would break many DRT-tests by changing debugString.

It's unrelated except that they both output using debugString().  That's what I thought about debugString() -- that changing it would break everything.  Thanks for confirming that.  I guess what we should aim for is round-trip preservation of values, so if I enter 12345678901234567890 then debugString should output "12345678901234567890[.00]".  That's probably very difficult to do.

[Dirk Schulze]

(In reply to comment #12)
> I agree that making a cross-platform version of Path::debugString() makes the
> most sense.

Not sure if it makes sense and realy depends on how we are doing it. E.g. if we save every point , every line or curve or transformation in an extra array, we don't know what the different platforms are realy doing. The question is, if we care about the real output, or if we care about what webkit wants to draw (independent of what we see on the screen).
Both things can be from interest.

Note, that transformations on a path would also cause transformations on previous stored values on the array, for the platform independent solution.

[Nikolas Zimmermann]

Just brainstormed this again with Dirk and there's a new idea:

How about utilizing the already present coordinate information (path segments + coordinates) from the SVG DOM when dumping the RenderPath debugString?

Instead of pulling it from the engines (ask Cg for path segments..) we could ask just access the node() of the RenderPath, which is a SVGPathElement. This contains a SVGPathSegList with all necessary information. The only drawback is that we have to manually transform the coordinates into absolute coords on screen, to get the same results we dump now (same coord space).

The benefit is that OpenVG which doesn't even offer access to the path segments can easily be supported. The drawback is that we loose information from the engine itself.

We could do following: Keep the current debugString implementation seperated, implement a new cross-platfrom debugstring utiliyzing SVGPathSegList and compare the transformed coordinates from the engine and our internally stored coordinates from SVGPathSegList using a tolerance (aka. "fuzzy compare"). If we see significant deviations, we could dump two debug strings in the DRT results, indicating an error, if not we could just dump the cross-platform version.

Details need to be worked out, but we think this may be the right step in the right direction.

Comments appreciated!

[Nikolas Zimmermann]

I'm going to reenable the tests in question, it makes no sense to skip them just because of 0.0 vs -0.0 issues. The tests are helpful, and we want them nontheless.

The debugString() problem is being worked on at the moment, and we need these tests to verify the problem is gone, see bug 47340.

[Nikolas Zimmermann]

Marked as duplicate of 47467.

*** This bug has been marked as a duplicate of bug 47467 ***