Split off SkConsistentChecksum from SkChecksum

Split off SkConsistentChecksum from SkChecksum
as part of https://goto.google.com/ImprovingTheSkiaRebaseliningProcess
Committed: https://code.google.com/p/skia/source/detail?r=6270

[reed1, 2012-11-01 19:59:53]

can we put the checksum header/code in test/ as well. The fewer public "core"
APIs the better...

[reed1, 2012-11-01 20:05:31]

Had another thought today. We will likely need to checksum the pixels in a
color-component-order well-defined way. The upshot is that we can't assume that
a red pixel on one platform will be the same bytes as a red pixel on another
platform (this is why we get funny colors when we save a .skp from linux and
show it on mac).

I think the checksum loop needs to know how to swizzle a given pixel (int) into
some canonical order before it adds it to its accumulator.

Should not be hard; may just need a macro to be called in your loop: e.g.

for (loop through all the pixels in a row) {
    SkPMColor pixel = *row++;
    uint32_t canonical_pixel = SwizzlePMColorIntoChecksumOrder(pixel);
    accumulator = add_to_checksum(accumulator, canonical_pixel);
}

Where SwizzlePMColorIntoChecksumOrder(x) is defined in terms of the shift values
in SkColorPriv.h. e.g.

// returns color components in packed a,r,g,b order
//
uint32_t SwizzlePMColorIntoChecksumOrder(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;
}

[epoger, 2012-11-02 14:12:15]

On 2012/11/01 20:05:31, reed1 wrote:
> Had another thought today. We will likely need to checksum the pixels in a
> color-component-order well-defined way. The upshot is that we can't assume
that
> a red pixel on one platform will be the same bytes as a red pixel on another
> platform (this is why we get funny colors when we save a .skp from linux and
> show it on mac).
> 
> I think the checksum loop needs to know how to swizzle a given pixel (int)
into
> some canonical order before it adds it to its accumulator.

Gotcha; thanks for the warning.  This brings a few questions to my mind:

1. Do we want to provide a general-purpose, consistent checksumming algorithm,
or is it better to think of this as a checksum for just SkBitmaps?  (Either way,
we can't put it in test/, because it's needed by gm.  Maybe utils/ ?)

2. Maybe this is a vote in favor of encoding into PNG format before taking the
checksum?  (Because the PNG format takes care of issues like the one you
describe.)  Or, do we (should we) have a lightweight format that just represents
pixels in a simple, cross-platform-consistent fashion, and then take the
checksum of that?  It sounds like that lightweight format might also be useful
for .skp encoding.
> 
> Should not be hard; may just need a macro to be called in your loop: e.g.
> 
> for (loop through all the pixels in a row) {
>     SkPMColor pixel = *row++;
>     uint32_t canonical_pixel = SwizzlePMColorIntoChecksumOrder(pixel);
>     accumulator = add_to_checksum(accumulator, canonical_pixel);
> }
> 
> Where SwizzlePMColorIntoChecksumOrder(x) is defined in terms of the shift
values
> in SkColorPriv.h. e.g.
> 
> // returns color components in packed a,r,g,b order
> //
> uint32_t SwizzlePMColorIntoChecksumOrder(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;
> }

[reed1, 2012-11-02 14:29:11]

On Fri, Nov 2, 2012 at 10:12 AM,  <epoger@google.com> wrote:
> On 2012/11/01 20:05:31, reed1 wrote:
>>
>> Had another thought today. We will likely need to checksum the pixels
>
> in a
>>
>> color-component-order well-defined way. The upshot is that we can't
>
> assume that
>>
>> a red pixel on one platform will be the same bytes as a red pixel on
>
> another
>>
>> platform (this is why we get funny colors when we save a .skp from
>
> linux and
>>
>> show it on mac).
>
>
>> I think the checksum loop needs to know how to swizzle a given pixel
>
> (int) into
>>
>> some canonical order before it adds it to its accumulator.
>
>
> Gotcha; thanks for the warning.  This brings a few questions to my mind:
>
> 1. Do we want to provide a general-purpose, consistent checksumming
> algorithm, or is it better to think of this as a checksum for just
> SkBitmaps?  (Either way, we can't put it in test/, because it's needed
> by gm.  Maybe utils/ ?)

