Adds a function computing image color covariance.

Adds a function computing image color covariance on an SkBitmap.

BUG=155269
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=182163

[motek., 2013-02-12 18:46:37]

A function computing color covariance on an SKBitmap. 

BUG=155269

[Alexei Svitkine (slow), 2013-02-12 19:19:29]

+reed to review the Skia API use

LGTM with nits from me, but please wait for Mike's Reed review.

https://chromiumcodereview.appspot.com/12220123/diff/1/ui/gfx/color_analysis.cc
File ui/gfx/color_analysis.cc (right):

https://chromiumcodereview.appspot.com/12220123/diff/1/ui/gfx/color_analysis....
ui/gfx/color_analysis.cc:413: int pixel_width = bitmap.width();
Nit: Just use bitmap.width() directly.

https://chromiumcodereview.appspot.com/12220123/diff/1/ui/gfx/color_analysis....
ui/gfx/color_analysis.cc:414: int pixel_height = bitmap.height();
Nit: Just use bitmap.height() directly.

https://chromiumcodereview.appspot.com/12220123/diff/1/ui/gfx/color_analysis....
ui/gfx/color_analysis.cc:415: int pixel_n = pixel_width * pixel_height;
Nit: Move this down closer to where it's being used.

https://chromiumcodereview.appspot.com/12220123/diff/1/ui/gfx/color_analysis.h
File ui/gfx/color_analysis.h (right):

https://chromiumcodereview.appspot.com/12220123/diff/1/ui/gfx/color_analysis....
ui/gfx/color_analysis.h:102: // Compute color covariance matrix for the input
bitmap. 4x4 matrix is used
Your comment about a 4x4 matrix being used for the return type doesn't seem
accurate...

[motek., 2013-02-12 19:28:56]

Addressed nits, waiting for the rest.

https://chromiumcodereview.appspot.com/12220123/diff/1/ui/gfx/color_analysis.cc
File ui/gfx/color_analysis.cc (right):

https://chromiumcodereview.appspot.com/12220123/diff/1/ui/gfx/color_analysis....
ui/gfx/color_analysis.cc:413: int pixel_width = bitmap.width();
On 2013/02/12 19:19:29, Alexei Svitkine wrote:
> Nit: Just use bitmap.width() directly.

Done.

https://chromiumcodereview.appspot.com/12220123/diff/1/ui/gfx/color_analysis....
ui/gfx/color_analysis.cc:414: int pixel_height = bitmap.height();
On 2013/02/12 19:19:29, Alexei Svitkine wrote:
> Nit: Just use bitmap.height() directly.

Done.

https://chromiumcodereview.appspot.com/12220123/diff/1/ui/gfx/color_analysis....
ui/gfx/color_analysis.cc:415: int pixel_n = pixel_width * pixel_height;
On 2013/02/12 19:19:29, Alexei Svitkine wrote:
> Nit: Move this down closer to where it's being used.

Done.

https://chromiumcodereview.appspot.com/12220123/diff/1/ui/gfx/color_analysis.h
File ui/gfx/color_analysis.h (right):

https://chromiumcodereview.appspot.com/12220123/diff/1/ui/gfx/color_analysis....
ui/gfx/color_analysis.h:102: // Compute color covariance matrix for the input
bitmap. 4x4 matrix is used
On 2013/02/12 19:19:29, Alexei Svitkine wrote:
> Your comment about a 4x4 matrix being used for the return type doesn't seem
> accurate...

Done.

[reed1, 2013-02-12 19:34:51]

https://chromiumcodereview.appspot.com/12220123/diff/7001/ui/gfx/color_analys...
File ui/gfx/color_analysis.cc (right):

https://chromiumcodereview.appspot.com/12220123/diff/7001/ui/gfx/color_analys...
ui/gfx/color_analysis.cc:423: for (int y = 0; y < bitmap.height(); ++y) {
SkBitmap stores its pixels in SkPMColor format, not SkColor. You need to use
SkPMColor type in this code, and use the macros in SkColorPriv.h to extract
r,g,b bytes.

SkColor is always in a,r,g,b order as an int (e.g. a << 24, r << 16, g << 8) and
is unpremultiplied.

SkPMColor's byte order is variable, defined in SkColorPriv.h, and can change on
different platforms. Use SkGetPackedR32() etc. to extract r,g,b. Also, note that
these values will already be "pre" multiplied by that pixel's alpha
(SkGetPackedA32).

I don't know (1) if this function is ever called w/ bitmaps that have per-pixel
alpha, and (2) if that has any bearing on its covariance...

[motek., 2013-02-12 19:58:13]

https://chromiumcodereview.appspot.com/12220123/diff/7001/ui/gfx/color_analys...
File ui/gfx/color_analysis.cc (right):

https://chromiumcodereview.appspot.com/12220123/diff/7001/ui/gfx/color_analys...
ui/gfx/color_analysis.cc:423: for (int y = 0; y < bitmap.height(); ++y) {
On 2013/02/12 19:34:51, reed1 wrote:
> SkBitmap stores its pixels in SkPMColor format, not SkColor. You need to use
> SkPMColor type in this code, and use the macros in SkColorPriv.h to extract
> r,g,b bytes.
> 
> SkColor is always in a,r,g,b order as an int (e.g. a << 24, r << 16, g << 8)
and
> is unpremultiplied.
> 
> SkPMColor's byte order is variable, defined in SkColorPriv.h, and can change
on
> different platforms. Use SkGetPackedR32() etc. to extract r,g,b. Also, note
that
> these values will already be "pre" multiplied by that pixel's alpha
> (SkGetPackedA32).
> 
> I don't know (1) if this function is ever called w/ bitmaps that have
per-pixel
> alpha, and (2) if that has any bearing on its covariance...
Ah, I see. It seems I should un-pre-multiply and use color as-is. Fixed
accordingly.

[reed1, 2013-02-12 20:08:15]

lgtm w/ performance comment.

https://chromiumcodereview.appspot.com/12220123/diff/3002/ui/gfx/color_analys...
File ui/gfx/color_analysis.cc (right):

https://chromiumcodereview.appspot.com/12220123/diff/3002/ui/gfx/color_analys...
ui/gfx/color_analysis.cc:422: 
Performance note: perhaps the inner loops will run faster if your _sum variables
are int64_t. That would avoid having to convert r,g,b (ints) to doubles for each
pixel just for the add. The int64_t -> double conversion could happen once
outside of the loops.

Haven't measured, so this is just speculation.

[motek., 2013-02-12 22:18:42]

Applied the suggestion. Now that I got 2 LGTMs I am about to click 'commit'
unless I hear from you guys to the contrary today.

https://chromiumcodereview.appspot.com/12220123/diff/3002/ui/gfx/color_analys...
File ui/gfx/color_analysis.cc (right):

https://chromiumcodereview.appspot.com/12220123/diff/3002/ui/gfx/color_analys...
ui/gfx/color_analysis.cc:422: 
On 2013/02/12 20:08:15, reed1 wrote:
> Performance note: perhaps the inner loops will run faster if your _sum
variables
> are int64_t. That would avoid having to convert r,g,b (ints) to doubles for
each
> pixel just for the add. The int64_t -> double conversion could happen once
> outside of the loops.
> 
> Haven't measured, so this is just speculation.
Right. It probably doesn't really matter that much, but I don't see how that
could do any harm either. Fixed up, just in case.

[commit-bot: I haz the power, 2013-02-12 22:45:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/motek@chromium.org/12220123/12002

[commit-bot: I haz the power, 2013-02-13 08:21:48]

Change committed as 182163