[telemetry] bitmaptools as a standalone executable

[telemetry] bitmaptools as a standalone executable

The C++ binary implements simple per-pixel algorithms for SpeedIndex
computation. This allows us to achieve near real-time processing without
bringing external dependencies.

The bitmaptools binary needs to be built before it can be used.
The overhead of spawning a child process for each frame is about 3ms.

BUG=323813
TEST=telemetry bitmap_unittest

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

[szym, 2014-01-17 01:15:41]

Works on my linux box. Will see what the bots say.

[tonyg, 2014-01-17 01:43:17]

lgtm

Love it! I'm happy when the bots are happy.

https://codereview.chromium.org/136793022/diff/1/build/all.gyp
File build/all.gyp (right):

https://codereview.chromium.org/136793022/diff/1/build/all.gyp#newcode268
build/all.gyp:268: '../tools/telemetry/telemetry.gyp:*',
Could you also please add this to the chromium_builder_perf and android_tools
targets?

https://codereview.chromium.org/136793022/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/bitmap.py (right):

https://codereview.chromium.org/136793022/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/bitmap.py:64: assert len(dimensions) == 7
The 7 mystifies me. Could you please add a comment or constant.

[achuithb, 2014-01-17 04:01:24]

https://codereview.chromium.org/136793022/diff/240021/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/bitmap_unittest.py (left):

https://codereview.chromium.org/136793022/diff/240021/tools/telemetry/telemet...
tools/telemetry/telemetry/core/bitmap_unittest.py:105: @DisabledTest
Could we replace this with @DisabledTestonCrOS instead everywhere? I'm expecting
these tests will fail on cros.

[szym, 2014-01-17 05:58:19]

maruel@chromium.org: Please review changes in PRESUBMIT.py making exception for
printf/fprintf use in bitmaptools.

The standalone bitmaptools uses stdio to communicate with the caller.

[bulach, 2014-01-17 10:43:44]

lgtm.. it's a shame that the extension didn't stuck, but oh well, I guess we
have too many platforms anyways.. :)

tiny nits and one suggestion below:

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

https://codereview.chromium.org/136793022/diff/410002/tools/telemetry/telemet...
tools/telemetry/telemetry/core/bitmap.py:68: self.popen.stdin.write(pixels)
nit: perhaps a self.popen.flush() here?

https://codereview.chromium.org/136793022/diff/410002/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/bitmaptools.cc (right):

https://codereview.chromium.org/136793022/diff/410002/tools/telemetry/telemet...
tools/telemetry/telemetry/core/bitmaptools.cc:8: 
nit: could put all of these in an anonymous namespace

https://codereview.chromium.org/136793022/diff/410002/tools/telemetry/telemet...
tools/telemetry/telemetry/core/bitmaptools.cc:15: bool readInt(int* out) {
nit: "R"eadInt, ditto for "W"riteResponse below..

https://codereview.chromium.org/136793022/diff/410002/tools/telemetry/telemet...
tools/telemetry/telemetry/core/bitmaptools.cc:228: Bitmap bmp;
ignorable suggestion: not quite sure how frequently this would be called, but
would it make sense to make it long-lived / stateful?
that is, have a singleton "popen" higher up in python, and then it'd be spawned
just once...
then this would need another pair of commands, "SET_BITMAP" and "QUIT", and then
all operations would happen on the latest bitmap, and this would be inside
something like a "while (!quit)" loop.

no idea how much many frames we'd be processing and what the typical operations
are, so feel free to ignore ;)

[M-A Ruel, 2014-01-17 14:01:36]

PRESUBMIT.py lgtm

https://codereview.chromium.org/136793022/diff/410002/tools/telemetry/telemet...
File tools/telemetry/telemetry.gyp (right):

