Issue 6920050: Create SkBitmapChecksummer and associated SkBitmapTransformer

Issue 6920050: Create SkBitmapChecksummer and associated SkBitmapTransformer (Closed)

Can't Edit
Can't Publish+Mail
Start Review

Created:
11 years, 6 months ago by epoger

Modified:
11 years, 6 months ago

Reviewers:
reed1

CC:
skia-review_googlegroups.com, EricB, Leon

Base URL:
http://skia.googlecode.com/svn/trunk/

Visibility:
Public.

More Reviews

Description

Create SkBitmapChecksummer and associated SkBitmapTransformer As needed to start capturing gm image checksums. Committed: https://code.google.com/p/skia/source/detail?r=6759

Patch Set 2 : tiny_comment-only_changes #

Total comments: 7

Patch Set 3 : #

Created: 11 years, 6 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+431 lines, -0 lines)			Patch
M	gyp/tests.gyp	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	gyp/utils.gyp	View		1 chunk	+4 lines, -0 lines	0 comments	Download
A	src/utils/SkBitmapChecksummer.h	View		1 chunk	+37 lines, -0 lines	0 comments	Download
A	src/utils/SkBitmapChecksummer.cpp	View	1 2	1 chunk	+69 lines, -0 lines	0 comments	Download
A	src/utils/SkBitmapTransformer.h	View	1 2	1 chunk	+104 lines, -0 lines	0 comments	Download
A	src/utils/SkBitmapTransformer.cpp	View	1 2	1 chunk	+87 lines, -0 lines	0 comments	Download
A	tests/BitmapTransformerTest.cpp	View	1 2	1 chunk	+97 lines, -0 lines	0 comments	Download
M	tests/ChecksumTest.cpp	View		3 chunks	+32 lines, -0 lines	0 comments	Download

Messages

Total messages: 4

Expand All Messages | Collapse All Messages

reed1

Does BitmapTransformer need to be an object, instead of just a set of static functions? ...

11 years, 6 months ago (2012-12-11 21:55:54 UTC) #2

epoger

Addressed 'em all in patchset 3... https://codereview.appspot.com/6920050/diff/3001/src/utils/SkBitmapChecksummer.cpp File src/utils/SkBitmapChecksummer.cpp (right): https://codereview.appspot.com/6920050/diff/3001/src/utils/SkBitmapChecksummer.cpp#newcode17 src/utils/SkBitmapChecksummer.cpp:17: */ On 2012/12/11 ...

11 years, 6 months ago (2012-12-12 17:19:54 UTC) #3

Addressed 'em all in patchset 3...

https://codereview.appspot.com/6920050/diff/3001/src/utils/SkBitmapChecksumme...
File src/utils/SkBitmapChecksummer.cpp (right):

https://codereview.appspot.com/6920050/diff/3001/src/utils/SkBitmapChecksumme...
src/utils/SkBitmapChecksummer.cpp:17: */
On 2012/12/11 21:55:54, reed1 wrote:
> static local function typically are lower_case, or possibly lowerCase, but
never
> (afaik) UpperCase.

Done.

https://codereview.appspot.com/6920050/diff/3001/src/utils/SkBitmapTransformer.h
File src/utils/SkBitmapTransformer.h (right):

https://codereview.appspot.com/6920050/diff/3001/src/utils/SkBitmapTransforme...
src/utils/SkBitmapTransformer.h:33: class SkBitmapTransformer {
On 2012/12/11 21:55:54, reed1 wrote:
> Does BitmapTransformer need to be an object, instead of just a set
> of static functions? I ask partly because it seems to have only 1
> interested method, and does not seem stateful (in the sense of its
> state changing over time). Perhaps the future vision of this would
> make that design choice clearer?

Certainly, as it is used now, a set of static functions would suffice.  But if
the cost of it being an object instead is low (which I think it is), I like that
it would allow development in this direction:

- Add method to copy pixel data for a single row
- Add support for PixelFormats other than kARGB_8888_Premul_PixelFormat and/or
Configs other than kARGB_8888_Config
- PixelFormat/Config-specific setup in the constructor can be amortized over
multiple calls to copy pixel data for individual rows

That would allow us to replace code that is duplicated here as well as in
src/core/SkConfig8888.h and src/images/SkImageDecoder_libpng.cpp .  Which we may
or may not want to do, but I like the idea of it being possible...

So that's my reasoning.  If, given the above, you would materially prefer a set
of static functions, let me know and I will change it.

(For my edification, what's the cost of it being set up as an object?  Are there
performance concerns?)

https://codereview.appspot.com/6920050/diff/3001/src/utils/SkBitmapTransforme...
src/utils/SkBitmapTransformer.h:40: // marks the end of the list
On 2012/12/11 21:55:54, reed1 wrote:
> our convention for kLast is that it is equal to the last (other) enum in the
> list.
> 
> e.g.
> 
> kLast_PixelFormat = kARGB_8888_Premul_PixelFormat

Done.

https://codereview.appspot.com/6920050/diff/3001/src/utils/SkBitmapTransforme...
src/utils/SkBitmapTransformer.h:61: */
On 2012/12/11 21:55:54, reed1 wrote:
> method names begin with lowerCase.
> Static names begin with UpperCase.

Fixed throughout the CL.  Also, following
https://sites.google.com/site/skiadocs/developer-documentation/contributing-c...
, prefixed method calls within method calls with "this->".

lgtm

Expand All Messages | Collapse All Messages