GNOME Bugzilla – Bug 751745
Incorrect SVG path parsing
Last modified: 2017-10-17 20:08:08 UTC
Created attachment 306428 [details] Invalid The next SVG file is parsed differently with GIMP and Chromium: <svg xmlns="http://www.w3.org/2000/svg" width="48" height="48" viewBox="0 0 48 48"><path d="M41.88 22.17C40.96 13.83 34.34 7.21 26 6.29V2.17h-4v4.12c-8.34.92-14.96 7.54-15.88 15.88H2v4h4.12c.92 8.34 7.54 14.96 15.88 15.88v4.12h4v-4.12c8.34-.92 14.96-7.54 15.88-15.88H46v-4h-4.12zM24 38.17c-7.73 0-14-6.27-14-14s6.27-14 14-14 14 6.27 14 14-6.27 14-14 14z"/></svg> The issue seems to reside in parsing sequence `c-8.34.92-14.96`. Adding a whitespace fixes the issue: `c-8.34 .92-14.96`. But it's valid according to SVG Path spec http://www.w3.org/TR/SVG/paths.html#PathDataBNF : The processing of the BNF must consume as much of a given BNF production as possible, stopping at the point when a character is encountered which no longer satisfies the production. ... Similarly, for the string "M 0.6.5", the first coordinate of the "moveto" consumes the characters "0.6" and stops upon encountering the second decimal point because the production of a "coordinate" only allows one decimal point. The result is that the first coordinate will be "0.6" and the second coordinate will be ".5". The same issue is reproducible with GNOME Image Viewer (eog).
Originally I though it was an issue in SVG content: https://github.com/google/material-design-icons/issues/163
Created attachment 306596 [details] [review] sync svg path parser with librsvg It seems that with librsvg from git master this file is imported correctly in gimp master and in gimp-2-8, whereas importing that path from the path tool context-menu is still incorrect. Gimp borrowed and inlined the svg path parser from librsvg because it is not public API, so it gets not updated when librsvg fixes bugs. The attached patch is an attempt to resync the code with librsvg. Since the code changed, it required a little manual tweaking. So while with this patch that svg path is imported correctly, it is possible that I missed something. In short it requires testing. For example this path is not imported correctly (with or without the patch) <path d="M20 20 20 10 10 10 10 20 Z L 30 20 20 30 "/> gimp prints > (gimp-2.9:4): Gimp-Vectors-CRITICAL **: gimp_bezier_stroke_lineto: assertion 'stroke->closed == FALSE' failed I wanted to check whether the current point was reset correctly after a close path command, one adjustment necessary with respect to the librsvg RSVGPathBuilder code.
Would it make sense to simply include the entire file instead of patching? It would reduce the risk of errors when updating.
(In reply to Michael Natterer from comment #3) > Would it make sense to simply include the entire file instead of > patching? It would reduce the risk of errors when updating. I don't know, perhaps with preprocessor tricks. rsvg-path.c builds a cairo_path_t* using functions that accept doubles, gimp builds a GimpBezierStroke * using functions that accept GimpCoord. rsvg-path.c converts quadratic curves to cubic while gimp preserves that info. There are few differences for arcs etc... it is not a simple #include <rsvg-path.c>, work has to be done anyway. And it seems librsvg is LGPL-ed and GIMP core is GPL-ed. I don't even know if it is possible to resync the functions GIMP is using.
I admit I did not even look at the rsvg source :)
I have not checked at the code we borrow from librsvg either, but if that's not a public API, but still a useful API for a third-party project (GIMP), wouldn't it be better to actually ask librsvg developers if what we need could not become a public API?
(In reply to Jehan from comment #6) > I have not checked at the code we borrow from librsvg either, but if that's > not a public API, but still a useful API for a third-party project (GIMP), > wouldn't it be better to actually ask librsvg developers if what we need > could not become a public API? If you're asking me, I can say that librsvg has a function rsvg_parse_path that is marked G_GNUC_INTERNAL that accepts a const char* path_str and returns a cairo_path_t * If you're fine to convert the svg path to a sequence of linear and cubic segments, then looping over the cairo_path_t and converting it to a GimpBezierStroke should be easy. If you want to preserve quadratic and circular segments as such then you'd have to refactor librsvg parsing code to accept generic callback and reimplements gimp_vectors_import_buffer in GIMP. At that point I'd move gimp_vectors_import_buffer in a libgimp* library that is already LGPL licensed and where it is possible to copy, modify and redistribute librsvg code.
Reading from: https://gnu.org/licenses/license-compatibility.html <quote> If a program permits use under GNU LGPL version 3 or later, you can relicense it to GPL version 3 or later; for each future GPL version N (N > 3), we will make an LGPL version N which consists of the GPL version N plus added permissions </quote> Given that the 2 files (rsvg-path.[ch]) from librsvg are licensed LGPL version 2 or later I'd say the code can be updated merging the recent version of the functions used. So this bug is fixed also for paths imported, if in the future that code gets updated again...
Can you take care of this?
Is this an issue in which the end-user will see different results? I have a bug I am about to report, but I do not know if this is related. Here is my bug in layman's terms: SVG files appear distorted when imported into GIMP.
> Here is my bug in layman's terms: SVG files appear distorted when imported into GIMP. If you are not sure, please open a separate bug report with an example SVG file showing the problem. Also what are these keywords you have been adding (ui-review, usability and HELPWANTED)? This is not GUI or usability related (it's just a bug), and I don't even know what this third keyword is supposed to be used for.
(In reply to Jehan from comment #11) > > Here is my bug in layman's terms: SVG files appear distorted when imported into GIMP. > > If you are not sure, please open a separate bug report with an example SVG > file showing the problem. > > Also what are these keywords you have been adding (ui-review, usability and > HELPWANTED)? This is not GUI or usability related (it's just a bug), and I > don't even know what this third keyword is supposed to be used for. Sorry, I thought it may help in case if this bug had something to do with my bug. I added "usability" and "ui-review" because if this bug has something to do with the bug I reported, I know that the Usability and UI of the GIMP app need to be payed close attention to. The attachment labeled "Invalid" appears similar to what GIMP outputs for myself when I attempt to import an SVG vector file, though I may not understand the code very well. :) I added "HELPWANTED" because I know Michael Natterer asked Massimo for help in Comment #9.
Anyways, here is my bug: https://bugzilla.gnome.org/show_bug.cgi?id=788850
Please have a look at https://bugzilla.gnome.org/describekeywords.cgi *I* hesitate to set some of these keywords lightly, and I am a developer here.
Anyway let's remove the keywords.
Created attachment 361704 [details] original SVG from the description Duplicate of bug 783867, or vice versa. $ ./rsvg-convert-2.36.3 -o crosshair.png crosshair.svg produces essentially the same broken output as the first attached PNG.
Thanks for taking the time to report this. This particular bug has already been reported into our bug tracking system, but please feel free to report any further bugs you find. *** This bug has been marked as a duplicate of bug 783867 ***