GNOME Bugzilla – Bug 620565
omission of zero integer part of fractional constant in path data not always interpreted correctly
Last modified: 2013-10-13 15:10:17 UTC
Created attachment 162746 [details] example of misinterpreted ".5" in path data Overview: In path data, a fractional constant such as ".5" is sometimes misinterpreted and replaced by another seemingly arbitrary value. This value can be much larger, resulting in very distorted paths. The SVG specs allow omitting the leading zero[1]. As path data is an essential core component of SVG, I permitted myself to set the bug severity to major. Steps to Reproduce: 1) View following file with rsvg-view or EOG (see also attachment). For actual and expected results see XML comments. <?xml version="1.0" standalone="no"?> <svg xmlns="http://www.w3.org/2000/svg" width="350" height="50"> <g transform="translate(10,0) scale(4)"> <path stroke="red" d="M.5,.5h38.5h.5v9H.5z" stroke-width="1" fill="#FFF"/> <!-- Top line of above rectangle should be precicesly covered by the following line. Instead of starting at (0.5,0.5), the path starts at (0,0) with librsvg. --> <path stroke="black" d="M0,.5H40"/> <!-- Note that in the above path, M0,.5 is interpreted correctly by librsvg. The horizontal coordinate of the right rectangle edge should be 39, but it falsely is 78, as is shown by the following line (subtract half a line width from 78.5): --> <path stroke="black" d="M0,4.5H78.5"/> </g> </svg> Opera, Firefox, Google Chrome, Batik and Inkscape render the file as expected. Build Date & Platform: package librsvg2-2, version 2.26.0-1 on Ubuntu Karmic Additional Builds and Platforms: Same problems can be experienced on Wikipedia. [1] http://www.w3.org/TR/SVG/paths.html#PathDataBNF
Created attachment 162786 [details] [review] Patch for handling of coordinates starting with a dot rsvg-path.c does not properly start a new number when it encounters a dot right after a command (such as l.6 [lower case L dot 6]); only when it encounters a dot right after another fractional number. Current librsvg behavior for the path data "M15,20l.6,1" is to parse it as "Move (15, 20), LineToRelative (20, 0.6) (1, ParseError)" [it just keeps the previous number around]. The proposed patch fixes this in Eye of GNOME on Ubuntu 10.04 Lucid Lynx, giving the proper parse of "Move (15, 20) LineToRelative (0.6, 1)". Reported in Ubuntu at https://bugs.launchpad.net/ubuntu/+source/librsvg/+bug/370061 , with a testcase I used to test my fix. The red-filled path is in the wrong place without this patch.
Louis, the issue you mentioning seems to be another issue. Maybe bug 563933.
Created attachment 162793 [details] [review] fix
Created attachment 162831 [details] [review] fixed fix Hiroyuki-san, your patch doesn't cover the following: <?xml version="1.0" standalone="no"?> <svg xmlns="http://www.w3.org/2000/svg" viewBox="0 -5 10 15" width="100" height="100"> <!-- The black line's path is interpreted incorrectly. It should be where the green line is, not where the red line is. --> <path stroke="green" d="m0.5,0.5h10" stroke-width="2"/> <path stroke="red" d="m0.5,5h10" stroke-width="2"/> <path stroke="black" d="m.5.5h10"/> </svg> Attached patch fixes this, too.
Created attachment 162839 [details] emptytrash.svg (Ubuntu 10.04 Humanity theme) With all due respect, Hiroyuki-shi, bug 563933 (which refers to bug 548494) is about data like "l 1.055.117.481.113" which is parsed correctly in current librsvg (1.055 0.117 0.481 0.113). This specific issue is about the bug reporter's SVG data containing a dot right after a command that is not the first command in the path's data: "[...] h.5" has some quirky behavior. It should be parsed as HorizontalLineRelative (0.5), but some leftover data about the *previous* parsed number is used, so the value used is seemingly random. This leftover data bug affects exponents too, which none of us have a patch for. I attached a test case, which is an optimised trash can icon from Ubuntu 10.04. The relevant path data is ".0065-9E-7-.0096 0", which should be parsed as 0.0065 -9E-7 -0.0096 0 but isn't. You can work around this bug by adding a space after "-9E-7". Stay tuned for a "fixed fixed fix", if applicable.
So, by error, I made that patch against a very old version of librsvg's source code (from March 2010) instead of git head. Getting and building git head results in SVG rendering statistically indistinguishable from random data (like static on a television set). I must be doing something wrong, so I will go away from this bug report now. Sorry for having derailed this thread so much.
Thomas, thank you for the patch. But your fix seems to be wrong. Correct fix is to reset the sign variable to 1 after invoking rsvg_path_end_of_number().
Created attachment 162858 [details] [review] Revised patch
Mh, that can't be right. With your modifications, the trash can that Louis sent looks even more distorted than without the patch, while mine gets it at least almost right.
Created attachment 162887 [details] [review] patch that renders my two test examples and Louis' garbage can correctly Could you please review this patch? It renders all the problem cases discussed above correctly.
Review of attachment 162887 [details] [review]: This patch fixes the problem for me, on both the trash can icon and Obcine_Slovenija_2006_Crensovci.svg from Wikipedia (testcase for the bug from Ubuntu).
The rendering was so bad before Thomas W.'s patch that I thought I had a build error when the icon appeared this distorted. But now I know it was really just the code being FUBAR. Many thanks for this.
I am sorry, I did not check attachment 162839 [details] because I thought it is the same type SVG file. The issue of attachment 162839 [details] should be opened as a new bug.
If Thomas W.'s patch gets accepted for this issue, there is no need to open a new bug for the exponent issue. It fixes both. :)
No, one commit should be a fix for an issue. Otherwise it will be hard to fix regressions in the future.
And unfortunately, Thomas's patch still is not right.
Hiroyuki-san, what would you recommend? Maybe a separate, rewritten svg_parse_number function that branches off from svg_parse_path_data and has a cleaner state machine + better SVG path grammar conformance? That would help a lot with maintainability.
Though I did not recommend refactoring the path data parser, I'd welcome cleanup codes. But write test codes before refactoring, please.
I open a new bug for attachment 162839 [details]. bug #620821. I will attach a patch for it soon.
I can try to do this later. I already started on such a function, to tell the truth, but lost it after too many git clone commands...
Created attachment 162990 [details] [review] Path handling patch This patch against this morning's git is a rewrite of the path number parsing into a separate function and obsoletes everything else so far. It handles this bug and bug 620821. I personally tested it using Thomas W's test case, the trash can and Obcine_Slovenija (images of the renderings will follow, in that order), as well as about 500 various images from Ubuntu 10.04, Scour (by codedread)'s fulltests/ folder, and versions of all of these images optimised by Scour. Reviews welcome.
Created attachment 162991 [details] Render of Thomas W.'s test case in EOG
Created attachment 162992 [details] Render of trash can test case in EOG
Created attachment 162994 [details] Render of Obcine_Slovenija_2006_Crensovci.svg test case in EOG
Created attachment 162995 [details] Render of [this bug, comment 4] test case in EOG Also the SVG data from comment 4 :)
Louis, thanks for the patch! I have not looked into your patch in detail yet, it looks good to me in general. I'd suggest the following steps to get the patch in git master. 1. Open a new bug entry for the patch. 2. Attach the patch in the new bug. 3. Attach test codes which covers bug 620565 and 620821 and others. 4. Mark bug 620565 and 620821 as duplicate of the new bug. Thank you,
Review of attachment 162990 [details] [review]: Though I will review your patch in a new bug entry, here are a couple of notes for you. ::: rsvg-path.c @@ +454,3 @@ +#define RSVGN_IN_PREEXPONENT 3 +#define RSVGN_IN_EXPONENT 4 + These variable should be enum and INTERGER should be NUMBER? typedef enum { RSVG_NUMBER_PARSER_IN_PRENUMBER, RSVG_NUMBER_PARSER_IN_NUMBER, ... } RsvgNumberPaserState; @@ +466,3 @@ +{ + int length = 0; + int in = RSVGN_IN_PREINTEGER; /* Current location within the number */ I perfer this variable name is "state". @@ +580,1 @@ if (ctx->param) Please open another bug entry for this.
I can't edit the status of this bug and bug 620821. Hiroyuki-san, please mark them as duplicates of the new bug 620923.
Sure, I will do it.
*** This bug has been marked as a duplicate of bug 620923 ***
Any progress on this?