[Telemetry] Migrate bitmap.py from bitmaptools.cc to numpy

[Telemetry] Migrate bitmap.py from bitmaptools.cc to openCV

Some measurements from my testing:
Diff is ~8700x as fast.(!!!)
IsEqual is ~1400x as fast when tolerance is specified.(!)
FromPngFile is ~100x as fast
HistogramDistance is ~2x as fast
ColorHistogram is ~4x as fast
GetBoundingBox is ~3x as fast

BUG=425299, 437525
Committed: https://crrev.com/cc2f73e5c07adf639755d621155e4653863887a5
Cr-Commit-Position: refs/heads/master@{#308842}

[mthiesse, 2014-10-20 21:48:58]

Patchset #1 (id:1) has been deleted

[mthiesse, 2014-10-20 21:49:04]

Patchset #1 (id:20001) has been deleted

[mthiesse, 2014-10-20 21:49:10]

Patchset #1 (id:40001) has been deleted

[mthiesse, 2014-10-20 21:50:55]

Patchset #1 (id:60001) has been deleted

[mthiesse, 2014-10-21 14:44:40]

Patchset #1 (id:80001) has been deleted

[mthiesse, 2014-10-21 14:53:17]

mthiesse@chromium.org changed reviewers:
+ flackr@chromium.org, szym@chromium.org, tonyg@chromium.org

[mthiesse, 2014-10-21 14:53:18]

Hi Rob, Tony, szym@

PTAL. I was hoping to get rid of the dependency on png too, but it looks like
code elsewhere relies on decoding base64 strings as pngs.

[mthiesse, 2014-10-21 16:00:53]

Patchset #2 (id:120001) has been deleted

[tonyg, 2014-10-21 16:12:56]

tonyg@chromium.org changed reviewers:
+ dtu@chromium.org
- szym@chromium.org

[tonyg, 2014-10-21 16:12:57]

Overall this looks great. Just a few minor things and I think this'll be ready
to go once we have numpy/opencv everywhere.

https://codereview.chromium.org/668753002/diff/140001/content/test/gpu/gpu_te...
File content/test/gpu/gpu_tests/cloud_storage_test_base.py (right):

https://codereview.chromium.org/668753002/diff/140001/content/test/gpu/gpu_te...
content/test/gpu/gpu_tests/cloud_storage_test_base.py:12: import cv2
Nit: alphabetize

https://codereview.chromium.org/668753002/diff/140001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/bitmap.py (right):

https://codereview.chromium.org/668753002/diff/140001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/bitmap.py:220: def ColorHistogram(self,
ignore_color=None, tolerance=0):
Just curious how the runtime differs between the new and old methods. The old
ones were already a little too slow at calculating the speed index, I hope the
new ones are faster.

https://codereview.chromium.org/668753002/diff/140001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/bitmap_unittest.py (right):

https://codereview.chromium.org/668753002/diff/140001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/bitmap_unittest.py:9: import cv2
alphabetize

https://codereview.chromium.org/668753002/diff/140001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/bitmap_unittest.py:99: @benchmark.Disabled
We should probably enable this and the other disabled tests along with this
patch, right?

https://codereview.chromium.org/668753002/diff/140001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/tab.py (right):

https://codereview.chromium.org/668753002/diff/140001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/tab.py:117: int((color[2] << 16) | (color[1] <<
8) | color[0]), 5)
The inlined bit shifting makes me miss some sort of helper in bitmap for getting
colors. Any ideas for a better API?

https://codereview.chromium.org/668753002/diff/140001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/video.py (left):

https://codereview.chromium.org/668753002/diff/140001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/video.py:22: assert not
video_file_obj.close_called
Were these asserts incorrect?

[mthiesse, 2014-10-21 18:40:58]

https://codereview.chromium.org/668753002/diff/140001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/bitmap.py (right):

https://codereview.chromium.org/668753002/diff/140001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/bitmap.py:220: def ColorHistogram(self,
ignore_color=None, tolerance=0):
On 2014/10/21 16:12:56, tonyg wrote:
> Just curious how the runtime differs between the new and old methods. The old
> ones were already a little too slow at calculating the speed index, I hope the
> new ones are faster.