https://codereview.chromium.org/136793022/diff/410002/tools/telemetry/telemet...
tools/telemetry/telemetry.gyp:6: 'targets' : [
no space

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

https://codereview.chromium.org/136793022/diff/410002/tools/telemetry/telemet...
tools/telemetry/telemetry/core/bitmap.py:4: import array
insert an empty line

What's the purpose of this file? Add it in a docstring here.

https://codereview.chromium.org/136793022/diff/410002/tools/telemetry/telemet...
tools/telemetry/telemetry/core/bitmap.py:68: self.popen.stdin.write(pixels)
On 2014/01/17 10:43:44, bulach wrote:
> nit: perhaps a self.popen.flush() here?

well, keep in mind this code could dead lock.

https://codereview.chromium.org/136793022/diff/410002/tools/telemetry/telemet...
tools/telemetry/telemetry/core/bitmap.py:73: self.popen.stdin.close()
So the object can only have one call done on it and then it is invalid? I feel
like you want a function, not a class.

https://codereview.chromium.org/136793022/diff/410002/tools/telemetry/telemet...
tools/telemetry/telemetry/core/bitmap.py:125: if self._crop_box:
optionally, if you want to save space, you do this:
return self._crop_box[2] if self._crop_box else self._width

https://codereview.chromium.org/136793022/diff/410002/tools/telemetry/telemet...
tools/telemetry/telemetry/core/bitmap.py:138: # If we got a list of ints, we
need to convert it into a byte buffer.
As a docstring?

[szym, 2014-01-17 20:38:20]

https://codereview.chromium.org/136793022/diff/410002/tools/telemetry/telemet...
File tools/telemetry/telemetry.gyp (right):

https://codereview.chromium.org/136793022/diff/410002/tools/telemetry/telemet...
tools/telemetry/telemetry.gyp:6: 'targets' : [
On 2014/01/17 14:01:37, M-A Ruel wrote:
> no space

Done.

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

https://codereview.chromium.org/136793022/diff/410002/tools/telemetry/telemet...
tools/telemetry/telemetry/core/bitmap.py:4: import array
On 2014/01/17 14:01:37, M-A Ruel wrote:
> insert an empty line
> 
> What's the purpose of this file? Add it in a docstring here.

Done.

https://codereview.chromium.org/136793022/diff/410002/tools/telemetry/telemet...
tools/telemetry/telemetry/core/bitmap.py:68: self.popen.stdin.write(pixels)
On 2014/01/17 14:01:37, M-A Ruel wrote:
> On 2014/01/17 10:43:44, bulach wrote:
> > nit: perhaps a self.popen.flush() here?
> 
> well, keep in mind this code could dead lock.

We don't read from popen.stdout until _RunCommand where we popen.close(), so I
don't think a flush is necessary here.

I also don't think there's a chance of deadlock here. The bitmaptools binary
reads the whole expected input before writing anything out.

https://codereview.chromium.org/136793022/diff/410002/tools/telemetry/telemet...
tools/telemetry/telemetry/core/bitmap.py:73: self.popen.stdin.close()
On 2014/01/17 14:01:37, M-A Ruel wrote:
> So the object can only have one call done on it and then it is invalid? I feel
> like you want a function, not a class.

I want to reuse _RunCommand and __init__. I tried with @classmethod but it's
awkward to start each function with:
popen = _BitmapTools._Init(bitmap), and there is no way to curry the call, e.g.,
self._PrepareTools().CropPixels()

https://codereview.chromium.org/136793022/diff/410002/tools/telemetry/telemet...
tools/telemetry/telemetry/core/bitmap.py:125: if self._crop_box:
On 2014/01/17 14:01:37, M-A Ruel wrote:
> optionally, if you want to save space, you do this:
> return self._crop_box[2] if self._crop_box else self._width

Done.

https://codereview.chromium.org/136793022/diff/410002/tools/telemetry/telemet...
tools/telemetry/telemetry/core/bitmap.py:138: # If we got a list of ints, we
need to convert it into a byte buffer.
On 2014/01/17 14:01:37, M-A Ruel wrote:
> As a docstring?

This part describes just the |pixels| conversion, but added a docstring.

Done.

https://codereview.chromium.org/136793022/diff/410002/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/bitmaptools.cc (right):

https://codereview.chromium.org/136793022/diff/410002/tools/telemetry/telemet...
tools/telemetry/telemetry/core/bitmaptools.cc:8: 
On 2014/01/17 10:43:44, bulach wrote:
> nit: could put all of these in an anonymous namespace

A bit meaningless since this is the binary's only module.

https://codereview.chromium.org/136793022/diff/410002/tools/telemetry/telemet...
tools/telemetry/telemetry/core/bitmaptools.cc:15: bool readInt(int* out) {
On 2014/01/17 10:43:44, bulach wrote:
> nit: "R"eadInt, ditto for "W"riteResponse below..

Done.

https://codereview.chromium.org/136793022/diff/410002/tools/telemetry/telemet...
tools/telemetry/telemetry/core/bitmaptools.cc:228: Bitmap bmp;
On 2014/01/17 10:43:44, bulach wrote:
> ignorable suggestion: not quite sure how frequently this would be called, but
> would it make sense to make it long-lived / stateful?
> that is, have a singleton "popen" higher up in python, and then it'd be
spawned
> just once...
> then this would need another pair of commands, "SET_BITMAP" and "QUIT", and
then
> all operations would happen on the latest bitmap, and this would be inside
> something like a "while (!quit)" loop.
> 
> no idea how much many frames we'd be processing and what the typical
operations
> are, so feel free to ignore ;)

It would be called up to hundreds of times per page measurement, so I considered
this, but I'm leaving this out for later optimization.

Take a look at https://codereview.chromium.org/141053006/ which saves the pixel
copy via Python, by piping frames directly from avconv to bitmaptools.

[commit-bot: I haz the power, 2014-01-17 23:11:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/szym@chromium.org/136793022/380010

[commit-bot: I haz the power, 2014-01-18 00:51:55]

Change committed as 245684