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.
Created attachment 147682 [details] patch proposal
Is it possible to simply have an unsigned char constructor for Color?
Created attachment 148385 [details] updated patch: unsigned char constructor for Color
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?
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
Created attachment 148404 [details] patch v2.1: prevents build failure on mac
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.
Created attachment 148449 [details] patch v2.2: using static_cast
(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)
ccing adam barth as he's among one of the most few people to have review Color.h changes recently.
Is there a measurable performance gain from this change?
(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)
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.
Created attachment 148657 [details] patch v2.3: Color::createUnChecked function
Comment on attachment 148657 [details] patch v2.3: Color::createUnChecked function This looks much better. Thanks.
Comment on attachment 148657 [details] patch v2.3: Color::createUnChecked function Clearing flags on attachment: 148657 Committed r120905: <http://trac.webkit.org/changeset/120905>
All reviewed patches have been landed. Closing bug.