So, this is the test I'm running:
b1 = Bitmap.FromImageFile('test1.png')
b2 = Bitmap.FromImageFile('test2.png')
import time
start_time = time.time()
for i in xrange(100):
  hist1 = b1.ColorHistogram()
  hist2 = b2.ColorHistogram()
  hist1.Distance(hist2)
print time.time() - start_time

test1 and test2.png are 1080p screenshots.
master branch: 20.439s
my branch: 17.764s
Also, in my branch, most of the runtime comes from data type conversions.

These times exclude the PNG reading, which takes ~9.6s on master branch, and
~0.4s on my branch.

Now, if speed is really a concern, we shouldn't be using Earth Mover's Distance
at all; it's slow. Also, EMD, from my understanding of some papers I skimmed
through, is best for full image differences, not histogram differences. If we
were to switch to Bhattacharyya distance
(http://en.wikipedia.org/wiki/Bhattacharyya_distance), which has much better
openCV support than EMD, the above test runs in 0.814s!

[tonyg, 2014-10-21 19:05:39]

tonyg@chromium.org changed reviewers:
+ pmeenan@chromium.org

[tonyg, 2014-10-21 19:06:29]

On 2014/10/21 18:40:58, mthiesse wrote:
>
https://codereview.chromium.org/668753002/diff/140001/tools/telemetry/telemet...
> File tools/telemetry/telemetry/core/bitmap.py (right):
> 
>
https://codereview.chromium.org/668753002/diff/140001/tools/telemetry/telemet...
> tools/telemetry/telemetry/core/bitmap.py:220: def ColorHistogram(self,
> ignore_color=None, tolerance=0):
> On 2014/10/21 16:12:56, tonyg wrote:
> > Just curious how the runtime differs between the new and old methods. The
old
> > ones were already a little too slow at calculating the speed index, I hope
the
> > new ones are faster.
> 
> So, this is the test I'm running:
> b1 = Bitmap.FromImageFile('test1.png')
> b2 = Bitmap.FromImageFile('test2.png')
> import time
> start_time = time.time()
> for i in xrange(100):
>   hist1 = b1.ColorHistogram()
>   hist2 = b2.ColorHistogram()
>   hist1.Distance(hist2)
> print time.time() - start_time
> 
> test1 and test2.png are 1080p screenshots.
> master branch: 20.439s
> my branch: 17.764s
> Also, in my branch, most of the runtime comes from data type conversions.
> 
> These times exclude the PNG reading, which takes ~9.6s on master branch, and
> ~0.4s on my branch.

Nice :)

> 
> Now, if speed is really a concern, we shouldn't be using Earth Mover's
Distance
> at all; it's slow. Also, EMD, from my understanding of some papers I skimmed
> through, is best for full image differences, not histogram differences. If we
> were to switch to Bhattacharyya distance
> (http://en.wikipedia.org/wiki/Bhattacharyya_distance), which has much better
> openCV support than EMD, the above test runs in 0.814s!

That sounds super-promising. Dave, Pat, any opinions on Bhattacharyya distance
for Speed Index?

[flackr, 2014-10-21 21:50:11]

Just a few comments.

https://codereview.chromium.org/668753002/diff/140001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/bitmap.py (right):

https://codereview.chromium.org/668753002/diff/140001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/bitmap.py:195: top = 1e16
When possible, using legitimate input values is safer, i.e. left = width and top
= height.

https://codereview.chromium.org/668753002/diff/140001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/bitmap.py:239: hist_r[max(ignore_r - tolerance,
0):ignore_r + tolerance + 1] = 0
If I'm reading this correctly. It seems like this will ignore all red, green and
blue values which are individually near one of the ignore color components.
Isn't the intent of ignore_color only to ignore when all three components are
near that color?

https://codereview.chromium.org/668753002/diff/140001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/bitmap_unittest.py (right):

https://codereview.chromium.org/668753002/diff/140001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/bitmap_unittest.py:19: T8tgwbJAAAAABJRU5ErkJggg==
Curious, if the PNG still contains the same thing, what changed about it?

