[Telemetry] Convert mp4 video to Timestamp+Bitmaps.

[Telemetry] Convert mp4 video to Timestamp+Bitmaps.

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

[bulach, 2013-12-04 19:38:46]

lgtm, just nits and small suggestions.

are the videos and screenshots ok? would dev.chromium.org or some such be less
controversial? I wasn't in the discussions re: pagesets, so I have no idea....

it'd be pretty awesome and self-referential to have a video of this very
codereview... ;)

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

https://codereview.chromium.org/103723002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/bitmap.py:43: assert stride in [3, 4], 'Invalid
stride'
nit: hmm, I normally see "stride" as "word aligned width".. shouldn't this be
something like "bpp"?

https://codereview.chromium.org/103723002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/bitmap.py:70: base = ((y * self._width) + x) *
self._stride
nit: as above, I think it's more common as (y * self._stride) + x

https://codereview.chromium.org/103723002/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/platform/linux_platform_backend.py (right):

https://codereview.chromium.org/103723002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/linux_platform_backend.py:90:
os.environ["PATH"] += os.pathsep + telemetry_bin_dir
nit: s/"/'/

https://codereview.chromium.org/103723002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/linux_platform_backend.py:98:
sys.exit(1)
there's some dupe here and 68-77, perhaps extract into a 
def _InstallBinary?

[tonyg, 2013-12-04 20:54:25]

Thanks, all comments addressed. Hitting the CQ.

Szymon, if you have any comments, I'll un-cq if it isn't too late or else
address in a follow-up patch.

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

https://codereview.chromium.org/103723002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/bitmap.py:43: assert stride in [3, 4], 'Invalid
stride'
On 2013/12/04 19:38:46, bulach wrote:
> nit: hmm, I normally see "stride" as "word aligned width".. shouldn't this be
> something like "bpp"?

Done.

https://codereview.chromium.org/103723002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/bitmap.py:70: base = ((y * self._width) + x) *
self._stride
On 2013/12/04 19:38:46, bulach wrote:
> nit: as above, I think it's more common as (y * self._stride) + x

That doesn't work. I did something similar. Tell me if there is a better way.

You can tell I don't work on graphics :-P

https://codereview.chromium.org/103723002/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/platform/linux_platform_backend.py (right):

https://codereview.chromium.org/103723002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/linux_platform_backend.py:90:
os.environ["PATH"] += os.pathsep + telemetry_bin_dir
On 2013/12/04 19:38:46, bulach wrote:
> nit: s/"/'/

Done.

https://codereview.chromium.org/103723002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/linux_platform_backend.py:98:
sys.exit(1)
On 2013/12/04 19:38:46, bulach wrote:
> there's some dupe here and 68-77, perhaps extract into a 
> def _InstallBinary?

I'm planning a larger refactor of how we get and install support binaries. There
is a lot of inconsistency now throughout Telemetry. I'll just fix everything in
that CL if that is okay.

[commit-bot: I haz the power, 2013-12-04 20:54:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tonyg@chromium.org/103723002/20001

[bulach, 2013-12-04 21:07:07]

still lgtm, just pedantic clarification :)

https://codereview.chromium.org/103723002/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/bitmap.py (right):

https://codereview.chromium.org/103723002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/bitmap.py:69: stride = self._bpp * self._width
yeah, I don't work on graphics either and I have no idea about the "pixels"
layout at this level, but :)

this would normally be dword (sometimes word) aligned... we're probably lucky
that width is playing nicely :) but to be pedantic:
stride = ((self._bpp * self._width) + 31) / 32

https://codereview.chromium.org/103723002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/bitmap.py:72: return
RgbaColor(self._pixels[base+0], self._pixels[base+1],
nit: space around +

[szym, 2013-12-04 21:59:38]

lgtm, but a suggestion for improvement

https://codereview.chromium.org/103723002/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/android_platform_backend.py
(right):

https://codereview.chromium.org/103723002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/android_platform_backend.py:223: def
GetFrameTimestampMs(stderr):
Needs comment on what this tries to parse. For example:

""" Looks for frame timestamp int the log. The expected format is:
pts=%f$
"""

https://codereview.chromium.org/103723002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/android_platform_backend.py:231: return
int(1000 * float(line.split('=')[-1]))
This seems overly complicated. I suggest:

for line in stderr.xreadlines():
  if 'pts=' in line:
     ...
  

Perhaps even use re:

def GetFrameTimestampMs(log, pattern=re.compile('(?<=pts=).*$')):
  for line in log.xreadlines():
    match = pattern.search(line)
    if match:
      return int(1000 * float(match.group(0)))

[szym, 2013-12-04 22:30:26]

https://codereview.chromium.org/103723002/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/bitmap.py (right):

https://codereview.chromium.org/103723002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/bitmap.py:69: stride = self._bpp * self._width
On 2013/12/04 21:07:08, bulach wrote:
> yeah, I don't work on graphics either and I have no idea about the "pixels"
> layout at this level, but :)
> 
> this would normally be dword (sometimes word) aligned... we're probably lucky
> that width is playing nicely :) but to be pedantic:
> stride = ((self._bpp * self._width) + 31) / 32

No, this is not aligned at all. This layout is for serialization not for memory.

