Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2690513002:
Port Android palette API for deriving a prominent color from an image. (Closed)
|
Created:
3 years, 10 months ago by Evan Stade Modified:
3 years, 9 months ago CC:
chromium-reviews, bruthig Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPort Android palette API for deriving a prominent color from an image.
This code is not yet used but will soon be applied to coloration of the
CrOS shelf and ripples based on the wallpaper. It may also be used to
replace the KMeans algorithm that presently exists.
See Android package android.support.v7.graphics
https://developer.android.com/reference/android/support/v7/graphics/package-summary.html
BUG=687852, 595010, 689704
Review-Url: https://codereview.chromium.org/2690513002
Cr-Commit-Position: refs/heads/master@{#452564}
Committed: https://chromium.googlesource.com/chromium/src/+/d00f7189cf554e07c0b25788266919725d90ffb9
Patch Set 1 #
Total comments: 14
Patch Set 2 : git cl try #
Total comments: 47
Patch Set 3 : . #Patch Set 4 : more docs #Patch Set 5 : address msvc pickiness about comparisons #Patch Set 6 : less allocating #Patch Set 7 : better comment #Patch Set 8 : another comment update #
Messages
Total messages: 48 (24 generated)
Description was changed from ========== Port Android palette API for deriving a prominent color from an image. This code is not yet used but will soon be applied to coloration of the CrOS shelf and ripples based on the wallpaper. BUG=687852,595010 ========== to ========== Port Android palette API for deriving a prominent color from an image. This code is not yet used but will soon be applied to coloration of the CrOS shelf and ripples based on the wallpaper. It may also be used to replace the KMeans algorithm that presently exists. BUG=687852,595010,689704 ==========
estade@chromium.org changed reviewers: + sadrul@chromium.org
Description was changed from ========== Port Android palette API for deriving a prominent color from an image. This code is not yet used but will soon be applied to coloration of the CrOS shelf and ripples based on the wallpaper. It may also be used to replace the KMeans algorithm that presently exists. BUG=687852,595010,689704 ========== to ========== Port Android palette API for deriving a prominent color from an image. This code is not yet used but will soon be applied to coloration of the CrOS shelf and ripples based on the wallpaper. It may also be used to replace the KMeans algorithm that presently exists. BUG=687852,595010,689704 ==========
I didn't check the code for the actual procedure, since I am not familiar with it. Maybe someone in skia land would be a better reviewer for that code? I do have some nits https://codereview.chromium.org/2690513002/diff/1/ui/gfx/color_analysis.cc File ui/gfx/color_analysis.cc (right): https://codereview.chromium.org/2690513002/diff/1/ui/gfx/color_analysis.cc#ne... ui/gfx/color_analysis.cc:173: bool CanSplit() { return !color_range_.is_empty(); } const https://codereview.chromium.org/2690513002/diff/1/ui/gfx/color_analysis.cc#ne... ui/gfx/color_analysis.cc:176: ColorBox Split() { What happens if Split() is called when this has just one color? https://codereview.chromium.org/2690513002/diff/1/ui/gfx/color_analysis.cc#ne... ui/gfx/color_analysis.cc:185: SkColor a, SkColor b) { return SkColorGetR(a) < SkColorGetR(b); }; https://chromium-cpp.appspot.com/ claims this is banned. Use base::Bind instead (which does take lambdas, e.g. base::Bind([](SkColor a...) https://codereview.chromium.org/2690513002/diff/1/ui/gfx/color_analysis.cc#ne... ui/gfx/color_analysis.cc:206: for (auto i = color_range_.start() + 1; i < color_range_.end(); ++i) { Just use uint32_t here? https://codereview.chromium.org/2690513002/diff/1/ui/gfx/color_analysis.cc#ne... ui/gfx/color_analysis.cc:234: for (auto i = color_range_.start(); i <= color_range_.end(); ++i) { uint32_t https://codereview.chromium.org/2690513002/diff/1/ui/gfx/color_analysis.cc#ne... ui/gfx/color_analysis.cc:277: for (auto i = color_range_.start(); i < color_range_.end(); ++i) { uint32_t https://codereview.chromium.org/2690513002/diff/1/ui/gfx/color_analysis.h File ui/gfx/color_analysis.h (right): https://codereview.chromium.org/2690513002/diff/1/ui/gfx/color_analysis.h#new... ui/gfx/color_analysis.h:124: // should be used). If a value is succesfully calculated, the return value is I think it'd be better to use two different enums, instead of having separate categories within the same enum.
estade@chromium.org changed reviewers: + oshima@chromium.org
On 2017/02/10 19:36:44, sadrul wrote: > I didn't check the code for the actual procedure, since I am not familiar with > it. Maybe someone in skia land would be a better reviewer for that code? I do > have some nits I will address your other comments soon. re: finding an appropriate reviewer, why do you suggest skia? I don't think anyone from Chrome or Skia is already familiar with this algorithm. Maybe +oshima can help as general image/gfx wizard? To be honest though, I'm not sure how much verification the actual algorithm needs given how much work it would be for someone else to grok the algorithm and verify the code's correctness. Its output has already been vetted on a number of test images and the entire algorithm is heuristic anyway. As long as it gives output that Sebastien likes and it doesn't crash or hang I think we should be happy.
On 2017/02/10 21:46:03, Evan Stade wrote: > On 2017/02/10 19:36:44, sadrul wrote: > > I didn't check the code for the actual procedure, since I am not familiar with > > it. Maybe someone in skia land would be a better reviewer for that code? I do > > have some nits > > I will address your other comments soon. re: finding an appropriate reviewer, > why do you suggest skia? I don't think anyone from Chrome or Skia is already > familiar with this algorithm. Maybe +oshima can help as general image/gfx > wizard? To be honest though, I'm not sure how much verification the actual > algorithm needs given how much work it would be for someone else to grok the > algorithm and verify the code's correctness. Its output has already been vetted > on a number of test images and the entire algorithm is heuristic anyway. As long > as it gives output that Sebastien likes and it doesn't crash or hang I think we > should be happy. sgtm
Oshima, mind taking a look? https://codereview.chromium.org/2690513002/diff/1/ui/gfx/color_analysis.cc File ui/gfx/color_analysis.cc (right): https://codereview.chromium.org/2690513002/diff/1/ui/gfx/color_analysis.cc#ne... ui/gfx/color_analysis.cc:173: bool CanSplit() { return !color_range_.is_empty(); } On 2017/02/10 19:36:44, sadrul wrote: > const Done. https://codereview.chromium.org/2690513002/diff/1/ui/gfx/color_analysis.cc#ne... ui/gfx/color_analysis.cc:176: ColorBox Split() { On 2017/02/10 19:36:44, sadrul wrote: > What happens if Split() is called when this has just one color? It's an error to do that. You'd hit the dcheck in RecomputeBounds(). https://codereview.chromium.org/2690513002/diff/1/ui/gfx/color_analysis.cc#ne... ui/gfx/color_analysis.cc:185: SkColor a, SkColor b) { return SkColorGetR(a) < SkColorGetR(b); }; On 2017/02/10 19:36:44, sadrul wrote: > https://chromium-cpp.appspot.com/ claims this is banned. Use base::Bind instead > (which does take lambdas, e.g. base::Bind([](SkColor a...) well that's pretty annoying as std::sort doesn't take a base::Callback, but I've reworked this code to a (sadly more verbose) version that doesn't use std::function. https://codereview.chromium.org/2690513002/diff/1/ui/gfx/color_analysis.cc#ne... ui/gfx/color_analysis.cc:206: for (auto i = color_range_.start() + 1; i < color_range_.end(); ++i) { On 2017/02/10 19:36:44, sadrul wrote: > Just use uint32_t here? Done. https://codereview.chromium.org/2690513002/diff/1/ui/gfx/color_analysis.cc#ne... ui/gfx/color_analysis.cc:234: for (auto i = color_range_.start(); i <= color_range_.end(); ++i) { On 2017/02/10 19:36:44, sadrul wrote: > uint32_t Done. https://codereview.chromium.org/2690513002/diff/1/ui/gfx/color_analysis.cc#ne... ui/gfx/color_analysis.cc:277: for (auto i = color_range_.start(); i < color_range_.end(); ++i) { On 2017/02/10 19:36:44, sadrul wrote: > uint32_t Done. https://codereview.chromium.org/2690513002/diff/1/ui/gfx/color_analysis.h File ui/gfx/color_analysis.h (right): https://codereview.chromium.org/2690513002/diff/1/ui/gfx/color_analysis.h#new... ui/gfx/color_analysis.h:124: // should be used). If a value is succesfully calculated, the return value is On 2017/02/10 19:36:44, sadrul wrote: > I think it'd be better to use two different enums, instead of having separate > categories within the same enum. Done.
oshima@chromium.org changed reviewers: + reed@chromium.org
+reed@ Maybe reed@ knows who can/should review? Or we can just add reference to the code or algorithm it is ported from, and that may be good enough. https://codereview.chromium.org/2690513002/diff/20001/ui/gfx/color_analysis.h File ui/gfx/color_analysis.h (right): https://codereview.chromium.org/2690513002/diff/20001/ui/gfx/color_analysis.h... ui/gfx/color_analysis.h:117: }; nit: newline
On 2017/02/14 02:18:08, oshima wrote: > +reed@ > > Maybe reed@ knows who can/should review? Or we can just add reference to the > code or algorithm it is ported from, and that may be good enough. > The code does reference the Android Java package. > https://codereview.chromium.org/2690513002/diff/20001/ui/gfx/color_analysis.h > File ui/gfx/color_analysis.h (right): > > https://codereview.chromium.org/2690513002/diff/20001/ui/gfx/color_analysis.h... > ui/gfx/color_analysis.h:117: }; > nit: newline
Hi potential reviewers, how do you think we can move this CL forward?
estade@chromium.org changed reviewers: + sky@chromium.org, thakis@chromium.org
+2 more //ui/ reviewers. Please let me know if there's a way I can help ease the pain of this review.
I'm not at all familiar with this, but then I'm not sure if anyone is. If Mike can't review or suggest someone by tomorrow ping and I'll do my best.
On 2017/02/14 02:18:08, oshima wrote: > +reed@ > > Maybe reed@ knows who can/should review? Or we can just add reference to the > code or algorithm it is ported from, and that may be good enough. Just referencing the code it's ported from seems good enough. Ask sky@ said, I don't think we're going to find a particularly good person to review this. > > https://codereview.chromium.org/2690513002/diff/20001/ui/gfx/color_analysis.h > File ui/gfx/color_analysis.h (right): > > https://codereview.chromium.org/2690513002/diff/20001/ui/gfx/color_analysis.h... > ui/gfx/color_analysis.h:117: }; > nit: newline
Description was changed from ========== Port Android palette API for deriving a prominent color from an image. This code is not yet used but will soon be applied to coloration of the CrOS shelf and ripples based on the wallpaper. It may also be used to replace the KMeans algorithm that presently exists. BUG=687852,595010,689704 ========== to ========== Port Android palette API for deriving a prominent color from an image. This code is not yet used but will soon be applied to coloration of the CrOS shelf and ripples based on the wallpaper. It may also be used to replace the KMeans algorithm that presently exists. See Android package android.support.v7.graphics https://developer.android.com/reference/android/support/v7/graphics/package-s... BUG=687852,595010,689704 ==========
On 2017/02/16 22:54:51, sky wrote: > I'm not at all familiar with this, but then I'm not sure if anyone is. If Mike > can't review or suggest someone by tomorrow ping and I'll do my best. Thanks for offering. Here's a ping :)
Looks generally fine. What's the max size of images for this? If large: The code looks pretty non-optimized. If small: There should probably a check somewhere that only smallish images get passed in. https://codereview.chromium.org/2690513002/diff/20001/ui/gfx/color_analysis.cc File ui/gfx/color_analysis.cc (right): https://codereview.chromium.org/2690513002/diff/20001/ui/gfx/color_analysis.c... ui/gfx/color_analysis.cc:151: // Prominent color utilities --------------------------------------------------- kind of looks long enough that it could go into its own file https://codereview.chromium.org/2690513002/diff/20001/ui/gfx/color_analysis.c... ui/gfx/color_analysis.cc:186: }; optional nit: Since you don't use the name, I'd do enum { RED, GREEN, BLUE } channel = ... https://codereview.chromium.org/2690513002/diff/20001/ui/gfx/color_analysis.c... ui/gfx/color_analysis.cc:209: // Split at the first color value that's not less than the median. You mean "mean" not "median" here, right? Else you could just take the middle index of the range? https://codereview.chromium.org/2690513002/diff/20001/ui/gfx/color_analysis.c... ui/gfx/color_analysis.cc:248: int sum_r = 0; For a 4096x4096 all-red image I think this will overflow. Is there a fixed upper bound on images passed in to this? https://codereview.chromium.org/2690513002/diff/20001/ui/gfx/color_analysis.h File ui/gfx/color_analysis.h (right): https://codereview.chromium.org/2690513002/diff/20001/ui/gfx/color_analysis.h... ui/gfx/color_analysis.h:125: // return value is transparent. This currently calculates one color at a time. Maybe base::Optional<SkColor> or bool and SkColor as outparm? Having special failure case objects feels a bit brittle.
I got most of the way through this before I saw thakis's response. So, here's my comments until then. https://codereview.chromium.org/2690513002/diff/20001/ui/gfx/color_analysis.cc File ui/gfx/color_analysis.cc (right): https://codereview.chromium.org/2690513002/diff/20001/ui/gfx/color_analysis.c... ui/gfx/color_analysis.cc:157: int weight; Document what the range for this is. Always positive? Have a limit? https://codereview.chromium.org/2690513002/diff/20001/ui/gfx/color_analysis.c... ui/gfx/color_analysis.cc:157: int weight; Initialize color and weight? https://codereview.chromium.org/2690513002/diff/20001/ui/gfx/color_analysis.c... ui/gfx/color_analysis.cc:160: // A |ColorBox| represents a region in a color space (an ordered set of colors). Did you consider the name OrderedColorSet? IMO that better describes what it is. https://codereview.chromium.org/2690513002/diff/20001/ui/gfx/color_analysis.c... ui/gfx/color_analysis.cc:167: : ColorBox(color_space, gfx::Range(0, color_space->size() - 1)) {} DCHECK color_space !empty. That said, CanSplit() seems to check for empty, so do you know color_space is non-empty? Also, if the vector is ordered, should you DCHECK on the order? https://codereview.chromium.org/2690513002/diff/20001/ui/gfx/color_analysis.c... ui/gfx/color_analysis.cc:176: ColorBox Split() { This also sorts. Maybe SortAndSplit? https://codereview.chromium.org/2690513002/diff/20001/ui/gfx/color_analysis.c... ui/gfx/color_analysis.cc:178: int r_dimension = max_r_ - min_r_; As you have min/max as uint8_t, why did you use int instead of int8_t here? Also, why is long_dimension a long instead of uint8_t? If there is a good reason to use int over uint8_t, I would expect long_dimension to match the type. https://codereview.chromium.org/2690513002/diff/20001/ui/gfx/color_analysis.c... ui/gfx/color_analysis.cc:216: SkColorGetR(color_space_->at(i)) > (min_r_ + max_r_) / 2; optional: at -> (*color_space_)[i] https://codereview.chromium.org/2690513002/diff/20001/ui/gfx/color_analysis.c... ui/gfx/color_analysis.cc:234: gfx::Range other_range = color_range_; Does this do the right thing with 1 color? If there is only one color, then might the other_range get set to an end of start - 1? https://codereview.chromium.org/2690513002/diff/20001/ui/gfx/color_analysis.c... ui/gfx/color_analysis.cc:254: const SkColor color = color_space_->at(i); If you're going to use const (which I like) use it consistently. By which I mean r_dimension, color_channel and others can be const as well. https://codereview.chromium.org/2690513002/diff/20001/ui/gfx/color_analysis.c... ui/gfx/color_analysis.cc:274: // Comparisons are done by volume. Using operator < to mean compare by volume is obscure and easy to miss. I would prefer an explicit function. https://codereview.chromium.org/2690513002/diff/20001/ui/gfx/color_analysis.c... ui/gfx/color_analysis.cc:285: void RecomputeBounds() { optional: There are no bounds in here, and the name bounds slightly implies a relation to color_range_. Maybe RecomputeCachedValues? RecomputeColors()? https://codereview.chromium.org/2690513002/diff/20001/ui/gfx/color_analysis.c... ui/gfx/color_analysis.cc:314: // The range of indexes into |color_space_| that are part of this box. Why did you make this inclusive vs exclusive? Inclusive is mildly confusing and not what I would have expected. https://codereview.chromium.org/2690513002/diff/20001/ui/gfx/color_analysis.c... ui/gfx/color_analysis.cc:364: // TODO(port): This code assumes the CPU architecture is little-endian. Can you DCHECK on the image type? That it's 32bit? https://codereview.chromium.org/2690513002/diff/20001/ui/gfx/color_analysis.c... ui/gfx/color_analysis.cc:384: if (interesting_colors.empty()) If there is only one color, is it the prominent color? https://codereview.chromium.org/2690513002/diff/20001/ui/gfx/color_analysis.c... ui/gfx/color_analysis.cc:721: std::unique_ptr<uint32_t[]> image(new uint32_t[pixel_count]); image_data? bitmap_data? https://codereview.chromium.org/2690513002/diff/20001/ui/gfx/color_analysis.h File ui/gfx/color_analysis.h (right): https://codereview.chromium.org/2690513002/diff/20001/ui/gfx/color_analysis.h... ui/gfx/color_analysis.h:113: enum class LumaRange { Without any context and looking at these enum values I haven't a clue as to when I would use them and why. Please better document why I would want to futz with these values. Maybe CalculateProminentColorOfBitmap() should have default values to make the recommended choices more obvious?
reed@google.com changed reviewers: + brianosman@google.com, reed@google.com
> What's the max size of images for this? If large: The code > looks pretty non-optimized. If small: There should probably > a check somewhere that only smallish images get passed in. It remains to be seen, bruthig and I still have to mess around with this based on performance. I'm not sure the limitation should be within this function though --- if a caller is willing to pay the price of a lot of computation, why not let them? Ideally expensive computations (large images) would be performed on a helper thread. For example, the initial application of this is for wallpapers. We already have a helper thread to resize large wallpaper images because that operation can be expensive. This is similar --- there's no inherent limitation but callers should be wary. Regarding optimization: do you see any low hanging fruit? This is more or less directly equivalent to what Android does. I'd be wary of straying too far from the source in case Android makes updates and we want to port those over, or if someone comes along and wants to debug this implementation based on the Android version. https://codereview.chromium.org/2690513002/diff/20001/ui/gfx/color_analysis.cc File ui/gfx/color_analysis.cc (right): https://codereview.chromium.org/2690513002/diff/20001/ui/gfx/color_analysis.c... ui/gfx/color_analysis.cc:151: // Prominent color utilities --------------------------------------------------- On 2017/02/17 21:37:05, Nico wrote: > kind of looks long enough that it could go into its own file I like having all the moving parts to this algorithm in the same file, especially as they're not relevant to anything else. The overall file should get a lot shorter when the kmeans algorithm is removed (see cbrug.com/689704) https://codereview.chromium.org/2690513002/diff/20001/ui/gfx/color_analysis.c... ui/gfx/color_analysis.cc:157: int weight; On 2017/02/17 21:42:19, sky wrote: > Document what the range for this is. Always positive? Have a limit? done https://codereview.chromium.org/2690513002/diff/20001/ui/gfx/color_analysis.c... ui/gfx/color_analysis.cc:157: int weight; On 2017/02/17 21:42:19, sky wrote: > Initialize color and weight? changed it to use an explicit constructor https://codereview.chromium.org/2690513002/diff/20001/ui/gfx/color_analysis.c... ui/gfx/color_analysis.cc:160: // A |ColorBox| represents a region in a color space (an ordered set of colors). On 2017/02/17 21:42:19, sky wrote: > Did you consider the name OrderedColorSet? IMO that better describes what it is. no, I didn't consider that name. I drew the names from the Android code and I think there is value in matching that general terminology to make it easier to compare the code. Android doesn't have a ColorBox but it does use "box" extensively and the closest equivalent class is called "Vbox". https://codereview.chromium.org/2690513002/diff/20001/ui/gfx/color_analysis.c... ui/gfx/color_analysis.cc:167: : ColorBox(color_space, gfx::Range(0, color_space->size() - 1)) {} On 2017/02/17 21:42:20, sky wrote: > DCHECK color_space !empty. That said, CanSplit() seems to check for empty, so do > you know color_space is non-empty? An empty color_space would trip the second DCHECK in RecomputeBounds (which is called on construction). > Also, if the vector is ordered, should you DCHECK on the order? The vector isn't initially ordered. https://codereview.chromium.org/2690513002/diff/20001/ui/gfx/color_analysis.c... ui/gfx/color_analysis.cc:178: int r_dimension = max_r_ - min_r_; On 2017/02/17 21:42:19, sky wrote: > As you have min/max as uint8_t, why did you use int instead of int8_t here? updated these all to uint8_t. int8_t could overflow. (max value for int8_t is 127 right?) > Also, why is long_dimension a long instead of uint8_t? If there is a good reason > to use int over uint8_t, I would expect long_dimension to match the type. long_dimension was an int https://codereview.chromium.org/2690513002/diff/20001/ui/gfx/color_analysis.c... ui/gfx/color_analysis.cc:186: }; On 2017/02/17 21:37:05, Nico wrote: > optional nit: Since you don't use the name, I'd do > > enum { RED, GREEN, BLUE } channel = ... I like it, done. https://codereview.chromium.org/2690513002/diff/20001/ui/gfx/color_analysis.c... ui/gfx/color_analysis.cc:209: // Split at the first color value that's not less than the median. On 2017/02/17 21:37:06, Nico wrote: > You mean "mean" not "median" here, right? Else you could just take the middle > index of the range? hmm, I copied this terminology without thinking much about it. It's not the median but it's not the mean (of all values). It's the mean of the start and end values. Changed to midpoint. https://codereview.chromium.org/2690513002/diff/20001/ui/gfx/color_analysis.c... ui/gfx/color_analysis.cc:216: SkColorGetR(color_space_->at(i)) > (min_r_ + max_r_) / 2; On 2017/02/17 21:42:19, sky wrote: > optional: at -> (*color_space_)[i] Done. https://codereview.chromium.org/2690513002/diff/20001/ui/gfx/color_analysis.c... ui/gfx/color_analysis.cc:234: gfx::Range other_range = color_range_; On 2017/02/17 21:42:19, sky wrote: > Does this do the right thing with 1 color? If there is only one color, then > might the other_range get set to an end of start - 1? it's an error to call this function in that case (CanSplit would have returned false) and we'll DCHECK in RecomputeBounds. https://codereview.chromium.org/2690513002/diff/20001/ui/gfx/color_analysis.c... ui/gfx/color_analysis.cc:248: int sum_r = 0; On 2017/02/17 21:37:05, Nico wrote: > For a 4096x4096 all-red image I think this will overflow. Is there a fixed upper > bound on images passed in to this? that's bad. I've changed this to uint64_t. I do expect we'll want to limit the size of images fed to this function, but exactly where depends on performance. Testing and setting limits is one of the first steps we'll be taking while hooking this up (bruthig@ and me). https://codereview.chromium.org/2690513002/diff/20001/ui/gfx/color_analysis.c... ui/gfx/color_analysis.cc:254: const SkColor color = color_space_->at(i); On 2017/02/17 21:42:19, sky wrote: > If you're going to use const (which I like) use it consistently. By which I mean > r_dimension, color_channel and others can be const as well. Done. https://codereview.chromium.org/2690513002/diff/20001/ui/gfx/color_analysis.c... ui/gfx/color_analysis.cc:274: // Comparisons are done by volume. On 2017/02/17 21:42:19, sky wrote: > Using operator < to mean compare by volume is obscure and easy to miss. I would > prefer an explicit function. done. (If there's a more elegant way to do this, please point it out.) https://codereview.chromium.org/2690513002/diff/20001/ui/gfx/color_analysis.c... ui/gfx/color_analysis.cc:285: void RecomputeBounds() { On 2017/02/17 21:42:19, sky wrote: > optional: There are no bounds in here, and the name bounds slightly implies a > relation to color_range_. Maybe RecomputeCachedValues? RecomputeColors()? min_* and max_* describe upper and lower bounds for the colors this box covers. https://codereview.chromium.org/2690513002/diff/20001/ui/gfx/color_analysis.c... ui/gfx/color_analysis.cc:314: // The range of indexes into |color_space_| that are part of this box. On 2017/02/17 21:42:19, sky wrote: > Why did you make this inclusive vs exclusive? Inclusive is mildly confusing and > not what I would have expected. changed. In fact I think there was a bug in the std::sort call because of this. https://codereview.chromium.org/2690513002/diff/20001/ui/gfx/color_analysis.c... ui/gfx/color_analysis.cc:364: // TODO(port): This code assumes the CPU architecture is little-endian. On 2017/02/17 21:42:19, sky wrote: > Can you DCHECK on the image type? That it's 32bit? I was kind of just blindly copying this bit. I've rewritten this to be more C++ and less C-hacker. https://codereview.chromium.org/2690513002/diff/20001/ui/gfx/color_analysis.c... ui/gfx/color_analysis.cc:384: if (interesting_colors.empty()) On 2017/02/17 21:42:19, sky wrote: > If there is only one color, is it the prominent color? it could be, but only if it's "interesting" and within the provided bounds. If we have fewer than 12 colors to pick from we'll break out of the while loop below with the !cansplit break. https://codereview.chromium.org/2690513002/diff/20001/ui/gfx/color_analysis.c... ui/gfx/color_analysis.cc:721: std::unique_ptr<uint32_t[]> image(new uint32_t[pixel_count]); On 2017/02/17 21:42:19, sky wrote: > image_data? bitmap_data? removed https://codereview.chromium.org/2690513002/diff/20001/ui/gfx/color_analysis.h File ui/gfx/color_analysis.h (right): https://codereview.chromium.org/2690513002/diff/20001/ui/gfx/color_analysis.h... ui/gfx/color_analysis.h:113: enum class LumaRange { On 2017/02/17 21:42:20, sky wrote: > Without any context and looking at these enum values I haven't a clue as to when > I would use them and why. Please better document why I would want to futz with > these values. Maybe CalculateProminentColorOfBitmap() should have default values > to make the recommended choices more obvious? CalculateProminentColorOfBitmap() translates these enums into more specific values (i.e. something specific for what it means to be "dark vibrant"). The combinations of these enums matches the Android API, so I don't really want to change them, but I have added better docs. We're probably only going to use dark muted at first but it's cheap and easy to support the same set of values that Android has, and design has requested that all the values be available to tinker with. https://codereview.chromium.org/2690513002/diff/20001/ui/gfx/color_analysis.h... ui/gfx/color_analysis.h:117: }; On 2017/02/14 02:18:08, oshima wrote: > nit: newline Done. https://codereview.chromium.org/2690513002/diff/20001/ui/gfx/color_analysis.h... ui/gfx/color_analysis.h:125: // return value is transparent. This currently calculates one color at a time. On 2017/02/17 21:37:06, Nico wrote: > Maybe base::Optional<SkColor> or bool and SkColor as outparm? Having special > failure case objects feels a bit brittle. This function should only return fully opaque colors --- that's an inherent guarantee in the API. As such there's a range of values that would be invalid and it doesn't seem brittle to me to reuse something in that range as an indicator of failure.
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2690513002/diff/20001/ui/gfx/color_analysis.cc File ui/gfx/color_analysis.cc (right): https://codereview.chromium.org/2690513002/diff/20001/ui/gfx/color_analysis.c... ui/gfx/color_analysis.cc:176: ColorBox Split() { On 2017/02/17 21:42:19, sky wrote: > This also sorts. Maybe SortAndSplit? oops, missed this one. It sorts in an unpredictable way and only sorts a part of the color space. SortAndMutateColorSpace doesn't seem better than Sort to me.
LGTM https://codereview.chromium.org/2690513002/diff/20001/ui/gfx/color_analysis.cc File ui/gfx/color_analysis.cc (right): https://codereview.chromium.org/2690513002/diff/20001/ui/gfx/color_analysis.c... ui/gfx/color_analysis.cc:160: // A |ColorBox| represents a region in a color space (an ordered set of colors). On 2017/02/17 23:23:18, Evan Stade wrote: > On 2017/02/17 21:42:19, sky wrote: > > Did you consider the name OrderedColorSet? IMO that better describes what it > is. > > no, I didn't consider that name. I drew the names from the Android code and I > think there is value in matching that general terminology to make it easier to > compare the code. > > Android doesn't have a ColorBox but it does use "box" extensively and the > closest equivalent class is called "Vbox". I actually think 'box' is misleading in this context. Given the context it implies a region in the image, but that isn't at all what it is. That's why I like using OrderedColorSet, or perhaps just ColorSet. But naming is hard, so if you feel strongly leave it. https://codereview.chromium.org/2690513002/diff/20001/ui/gfx/color_analysis.h File ui/gfx/color_analysis.h (right): https://codereview.chromium.org/2690513002/diff/20001/ui/gfx/color_analysis.h... ui/gfx/color_analysis.h:113: enum class LumaRange { On 2017/02/17 23:23:19, Evan Stade wrote: > On 2017/02/17 21:42:20, sky wrote: > > Without any context and looking at these enum values I haven't a clue as to > when > > I would use them and why. Please better document why I would want to futz with > > these values. Maybe CalculateProminentColorOfBitmap() should have default > values > > to make the recommended choices more obvious? > > CalculateProminentColorOfBitmap() translates these enums into more specific > values (i.e. something specific for what it means to be "dark vibrant"). The > combinations of these enums matches the Android API, so I don't really want to > change them, but I have added better docs. > > We're probably only going to use dark muted at first but it's cheap and easy to > support the same set of values that Android has, and design has requested that > all the values be available to tinker with. If you only intend to call with one value, I recommend moving them to the .cc. That way you still have them, but consumers of this api don't have to understand what the default should be.
https://codereview.chromium.org/2690513002/diff/20001/ui/gfx/color_analysis.h File ui/gfx/color_analysis.h (right): https://codereview.chromium.org/2690513002/diff/20001/ui/gfx/color_analysis.h... ui/gfx/color_analysis.h:113: enum class LumaRange { On 2017/02/18 00:01:34, sky wrote: > On 2017/02/17 23:23:19, Evan Stade wrote: > > On 2017/02/17 21:42:20, sky wrote: > > > Without any context and looking at these enum values I haven't a clue as to > > when > > > I would use them and why. Please better document why I would want to futz > with > > > these values. Maybe CalculateProminentColorOfBitmap() should have default > > values > > > to make the recommended choices more obvious? > > > > CalculateProminentColorOfBitmap() translates these enums into more specific > > values (i.e. something specific for what it means to be "dark vibrant"). The > > combinations of these enums matches the Android API, so I don't really want to > > change them, but I have added better docs. > > > > We're probably only going to use dark muted at first but it's cheap and easy > to > > support the same set of values that Android has, and design has requested that > > all the values be available to tinker with. > > If you only intend to call with one value, I recommend moving them to the .cc. > That way you still have them, but consumers of this api don't have to understand > what the default should be. There is no default. Each consumer of the API (i.e. each UX designer who asks that a feature make use of this functionality) needs to consciously decide which combination to ask for depending on how the color will be used. They may even want to try one combination but fall back to another if the first one fails (that's not explicitly supported yet but we've been talking about doing that for wallpapers).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
thakis@, did you have any additional comments/requests?
lgtm Re optimization: It looks like this does a ton of allocations, copies the bitmap to get unpremultiplied data, etc. It just feels like there's a lot of slack, without having taken a deep look. Unpremultiplying just one row at a time and streaming rows, fewer larger push_back()s etc, are things I'd expect to help with performance a ton. But if it's not exactly clear what this is for and you think that staying similar to the android code is important, I suppose it's fine for now.
On 2017/02/23 17:11:14, Nico wrote: > lgtm > > Re optimization: It looks like this does a ton of allocations, copies the bitmap > to get unpremultiplied data, etc. I guess this was some more blind copying. It was done this way for the kmeans algorithm: "Transform the bitmap before we analyze it because the function reads each pixel multiple times." But we only read each pixel once, so there's no need for a copy of the image. Updated. > It just feels like there's a lot of slack, > without having taken a deep look. Unpremultiplying just one row at a time and > streaming rows, fewer larger push_back()s etc, what do you mean by a large push_back? I guess there are two places where we push_back a bunch of times in a row. One of them pre-reserves space and the other doesn't, so I've added it there too (interesting_colors). Also, the first one (GetUnPreMultipliedPixels) is removed.
The CQ bit was checked by estade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org, thakis@chromium.org Link to the patchset: https://codereview.chromium.org/2690513002/#ps140001 (title: "another comment update")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1487872195046520,
"parent_rev": "45975708707a4b35561d074e0315c517800f6bed", "commit_rev":
"d00f7189cf554e07c0b25788266919725d90ffb9"}
Message was sent while issue was closed.
Description was changed from ========== Port Android palette API for deriving a prominent color from an image. This code is not yet used but will soon be applied to coloration of the CrOS shelf and ripples based on the wallpaper. It may also be used to replace the KMeans algorithm that presently exists. See Android package android.support.v7.graphics https://developer.android.com/reference/android/support/v7/graphics/package-s... BUG=687852,595010,689704 ========== to ========== Port Android palette API for deriving a prominent color from an image. This code is not yet used but will soon be applied to coloration of the CrOS shelf and ripples based on the wallpaper. It may also be used to replace the KMeans algorithm that presently exists. See Android package android.support.v7.graphics https://developer.android.com/reference/android/support/v7/graphics/package-s... BUG=687852,595010,689704 Review-Url: https://codereview.chromium.org/2690513002 Cr-Commit-Position: refs/heads/master@{#452564} Committed: https://chromium.googlesource.com/chromium/src/+/d00f7189cf554e07c0b257882669... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/d00f7189cf554e07c0b257882669... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