[slamm, 2014-10-21 23:21:57]

slamm@chromium.org changed reviewers:
+ slamm@chromium.org

[slamm, 2014-10-21 23:21:58]

A few style suggestions.

https://codereview.chromium.org/668753002/diff/140001/content/test/gpu/gpu_te...
File content/test/gpu/gpu_tests/cloud_storage_test_base.py (right):

https://codereview.chromium.org/668753002/diff/140001/content/test/gpu/gpu_te...
content/test/gpu/gpu_tests/cloud_storage_test_base.py:40:
expectation["color"][2])
Nit. Follow style-guide for indentation.

https://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Inden...

One of two options:

expected_color = (
    expectation["color"][0], expectation["color"][1], expectation["color"][2])

expected_color = (expectation["color"][0], expectation["color"][1],
                             expectation["color"][3])

https://codereview.chromium.org/668753002/diff/140001/content/test/gpu/gpu_te...
content/test/gpu/gpu_tests/cloud_storage_test_base.py:42:
expectation["tolerance"]):
Fix indentation (same issue as line 39).

https://codereview.chromium.org/668753002/diff/140001/content/test/gpu/gpu_te...
content/test/gpu/gpu_tests/cloud_storage_test_base.py:48: str(actual_color[0]) +
"]")
Nit that precedes you. Use '%' formatting:

raise page_test.Failure(
    'Expected pixel at %s to be %s but got [%s, %s, %s]' %
    location, expectation["color"], actual_color[2], actual_color[1],
actual_color[0])

https://codereview.chromium.org/668753002/diff/140001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/bitmap.py (right):

https://codereview.chromium.org/668753002/diff/140001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/bitmap.py:15: 
Drop the blank line.

https://codereview.chromium.org/668753002/diff/140001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/bitmap.py:16: import numpy as np
Just "import numpy"?

https://codereview.chromium.org/668753002/diff/140001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/bitmap.py:54: """ Tests whether two colors are
equal within the given tolerance.
No space after triple quotes.

https://codereview.chromium.org/668753002/diff/140001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/bitmap.py:65: return True
return all(abs(color1[i]  - color2[i]) <= tolerance
           for i in xrange(color_depth))

https://codereview.chromium.org/668753002/diff/140001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/bitmap.py:102: """ Utilities for parsing and
inspecting a bitmap.
No Space after triple quotes.

https://codereview.chromium.org/668753002/diff/140001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/bitmap.py:109: """ Initializes the Bitmap object.
Here too.

https://codereview.chromium.org/668753002/diff/140001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/bitmap.py:166: if np.amax(diff_img) > tolerance:
return np.amax(diff_imb) <= tolerance

[mthiesse, 2014-10-22 15:33:10]

https://codereview.chromium.org/668753002/diff/140001/content/test/gpu/gpu_te...
File content/test/gpu/gpu_tests/cloud_storage_test_base.py (right):

https://codereview.chromium.org/668753002/diff/140001/content/test/gpu/gpu_te...
content/test/gpu/gpu_tests/cloud_storage_test_base.py:12: import cv2
On 2014/10/21 16:12:56, tonyg wrote:
> Nit: alphabetize

Done.

https://codereview.chromium.org/668753002/diff/140001/content/test/gpu/gpu_te...
content/test/gpu/gpu_tests/cloud_storage_test_base.py:40:
expectation["color"][2])
On 2014/10/21 23:21:58, slamm wrote:
> Nit. Follow style-guide for indentation.
> 
>
https://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Inden...
> 
> One of two options:
> 
> expected_color = (
>     expectation["color"][0], expectation["color"][1], expectation["color"][2])
> 
> expected_color = (expectation["color"][0], expectation["color"][1],
>                              expectation["color"][3])

Done.

https://codereview.chromium.org/668753002/diff/140001/content/test/gpu/gpu_te...
content/test/gpu/gpu_tests/cloud_storage_test_base.py:42:
expectation["tolerance"]):
On 2014/10/21 23:21:58, slamm wrote:
> Fix indentation (same issue as line 39).

Done.

