Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12335009:
Implementation of principal-component color reduction. (Closed)
|
Created:
7 years, 10 months ago by motek. Modified:
7 years, 10 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImplementation of principal-component color reduction.
R=asvitkine@chromium.org,reed@chromium.org
BUG=155269
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=184174
Patch Set 1 #
Total comments: 18
Patch Set 2 : Addressed (some) remarks from the review. #
Total comments: 13
Patch Set 3 : More review fixes. #
Total comments: 14
Patch Set 4 : De-nitted. #Patch Set 5 : Moved to another fileset to make iOS build happy. #
Messages
Total messages: 19 (0 generated)
all skia-isms look fine.
https://codereview.chromium.org/12335009/diff/1/ui/gfx/color_utils.cc File ui/gfx/color_utils.cc (right): https://codereview.chromium.org/12335009/diff/1/ui/gfx/color_utils.cc#newcode296 ui/gfx/color_utils.cc:296: int width = std::min(source_bitmap.width(), target_bitmap->width()); Instead of doing this, why not create the target_bitmap with the right size already? https://codereview.chromium.org/12335009/diff/1/ui/gfx/color_utils.h File ui/gfx/color_utils.h (right): https://codereview.chromium.org/12335009/diff/1/ui/gfx/color_utils.h#newcode87 ui/gfx/color_utils.h:87: // Returns the effective processed range. Can the comment elaborate more on what "the effective processed range" is? In which cases would it differ from the size of the source bitmap? https://codereview.chromium.org/12335009/diff/1/ui/gfx/color_utils.h#newcode95 ui/gfx/color_utils.h:95: // initialized to the required size and type (SkBitmap::kA8_Config). Why not have the function do the initialization of the resulting bitmap? In fact, I'd suggest just returning a SkBitmap here instead of taking the second param and returning a SkBitmap() in case of failure (which the caller can check .empty() on to detect this). https://codereview.chromium.org/12335009/diff/1/ui/gfx/color_utils_unittest.cc File ui/gfx/color_utils_unittest.cc (right): https://codereview.chromium.org/12335009/diff/1/ui/gfx/color_utils_unittest.c... ui/gfx/color_utils_unittest.cc:19: void Calculate8bitBitmapMinMax( ADD comment explaining what this does. https://codereview.chromium.org/12335009/diff/1/ui/gfx/color_utils_unittest.c... ui/gfx/color_utils_unittest.cc:20: const SkBitmap& bitmap, uint8_t* min_gl, uint8_t* max_gl) { Each param on a separate line. https://codereview.chromium.org/12335009/diff/1/ui/gfx/color_utils_unittest.c... ui/gfx/color_utils_unittest.cc:37: } // namespace https://codereview.chromium.org/12335009/diff/1/ui/gfx/vector3d_f.cc File ui/gfx/vector3d_f.cc (right): https://codereview.chromium.org/12335009/diff/1/ui/gfx/vector3d_f.cc#newcode75 ui/gfx/vector3d_f.cc:75: float DotProduct(const Vector3dF& lhs, float x, float y, float z) { Instead of this, can you make a Vector3dF constructor that takes three floats? Then you can do DotProduct(a, Vector3dF(x, y, z)).
Responded to all questions. One request for clarification and one case where I argue for the proposed implementation. Other things fixed as suggested. https://chromiumcodereview.appspot.com/12335009/diff/1/ui/gfx/color_utils.cc File ui/gfx/color_utils.cc (right): https://chromiumcodereview.appspot.com/12335009/diff/1/ui/gfx/color_utils.cc#... ui/gfx/color_utils.cc:296: int width = std::min(source_bitmap.width(), target_bitmap->width()); On 2013/02/21 16:49:59, Alexei Svitkine wrote: > Instead of doing this, why not create the target_bitmap with the right size > already? As explained in the header, I believe the convention allowing pre-allocated images to be reused might be better in this case. https://chromiumcodereview.appspot.com/12335009/diff/1/ui/gfx/color_utils.h File ui/gfx/color_utils.h (right): https://chromiumcodereview.appspot.com/12335009/diff/1/ui/gfx/color_utils.h#n... ui/gfx/color_utils.h:87: // Returns the effective processed range. On 2013/02/21 16:49:59, Alexei Svitkine wrote: > Can the comment elaborate more on what "the effective processed range" is? In > which cases would it differ from the size of the source bitmap? Changed the return type, this whole idea wasn't any good. Should be simpler now. https://chromiumcodereview.appspot.com/12335009/diff/1/ui/gfx/color_utils.h#n... ui/gfx/color_utils.h:95: // initialized to the required size and type (SkBitmap::kA8_Config). On 2013/02/21 16:49:59, Alexei Svitkine wrote: > Why not have the function do the initialization of the resulting bitmap? In > fact, I'd suggest just returning a SkBitmap here instead of taking the second > param and returning a SkBitmap() in case of failure (which the caller can check > .empty() on to detect this). In processing images it is generally expected that the same buffer will be reused over and over and so I believe that for this set of cases we are generally better off with a convention where an allocated image is passed as a parameter. In this particular case it doesn't matter much, there is only a negligible improvement I'll be able to extract from that (if principal component computation fails, I'll need to put out some other monochrome image; luminance most likely), so what you suggest would work. But in general I thought the convention should be as implemented here. https://chromiumcodereview.appspot.com/12335009/diff/1/ui/gfx/color_utils_uni... File ui/gfx/color_utils_unittest.cc (right): https://chromiumcodereview.appspot.com/12335009/diff/1/ui/gfx/color_utils_uni... ui/gfx/color_utils_unittest.cc:19: void Calculate8bitBitmapMinMax( On 2013/02/21 16:49:59, Alexei Svitkine wrote: > ADD comment explaining what this does. Done. https://chromiumcodereview.appspot.com/12335009/diff/1/ui/gfx/color_utils_uni... ui/gfx/color_utils_unittest.cc:20: const SkBitmap& bitmap, uint8_t* min_gl, uint8_t* max_gl) { On 2013/02/21 16:49:59, Alexei Svitkine wrote: > Each param on a separate line. Done. https://chromiumcodereview.appspot.com/12335009/diff/1/ui/gfx/color_utils_uni... ui/gfx/color_utils_unittest.cc:37: } On 2013/02/21 16:49:59, Alexei Svitkine wrote: > // namespace Done. https://chromiumcodereview.appspot.com/12335009/diff/1/ui/gfx/vector3d_f.cc File ui/gfx/vector3d_f.cc (right): https://chromiumcodereview.appspot.com/12335009/diff/1/ui/gfx/vector3d_f.cc#n... ui/gfx/vector3d_f.cc:75: float DotProduct(const Vector3dF& lhs, float x, float y, float z) { On 2013/02/21 16:49:59, Alexei Svitkine wrote: > Instead of this, can you make a Vector3dF constructor that takes three floats? > Then you can do DotProduct(a, Vector3dF(x, y, z)). Well, I would need to call this, in a typical image process, some 10M times. I realize the compiler would make a good job of it, but still it will just scream *wrong*. If you particularly dislike this change, I'd rather implement multiplication in-place.
https://chromiumcodereview.appspot.com/12335009/diff/1/ui/gfx/vector3d_f.cc File ui/gfx/vector3d_f.cc (right): https://chromiumcodereview.appspot.com/12335009/diff/1/ui/gfx/vector3d_f.cc#n... ui/gfx/vector3d_f.cc:75: float DotProduct(const Vector3dF& lhs, float x, float y, float z) { On 2013/02/21 18:38:43, motek. wrote: > On 2013/02/21 16:49:59, Alexei Svitkine wrote: > > Instead of this, can you make a Vector3dF constructor that takes three floats? > > Then you can do DotProduct(a, Vector3dF(x, y, z)). > Well, I would need to call this, in a typical image process, some 10M times. I > realize the compiler would make a good job of it, but still it will just scream > *wrong*. > If you particularly dislike this change, I'd rather implement multiplication > in-place. +1 -- the inner loop is no place to unnecessarily rely on an optional compiler optimization.
https://codereview.chromium.org/12335009/diff/1/ui/gfx/color_utils.cc File ui/gfx/color_utils.cc (right): https://codereview.chromium.org/12335009/diff/1/ui/gfx/color_utils.cc#newcode296 ui/gfx/color_utils.cc:296: int width = std::min(source_bitmap.width(), target_bitmap->width()); On 2013/02/21 18:38:43, motek. wrote: > On 2013/02/21 16:49:59, Alexei Svitkine wrote: > > Instead of doing this, why not create the target_bitmap with the right size > > already? > As explained in the header, I believe the convention allowing pre-allocated > images to be reused might be better in this case. Got it. Given that you already have DCHECKs() about height/width being equal, do you need the std::min() calls here, or can you just use source_bitmaps.height() directly? https://codereview.chromium.org/12335009/diff/7001/ui/gfx/color_utils.cc File ui/gfx/color_utils.cc (right): https://codereview.chromium.org/12335009/diff/7001/ui/gfx/color_utils.cc#newc... ui/gfx/color_utils.cc:273: if (!target_bitmap) { Do you really need all these early returns? Can you make it the caller's responsibility that they pass a non-NULL bitmap with pixels available, etc. and just DCHECK() on that? https://codereview.chromium.org/12335009/diff/7001/ui/gfx/color_utils.cc#newc... ui/gfx/color_utils.cc:300: if (scale_result) { I think it would be cleaner to do the scaling in a separate function. Here's my reasoning: 1. It would make this function cleaner and easier to read. 2. Sure, you'd need an extra iteration over the pixels, but you wouldn't need to do the gfx::DotProduct() twice for each pixel, which you currently do. 3. In the case where scaling is not needed, you avoid a float multiplication for each pixel. Also, I'm not sure "scaling" is the best term for this, since for bitmaps it usually means resizing the bitmap. Perhaps "normalize"? https://codereview.chromium.org/12335009/diff/7001/ui/gfx/color_utils.cc#newc... ui/gfx/color_utils.cc:306: SkPMColor* current_color = static_cast<uint32_t*>( Should this be a static_cast<SkPMColor*>? https://codereview.chromium.org/12335009/diff/7001/ui/gfx/color_utils.cc#newc... ui/gfx/color_utils.cc:328: SkPMColor* current_color = static_cast<uint32_t*>( Likewise, cast to SkPMColor*? https://codereview.chromium.org/12335009/diff/7001/ui/gfx/color_utils.h File ui/gfx/color_utils.h (right): https://codereview.chromium.org/12335009/diff/7001/ui/gfx/color_utils.h#newco... ui/gfx/color_utils.h:85: // If |scale_result|, result is linearly scalled to fit 0-0xFF range. Otherwise, Nit: scalled -> scaled https://codereview.chromium.org/12335009/diff/7001/ui/gfx/color_utils_unittes... File ui/gfx/color_utils_unittest.cc (right): https://codereview.chromium.org/12335009/diff/7001/ui/gfx/color_utils_unittes... ui/gfx/color_utils_unittest.cc:23: // |bitmap|. |bitmap| has to be allocated and configured to kA8_Config. Nit: move comment above function
https://chromiumcodereview.appspot.com/12335009/diff/1/ui/gfx/vector3d_f.cc File ui/gfx/vector3d_f.cc (right): https://chromiumcodereview.appspot.com/12335009/diff/1/ui/gfx/vector3d_f.cc#n... ui/gfx/vector3d_f.cc:75: float DotProduct(const Vector3dF& lhs, float x, float y, float z) { On 2013/02/21 19:36:37, reed1 wrote: > On 2013/02/21 18:38:43, motek. wrote: > > On 2013/02/21 16:49:59, Alexei Svitkine wrote: > > > Instead of this, can you make a Vector3dF constructor that takes three > floats? > > > Then you can do DotProduct(a, Vector3dF(x, y, z)). > > Well, I would need to call this, in a typical image process, some 10M times. I > > realize the compiler would make a good job of it, but still it will just > scream > > *wrong*. > > If you particularly dislike this change, I'd rather implement multiplication > > in-place. > > +1 -- the inner loop is no place to unnecessarily rely on an optional compiler > optimization. I'd be surprised if the compilers we use don't produce equivalent code if the Vector3dF(x,y,z) ctor is put in the header and marked inline. I'd like to see evidence to the contrary here, in which case we can add a comment explaining why it's done this way. If committed this way without any comment, then it's more than likely someone will end up refactoring it to the form I suggest if the rationale for doing it this way is not in the code.
https://chromiumcodereview.appspot.com/12335009/diff/1/ui/gfx/vector3d_f.cc File ui/gfx/vector3d_f.cc (right): https://chromiumcodereview.appspot.com/12335009/diff/1/ui/gfx/vector3d_f.cc#n... ui/gfx/vector3d_f.cc:75: float DotProduct(const Vector3dF& lhs, float x, float y, float z) { On 2013/02/21 19:54:31, Alexei Svitkine wrote: > On 2013/02/21 19:36:37, reed1 wrote: > > On 2013/02/21 18:38:43, motek. wrote: > > > On 2013/02/21 16:49:59, Alexei Svitkine wrote: > > > > Instead of this, can you make a Vector3dF constructor that takes three > > floats? > > > > Then you can do DotProduct(a, Vector3dF(x, y, z)). > > > Well, I would need to call this, in a typical image process, some 10M times. > I > > > realize the compiler would make a good job of it, but still it will just > > scream > > > *wrong*. > > > If you particularly dislike this change, I'd rather implement multiplication > > > in-place. > > > > +1 -- the inner loop is no place to unnecessarily rely on an optional compiler > > optimization. > > I'd be surprised if the compilers we use don't produce equivalent code if the > Vector3dF(x,y,z) ctor is put in the header and marked inline. > > I'd like to see evidence to the contrary here, in which case we can add a > comment explaining why it's done this way. If committed this way without any > comment, then it's more than likely someone will end up refactoring it to the > form I suggest if the rationale for doing it this way is not in the code. Possibly, or the reverse, where we would need a comment assuring the reader that the optimization always occurs (w/ some test to back that up.) I think here may be a case where we defer to the aesthetics of the author.
A quick comment re: performance. On debug code placing a vector constructor inside the function call results in ~20% performance penalty. I do realize that this is without optimizations we normally benefit from, but it is considerable. Unrolling multiplication cuts another 20% off the cost. I am of the same mind as reed@. If there is ever a good place to hand-optimize, the innermost operation in a loop that runs millions of times is it. Not being 100% sure I believe that in-lining everything will still result in copying values. So, I am likely best off unloading these fields to local variables and applying them as such. Any opposition to this idea? On 2013/02/21 19:57:09, reed1 wrote: > https://chromiumcodereview.appspot.com/12335009/diff/1/ui/gfx/vector3d_f.cc > File ui/gfx/vector3d_f.cc (right): > > https://chromiumcodereview.appspot.com/12335009/diff/1/ui/gfx/vector3d_f.cc#n... > ui/gfx/vector3d_f.cc:75: float DotProduct(const Vector3dF& lhs, float x, float > y, float z) { > On 2013/02/21 19:54:31, Alexei Svitkine wrote: > > On 2013/02/21 19:36:37, reed1 wrote: > > > On 2013/02/21 18:38:43, motek. wrote: > > > > On 2013/02/21 16:49:59, Alexei Svitkine wrote: > > > > > Instead of this, can you make a Vector3dF constructor that takes three > > > floats? > > > > > Then you can do DotProduct(a, Vector3dF(x, y, z)). > > > > Well, I would need to call this, in a typical image process, some 10M > times. > > I > > > > realize the compiler would make a good job of it, but still it will just > > > scream > > > > *wrong*. > > > > If you particularly dislike this change, I'd rather implement > multiplication > > > > in-place. > > > > > > +1 -- the inner loop is no place to unnecessarily rely on an optional > compiler > > > optimization. > > > > I'd be surprised if the compilers we use don't produce equivalent code if the > > Vector3dF(x,y,z) ctor is put in the header and marked inline. > > > > I'd like to see evidence to the contrary here, in which case we can add a > > comment explaining why it's done this way. If committed this way without any > > comment, then it's more than likely someone will end up refactoring it to the > > form I suggest if the rationale for doing it this way is not in the code. > > Possibly, or the reverse, where we would need a comment assuring the reader that > the optimization always occurs (w/ some test to back that up.) > > I think here may be a case where we defer to the aesthetics of the author.
On Thu, Feb 21, 2013 at 3:17 PM, <motek@chromium.org> wrote: > A quick comment re: performance. On debug code placing a vector constructor > inside the function call results in ~20% performance penalty. > I do realize that this is without optimizations we normally benefit from, > but it > is considerable. Unrolling multiplication cuts another 20% off the cost. > > I am of the same mind as reed@. If there is ever a good place to > hand-optimize, > the innermost operation in a loop that runs millions of times is it. Not > being > 100% sure I believe that in-lining everything will still result in copying > values. So, I am likely best off unloading these fields to local variables > and > applying them as such. > Any opposition to this idea? All right, as long as you add an appropriate comment explaining the reasoning. > > > On 2013/02/21 19:57:09, reed1 wrote: > >> https://chromiumcodereview.**appspot.com/12335009/diff/1/** >> ui/gfx/vector3d_f.cc<https://chromiumcodereview.appspot.com/12335009/diff/1/ui/gfx/vector3d_f.cc> >> File ui/gfx/vector3d_f.cc (right): >> > > > https://chromiumcodereview.**appspot.com/12335009/diff/1/** > ui/gfx/vector3d_f.cc#newcode75<https://chromiumcodereview.appspot.com/12335009/diff/1/ui/gfx/vector3d_f.cc#newcode75> > >> ui/gfx/vector3d_f.cc:75: float DotProduct(const Vector3dF& lhs, float x, >> float >> y, float z) { >> On 2013/02/21 19:54:31, Alexei Svitkine wrote: >> > On 2013/02/21 19:36:37, reed1 wrote: >> > > On 2013/02/21 18:38:43, motek. wrote: >> > > > On 2013/02/21 16:49:59, Alexei Svitkine wrote: >> > > > > Instead of this, can you make a Vector3dF constructor that takes >> three >> > > floats? >> > > > > Then you can do DotProduct(a, Vector3dF(x, y, z)). >> > > > Well, I would need to call this, in a typical image process, some >> 10M >> times. >> > I >> > > > realize the compiler would make a good job of it, but still it will >> just >> > > scream >> > > > *wrong*. >> > > > If you particularly dislike this change, I'd rather implement >> multiplication >> > > > in-place. >> > > >> > > +1 -- the inner loop is no place to unnecessarily rely on an optional >> compiler >> > > optimization. >> > >> > I'd be surprised if the compilers we use don't produce equivalent code >> if >> > the > >> > Vector3dF(x,y,z) ctor is put in the header and marked inline. >> > >> > I'd like to see evidence to the contrary here, in which case we can add >> a >> > comment explaining why it's done this way. If committed this way >> without any >> > comment, then it's more than likely someone will end up refactoring it >> to >> > the > >> > form I suggest if the rationale for doing it this way is not in the >> code. >> > > Possibly, or the reverse, where we would need a comment assuring the >> reader >> > that > >> the optimization always occurs (w/ some test to back that up.) >> > > I think here may be a case where we defer to the aesthetics of the author. >> > > > > https://chromiumcodereview.**appspot.com/12335009/<https://chromiumcodereview... >
https://chromiumcodereview.appspot.com/12335009/diff/7001/ui/gfx/color_utils.cc File ui/gfx/color_utils.cc (right): https://chromiumcodereview.appspot.com/12335009/diff/7001/ui/gfx/color_utils.... ui/gfx/color_utils.cc:273: if (!target_bitmap) { On 2013/02/21 19:49:33, Alexei Svitkine wrote: > Do you really need all these early returns? Can you make it the caller's > responsibility that they pass a non-NULL bitmap with pixels available, etc. and > just DCHECK() on that? I think I borrowed this idiom from somewhere. Likely it is not needed. Done as suggested. https://chromiumcodereview.appspot.com/12335009/diff/7001/ui/gfx/color_utils.... ui/gfx/color_utils.cc:300: if (scale_result) { I don't really follow. To know how to scale I need to know the range. Thus this extra pass (with dot-products) is needed. If I wanted to do it once, I'd need some form of intermediate storage (because the target is 8-bit). Data loss would be a problem. https://chromiumcodereview.appspot.com/12335009/diff/7001/ui/gfx/color_utils.... ui/gfx/color_utils.cc:306: SkPMColor* current_color = static_cast<uint32_t*>( On 2013/02/21 19:49:33, Alexei Svitkine wrote: > Should this be a static_cast<SkPMColor*>? Seems you are right. Done. https://chromiumcodereview.appspot.com/12335009/diff/7001/ui/gfx/color_utils.... ui/gfx/color_utils.cc:328: SkPMColor* current_color = static_cast<uint32_t*>( On 2013/02/21 19:49:33, Alexei Svitkine wrote: > Likewise, cast to SkPMColor*? Done. https://chromiumcodereview.appspot.com/12335009/diff/7001/ui/gfx/color_utils.h File ui/gfx/color_utils.h (right): https://chromiumcodereview.appspot.com/12335009/diff/7001/ui/gfx/color_utils.... ui/gfx/color_utils.h:85: // If |scale_result|, result is linearly scalled to fit 0-0xFF range. Otherwise, On 2013/02/21 19:49:33, Alexei Svitkine wrote: > Nit: scalled -> scaled Done. https://chromiumcodereview.appspot.com/12335009/diff/7001/ui/gfx/color_utils_... File ui/gfx/color_utils_unittest.cc (right): https://chromiumcodereview.appspot.com/12335009/diff/7001/ui/gfx/color_utils_... ui/gfx/color_utils_unittest.cc:23: // |bitmap|. |bitmap| has to be allocated and configured to kA8_Config. On 2013/02/21 19:49:33, Alexei Svitkine wrote: > Nit: move comment above function Done.
LGTM with some nits https://codereview.chromium.org/12335009/diff/7001/ui/gfx/color_utils.cc File ui/gfx/color_utils.cc (right): https://codereview.chromium.org/12335009/diff/7001/ui/gfx/color_utils.cc#newc... ui/gfx/color_utils.cc:300: if (scale_result) { On 2013/02/21 22:45:03, motek. wrote: > I don't really follow. To know how to scale I need to know the range. Thus this > extra pass (with dot-products) is needed. If I wanted to do it once, I'd need > some form of intermediate storage (because the target is 8-bit). Data loss would > be a problem. Ah, you're right, I didn't realise we couldn't re-use the same bitmap for the intermediate storage - without losing precision. Fair enough. https://codereview.chromium.org/12335009/diff/9/ui/gfx/color_utils.cc File ui/gfx/color_utils.cc (right): https://codereview.chromium.org/12335009/diff/9/ui/gfx/color_utils.cc#newcode279 ui/gfx/color_utils.cc:279: DCHECK(source_bitmap.config() == SkBitmap::kARGB_8888_Config); I am assuming DCHECK_EQ() results in a compile failure here? If not, please use it. https://codereview.chromium.org/12335009/diff/9/ui/gfx/color_utils.cc#newcode284 ui/gfx/color_utils.cc:284: DCHECK(source_bitmap.height() > 0 && source_bitmap.width() > 0); Nit: DCHECK(!source_bitmap.empty()) instead https://codereview.chromium.org/12335009/diff/9/ui/gfx/color_utils.cc#newcode325 ui/gfx/color_utils.cc:325: SkPMColor* current_color = static_cast<SkPMColor*>( Nit: How about |source_color_row| and make it const. Here and in the loop above. https://codereview.chromium.org/12335009/diff/9/ui/gfx/color_utils.cc#newcode327 ui/gfx/color_utils.cc:327: uint8_t* target_color = target_bitmap->getAddr8(0, y); Nit: How about |target_color_row| Here and in the loop above. https://codereview.chromium.org/12335009/diff/9/ui/gfx/color_utils.cc#newcode329 ui/gfx/color_utils.cc:329: SkColor c = SkUnPreMultiply::PMColorToColor(*current_color); Nit: How about source_color_row[x] instead of doing ++current_color? So that it's consistent with the use of target_color. Here and in the loop above. https://codereview.chromium.org/12335009/diff/9/ui/gfx/color_utils_unittest.cc File ui/gfx/color_utils_unittest.cc (right): https://codereview.chromium.org/12335009/diff/9/ui/gfx/color_utils_unittest.c... ui/gfx/color_utils_unittest.cc:191: Nit: Remove blank line. https://codereview.chromium.org/12335009/diff/9/ui/gfx/color_utils_unittest.c... ui/gfx/color_utils_unittest.cc:233: Nit: Remove blank line.
Fixed all. Submitting. https://chromiumcodereview.appspot.com/12335009/diff/9/ui/gfx/color_utils.cc File ui/gfx/color_utils.cc (right): https://chromiumcodereview.appspot.com/12335009/diff/9/ui/gfx/color_utils.cc#... ui/gfx/color_utils.cc:279: DCHECK(source_bitmap.config() == SkBitmap::kARGB_8888_Config); On 2013/02/22 02:24:06, Alexei Svitkine wrote: > I am assuming DCHECK_EQ() results in a compile failure here? If not, please use > it. Done. https://chromiumcodereview.appspot.com/12335009/diff/9/ui/gfx/color_utils.cc#... ui/gfx/color_utils.cc:284: DCHECK(source_bitmap.height() > 0 && source_bitmap.width() > 0); On 2013/02/22 02:24:06, Alexei Svitkine wrote: > Nit: DCHECK(!source_bitmap.empty()) instead Done. https://chromiumcodereview.appspot.com/12335009/diff/9/ui/gfx/color_utils.cc#... ui/gfx/color_utils.cc:325: SkPMColor* current_color = static_cast<SkPMColor*>( On 2013/02/22 02:24:06, Alexei Svitkine wrote: > Nit: How about |source_color_row| and make it const. > > Here and in the loop above. Done. https://chromiumcodereview.appspot.com/12335009/diff/9/ui/gfx/color_utils.cc#... ui/gfx/color_utils.cc:327: uint8_t* target_color = target_bitmap->getAddr8(0, y); On 2013/02/22 02:24:06, Alexei Svitkine wrote: > Nit: How about |target_color_row| > > Here and in the loop above. Done. https://chromiumcodereview.appspot.com/12335009/diff/9/ui/gfx/color_utils.cc#... ui/gfx/color_utils.cc:329: SkColor c = SkUnPreMultiply::PMColorToColor(*current_color); On 2013/02/22 02:24:06, Alexei Svitkine wrote: > Nit: How about source_color_row[x] instead of doing ++current_color? So that > it's consistent with the use of target_color. > > Here and in the loop above. Done. https://chromiumcodereview.appspot.com/12335009/diff/9/ui/gfx/color_utils_uni... File ui/gfx/color_utils_unittest.cc (right): https://chromiumcodereview.appspot.com/12335009/diff/9/ui/gfx/color_utils_uni... ui/gfx/color_utils_unittest.cc:191: On 2013/02/22 02:24:06, Alexei Svitkine wrote: > Nit: Remove blank line. Done. https://chromiumcodereview.appspot.com/12335009/diff/9/ui/gfx/color_utils_uni... ui/gfx/color_utils_unittest.cc:233: On 2013/02/22 02:24:06, Alexei Svitkine wrote: > Nit: Remove blank line. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/motek@chromium.org/12335009/3003
Sorry for I got bad news for ya. Compile failed with a clobber build on ios_dbg_simulator. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
PTAL Given build troubles on iOS, I needed to move all that to color_analysis.*
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/motek@chromium.org/12335009/10007
Message was sent while issue was closed.
Change committed as 184174 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
