ARGB image encoder for checksums.

ARGB image encoder for checksums.

[bungeman-skia, 2013-04-19 19:56:01]

This is was I would like to see against current so that  calculating the
checksums with MD5/SHA1 is saner.

[epoger, 2013-04-19 21:13:02]

LGTM with some relatively small suggestions...

Thanks for taking the trouble... I'm not sure how this will make
https://codereview.chromium.org/14265010/ much "saner", though.  AFAICT it will
only affect a few lines in that CL.

https://codereview.chromium.org/14267031/diff/7001/src/utils/SkBitmapHasher.cpp
File src/utils/SkBitmapHasher.cpp (right):

https://codereview.chromium.org/14267031/diff/7001/src/utils/SkBitmapHasher.c...
src/utils/SkBitmapHasher.cpp:16: * Write an integer value into a bytebuffer in
little-endian order.
"into a bytebuffer" -> "to a stream"

https://codereview.chromium.org/14267031/diff/7001/src/utils/SkBitmapHasher.c...
src/utils/SkBitmapHasher.cpp:40: SkAutoTDelete<SkImageEncoder>
enc(CreateARGBImageEncoder());
Is there a significant performance penalty for creating an SkARGBImageEncoder
every time we call ComputeDigestInternal()?  If not, this should be fine...

