[webkit-reviews] review denied: [Bug 17284] [CAIRO] Introduce single-pixel image optimizations : [Attachment 28192] single pixel optimization

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Mar 2 13:21:30 PST 2009


Eric Seidel <eric at webkit.org> has denied Dirk Schulze <krit at webkit.org>'s
request for review:
Bug 17284: [CAIRO] Introduce single-pixel image optimizations
https://bugs.webkit.org/show_bug.cgi?id=17284

Attachment 28192: single pixel optimization
https://bugs.webkit.org/attachment.cgi?id=28192&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
I don't really know enough about Cairo to know if your color extraction code is
correct.  I think I would have abstracted it into a nice little static function
though:

     uint32_t* pixel = reinterpret_cast<uint32_t*>(data);
 208	 int alpha, red, green, blue;
 209 
 210	 if (alpha = (*pixel & 0xff000000) >> 24) {
 211	     red = ((*pixel & 0x00ff0000) >> 16) * 255 / alpha;
 212	     green = ((*pixel & 0x0000ff00) >> 8) * 255 / alpha;
 213	     blue = (*pixel & 0x000000ff) * 255 / alpha;
 214	 } else {
 215	     red = (*pixel & 0x00ff0000) >> 16;
 216	     green = (*pixel & 0x0000ff00) >> 8;
 217	     blue = (*pixel & 0x000000ff);
 218	 }

Color color = colorFromCairoPixel(*pixel);

I think someone who knows more about Cairo should comment before I can mark
this r+.

Also... is (*pixel & 0xff000000) >> 24) the cleanest way to get the alpha out
of a cairo pixel?  Talk about ugly, error-prone math...

I think actually I'm going to r- for the way you grab colors out of the pixel. 
Even if that's the only way, we should make those static inlines so that
they're not so error prone.


More information about the webkit-reviews mailing list