Rename SkCanvas::Config8888 enum as SkBitmap::ExternalConfig.

Rename SkCanvas::Config8888 enum as SkBitmap::ExternalConfig.

[epoger, 2012-12-07 18:27:15]

This is step 1 of the transformation detailed in
https://codereview.appspot.com/6849119/#msg6 .

The idea here is to broaden the set of pixel formats described by Config8888, so
that we can use that one enumeration to describe all of our pixel-format
transformations.

I also think it makes more sense for the ExternalConfigs to be enumerated right
next to the (internal) Configs in SkBitmap.h, rather than in SkCanvas.h.

https://codereview.appspot.com/6900047/diff/1/include/core/SkBitmap.h
File include/core/SkBitmap.h (right):

https://codereview.appspot.com/6900047/diff/1/include/core/SkBitmap.h#newcode87
include/core/SkBitmap.h:87: kARGB_8888_Premul_ExternalConfig,
Note that I renamed these 2 from "kNative_xxx" to "kARGB_xxx".  I figured
calling code was more concerned with the byte order, and less concerned about
which byte order happens to be Skia's native...

https://codereview.appspot.com/6900047/diff/1/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

https://codereview.appspot.com/6900047/diff/1/include/core/SkCanvas.h#newcode119
include/core/SkCanvas.h:119: * TODO(epoger): ASAP: update all code within the
Skia repo to use
I will create a second CL to do this right away, but I wanted to get feedback on
the "meat" of the change first.

[epoger, 2012-12-07 18:31:14]

https://codereview.appspot.com/6900047/diff/1/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

https://codereview.appspot.com/6900047/diff/1/include/core/SkCanvas.h#newcode122
include/core/SkCanvas.h:122: * TODO: Update Chrome/WebKit code that uses this to
point at the
P.S. Chromium codesearch reveals these callers that would need to be updated
before deleting the alias:

src/ui/aura/root_window_host_linux.cc
src/third_party/WebKit/Source/WebCore/platform/graphics/skia/PlatformContextSkia.h
src/third_party/WebKit/Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp

[bsalomon, 2012-12-07 19:12:12]

https://codereview.appspot.com/6900047/diff/1/include/core/SkBitmap.h
File include/core/SkBitmap.h (right):

https://codereview.appspot.com/6900047/diff/1/include/core/SkBitmap.h#newcode87
include/core/SkBitmap.h:87: kARGB_8888_Premul_ExternalConfig,
On 2012/12/07 18:27:15, epoger wrote:
> Note that I renamed these 2 from "kNative_xxx" to "kARGB_xxx".  I figured
> calling code was more concerned with the byte order, and less concerned about
> which byte order happens to be Skia's native...

Maybe we should just get rid of them if they aren't being used other than in our
tests. This name is definitely not right because the byte order may or may not
be ARGB as the name suggests.

[reed1, 2012-12-07 19:14:30]

I am hesitant to expand our public api in this area just yet, esp. for types
that only work with devices right now, and that don't necessarily reflect our
(fuzzy) future plans for SkBitmap.

What is the meta goal for this change?

[epoger, 2012-12-07 19:45:49]

On 2012/12/07 19:14:30, reed1 wrote:
> I am hesitant to expand our public api in this area just yet

Understandable...
 
> What is the meta goal for this change?

Here's where I started and how we got here:

1. I'm trying to add the ability to generate a checksum, given an SkBitmap. 
Ideally this checksum would be valid across platforms (so that a pixel-identical
image on another platform would yield the same checksum).

2. Thus, I need to get the pixel data out of an SkBitmap in a
cross-platform-consistent format.  This started email thread "should we combine
SkConfig8888.cpp and transform_scanline.h ?", which noted that similar
functionality is replicated across src/images/SkImageDecoder_libpng.cpp and
src/core/SkConfig8888.cpp .

3. I wrote an attempt at implementing this in
https://codereview.appspot.com/6849119/ ('Add exportAllPixels() and
exportRowPixels() to SkBitmap.').  In that review, Brian wrote: "I think we
should use SkCanvas::Config8888 here (moving or renaming it if desired) rather
than adding yet another config/format enum."

4. In response to that comment, I wrote up the proposal at
https://codereview.appspot.com/6849119/#msg6 .  Since there wasn't objection, I
got to work.


Alternatively, I could create a private SkBitmapTransformer class that has
nothing to do with the SkConfig8888.cpp transformations (it would be used for
image checksums and PNG encoding, but not SkCanvas::readPixels() ).  It would
have its own enumeration of formats, like I created in
https://codereview.appspot.com/6849119/ , which would overlap
SkCanvas::Config8888 (that was Brian's objection) .  But at least it would be
more generally useful than the pixel-swizzlers in
https://code.google.com/p/skia/source/browse/trunk/src/images/transform_scanl...
, and would not mess with the public Config8888 enum.

But I'm open to anything, as long as I can get this checksum functionality in
somehow or other.

[reed1, 2012-12-07 20:08:44]

Seems like its very straight-forward to swizzle SkPMColor into a canonical
format, without the additional machinations of unpremul.

uint32_t pmcolor_2_argb(SkPMColor c) {
    unsigned a = SkGetPackedA32(c);
    unsigned r = SkGetPackedR32(c);
    unsigned g = SkGetPackedG32(c);
    unsigned b = SkGetPackedB32(c);
    return (a << 24) | (r << 16) | (g << 8) | b;
}

This should return the same bytes on all platforms.

[epoger, 2012-12-07 20:55:55]

On 2012/12/07 20:08:44, reed1 wrote:
> Seems like its very straight-forward to swizzle SkPMColor into a canonical
> format, without the additional machinations of unpremul.
> 
> uint32_t pmcolor_2_argb(SkPMColor c) {
>     unsigned a = SkGetPackedA32(c);
>     unsigned r = SkGetPackedR32(c);
>     unsigned g = SkGetPackedG32(c);
>     unsigned b = SkGetPackedB32(c);
>     return (a << 24) | (r << 16) | (g << 8) | b;
> }
> 
> This should return the same bytes on all platforms.

Sure, if we are willing to accept the constraint that the source SkBitmap has
config kARGB_8888_Config.  Which is OK in this particular case (gm will still
perform reasonably well if I have to make an intermediate 8888 copy of the
bitmap).

Beyond that, there is still the matter of figuring out how big the dest buffer
needs to be to hold the entire image, walking the scanlines, etc.  Of course, we
know how to do it, but it seems to me it would be better to make routines that
could be shared between the checksummer, the PNG encoder, and future consumers.

Toward that end, I will complete the implementation of SkBitmapTransformer as
proposed in
https://codereview.appspot.com/6843123/diff/1/src/core/SkBitmapTransformer.h ,
with these changes: 
1. instead of allowing for selectable PixelBufferFormat, it will assume ARGB8888
2. it will only export pixels from the SkBitmap, not import them

If we want to generalize it later, we can.

[epoger, 2012-12-12 17:24:20]

Closing this one in favor of https://codereview.appspot.com/6920050/ ('Create
SkBitmapChecksummer and associated SkBitmapTransformer'), just committed as
https://code.google.com/p/skia/source/detail?r=6759 .