https://codereview.chromium.org/668753002/diff/140001/content/test/gpu/gpu_te...
content/test/gpu/gpu_tests/cloud_storage_test_base.py:48: str(actual_color[0]) +
"]")
On 2014/10/21 23:21:58, slamm wrote:
> Nit that precedes you. Use '%' formatting:
> 
> raise page_test.Failure(
>     'Expected pixel at %s to be %s but got [%s, %s, %s]' %
>     location, expectation["color"], actual_color[2], actual_color[1],
> actual_color[0])

Done.

https://codereview.chromium.org/668753002/diff/140001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/bitmap.py (right):

https://codereview.chromium.org/668753002/diff/140001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/bitmap.py:15: 
On 2014/10/21 23:21:58, slamm wrote:
> Drop the blank line.

Done.

https://codereview.chromium.org/668753002/diff/140001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/bitmap.py:16: import numpy as np
On 2014/10/21 23:21:58, slamm wrote:
> Just "import numpy"?

Well, it seems to be convention in python to import numpy as np. However, I
don't have a strong opinion either way, so if you insist I'll change it.

https://codereview.chromium.org/668753002/diff/140001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/bitmap.py:54: """ Tests whether two colors are
equal within the given tolerance.
On 2014/10/21 23:21:58, slamm wrote:
> No space after triple quotes.

Done.

https://codereview.chromium.org/668753002/diff/140001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/bitmap.py:65: return True
On 2014/10/21 23:21:58, slamm wrote:
> return all(abs(color1[i]  - color2[i]) <= tolerance
>            for i in xrange(color_depth))

Done.

https://codereview.chromium.org/668753002/diff/140001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/bitmap.py:102: """ Utilities for parsing and
inspecting a bitmap.
On 2014/10/21 23:21:58, slamm wrote:
> No Space after triple quotes.

Done.

https://codereview.chromium.org/668753002/diff/140001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/bitmap.py:109: """ Initializes the Bitmap object.
On 2014/10/21 23:21:58, slamm wrote:
> Here too.

Done.

https://codereview.chromium.org/668753002/diff/140001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/bitmap.py:166: if np.amax(diff_img) > tolerance:
On 2014/10/21 23:21:58, slamm wrote:
> return np.amax(diff_imb) <= tolerance

Done.

https://codereview.chromium.org/668753002/diff/140001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/bitmap.py:195: top = 1e16
On 2014/10/21 21:50:10, flackr wrote:
> When possible, using legitimate input values is safer, i.e. left = width and
top
> = height.

Done.

https://codereview.chromium.org/668753002/diff/140001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/bitmap.py:239: hist_r[max(ignore_r - tolerance,
0):ignore_r + tolerance + 1] = 0
On 2014/10/21 21:50:10, flackr wrote:
> If I'm reading this correctly. It seems like this will ignore all red, green
and
> blue values which are individually near one of the ignore color components.
> Isn't the intent of ignore_color only to ignore when all three components are
> near that color?

Ah, yes, good catch. Fixed and added a test case for this.

https://codereview.chromium.org/668753002/diff/140001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/bitmap_unittest.py (right):

https://codereview.chromium.org/668753002/diff/140001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/bitmap_unittest.py:9: import cv2
On 2014/10/21 16:12:57, tonyg wrote:
> alphabetize

Done.

https://codereview.chromium.org/668753002/diff/140001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/bitmap_unittest.py:19: T8tgwbJAAAAABJRU5ErkJggg==
On 2014/10/21 21:50:10, flackr wrote:
> Curious, if the PNG still contains the same thing, what changed about it?

It's the same PNG, but properly compressed :P Looking at the string I had a
strong feeling it was far too long, and wanted to see if I was right :P

https://codereview.chromium.org/668753002/diff/140001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/bitmap_unittest.py:99: @benchmark.Disabled
On 2014/10/21 16:12:57, tonyg wrote:
> We should probably enable this and the other disabled tests along with this
> patch, right?

Do you happen to know why all of these tests were disabled?

https://codereview.chromium.org/668753002/diff/140001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/tab.py (right):

