Add bitmap manipulation functions in advance of themes.

Add bitmap manipulation functions in advance of themes.

HSLToSkColor premultiplies its output, should this instead live in the ImageOperations bitmap munging code?

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=14712

[brettw, 2009-04-20 06:30:48]

http://codereview.chromium.org/79082/diff/32/34
File skia/ext/image_operations.cc (right):

http://codereview.chromium.org/79082/diff/32/34#newcode420
Line 420: for (int y = 0; y < bitmap.height(); y++) {  // Skip top and bottom
edges
Can you check that your comments end in periods?

http://codereview.chromium.org/79082/diff/32/34#newcode450
Line 450: // For this pixel in the new image, set the gray value
I don't know what you mean by "gray value". Since you're just setting the
computed values, does this even need a comment (or much of one)?

http://codereview.chromium.org/79082/diff/32/34#newcode507
Line 507: if (hsl_shift[1] <= 0.5) {
Your algorithm assumes SkScalar == float. I'd say that you should just make
everything float.

http://codereview.chromium.org/79082/diff/32/35
File skia/ext/image_operations.h (right):

http://codereview.chromium.org/79082/diff/32/35#newcode62
Line 62: // Blur a bitmap.
Can you say what blurring algorithm it uses?

Also, can you mention that the blur is wrapped around when it goes off the
edges?

http://codereview.chromium.org/79082/diff/32/35#newcode66
Line 66: // 0.5 being 'no change'.
CAn you describe the values in hsl_shift? What does the value mean, does it
wrap?

http://codereview.chromium.org/79082/diff/32/35#newcode73
Line 73: static SkBitmap CreateCroppedBitmap(const SkBitmap& bitmap,
I think a better name would be CreateTiledBitmap.

http://codereview.chromium.org/79082/diff/32/38
File skia/ext/image_operations_unittest.cc (right):

http://codereview.chromium.org/79082/diff/32/38#newcode206
Line 206: reinterpret_cast<unsigned char*>(alpha.getAddr32(0, 0));
Indent 2 more spaces.

http://codereview.chromium.org/79082/diff/32/38#newcode226
Line 226: int alpha = (alpha_pixel & 0xff000000) >> 24;
These shifts will fail on platforms where the RGBA order is different. You
should use SK_?32_SHIFT in SkUserConfig instead.

http://codereview.chromium.org/79082/diff/32/38#newcode350
Line 350: SkDoubleToScalar(-1)};
I think your indenting is all off here. Plus, I'd put } on the next line like
you did above.

[Glen Murphy, 2009-04-23 02:09:49]

Done.

Can you verify that premultiplying the HSLToSkColor output is correct (vs. doing
it as needed in ImageOperations).

[brettw, 2009-04-28 02:49:43]

LGTM with these fixes.

http://codereview.chromium.org/79082/diff/2001/3002
File skia/ext/image_operations.cc (right):

http://codereview.chromium.org/79082/diff/2001/3002#newcode532
Line 532: SkBitmap ImageOperations::CreateTiledBitmap(const SkBitmap& source,
FWIW this could have been done by making a tiled bitmap shader and just drawing
the bitmap to the canvas using Skia. But your approach is fine.

http://codereview.chromium.org/79082/diff/2001/3003
File skia/ext/image_operations.h (right):

http://codereview.chromium.org/79082/diff/2001/3003#newcode58
Line 58: // in the alpha bitmap.
Can you add in this comment whether the images can be different sizes and what
will happen in this case?

http://codereview.chromium.org/79082/diff/2001/3005
File skia/ext/skia_utils.h (right):

http://codereview.chromium.org/79082/diff/2001/3005#newcode25
Line 25: void SkColorToHSL(SkColor c, float hsl[3]);
Since the colors are premultiplied, you should use SkPMColor here, below, and in
image_operations internally when you read the values. That way the
premultiplication aspect of it (which is fine) is self-documenting and
type-checked.