Port TBMv2-based media benchmark to Android

Port TBMv2-based media benchmark to Android

This is a continuation of porting media benchmarks to use TBMv2.
It allows using TBMv2 to collect power (using BattOr) and CPU time
metrics on Android devices, similar to what's already available
on desktop.

BUG=700160
Review-Url: https://codereview.chromium.org/2775883003
Cr-Commit-Position: refs/heads/master@{#460781}
Committed: https://chromium.googlesource.com/chromium/src/+/7cda4ca0bfbdb63159254a5b274833f5b091a9a0

[johnchen, 2017-03-24 21:34:43]

Description was changed from

==========
Port TBMv2-based media benchmark to Android

This is a continuation of porting media benchmarks to use TBMv2.
It allows using TBMv2 to collect power (using BattOr) and CPU time
metrics on Android devices, similar to what's already available
on desktop.

BUG=700160
==========

to

==========
Port TBMv2-based media benchmark to Android

This is a continuation of porting media benchmarks to use TBMv2.
It allows using TBMv2 to collect power (using BattOr) and CPU time
metrics on Android devices, similar to what's already available
on desktop.

BUG=700160
==========

[johnchen, 2017-03-24 21:34:43]

johnchen@chromium.org changed reviewers:
+ benjhayden@chromium.org, charliea@chromium.org, crouleau@chromium.org,
nednguyen@google.com, watk@chromium.org, zhenw@chromium.org

[johnchen, 2017-03-24 21:38:06]

PTAL

This is a continuation of https://codereview.chromium.org/2743773002/, to port
media benchmarks to use TBMv2. It allows using TBMv2 to collect power (using
BattOr) and CPU time metrics on Android devices, similar to what's already
available on desktop.

[CalebRouleau, 2017-03-27 16:24:45]

LGTM

https://codereview.chromium.org/2775883003/diff/20001/tools/perf/benchmarks/m...
File tools/perf/benchmarks/media.py (right):

https://codereview.chromium.org/2775883003/diff/20001/tools/perf/benchmarks/m...
tools/perf/benchmarks/media.py:155: # By default, Chrome on Android disallows
auto play of media.
To make this more clear:

"By default, Chrome on Android does not allow autoplay of media: it requires a
user gesture event to start a video. The following option works around that."

[nednguyen, 2017-03-27 16:35:52]

On 2017/03/27 16:24:45, crouleau wrote:
> LGTM
> 
>
https://codereview.chromium.org/2775883003/diff/20001/tools/perf/benchmarks/m...
> File tools/perf/benchmarks/media.py (right):
> 
>
https://codereview.chromium.org/2775883003/diff/20001/tools/perf/benchmarks/m...
> tools/perf/benchmarks/media.py:155: # By default, Chrome on Android disallows
> auto play of media.
> To make this more clear:
> 
> "By default, Chrome on Android does not allow autoplay of media: it requires a
> user gesture event to start a video. The following option works around that."

Our bots are currently running out of capacity (crbug.com/694589), so we cannot
add more benchmarks without removing some existing ones. You either need to land
another CL to remove some of the old benchmarks or do it in this CL.

[CalebRouleau, 2017-03-27 16:48:17]

On 2017/03/27 16:35:52, nednguyen wrote:
> On 2017/03/27 16:24:45, crouleau wrote:
> > LGTM
> > 
> >
>
https://codereview.chromium.org/2775883003/diff/20001/tools/perf/benchmarks/m...
> > File tools/perf/benchmarks/media.py (right):
> > 
> >
>
https://codereview.chromium.org/2775883003/diff/20001/tools/perf/benchmarks/m...
> > tools/perf/benchmarks/media.py:155: # By default, Chrome on Android
disallows
> > auto play of media.
> > To make this more clear:
> > 
> > "By default, Chrome on Android does not allow autoplay of media: it requires
a
> > user gesture event to start a video. The following option works around
that."
> 
> Our bots are currently running out of capacity (crbug.com/694589), so we
cannot
> add more benchmarks without removing some existing ones. You either need to
land
> another CL to remove some of the old benchmarks or do it in this CL.

Per https://bugs.chromium.org/p/chromium/issues/detail?id=703098#c12, we have no
other automated method to measure power on Android to help our devs make
tradeoffs, so this is P1 to get this working. This new benchmark is meant to
replace the old media.android.tough_video_cases benchmark, but we aren't
comfortable removing that one because we don't have this new TBMv2 one fully
functional yet.

What if we remove media.media_cns_cases in this CL? Would that mean that we
could move forward here?

[charliea (OOO until 10-5), 2017-03-27 17:16:22]

lgtm w/ comment, but defer to Ned for final review

https://codereview.chromium.org/2775883003/diff/20001/tools/perf/benchmarks/m...
File tools/perf/benchmarks/media.py (right):

https://codereview.chromium.org/2775883003/diff/20001/tools/perf/benchmarks/m...
tools/perf/benchmarks/media.py:141: # Exclude is_4k and 50 fps media files
(garden* & crowd*).
nit: I'm not sure how useful this comment is. The "options" reads pretty much
like English, and it seems like specifying the exact media files that are being
excluded only sets us up for this comment to get out of date in the future.
(same w/ same comment above)

https://codereview.chromium.org/2775883003/diff/20001/tools/perf/benchmarks/m...
tools/perf/benchmarks/media.py:146: if possible_browser.platform.GetOSName() !=
"android":
You can get rid of this lineby putting:

@benchmark.Enabled('android') above
@benchmark.Disabled('l', 'android-webview')

Enabling this test on Android that way is more idiomatic for Telemetry tests

you can think of whether the benchmark runs as a sort of logical "and" between
all of the different possible places to enable/disable something: if the
platform is Android AND the platform is not "l" or "android-webview" AND the
"ShouldDisable" clause returns false, then run this benchmark

[nednguyen, 2017-03-27 17:22:28]

lgtm but please make sure Charlie is happy

[johnchen, 2017-03-27 19:54:03]

Thanks for the review and suggestions. I'll update the code based on the
suggestions, but it will take some extra time, as I'm currently in MTV for
another project.

[johnchen, 2017-03-28 04:25:13]

Uploaded new patch set with changes based on review feedback.

[johnchen, 2017-03-28 04:31:19]

Running the new benchmark on Trybots has revealed an issue. Sometimes the test
fails with the following call stack:

TraceImportError: Expected no more than one CrGpuMain
    at Process.findAtMostOneThreadNamed
(/tracing/model/process_base.html:175:15)
    at ChromeGpuHelper.isGpuProcess
(/tracing/model/helpers/chrome_gpu_helper.html:30:20)
    at Model.getAllProcesses (/tracing/model/model.html:276:44)
    at findChromeGpuProcess
(/tracing/model/helpers/chrome_model_helper.html:32:30)
    at ChromeModelHelper (/tracing/model/helpers/chrome_model_helper.html:48:22)
    at Model.getOrCreateHelper (/tracing/model/model.html:142:60)
    at ChromeAuditor (/tracing/extras/chrome/chrome_auditor.html:35:34)
    at auditors.importOptions_.auditorConstructors.map.auditorConstructor
(/tracing/importer/import.html:234:35)
    at Array.map (native)
    at addImportStage (/tracing/importer/import.html:233:60)

This error most often occurs with smpte_3840x2160_60fps_vp9.webm, but has been
observed on other media pages as well. It looks like the code only expects a
single thread named CrGpuMain, but in reality Chrome on Android sometimes has
multiple threads with that name. Is this behavior expected? Similar error hasn't
been observed with desktop Chrome.

[crouleau1, 2017-03-28 05:25:16]

On 2017/03/28 04:31:19, johnchen wrote:
> Running the new benchmark on Trybots has revealed an issue. Sometimes the test
> fails with the following call stack:
> 
> TraceImportError: Expected no more than one CrGpuMain
>     at Process.findAtMostOneThreadNamed
> (/tracing/model/process_base.html:175:15)
>     at ChromeGpuHelper.isGpuProcess
> (/tracing/model/helpers/chrome_gpu_helper.html:30:20)
>     at Model.getAllProcesses (/tracing/model/model.html:276:44)
>     at findChromeGpuProcess
> (/tracing/model/helpers/chrome_model_helper.html:32:30)
>     at ChromeModelHelper
(/tracing/model/helpers/chrome_model_helper.html:48:22)
>     at Model.getOrCreateHelper (/tracing/model/model.html:142:60)
>     at ChromeAuditor (/tracing/extras/chrome/chrome_auditor.html:35:34)
>     at auditors.importOptions_.auditorConstructors.map.auditorConstructor
> (/tracing/importer/import.html:234:35)
>     at Array.map (native)
>     at addImportStage (/tracing/importer/import.html:233:60)
> 
> This error most often occurs with smpte_3840x2160_60fps_vp9.webm, but has been
> observed on other media pages as well. It looks like the code only expects a
> single thread named CrGpuMain, but in reality Chrome on Android sometimes has
> multiple threads with that name. Is this behavior expected? Similar error
hasn't
> been observed with desktop Chrome.

Seems like a bug in tracing. Perhaps you could file a crbug/ for this? For this
changeling, could you disable 4k (3840x2160 = 4k) so that you are not blocked as
the bug is getting fixed?

[johnchen, 2017-03-28 06:09:05]

On 2017/03/28 05:25:16, crouleau1 wrote:
> On 2017/03/28 04:31:19, johnchen wrote:
> > Running the new benchmark on Trybots has revealed an issue. Sometimes the
test
> > fails with the following call stack:
> > 
> > TraceImportError: Expected no more than one CrGpuMain
> >     at Process.findAtMostOneThreadNamed
> > (/tracing/model/process_base.html:175:15)
> >     at ChromeGpuHelper.isGpuProcess
> > (/tracing/model/helpers/chrome_gpu_helper.html:30:20)
> >     at Model.getAllProcesses (/tracing/model/model.html:276:44)
> >     at findChromeGpuProcess
> > (/tracing/model/helpers/chrome_model_helper.html:32:30)
> >     at ChromeModelHelper
> (/tracing/model/helpers/chrome_model_helper.html:48:22)
> >     at Model.getOrCreateHelper (/tracing/model/model.html:142:60)
> >     at ChromeAuditor (/tracing/extras/chrome/chrome_auditor.html:35:34)
> >     at auditors.importOptions_.auditorConstructors.map.auditorConstructor
> > (/tracing/importer/import.html:234:35)
> >     at Array.map (native)
> >     at addImportStage (/tracing/importer/import.html:233:60)
> > 
> > This error most often occurs with smpte_3840x2160_60fps_vp9.webm, but has
been
> > observed on other media pages as well. It looks like the code only expects a
> > single thread named CrGpuMain, but in reality Chrome on Android sometimes
has
> > multiple threads with that name. Is this behavior expected? Similar error
> hasn't
> > been observed with desktop Chrome.
> 
> Seems like a bug in tracing. Perhaps you could file a crbug/ for this? For
this
> changeling, could you disable 4k (3840x2160 = 4k) so that you are not blocked
as
> the bug is getting fixed?

Filed crbug/705847 for this issue, and added tag is_4k to
smpte_3840x2160_60fps_vp9.webm page so it is skipped on Android.

[chromium-reviews, 2017-03-28 15:28:04]

Are there any other videos that this is happening on as well? I'm okay with
dropping the 1080p videos temporarily as well if they are causing
flakiness. We can always bring them back later.

On Mar 27, 2017 11:09 PM, <johnchen@chromium.org> wrote:

On 2017/03/28 05:25:16, crouleau1 wrote:
> On 2017/03/28 04:31:19, johnchen wrote:
> > Running the new benchmark on Trybots has revealed an issue. Sometimes
the
test
> > fails with the following call stack:
> >
> > TraceImportError: Expected no more than one CrGpuMain
> > at Process.findAtMostOneThreadNamed
> > (/tracing/model/process_base.html:175:15)
> > at ChromeGpuHelper.isGpuProcess
> > (/tracing/model/helpers/chrome_gpu_helper.html:30:20)
> > at Model.getAllProcesses (/tracing/model/model.html:276:44)
> > at findChromeGpuProcess
> > (/tracing/model/helpers/chrome_model_helper.html:32:30)
> > at ChromeModelHelper
> (/tracing/model/helpers/chrome_model_helper.html:48:22)
> > at Model.getOrCreateHelper (/tracing/model/model.html:142:60)
> > at ChromeAuditor (/tracing/extras/chrome/chrome_auditor.html:35:34)
> > at auditors.importOptions_.auditorConstructors.map.auditorConstructor
> > (/tracing/importer/import.html:234:35)
> > at Array.map (native)
> > at addImportStage (/tracing/importer/import.html:233:60)
> >
> > This error most often occurs with smpte_3840x2160_60fps_vp9.webm, but
has
been
> > observed on other media pages as well. It looks like the code only
expects a
> > single thread named CrGpuMain, but in reality Chrome on Android
sometimes
has
> > multiple threads with that name. Is this behavior expected? Similar
error
> hasn't
> > been observed with desktop Chrome.
>
> Seems like a bug in tracing. Perhaps you could file a crbug/ for this? For
this
> changeling, could you disable 4k (3840x2160 = 4k) so that you are not
blocked
as
> the bug is getting fixed?

Filed crbug/705847 <https://crbug.com/705847> for this issue, and added tag
is_4k to
smpte_3840x2160_60fps_vp9.webm page so it is skipped on Android.

https://codereview.chromium.org/2775883003/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[CalebRouleau, 2017-03-28 21:28:41]

LGTM

[benjhayden, 2017-03-28 21:45:22]

lgtm

[johnchen, 2017-03-30 05:18:32]

On 2017/03/28 15:28:04, chromium-reviews wrote:
> Are there any other videos that this is happening on as well? I'm okay with
> dropping the 1080p videos temporarily as well if they are causing
> flakiness. We can always bring them back later.

The only video that fails consistently is smpte_3840x2160_60fps_vp9.webm.
Failures with other videos are rare and inconsistent, and isn't limited to 1080p
videos.

[johnchen, 2017-03-30 05:18:50]

The CQ bit was checked by johnchen@chromium.org

[johnchen, 2017-03-30 05:18:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
nednguyen@google.com, charliea@chromium.org
Link to the patchset: https://codereview.chromium.org/2775883003/#ps60001
(title: "Tag smpte_3840x2160_60fps_vp9.webm with is_4k")

[commit-bot: I haz the power, 2017-03-30 05:19:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 06:24:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-30 06:24:02]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[charliea (OOO until 10-5), 2017-03-30 14:22:01]

The CQ bit was checked by charliea@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 14:22:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 14:53:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-30 14:53:14]

Dry run: This issue passed the CQ dry run.

[johnchen, 2017-03-30 15:58:53]

The CQ bit was checked by johnchen@chromium.org

[commit-bot: I haz the power, 2017-03-30 15:59:16]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 16:06:08]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1490889533042370,
"parent_rev": "f1cfaa03b05e225971f220bed2451cb2dc4b497d", "commit_rev":
"7cda4ca0bfbdb63159254a5b274833f5b091a9a0"}

[commit-bot: I haz the power, 2017-03-30 16:07:13]

Description was changed from

==========
Port TBMv2-based media benchmark to Android

This is a continuation of porting media benchmarks to use TBMv2.
It allows using TBMv2 to collect power (using BattOr) and CPU time
metrics on Android devices, similar to what's already available
on desktop.

BUG=700160
==========

to

==========
Port TBMv2-based media benchmark to Android

This is a continuation of porting media benchmarks to use TBMv2.
It allows using TBMv2 to collect power (using BattOr) and CPU time
metrics on Android devices, similar to what's already available
on desktop.

BUG=700160

Review-Url: https://codereview.chromium.org/2775883003
Cr-Commit-Position: refs/heads/master@{#460781}
Committed:
https://chromium.googlesource.com/chromium/src/+/7cda4ca0bfbdb63159254a5b2748...
==========

[commit-bot: I haz the power, 2017-03-30 16:07:14]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/7cda4ca0bfbdb63159254a5b2748...