Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
93733002:
[Telemetry] Refactoring in preparation for video-based Speed Index support. (Closed)
|
Created:
7 years ago by tonyg Modified:
7 years ago CC:
chromium-reviews, chrome-speed-team+watch_google.com, telemetry+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[Telemetry] Refactoring in preparation for video-based Speed Index support.
1. Introduce a platform API for video capture.
2. Implement that API for KitKat+ releases of Android.
3. Refactor speed index metric to have Paint Rect and Video impls. Use video
when available.
BUG=323813
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=238270
Patch Set 1 #
Total comments: 13
Patch Set 2 : szym comments #Patch Set 3 : Use png_bitmap #Patch Set 4 : Move API to Tab #Patch Set 5 : Change to generators #
Total comments: 12
Patch Set 6 : Fix lint #
Total comments: 1
Messages
Total messages: 19 (0 generated)
drive-by: https://codereview.chromium.org/93733002/diff/1/tools/telemetry/telemetry/cor... File tools/telemetry/telemetry/core/platform/android_platform_backend.py (right): https://codereview.chromium.org/93733002/diff/1/tools/telemetry/telemetry/cor... tools/telemetry/telemetry/core/platform/android_platform_backend.py:201: raise NotImplementedError("mp4 video saved to %s, but Telemetry doesn't " drive-by: no idea if this is what you need, but: avconv -i foo.mp4 Foo%d.bmp would generate a gazillion huge bitmaps :) avconv is part of libffmpeg but I'm not sure we're building it for host.. sudo apt-get install libav-tools should also work.
https://codereview.chromium.org/93733002/diff/1/tools/perf/metrics/speedindex.py File tools/perf/metrics/speedindex.py (right): https://codereview.chromium.org/93733002/diff/1/tools/perf/metrics/speedindex... tools/perf/metrics/speedindex.py:131: prev_time = self.GetStartTime() What is GetStartTime for? It's probably simpler to make the assumption above: the first value in the time_completeness_dict is at the start time. https://codereview.chromium.org/93733002/diff/1/tools/perf/metrics/speedindex... tools/perf/metrics/speedindex.py:132: for time, completeness in sorted(time_completeness_dict.items()): I'd strongly suggest that GetTimeCompleteness should return a sorted list of tuples (time, completeness) rather than a dict. https://codereview.chromium.org/93733002/diff/1/tools/perf/metrics/speedindex... tools/perf/metrics/speedindex.py:152: self.tab.browser.platform.StartBrowserVideoCapture(self.tab, 4) Why 4? https://codereview.chromium.org/93733002/diff/1/tools/telemetry/telemetry/cor... File tools/telemetry/telemetry/core/platform/__init__.py (right): https://codereview.chromium.org/93733002/diff/1/tools/telemetry/telemetry/cor... tools/telemetry/telemetry/core/platform/__init__.py:146: rate. Must be in range [0.1, 100]. Should webcam capture enforce the bitrate limit as well? https://codereview.chromium.org/93733002/diff/1/tools/telemetry/telemetry/cor... File tools/telemetry/telemetry/core/platform/android_platform_backend.py (right): https://codereview.chromium.org/93733002/diff/1/tools/telemetry/telemetry/cor... tools/telemetry/telemetry/core/platform/android_platform_backend.py:201: raise NotImplementedError("mp4 video saved to %s, but Telemetry doesn't " On 2013/11/28 11:05:23, bulach wrote: > drive-by: no idea if this is what you need, but: > avconv -i foo.mp4 Foo%d.bmp would generate a gazillion huge bitmaps :) > > avconv is part of libffmpeg but I'm not sure we're building it for host.. sudo > apt-get install libav-tools should also work. Like you said, it could easily reach 1-2GB. It would be great to use ffmpeg with python bindings (like pyffmpeg), but if we want to keep telemetry dependency-free, I think piping the output of ffmpeg is going to work just fine. If we really want to keep telemetry dependency-free, then we could pipe rawvideo: def ReadFrames(inputfile): size = None proc = subprocess.Popen(['avconv', '-i', inputfile], stderr=subprocess.PIPE) for line in proc.stderr.readlines(): if 'Video:' in line: size = line.split(',')[2] size = map(int, size.split()[0].split('x')) break assert size proc.wait() frame_length = size[0] * size[1] * 3 frame_data = bytearray(frame_length) with open(os.devnull, 'w') as fnull: # Use rawvideo so that we don't need any external library to parse frames. proc = subprocess.Popen(['avconv', '-i', inputfile, '-vcodec', 'rawvideo', '-pix_fmt', 'rgb24', '-f', 'rawvideo', '-'], stderr=fnull, stdout=subprocess.PIPE) while True: num_read = proc.stdout.readinto(frame_data) if not num_read: raise StopIteration assert num_read == len(frame_data) yield frame_data, size On my laptop this runs at 110 fps. However, computing the histogram in a python loop will slow things down a lot. In experiments on my laptop, I got: raw python: 4.5 fps numpy.histogram: 9 fps PIL.Image.histogram: 75 fps I think PIL is available on all telemetry platforms.
Wonderful review. All comments addressed. https://codereview.chromium.org/93733002/diff/1/tools/perf/metrics/speedindex.py File tools/perf/metrics/speedindex.py (right): https://codereview.chromium.org/93733002/diff/1/tools/perf/metrics/speedindex... tools/perf/metrics/speedindex.py:131: prev_time = self.GetStartTime() On 2013/11/29 11:05:12, szym wrote: > What is GetStartTime for? It's probably simpler to make the assumption above: > the first value in the time_completeness_dict is at the start time. Totally. Done. https://codereview.chromium.org/93733002/diff/1/tools/perf/metrics/speedindex... tools/perf/metrics/speedindex.py:132: for time, completeness in sorted(time_completeness_dict.items()): On 2013/11/29 11:05:12, szym wrote: > I'd strongly suggest that GetTimeCompleteness should return a sorted list of > tuples (time, completeness) rather than a dict. You are right, dict is an odd choice here. Fixed. https://codereview.chromium.org/93733002/diff/1/tools/perf/metrics/speedindex... tools/perf/metrics/speedindex.py:152: self.tab.browser.platform.StartBrowserVideoCapture(self.tab, 4) On 2013/11/29 11:05:12, szym wrote: > Why 4? 4 is the screenshot capture util's default. I'm planning to experiment with overhead and set this appropriately. I added a TODO for this patch. https://codereview.chromium.org/93733002/diff/1/tools/telemetry/telemetry/cor... File tools/telemetry/telemetry/core/platform/__init__.py (right): https://codereview.chromium.org/93733002/diff/1/tools/telemetry/telemetry/cor... tools/telemetry/telemetry/core/platform/__init__.py:146: rate. Must be in range [0.1, 100]. On 2013/11/29 11:05:12, szym wrote: > Should webcam capture enforce the bitrate limit as well? To your point, max_bitrate seems like it requires implementation-specific knowledge. I've changed it to min_bitrate instead. Because that's what a user of this api requires. The platform will be responsible for achieving that bitrate or throwing an exception. That seems to work better for webcam, right? https://codereview.chromium.org/93733002/diff/1/tools/telemetry/telemetry/cor... tools/telemetry/telemetry/core/platform/__init__.py:151: def StopBrowserVideoCapture(self): I changed this to return keyframes instead of all frames. If avconv can efficiently ignore duplicate frames, it should make the memory and speed of this API much more tenable. https://codereview.chromium.org/93733002/diff/1/tools/telemetry/telemetry/cor... File tools/telemetry/telemetry/core/platform/android_platform_backend.py (right): https://codereview.chromium.org/93733002/diff/1/tools/telemetry/telemetry/cor... tools/telemetry/telemetry/core/platform/android_platform_backend.py:201: raise NotImplementedError("mp4 video saved to %s, but Telemetry doesn't " > I think PIL is available on all telemetry platforms. We accidentally checked in a PIL dep on the bots recently and it failed. That's not to say we couldn't get it installed. Can we pull it into third_party?
> def ReadFrames(inputfile):
> size = None
> proc = subprocess.Popen(['avconv', '-i', inputfile],
> stderr=subprocess.PIPE)
> for line in proc.stderr.readlines():
> if 'Video:' in line:
> size = line.split(',')[2]
> size = map(int, size.split()[0].split('x'))
> break
> assert size
> proc.wait()
>
> frame_length = size[0] * size[1] * 3
> frame_data = bytearray(frame_length)
>
> with open(os.devnull, 'w') as fnull:
> # Use rawvideo so that we don't need any external library to parse frames.
> proc = subprocess.Popen(['avconv', '-i', inputfile, '-vcodec', 'rawvideo',
> '-pix_fmt', 'rgb24', '-f', 'rawvideo', '-'],
> stderr=fnull,
> stdout=subprocess.PIPE)
> while True:
> num_read = proc.stdout.readinto(frame_data)
> if not num_read:
> raise StopIteration
> assert num_read == len(frame_data)
> yield frame_data, size
I noticed something desirable about this algorithm. It only produces keyframes
for KitKat videos by default. My sample 2.5s page load video only had 16 frames.
So
performance might actually be acceptable. The catch is that we have to modify
this algorithm to get the timestamps of each frame.
Webcam should be able to apply some noise filter to output only key frames as
well.
All changes to speed index LGTM :-) It'll be neat when the video speed index is implemented.
Cool, thanks for the review. I'll wait for szym to approve too and then land this structure so that we can fill in the TODOs in subsequent patches.
drive-by: https://codereview.chromium.org/93733002/diff/1/tools/telemetry/telemetry/cor... File tools/telemetry/telemetry/core/platform/android_platform_backend.py (right): https://codereview.chromium.org/93733002/diff/1/tools/telemetry/telemetry/cor... tools/telemetry/telemetry/core/platform/android_platform_backend.py:201: raise NotImplementedError("mp4 video saved to %s, but Telemetry doesn't " On 2013/11/29 20:42:07, tonyg wrote: > > I think PIL is available on all telemetry platforms. > > We accidentally checked in a PIL dep on the bots recently and it failed. That's > not to say we couldn't get it installed. Can we pull it into third_party? I'm not volunteering myself but just throwing some ideas.. :) ffmpeg is already in third_party.. I suppose we could write a small host tool in c++ for all platforms that would do the keyframe decoding *and* calculate the histograms from raw frames directly in c++ as well? I'm not at all familiar with ffmpeg though, so have no idea how easy is to rip "avconv" and hook up in an histogram calculator... since ideas are cheap :) here's another one: "imagemagick" package has a histogram calculator, not sure if this is what we're looking for here. $ convert chrome/test/data/dromaeo/images/dino7.png -define histogram:unique-colors=true -format %c histogram:info:- ... 102: ( 8, 5, 3,110) #0805036E rgba(8,5,3,0.431373) 19: ( 8, 5, 3, 66) #08050342 rgba(8,5,3,0.258824) 17: ( 8, 5, 3, 88) #08050358 rgba(8,5,3,0.345098) 10: ( 8, 5, 3, 44) #0805032C rgba(8,5,3,0.172549) 8: ( 8, 5, 3, 22) #08050316 rgba(8,5,3,0.0862745) 8: ( 8, 5, 3, 0) #08050300 rgba(8,5,3,0) 2: ( 8, 5, 3,104) #08050368 rgba(8,5,3,0.407843) 2: ( 8, 5, 3,105) #08050369 rgba(8,5,3,0.411765) 2: ( 8, 5, 3, 97) #08050361 rgba(8,5,3,0.380392) 2: ( 8, 5, 3, 87) #08050357 rgba(8,5,3,0.341176) ... it looks like imagemagick also work on all platforms and its apache-style licensed... so perhaps we could pipe from avconv into imagemagick? szym, would you be able to try that loop?
[+pmeenan] Oops, meant to do that originally.
On 2013/11/29 22:14:50, tonyg wrote: > > yield frame_data, size > > I noticed something desirable about this algorithm. It only produces keyframes > for KitKat videos by default. My sample 2.5s page load video only had 16 frames. > So > performance might actually be acceptable. The catch is that we have to modify > this algorithm to get the timestamps of each frame. > > Webcam should be able to apply some noise filter to output only key frames as > well. Ah, I forgot about timestamps. I'm afraid detecting whether two frames are only noise apart is no faster than computing a histogram.
On 2013/12/02 19:21:19, szym wrote: > On 2013/11/29 22:14:50, tonyg wrote: > > > yield frame_data, size > > > > I noticed something desirable about this algorithm. It only produces keyframes > > for KitKat videos by default. My sample 2.5s page load video only had 16 > frames. > > So > > performance might actually be acceptable. The catch is that we have to modify > > this algorithm to get the timestamps of each frame. > > > > Webcam should be able to apply some noise filter to output only key frames as > > well. > > Ah, I forgot about timestamps. I'm afraid detecting whether two frames are only > noise apart is no faster than computing a histogram. That should be fine too, right? It will just mean that the webcam will have more keyframes than other implementations.
https://codereview.chromium.org/93733002/diff/80001/tools/perf/metrics/speedi... File tools/perf/metrics/speedindex.py (right): https://codereview.chromium.org/93733002/diff/80001/tools/perf/metrics/speedi... tools/perf/metrics/speedindex.py:38: PaintRectSpeedIndexImpl(tab)) Given that the two speed indexes have such different implementations, it seems like the type of speed index should be indicated where the results are presented. This line makes it look like they are treated as interchangeable. https://codereview.chromium.org/93733002/diff/80001/tools/perf/metrics/speedi... tools/perf/metrics/speedindex.py:141: def __init__(self, tab): Document "tab"? https://codereview.chromium.org/93733002/diff/80001/tools/perf/metrics/speedi... tools/perf/metrics/speedindex.py:252: y1 = min(height, y1) Better or worse? clipped_width = max(0, min(width, x1) - max(0, x0)) clipped_height = max(0, min(height, y1) - max(0, y0)) return clipped_width * clipped_height It avoids x0, x1, y0, y1 having two different meanings (original rectangle coord and partially clipped rectangle coord). https://codereview.chromium.org/93733002/diff/80001/tools/perf/metrics/speedi... tools/perf/metrics/speedindex.py:296: return (frame,) + GetBox(paint_event.args['data']['clip']) Perhaps one of the following: (frame, GetBox(...)) frame = ... box = ... return (frame, box) https://codereview.chromium.org/93733002/diff/80001/tools/telemetry/telemetry... File tools/telemetry/telemetry/core/platform/platform_backend.py (right): https://codereview.chromium.org/93733002/diff/80001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/platform/platform_backend.py:85: def StopVideoCapture(self): Should this have a doc string? BTW, it surprised me that StopVideoCapture returns the frames. The name made me think it would only stop it (instead of collecting the frames and returning them).
Thanks, slamm for the comments! szym, do you think this is good enough to land so that we can start filling in the implementations? https://codereview.chromium.org/93733002/diff/80001/tools/perf/metrics/speedi... File tools/perf/metrics/speedindex.py (right): https://codereview.chromium.org/93733002/diff/80001/tools/perf/metrics/speedi... tools/perf/metrics/speedindex.py:38: PaintRectSpeedIndexImpl(tab)) On 2013/12/02 22:51:15, slamm wrote: > Given that the two speed indexes have such different implementations, it seems > like the type of speed index should be indicated where the results are > presented. > > This line makes it look like they are treated as interchangeable. I see your point. But I think the Measurements themselves just want SpeedIndex captured in the most accurate way possible. I'd like to set the page cyclers on the bots to always capture it. Then the implementation just chooses the best method available. We know that video always beats paint rect, and I'd like to implement video in more places. https://codereview.chromium.org/93733002/diff/80001/tools/perf/metrics/speedi... tools/perf/metrics/speedindex.py:141: def __init__(self, tab): On 2013/12/02 22:51:15, slamm wrote: > Document "tab"? Done (in the base class). https://codereview.chromium.org/93733002/diff/80001/tools/perf/metrics/speedi... tools/perf/metrics/speedindex.py:252: y1 = min(height, y1) On 2013/12/02 22:51:15, slamm wrote: > Better or worse? > > clipped_width = max(0, min(width, x1) - max(0, x0)) > clipped_height = max(0, min(height, y1) - max(0, y0)) > return clipped_width * clipped_height > > It avoids x0, x1, y0, y1 having two different meanings (original rectangle coord > and partially clipped rectangle coord). I like your implementation. Changed here. https://codereview.chromium.org/93733002/diff/80001/tools/perf/metrics/speedi... tools/perf/metrics/speedindex.py:296: return (frame,) + GetBox(paint_event.args['data']['clip']) On 2013/12/02 22:51:15, slamm wrote: > Perhaps one of the following: > > (frame, GetBox(...)) > > frame = ... > box = ... > return (frame, box) GetBox returns a tuple, so I don't think those implementations are equivalent. https://codereview.chromium.org/93733002/diff/80001/tools/telemetry/telemetry... File tools/telemetry/telemetry/core/platform/platform_backend.py (right): https://codereview.chromium.org/93733002/diff/80001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/platform/platform_backend.py:85: def StopVideoCapture(self): On 2013/12/02 22:51:15, slamm wrote: > Should this have a doc string? The docs all live in platform/__init__.py > BTW, it surprised me that StopVideoCapture returns the frames. The name made me > think it would only stop it (instead of collecting the frames and returning > them). Yeah, I think you are right about this, but we currently use this paradigm of Stop*() returning the results all over. My vote would be to stick with consistency for now and clean up in the future.
Lgtm On Dec 2, 2013 3:18 PM, <tonyg@chromium.org> wrote: > Thanks, slamm for the comments! > > szym, do you think this is good enough to land so that we can start > filling in > the implementations? > > > https://codereview.chromium.org/93733002/diff/80001/tools/ > perf/metrics/speedindex.py > File tools/perf/metrics/speedindex.py (right): > > https://codereview.chromium.org/93733002/diff/80001/tools/ > perf/metrics/speedindex.py#newcode38 > tools/perf/metrics/speedindex.py:38: PaintRectSpeedIndexImpl(tab)) > On 2013/12/02 22:51:15, slamm wrote: > >> Given that the two speed indexes have such different implementations, >> > it seems > >> like the type of speed index should be indicated where the results are >> presented. >> > > This line makes it look like they are treated as interchangeable. >> > > I see your point. But I think the Measurements themselves just want > SpeedIndex captured in the most accurate way possible. I'd like to set > the page cyclers on the bots to always capture it. > > Then the implementation just chooses the best method available. We know > that video always beats paint rect, and I'd like to implement video in > more places. > > https://codereview.chromium.org/93733002/diff/80001/tools/ > perf/metrics/speedindex.py#newcode141 > tools/perf/metrics/speedindex.py:141: def __init__(self, tab): > On 2013/12/02 22:51:15, slamm wrote: > >> Document "tab"? >> > > Done (in the base class). > > https://codereview.chromium.org/93733002/diff/80001/tools/ > perf/metrics/speedindex.py#newcode252 > tools/perf/metrics/speedindex.py:252: y1 = min(height, y1) > On 2013/12/02 22:51:15, slamm wrote: > >> Better or worse? >> > > clipped_width = max(0, min(width, x1) - max(0, x0)) >> clipped_height = max(0, min(height, y1) - max(0, y0)) >> return clipped_width * clipped_height >> > > It avoids x0, x1, y0, y1 having two different meanings (original >> > rectangle coord > >> and partially clipped rectangle coord). >> > > I like your implementation. Changed here. > > https://codereview.chromium.org/93733002/diff/80001/tools/ > perf/metrics/speedindex.py#newcode296 > tools/perf/metrics/speedindex.py:296: return (frame,) + > GetBox(paint_event.args['data']['clip']) > On 2013/12/02 22:51:15, slamm wrote: > >> Perhaps one of the following: >> > > (frame, GetBox(...)) >> > > frame = ... >> box = ... >> return (frame, box) >> > > GetBox returns a tuple, so I don't think those implementations are > equivalent. > > https://codereview.chromium.org/93733002/diff/80001/tools/ > telemetry/telemetry/core/platform/platform_backend.py > File tools/telemetry/telemetry/core/platform/platform_backend.py > (right): > > https://codereview.chromium.org/93733002/diff/80001/tools/ > telemetry/telemetry/core/platform/platform_backend.py#newcode85 > tools/telemetry/telemetry/core/platform/platform_backend.py:85: def > StopVideoCapture(self): > On 2013/12/02 22:51:15, slamm wrote: > >> Should this have a doc string? >> > > The docs all live in platform/__init__.py > > BTW, it surprised me that StopVideoCapture returns the frames. The >> > name made me > >> think it would only stop it (instead of collecting the frames and >> > returning > >> them). >> > > Yeah, I think you are right about this, but we currently use this > paradigm of Stop*() returning the results all over. My vote would be to > stick with consistency for now and clean up in the future. > > https://codereview.chromium.org/93733002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tonyg@chromium.org/93733002/80001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tonyg@chromium.org/93733002/100001
https://codereview.chromium.org/93733002/diff/80001/tools/perf/metrics/speedi... File tools/perf/metrics/speedindex.py (right): https://codereview.chromium.org/93733002/diff/80001/tools/perf/metrics/speedi... tools/perf/metrics/speedindex.py:38: PaintRectSpeedIndexImpl(tab)) On 2013/12/02 23:18:36, tonyg wrote: > On 2013/12/02 22:51:15, slamm wrote: > > Given that the two speed indexes have such different implementations, it seems > > like the type of speed index should be indicated where the results are > > presented. > > > > This line makes it look like they are treated as interchangeable. > > I see your point. But I think the Measurements themselves just want SpeedIndex > captured in the most accurate way possible. I'd like to set the page cyclers on > the bots to always capture it. > > Then the implementation just chooses the best method available. We know that > video always beats paint rect, and I'd like to implement video in more places. Thanks for the context. https://codereview.chromium.org/93733002/diff/80001/tools/perf/metrics/speedi... tools/perf/metrics/speedindex.py:296: return (frame,) + GetBox(paint_event.args['data']['clip']) On 2013/12/02 23:18:36, tonyg wrote: > On 2013/12/02 22:51:15, slamm wrote: > > Perhaps one of the following: > > > > (frame, GetBox(...)) > > > > frame = ... > > box = ... > > return (frame, box) > > GetBox returns a tuple, so I don't think those implementations are equivalent. Ah, of course. It's right there. https://codereview.chromium.org/93733002/diff/100001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/tab.py (right): https://codereview.chromium.org/93733002/diff/100001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/tab.py:109: return t yield t or return (t for t in self.browser.platform.StopVideoCapture()) or, if it is already a generator, return self.browser.platform.StopVideoCapture()
Message was sent while issue was closed.
Change committed as 238270 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