I checked that with avconv, pix_fmt=rgb24 and geometry=421x517 (stride=1263),
the stride really is odd.

[szym, 2013-12-04 22:33:40]

https://codereview.chromium.org/103723002/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/bitmap.py (right):

https://codereview.chromium.org/103723002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/bitmap.py:69: stride = self._bpp * self._width
On 2013/12/04 22:30:26, szym wrote:
> On 2013/12/04 21:07:08, bulach wrote:
> > yeah, I don't work on graphics either and I have no idea about the "pixels"
> > layout at this level, but :)
> > 
> > this would normally be dword (sometimes word) aligned... we're probably
lucky
> > that width is playing nicely :) but to be pedantic:
> > stride = ((self._bpp * self._width) + 31) / 32
> 
> No, this is not aligned at all. This layout is for serialization not for
memory.
> 
> I checked that with avconv, pix_fmt=rgb24 and geometry=421x517 (stride=1263),
> the stride really is odd.

Looking at png.py I see similarly no alignment:
# Values per row
vpr = planes * width

[tonyg, 2013-12-04 23:01:41]

https://codereview.chromium.org/103723002/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/bitmap.py (right):

https://codereview.chromium.org/103723002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/bitmap.py:69: stride = self._bpp * self._width
I got rid of the stride variable to avoid confusion.

https://codereview.chromium.org/103723002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/bitmap.py:72: return
RgbaColor(self._pixels[base+0], self._pixels[base+1],
On 2013/12/04 21:07:08, bulach wrote:
> nit: space around +

Done.

https://codereview.chromium.org/103723002/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/android_platform_backend.py
(right):

https://codereview.chromium.org/103723002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/android_platform_backend.py:223: def
GetFrameTimestampMs(stderr):
On 2013/12/04 21:59:38, szym wrote:
> Needs comment on what this tries to parse. For example:
> 
> """ Looks for frame timestamp int the log. The expected format is:
> pts=%f$
> """

Done.

https://codereview.chromium.org/103723002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/android_platform_backend.py:231: return
int(1000 * float(line.split('=')[-1]))
On 2013/12/04 21:59:38, szym wrote:
> This seems overly complicated. I suggest:
> 
> for line in stderr.xreadlines():
>   if 'pts=' in line:
>      ...
>   
> 
> Perhaps even use re:
> 
> def GetFrameTimestampMs(log, pattern=re.compile('(?<=pts=).*$')):
>   for line in log.xreadlines():
>     match = pattern.search(line)
>     if match:
>       return int(1000 * float(match.group(0)))
> 

That doesn't work. I added a comment explaining.

[commit-bot: I haz the power, 2013-12-04 23:01:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tonyg@chromium.org/103723002/40001

[szym, 2013-12-05 01:27:04]

https://codereview.chromium.org/103723002/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/android_platform_backend.py
(right):

https://codereview.chromium.org/103723002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/android_platform_backend.py:231: return
int(1000 * float(line.split('=')[-1]))
On 2013/12/04 23:01:41, tonyg wrote:
> On 2013/12/04 21:59:38, szym wrote:
> > This seems overly complicated. I suggest:
> > 
> > for line in stderr.xreadlines():
> >   if 'pts=' in line:
> >      ...
> >   
> > 
> > Perhaps even use re:
> > 
> > def GetFrameTimestampMs(log, pattern=re.compile('(?<=pts=).*$')):
> >   for line in log.xreadlines():
> >     match = pattern.search(line)
> >     if match:
> >       return int(1000 * float(match.group(0)))
> > 
> 
> That doesn't work. I added a comment explaining.

Ok, xreadlines might be doing some internal buffering. However,

while True:
  line = stderr.readline()

works for me.

[tonyg, 2013-12-05 01:32:39]

Thanks, I have to run, but I'll fix this up.

BTW, I'll work on getting PIL into Telemetry. For the purposes of your
work, just assume it will exist.

On Wed, Dec 4, 2013 at 5:27 PM,  <szym@chromium.org> wrote:
>
>
https://codereview.chromium.org/103723002/diff/20001/tools/telemetry/telemetr...
> File tools/telemetry/telemetry/core/platform/android_platform_backend.py
> (right):
>
>
https://codereview.chromium.org/103723002/diff/20001/tools/telemetry/telemetr...
> tools/telemetry/telemetry/core/platform/android_platform_backend.py:231:
> return int(1000 * float(line.split('=')[-1]))
> On 2013/12/04 23:01:41, tonyg wrote:
>>
>> On 2013/12/04 21:59:38, szym wrote:
>> > This seems overly complicated. I suggest:
>> >
>> > for line in stderr.xreadlines():
>> >   if 'pts=' in line:
>> >      ...
>> >
>> >
>> > Perhaps even use re:
>> >
>> > def GetFrameTimestampMs(log, pattern=re.compile('(?<=pts=).*$')):
>> >   for line in log.xreadlines():
>> >     match = pattern.search(line)
>> >     if match:
>> >       return int(1000 * float(match.group(0)))
>> >
>
>
>> That doesn't work. I added a comment explaining.
>
>
> Ok, xreadlines might be doing some internal buffering. However,
>
> while True:
>   line = stderr.readline()
>
> works for me.
>
> https://codereview.chromium.org/103723002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2013-12-05 03:37:06]

Change committed as 238880