Bug 42965 – CSS: Add fast-path for rgba() color parsing

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Andreas Kling]

Currently #HHEEXX, #HEX, rgb() and named colors are supported by the CSSParser::parseColor() fast-path.

We should add support for rgba() colors as well.

Example of a page which spends way too much time parsing rgba() colors:
http://www.mrspeaker.net/dev/parcycle/

[Andreas Kling]

Created attachment 62561 [details]
Proposed patch

[Eric Seidel (no email)]

Attachment 62561 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3601387

[Andreas Kling]

Created attachment 62689 [details]
Proposed patch v2

Same patch with fixes for the 2 warnings from the EWS.

[Andreas Kling]

Created attachment 62690 [details]
Proposed patch v2

[Darin Adler]

Comment on attachment 62690 [details]
Proposed patch v2

> +static inline bool parseAlphaDouble(const UChar*& string, const UChar* end, UChar terminator, double& value)
> +{
> +    int length = end - string;
> +    if (!length)
> +        return false;
> +
> +    Vector<char, 256> bytes(length + 1);
> +    for (int i = 0; i < length; ++i) {
> +        if (!isASCIIDigit(string[i]) && string[i] != '.' && string[i] != ')')
> +            return false;
> +        bytes[i] = string[i];
> +    }
> +    bytes[length] = '\0';
> +    char* foundTerminator;
> +    value = WTF::strtod(bytes.data(), &foundTerminator);
> +    value = max(0.0, min(value, 1.0));
> +    string += (foundTerminator - bytes.data()) + 1;
> +    return *foundTerminator == terminator;
> +}

For the typical alpha values like 0, 1, and the tenths and simple powers of too we can do a *lot* faster than calling the general purpose strtod function every time, perhaps even avoiding floating point math altogether.

> +        rgb = makeRGBA(red, green, blue, static_cast<int>(lroundf(255.0f * static_cast<float>(alpha))));

I don’t think it’s correct to multiply the alpha value by 255. At other call sites you'll see code more like this:

    nextafter(256, 0) * alpha

We need some test cases that cover the edge cases, the boundaries between alpha of 254 and 255 for example, and alpha of 0 and 1. The relevant functions to look at are CSSParser::parseColorParameters and the various functions in the Color.cpp file. There may be some cases where multiplying by 255 is right. Maybe we can also do code sharing.

If the test cases prove that you are not changing behavior, then I suppose you could put this patch up for review again.

review- because of the 255/256 thing.

[Andreas Kling]

Created attachment 63183 [details]
Proposed patch v3

Updated patch.
* Use nextafter(256.0, 0.0)
* Avoid strtod() for "0", "1", and "0.x"
* Added a test to verify that we're not inconsistent with the slow-path.

[Eric Seidel (no email)]

Attachment 63183 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3619464

[Andreas Kling]

(In reply to comment #7)
> Attachment 63183 [details] did not build on mac:
> Build output: http://queues.webkit.org/results/3619464

/Users/eseidel/Projects/MacEWS/WebCore/css/CSSParser.cpp: In static member function 'static bool WebCore::CSSParser::parseColor(const WebCore::String&, WebCore::RGBA32&, bool)':
/Users/eseidel/Projects/MacEWS/WebCore/css/CSSParser.cpp:3811: warning: 'alpha' may be used uninitialized in this function

False alarm AFAICS. I can jam a "value = 0;" at the start of parseAlphaValue() to silence it if desired.

[Eric Seidel (no email)]

Comment on attachment 63183 [details]
Proposed patch v3

False alarm or not, it breaks Mac, which is a No-Go.

It's definitely possible to return out of parseAlphaValue without setting "value" which may be what's confusing the compiler?

Then again the compiler could be right and we could be reading something wrong here.

[Darin Adler]

Comment on attachment 63183 [details]
Proposed patch v3

Initializing alpha to 0 is probably the simplest way to get rid of that warning.

Are strings like ".1" or ".9" valid for alpha, as opposed to "0.1" and "0.9"? Do we have test cases that cover that?

[Andreas Kling]

Created attachment 63446 [details]
Proposed patch v4

Updated to support ".0" through ".9" as well. Fleshed out the layout test a bit.

[Darin Adler]

Comment on attachment 63446 [details]
Proposed patch v4

What is an alpha value such as "2" supposed to do? Does it still do what it used to do before?

[Andreas Kling]

(In reply to comment #12)
> (From update of attachment 63446 [details])
> What is an alpha value such as "2" supposed to do? Does it still do what it used to do before?

Alpha values should be clamped between 0.0 and 1.0, so "2" will result in 1.0 (via the slow-path.)

[Andreas Kling]

Here's the relevant spec bit about alpha value clamping:
http://www.w3.org/TR/css3-color/#alphavaluedt

@Darin: Ping. :-)

[Darin Adler]

Comment on attachment 63446 [details]
Proposed patch v4

> +    int length = end - string;
> +    if (length < 2)
> +        return false;

So just plain "0" and "1" are not in the fast path?

> +    if ((length == 4 && string[0] == '0' && string[1] == '.' && isASCIIDigit(string[2])
> +        || (length == 3 && string[0] == '.' && isASCIIDigit(string[1])))) {

Might be nice to use a helper function just to make this if statement easier to read.

> +        switch (string[length - 2]) {
> +        case '0': value = 0; break;
> +        case '1': value = 25; break;
> +        case '2': value = 51; break;
> +        case '3': value = 76; break;
> +        case '4': value = 102; break;
> +        case '5': value = 127; break;
> +        case '6': value = 153; break;
> +        case '7': value = 179; break;
> +        case '8': value = 204; break;
> +        case '9': value = 230; break;
> +        }

Seems like a small array would work better here than a switch.

> +    Vector<char, 256> bytes(length + 1);

The number 256 seems a little high here.

r=me

[Andreas Kling]

(In reply to comment #15)
> (From update of attachment 63446 [details])
> > +    int length = end - string;
> > +    if (length < 2)
> > +        return false;
> 
> So just plain "0" and "1" are not in the fast path?

'length' includes the terminating character.

> r=me

Hugs! Will make the requested adjustments before landing.

[Andreas Kling]

Committed r65475: <http://trac.webkit.org/changeset/65475>

[Simon Hausmann]

Revision r65475 cherry-picked into qtwebkit-2.1 with commit aa4d1183733f4669dcf3ef89b48ae01b2fbc3fa0