Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8764007:
Adding Youtube dropped frames performance measurement for different types of videos (Closed)
|
Created:
9 years ago by rohitbm Modified:
9 years ago CC:
chromium-reviews, krisr Visibility:
Public. |
DescriptionAdding Youtube dropped frames performance measurement for different types of videos
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=113036
Patch Set 1 #Patch Set 2 : '' #
Total comments: 2
Patch Set 3 : '' #Patch Set 4 : '' #Patch Set 5 : '' #
Total comments: 17
Patch Set 6 : '' #
Messages
Total messages: 15 (0 generated)
http://codereview.chromium.org/8764007/diff/2001/data/media/youtube_Avril.html File data/media/youtube_Avril.html (right): http://codereview.chromium.org/8764007/diff/2001/data/media/youtube_Avril.htm... data/media/youtube_Avril.html:1: <!-- This is a test html file for Youtube video tests --> The only difference between this and youtube.html is the video URL, right? Take that as a parameter. http://codereview.chromium.org/8764007/diff/2001/functional/youtube.py File functional/youtube.py (right): http://codereview.chromium.org/8764007/diff/2001/functional/youtube.py#newcod... functional/youtube.py:124: def PlayVideoAndAssert(self, youtube_file='youtube.html'): why not pass the youtube video's URL as argument to youtube.html? (you won't need to have multiple html files)
On 2011/12/01 00:03:14, Nirnimesh wrote: > http://codereview.chromium.org/8764007/diff/2001/data/media/youtube_Avril.html > File data/media/youtube_Avril.html (right): > > http://codereview.chromium.org/8764007/diff/2001/data/media/youtube_Avril.htm... > data/media/youtube_Avril.html:1: <!-- This is a test html file for Youtube video > tests --> > The only difference between this and youtube.html is the video URL, right? > Take that as a parameter. Done. Thanks. This is a good idea. Using youtube.html with different parameters. > > http://codereview.chromium.org/8764007/diff/2001/functional/youtube.py > File functional/youtube.py (right): > > http://codereview.chromium.org/8764007/diff/2001/functional/youtube.py#newcod... > functional/youtube.py:124: def PlayVideoAndAssert(self, > youtube_file='youtube.html'): > why not pass the youtube video's URL as argument to youtube.html? (you won't > need to have multiple html files) Done. Deleted all youtube_*.html files. Using youtube.html now.
http://codereview.chromium.org/8764007/diff/4003/data/media/youtube.html File data/media/youtube.html (right): http://codereview.chromium.org/8764007/diff/4003/data/media/youtube.html#newc... data/media/youtube.html:18: var video_id = url.split("video="); nit: we could combine the above 2 lines to get rid of the "url" variable http://codereview.chromium.org/8764007/diff/4003/functional/perf.py File functional/perf.py (right): http://codereview.chromium.org/8764007/diff/4003/functional/perf.py#newcode641 functional/perf.py:641: 'Fast':'8ETDE0VGJY4', add a space right after the ':' in each of the above 3 lines. http://codereview.chromium.org/8764007/diff/4003/functional/perf.py#newcode655 functional/perf.py:655: graph_description = 'YoutubeDroppedFrames' + '_' + video_type I recommend removing the '_' character that's being appended here. It's not necessary, and we should try to keep these descriptions as short as possible since there's a 30-character limit imposed by the autotest database. http://codereview.chromium.org/8764007/diff/4003/functional/perf.py#newcode656 functional/perf.py:656: self._PrintSummaryResults(graph_description, dropped_fps, 'frames') Note that this change gets rid of the old perf key named "frames_YoutubeDroppedFrames". So once this change lands, no more results will show up in the existing perf graph for Youtube dropped frames. Once you enable the graphs for the 3 new perf keys here, we should remove the old graph. http://codereview.chromium.org/8764007/diff/4003/functional/youtube.py File functional/youtube.py (right): http://codereview.chromium.org/8764007/diff/4003/functional/youtube.py#newcod... functional/youtube.py:127: By default test uses http://www.youtube.com/watch?v=zuzaxlddWbki . remove the space right before the '.' at the end of this line http://codereview.chromium.org/8764007/diff/4003/functional/youtube.py#newcod... functional/youtube.py:128: """ Add an "Args:" section to this docstring: Args: youtube_video: The string ID of the youtube video to play. http://codereview.chromium.org/8764007/diff/4003/functional/youtube.py#newcod... functional/youtube.py:132: 'youtube.html?video=' + youtube_video) move both parameters to the second line: url = self._pyauto.GetHttpURLForDataPath( 'media', 'youtube.html?video=' + youtube_video)
http://codereview.chromium.org/8764007/diff/4003/functional/perf.py File functional/perf.py (right): http://codereview.chromium.org/8764007/diff/4003/functional/perf.py#newcode656 functional/perf.py:656: self._PrintSummaryResults(graph_description, dropped_fps, 'frames') If we already are renaming I would like all Flash tests to have the word Flash in them. That would make it easier to find them on the perf pages and it would distinguish between html5 tests that are surely be coming one day.
http://codereview.chromium.org/8764007/diff/4003/functional/perf.py File functional/perf.py (right): http://codereview.chromium.org/8764007/diff/4003/functional/perf.py#newcode656 functional/perf.py:656: self._PrintSummaryResults(graph_description, dropped_fps, 'frames') On 2011/12/01 04:13:06, ilja wrote: > If we already are renaming I would like all Flash tests to have the word Flash > in them. That would make it easier to find them on the perf pages and it would > distinguish between html5 tests that are surely be coming one day. I'd like to create a new perf report on the autotest dashboard for only these pyauto-based perf tests, and perhaps organize the graphs by the name of the class containing the tests. For example, there's a "FlashTest" class here that contains the 3 flash tests we currently have (one of which -- the one for the ScimarkGui benchmark -- doesn't have "flash" in the test name or the perf key). Organizing the graphs by test class name would ensure that ScimarkGui is grouped with the other flash tests, even if it doesn't have "flash" in the perf key. Would that work for you? I could add "flash" to the ScimarkGui test name and perf keys, but doing so would remove 5 characters from the description of some of the perf values (as they appear in the graphs), due to the 30-character limit imposed by the autotest DB.
I agree that the 30 character limit is a problem, but grouping as suggested into its own report sounds fine to me. In that case we don't need to add "flash". One thing right now that seems problematic with the perf reports in general is that it is sometimes not easy to infer if going up or going down is good/bad. I understand there is a unit attached that helps with interpretation, but I don't know if there is any other meta information that would make this trivial.
I agree with the problem of difficulty in deciding whether a higher/lower number is good/bad. I assigned a bug to myself to better organize how the perf graphs are displayed in the dashboard: crosbug.com/23661 -Dennis On Thu, Dec 1, 2011 at 11:32 AM, <ihf@chromium.org> wrote: > I agree that the 30 character limit is a problem, but grouping as > suggested into > its own report sounds fine to me. In that case we don't need to add > "flash". > > One thing right now that seems problematic with the perf reports in > general is > that it is sometimes not easy to infer if going up or going down is > good/bad. I > understand there is a unit attached that helps with interpretation, but I > don't > know if there is any other meta information that would make this trivial. > > http://codereview.chromium.**org/8764007/<http://codereview.chromium.org/8764... >
http://codereview.chromium.org/8764007/diff/4003/data/media/youtube.html File data/media/youtube.html (right): http://codereview.chromium.org/8764007/diff/4003/data/media/youtube.html#newc... data/media/youtube.html:18: var video_id = url.split("video="); On 2011/12/01 02:44:56, dennis_jeffrey wrote: > nit: we could combine the above 2 lines to get rid of the "url" variable Done. http://codereview.chromium.org/8764007/diff/4003/functional/perf.py File functional/perf.py (right): http://codereview.chromium.org/8764007/diff/4003/functional/perf.py#newcode641 functional/perf.py:641: 'Fast':'8ETDE0VGJY4', On 2011/12/01 02:44:56, dennis_jeffrey wrote: > add a space right after the ':' in each of the above 3 lines. Done. http://codereview.chromium.org/8764007/diff/4003/functional/perf.py#newcode655 functional/perf.py:655: graph_description = 'YoutubeDroppedFrames' + '_' + video_type On 2011/12/01 02:44:56, dennis_jeffrey wrote: > I recommend removing the '_' character that's being appended here. It's not > necessary, and we should try to keep these descriptions as short as possible > since there's a 30-character limit imposed by the autotest database. Done. http://codereview.chromium.org/8764007/diff/4003/functional/perf.py#newcode656 functional/perf.py:656: self._PrintSummaryResults(graph_description, dropped_fps, 'frames') Sure, I will remove the old graphs before submitting this change. On 2011/12/01 02:44:56, dennis_jeffrey wrote: > Note that this change gets rid of the old perf key named > "frames_YoutubeDroppedFrames". So once this change lands, no more results will > show up in the existing perf graph for Youtube dropped frames. Once you enable > the graphs for the 3 new perf keys here, we should remove the old graph. http://codereview.chromium.org/8764007/diff/4003/functional/youtube.py File functional/youtube.py (right): http://codereview.chromium.org/8764007/diff/4003/functional/youtube.py#newcod... functional/youtube.py:127: By default test uses http://www.youtube.com/watch?v=zuzaxlddWbki . On 2011/12/01 02:44:56, dennis_jeffrey wrote: > remove the space right before the '.' at the end of this line Done. http://codereview.chromium.org/8764007/diff/4003/functional/youtube.py#newcod... functional/youtube.py:128: """ On 2011/12/01 02:44:56, dennis_jeffrey wrote: > Add an "Args:" section to this docstring: > > Args: > youtube_video: The string ID of the youtube video to play. Done. http://codereview.chromium.org/8764007/diff/4003/functional/youtube.py#newcod... functional/youtube.py:132: 'youtube.html?video=' + youtube_video) On 2011/12/01 02:44:56, dennis_jeffrey wrote: > move both parameters to the second line: > > url = self._pyauto.GetHttpURLForDataPath( > 'media', 'youtube.html?video=' + youtube_video) Done.
lgtm
LGTM. Thank you for adding this to the perf suite! http://codereview.chromium.org/8764007/diff/4003/functional/perf.py File functional/perf.py (right): http://codereview.chromium.org/8764007/diff/4003/functional/perf.py#newcode656 functional/perf.py:656: self._PrintSummaryResults(graph_description, dropped_fps, 'frames') I recommend waiting until we enable the new graphs, then remove the old graph after that. That way, we don't have a period of time when there are no Youtube dropped frames graphs showing up on the dashboard. On 2011/12/03 02:20:58, rohitbm wrote: > Sure, I will remove the old graphs before submitting this change. > > On 2011/12/01 02:44:56, dennis_jeffrey wrote: > > Note that this change gets rid of the old perf key named > > "frames_YoutubeDroppedFrames". So once this change lands, no more results > will > > show up in the existing perf graph for Youtube dropped frames. Once you > enable > > the graphs for the 3 new perf keys here, we should remove the old graph. >
LGTM, in case you were waiting on me.
Thanks all! I waiting for https://gerrit.chromium.org/gerrit/#change,12456 to go in. On 2011/12/05 20:36:40, Nirnimesh wrote: > LGTM, in case you were waiting on me.
Hi Rohit, Sorry if I wasn't clear in my earlier comment about waiting to submit. I meant that we should wait to remove the existing Youtube dropped frames graph, until after we enable the new graphs resulting from the current CL. So the overall sequence of events should be: 1) Submit the current CL, so that the new Youtube dropped frames tests start running. 2) Once the test starts running, create and submit the CL to enable the new graphs on the autotest dashboard 3) Once the new graphs show up on the autotest dashboard, then create and submit a CL to remove the old Youtube dropped frames graph from the autotest dashboard. -Dennis On Mon, Dec 5, 2011 at 1:01 PM, <rohitbm@chromium.org> wrote: > Thanks all! I waiting for https://gerrit.chromium.org/** > gerrit/#change,12456 <https://gerrit.chromium.org/gerrit/#change,12456>to go > in. > > > On 2011/12/05 20:36:40, Nirnimesh wrote: > >> LGTM, in case you were waiting on me. >> > > > > http://codereview.chromium.**org/8764007/<http://codereview.chromium.org/8764... >
Thanks Dennis! On 2011/12/05 21:15:24, dennisjeffrey wrote: > Hi Rohit, > > Sorry if I wasn't clear in my earlier comment about waiting to submit. I > meant that we should wait to remove the existing Youtube dropped frames > graph, until after we enable the new graphs resulting from the current CL. > So the overall sequence of events should be: > > 1) Submit the current CL, so that the new Youtube dropped frames tests > start running. > 2) Once the test starts running, create and submit the CL to enable the new > graphs on the autotest dashboard > 3) Once the new graphs show up on the autotest dashboard, then create and > submit a CL to remove the old Youtube dropped frames graph from the > autotest dashboard. > > -Dennis > > > On Mon, Dec 5, 2011 at 1:01 PM, <mailto:rohitbm@chromium.org> wrote: > > > Thanks all! I waiting for https://gerrit.chromium.org/** > > gerrit/#change,12456 <https://gerrit.chromium.org/gerrit/#change%2C12456%3Eto go > > in. > > > > > > On 2011/12/05 20:36:40, Nirnimesh wrote: > > > >> LGTM, in case you were waiting on me. > >> > > > > > > > > > http://codereview.chromium.**org/8764007/%3Chttp://codereview.chromium.org/87...> > > |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