https://codereview.chromium.org/14267031/diff/7001/src/utils/SkBitmapHasher.c...
src/utils/SkBitmapHasher.cpp:41: if (!enc->encodeStream(out, bitmap, 100)) {
Where does the magic "100" come from?  Can we refer to a constant variable
instead?

https://codereview.chromium.org/14267031/diff/7001/src/utils/SkBitmapHasher.c...
src/utils/SkBitmapHasher.cpp:53: // Hmm, that didn't work. Maybe if we create a
new
I don't know why we would encounter this anymore, now that the transformer can
handle lots of different input configs.  But I guess there's no harm...

https://codereview.chromium.org/14267031/diff/7001/tests/BitmapTransformerTes...
File tests/BitmapTransformerTest.cpp (right):

https://codereview.chromium.org/14267031/diff/7001/tests/BitmapTransformerTes...
tests/BitmapTransformerTest.cpp:8: /** Tests for RAWARGBImageEncoder. */
"RAWARGBImageEncoder" -> "SkARGBImageEncoder" ?

And maybe this file should be renamed as ARGBImageEncoderTest?

https://codereview.chromium.org/14267031/diff/7001/tests/BitmapTransformerTes...
tests/BitmapTransformerTest.cpp:41:
bitmap.setConfig(SkBitmap::kARGB_8888_Config, kWidth, kHeight);
Since SkARGBImageEncoder supports input configs other than 8888, can you please
add tests that exercise them?  I think you can just extract this block as a
method, pass in the config as a parameter, and return the generated SkBitmap.

https://codereview.chromium.org/14267031/diff/7001/tests/BitmapTransformerTes...
tests/BitmapTransformerTest.cpp:65: REPORTER_ASSERT(reporter,
enc->encodeStream(out, bitmap, 100));
magic "100" again, please use a constant variable if possible

[bungeman-skia, 2013-04-22 18:31:13]

I was interested in the performance of the digests, so added some temporary
changes here to gmmain.cpp and the bitmap hasher. On my machine I get something
like

const 54424 msecs  + 0, 1.00
city  58738 msecs  + 4, 1.08
md5   66461 msecs  +12, 1.22
sha1  74538 msecs  +20, 1.37

for runs of gm. So ~8% tacked on for city hash and ~20% for md5 over a constant
'hash'. I'm not sure whether to wonder that the hashes are so slow or that Skia
is so fast. In any event, this seems a bit less disparate than what was seen at
https://code.google.com/p/skia/issues/detail?id=1257 .

One other thing I noticed is that aside from verylargebitmap and maybe one or
two other gms, I never saw even one cpu peg. We should look into threading gm.

https://codereview.chromium.org/14267031/diff/7001/src/utils/SkBitmapHasher.cpp
File src/utils/SkBitmapHasher.cpp (right):

https://codereview.chromium.org/14267031/diff/7001/src/utils/SkBitmapHasher.c...
src/utils/SkBitmapHasher.cpp:16: * Write an integer value into a bytebuffer in
little-endian order.
On 2013/04/19 21:13:02, epoger wrote:
> "into a bytebuffer" -> "to a stream"

Done.

https://codereview.chromium.org/14267031/diff/7001/src/utils/SkBitmapHasher.c...
src/utils/SkBitmapHasher.cpp:41: if (!enc->encodeStream(out, bitmap, 100)) {
On 2013/04/19 21:13:02, epoger wrote:
> Where does the magic "100" come from?  Can we refer to a constant variable
> instead?

Ah, you need a good editor which will show documentation as you
type/hover/navigate.

"at quality level 'quality' (which can be in range 0-100)"

However, changed to use SkImageEncoder::kDefaultQuality.

https://codereview.chromium.org/14267031/diff/7001/src/utils/SkBitmapHasher.c...
src/utils/SkBitmapHasher.cpp:53: // Hmm, that didn't work. Maybe if we create a
new
On 2013/04/19 21:13:02, epoger wrote:
> I don't know why we would encounter this anymore, now that the transformer can
> handle lots of different input configs.  But I guess there's no harm...

Well, this doesn't support A8 or A1, since I didn't know what to do with them.
On the other hand, bitmap.copyTo doesn't handle A1, and seems a bit fuzzy about
A8 itself.

https://codereview.chromium.org/14267031/diff/7001/tests/BitmapTransformerTes...
File tests/BitmapTransformerTest.cpp (right):

https://codereview.chromium.org/14267031/diff/7001/tests/BitmapTransformerTes...
tests/BitmapTransformerTest.cpp:8: /** Tests for RAWARGBImageEncoder. */
On 2013/04/19 21:13:02, epoger wrote:
> "RAWARGBImageEncoder" -> "SkARGBImageEncoder" ?
> 
> And maybe this file should be renamed as ARGBImageEncoderTest?

Done.

https://codereview.chromium.org/14267031/diff/7001/tests/BitmapTransformerTes...
tests/BitmapTransformerTest.cpp:41:
bitmap.setConfig(SkBitmap::kARGB_8888_Config, kWidth, kHeight);
On 2013/04/19 21:13:02, epoger wrote:
> Since SkARGBImageEncoder supports input configs other than 8888, can you
please
> add tests that exercise them?  I think you can just extract this block as a
> method, pass in the config as a parameter, and return the generated SkBitmap.

Done.

https://codereview.chromium.org/14267031/diff/7001/tests/BitmapTransformerTes...
tests/BitmapTransformerTest.cpp:65: REPORTER_ASSERT(reporter,
enc->encodeStream(out, bitmap, 100));
On 2013/04/19 21:13:02, epoger wrote:
> magic "100" again, please use a constant variable if possible

Done.

[epoger, 2013-04-22 19:53:28]

LGTM, sort of.

As detailed below, I believe this is significantly less efficient than the
existing code because SkMemoryWStream has to guess at how large of a buffer to
allocate.

I do think the code is cleaner, though, so maybe the thing to do is commit it
and see if gm performance is degraded?  If so, we can revert it... and if not,
it's a win.

https://codereview.chromium.org/14267031/diff/11001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/14267031/diff/11001/gm/gmmain.cpp#newcode1594
gm/gmmain.cpp:1594: #include "SkTime.h"
You're gonna want to revert all changes to this file before committing. :-)

https://codereview.chromium.org/14267031/diff/11001/src/utils/SkBitmapHasher.cpp
File src/utils/SkBitmapHasher.cpp (right):

https://codereview.chromium.org/14267031/diff/11001/src/utils/SkBitmapHasher....
src/utils/SkBitmapHasher.cpp:56: if (!enc->encodeStream(&out, bitmap,
SkImageEncoder::kDefaultQuality)) {
So, I guess SkMemoryWStream is going to keep reallocating its memory buffer
until it is large enough to hold the entire transformed bitmap, right?

Doesn't that seem less efficient than my previous code, in which we call
transformer.bytesNeededTotal() and allocate a large enough chunk in one go?

Even better is the code in
https://codereview.chromium.org/14265010/diff2/53019:12005/src/utils/SkBitmap...
, where we only need to allocate a chunk large enough for a single row of
pixels.

https://codereview.chromium.org/14267031/diff/11001/src/utils/SkBitmapHasher....
src/utils/SkBitmapHasher.cpp:65: *result = *((SkHashDigest*)&digest);
Since SkHashDigest is just a typedef for uint64_t, this is going to truncate the
16-byte-long MD5 hash digest, right?  That may account for why you didn't see as
big of a runtime difference as I did in
https://code.google.com/p/skia/issues/detail?id=1257 ('deciding between
CityHash, MD5, and SHA1 for image hash algorithm')

https://codereview.chromium.org/14267031/diff/11001/tests/ARGBImageEncoderTes...
File tests/ARGBImageEncoderTest.cpp (right):

https://codereview.chromium.org/14267031/diff/11001/tests/ARGBImageEncoderTes...
tests/ARGBImageEncoderTest.cpp:48: {
maybe move this section into its own method?

SkBitmap bitmap;
createTestBitmap(configs[configIndex], bitmap);

[epoger, 2013-04-22 19:55:58]

On 2013/04/22 18:31:13, bungeman1 wrote:
> One other thing I noticed is that aside from verylargebitmap and maybe one or
> two other gms, I never saw even one cpu peg. We should look into threading gm.

Maybe, maybe not.  It really depends on the situation for the constrained
platforms (Android).  Worth investigating, though...

[bungeman-skia, 2013-04-22 20:08:00]

https://codereview.chromium.org/14267031/diff/11001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/14267031/diff/11001/gm/gmmain.cpp#newcode1594
gm/gmmain.cpp:1594: #include "SkTime.h"
On 2013/04/22 19:53:28, epoger wrote:
> You're gonna want to revert all changes to this file before committing. :-)

Yes, I just wanted to have this here so it cold be seen, instead of pulling a
diff out of the skia try project.

https://codereview.chromium.org/14267031/diff/11001/src/utils/SkBitmapHasher.cpp
File src/utils/SkBitmapHasher.cpp (right):

https://codereview.chromium.org/14267031/diff/11001/src/utils/SkBitmapHasher....
src/utils/SkBitmapHasher.cpp:56: if (!enc->encodeStream(&out, bitmap,
SkImageEncoder::kDefaultQuality)) {
On 2013/04/22 19:53:28, epoger wrote:
> So, I guess SkMemoryWStream is going to keep reallocating its memory buffer
> until it is large enough to hold the entire transformed bitmap, right?

No. We're only using the SkMemoryWStream in the CityHash case, and we're
pre-allocating the memory as ARGBImageEncoder advertises the amount of memory it
requires. Note that this is an SkMemoryWStream, not a SkDynamicMemoryWStream.

> 
> Doesn't that seem less efficient than my previous code, in which we call
> transformer.bytesNeededTotal() and allocate a large enough chunk in one go?
> 
> Even better is the code in
>
https://codereview.chromium.org/14265010/diff2/53019:12005/src/utils/SkBitmap...
> , where we only need to allocate a chunk large enough for a single row of
> pixels.

The MD5 case just passes the MD5 in, since it is a SkWStream. That's part of its
optimization / convenience, it's stream based, not a one shot hash.

https://codereview.chromium.org/14267031/diff/11001/src/utils/SkBitmapHasher....
src/utils/SkBitmapHasher.cpp:65: *result = *((SkHashDigest*)&digest);
On 2013/04/22 19:53:28, epoger wrote:
> Since SkHashDigest is just a typedef for uint64_t, this is going to truncate
the
> 16-byte-long MD5 hash digest, right?  That may account for why you didn't see
as
> big of a runtime difference as I did in
> https://code.google.com/p/skia/issues/detail?id=1257 ('deciding between
> CityHash, MD5, and SHA1 for image hash algorithm')

I was interested in the difference between MD5 and CityHash, the rest should be
minimal (if it isn't, that needs to be fixed). Truncating a digest is just as
valid, you're just throwing away the rest of the entropy.

https://codereview.chromium.org/14267031/diff/11001/tests/ARGBImageEncoderTes...
File tests/ARGBImageEncoderTest.cpp (right):

https://codereview.chromium.org/14267031/diff/11001/tests/ARGBImageEncoderTes...
tests/ARGBImageEncoderTest.cpp:48: {
On 2013/04/22 19:53:28, epoger wrote:
> maybe move this section into its own method?
> 
> SkBitmap bitmap;
> createTestBitmap(configs[configIndex], bitmap);

I actually think that's a bad idea without adding a lot of other things to this.
This code is highly coupled to the comparisonBuffer above, pulling this code out
makes it look like it isn't.

[epoger, 2013-04-23 15:01:41]

Like I wrote yesterday, I recommend committing this CL as-is and seeing if it
degrades performance.  If not, it's a win; if it does, we can revert it and then
decide what to do.

[epoger, 2013-04-23 16:57:59]

On 2013/04/23 15:01:41, epoger wrote:
> Like I wrote yesterday, I recommend committing this CL as-is and seeing if it
> degrades performance.  If not, it's a win; if it does, we can revert it and
then
> decide what to do.

Ben, do you intend to commit this soon?   As noted in
https://codereview.chromium.org/14265010/ ('Make SkSHA1 and SkM5 use common
SkDigestHash result type'), I'd like to see how this lands, and then pick it up
and run with it.

[bungeman-skia, 2013-04-23 17:27:19]

> Ben, do you intend to commit this soon?   As noted in
> https://codereview.chromium.org/14265010/ ('Make SkSHA1 and SkM5 use common
> SkDigestHash result type'), I'd like to see how this lands, and then pick it
up
> and run with it.

Yes, I'll get this in soon. I was looking to see if there were any MD5
efficiencies which we might be lacking, but the fastest hand assembly x64 MD5
implementation I found was only ~10% faster. Also I was looking to see if there
was a way to use the image encoder interface to decode directly to the stream's
memory instead of needing to do the copy (in the CityHash case, but this doesn't
seem to really change anything).

[epoger, 2013-04-23 18:04:32]

On 2013/04/23 17:27:19, bungeman1 wrote:
> > Ben, do you intend to commit this soon?   As noted in
> > https://codereview.chromium.org/14265010/ ('Make SkSHA1 and SkM5 use common
> > SkDigestHash result type'), I'd like to see how this lands, and then pick it
> up
> > and run with it.
> 
> Yes, I'll get this in soon. I was looking to see if there were any MD5
> efficiencies which we might be lacking, but the fastest hand assembly x64 MD5
> implementation I found was only ~10% faster. Also I was looking to see if
there
> was a way to use the image encoder interface to decode directly to the
stream's
> memory instead of needing to do the copy (in the CityHash case, but this
doesn't
> seem to really change anything).

Sounds good.  I think it would be good for any MD5 implementation enhancements
to be in a separate CL, anyway, leaving this one for the encoder plumbing.

[epoger, 2013-04-23 19:05:57]

Looks like this was committed as
https://code.google.com/p/skia/source/detail?r=8831 .  I dunno why that didn't
show up here automatically.

[bungeman-skia, 2013-04-23 19:17:08]

Committed revision 8831.

[epoger, 2013-04-23 19:32:22]

On 2013/04/23 15:01:41, epoger wrote:
> Like I wrote yesterday, I recommend committing this CL as-is and seeing if it
> degrades performance.  If not, it's a win; if it does, we can revert it and
then
> decide what to do.

So far this change is looking good, performance-wise! Looking at how long it
took to run the GenerateGMs step, in min:sec ...

Skia_Xoom_4-1_Float_Debug_32 improved about 4% (15:35 -> 14:53) :
http://108.170.217.252:10117/builders/Skia_Xoom_4-1_Float_Debug_32/builds/1713
http://108.170.217.252:10117/builders/Skia_Xoom_4-1_Float_Debug_32/builds/1714

Skia_Xoom_4-1_Float_Release_32 improved slightly (4:44 -> 4:38):
http://108.170.217.252:10117/builders/Skia_Xoom_4-1_Float_Release_32/builds/1647
http://108.170.217.252:10117/builders/Skia_Xoom_4-1_Float_Release_32/builds/1648

Skia_Nexus7_4-1_Float_Debug_32 improved about 4% (11:57 -> 11:28) :
http://108.170.217.252:10117/builders/Skia_Nexus7_4-1_Float_Debug_32/builds/1530
http://108.170.217.252:10117/builders/Skia_Nexus7_4-1_Float_Debug_32/builds/1531

Skia_Nexus4_4-1_Float_Debug_32 improved about 7% (11:53 -> 11:04) :
http://108.170.217.252:10117/builders/Skia_Nexus4_4-1_Float_Debug_32/builds/627
http://108.170.217.252:10117/builders/Skia_Nexus4_4-1_Float_Debug_32/builds/628

Skia_RazrI_4-1_Float_Release_32 stayed about the same (3:18 -> 3:17) :
http://108.170.217.252:10117/builders/Skia_RazrI_4-1_Float_Release_32/builds/334
http://108.170.217.252:10117/builders/Skia_RazrI_4-1_Float_Release_32/builds/335

Skia_GalaxyNexus_4-1_Float_Debug_32 improved 3% (19:36 -> 18:51) :
http://108.170.217.252:10117/builders/Skia_GalaxyNexus_4-1_Float_Debug_32/bui...
http://108.170.217.252:10117/builders/Skia_GalaxyNexus_4-1_Float_Debug_32/bui...

[epoger, 2013-04-24 16:30:43]

On 2013/04/23 19:32:22, epoger wrote:
> So far this change is looking good, performance-wise! Looking at how long it
> took to run the GenerateGMs step, in min:sec ...

Just to add results from the Skia_NexusS_4-1_Float_Debug_32 bot, because that's
the one I tried in https://code.google.com/p/skia/issues/detail?id=1257
('deciding between CityHash, MD5, and SHA1 for image hash algorithm') :

Skia_NexusS_4-1_Float_Debug_32 improved 3% (19:03 -> 18:24) :
http://108.170.217.252:10117/builders/Skia_NexusS_4-1_Float_Debug_32/builds/1143
http://108.170.217.252:10117/builders/Skia_NexusS_4-1_Float_Debug_32/builds/1144

So this CL was definitely a win!  Cleaner, and performance got slightly better. 
Thanks, Ben.