https://codereview.chromium.org/668753002/diff/140001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/tab.py:117: int((color[2] << 16) | (color[1] <<
8) | color[0]), 5)
On 2014/10/21 16:12:57, tonyg wrote:
> The inlined bit shifting makes me miss some sort of helper in bitmap for
getting
> colors. Any ideas for a better API?

Done.

https://codereview.chromium.org/668753002/diff/140001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/video.py (left):

https://codereview.chromium.org/668753002/diff/140001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/video.py:22: assert not
video_file_obj.close_called
On 2014/10/21 16:12:57, tonyg wrote:
> Were these asserts incorrect?

Yeah, these are bizarre assertions, because the video_file_obj passed is just a
string. Removing them allows the test to pass. Is this why it was disabled?

At any rate, I'll re-enable the test because it passes now.

[slamm, 2014-10-22 16:41:22]

https://codereview.chromium.org/668753002/diff/140001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/bitmap.py (right):

https://codereview.chromium.org/668753002/diff/140001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/bitmap.py:16: import numpy as np
On 2014/10/22 15:33:10, mthiesse wrote:
> On 2014/10/21 23:21:58, slamm wrote:
> > Just "import numpy"?
> 
> Well, it seems to be convention in python to import numpy as np. However, I
> don't have a strong opinion either way, so if you insist I'll change it.

It does not seem like an important enough detail to insist. Thanks for making
the other edits.

[flackr, 2014-10-22 19:03:39]

lgtm

[tonyg, 2014-10-22 19:35:22]

lgtm (once this works across platforms)

[slamm, 2014-10-22 21:15:50]

lgtm. BTW, at one point, Pat compared different histogram distance algorithms.
The earth movers algorithm was not the best. Also, we found that the video
bitrate made a difference. It is set to 4, and Telemetry product better numbers
when it was set to 8.

[szym, 2014-10-23 05:56:43]

overall lgtm

The new BoundingBox code is much slower (a plain python loop over all pixels),
and could be sped up significantly by using numpy or opencv (cv2.findCountours
and cv2.boundingRect), although the code would be a bit less obvious to
accommodate tolerance. That said, BoundingBox is used only to detect the initial
color flash so applies to only a fraction of all video frames.

[slamm, 2014-10-23 17:46:33]

https://codereview.chromium.org/668753002/diff/160001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/bitmap.py (right):

https://codereview.chromium.org/668753002/diff/160001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/bitmap.py:210: if ColorsAreEqual(img[y][x],
color, tolerance):
Short of the alternatives @symn mentioned, would indexing in one step give a
performance boost?

I am unfamiliar with these libraries, however, it looks like you can do one of
the following:

if ColorsAreEqual(img.item(y, x), color, tolerrance):
if ColorsAreEqual(img[y, x], color, tolerrance):

http://docs.opencv.org/trunk/doc/py_tutorials/py_core/py_basic_ops/py_basic_o...

[mthiesse, 2014-10-23 21:45:36]

Patchset #4 (id:180001) has been deleted

[mthiesse, 2014-10-23 21:51:51]

Great suggestions szym,

I guess I didn't realize just how slow python was :P
Ran some tests using a 1000x1000 image, with a rect covering ~50% of the image.
Runtime:
Previous patch set: ~19s
After much optimization (without using contours): ~3s
Master branch: ~0.04s
Current patch set (using contours): ~0.008s
(Yes, faster!)

https://codereview.chromium.org/668753002/diff/160001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/bitmap.py (right):

https://codereview.chromium.org/668753002/diff/160001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/bitmap.py:210: if ColorsAreEqual(img[y][x],
color, tolerance):
On 2014/10/23 17:46:32, slamm wrote:
> Short of the alternatives @symn mentioned, would indexing in one step give a
> performance boost?
> 
> I am unfamiliar with these libraries, however, it looks like you can do one of
> the following:
> 
> if ColorsAreEqual(img.item(y, x), color, tolerrance):
> if ColorsAreEqual(img[y, x], color, tolerrance):
> 
>
http://docs.opencv.org/trunk/doc/py_tutorials/py_core/py_basic_ops/py_basic_o...

FYI, in testing, it appears that img[y, x] is slower than img[y][x].

[slamm, 2014-10-23 22:57:44]

