Bug 23927

Summary: LayoutTests for svg fail after TransformationMatrix changes
Product: WebKit Reporter: Chris Marrin <cmarrin>
Component: Tools / TestsAssignee: Chris Marrin <cmarrin>
Status: RESOLVED DUPLICATE    
Severity: Normal CC: davinci, eric, krit, oliver, simon.fraser, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.5   
Attachments:
Description Flags
Patch for getTransformToElement fix
none
Patch to skip SVG tests until problem is resolved none

Chris Marrin
Reported 2009-02-12 10:16:44 PST
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.
Attachments
Patch for getTransformToElement fix (1.45 KB, patch)
2009-02-12 10:20 PST, Chris Marrin
no flags
Patch to skip SVG tests until problem is resolved (1.60 KB, patch)
2009-02-12 10:28 PST, Chris Marrin
no flags
Chris Marrin
Comment 1 2009-02-12 10:20:35 PST
Created attachment 27605 [details] Patch for getTransformToElement fix This patch fixes one test on PPC Mac
Chris Marrin
Comment 2 2009-02-12 10:28:55 PST
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 3 2009-02-12 10:44:14 PST
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
Comment 4 2009-02-12 11:25:51 PST
Applied getTransfrmToElement patch Sending LayoutTests/ChangeLog Sending LayoutTests/svg/custom/getTransformToElement.svg Transmitting file data .. Committed revision 40914.
Chris Marrin
Comment 5 2009-02-12 11:26:24 PST
Applied patch to skip some tests Sending LayoutTests/ChangeLog Sending LayoutTests/platform/mac/Skipped Transmitting file data .. Committed revision 40915.
Chris Marrin
Comment 6 2009-02-13 08:32:45 PST
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 7 2009-04-29 14:55:07 PDT
Comment on attachment 27605 [details] Patch for getTransformToElement fix Clearing review flag to remove from commit queue.
Eric Seidel (no email)
Comment 8 2009-04-29 14:55:14 PDT
Comment on attachment 27608 [details] Patch to skip SVG tests until problem is resolved Clearing review flag to remove from commit queue.
Nikolas Zimmermann
Comment 9 2010-01-02 10:07:54 PST
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
Comment 10 2010-03-24 10:21:54 PDT
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
Comment 11 2010-03-24 10:35:53 PDT
(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)
Comment 12 2010-03-24 10:39:25 PDT
I agree that making a cross-platform version of Path::debugString() makes the most sense.
David Yonge-Mallo
Comment 13 2010-03-24 13:51:44 PDT
(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
Comment 14 2010-03-24 15:55:40 PDT
(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
Comment 15 2010-03-26 04:35:25 PDT
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
Comment 16 2010-10-07 06:12:21 PDT
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
Comment 17 2011-11-11 07:28:17 PST
Marked as duplicate of 47467. *** This bug has been marked as a duplicate of bug 47467 ***
Note You need to log in before you can comment on or make changes to this bug.