Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
13947013:
Complete (but inefficient) implementation of the image retargetting method. (Closed)
|
Created:
7 years, 8 months ago by motek. Modified:
7 years, 8 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionComplete (but inefficient) implementation of the image retargetting method.
This CL contains:
1. Convolution of arbitrary channel with arbitrary kernel (convolver*).
2. Gaussian gradient magnitude implementation.
3. Image profile (X and Y projections) computations.
4. Otsu-like thresholding of profiles.
5. Image decimation.
6. The main routine which binds it all together.
Note: 1 and 2 do work, but remain main sources of suckiness due to performance
problems. I actively work on this. Still, I'd love to get the current state in
to establish a baseline and for viewing pleasure of those who are interested.
BUG=155269
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=194565
Patch Set 1 #
Total comments: 37
Patch Set 2 : Addressed reviewers' remarks. #
Total comments: 36
Patch Set 3 : Second round of reviews (+ making complier happy). #Patch Set 4 : vcpp compilation fix #Patch Set 5 : Oops. Now saved and uploaded. #
Total comments: 1
Patch Set 6 : Added early exit when filter is bigger than the image or if there is no filter (all zeros). #
Total comments: 2
Patch Set 7 : Moved content_analysis* to browser/thumbnails/ . #Patch Set 8 : Rmvd abbrvs. #
Messages
Total messages: 19 (0 generated)
Hi, could I bother you guys for a review?
looks like all of the skia-isms are correct. I feel a bit underqualified to comment on the image processing aspects of it. Adding stephen to the cc. https://codereview.chromium.org/13947013/diff/1/skia/ext/convolver.h File skia/ext/convolver.h (right): https://codereview.chromium.org/13947013/diff/1/skia/ext/convolver.h#newcode116 skia/ext/convolver.h:116: // There will be |filter_length| values in the return array. Do you want to comment that this can return NULL (and why)?
I'll let Stephen do the review of the image processing logic. In the meantime, here's some style nits. https://codereview.chromium.org/13947013/diff/1/ui/gfx/content_analysis.cc File ui/gfx/content_analysis.cc (right): https://codereview.chromium.org/13947013/diff/1/ui/gfx/content_analysis.cc#ne... ui/gfx/content_analysis.cc:288: for (it = input.begin(); it < input.end(); ++it) { Be consistent whether you use iterators or indices, either use indices everywhere or iterators everywhere. https://codereview.chromium.org/13947013/diff/1/ui/gfx/content_analysis.cc#ne... ui/gfx/content_analysis.cc:310: for (unsigned i = 1; i < histogram.size(); ++i) { Use size_t, here and throughout for the indices. https://codereview.chromium.org/13947013/diff/1/ui/gfx/content_analysis_unitt... File ui/gfx/content_analysis_unittest.cc (right): https://codereview.chromium.org/13947013/diff/1/ui/gfx/content_analysis_unitt... ui/gfx/content_analysis_unittest.cc:35: void DumpBitmapToFile(const SkBitmap& bitmap) { If you're not using these in your tests, please remove them and the associated std header files you're including. https://codereview.chromium.org/13947013/diff/1/ui/gfx/content_analysis_unitt... ui/gfx/content_analysis_unittest.cc:114: } Add " // namespace"
Cool stuff. Most of my comments are just suggestions/nits, except for the naming stuff. Judicious use of getAddr8() might clean up content_analysis.cc a bit, though. https://codereview.chromium.org/13947013/diff/1/skia/ext/convolver.cc File skia/ext/convolver.cc (right): https://codereview.chromium.org/13947013/diff/1/skia/ext/convolver.cc#newcode899 skia/ext/convolver.cc:899: int pixel_index = c + i - centrepoint; Perf: could probably hoist this out of the loop. https://codereview.chromium.org/13947013/diff/1/skia/ext/convolver.cc#newcode900 skia/ext/convolver.cc:900: // Handling of edges is always wrap-around. Clip to 0 / width. Seems strange to wrap-around instead of clamp, but I don't know your application. https://codereview.chromium.org/13947013/diff/1/skia/ext/convolver.cc#newcode901 skia/ext/convolver.cc:901: pixel_index = (pixel_index + image_size.width()) % image_size.width(); Perf: could compute the margins in separate inner loops, to avoid the mod. https://codereview.chromium.org/13947013/diff/1/skia/ext/convolver.h File skia/ext/convolver.h (right): https://codereview.chromium.org/13947013/diff/1/skia/ext/convolver.h#newcode13 skia/ext/convolver.h:13: #include "third_party/skia/include/core/SkSize.h" I think you could forward-declare SkISize instead of #including here. https://codereview.chromium.org/13947013/diff/1/skia/ext/convolver.h#newcode114 skia/ext/convolver.h:114: // |specified_filter_size| may be different if leading/trailing zeros of the Maybe this comment should be on the declaration for filter_size/length themselves? https://codereview.chromium.org/13947013/diff/1/skia/ext/convolver.h#newcode117 skia/ext/convolver.h:117: inline const Fixed* GetSingleFilter(int* specified_filter_size, Nit: prefer out-of-line here unless performance is critical (I realize the above is inline too; should probably be checked.) https://codereview.chromium.org/13947013/diff/1/skia/ext/convolver.h#newcode153 skia/ext/convolver.h:153: int filter_size; A bit confusing.. maybe filter_size -> length, and length -> trimmed_length? https://codereview.chromium.org/13947013/diff/1/skia/ext/convolver.h#newcode201 skia/ext/convolver.h:201: SK_API void SingleChannelConvolve1D_X(const unsigned char* source_data, Style guide says no underscores in function names, although I can see why you did it. Perhaps SingleChannelConvolveX1D/Y1D? Or you could also use a single function for both by passing a pixel stride as well as a row stride (could be a template param if there's a perf impact). https://codereview.chromium.org/13947013/diff/1/ui/gfx/content_analysis.cc File ui/gfx/content_analysis.cc (right): https://codereview.chromium.org/13947013/diff/1/ui/gfx/content_analysis.cc#ne... ui/gfx/content_analysis.cc:118: static_cast<const uint8*>(input_bitmap->getPixels()), Suggestion: use input_bitmap->getAddr8(0, 0) to hide the cast. (Here, and many places below.) https://codereview.chromium.org/13947013/diff/1/ui/gfx/content_analysis.cc#ne... ui/gfx/content_analysis.cc:177: std::log10(static_cast<float>(grad_max)) / std::log10(2.0f)) - 7; Not that it really matters, but these could probably just be log, instead of log10. https://codereview.chromium.org/13947013/diff/1/ui/gfx/content_analysis.cc#ne... ui/gfx/content_analysis.cc:180: intermediate.getPixels()) + r * intermediate.rowBytes(); intermediate.getAddr8(0, r) should work here, and below. https://codereview.chromium.org/13947013/diff/1/ui/gfx/content_analysis.cc#ne... ui/gfx/content_analysis.cc:247: 2 * static_cast<int>(0.5f * area.width() / target_size.width() + 0.5); 0.5 should probably be 0.5f, unless you really need the last + to be done in doubles. https://codereview.chromium.org/13947013/diff/1/ui/gfx/content_analysis.cc#ne... ui/gfx/content_analysis.cc:249: 2 * static_cast<int>(0.5f * area.height() / target_size.height() + 0.5); Same here.
I addressed all remarks, implementing all but one suggestions. Note that changes in convolver.cc go beyond simple fixes. The code ended up more complex but it is considerably faster indeed. https://codereview.chromium.org/13947013/diff/1/skia/ext/convolver.cc File skia/ext/convolver.cc (right): https://codereview.chromium.org/13947013/diff/1/skia/ext/convolver.cc#newcode899 skia/ext/convolver.cc:899: int pixel_index = c + i - centrepoint; On 2013/04/10 15:20:31, Stephen White wrote: > Perf: could probably hoist this out of the loop. I have reworked that from the perf angle. https://codereview.chromium.org/13947013/diff/1/skia/ext/convolver.cc#newcode900 skia/ext/convolver.cc:900: // Handling of edges is always wrap-around. Clip to 0 / width. On 2013/04/10 15:20:31, Stephen White wrote: > Seems strange to wrap-around instead of clamp, but I don't know your > application. Padding would be most reasonable for this application but I done wraparound because it was simpler. Changed now/ https://codereview.chromium.org/13947013/diff/1/skia/ext/convolver.cc#newcode901 skia/ext/convolver.cc:901: pixel_index = (pixel_index + image_size.width()) % image_size.width(); On 2013/04/10 15:20:31, Stephen White wrote: > Perf: could compute the margins in separate inner loops, to avoid the mod. Done. https://codereview.chromium.org/13947013/diff/1/skia/ext/convolver.h File skia/ext/convolver.h (right): https://codereview.chromium.org/13947013/diff/1/skia/ext/convolver.h#newcode13 skia/ext/convolver.h:13: #include "third_party/skia/include/core/SkSize.h" On 2013/04/10 15:20:31, Stephen White wrote: > I think you could forward-declare SkISize instead of #including here. SkISize is declared: typedef struct SkTSize<int> SkISize The compiler will want me to forward declare the actual type first, right? I thought it would way too much detail for a forward declaration. https://codereview.chromium.org/13947013/diff/1/skia/ext/convolver.h#newcode114 skia/ext/convolver.h:114: // |specified_filter_size| may be different if leading/trailing zeros of the On 2013/04/10 15:20:31, Stephen White wrote: > Maybe this comment should be on the declaration for filter_size/length > themselves? Added it there. https://codereview.chromium.org/13947013/diff/1/skia/ext/convolver.h#newcode116 skia/ext/convolver.h:116: // There will be |filter_length| values in the return array. On 2013/04/09 15:43:18, reed1 wrote: > Do you want to comment that this can return NULL (and why)? Done. https://codereview.chromium.org/13947013/diff/1/skia/ext/convolver.h#newcode117 skia/ext/convolver.h:117: inline const Fixed* GetSingleFilter(int* specified_filter_size, On 2013/04/10 15:20:31, Stephen White wrote: > Nit: prefer out-of-line here unless performance is critical (I realize the > above is inline too; should probably be checked.) The original is processed in a loop, so I assume this is intentional. Moved this one out-of-line. https://codereview.chromium.org/13947013/diff/1/skia/ext/convolver.h#newcode153 skia/ext/convolver.h:153: int filter_size; On 2013/04/10 15:20:31, Stephen White wrote: > A bit confusing.. maybe filter_size -> length, and length -> trimmed_length? Done. https://codereview.chromium.org/13947013/diff/1/skia/ext/convolver.h#newcode201 skia/ext/convolver.h:201: SK_API void SingleChannelConvolve1D_X(const unsigned char* source_data, On 2013/04/10 15:20:31, Stephen White wrote: > Style guide says no underscores in function names, although I can see why you > did it. Perhaps SingleChannelConvolveX1D/Y1D? > > Or you could also use a single function for both by passing a pixel stride as > well as a row stride (could be a template param if there's a perf impact). Renamed as suggested. As for the other suggestion: templatizing strides could indeed have influence over performance, but I am not sure how much exactly. I do intend to make a performance-focused pass over this, I'll change at that time if this turns out to make any difference. Would that be OK with you? https://codereview.chromium.org/13947013/diff/1/ui/gfx/content_analysis.cc File ui/gfx/content_analysis.cc (right): https://codereview.chromium.org/13947013/diff/1/ui/gfx/content_analysis.cc#ne... ui/gfx/content_analysis.cc:118: static_cast<const uint8*>(input_bitmap->getPixels()), On 2013/04/10 15:20:31, Stephen White wrote: > Suggestion: use input_bitmap->getAddr8(0, 0) to hide the cast. (Here, and many > places below.) Done. https://codereview.chromium.org/13947013/diff/1/ui/gfx/content_analysis.cc#ne... ui/gfx/content_analysis.cc:177: std::log10(static_cast<float>(grad_max)) / std::log10(2.0f)) - 7; On 2013/04/10 15:20:31, Stephen White wrote: > Not that it really matters, but these could probably just be log, instead of > log10. They certainly could. I need log2 which is not available, so I use the property of logarithm in general. I dunno, log10 seemed just more 'decimal' than at natural base. https://codereview.chromium.org/13947013/diff/1/ui/gfx/content_analysis.cc#ne... ui/gfx/content_analysis.cc:180: intermediate.getPixels()) + r * intermediate.rowBytes(); On 2013/04/10 15:20:31, Stephen White wrote: > intermediate.getAddr8(0, r) should work here, and below. Done. https://codereview.chromium.org/13947013/diff/1/ui/gfx/content_analysis.cc#ne... ui/gfx/content_analysis.cc:247: 2 * static_cast<int>(0.5f * area.width() / target_size.width() + 0.5); On 2013/04/10 15:20:31, Stephen White wrote: > 0.5 should probably be 0.5f, unless you really need the last + to be done in > doubles. Done. https://codereview.chromium.org/13947013/diff/1/ui/gfx/content_analysis.cc#ne... ui/gfx/content_analysis.cc:249: 2 * static_cast<int>(0.5f * area.height() / target_size.height() + 0.5); On 2013/04/10 15:20:31, Stephen White wrote: > Same here. Done. https://codereview.chromium.org/13947013/diff/1/ui/gfx/content_analysis.cc#ne... ui/gfx/content_analysis.cc:288: for (it = input.begin(); it < input.end(); ++it) { On 2013/04/09 18:02:23, Alexei Svitkine wrote: > Be consistent whether you use iterators or indices, either use indices > everywhere or iterators everywhere. I think I am consistent. I use iterators everywhere except that iteration over histogram below, where I actually need the index as a result. https://codereview.chromium.org/13947013/diff/1/ui/gfx/content_analysis.cc#ne... ui/gfx/content_analysis.cc:310: for (unsigned i = 1; i < histogram.size(); ++i) { On 2013/04/09 18:02:23, Alexei Svitkine wrote: > Use size_t, here and throughout for the indices. Done. https://codereview.chromium.org/13947013/diff/1/ui/gfx/content_analysis_unitt... File ui/gfx/content_analysis_unittest.cc (right): https://codereview.chromium.org/13947013/diff/1/ui/gfx/content_analysis_unitt... ui/gfx/content_analysis_unittest.cc:35: void DumpBitmapToFile(const SkBitmap& bitmap) { On 2013/04/09 18:02:23, Alexei Svitkine wrote: > If you're not using these in your tests, please remove them and the associated > std header files you're including. Done. https://codereview.chromium.org/13947013/diff/1/ui/gfx/content_analysis_unitt... ui/gfx/content_analysis_unittest.cc:114: } On 2013/04/09 18:02:23, Alexei Svitkine wrote: > Add " // namespace" Done.
LGTM. The rest are just suggestions. https://codereview.chromium.org/13947013/diff/1/skia/ext/convolver.h File skia/ext/convolver.h (right): https://codereview.chromium.org/13947013/diff/1/skia/ext/convolver.h#newcode13 skia/ext/convolver.h:13: #include "third_party/skia/include/core/SkSize.h" On 2013/04/11 16:18:38, motek. wrote: > On 2013/04/10 15:20:31, Stephen White wrote: > > I think you could forward-declare SkISize instead of #including here. > SkISize is declared: > typedef struct SkTSize<int> SkISize > > The compiler will want me to forward declare the actual type first, right? I > thought it would way too much detail for a forward declaration. Ahh, you're right. I'd forgotten it was a typedef. https://codereview.chromium.org/13947013/diff/9001/ui/gfx/content_analysis.cc File ui/gfx/content_analysis.cc (right): https://codereview.chromium.org/13947013/diff/9001/ui/gfx/content_analysis.cc... ui/gfx/content_analysis.cc:180: intermediate.getAddr8(0, 0) + r * intermediate.rowBytes(); Just a suggestion, but this could be intermediate.getAddr8(0, r), which save you having to multiply by rowBytes() yourself. https://codereview.chromium.org/13947013/diff/9001/ui/gfx/content_analysis.cc... ui/gfx/content_analysis.cc:182: intermediate2.getAddr8(0, 0) + r * intermediate2.rowBytes(); Same here. https://codereview.chromium.org/13947013/diff/9001/ui/gfx/content_analysis.cc... ui/gfx/content_analysis.cc:184: input_bitmap->getAddr8(0, 0) + r * input_bitmap->rowBytes(); Same here. https://codereview.chromium.org/13947013/diff/9001/ui/gfx/content_analysis.cc... ui/gfx/content_analysis.cc:216: const uint8* image_row = input_bitmap.getAddr8(0, 0) + Same here.
https://codereview.chromium.org/13947013/diff/9001/skia/ext/convolver.cc File skia/ext/convolver.cc (right): https://codereview.chromium.org/13947013/diff/9001/skia/ext/convolver.cc#newc... skia/ext/convolver.cc:318: if (filter.trimmed_length == 0) { Nit: Remove {}'s. https://codereview.chromium.org/13947013/diff/9001/skia/ext/convolver.cc#newc... skia/ext/convolver.cc:627: for (; i < centrepoint - r; ++i) // Padding part. This logic seems identical to SingleChannelConvolveX1D(). Can you refactor the common functionality to a helper function in an anon namespace? https://codereview.chromium.org/13947013/diff/9001/skia/ext/convolver_unittes... File skia/ext/convolver_unittest.cc (right): https://codereview.chromium.org/13947013/diff/9001/skia/ext/convolver_unittes... skia/ext/convolver_unittest.cc:336: int img_width = kSize; Do you need all these temp variables? e.g. can you just use kSize directly? Also, everything that's not changing after init, please make const. https://codereview.chromium.org/13947013/diff/9001/skia/ext/convolver_unittes... skia/ext/convolver_unittest.cc:359: for (int x = signal_x - 2; x <= signal_x + 2; ++x) { Nit: No need for {}'s. https://codereview.chromium.org/13947013/diff/9001/skia/ext/convolver_unittes... skia/ext/convolver_unittest.cc:369: for (int y = signal_y - 2; y <= signal_y + 2; ++y) { Nit: No need for {}'s. https://codereview.chromium.org/13947013/diff/9001/skia/ext/convolver_unittes... skia/ext/convolver_unittest.cc:458: EXPECT_TRUE( Can you use EXPECT_NEAR() for these? https://codereview.chromium.org/13947013/diff/9001/ui/gfx/content_analysis.cc File ui/gfx/content_analysis.cc (right): https://codereview.chromium.org/13947013/diff/9001/ui/gfx/content_analysis.cc... ui/gfx/content_analysis.cc:34: DCHECK(front_tail_length < last - first); Can you use DCHECK_LT? https://codereview.chromium.org/13947013/diff/9001/ui/gfx/content_analysis.cc... ui/gfx/content_analysis.cc:56: for (;front_tail_length >= 0; --front_tail_length, ++i) { Nit: Add a space after the first ; https://codereview.chromium.org/13947013/diff/9001/ui/gfx/content_analysis.cc... ui/gfx/content_analysis.cc:326: m2 = t2 / w2; Does the algorithm ensure that w1 and w2 are never 0? If not, add a DCHECK or an early return. https://codereview.chromium.org/13947013/diff/9001/ui/gfx/content_analysis.cc... ui/gfx/content_analysis.cc:348: unsigned tgt_row_count = std::count(rows.begin(), rows.end(), true); Please don't use abbrevs; target_row_count. https://codereview.chromium.org/13947013/diff/9001/ui/gfx/content_analysis.cc... ui/gfx/content_analysis.cc:349: unsigned tgt_column_count = std::count(columns.begin(), columns.end(), true); Please don't use abbrevs; target_column_count. https://codereview.chromium.org/13947013/diff/9001/ui/gfx/content_analysis.cc... ui/gfx/content_analysis.cc:351: if (tgt_row_count == 0 || tgt_column_count == 0) Put the checks for |target_row_count| right after you compute it. e.g.: unsigned target_row_count = std::count(rows.begin(), rows.end(), true); if (target_row_count == 0 || target_row_count == rows.size()) return NULL; Same for target_column_count. https://codereview.chromium.org/13947013/diff/9001/ui/gfx/content_analysis.h File ui/gfx/content_analysis.h (right): https://codereview.chromium.org/13947013/diff/9001/ui/gfx/content_analysis.h#... ui/gfx/content_analysis.h:61: UI_EXPORT SkBitmap* CreateRetargettedThumbnailImage( If you're transferring ownership of the result to the caller, please return it as a scoped_ptr<SkBitmap>. However, since SkBitmap uses ref-counting behind the scenes for its heap-allocated backing, please consider return the SkBitmap by value, and an empty one on failure (which is easy to test for by the caller). https://codereview.chromium.org/13947013/diff/9001/ui/gfx/content_analysis.h#... ui/gfx/content_analysis.h:65: } // namespace color_utils Nit: Add a blank line before this.
https://codereview.chromium.org/13947013/diff/9001/skia/ext/convolver.cc File skia/ext/convolver.cc (right): https://codereview.chromium.org/13947013/diff/9001/skia/ext/convolver.cc#newc... skia/ext/convolver.cc:318: if (filter.trimmed_length == 0) { On 2013/04/11 18:25:37, Alexei Svitkine wrote: > Nit: Remove {}'s. Done. https://codereview.chromium.org/13947013/diff/9001/skia/ext/convolver.cc#newc... skia/ext/convolver.cc:627: for (; i < centrepoint - r; ++i) // Padding part. On 2013/04/11 18:25:37, Alexei Svitkine wrote: > This logic seems identical to SingleChannelConvolveX1D(). Can you refactor the > common functionality to a helper function in an anon namespace? Sort of, kind of. I have plucked a fragment that was indeed repeated verbatim over and over and moved it to a common part. Where it comes to the parts of iteration, it is more complicated than that. For instance, to refactor out the four lines of code here (two four loops + shift-back, which I indeed refactored now) I would need a function with more parameters than I can count (all but one trivial). So, picking these individual parts out is probably not worth it. On the other hand, you are correct to remark that the logic of both routines is very alike. But they process the image in different order (row first, column first) and that makes abstracting parts out a bit of a bother. It is not impossible, of course. Since in reality this is all operating on 1d buffers, some clever use of inter-pixel and inter-row strides could result in basically one function invoked in two different ways. This is not so easy though and code becomes tricky to interpret because the direct reference to rows/columns gets lost. In fact, we have discussed this option with senorblanco@ and as far as I remember he agreed with me that we wouldn't do it. At least not now. Generally I'd leave this option open as I continue to work on this code, but I am not sure of it yet. https://codereview.chromium.org/13947013/diff/9001/skia/ext/convolver_unittes... File skia/ext/convolver_unittest.cc (right): https://codereview.chromium.org/13947013/diff/9001/skia/ext/convolver_unittes... skia/ext/convolver_unittest.cc:336: int img_width = kSize; On 2013/04/11 18:25:37, Alexei Svitkine wrote: > Do you need all these temp variables? > > e.g. can you just use kSize directly? > > Also, everything that's not changing after init, please make const. Nah, I don't need them. Copied as such from a routine above. Fixed up as suggested. https://codereview.chromium.org/13947013/diff/9001/skia/ext/convolver_unittes... skia/ext/convolver_unittest.cc:359: for (int x = signal_x - 2; x <= signal_x + 2; ++x) { On 2013/04/11 18:25:37, Alexei Svitkine wrote: > Nit: No need for {}'s. Done. https://codereview.chromium.org/13947013/diff/9001/skia/ext/convolver_unittes... skia/ext/convolver_unittest.cc:369: for (int y = signal_y - 2; y <= signal_y + 2; ++y) { On 2013/04/11 18:25:37, Alexei Svitkine wrote: > Nit: No need for {}'s. Done. https://codereview.chromium.org/13947013/diff/9001/skia/ext/convolver_unittes... skia/ext/convolver_unittest.cc:458: EXPECT_TRUE( On 2013/04/11 18:25:37, Alexei Svitkine wrote: > Can you use EXPECT_NEAR() for these? Done. https://codereview.chromium.org/13947013/diff/9001/ui/gfx/content_analysis.cc File ui/gfx/content_analysis.cc (right): https://codereview.chromium.org/13947013/diff/9001/ui/gfx/content_analysis.cc... ui/gfx/content_analysis.cc:34: DCHECK(front_tail_length < last - first); On 2013/04/11 18:25:37, Alexei Svitkine wrote: > Can you use DCHECK_LT? Done. https://codereview.chromium.org/13947013/diff/9001/ui/gfx/content_analysis.cc... ui/gfx/content_analysis.cc:56: for (;front_tail_length >= 0; --front_tail_length, ++i) { On 2013/04/11 18:25:37, Alexei Svitkine wrote: > Nit: Add a space after the first ; Done. https://codereview.chromium.org/13947013/diff/9001/ui/gfx/content_analysis.cc... ui/gfx/content_analysis.cc:180: intermediate.getAddr8(0, 0) + r * intermediate.rowBytes(); On 2013/04/11 17:16:38, Stephen White wrote: > Just a suggestion, but this could be intermediate.getAddr8(0, r), which save you > having to multiply by rowBytes() yourself. Done. https://codereview.chromium.org/13947013/diff/9001/ui/gfx/content_analysis.cc... ui/gfx/content_analysis.cc:182: intermediate2.getAddr8(0, 0) + r * intermediate2.rowBytes(); On 2013/04/11 17:16:38, Stephen White wrote: > Same here. Done. https://codereview.chromium.org/13947013/diff/9001/ui/gfx/content_analysis.cc... ui/gfx/content_analysis.cc:184: input_bitmap->getAddr8(0, 0) + r * input_bitmap->rowBytes(); On 2013/04/11 17:16:38, Stephen White wrote: > Same here. Done. https://codereview.chromium.org/13947013/diff/9001/ui/gfx/content_analysis.cc... ui/gfx/content_analysis.cc:216: const uint8* image_row = input_bitmap.getAddr8(0, 0) + On 2013/04/11 17:16:38, Stephen White wrote: > Same here. Done. https://codereview.chromium.org/13947013/diff/9001/ui/gfx/content_analysis.cc... ui/gfx/content_analysis.cc:326: m2 = t2 / w2; On 2013/04/11 18:25:37, Alexei Svitkine wrote: > Does the algorithm ensure that w1 and w2 are never 0? > > If not, add a DCHECK or an early return. I believe it effectively does. First of all, if min == max (or even if they are too close in numerical sense), it is an early exit. If they are not the same and are distinguishable, the float range is scaled linearly onto 0-255, assuring that: a) there is something at index 0 (thus, w1 != 0) b) there is at least one other bin (thus, w2 != 0). https://codereview.chromium.org/13947013/diff/9001/ui/gfx/content_analysis.cc... ui/gfx/content_analysis.cc:348: unsigned tgt_row_count = std::count(rows.begin(), rows.end(), true); On 2013/04/11 18:25:37, Alexei Svitkine wrote: > Please don't use abbrevs; target_row_count. Done. https://codereview.chromium.org/13947013/diff/9001/ui/gfx/content_analysis.cc... ui/gfx/content_analysis.cc:349: unsigned tgt_column_count = std::count(columns.begin(), columns.end(), true); On 2013/04/11 18:25:37, Alexei Svitkine wrote: > Please don't use abbrevs; target_column_count. Done. https://codereview.chromium.org/13947013/diff/9001/ui/gfx/content_analysis.cc... ui/gfx/content_analysis.cc:351: if (tgt_row_count == 0 || tgt_column_count == 0) On 2013/04/11 18:25:37, Alexei Svitkine wrote: > Put the checks for |target_row_count| right after you compute it. > > e.g.: > > unsigned target_row_count = std::count(rows.begin(), rows.end(), true); > if (target_row_count == 0 || target_row_count == rows.size()) > return NULL; > > Same for target_column_count. This isn't quite what the code does. The second check uses && operator, rather than || (for a reason). https://codereview.chromium.org/13947013/diff/9001/ui/gfx/content_analysis.h File ui/gfx/content_analysis.h (right): https://codereview.chromium.org/13947013/diff/9001/ui/gfx/content_analysis.h#... ui/gfx/content_analysis.h:61: UI_EXPORT SkBitmap* CreateRetargettedThumbnailImage( On 2013/04/11 18:25:37, Alexei Svitkine wrote: > If you're transferring ownership of the result to the caller, please return it > as a scoped_ptr<SkBitmap>. > > However, since SkBitmap uses ref-counting behind the scenes for its > heap-allocated backing, please consider return the SkBitmap by value, and an > empty one on failure (which is easy to test for by the caller). Done. https://codereview.chromium.org/13947013/diff/9001/ui/gfx/content_analysis.h#... ui/gfx/content_analysis.h:65: } // namespace color_utils On 2013/04/11 18:25:37, Alexei Svitkine wrote: > Nit: Add a blank line before this. Done.
https://codereview.chromium.org/13947013/diff/54001/skia/ext/convolver.cc File skia/ext/convolver.cc (right): https://codereview.chromium.org/13947013/diff/54001/skia/ext/convolver.cc#new... skia/ext/convolver.cc:573: int overlap_taps = image_size.width() - c + centrepoint; Does this handle the case where the image width is less than the filter width?
On 2013/04/12 12:15:21, Stephen White wrote: > https://codereview.chromium.org/13947013/diff/54001/skia/ext/convolver.cc > File skia/ext/convolver.cc (right): > > https://codereview.chromium.org/13947013/diff/54001/skia/ext/convolver.cc#new... > skia/ext/convolver.cc:573: int overlap_taps = image_size.width() - c + > centrepoint; > Does this handle the case where the image width is less than the filter width? Nope. This is not a practical consideration. DCHECK and early exit, I presume?
On 2013/04/12 12:29:44, motek. wrote: > On 2013/04/12 12:15:21, Stephen White wrote: > > https://codereview.chromium.org/13947013/diff/54001/skia/ext/convolver.cc > > File skia/ext/convolver.cc (right): > > > > > https://codereview.chromium.org/13947013/diff/54001/skia/ext/convolver.cc#new... > > skia/ext/convolver.cc:573: int overlap_taps = image_size.width() - c + > > centrepoint; > > Does this handle the case where the image width is less than the filter width? > > Nope. This is not a practical consideration. DCHECK and early exit, I presume? As long as the user/content can't cause that condition, that sounds fine.
On 2013/04/12 12:41:50, Stephen White wrote: > On 2013/04/12 12:29:44, motek. wrote: > > On 2013/04/12 12:15:21, Stephen White wrote: > > > https://codereview.chromium.org/13947013/diff/54001/skia/ext/convolver.cc > > > File skia/ext/convolver.cc (right): > > > > > > > > > https://codereview.chromium.org/13947013/diff/54001/skia/ext/convolver.cc#new... > > > skia/ext/convolver.cc:573: int overlap_taps = image_size.width() - c + > > > centrepoint; > > > Does this handle the case where the image width is less than the filter > width? > > > > Nope. This is not a practical consideration. DCHECK and early exit, I presume? > > As long as the user/content can't cause that condition, that sounds fine. Done so.
LGTM
Thanks for addressing my comments, code looks good now. However, I'm not sure content_analysis.cc belongs in ui/gfx. You can think of ui/gfx as something like base/ but for graphics-related utility code. So, for something to go there, it should be something that will be used by multiple different users in the codebase. Looking at the new code you're adding, it's not clear to me that this will be the case. Do you expect that this code will need to be used by many different places? If not, I suggest moving the code closer to the feature code that will be using it (i.e. in the same directory). We can always move it out later, if we find there's a compelling reason to do so.
On 2013/04/12 14:40:52, Alexei Svitkine wrote: > Thanks for addressing my comments, code looks good now. > > However, I'm not sure content_analysis.cc belongs in ui/gfx. You can think of > ui/gfx as something like base/ but for graphics-related utility code. > > So, for something to go there, it should be something that will be used by > multiple different users in the codebase. Looking at the new code you're adding, > it's not clear to me that this will be the case. > > Do you expect that this code will need to be used by many different places? If > not, I suggest moving the code closer to the feature code that will be using it > (i.e. in the same directory). We can always move it out later, if we find > there's a compelling reason to do so. Moved to browser/thumbnails
LGTM https://codereview.chromium.org/13947013/diff/68001/ui/gfx/content_analysis_u... File ui/gfx/content_analysis_unittest.cc (right): https://codereview.chromium.org/13947013/diff/68001/ui/gfx/content_analysis_u... ui/gfx/content_analysis_unittest.cc:55: SkColor clr_left = bitmap_left.getColor(origin_left.x() + c, Nit: Don't use abbrevs.
https://codereview.chromium.org/13947013/diff/68001/ui/gfx/content_analysis_u... File ui/gfx/content_analysis_unittest.cc (right): https://codereview.chromium.org/13947013/diff/68001/ui/gfx/content_analysis_u... ui/gfx/content_analysis_unittest.cc:55: SkColor clr_left = bitmap_left.getColor(origin_left.x() + c, On 2013/04/15 13:15:36, Alexei Svitkine wrote: > Nit: Don't use abbrevs. Done.
When you add someone you want an owner review from to a CL, please add a sentence like "thakis: OWNERS for chrome/". I had to figure out what you wanted me to do by reading the comments on the CL. lgtm
Message was sent while issue was closed.
Committed patchset #8 manually as r194565 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