Nice!

https://codereview.chromium.org/668753002/diff/200001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/bitmap.py (right):

https://codereview.chromium.org/668753002/diff/200001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/bitmap.py:198: Ignores the alpha channel."""
Would you update the doc string to match our style guide while you are here?

One possibility:

"""Finds the minimum box surrounding all occurrences of bgr |color|.

Ignores the alpha channel.

Args:
  color: (b, g, r)  or (b, g, r, a); bounding box color.
  tolerance: tolerance for the bounding box color as an integer.

Returns:
  (top, left, width, height), match_count
"""

https://codereview.chromium.org/668753002/diff/200001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/bitmap.py:204: contours, _ =
cv2.findContours(img, cv2.RETR_LIST, cv2.CHAIN_APPROX_NONE)
Use cv2.CV_CHAIN_APPROX_SIMPLE?

CV_CHAIN_APPROX_SIMPLE compresses horizontal, vertical, and diagonal segments
and leaves only their end points. For example, an up-right rectangular contour
is encoded with 4 points.
http://docs.opencv.org/modules/imgproc/doc/structural_analysis_and_shape_desc...

https://codereview.chromium.org/668753002/diff/200001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/bitmap.py:205: return
cv2.boundingRect(contours[0]), count
I saw some talk of finding the contour with the maximum area. 

http://stackoverflow.com/questions/16538774/dealing-with-contours-and-boundin...

Perhaps you could just check that exactly one contour gets created.

[szym, 2014-10-23 23:17:06]

szym@chromium.org changed reviewers:
+ szym@chromium.org

[szym, 2014-10-23 23:17:07]

https://codereview.chromium.org/668753002/diff/200001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/bitmap.py (right):

https://codereview.chromium.org/668753002/diff/200001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/bitmap.py:204: contours, _ =
cv2.findContours(img, cv2.RETR_LIST, cv2.CHAIN_APPROX_NONE)
On 2014/10/23 22:57:44, slamm wrote:
> Use cv2.CV_CHAIN_APPROX_SIMPLE?
> 
> CV_CHAIN_APPROX_SIMPLE compresses horizontal, vertical, and diagonal segments
> and leaves only their end points. For example, an up-right rectangular contour
> is encoded with 4 points.
>
http://docs.opencv.org/modules/imgproc/doc/structural_analysis_and_shape_desc...

Since this is followed by boundingRect, I'm not sure approximating is going to
speed things much (I am guessing computing bounding box is easier than
simplifying a contour). CV_RETR_EXTERNAL might be more useful, but given that
we're dealing with something that is likely a solid color rectangle, I don't
think you'll see more than one contour in practice. I think mthiesse already hit
>99% of the optimization potential.

[mthiesse, 2014-10-24 14:48:12]

https://codereview.chromium.org/668753002/diff/200001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/bitmap.py (right):

https://codereview.chromium.org/668753002/diff/200001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/bitmap.py:198: Ignores the alpha channel."""
On 2014/10/23 22:57:44, slamm wrote:
> Would you update the doc string to match our style guide while you are here?
> 
> One possibility:
> 
> """Finds the minimum box surrounding all occurrences of bgr |color|.
> 
> Ignores the alpha channel.
> 
> Args:
>   color: (b, g, r)  or (b, g, r, a); bounding box color.
>   tolerance: tolerance for the bounding box color as an integer.
> 
> Returns:
>   (top, left, width, height), match_count
> """

Done.

https://codereview.chromium.org/668753002/diff/200001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/bitmap.py:204: contours, _ =
cv2.findContours(img, cv2.RETR_LIST, cv2.CHAIN_APPROX_NONE)
On 2014/10/23 23:17:07, szym wrote:
> On 2014/10/23 22:57:44, slamm wrote:
> > Use cv2.CV_CHAIN_APPROX_SIMPLE?
> > 
> > CV_CHAIN_APPROX_SIMPLE compresses horizontal, vertical, and diagonal
segments
> > and leaves only their end points. For example, an up-right rectangular
contour
> > is encoded with 4 points.
> >
>
http://docs.opencv.org/modules/imgproc/doc/structural_analysis_and_shape_desc...
> 
> Since this is followed by boundingRect, I'm not sure approximating is going to
> speed things much (I am guessing computing bounding box is easier than
> simplifying a contour). CV_RETR_EXTERNAL might be more useful, but given that
> we're dealing with something that is likely a solid color rectangle, I don't
> think you'll see more than one contour in practice. I think mthiesse already
hit
> >99% of the optimization potential.

