Make nsvg__parseColorRGB() independent of the current locale (#139)

[Albrecht-S]

Rationale: This commit fixes the locale dependency (re-)introduced by commit c3ad36ef81992ff714cdbb4543cd67cb66daad8c by using sscanf() to parse floating point values.

This modification uses nsvg__atof() which is independent of the current locale.

[Albrecht-S]

Things I noticed during development of this fix:

1. It was longer than anticipated. Using nsvg__atof() for parsing lacks a return value of how many characters (bytes) it consumed. This is something that strtof() or strtod() would provide. The man page of atof() says: "The behavior is the same as strtod(nptr, NULL);". I suggest to implement nsvg__strtof() or nsvg__strtod() which would allow to return a pointer to (after) the end of the parsed string. This would basically be nsvg__atof() with a second argument. Then the implementation of nsvg__atof() would be as simple as written in the man page (above):

   static double nsvg__atof(const char* s)
   {
   return nsvg__atof(s, NULL);
   }

@memononen I would volunteer to make a PR covering this enhancement if there was a chance that this would be accepted. This would simplify the implementation of nsvg__parseColorRGB() a lot. What do you think?

But anyway, I'd appreciate if this PR would be accepted as-is to fix this issue ASAP. Thanks.

2. All "image viewers" I tested so far on my Linux system (firefox, google chrome, eog, gimp) obviously use black as the color on parse errors whereas nanosvg returns gray (#808080). This would be easy to fix in nsvg__parseColorRGB() but earlier parse errors in the calling code would still return gray. Would this be worth changing, or is this gray value explicitly intended?

3. And finally, while we're at it: what is the intended behavior of nanosvg's parser:

   • option 1: it's enough to parse well-formed SVG files correctly and ignore other parse errors
   • option 2: find all parse errors and return an error.

What I mean with option 2: what should be done with a faulty value like rgb(100, 120, 130, 140) (too many arguments)? Should it ignore the error, i.e. return rgb(100, 120, 130) or the error value (gray or black)?

[memononen]

Merged.

1) Changing nsvg__atof() to nsvg_strtof() is good idea. I dont think we need the atof() version.

I have usually used helper functions like nsvg__getNextPathItem() to parse next item so that when atof() is called we have already consumed the input. My attempt at solving this issues looked like this:

static unsigned int nsvg__hasPercent(const char* str)
{
    while (*str != '\0') {
        if (*str == '%') return 1;
        str++;
    }
    return 0;
}

static const char* nsvg__getNextColorItem(const char* s, char* it)
{
    int n = 0;
    it[0] = '\0';
    // Skip until a digit or dot
    while (*s && (!nsvg__isdigit(*s) && *s != '.')) s++;
    // Advance until digit or dot.
    while (*s && (nsvg__isdigit(*s) || *s == '.')) {
        if (n < 63)
            it[n++] = *s;
        s++;
    }
    it[n++] = '\0';
    return s;
}

static unsigned int nsvg__parseColorRGB(const char* str)
{
    unsigned int col[3] = {128, 128, 128};
    char item[64];

    // skip "rgb("
    str += 4;

    // parse color components
    if (nsvg__hasPercent(str)) {
        // rgba(123.45%, 123.45%, 123.45%)
        for (int i = 0; i < 3; i++) {
            str = nsvg__getNextColorItem(str, item);
            col[i] = (unsigned int)roundf(nsvg__atof(item) / 100.f * 255.f);
        }
    } else {
        // rgba(123, 123, 123)
        for (int i = 0; i < 3; i++) {
            str = nsvg__getNextColorItem(str, item);
            col[i] = strtoul(item, NULL, 10);
        }
    }

    return NSVG_RGB(col[0], col[1], col[2]);
}

2) Returning black as error sounds good to me, lets change that.

3) Option 1. NanoSVG assumes valid SVG, and tries to parse as much as it can and ignore errors unless fatal.

[Albrecht-S]

@memononen Thank you very much for the quick merge here as well. We're using NanoSVG in FLTK and this 'locale' issue seemed to be a problem that needed a fix. I can now remove two patches (both PR's) from our fork. Thank you.

1. Changing nsvg__atof() to nsvg_strtof() is good idea. I dont think we need the atof() version.

I can look into it. This doesn't look like a big deal. If you like I can try to make a PR.

Your code version looks interesting. Given point 3, option 1, it looks as if it would parse valid files correctly. However, I assume that it would not catch missing '%' characters in some of the arguments, e.g. rgb(100%, 80, 50%). Wouldn't it interpret 80 as 80%? Sure, that wouldn't matter because it's invalid syntax (all components must use the same format, either % or not) but I believe it could lead to unexpected results. Parsing this as an error seems more appropriate to me. Anyway, this is a moot point now after the merge.

2. Returning black as error sounds good to me, lets change that.

It's easy in the new parser function but rgb(128, 128, 128) must be set somewhere on the calling level if something goes awry and nsvg__parseColorRGB() is not called at all. I can try to fix this too, but I'm not sure if I can find it easily. Maybe it's easy for you, would you like to do this yourself?

3. NanoSVG assumes valid SVG, ...

Thanks for clarification, this can make some things easier. If we could use nsvg_strtof() in nsvg__parseColorRGB() this could simplify the code significantly but then some other obviously invalid syntax like 50.% (according to my tests with image viewers, not by reading the standard) might slip through because (I assume that) nsvg_strtof() would allow a floating point number with a trailing period. This could still be flagged as an error but it would probably be easier to accept it (see "rule" 3. above).

[memononen]

I did not have time to properly test my approach, I liked he error handling in your version better.

If you like I can try to make a PR. Yes, please :)

The default color is set in nsvg__createParser(), which is black. Also the default color for a gradient stop is black. It should be just matter of changing those 128,128,128 to 0,0,0. I can do this.

Your last point is exactly why I've used those parse item functions in the past. It makes it manageable to add the correct checking and then use the common logic to do the conversion once you've validated the input.

[Albrecht-S]

I remember that I got a gray image (I've used a simple rectangle) in some cases even after I had changed the error color in nsvg__parseColorRGB() because it was not called at all under some circumstances.

I can check this tomorrow, not before afternoon CEST, but if you would like to change the error return colors I would appreciate it. Just FYI: in the current version of nsvg__parseColorRGB() it would only be the assignment near the end:

rgbi[0] = rgbi[1] = rgbi[2] = 128;

but I assume you know that already.

For the other gray return values I found only:

• nsvg__parseColorName()
• nsvg__parseColorHex()

Both functions use NSVG_RGB(128, 128, 128); - I'm not aware of another code part but maybe you know better. If you could do this it would save us another PR. ;-) Thanks in advance.

Your last point is exactly why I've used those parse item functions in the past...

Yes, this is just good programming style. If used consequently you can fix a bug or change a function in one place rather than a hundred similar duplicated code lines. That's also the reason why I suggested to introduce nsvg__strtof() to avoid parsing the string twice and reuse existing code for both the conversion and the advancement of the string pointer.