Make recording basic media metrics optional.

At present, we log all the media-metrics that are calculated, for all media actions. 

There are situations where this may not be ideal: for example, when we perform seek actions and want to only record seek-times.

This CL is to make the logging of basic media-metrics optional. By default, these will be logged.

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

[anandc, 2013-08-27 23:40:26]

Shadi/Dave/Tony: PTAL. Thanks.

[tonyg, 2013-08-28 01:33:49]

https://codereview.chromium.org/23523005/diff/1/tools/perf/metrics/media.py
File tools/perf/metrics/media.py (right):

https://codereview.chromium.org/23523005/diff/1/tools/perf/metrics/media.py#n...
tools/perf/metrics/media.py:28: if hasattr(page, 'skip_basic_metrics'):
Where does this come from? Does this mean we have to specify that on a per-page
basis in the page set? Can you explain a bit more about the use-case. I'm
wondering whether there is a cleaner way to factor this. Like maybe we break up
the page sets differently and set the property on a per page set basis.

[anandc, 2013-08-28 17:31:37]

Hi Tony,

PTAL at explanation and follow-up comments/question. Thanks.

https://codereview.chromium.org/23523005/diff/1/tools/perf/metrics/media.py
File tools/perf/metrics/media.py (right):

https://codereview.chromium.org/23523005/diff/1/tools/perf/metrics/media.py#n...
tools/perf/metrics/media.py:28: if hasattr(page, 'skip_basic_metrics'):
On 2013/08/28 01:33:49, tonyg wrote:
> Where does this come from? Does this mean we have to specify that on a
per-page
> basis in the page set? Can you explain a bit more about the use-case. I'm
> wondering whether there is a cleaner way to factor this. Like maybe we break
up
> the page sets differently and set the property on a per page set basis.

Yes, setting the property on a per page-set basis would be better. Is there a
good/recommended way to this? I looked at measurements/blink_perf.py and
./telemetry/page/record_wpr.py, where they obtain the page-set from the
command-line and parse out properties from it. That looks like a good approach;
let me know if there are better alternatives.

Use-case:
Let's say we have 1 media file, test.webm, for which we want to measure total
dropped frames. For that, we play (perform the play-action on) the file to
completion and get the dropped-frames count.
Another metric we might want to calculate with the same file (i.e., for the same
media-format) is time to seek. So, we perform the seek action, and measure time
to complete that act. For this scenario, we don't want to write out
dropped-frames count, because it is not what we are measuring and also it won't
be accurate to get that count for just a seek (more accurate would be to
play-to-end).

Let me know if that was not clear. The above detail has also been added to the
tracking bug, http://crbug/280350.

[anandc, 2013-08-30 00:19:34]

Hi tonyg@,

Could you PTAL at PS#2? 

Based off similar work in blink_perf.py, etc., I now have an override for
CreatePageSet, and was going to parse page-set level properties and, based on a
specified value, set a Media-measurement property to skip basic metrics.

However, in testing this, I find that CreatePageSet is not being called at all.
In comparison, running blink_perf measurement does seem to call its
CreatePageSet override. What am I missing?

[anandc, 2013-08-30 00:28:23]

On 2013/08/30 00:19:34, anandc wrote:
> Hi tonyg@,
> 
> Could you PTAL at PS#2? 
> 
> Based off similar work in blink_perf.py, etc., I now have an override for
> CreatePageSet, and was going to parse page-set level properties and, based on
a
> specified value, set a Media-measurement property to skip basic metrics.
> 
> However, in testing this, I find that CreatePageSet is not being called at
all.
> In comparison, running blink_perf measurement does seem to call its
> CreatePageSet override. What am I missing?

To be clear, the implementation of CreatePageSet in media.py doesn't really do
anything right now. It was written up just to see if I could parse out the
page-set in the measurement.
If there is a simpler way to just read the page-set that a measurement is
passed-in, please let me know.

[tonyg, 2013-08-30 03:07:52]

I'll be out of the office until Tuesday, so Dave should take over this review.

https://codereview.chromium.org/23523005/diff/6001/tools/perf/metrics/media.py
File tools/perf/metrics/media.py (right):

https://codereview.chromium.org/23523005/diff/6001/tools/perf/metrics/media.p...
tools/perf/metrics/media.py:26: def Start(self, page, tab):
Here you should be able to just access page.page_set. Test if it has the
property you are looking for, and if so use it.

[anandc, 2013-08-30 16:49:47]

Thanks a lot, Tony.

dtu@: PTAL at PS#3. Now we check at the page-set level for the
skip_basic_metrics attribute, and if it is set to true, we don't log the basic
media metrics.

