Add ability to constrain dominant color selection to a HSL range.

Add ability to constrain dominant color selection to a HSL range.

This CL adds the ability to supply CalculateKMeanColorOfPNG with a lower
and upper HSL bound which will be used to select the dominant color.

This CL also adds CalculateKMeanColorOfPNG with only a PNG param as a
convenience method, using sane defaults and a utility function
IsWithinHSLRange for checking whether a given HSL is within the given
HSL range.

BUG=376613
TBR=benwells@chromium.org, estade@chromium.org

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

[Matt Giuca, 2014-05-22 05:03:51]

So I think you should split this into 3 CLs (as detailed in the comments):

1. Add extra overloads of those calculate mean color functions.
2. The main part of this CL.
3. Change the color bounds for the App Launcher tile.

https://codereview.chromium.org/289283004/diff/60001/ui/app_list/views/tile_i...
File ui/app_list/views/tile_item_view.cc (right):

https://codereview.chromium.org/289283004/diff/60001/ui/app_list/views/tile_i...
ui/app_list/views/tile_item_view.cc:109:
background_->set_strip_color(color_utils::CalculateKMeanColorOfPNG(
Ew, please don't convert to a PNG just to get around the fact that there is no
way to give explicit values to CalculateKMeanColorOfBitmap. See my comment on
that function.

Also: I think you should save this change for a follow-up CL. This CL should
just be changing the algorithm, not actually changing the parameters used by any
specific client. So in this CL, call the default overload of
CalculateKMeanColorOfBitmap.

https://codereview.chromium.org/289283004/diff/60001/ui/gfx/color_analysis.cc
File ui/gfx/color_analysis.cc (right):

https://codereview.chromium.org/289283004/diff/60001/ui/gfx/color_analysis.cc...
ui/gfx/color_analysis.cc:24: const color_utils::HSL kLowerHSLBound = {-1, -1,
0.15};
kDefaultLowerHSLBound
kDefaultUpperHSLBound

https://codereview.chromium.org/289283004/diff/60001/ui/gfx/color_analysis.h
File ui/gfx/color_analysis.h (right):

https://codereview.chromium.org/289283004/diff/60001/ui/gfx/color_analysis.h#...
ui/gfx/color_analysis.h:61: // |lower_bound| represents the minimum bounding
range of HSL values to allow.
"minimum bound of ..." (what is a "minimum bounding range"? The range is both
the min and max).

https://codereview.chromium.org/289283004/diff/60001/ui/gfx/color_analysis.h#...
ui/gfx/color_analysis.h:86: //   largest weight that's centroid that falls
between the given |lower_bound|
Remove "that" and "the given".

https://codereview.chromium.org/289283004/diff/60001/ui/gfx/color_analysis.h#...
ui/gfx/color_analysis.h:103: GFX_EXPORT SkColor
CalculateKMeanColorOfBitmap(const SkBitmap& bitmap);
Can you also add an overload to CalculateKMeanColorOfBitmap that takes explicit
values. It doesn't make sense that you have to convert to a PNG to do this (and
you do).

You could do this change, and the addition of the new CalculateKMeanColorOfPNG
overload, in a precursor CL which would be a good way to split it.

https://codereview.chromium.org/289283004/diff/60001/ui/gfx/color_analysis_un...
File ui/gfx/color_analysis_unittest.cc (right):

https://codereview.chromium.org/289283004/diff/60001/ui/gfx/color_analysis_un...
ui/gfx/color_analysis_unittest.cc:95: // Creates a 1 dimensional png of the
pixel colors found in |colors|.
Nit: 1-dimensional.

https://codereview.chromium.org/289283004/diff/60001/ui/gfx/color_utils.cc
File ui/gfx/color_utils.cc (right):

https://codereview.chromium.org/289283004/diff/60001/ui/gfx/color_utils.cc#ne...
ui/gfx/color_utils.cc:157: DCHECK(hsl.h >= 0 && hsl.h < 1);
Your comment says this is inclusive, inclusive. What did you end up deciding?

https://codereview.chromium.org/289283004/diff/60001/ui/gfx/color_utils.cc#ne...
ui/gfx/color_utils.cc:157: DCHECK(hsl.h >= 0 && hsl.h < 1);
Should also DCHECK the hsl.s and hsl.l, as well as the bounds.

DCHECK(lower_bound.h >= 0 && lower_bound.h <= 1);
DCHECK(upper_bound.h >= lower_bound.h && upper_bound.h <= lower_bound.h + 1);
and so on.

https://codereview.chromium.org/289283004/diff/60001/ui/gfx/color_utils.h
File ui/gfx/color_utils.h (right):

https://codereview.chromium.org/289283004/diff/60001/ui/gfx/color_utils.h#new...
ui/gfx/color_utils.h:36: // ignored.
This could be construed as "the component will be extended to its logical
limit."

For example, if lower_bound.s = -1 and upper_bound.s = 0.4, I might expect it to
ignore lower_bound, but still apply upper_bound (so effectively be checking s in
[0.0, 0.4]). That's not the case with your algorithm -- if either lower or upper
is -1, the entire attribute is ignored. Either fix the comment or the code.

Also, don't say "that are -1", say "that are negative".

[calamity, 2014-05-22 07:36:36]

Precursor is up at https://codereview.chromium.org/291653004/ .

https://codereview.chromium.org/289283004/diff/60001/ui/app_list/views/tile_i...
File ui/app_list/views/tile_item_view.cc (right):

https://codereview.chromium.org/289283004/diff/60001/ui/app_list/views/tile_i...
ui/app_list/views/tile_item_view.cc:109:
background_->set_strip_color(color_utils::CalculateKMeanColorOfPNG(
On 2014/05/22 05:03:52, Matt Giuca wrote:
> Ew, please don't convert to a PNG just to get around the fact that there is no
> way to give explicit values to CalculateKMeanColorOfBitmap. See my comment on
> that function.
> 
> Also: I think you should save this change for a follow-up CL. This CL should
> just be changing the algorithm, not actually changing the parameters used by
any
> specific client. So in this CL, call the default overload of
> CalculateKMeanColorOfBitmap.


Added overload in precursor CL.

https://codereview.chromium.org/289283004/diff/60001/ui/gfx/color_analysis.cc
File ui/gfx/color_analysis.cc (right):

https://codereview.chromium.org/289283004/diff/60001/ui/gfx/color_analysis.cc...
ui/gfx/color_analysis.cc:24: const color_utils::HSL kLowerHSLBound = {-1, -1,
0.15};
On 2014/05/22 05:03:52, Matt Giuca wrote:
> kDefaultLowerHSLBound
> kDefaultUpperHSLBound

Done.

https://codereview.chromium.org/289283004/diff/60001/ui/gfx/color_analysis.h
File ui/gfx/color_analysis.h (right):

https://codereview.chromium.org/289283004/diff/60001/ui/gfx/color_analysis.h#...
ui/gfx/color_analysis.h:61: // |lower_bound| represents the minimum bounding
range of HSL values to allow.
On 2014/05/22 05:03:52, Matt Giuca wrote:
> "minimum bound of ..." (what is a "minimum bounding range"? The range is both
> the min and max).

Done.

https://codereview.chromium.org/289283004/diff/60001/ui/gfx/color_analysis.h#...
ui/gfx/color_analysis.h:86: //   largest weight that's centroid that falls
between the given |lower_bound|
On 2014/05/22 05:03:52, Matt Giuca wrote:
> Remove "that" and "the given".

Done.

https://codereview.chromium.org/289283004/diff/60001/ui/gfx/color_analysis.h#...
ui/gfx/color_analysis.h:103: GFX_EXPORT SkColor
CalculateKMeanColorOfBitmap(const SkBitmap& bitmap);
On 2014/05/22 05:03:52, Matt Giuca wrote:
> Can you also add an overload to CalculateKMeanColorOfBitmap that takes
explicit
> values. It doesn't make sense that you have to convert to a PNG to do this
(and
> you do).
> 
> You could do this change, and the addition of the new CalculateKMeanColorOfPNG
> overload, in a precursor CL which would be a good way to split it.

Done.

https://codereview.chromium.org/289283004/diff/60001/ui/gfx/color_analysis_un...
File ui/gfx/color_analysis_unittest.cc (right):

https://codereview.chromium.org/289283004/diff/60001/ui/gfx/color_analysis_un...
ui/gfx/color_analysis_unittest.cc:95: // Creates a 1 dimensional png of the
pixel colors found in |colors|.
On 2014/05/22 05:03:52, Matt Giuca wrote:
> Nit: 1-dimensional.

Done.

https://codereview.chromium.org/289283004/diff/60001/ui/gfx/color_utils.cc
File ui/gfx/color_utils.cc (right):

https://codereview.chromium.org/289283004/diff/60001/ui/gfx/color_utils.cc#ne...
ui/gfx/color_utils.cc:157: DCHECK(hsl.h >= 0 && hsl.h < 1);
On 2014/05/22 05:03:52, Matt Giuca wrote:
> Your comment says this is inclusive, inclusive. What did you end up deciding?

The comment says the bounds are inclusive, inclusive. This is an assertion about
the input color. Amended comment and added those checks.

https://codereview.chromium.org/289283004/diff/60001/ui/gfx/color_utils.cc#ne...
ui/gfx/color_utils.cc:157: DCHECK(hsl.h >= 0 && hsl.h < 1);
On 2014/05/22 05:03:52, Matt Giuca wrote:
> Should also DCHECK the hsl.s and hsl.l, as well as the bounds.
> 
> DCHECK(lower_bound.h >= 0 && lower_bound.h <= 1);
Values can be negative to specify ignore.
> DCHECK(upper_bound.h >= lower_bound.h && upper_bound.h <= lower_bound.h + 1);
> and so on.
DCHECKed what I could.

https://codereview.chromium.org/289283004/diff/60001/ui/gfx/color_utils.h
File ui/gfx/color_utils.h (right):

https://codereview.chromium.org/289283004/diff/60001/ui/gfx/color_utils.h#new...
ui/gfx/color_utils.h:36: // ignored.
On 2014/05/22 05:03:52, Matt Giuca wrote:
> This could be construed as "the component will be extended to its logical
> limit."
> 
> For example, if lower_bound.s = -1 and upper_bound.s = 0.4, I might expect it
to
> ignore lower_bound, but still apply upper_bound (so effectively be checking s
in
> [0.0, 0.4]). That's not the case with your algorithm -- if either lower or
upper
> is -1, the entire attribute is ignored. Either fix the comment or the code.
> 
> Also, don't say "that are -1", say "that are negative".

Done.

[calamity, 2014-05-22 07:50:04]

+asvitkine for ui/gfx OWNERS.

[Alexei Svitkine (slow), 2014-05-22 13:05:21]

Seems that this work is sufficiently non-trivial that there should be a crbug
for it. Can you set the BUG= in the description to it? (create it if one doesn't
exist yet)

https://codereview.chromium.org/289283004/diff/120001/ui/gfx/color_utils_unit...
File ui/gfx/color_utils_unittest.cc (right):

https://codereview.chromium.org/289283004/diff/120001/ui/gfx/color_utils_unit...
ui/gfx/color_utils_unittest.cc:90: // Check hue wrap-around.
Can this be a separate TEST?

https://codereview.chromium.org/289283004/diff/120001/ui/gfx/color_utils_unit...
ui/gfx/color_utils_unittest.cc:100: }
Nit: Add blank line after this.

[calamity, 2014-05-23 03:03:40]

@asvitkine: I just realized you're OWNERS of ui/gfx specifically for font
rendering. Let me know if you want me to find a more suitable reviewer for this.

https://codereview.chromium.org/289283004/diff/120001/ui/gfx/color_utils_unit...
File ui/gfx/color_utils_unittest.cc (right):

https://codereview.chromium.org/289283004/diff/120001/ui/gfx/color_utils_unit...
ui/gfx/color_utils_unittest.cc:90: // Check hue wrap-around.
On 2014/05/22 13:05:22, Alexei Svitkine wrote:
> Can this be a separate TEST?

Done.

https://codereview.chromium.org/289283004/diff/120001/ui/gfx/color_utils_unit...
ui/gfx/color_utils_unittest.cc:100: }
On 2014/05/22 13:05:22, Alexei Svitkine wrote:
> Nit: Add blank line after this.

Done.

[Matt Giuca, 2014-05-23 03:33:35]

I looked over the DCHECKs and the changes to the default values. Everything
seems good. LGTM.

https://codereview.chromium.org/289283004/diff/160001/ui/gfx/color_analysis_u...
File ui/gfx/color_analysis_unittest.cc (right):

https://codereview.chromium.org/289283004/diff/160001/ui/gfx/color_analysis_u...
ui/gfx/color_analysis_unittest.cc:246: TEST_F(ColorAnalysisTest,
CalculatePNGKMeanIgnoreGreySaturation) {
nit: Rename to ...IgnoreLowSaturation.

[Matt Giuca, 2014-05-23 05:00:28]

So I looked a bit deeper into this and did some calculations.

The old system was basically not filtering on hue or saturation at all, and
filtering on a weird RGB-based lightness, such that (with the default values)
100 <= R+G+B <= 665.

The new system, by default, is also not filtering hue or saturation, but
filtering actual lightness, such that the average of the maximum and minimum
channel (out of 1.0) is in range [0.15, 0.85].

This means that if saturation is 0:
- 0.15 under the new system = 115.2 under the old system
- 0.85 under the new system = 652.8 under the old system
which is fairly close to the old range of [100, 665].

If saturation = 1, and hue is pure red:
- 0.15 under the new system = 76.8 under the old system
- 0.85 under the new system = 614.4 under the old system
which means for fully saturated colours, we will be keeping more dark colours
and filtering out more light colours than we were before. For example, #ffcdcd
was previously the lightest fully saturated red we would keep (with a lightness
of .90); now the lightest fully saturated red we'll keep is #ffb3b3.

This doesn't make a lot of difference, and I think it is an improvement because
lightness is what matters here, not the RGB values. But there will be subtle
changes to the colours chosen.

[Alexei Svitkine (slow), 2014-05-30 14:56:38]

https://codereview.chromium.org/289283004/diff/180001/ui/gfx/color_analysis.cc
File ui/gfx/color_analysis.cc (right):

https://codereview.chromium.org/289283004/diff/180001/ui/gfx/color_analysis.c...
ui/gfx/color_analysis.cc:24: const color_utils::HSL kDefaultLowerHSLBound = {-1,
-1, 0.15};
Nit: Move the anonymous namespace inside color_utils namespace, so you don't
have to prefix things here.

https://codereview.chromium.org/289283004/diff/180001/ui/gfx/color_analysis.c...
ui/gfx/color_analysis.cc:340: if (color_utils::IsWithinHSLRange(hsl,
lower_bound, upper_bound)) {
Nit: color_utils:: unecessary

https://codereview.chromium.org/289283004/diff/180001/ui/gfx/color_analysis_u...
File ui/gfx/color_analysis_unittest.cc (right):

https://codereview.chromium.org/289283004/diff/180001/ui/gfx/color_analysis_u...
ui/gfx/color_analysis_unittest.cc:17: using color_utils::FindClosestColor;
Nit: Put everything in the file into the color_utils namespace to match the .cc
file, then you don't need these prefixes.

https://codereview.chromium.org/289283004/diff/180001/ui/gfx/color_utils.cc
File ui/gfx/color_utils.cc (right):

https://codereview.chromium.org/289283004/diff/180001/ui/gfx/color_utils.cc#n...
ui/gfx/color_utils.cc:157: DCHECK(hsl.h >= 0 && hsl.h <= 1);
Nit: DCHECK(hsl.h >= 0 && hsl.h <= 1) << hsl.h;

So that it prints the hsl.h value if the check fails. Same for the two below.

https://codereview.chromium.org/289283004/diff/180001/ui/gfx/color_utils.cc#n...
ui/gfx/color_utils.cc:160: DCHECK(lower_bound.h <= 1);
Nit: DCHECK_LE

https://codereview.chromium.org/289283004/diff/180001/ui/gfx/color_utils.cc#n...
ui/gfx/color_utils.cc:164: (upper_bound.h >= 0 && upper_bound.h <= lower_bound.h
+ 1));
Don't these three checks not match what's documented by this comment:

// If a component is negative in either |lower_bound| or |upper_bound|, that
// component will be ignored.

i.e. the above comment seems to imply you can set e.g. upper_bound.h to a
negative value to ignore that component, but these checks are only checking the
lower_bound.h for negative value.

Either fix the checks or make the comment match the checks.

https://codereview.chromium.org/289283004/diff/180001/ui/gfx/color_utils_unit...
File ui/gfx/color_utils_unittest.cc (right):

https://codereview.chromium.org/289283004/diff/180001/ui/gfx/color_utils_unit...
ui/gfx/color_utils_unittest.cc:70: color_utils::HSL hsl = {0.3, 0.4, 0.5};
Nit: Make the test file have namespace color_utils, so you don't need to prefix
things within it.

[calamity, 2014-06-02 03:53:16]

https://codereview.chromium.org/289283004/diff/180001/ui/gfx/color_analysis.cc
File ui/gfx/color_analysis.cc (right):

https://codereview.chromium.org/289283004/diff/180001/ui/gfx/color_analysis.c...
ui/gfx/color_analysis.cc:24: const color_utils::HSL kDefaultLowerHSLBound = {-1,
-1, 0.15};
On 2014/05/30 14:56:39, Alexei Svitkine wrote:
> Nit: Move the anonymous namespace inside color_utils namespace, so you don't
> have to prefix things here.

Done.

https://codereview.chromium.org/289283004/diff/180001/ui/gfx/color_analysis.c...
ui/gfx/color_analysis.cc:340: if (color_utils::IsWithinHSLRange(hsl,
lower_bound, upper_bound)) {
On 2014/05/30 14:56:39, Alexei Svitkine wrote:
> Nit: color_utils:: unecessary

Done.

https://codereview.chromium.org/289283004/diff/180001/ui/gfx/color_analysis_u...
File ui/gfx/color_analysis_unittest.cc (right):

https://codereview.chromium.org/289283004/diff/180001/ui/gfx/color_analysis_u...
ui/gfx/color_analysis_unittest.cc:17: using color_utils::FindClosestColor;
On 2014/05/30 14:56:39, Alexei Svitkine wrote:
> Nit: Put everything in the file into the color_utils namespace to match the
.cc
> file, then you don't need these prefixes.

Done.

https://codereview.chromium.org/289283004/diff/180001/ui/gfx/color_utils.cc
File ui/gfx/color_utils.cc (right):

https://codereview.chromium.org/289283004/diff/180001/ui/gfx/color_utils.cc#n...
ui/gfx/color_utils.cc:157: DCHECK(hsl.h >= 0 && hsl.h <= 1);
On 2014/05/30 14:56:39, Alexei Svitkine wrote:
> Nit: DCHECK(hsl.h >= 0 && hsl.h <= 1) << hsl.h;
> 
> So that it prints the hsl.h value if the check fails. Same for the two below.

Done.

https://codereview.chromium.org/289283004/diff/180001/ui/gfx/color_utils.cc#n...
ui/gfx/color_utils.cc:160: DCHECK(lower_bound.h <= 1);
On 2014/05/30 14:56:39, Alexei Svitkine wrote:
> Nit: DCHECK_LE

Removed.

https://codereview.chromium.org/289283004/diff/180001/ui/gfx/color_utils.cc#n...
ui/gfx/color_utils.cc:164: (upper_bound.h >= 0 && upper_bound.h <= lower_bound.h
+ 1));
On 2014/05/30 14:56:39, Alexei Svitkine wrote:
> Don't these three checks not match what's documented by this comment:
> 
> // If a component is negative in either |lower_bound| or |upper_bound|, that
> // component will be ignored.
> 
> i.e. the above comment seems to imply you can set e.g. upper_bound.h to a
> negative value to ignore that component, but these checks are only checking
the
> lower_bound.h for negative value.
> 
> Either fix the checks or make the comment match the checks.

They were a subset. Changed to exactly match comment.

https://codereview.chromium.org/289283004/diff/180001/ui/gfx/color_utils_unit...
File ui/gfx/color_utils_unittest.cc (right):

https://codereview.chromium.org/289283004/diff/180001/ui/gfx/color_utils_unit...
ui/gfx/color_utils_unittest.cc:70: color_utils::HSL hsl = {0.3, 0.4, 0.5};
On 2014/05/30 14:56:39, Alexei Svitkine wrote:
> Nit: Make the test file have namespace color_utils, so you don't need to
prefix
> things within it.

Done.

[Matt Giuca, 2014-06-02 04:12:23]

slgtm

https://codereview.chromium.org/289283004/diff/260001/ui/gfx/color_utils.cc
File ui/gfx/color_utils.cc (right):

https://codereview.chromium.org/289283004/diff/260001/ui/gfx/color_utils.cc#n...
ui/gfx/color_utils.cc:162: << "lower_bound.h:" << lower_bound.h
nit: Can you put spaces after the ':' and commas at the front of ",
upper_bound.h".

<< "lower_bound.s: " << lower_bound.s
<< ", upper_bound.s: " << upper_bound.s;

[Alexei Svitkine (slow), 2014-06-02 15:35:06]

LGTM

[calamity, 2014-06-03 02:49:50]

https://codereview.chromium.org/289283004/diff/260001/ui/gfx/color_utils.cc
File ui/gfx/color_utils.cc (right):

https://codereview.chromium.org/289283004/diff/260001/ui/gfx/color_utils.cc#n...
ui/gfx/color_utils.cc:162: << "lower_bound.h:" << lower_bound.h
On 2014/06/02 04:12:23, Matt Giuca wrote:
> nit: Can you put spaces after the ':' and commas at the front of ",
> upper_bound.h".
> 
> << "lower_bound.s: " << lower_bound.s
> << ", upper_bound.s: " << upper_bound.s;

Done.

[calamity, 2014-06-03 02:49:57]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2014-06-03 02:51:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/calamity@chromium.org/289283004/280001

[calamity, 2014-06-03 05:03:02]

TBRing benwells@ for bookmark_app_helper.cc and estade@ for
favicon_webui_handler.cc.

[calamity, 2014-06-03 05:03:12]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2014-06-03 05:04:40]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/calamity@chromium.org/289283004/300001

[calamity, 2014-06-03 05:29:50]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2014-06-03 05:30:56]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/calamity@chromium.org/289283004/300001

[commit-bot: I haz the power, 2014-06-03 13:15:06]

Change committed as 274500