ok, utils/ (we can't have gm reach across to another directory?)

>
> 2. Maybe this is a vote in favor of encoding into PNG format before
> taking the checksum?  (Because the PNG format takes care of issues like
> the one you describe.)  Or, do we (should we) have a lightweight format
> that just represents pixels in a simple, cross-platform-consistent
> fashion, and then take the checksum of that?  It sounds like that
> lightweight format might also be useful for .skp encoding.

I think a version of my macro is that cross-platform-consistent thing.
For perf and fragile reasons, I think checksuming the raw bitmap is
better.

For .skp, encoding using PNG (or other) seems best, since the size
reduction seems very valuable.

>
>
>> Should not be hard; may just need a macro to be called in your loop:
>
> e.g.
>
>> for (loop through all the pixels in a row) {
>>      SkPMColor pixel = *row++;
>>      uint32_t canonical_pixel = SwizzlePMColorIntoChecksumOrder(pixel);
>>      accumulator = add_to_checksum(accumulator, canonical_pixel);
>> }
>
>
>> Where SwizzlePMColorIntoChecksumOrder(x) is defined in terms of the
>
> shift values
>>
>> in SkColorPriv.h. e.g.
>
>
>> // returns color components in packed a,r,g,b order
>> //
>> uint32_t SwizzlePMColorIntoChecksumOrder(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;
>> }
>
>
>
>
> https://codereview.appspot.com/6820074/

[epoger, 2012-11-02 14:59:18]

On 2012/11/02 14:29:11, reed1 wrote:
> ok, utils/ (we can't have gm reach across to another directory?)

We could, but especially if we think the consistent checksumming capability
might be handy for other purposes, I think tests/ would be a strange place to
put it.

In patchset 2 I have moved it into utils/ (there are no other changes in
patchset 2)

> >> for (loop through all the pixels in a row) {
> >>      SkPMColor pixel = *row++;
> >>      uint32_t canonical_pixel = SwizzlePMColorIntoChecksumOrder(pixel);
> >>      accumulator = add_to_checksum(accumulator, canonical_pixel);
> >> }

I haven't added this add_to_checksum() capability to SkConsistentChecksum.h yet.
 My inclination is to commit this CL first and then add that interface in
another CL.  Let me know if you disagree...

[reed1, 2012-11-02 15:19:10]

I always push to make headers as private/hidden as possible, and only make them
public when we have to, hence my suggestion about test/. Much easier to promote
APIs than to demote them. As a compromise, how about we store this guy in
src/utils instead of include/utils initially.

API

Given that we know we need to swizzle values, and therefore the computed
checksum is SkPMColor specific, perhaps the utility might as well be
bitmap-specific.

e.g.

uint32_t compute_bitmap_checksum(const SkBitmap&);

1. makes it clear what its for
2. can handle swizzing under the hood
3. handles the trimming of row-bytes for each scanline

[epoger, 2012-11-02 15:45:25]

On 2012/11/02 15:19:10, reed1 wrote:
> I always push to make headers as private/hidden as possible, and only make
them
> public when we have to, hence my suggestion about test/. Much easier to
promote
> APIs than to demote them. As a compromise, how about we store this guy in
> src/utils instead of include/utils initially.

Makes sense.  In patchset 3, I have moved it to src/utils.

> 
> API
> 
> Given that we know we need to swizzle values, and therefore the computed
> checksum is SkPMColor specific, perhaps the utility might as well be
> bitmap-specific.
> 
> e.g.
> 
> uint32_t compute_bitmap_checksum(const SkBitmap&);
> 
> 1. makes it clear what its for
> 2. can handle swizzing under the hood
> 3. handles the trimming of row-bytes for each scanline

Will do.  If it's OK with you, I'd like to commit this CL *without* that API for
now, and add it in a separate CL.

[reed1, 2012-11-02 18:32:05]

lgtm

[epoger, 2012-11-02 18:34:39]

On 2012/11/02 18:32:05, reed1 wrote:
> lgtm

Thanks. Next I will add the compute_bitmap_checksum(const SkBitmap&) function
alongside the existing Compute(const uint32_t* data, size_t size).  And maybe
we'll decide to get rid of Compute()... but at least we'll see whether the
algorithm works across platforms...

[reed1, 2012-11-02 18:36:18]

sure thing. though since we need to swizzle each int, I'm not sure we can use
the ptr/length version for our use case.