Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
16854013:
Telemetry media_measurement plus play action and tests. (Closed)
|
Created:
7 years, 6 months ago by shadi Modified:
7 years, 5 months ago Reviewers:
nduca, dtu-google (do not use), phoglund_chromium, yihongg1, anandc, scherkus (not reviewing) CC:
chromium-reviews, chrome-speed-team+watch_google.com, telemetry+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionTelemetry media_measurement plus play action and tests.
The media_measurement reports common HTML5 media stats for every <video> and <audio> element on the page by default.
The play action provides options like selecting certain media elements to play, and 'wait_for_playing' and 'wait_for_ended' to wait for media events that will automatically append 'ttp' (time_to_play) and 'playback_time' metrics as they happen.
BUG=249435
NOTRY=true
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=211720
Patch Set 1 #
Total comments: 31
Patch Set 2 : #Patch Set 3 : removed un-needed file #
Total comments: 19
Patch Set 4 : Split measurement JS from action JS. Wrap JS access in Py class. Add more unit tests. #
Total comments: 43
Patch Set 5 : Create media base and subclass and other nits. #Patch Set 6 : Add expected metrics to python. clean up some code. #Patch Set 7 : remove unneeded deps check in play.py #Patch Set 8 : remove media files (submitted separately) #Messages
Total messages: 22 (0 generated)
Telemetry and video stack guys, PTAL. Note: The tools/perf/page_sets/video_stack/* files serve as a test scenario for media_measurements. It is not the full video stack page_sets. I added them to lay the base files to help edit them later.
FYI I'm not a JS/Python pro https://codereview.chromium.org/16854013/diff/1/tools/perf/page_sets/video_st... File tools/perf/page_sets/video_stack.json (right): https://codereview.chromium.org/16854013/diff/1/tools/perf/page_sets/video_st... tools/perf/page_sets/video_stack.json:4: { "url": "http://shawarma.kir/alcatraz/eme_fps.html" } I'm assuming this has to point to a real site at some point? https://codereview.chromium.org/16854013/diff/1/tools/perf/page_sets/video_st... File tools/perf/page_sets/video_stack/video.html (right): https://codereview.chromium.org/16854013/diff/1/tools/perf/page_sets/video_st... tools/perf/page_sets/video_stack/video.html:1: <html> add <!DOCTYPE html> to top of page https://codereview.chromium.org/16854013/diff/1/tools/perf/page_sets/video_st... tools/perf/page_sets/video_stack/video.html:8: <script type="text/javascript"> this can go in the <body> as well https://codereview.chromium.org/16854013/diff/1/tools/perf/page_sets/video_st... tools/perf/page_sets/video_stack/video.html:11: if(mediaFile) space between if and ( https://codereview.chromium.org/16854013/diff/1/tools/perf/page_sets/video_st... File tools/perf/page_sets/video_stack/video_stack.json (right): https://codereview.chromium.org/16854013/diff/1/tools/perf/page_sets/video_st... tools/perf/page_sets/video_stack/video_stack.json:8: "selector": "video[id=\"video1\"]", AFAIK if this a CSS selector then "#video1" should also work https://codereview.chromium.org/16854013/diff/1/tools/perf/perf_tools/media_m... File tools/perf/perf_tools/media_measurement.py (right): https://codereview.chromium.org/16854013/diff/1/tools/perf/perf_tools/media_m... tools/perf/perf_tools/media_measurement.py:61: # Media actions could have already loaded the JS file. is it possible to always have actions load the JS? I'm curious whether we can trim down the # of possible ways to load JS to one https://codereview.chromium.org/16854013/diff/1/tools/perf/perf_tools/media_m... tools/perf/perf_tools/media_measurement.py:77: if isinstance(metric, (list, tuple)): what's an example of a metric that has no units? do we generate any unitless metrics? would it be better to require JS to always provide units (even if they're no_unit) so we don't need special casing behaviour here? https://codereview.chromium.org/16854013/diff/1/tools/perf/perf_tools/media_m... tools/perf/perf_tools/media_measurement.py:82: return (metric, 'no_unit') is no_unit a special unit from telemetry's perspective? https://codereview.chromium.org/16854013/diff/1/tools/perf/perf_tools/media_m... File tools/perf/perf_tools/media_metrics.js (right): https://codereview.chromium.org/16854013/diff/1/tools/perf/perf_tools/media_m... tools/perf/perf_tools/media_metrics.js:16: this.isHTML5 = element instanceof HTMLMediaElement; when would we create a MediaRecorder for a non-media element? if we could guarantee that would never be the case (or, for example, throw an exception here) then we could eliminate a the isHTML5 branches https://codereview.chromium.org/16854013/diff/1/tools/perf/perf_tools/media_m... tools/perf/perf_tools/media_metrics.js:39: // For the cases where autoplay=true, and without a 'play' action, we want are we going to have tests w/ autoplay or should we design the tests to have MediaRecorder always control when playback starts? https://codereview.chromium.org/16854013/diff/1/tools/telemetry/telemetry/pag... File tools/telemetry/telemetry/page/actions/play.py (right): https://codereview.chromium.org/16854013/diff/1/tools/telemetry/telemetry/pag... tools/telemetry/telemetry/page/actions/play.py:29: if not deps_found: extra space after not https://codereview.chromium.org/16854013/diff/1/tools/telemetry/telemetry/pag... tools/telemetry/telemetry/page/actions/play.py:52: """Halts play action untill the selector's event is fired.""" s/untill/until/ https://codereview.chromium.org/16854013/diff/1/tools/telemetry/unittest_data... File tools/telemetry/unittest_data/video_test.html (right): https://codereview.chromium.org/16854013/diff/1/tools/telemetry/unittest_data... tools/telemetry/unittest_data/video_test.html:1: <html> <!DOCTYPE html> https://codereview.chromium.org/16854013/diff/1/tools/telemetry/unittest_data... tools/telemetry/unittest_data/video_test.html:3: <video id="video_1" src="bear.webm" controls></video> nit: in your other html pages you don't use a "_" for the IDs my $0.02 would be to try and be consistent w/ naming https://codereview.chromium.org/16854013/diff/1/tools/telemetry/unittest_data... tools/telemetry/unittest_data/video_test.html:6: <script type="text/javascript"> script can go in body https://codereview.chromium.org/16854013/diff/1/tools/telemetry/unittest_data... tools/telemetry/unittest_data/video_test.html:7: var videos = document.getElementsByTagName('video'); what is the point of this statement? is some other script/test interacting with this variable?
Thanks scherkus@. telemetry guys, can you PTAL? https://codereview.chromium.org/16854013/diff/1/tools/perf/page_sets/video_st... File tools/perf/page_sets/video_stack.json (right): https://codereview.chromium.org/16854013/diff/1/tools/perf/page_sets/video_st... tools/perf/page_sets/video_stack.json:4: { "url": "http://shawarma.kir/alcatraz/eme_fps.html" } On 2013/06/14 20:50:45, scherkus wrote: > I'm assuming this has to point to a real site at some point? Yes. But not for video stack at the time being. This file should not be part of the CL, we will use .json file in video_stack/ folder instead. https://codereview.chromium.org/16854013/diff/1/tools/perf/page_sets/video_st... File tools/perf/page_sets/video_stack/video.html (right): https://codereview.chromium.org/16854013/diff/1/tools/perf/page_sets/video_st... tools/perf/page_sets/video_stack/video.html:8: <script type="text/javascript"> On 2013/06/14 20:50:45, scherkus wrote: > this can go in the <body> as well Done. https://codereview.chromium.org/16854013/diff/1/tools/perf/page_sets/video_st... tools/perf/page_sets/video_stack/video.html:11: if(mediaFile) On 2013/06/14 20:50:45, scherkus wrote: > space between if and ( Done. https://codereview.chromium.org/16854013/diff/1/tools/perf/page_sets/video_st... File tools/perf/page_sets/video_stack/video_stack.json (right): https://codereview.chromium.org/16854013/diff/1/tools/perf/page_sets/video_st... tools/perf/page_sets/video_stack/video_stack.json:8: "selector": "video[id=\"video1\"]", On 2013/06/14 20:50:45, scherkus wrote: > AFAIK if this a CSS selector then "#video1" should also work Done. https://codereview.chromium.org/16854013/diff/1/tools/perf/perf_tools/media_m... File tools/perf/perf_tools/media_measurement.py (right): https://codereview.chromium.org/16854013/diff/1/tools/perf/perf_tools/media_m... tools/perf/perf_tools/media_measurement.py:61: # Media actions could have already loaded the JS file. On 2013/06/14 20:50:45, scherkus wrote: > is it possible to always have actions load the JS? > > I'm curious whether we can trim down the # of possible ways to load JS to one The problem is that we might run a media measurement without any actions, and we might run media action outside media measurement tests. So, we need both to load the JS when needed. https://codereview.chromium.org/16854013/diff/1/tools/perf/perf_tools/media_m... tools/perf/perf_tools/media_measurement.py:77: if isinstance(metric, (list, tuple)): On 2013/06/14 20:50:45, scherkus wrote: > what's an example of a metric that has no units? do we generate any unitless > metrics? > > would it be better to require JS to always provide units (even if they're > no_unit) so we don't need special casing behaviour here? We don't generate unitless metrics. We can enforce that rule anyway. https://codereview.chromium.org/16854013/diff/1/tools/perf/perf_tools/media_m... tools/perf/perf_tools/media_measurement.py:82: return (metric, 'no_unit') On 2013/06/14 20:50:45, scherkus wrote: > is no_unit a special unit from telemetry's perspective? No it is not. https://codereview.chromium.org/16854013/diff/1/tools/perf/perf_tools/media_m... File tools/perf/perf_tools/media_metrics.js (right): https://codereview.chromium.org/16854013/diff/1/tools/perf/perf_tools/media_m... tools/perf/perf_tools/media_metrics.js:16: this.isHTML5 = element instanceof HTMLMediaElement; On 2013/06/14 20:50:45, scherkus wrote: > when would we create a MediaRecorder for a non-media element? > > if we could guarantee that would never be the case (or, for example, throw an > exception here) then we could eliminate a the isHTML5 branches The plan is that we will add support for non HTML5 players like youtube flash or other ones as well. So, I wanted to prepare/test the code for that. https://codereview.chromium.org/16854013/diff/1/tools/perf/perf_tools/media_m... tools/perf/perf_tools/media_metrics.js:39: // For the cases where autoplay=true, and without a 'play' action, we want On 2013/06/14 20:50:45, scherkus wrote: > are we going to have tests w/ autoplay or should we design the tests to have > MediaRecorder always control when playback starts? Videostack tests won't have autoplay, but media measurements should be generic to support metrics by default for other teams/web pages, etc. https://codereview.chromium.org/16854013/diff/1/tools/telemetry/telemetry/pag... File tools/telemetry/telemetry/page/actions/play.py (right): https://codereview.chromium.org/16854013/diff/1/tools/telemetry/telemetry/pag... tools/telemetry/telemetry/page/actions/play.py:29: if not deps_found: On 2013/06/14 20:50:45, scherkus wrote: > extra space after not Done. Nice catch, gpylint did not catch it. https://codereview.chromium.org/16854013/diff/1/tools/telemetry/telemetry/pag... tools/telemetry/telemetry/page/actions/play.py:52: """Halts play action untill the selector's event is fired.""" On 2013/06/14 20:50:45, scherkus wrote: > s/untill/until/ Done. https://codereview.chromium.org/16854013/diff/1/tools/telemetry/unittest_data... File tools/telemetry/unittest_data/video_test.html (right): https://codereview.chromium.org/16854013/diff/1/tools/telemetry/unittest_data... tools/telemetry/unittest_data/video_test.html:1: <html> On 2013/06/14 20:50:45, scherkus wrote: > <!DOCTYPE html> Done. https://codereview.chromium.org/16854013/diff/1/tools/telemetry/unittest_data... tools/telemetry/unittest_data/video_test.html:3: <video id="video_1" src="bear.webm" controls></video> On 2013/06/14 20:50:45, scherkus wrote: > nit: in your other html pages you don't use a "_" for the IDs > > my $0.02 would be to try and be consistent w/ naming Done. https://codereview.chromium.org/16854013/diff/1/tools/telemetry/unittest_data... tools/telemetry/unittest_data/video_test.html:6: <script type="text/javascript"> On 2013/06/14 20:50:45, scherkus wrote: > script can go in body Done. https://codereview.chromium.org/16854013/diff/1/tools/telemetry/unittest_data... tools/telemetry/unittest_data/video_test.html:7: var videos = document.getElementsByTagName('video'); On 2013/06/14 20:50:45, scherkus wrote: > what is the point of this statement? is some other script/test interacting with > this variable? Not really. I added it when I was thinking of other unit tests I was going to add.
shadi, this looks really solid. awesomely done. some initial feedback. https://codereview.chromium.org/16854013/diff/15001/tools/perf/page_sets/vide... File tools/perf/page_sets/video_stack/utils.js (left): https://codereview.chromium.org/16854013/diff/15001/tools/perf/page_sets/vide... tools/perf/page_sets/video_stack/utils.js:3: // found in the LICENSE file. this file seems unused? or rather a lot of the stuff in it seems unused... help me understand? https://codereview.chromium.org/16854013/diff/15001/tools/perf/page_sets/vide... File tools/perf/page_sets/video_stack/video.html (right): https://codereview.chromium.org/16854013/diff/15001/tools/perf/page_sets/vide... tools/perf/page_sets/video_stack/video.html:2: <html> why not just make a few html files that do the right thing and put a few different pageset entries at the different htmls? try to stick to minimal html unless you truly need it... https://codereview.chromium.org/16854013/diff/15001/tools/perf/page_sets/vide... File tools/perf/page_sets/video_stack/video_stack.json (right): https://codereview.chromium.org/16854013/diff/15001/tools/perf/page_sets/vide... tools/perf/page_sets/video_stack/video_stack.json:1: { i think current custom is to call this file and folder tough_video_cases, and put the json parallel to the directory. Copy tough scrolling cases. https://codereview.chromium.org/16854013/diff/15001/tools/perf/perf_tools/med... File tools/perf/perf_tools/media_measurement.py (right): https://codereview.chromium.org/16854013/diff/15001/tools/perf/perf_tools/med... tools/perf/perf_tools/media_measurement.py:46: media_metrics = tab.EvaluateJavaScript('window.__getAllMetrics()') so you should make a MediaMetrics class that seals this injection. Ideally just call it media_metrics.py... https://codereview.chromium.org/16854013/diff/15001/tools/perf/perf_tools/med... tools/perf/perf_tools/media_measurement.py:50: def AddResult(self, media_metric, results): looks like this doesn't have to be a class method... move it to be a global function instead? https://codereview.chromium.org/16854013/diff/15001/tools/perf/perf_tools/med... tools/perf/perf_tools/media_measurement.py:75: self.LoadDepsJS(tab) how bout making this a start/stop pattern and do the inject it in DidNavigateToPage(...). https://codereview.chromium.org/16854013/diff/15001/tools/telemetry/telemetry... File tools/telemetry/telemetry/page/actions/play.py (right): https://codereview.chromium.org/16854013/diff/15001/tools/telemetry/telemetry... tools/telemetry/telemetry/page/actions/play.py:30: with open( you should have play.js here --- you cant share code between these two js files im afraid... its just different layers of the cake, sorry!
Thanks nduca@ I am working on updating the CL but meanwhile I want to discuss some comments inlined. Thanks! https://codereview.chromium.org/16854013/diff/15001/tools/perf/page_sets/vide... File tools/perf/page_sets/video_stack/video.html (right): https://codereview.chromium.org/16854013/diff/15001/tools/perf/page_sets/vide... tools/perf/page_sets/video_stack/video.html:2: <html> On 2013/06/17 23:01:33, nduca wrote: > why not just make a few html files that do the right thing and put a few > different pageset entries at the different htmls? try to stick to minimal html > unless you truly need it... This was just for demonstration/testing purposes. Future CL's will include useful pagesets needed for video stack perf. https://codereview.chromium.org/16854013/diff/15001/tools/perf/page_sets/vide... File tools/perf/page_sets/video_stack/video_stack.json (right): https://codereview.chromium.org/16854013/diff/15001/tools/perf/page_sets/vide... tools/perf/page_sets/video_stack/video_stack.json:1: { On 2013/06/17 23:01:33, nduca wrote: > i think current custom is to call this file and folder tough_video_cases, and > put the json parallel to the directory. Copy tough scrolling cases. Why did you guys go with tough_*_cases naming? Page sets don't have to be "tough". Is it a strict naming convention? Why "tough"? I like the folder & .json separation though. https://codereview.chromium.org/16854013/diff/15001/tools/perf/perf_tools/med... File tools/perf/perf_tools/media_measurement.py (right): https://codereview.chromium.org/16854013/diff/15001/tools/perf/perf_tools/med... tools/perf/perf_tools/media_measurement.py:75: self.LoadDepsJS(tab) On 2013/06/17 23:01:33, nduca wrote: > how bout making this a start/stop pattern and do the inject it in > DidNavigateToPage(...). I don't see how a start/stop pattern would work for our use cases. The current implementation seems to work fine for the time being, if we see that necessary we can update it later. https://codereview.chromium.org/16854013/diff/15001/tools/telemetry/telemetry... File tools/telemetry/telemetry/page/actions/play.py (right): https://codereview.chromium.org/16854013/diff/15001/tools/telemetry/telemetry... tools/telemetry/telemetry/page/actions/play.py:30: with open( On 2013/06/17 23:01:33, nduca wrote: > you should have play.js here --- you cant share code between these two js files > im afraid... its just different layers of the cake, sorry! But it is the same code that we want to run. What is wrong with using it this way? We can not inject the same code twice...The measurement class needs it in case we are not running any actions, and actions need it in case they are run outside the media_measurement scope.
> Why did you guys go with tough_*_cases naming? Page sets don't have to be > "tough". Is it a strict naming convention? Why "tough"? I like the folder & > .json separation though. Its just what we do now. If you dont like it, you're welcome to put up a bulk rename to something better. I'd like to keep to the convention though, if thats okay with you. > I don't see how a start/stop pattern would work for our use cases. The current > implementation seems to work fine for the time being, if we see that necessary > we can update it later. I think the MeasurePage function should not make injection calls. That should be in the DidNavigate, at a minimum. > But it is the same code that we want to run. What is wrong with using it this > way? Thats how it is. Telemetry doesn't use code from perf, and actions and measurements dont use code from each other. > We can not inject the same code twice... The measurement class needs it in case > we are not running any actions, and actions need it in case they are run outside > the media_measurement scope. Try to factor out the code that does the playing action from the code that does the measuring action. Are you saying these can't be separated? If so, why?
> Why did you guys go with tough_*_cases naming? Page sets don't have to be >> "tough". Is it a strict naming convention? Why "tough"? I like the folder >> & >> .json separation though. >> > > Its just what we do now. If you dont like it, you're welcome to put up a > bulk > rename to something better. I'd like to keep to the convention though, if > thats > okay with you. Okay. A bulk rename is out of the scope of this CL anyway. > > I don't see how a start/stop pattern would work for our use cases. The >> > current > >> implementation seems to work fine for the time being, if we see that >> necessary >> we can update it later. >> > I think the MeasurePage function should not make injection calls. That > should be > in the DidNavigate, at a minimum. Fair enough. > > But it is the same code that we want to run. What is wrong with using it >> this >> way? >> > Thats how it is. Telemetry doesn't use code from perf, and actions and > measurements dont use code from each other. > > We can not inject the same code twice... The measurement class needs it in >> > case > >> we are not running any actions, and actions need it in case they are run >> > outside > >> the media_measurement scope. >> > > Try to factor out the code that does the playing action from the code that > does > the measuring action. Are you saying these can't be separated? If so, why? > The complication arises because the action is aware about what/when it can record metrics. In specific, the "play" action measures both time_to_play and playback_time. So, following the [begin|end]MeasurementHook model won't work. So, letting the action record metrics and following your model or separating actions from measurements would yield two copies of actions, play action that records metrics and play action that does not. And if we let the action do all the recording of metrics, then media_measurement with no actions will report nothing. This is why I need to inject the JS in the measurement class to record metrics not related to media actions. I am still figuring out what is the nicest way to separate the JS code into two without duplication and added complexity, but having one common file for both would be awesome! > https://codereview.chromium.**org/16854013/<https://codereview.chromium.org/1... >
If you want to chat on im/etc i'm fine with it. Maybe your play action can in
addition to playing the thing, put a flag on the element that played, e.g.
play(el) { ..., el.played=true; }. Then in the measurement action, you can
queryselectorall ('video').filter(x=>x.played) and then report metrics on those?
just some comments https://codereview.chromium.org/16854013/diff/15001/tools/perf/perf_tools/med... File tools/perf/perf_tools/media_metrics.js (right): https://codereview.chromium.org/16854013/diff/15001/tools/perf/perf_tools/med... tools/perf/perf_tools/media_metrics.js:193: var recorders = findMediaRecorders(selector); this should throw an exception if none was found? https://codereview.chromium.org/16854013/diff/15001/tools/perf/perf_tools/med... tools/perf/perf_tools/media_metrics.js:195: recorders[i].play(); var e = document.createEvent('Event'); e.initEvent('willPlay', false, false); elements[i].dispatchEvent(e); https://codereview.chromium.org/16854013/diff/15001/tools/perf/perf_tools/med... tools/perf/perf_tools/media_metrics.js:203: if (!recorders[i].hasEventCompleted(event_name)) hopefully this we can avoid this too :)
I just realized, we also need to make sure that the video gets played during wpr_record so that its cached in the wpr... I'm pretty sure thats possible, and possibly even automatic, but we need to do the work to verify that its done and if not, figure out why it isn't. :)
Thanks for the comments, online and offline. I believe I addressed all the issues. PTAL. I will test wpr and comment if there are any problems. https://codereview.chromium.org/16854013/diff/15001/tools/perf/page_sets/vide... File tools/perf/page_sets/video_stack/utils.js (left): https://codereview.chromium.org/16854013/diff/15001/tools/perf/page_sets/vide... tools/perf/page_sets/video_stack/utils.js:3: // found in the LICENSE file. On 2013/06/17 23:01:33, nduca wrote: > this file seems unused? or rather a lot of the stuff in it seems unused... help > me understand? It is used by the page sets html files. It serves as a URL query function. https://codereview.chromium.org/16854013/diff/15001/tools/perf/perf_tools/med... File tools/perf/perf_tools/media_measurement.py (right): https://codereview.chromium.org/16854013/diff/15001/tools/perf/perf_tools/med... tools/perf/perf_tools/media_measurement.py:46: media_metrics = tab.EvaluateJavaScript('window.__getAllMetrics()') On 2013/06/17 23:01:33, nduca wrote: > so you should make a MediaMetrics class that seals this injection. Ideally just > call it media_metrics.py... Done. https://codereview.chromium.org/16854013/diff/15001/tools/perf/perf_tools/med... tools/perf/perf_tools/media_measurement.py:50: def AddResult(self, media_metric, results): On 2013/06/17 23:01:33, nduca wrote: > looks like this doesn't have to be a class method... move it to be a global > function instead? Done. https://codereview.chromium.org/16854013/diff/15001/tools/perf/perf_tools/med... File tools/perf/perf_tools/media_metrics.js (right): https://codereview.chromium.org/16854013/diff/15001/tools/perf/perf_tools/med... tools/perf/perf_tools/media_metrics.js:195: recorders[i].play(); On 2013/06/19 18:12:03, nduca wrote: > var e = document.createEvent('Event'); > e.initEvent('willPlay', false, false); > elements[i].dispatchEvent(e); Done. https://codereview.chromium.org/16854013/diff/15001/tools/perf/perf_tools/med... tools/perf/perf_tools/media_metrics.js:203: if (!recorders[i].hasEventCompleted(event_name)) On 2013/06/19 18:12:03, nduca wrote: > hopefully this we can avoid this too :) Done.
Ping. nduca@ can you PTAL?
Ping. Can you PTAL?
getting really close, sorry one more round of feedback. my apologies about the delay https://codereview.chromium.org/16854013/diff/37001/tools/perf/page_sets/toug... File tools/perf/page_sets/tough_video_cases/utils.js (left): https://codereview.chromium.org/16854013/diff/37001/tools/perf/page_sets/toug... tools/perf/page_sets/tough_video_cases/utils.js:9: var QueryString = function () { this is a very strange way to set a global variable. Why self evaluate like this and not just function getQueryString()? Classes are the only things that should begin with capitals in js btw (google js style) https://codereview.chromium.org/16854013/diff/37001/tools/perf/page_sets/toug... tools/perf/page_sets/tough_video_cases/utils.js:62: return src + ch + 't=' + (new Date()).getTime(); this will get turned into something deterministic by web page replay. are you aware of this? https://codereview.chromium.org/16854013/diff/37001/tools/perf/page_sets/toug... File tools/perf/page_sets/tough_video_cases/video.html (right): https://codereview.chromium.org/16854013/diff/37001/tools/perf/page_sets/toug... tools/perf/page_sets/tough_video_cases/video.html:9: var mediaFile = QueryString.mediaFile; i'd rather you have bear_webm.html rather than something that listens to query strigs. I believe I mentioned this previously. Otherwise, the user has to understand 3 different files and how they interact just to understand that this file plays bear.webm https://codereview.chromium.org/16854013/diff/37001/tools/perf/perf_tools/med... File tools/perf/perf_tools/media_metrics.js (right): https://codereview.chromium.org/16854013/diff/37001/tools/perf/perf_tools/med... tools/perf/perf_tools/media_metrics.js:11: function MediaMetric(element) { should a MediaMetricBase base class, a HTML5MediaMetric subclass? Then you could have a faux constructor: function MediaMetric(element) { if (element instanceof HTMLMediaElement) return new HTMLMediaMetric(); throw new Error('Unrecognized element type'); } https://codereview.chromium.org/16854013/diff/37001/tools/perf/perf_tools/med... tools/perf/perf_tools/media_metrics.js:15: this.isHTML5 = element instanceof HTMLMediaElement; this could be a getter on MediaMetric. https://codereview.chromium.org/16854013/diff/37001/tools/perf/perf_tools/med... tools/perf/perf_tools/media_metrics.js:19: this.initHTML5(); what happens if you accidentally bind a media metric twice to an element? You should at least explode if this happens https://codereview.chromium.org/16854013/diff/37001/tools/perf/perf_tools/med... tools/perf/perf_tools/media_metrics.js:20: } should you explode if this isnt' html5? https://codereview.chromium.org/16854013/diff/37001/tools/perf/perf_tools/med... tools/perf/perf_tools/media_metrics.js:23: this.ttp_timer = new Timer(); name this fully https://codereview.chromium.org/16854013/diff/37001/tools/perf/perf_tools/med... tools/perf/perf_tools/media_metrics.js:90: function Timer() { why does this not call start()? and why not just have timer do nothing, and have the person creating the timer call start after you create it? https://codereview.chromium.org/16854013/diff/37001/tools/perf/perf_tools/med... tools/perf/perf_tools/media_metrics.js:108: function LoadMediaMetrics() { This isn't really loading. createMediaMetricsForDocument may be appropriate. https://codereview.chromium.org/16854013/diff/37001/tools/perf/perf_tools/med... tools/perf/perf_tools/media_metrics.js:111: var media = document.querySelectorAll('video, audio'); mediaElements https://codereview.chromium.org/16854013/diff/37001/tools/perf/perf_tools/med... tools/perf/perf_tools/media_metrics.js:112: for (var i = 0; i < media.length; i++) { one liners dont get {} https://codereview.chromium.org/16854013/diff/37001/tools/perf/perf_tools/med... tools/perf/perf_tools/media_metrics.js:113: mediaMetrics.push(new MediaMetric(media[i])); explicitly say window.__mediaMetrics perhaps instead of binding to a local var https://codereview.chromium.org/16854013/diff/37001/tools/perf/perf_tools/med... tools/perf/perf_tools/media_metrics.js:125: return Date().getTime(); Date.now()? does't create garbage... https://codereview.chromium.org/16854013/diff/37001/tools/perf/perf_tools/med... tools/perf/perf_tools/media_metrics.js:131: for (var i = 0; i < mediaMetrics.length; i++) { one line fors get no bracez https://codereview.chromium.org/16854013/diff/37001/tools/perf/perf_tools/med... tools/perf/perf_tools/media_metrics.js:141: LoadMediaMetrics(); you should keep injection of the script separate from when this function is called. Inject the script. Then create the metrics explicitly from python. https://codereview.chromium.org/16854013/diff/37001/tools/perf/perf_tools/med... File tools/perf/perf_tools/media_metrics.py (right): https://codereview.chromium.org/16854013/diff/37001/tools/perf/perf_tools/med... tools/perf/perf_tools/media_metrics.py:18: # Measurement class and media actions class can both load media_metrics.js. is this comment still true? https://codereview.chromium.org/16854013/diff/37001/tools/perf/perf_tools/med... tools/perf/perf_tools/media_metrics.py:26: you should have a Start/Stop/Add in style of memory metrics, per the comments before about the distinction between injection vs recording https://codereview.chromium.org/16854013/diff/37001/tools/perf/perf_tools/med... tools/perf/perf_tools/media_metrics.py:27: def ReportCollectedMetrics(self, results): your naming shoudl conform to memory_metrics.py https://codereview.chromium.org/16854013/diff/37001/tools/perf/perf_tools/med... tools/perf/perf_tools/media_metrics.py:54: for metric in metrics: i'd love this to be a bit more strongly typed. Where you explictly add the results one by one that you have, rather than just blindly converting the stuff from js to the right hting. It may be more typing but its going to be a LOT easier to read. https://codereview.chromium.org/16854013/diff/37001/tools/perf/perf_tools/med... tools/perf/perf_tools/media_metrics.py:65: if isinstance(metric, (list, tuple)): per note above, rather this wasn't here and you hand typed out a lot of addResult lines https://codereview.chromium.org/16854013/diff/37001/tools/perf/perf_tools/med... tools/perf/perf_tools/media_metrics.py:73: def GetTraceName(info): per note above, rather this wasn't here and you hand typed out a lot of addResult lines
Thanks for review. PTAL with questions inlined. https://codereview.chromium.org/16854013/diff/37001/tools/perf/page_sets/toug... File tools/perf/page_sets/tough_video_cases/utils.js (left): https://codereview.chromium.org/16854013/diff/37001/tools/perf/page_sets/toug... tools/perf/page_sets/tough_video_cases/utils.js:9: var QueryString = function () { On 2013/07/09 23:07:19, nduca wrote: > this is a very strange way to set a global variable. Why self evaluate like this > and not just function getQueryString()? > > Classes are the only things that should begin with capitals in js btw (google js > style) Not a big deal. It made sense to self evaluate when you pass multiple query params (which is not the case yet). https://codereview.chromium.org/16854013/diff/37001/tools/perf/page_sets/toug... tools/perf/page_sets/tough_video_cases/utils.js:62: return src + ch + 't=' + (new Date()).getTime(); On 2013/07/09 23:07:19, nduca wrote: > this will get turned into something deterministic by web page replay. are you > aware of this? This was an old version. The code is deleted in newest version. https://codereview.chromium.org/16854013/diff/37001/tools/perf/page_sets/toug... File tools/perf/page_sets/tough_video_cases/video.html (right): https://codereview.chromium.org/16854013/diff/37001/tools/perf/page_sets/toug... tools/perf/page_sets/tough_video_cases/video.html:9: var mediaFile = QueryString.mediaFile; On 2013/07/09 23:07:19, nduca wrote: > i'd rather you have bear_webm.html rather than something that listens to query > strigs. I believe I mentioned this previously. Otherwise, the user has to > understand 3 different files and how they interact just to understand that this > file plays bear.webm As I mentioned in previous comments, this file is not the official video stack test cases. It serves more as an example as you can see it contains three video tags. However, I do anticipate we want to pass parameters through URL. We want to have a simple page that plays a video or audio file. Considering all codecs and types, it made sense to pass the media file by param instead of having 10 html files. I would rather have this discussion when landing video stack page sets CL later. https://codereview.chromium.org/16854013/diff/37001/tools/perf/perf_tools/med... File tools/perf/perf_tools/media_metrics.js (right): https://codereview.chromium.org/16854013/diff/37001/tools/perf/perf_tools/med... tools/perf/perf_tools/media_metrics.js:11: function MediaMetric(element) { On 2013/07/09 23:07:19, nduca wrote: > should a MediaMetricBase base class, a HTML5MediaMetric subclass? Then you > could have a faux constructor: > function MediaMetric(element) { > if (element instanceof HTMLMediaElement) > return new HTMLMediaMetric(); > throw new Error('Unrecognized element type'); > } PTAL. I kept all fields common to any media metric in base class. This can be updated once we get more than one subclass. https://codereview.chromium.org/16854013/diff/37001/tools/perf/perf_tools/med... tools/perf/perf_tools/media_metrics.js:15: this.isHTML5 = element instanceof HTMLMediaElement; On 2013/07/09 23:07:19, nduca wrote: > this could be a getter on MediaMetric. This is avoided with subclasses. https://codereview.chromium.org/16854013/diff/37001/tools/perf/perf_tools/med... tools/perf/perf_tools/media_metrics.js:19: this.initHTML5(); On 2013/07/09 23:07:19, nduca wrote: > what happens if you accidentally bind a media metric twice to an element? You > should at least explode if this happens Done. https://codereview.chromium.org/16854013/diff/37001/tools/perf/perf_tools/med... tools/perf/perf_tools/media_metrics.js:20: } On 2013/07/09 23:07:19, nduca wrote: > should you explode if this isnt' html5? not really. If there are no html5 elements in the page, then nothing should happen. https://codereview.chromium.org/16854013/diff/37001/tools/perf/perf_tools/med... tools/perf/perf_tools/media_metrics.js:23: this.ttp_timer = new Timer(); On 2013/07/09 23:07:19, nduca wrote: > name this fully Done. https://codereview.chromium.org/16854013/diff/37001/tools/perf/perf_tools/med... tools/perf/perf_tools/media_metrics.js:90: function Timer() { On 2013/07/09 23:07:19, nduca wrote: > why does this not call start()? > > and why not just have timer do nothing, and have the person creating the timer > call start after you create it? It is usually the case where you need to start the timer just after creating it, which makes start() redundant unless you want to reset the same timer. https://codereview.chromium.org/16854013/diff/37001/tools/perf/perf_tools/med... tools/perf/perf_tools/media_metrics.js:108: function LoadMediaMetrics() { On 2013/07/09 23:07:19, nduca wrote: > This isn't really loading. createMediaMetricsForDocument may be appropriate. Done. https://codereview.chromium.org/16854013/diff/37001/tools/perf/perf_tools/med... tools/perf/perf_tools/media_metrics.js:111: var media = document.querySelectorAll('video, audio'); On 2013/07/09 23:07:19, nduca wrote: > mediaElements Done. https://codereview.chromium.org/16854013/diff/37001/tools/perf/perf_tools/med... tools/perf/perf_tools/media_metrics.js:112: for (var i = 0; i < media.length; i++) { On 2013/07/09 23:07:19, nduca wrote: > one liners dont get {} Done. https://codereview.chromium.org/16854013/diff/37001/tools/perf/perf_tools/med... tools/perf/perf_tools/media_metrics.js:113: mediaMetrics.push(new MediaMetric(media[i])); On 2013/07/09 23:07:19, nduca wrote: > explicitly say window.__mediaMetrics perhaps instead of binding to a local var Done. https://codereview.chromium.org/16854013/diff/37001/tools/perf/perf_tools/med... tools/perf/perf_tools/media_metrics.js:125: return Date().getTime(); On 2013/07/09 23:07:19, nduca wrote: > Date.now()? does't create garbage... Done. https://codereview.chromium.org/16854013/diff/37001/tools/perf/perf_tools/med... tools/perf/perf_tools/media_metrics.js:131: for (var i = 0; i < mediaMetrics.length; i++) { On 2013/07/09 23:07:19, nduca wrote: > one line fors get no bracez Done. https://codereview.chromium.org/16854013/diff/37001/tools/perf/perf_tools/med... tools/perf/perf_tools/media_metrics.js:141: LoadMediaMetrics(); On 2013/07/09 23:07:19, nduca wrote: > you should keep injection of the script separate from when this function is > called. Inject the script. Then create the metrics explicitly from python. Is there a good reason for this? Is there a case where we want to load this JS file and not call the function? The sooner we load the metrics the better. Doing it from python just adds delay with no obvious benefits IMO. https://codereview.chromium.org/16854013/diff/37001/tools/perf/perf_tools/med... File tools/perf/perf_tools/media_metrics.py (right): https://codereview.chromium.org/16854013/diff/37001/tools/perf/perf_tools/med... tools/perf/perf_tools/media_metrics.py:18: # Measurement class and media actions class can both load media_metrics.js. On 2013/07/09 23:07:19, nduca wrote: > is this comment still true? Oops, removed. https://codereview.chromium.org/16854013/diff/37001/tools/perf/perf_tools/med... tools/perf/perf_tools/media_metrics.py:27: def ReportCollectedMetrics(self, results): On 2013/07/09 23:07:19, nduca wrote: > your naming shoudl conform to memory_metrics.py Done. https://codereview.chromium.org/16854013/diff/37001/tools/perf/perf_tools/med... tools/perf/perf_tools/media_metrics.py:54: for metric in metrics: On 2013/07/09 23:07:19, nduca wrote: > i'd love this to be a bit more strongly typed. Where you explictly add the > results one by one that you have, rather than just blindly converting the stuff > from js to the right hting. It may be more typing but its going to be a LOT > easier to read. The idea is that we don't know how many metrics have been recorded at this time. That's why I look through all metrics recorded and report them. Are you suggesting keeping a hard coded list of all possible metrics that can be collected here and searching for each if recorded or not? Then whenever we want to add a new metric, we need to update both the media_metrics.js and media_metrics.py vs now you can simply add a new metric in the media_metrics.js following the same metric format 'name'" [value, unit]. https://codereview.chromium.org/16854013/diff/37001/tools/perf/perf_tools/med... tools/perf/perf_tools/media_metrics.py:65: if isinstance(metric, (list, tuple)): On 2013/07/09 23:07:19, nduca wrote: > per note above, rather this wasn't here and you hand typed out a lot of > addResult lines See earlier comment. https://codereview.chromium.org/16854013/diff/37001/tools/perf/perf_tools/med... tools/perf/perf_tools/media_metrics.py:73: def GetTraceName(info): On 2013/07/09 23:07:19, nduca wrote: > per note above, rather this wasn't here and you hand typed out a lot of > addResult lines See earlier comment.
> > file plays bear.webm > types, it made sense to pass the media file by param instead of having 10 html > files. > > I would rather have this discussion when landing video stack page sets CL later. stat with 1 html file and no companion js. when you get to the point of having 10, then thats the point to add the complexity, not now. simple first. > It is usually the case where you need to start the timer just after creating it, > which makes start() redundant unless you want to reset the same timer. just a suggestion. > Is there a good reason for this? Is there a case where we want to load this JS > file and not call the function? > > The sooner we load the metrics the better. Doing it from python just adds delay > with no obvious benefits IMO. Delay is not an issue. You're talking microsecodns. The approach I proposed is cleaner and easier to read, without any odd side effects. > The idea is that we don't know how many metrics have been recorded at this time. > That's why I look through all metrics recorded and report them. > > Are you suggesting keeping a hard coded list of all possible metrics that can be > collected here and searching for each if recorded or not? Then whenever we want > to add a new metric, we need to update both the media_metrics.js and > media_metrics.py vs now you can simply add a new metric in the media_metrics.js > following the same metric format 'name'" [value, unit]. Yes thats what Im suggesting. Can you please give it a try and see what it looks like? It really didn't look all that hard, and it'll be really clean and easy to read. Right now, only you can understand what metrics are output by the test without inspection. There are other reasons we need this --- soon, telemetry is going to require that you list the metrics out longhand before a benchmark runs. Thats for waterfall integration issues. Basically, please dont be clever, not here at least.
PTAL
You're gonna hate me for saying this but ... lgtm Nicely done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/shadi@chromium.org/16854013/66001
Can't process patch for file tools/telemetry/unittest_data/bear.webm. Binary file support is temporarilly disabled due to a bug. Please commit blindly the binary files first then commit the source change as a separate CL. Sorry for the annoyance.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/shadi@chromium.org/16854013/81001
Message was sent while issue was closed.
Change committed as 211720 |