https://codereview.chromium.org/23523005/diff/6001/tools/perf/metrics/media.py
File tools/perf/metrics/media.py (right):

https://codereview.chromium.org/23523005/diff/6001/tools/perf/metrics/media.p...
tools/perf/metrics/media.py:26: def Start(self, page, tab):
On 2013/08/30 03:07:52, tonyg wrote:
> Here you should be able to just access page.page_set. Test if it has the
> property you are looking for, and if so use it.

Done.

[dtu, 2013-08-30 19:38:34]

https://chromiumcodereview.appspot.com/23523005/diff/16001/tools/perf/measure...
File tools/perf/measurements/media.py (right):

https://chromiumcodereview.appspot.com/23523005/diff/16001/tools/perf/measure...
tools/perf/measurements/media.py:23: def CreatePageSet(self, args, options):
Are you still using this? Remove?

[anandc, 2013-08-30 20:18:51]

Thanks, Dave. PTAL.

https://chromiumcodereview.appspot.com/23523005/diff/16001/tools/perf/measure...
File tools/perf/measurements/media.py (right):

https://chromiumcodereview.appspot.com/23523005/diff/16001/tools/perf/measure...
tools/perf/measurements/media.py:23: def CreatePageSet(self, args, options):
On 2013/08/30 19:38:34, Dave Tu wrote:
> Are you still using this? Remove?

Thanks for the catch. Removed.

[dtu, 2013-08-30 21:08:05]

lgtm

[commit-bot: I haz the power, 2013-08-30 21:29:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/anandc@chromium.org/23523005/2

[commit-bot: I haz the power, 2013-08-30 23:04:13]

Retried try job too often on linux_chromeos for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-08-31 00:16:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/anandc@chromium.org/23523005/2

[commit-bot: I haz the power, 2013-08-31 00:16:10]

Failed to apply patch for tools/perf/metrics/media.py:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file tools/perf/metrics/media.py
  Hunk #3 FAILED at 63.
  1 out of 3 hunks FAILED -- saving rejects to file
tools/perf/metrics/media.py.rej

Patch:       tools/perf/metrics/media.py
Index: tools/perf/metrics/media.py
diff --git a/tools/perf/metrics/media.py b/tools/perf/metrics/media.py
index
282c9bd2f3d13ad8669af493efe77dccd8f96bb8..adf5ddaa9d70e884f448d55baace703bb06a46b7
100644
--- a/tools/perf/metrics/media.py
+++ b/tools/perf/metrics/media.py
@@ -6,6 +6,7 @@ import os
 
 from metrics import Metric
 
+
 class MediaMetric(Metric):
   """MediaMetric class injects and calls JS responsible for recording metrics.
 
@@ -13,15 +14,19 @@ class MediaMetric(Metric):
   such as decoded_frame_count, dropped_frame_count, decoded_video_bytes, and
   decoded_audio_bytes.
   """
+
   def __init__(self, tab):
     super(MediaMetric, self).__init__()
     with open(os.path.join(os.path.dirname(__file__), 'media.js')) as f:
       js = f.read()
       tab.ExecuteJavaScript(js)
     self._results = None
+    self._skip_basic_metrics = False
 
   def Start(self, page, tab):
     """Create the media metrics for all media elements in the document."""
+    if hasattr(page.page_set, 'skip_basic_metrics'):
+      self._skip_basic_metrics = page.page_set.skip_basic_metrics
     tab.ExecuteJavaScript('window.__createMediaMetricsForDocument()')
 
   def Stop(self, page, tab):
@@ -58,11 +63,14 @@ class MediaMetric(Metric):
     if not trace:
       logging.error('Metrics ID is missing in results.')
       return
-    AddOneResult('decoded_audio_bytes', 'bytes')
-    AddOneResult('decoded_video_bytes', 'bytes')
-    AddOneResult('decoded_frame_count', 'frames')
-    AddOneResult('dropped_frame_count', 'frames')
-    AddOneResult('playback_time', 'sec')
+
+    if not self._skip_basic_metrics:
+      AddOneResult('decoded_audio_bytes', 'bytes')
+      AddOneResult('decoded_video_bytes', 'bytes')
+      AddOneResult('decoded_frame_count', 'frames')
+      AddOneResult('dropped_frame_count', 'frames')
+      AddOneResult('time_to_play', 'sec')
+      AddOneResult('playback_time', 'sec')
+
     AddOneResult('seek', 'sec')
-    AddOneResult('time_to_play', 'sec')

[commit-bot: I haz the power, 2013-09-02 19:00:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/anandc@chromium.org/23523005/42001

[commit-bot: I haz the power, 2013-09-02 21:38:01]

Change committed as 220869