Acknowledged.

https://codereview.chromium.org/668753002/diff/200001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/bitmap.py:205: return
cv2.boundingRect(contours[0]), count
On 2014/10/23 22:57:44, slamm wrote:
> I saw some talk of finding the contour with the maximum area. 
> 
>
http://stackoverflow.com/questions/16538774/dealing-with-contours-and-boundin...
> 
> Perhaps you could just check that exactly one contour gets created.

Okay, what I've done instead is concatenated any contours (sets of points), then
computed the bounding box on that.

In the common case (a single rectangle) this will be a no-op, but for degenerate
cases, or for different future use cases where there might not be a nice
rectangle, this should compute a bounding box surrounding all occurrences of the
color correctly, and match the behavior of the bitmaptools.cc code.

[slamm, 2014-10-27 16:32:32]

lgtm

[mthiesse, 2014-11-26 22:24:16]

This may warrant another look,

I've made the openCV dependency in this CL optional, replacing openCV calls with
numpy calls, since numpy is supported cross-platform, while openCV is not.

[mthiesse, 2014-12-09 16:39:28]

Patchset #9 (id:300001) has been deleted

[mthiesse, 2014-12-09 16:55:09]

Alright, now this definitely requires another look :)

I've made all dependencies optional by moving the new implementation behind a
common interface.

[mthiesse, 2014-12-15 18:57:51]

Patchset #14 (id:420001) has been deleted

[mthiesse, 2014-12-15 19:34:47]

Patchset #15 (id:460001) has been deleted

[slamm, 2014-12-16 22:09:42]

On 2014/12/09 16:55:09, mthiesse wrote:
> Alright, now this definitely requires another look :)
> 
> I've made all dependencies optional by moving the new implementation behind a
> common interface.

LGTM - The optional dependencies approach looks nice.

[mthiesse, 2014-12-16 22:14:51]

mthiesse@chromium.org changed reviewers:
+ kbr@chromium.org

[mthiesse, 2014-12-16 22:14:51]

+kbr for content/test/gpu/ OWNERS

[Ken Russell (switch to Gerrit), 2014-12-16 23:41:47]

LGTM overall.

Please document somewhere exactly what the requirements are to take the new code
paths.

Also, please watch this bot:
http://build.chromium.org/p/chromium.gpu/builders/Android%20Debug%20(Nexus%207)

after this lands. That configuration isn't covered by try servers. If it breaks,
please revert, and let me know if you need help fixing it. Thanks.

https://codereview.chromium.org/668753002/diff/490001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/video.py (right):

https://codereview.chromium.org/668753002/diff/490001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/video.py:9: from telemetry.image_processing
import image_util
Alphabetize imports.

https://codereview.chromium.org/668753002/diff/490001/tools/telemetry/telemet...
File tools/telemetry/telemetry/image_processing/image_util_impl.py (right):

https://codereview.chromium.org/668753002/diff/490001/tools/telemetry/telemet...
tools/telemetry/telemetry/image_processing/image_util_impl.py:1: # Copyright
2013 The Chromium Authors. All rights reserved.
I think this file should be named more concretely "image_util_opencv_impl.py".
Also, the copyright year should be updated.

https://codereview.chromium.org/668753002/diff/490001/tools/telemetry/telemet...
File tools/telemetry/telemetry/image_processing/rgba_color.py (right):

https://codereview.chromium.org/668753002/diff/490001/tools/telemetry/telemet...
tools/telemetry/telemetry/image_processing/rgba_color.py:8: """Encapsulates an
RGBA color retreived from a Bitmap"""
typo: retrieved

[achuithb, 2014-12-17 02:34:39]

achuith@chromium.org changed reviewers:
+ achuith@chromium.org, ihf@chromium.org

