Get our color channels straight. The reason why the color swap was required...

Get our color channels straight. The reason why the weird color swap in
cocoa_utils_unittest.mm was required was that the SkBitmap that
SkBitmapToNSImage() in cocoa_utils received already had messed-up channels when
it was passed in from Chromium but not when it was passed in from the test. The
reason for it being corrupt when being passed in from Chromium was that
CGImageToSkBitmap(), which sends it from the renderer to the browser, assumes
that Skia's and CoreGraphic's image memory layout match.

This is a reasonable assumption to make (we don't want to swizzle bytes around
all the time when converting between the two image types), but it is wrong at
the moment. CoreGraphics uses BGRA in memory on little-endian machines with
Apple's recommended kCGBitmapByteOrder32Host | kCGImageAlphaPremultipliedFirst
configuration. Skia is currently configured to use RGBA for some reason -- so
the red and blue channels are swapped.

Since we can't change CoreGraphics, I changed Skia, and things still seem to
work (a small fix to Skia's SkCreateCGImageRef() is required, because this
function assumes that Skia's default argb layout is used). I do realize that
this is a larger change, so please review carefully. But it _is_ the right
thing to do in my eyes.

I also added a few preprocessor checks to the places where we make assumptions
about Skia's image memory layout.

This requires a patch to Skia, too: http://codereview.appspot.com/84041

BUG=http://crbug.com/14020
TEST=Check that favicons in the bookmark bar still look correct and that
cocoa_utils_unittest now passes without that weird "swap" flag.

[Nico, 2009-06-22 05:24:19]

I looked into why rsesek's favicon patch for OS X might be flaky, and found that
our color channel handling is…not optimal.

jrg, you're here because I'm trying to do one of your TODOs/bugs.

Avi, Amanda, you were involved with rsesek's CL, so you're here too.

senorblanco, your name appeared in the history of SkUserConfig.h, and you seem
to be a Skia reviewer (and this patch needs a small upstream change to Skia)

[Amanda Walker, 2009-06-22 12:20:58]

Nice detective work!  Originally, we tried to accomodate Skia's layout, which
worked but meant our pixel test output did not match the output for Apple's
webkit port.  Sounds we missed something when we fixed up that mismatch. 
Generally LGTM, but before landing please (a) also wait for jrg's LGTM, and (b)
do a layout test run without the --no-pixel-tests flag before and after your
patch to make sure they don't regress.  You should expect a lot of failures
before and after (which is why pixel tests are not yet enabled by default), but
the failures should not differ between the runs (and mismatches are not yet
caught by the buildbots).

[Avi (use Gerrit), 2009-06-22 14:16:20]

Wow. Any change that takes 60 lines of code out is a good one. LG.

http://codereview.chromium.org/141037/diff/1014/1016
File chrome/browser/cocoa/cocoa_utils.mm (right):

http://codereview.chromium.org/141037/diff/1014/1016#newcode24
Line 24: NSImage* SkBitmapToNSImage(const SkBitmap& icon) {
For generality's sake, can we change the name of the parameter? I can imagine
situations where we're not converting an icon.

[Stephen White, 2009-06-22 16:01:44]

Sorry -- although I have a growing familiarity with Skia, I'm not a Skia
reviewer.  Mike Reed is your best bet.  And I think Adam Langley has commit
access to the Skia repo in a pinch.

One question about the Skia change:  why are there byte swaps in the shift
checks here:

 23 #if defined(SK_CPU_LENDIAN) && HAS_ARGB_SHIFTS(24, 0, 8, 16) \
 24  || defined(SK_CPU_BENDIAN) && HAS_ARGB_SHIFTS(0, 24, 16, 8)

 25             // The default rgba ordering from SkColorPriv.h
 18             *info = kCGBitmapByteOrder32Big |	 26             *info =
kCGBitmapByteOrder32Big |
 19                     kCGImageAlphaPremultipliedLast;	 27                    
kCGImageAlphaPremultipliedLast;

But not here:

 28 #elif defined(SK_CPU_LENDIAN) && HAS_ARGB_SHIFTS(24, 16, 8, 0) \
 29    || defined(SK_CPU_BENDIAN) && HAS_ARGB_SHIFTS(24, 16, 8, 0)

Also, would it be easier to back the byte swaps into 
your HAS_ARGB_SHIFTS macro, so you don't need to check the endianness
everywhere?

The chrome-side change looks good to me.  In addition to cleaning up
the code, our Mac and Win performance for Skia should be similar now.

(Tangentially:  I'm guessing the reason Apple recommends BGRA is
because this was traditionally the fastest path for 8-bit-per-component data on
many video cards, since the driver didn't need to do 
any CPU swizzling before DMA'ing it to the card (this was true on 
NVidia hardware, at least).  Not that it matters for us, but for 
16-bit-float-per-component ("half") data, RGBA was the fast path, just 
to be annoying.

Nowadays, modern drivers have ameliorated this performance advantage
somewhat, probably by swizzling on the GPU instead.)

[John Grabowski, 2009-06-22 18:18:26]

LGTM!
Glad to see this problem tracked down.

[Nico, 2009-06-25 03:13:21]

Amanda submitted this as part of her skia & webkit merges today (thanks!), so
I'm just closing this.

http://codereview.chromium.org/141037/diff/1014/1016
File chrome/browser/cocoa/cocoa_utils.mm (right):

http://codereview.chromium.org/141037/diff/1014/1016#newcode24
Line 24: NSImage* SkBitmapToNSImage(const SkBitmap& icon) {
On 2009/06/22 14:16:21, Avi wrote:
> For generality's sake, can we change the name of the parameter? I can imagine
> situations where we're not converting an icon.

Done.