Bug 89138 – [cairo] improve putByteArray speed by avoiding max/min checks at Color construction.

• Component: Platform
• Status: RESOLVED FIXED

[arno.]

Hi,
as explained in bug #89130, putByteArray constructs Color objects from unsigned char* elements.
Color constructors takes integers arguments and checks they are between 0 and 255.
As arguments are unsigned char*, we known for sure they're between 0 and 255, so we could probably try to skip this step.

[arno.]

Created attachment 147682 [details]
patch proposal

[Martin Robinson]

Is it possible to simply have an unsigned char constructor for Color?

[arno.]

Created attachment 148385 [details]
updated patch: unsigned char constructor for Color

[Martin Robinson]

Comment on attachment 148385 [details]
updated patch: unsigned char constructor for Color

View in context: https://bugs.webkit.org/attachment.cgi?id=148385&action=review

> Source/WebCore/platform/graphics/Color.h:75
> +inline RGBA32 makeRGBUnsafe(int r, int g, int b)
> +{
> +    return 0xFF000000 | r << 16 | g << 8 | b;
> +}
> +
> +inline RGBA32 makeRGBAUnsafe(int r, int g, int b, int a)
> +{
> +    return a << 24 | r << 16 | g << 8 | b;
> +}
> +

Why not simply override these methods as well to take unsigneds?

[Build Bot]

Comment on attachment 148385 [details]
updated patch: unsigned char constructor for Color

Attachment 148385 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12992015

[arno.]

Created attachment 148404 [details]
patch v2.1: prevents build failure on mac

[Igor Trindade Oliveira]

Comment on attachment 148404 [details]
patch v2.1: prevents build failure on mac

View in context: https://bugs.webkit.org/attachment.cgi?id=148404&action=review

> Source/WebCore/platform/graphics/cg/ImageCG.cpp:131
> +            m_solidColor = Color(pixel[0] * 255 / pixel[3], pixel[1] * 255 / pixel[3], pixel[2] * 255 / pixel[3], (int)pixel[3]);

nit: normally we use c++ cast.

[arno.]

Created attachment 148449 [details]
patch v2.2: using static_cast

[arno.]

(In reply to comment #4)
> (From update of attachment 148385 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=148385&action=review
> 
> > Source/WebCore/platform/graphics/Color.h:75
> > +inline RGBA32 makeRGBUnsafe(int r, int g, int b)
> > +{
> > +    return 0xFF000000 | r << 16 | g << 8 | b;
> > +}
> > +
> > +inline RGBA32 makeRGBAUnsafe(int r, int g, int b, int a)
> > +{
> > +    return a << 24 | r << 16 | g << 8 | b;
> > +}
> > +
> 
> Why not simply override these methods as well to take unsigneds?

Because some parts of the code call makeRGB(float, ), and if we make makeRGB(unsigned char), those calls would become redundant. Also, maybe in some cases, it would be convenient to call makeRGBUnsafe with integers if we already known those integers are between 0 and 255 (I'm thinking for example at the colorFromPremultipliedARGB function)

[arno.]

ccing adam barth as he's among one of the most few people to have review Color.h changes recently.

[Adam Barth]

Is there a measurable performance gain from this change?

[arno.]

(In reply to comment #11)
> Is there a measurable performance gain from this change?

http://renevier.net/misc/webkit_89138.html
calls getImageData 100 times. Time it takes goes from 220ms to 165ms with GtkLauncher (185ms if function is not inlined)

[Adam Barth]

This change seems valuable, but I'm slightly worried that it introduces a subtle overloading of the constructor.  I wonder if it would make more sense to introduce a new static function for creating these objects so callers can be explicit about whether they want to unchecked version.

[arno.]

Created attachment 148657 [details]
patch v2.3: Color::createUnChecked function

[Adam Barth]

Comment on attachment 148657 [details]
patch v2.3: Color::createUnChecked function

This looks much better.  Thanks.

[WebKit Review Bot]

Comment on attachment 148657 [details]
patch v2.3: Color::createUnChecked function

Clearing flags on attachment: 148657

Committed r120905: <http://trac.webkit.org/changeset/120905>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.