[achuithb, 2014-12-17 02:34:40]

There's a good chance this will also break on cros.

[szym, 2014-12-17 05:15:26]

lgtm

https://codereview.chromium.org/668753002/diff/490001/tools/telemetry/telemet...
File tools/telemetry/telemetry/image_processing/image_util_impl.py (right):

https://codereview.chromium.org/668753002/diff/490001/tools/telemetry/telemet...
tools/telemetry/telemetry/image_processing/image_util_impl.py:123: w =
np.where(((b >= colorm[0]) & (b <= colorp[0]) &
Because it enumerates all matching locations, numpy.where is likely not as fast
as cv2.findContour, and the size of |w| will be very large for the matching
frame (since the flash of target |color| will fill most of the screen).

I'd suggest to benchmark against cv2 and, if the performance warrants it, use
the inRange|findContours|boundingRect approach if cv2 is available.

[mthiesse, 2014-12-17 16:56:10]

https://codereview.chromium.org/668753002/diff/490001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/video.py (right):

https://codereview.chromium.org/668753002/diff/490001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/video.py:9: from telemetry.image_processing
import image_util
On 2014/12/16 23:41:47, Ken Russell wrote:
> Alphabetize imports.

Done.

https://codereview.chromium.org/668753002/diff/490001/tools/telemetry/telemet...
File tools/telemetry/telemetry/image_processing/image_util_impl.py (right):

https://codereview.chromium.org/668753002/diff/490001/tools/telemetry/telemet...
tools/telemetry/telemetry/image_processing/image_util_impl.py:1: # Copyright
2013 The Chromium Authors. All rights reserved.
On 2014/12/16 23:41:47, Ken Russell wrote:
> I think this file should be named more concretely "image_util_opencv_impl.py".
> Also, the copyright year should be updated.

Done. I've used numpy instead of opencv in the name, it's more accurate.

https://codereview.chromium.org/668753002/diff/490001/tools/telemetry/telemet...
tools/telemetry/telemetry/image_processing/image_util_impl.py:123: w =
np.where(((b >= colorm[0]) & (b <= colorp[0]) &
On 2014/12/17 05:15:25, szym wrote:
> Because it enumerates all matching locations, numpy.where is likely not as
fast
> as cv2.findContour, and the size of |w| will be very large for the matching
> frame (since the flash of target |color| will fill most of the screen).
> 
> I'd suggest to benchmark against cv2 and, if the performance warrants it, use
> the inRange|findContours|boundingRect approach if cv2 is available.

Alright, I've added back the cv2 implementation for when cv2 is available. It's
about 3x as fast in the tolerance != 0 case.

I've also added a simpler numpy implementation in the case tolerance is 0, which
is almost 2x as fast.

https://codereview.chromium.org/668753002/diff/490001/tools/telemetry/telemet...
File tools/telemetry/telemetry/image_processing/rgba_color.py (right):

https://codereview.chromium.org/668753002/diff/490001/tools/telemetry/telemet...
tools/telemetry/telemetry/image_processing/rgba_color.py:8: """Encapsulates an
RGBA color retreived from a Bitmap"""
On 2014/12/16 23:41:47, Ken Russell wrote:
> typo: retrieved

Done.

[mthiesse, 2014-12-17 16:57:01]

The CQ bit was checked by mthiesse@chromium.org

[commit-bot: I haz the power, 2014-12-17 16:58:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/668753002/510001

[commit-bot: I haz the power, 2014-12-17 17:03:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-17 17:03:54]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_chromium_gn_compile_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_asan_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_ng on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[mthiesse, 2014-12-17 17:40:43]

The CQ bit was checked by mthiesse@chromium.org

[commit-bot: I haz the power, 2014-12-17 17:41:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/668753002/530001

[commit-bot: I haz the power, 2014-12-17 19:40:27]

Committed patchset #18 (id:530001)

[commit-bot: I haz the power, 2014-12-17 19:41:03]

Patchset 18 (id:??) landed as
https://crrev.com/cc2f73e5c07adf639755d621155e4653863887a5
Cr-Commit-Position: refs/heads/master@{#308842}