expose CGImage -> SkBitmap

expose CGImage -> SkBitmap

BUG=skia:

Committed: http://code.google.com/p/skia/source/detail?r=14315

[hal.canary, 2014-04-21 20:15:50]

https://codereview.chromium.org/243463005/diff/1/include/utils/mac/SkCGUtils.h
File include/utils/mac/SkCGUtils.h (right):

https://codereview.chromium.org/243463005/diff/1/include/utils/mac/SkCGUtils....
include/utils/mac/SkCGUtils.h:36: *  Given a CGImage, allocate a SkBitmap and
copy the images's pixels into it. If scaleToFit is not
that's now what this does.

https://codereview.chromium.org/243463005/diff/1/src/ports/SkImageDecoder_CG.cpp
File src/ports/SkImageDecoder_CG.cpp (right):

https://codereview.chromium.org/243463005/diff/1/src/ports/SkImageDecoder_CG....
src/ports/SkImageDecoder_CG.cpp:89: switch (info) {
This logic is also in SkCreateBitmapFromCGImage.  Can we consolidate even more?

[scroggo, 2014-04-21 20:18:44]

Meta concern: I find it problematic that we have separate image decoders for
each platform. When I try to make a change that is important for Android, I have
to make sure that it works on all platforms. For example, I had to revert
https://codereview.chromium.org/223893002/ because the decoders on Mac are
different. (That CL won't back go in until after I fix my Mac so I can test on
Mac. An alternate solution would be to only run the tests on Android, though
that would still mean we had something working on Android and broken elsewhere.)
I don't really see the advantage to maintaining the other decoders.

https://codereview.chromium.org/243463005/diff/1/include/utils/mac/SkCGUtils.h
File include/utils/mac/SkCGUtils.h (right):

https://codereview.chromium.org/243463005/diff/1/include/utils/mac/SkCGUtils....
include/utils/mac/SkCGUtils.h:27: *  Given a CGImage, allocate a SkBitmap and
copy the images's pixels into it. If scaleToFit is not
nit: an SkBitmap
nit: image's

https://codereview.chromium.org/243463005/diff/1/include/utils/mac/SkCGUtils....
include/utils/mac/SkCGUtils.h:33: SK_API bool
SkCreateBitmapFromCGImage(SkBitmap* dst, CGImageRef src, SkISize* scaleToFit =
NULL);
This version is unused. Does Chrome want it?

https://codereview.chromium.org/243463005/diff/1/include/utils/mac/SkCGUtils....
include/utils/mac/SkCGUtils.h:36: *  Given a CGImage, allocate a SkBitmap and
copy the images's pixels into it. If scaleToFit is not
This comment describes the other function, not this one.

https://codereview.chromium.org/243463005/diff/1/src/utils/mac/SkCreateCGImag...
File src/utils/mac/SkCreateCGImageRef.cpp (right):

https://codereview.chromium.org/243463005/diff/1/src/utils/mac/SkCreateCGImag...
src/utils/mac/SkCreateCGImageRef.cpp:253: CGContextRef cg =
CGBitmapContextCreate(pixels, info.width(), info.height(), 8, rowBytes, cs,
Isn't this 8 specific to 8888? If info.colorType() was 565, would this be the
correct value?

https://codereview.chromium.org/243463005/diff/1/src/utils/mac/SkCreateCGImag...
src/utils/mac/SkCreateCGImageRef.cpp:283: CGImageAlphaInfo cgInfo =
CGImageGetAlphaInfo(image);
I'm not a big fan of this duplicated code, if we can avoid it.

[reed1, 2014-04-21 20:44:41]

meta-concern: I don't disagree. Just sharing 1 set of codecs sounds good to me.

This CL in fact simplifies the mac codec a little, but its real intent is to
expose CGImage-specific utilities (mostly for sharing with Chrome). If we ended
up killing the CG codec, this change should still make sense.

https://codereview.chromium.org/243463005/diff/1/include/utils/mac/SkCGUtils.h
File include/utils/mac/SkCGUtils.h (right):

https://codereview.chromium.org/243463005/diff/1/include/utils/mac/SkCGUtils....
include/utils/mac/SkCGUtils.h:27: *  Given a CGImage, allocate a SkBitmap and
copy the images's pixels into it. If scaleToFit is not
On 2014/04/21 20:18:44, scroggo wrote:
> nit: an SkBitmap
> nit: image's

Done.

https://codereview.chromium.org/243463005/diff/1/include/utils/mac/SkCGUtils....
include/utils/mac/SkCGUtils.h:33: SK_API bool
SkCreateBitmapFromCGImage(SkBitmap* dst, CGImageRef src, SkISize* scaleToFit =
NULL);
On 2014/04/21 20:18:44, scroggo wrote:
> This version is unused. Does Chrome want it?

Yes, this is explicit added for chrome!

https://codereview.chromium.org/243463005/diff/1/include/utils/mac/SkCGUtils....
include/utils/mac/SkCGUtils.h:36: *  Given a CGImage, allocate a SkBitmap and
copy the images's pixels into it. If scaleToFit is not
On 2014/04/21 20:18:44, scroggo wrote:
> This comment describes the other function, not this one.

You caught me copy-pasting. Done.

https://codereview.chromium.org/243463005/diff/1/src/ports/SkImageDecoder_CG.cpp
File src/ports/SkImageDecoder_CG.cpp (right):

https://codereview.chromium.org/243463005/diff/1/src/ports/SkImageDecoder_CG....
src/ports/SkImageDecoder_CG.cpp:89: switch (info) {
On 2014/04/21 20:15:51, Hal Canary wrote:
> This logic is also in SkCreateBitmapFromCGImage.  Can we consolidate even
more?

Yea, still trying to understand if this can be shared, and at what level. That
may have to be a separate CL.

https://codereview.chromium.org/243463005/diff/1/src/utils/mac/SkCreateCGImag...
File src/utils/mac/SkCreateCGImageRef.cpp (right):

https://codereview.chromium.org/243463005/diff/1/src/utils/mac/SkCreateCGImag...
src/utils/mac/SkCreateCGImageRef.cpp:253: CGContextRef cg =
CGBitmapContextCreate(pixels, info.width(), info.height(), 8, rowBytes, cs,
On 2014/04/21 20:18:44, scroggo wrote:
> Isn't this 8 specific to 8888? If info.colorType() was 565, would this be the
> correct value?

Yes. that is specfic, and so is the cg_bitmap_info at the moment. I won't land
this until those are fixed.

https://codereview.chromium.org/243463005/diff/1/src/utils/mac/SkCreateCGImag...
src/utils/mac/SkCreateCGImageRef.cpp:283: CGImageAlphaInfo cgInfo =
CGImageGetAlphaInfo(image);
On 2014/04/21 20:18:44, scroggo wrote:
> I'm not a big fan of this duplicated code, if we can avoid it.

Working on it, but haven't seen how to share yet. Perhaps in a separate CL

[reed1, 2014-04-21 22:41:31]

now with GM testing -- PTAL

[scroggo, 2014-04-22 14:58:17]

lgtm

https://codereview.chromium.org/243463005/diff/2/gm/aaclip.cpp
File gm/aaclip.cpp (right):

https://codereview.chromium.org/243463005/diff/2/gm/aaclip.cpp#newcode178
gm/aaclip.cpp:178: virtual uint32_t onGetFlags() const { return kSkipPipe_Flag;
}
SK_OVERRIDE

https://codereview.chromium.org/243463005/diff/2/src/utils/mac/SkCreateCGImag...
File src/utils/mac/SkCreateCGImageRef.cpp (right):

https://codereview.chromium.org/243463005/diff/2/src/utils/mac/SkCreateCGImag...
src/utils/mac/SkCreateCGImageRef.cpp:12: static uint32_t
ComputeCGAlphaInfo_RGBA(SkAlphaType at) {
Should these return CGBitmapInfo for documentation purposes?

https://codereview.chromium.org/243463005/diff/2/src/utils/mac/SkCreateCGImag...
src/utils/mac/SkCreateCGImageRef.cpp:59: const bool opaque = kOpaque_SkAlphaType
== bm.alphaType();
It seems like this could go in the 4444 case (the only place it's used).

https://codereview.chromium.org/243463005/diff/2/src/utils/mac/SkCreateCGImag...
src/utils/mac/SkCreateCGImageRef.cpp:60: const bool premul =
kUnpremul_SkAlphaType != bm.alphaType();
This variable is unused.

[reed1, 2014-04-22 17:51:34]

https://codereview.chromium.org/243463005/diff/2/gm/aaclip.cpp
File gm/aaclip.cpp (right):

https://codereview.chromium.org/243463005/diff/2/gm/aaclip.cpp#newcode178
gm/aaclip.cpp:178: virtual uint32_t onGetFlags() const { return kSkipPipe_Flag;
}
On 2014/04/22 14:58:17, scroggo wrote:
> SK_OVERRIDE

Done.

https://codereview.chromium.org/243463005/diff/2/src/utils/mac/SkCreateCGImag...
File src/utils/mac/SkCreateCGImageRef.cpp (right):

https://codereview.chromium.org/243463005/diff/2/src/utils/mac/SkCreateCGImag...
src/utils/mac/SkCreateCGImageRef.cpp:12: static uint32_t
ComputeCGAlphaInfo_RGBA(SkAlphaType at) {
On 2014/04/22 14:58:17, scroggo wrote:
> Should these return CGBitmapInfo for documentation purposes?

Done.

https://codereview.chromium.org/243463005/diff/2/src/utils/mac/SkCreateCGImag...
src/utils/mac/SkCreateCGImageRef.cpp:59: const bool opaque = kOpaque_SkAlphaType
== bm.alphaType();
On 2014/04/22 14:58:17, scroggo wrote:
> It seems like this could go in the 4444 case (the only place it's used).

Done.

https://codereview.chromium.org/243463005/diff/2/src/utils/mac/SkCreateCGImag...
src/utils/mac/SkCreateCGImageRef.cpp:60: const bool premul =
kUnpremul_SkAlphaType != bm.alphaType();
On 2014/04/22 14:58:17, scroggo wrote:
> This variable is unused.

Done.

[scroggo, 2014-04-22 17:54:44]

On 2014/04/22 17:51:34, reed1 wrote:
> https://codereview.chromium.org/243463005/diff/2/gm/aaclip.cpp
> File gm/aaclip.cpp (right):
> 
> https://codereview.chromium.org/243463005/diff/2/gm/aaclip.cpp#newcode178
> gm/aaclip.cpp:178: virtual uint32_t onGetFlags() const { return
kSkipPipe_Flag;
> }
> On 2014/04/22 14:58:17, scroggo wrote:
> > SK_OVERRIDE
> 
> Done.
> 
>
https://codereview.chromium.org/243463005/diff/2/src/utils/mac/SkCreateCGImag...
> File src/utils/mac/SkCreateCGImageRef.cpp (right):
> 
>
https://codereview.chromium.org/243463005/diff/2/src/utils/mac/SkCreateCGImag...
> src/utils/mac/SkCreateCGImageRef.cpp:12: static uint32_t
> ComputeCGAlphaInfo_RGBA(SkAlphaType at) {
> On 2014/04/22 14:58:17, scroggo wrote:
> > Should these return CGBitmapInfo for documentation purposes?
> 
> Done.
> 
>
https://codereview.chromium.org/243463005/diff/2/src/utils/mac/SkCreateCGImag...
> src/utils/mac/SkCreateCGImageRef.cpp:59: const bool opaque =
kOpaque_SkAlphaType
> == bm.alphaType();
> On 2014/04/22 14:58:17, scroggo wrote:
> > It seems like this could go in the 4444 case (the only place it's used).
> 
> Done.
> 
>
https://codereview.chromium.org/243463005/diff/2/src/utils/mac/SkCreateCGImag...
> src/utils/mac/SkCreateCGImageRef.cpp:60: const bool premul =
> kUnpremul_SkAlphaType != bm.alphaType();
> On 2014/04/22 14:58:17, scroggo wrote:
> > This variable is unused.
> 
> Done.

lgtm

[reed1, 2014-04-22 18:50:29]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-04-22 18:50:46]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/reed@google.com/243463005/90001

[commit-bot: I haz the power, 2014-04-22 20:24:37]

Change committed as 14315