Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(448)

Issue 3020433002: Finish migrating media metrics to TBMv2 (Closed)

Created:
3 years, 3 months ago by johnchen
Modified:
3 years, 2 months ago
CC:
catapult-reviews_chromium.org, tracing-review_chromium.org
Target Ref:
refs/heads/master
Project:
catapult
Visibility:
Public.

Description

Finish migrating media metrics to TBMv2 Update media metrics to add buffering time, dropped frame count, and seek time. An example trace that can use these metrics is available at http://www/~zhanliang/3020433002.trace.html, with corresponding histograms at http://www/~zhanliang/3020433002.html. BUG=chromium:700160 Review-Url: https://chromiumcodereview.appspot.com/3020433002 Committed: https://chromium.googlesource.com/catapult/+/e79820f09d19a90d428f9534ce80e47203fe185c

Patch Set 1 #

Total comments: 2

Patch Set 2 : Refactoring #

Total comments: 6

Patch Set 3 : Optimization #

Patch Set 4 : Fix typo #

Total comments: 19

Patch Set 5 : More refactoring #

Total comments: 1

Patch Set 6 : Update based on feedback from crouleau@ #

Total comments: 2

Patch Set 7 : Remove an unnecessary if #

Unified diffs Side-by-side diffs Delta from patch set Stats (+365 lines, -80 lines) Patch
M tracing/tracing/metrics/media_metric.html View 1 2 3 4 5 6 3 chunks +138 lines, -45 lines 0 comments Download
M tracing/tracing/metrics/media_metric_test.html View 1 2 3 4 5 2 chunks +227 lines, -35 lines 0 comments Download

Messages

Total messages: 30 (10 generated)
johnchen
This CL, together with a CL in media directory (https://crrev.com/c/674166), finishes migrating media metrics to ...
3 years, 3 months ago (2017-09-20 18:07:07 UTC) #2
CalebRouleau
Thanks for working on this! I hope my comment isn't too rough: I just know ...
3 years, 3 months ago (2017-09-20 22:53:20 UTC) #5
johnchen
On 2017/09/20 22:53:20, CalebRouleau wrote: > Thanks for working on this! I hope my comment ...
3 years, 3 months ago (2017-09-21 04:34:32 UTC) #6
johnchen
On 2017/09/20 22:53:20, CalebRouleau wrote: > Thanks for working on this! I hope my comment ...
3 years, 3 months ago (2017-09-21 04:34:33 UTC) #7
benjhayden
Thanks! I have a few questions. https://codereview.chromium.org/3020433002/diff/20001/tracing/tracing/metrics/media_metric.html File tracing/tracing/metrics/media_metric.html (right): https://codereview.chromium.org/3020433002/diff/20001/tracing/tracing/metrics/media_metric.html#newcode116 tracing/tracing/metrics/media_metric.html:116: for (const event ...
3 years, 3 months ago (2017-09-21 05:57:30 UTC) #8
CalebRouleau
On 2017/09/21 04:34:33, johnchen wrote: > On 2017/09/20 22:53:20, CalebRouleau wrote: > > Thanks for ...
3 years, 3 months ago (2017-09-21 18:28:13 UTC) #9
johnchen
https://codereview.chromium.org/3020433002/diff/20001/tracing/tracing/metrics/media_metric.html File tracing/tracing/metrics/media_metric.html (right): https://codereview.chromium.org/3020433002/diff/20001/tracing/tracing/metrics/media_metric.html#newcode116 tracing/tracing/metrics/media_metric.html:116: for (const event of audioThread.getDescendantEvents()) { On 2017/09/21 05:57:30, ...
3 years, 3 months ago (2017-09-22 16:25:40 UTC) #10
CalebRouleau
https://codereview.chromium.org/3020433002/diff/60001/tracing/tracing/metrics/media_metric.html File tracing/tracing/metrics/media_metric.html (right): https://codereview.chromium.org/3020433002/diff/60001/tracing/tracing/metrics/media_metric.html#newcode73 tracing/tracing/metrics/media_metric.html:73: const droppedFrameCount = timeToVideoPlay === undefined ? undefined : ...
3 years, 3 months ago (2017-09-22 17:44:22 UTC) #11
johnchen
https://codereview.chromium.org/3020433002/diff/60001/tracing/tracing/metrics/media_metric.html File tracing/tracing/metrics/media_metric.html (right): https://codereview.chromium.org/3020433002/diff/60001/tracing/tracing/metrics/media_metric.html#newcode73 tracing/tracing/metrics/media_metric.html:73: const droppedFrameCount = timeToVideoPlay === undefined ? undefined : ...
3 years, 3 months ago (2017-09-23 04:43:03 UTC) #12
CalebRouleau
LGTM % nits and offline nits. https://codereview.chromium.org/3020433002/diff/60001/tracing/tracing/metrics/media_metric.html File tracing/tracing/metrics/media_metric.html (right): https://codereview.chromium.org/3020433002/diff/60001/tracing/tracing/metrics/media_metric.html#newcode73 tracing/tracing/metrics/media_metric.html:73: const droppedFrameCount = ...
3 years, 2 months ago (2017-09-25 23:48:10 UTC) #13
johnchen
A couple of minor updates: * Use an empty Map (instead of undefined) to indicate ...
3 years, 2 months ago (2017-09-26 01:13:45 UTC) #14
CalebRouleau
https://codereview.chromium.org/3020433002/diff/100001/tracing/tracing/metrics/media_metric.html File tracing/tracing/metrics/media_metric.html (right): https://codereview.chromium.org/3020433002/diff/100001/tracing/tracing/metrics/media_metric.html#newcode95 tracing/tracing/metrics/media_metric.html:95: if (seekTimes.size !== 0) { You can drop this ...
3 years, 2 months ago (2017-09-26 01:26:35 UTC) #15
johnchen
https://codereview.chromium.org/3020433002/diff/100001/tracing/tracing/metrics/media_metric.html File tracing/tracing/metrics/media_metric.html (right): https://codereview.chromium.org/3020433002/diff/100001/tracing/tracing/metrics/media_metric.html#newcode95 tracing/tracing/metrics/media_metric.html:95: if (seekTimes.size !== 0) { On 2017/09/26 01:26:35, CalebRouleau ...
3 years, 2 months ago (2017-09-26 02:22:13 UTC) #16
benjhayden
Thanks for the ping! The code looks good, but there are just a few other ...
3 years, 2 months ago (2017-09-26 23:25:43 UTC) #17
johnchen
Thanks for the suggestions! Please see reply below: > Can you add to the CL ...
3 years, 2 months ago (2017-09-27 00:43:35 UTC) #20
benjhayden
On 2017/09/27 at 00:43:35, johnchen wrote: > Thanks for the suggestions! Please see reply below: ...
3 years, 2 months ago (2017-09-27 20:48:16 UTC) #22
johnchen
On 2017/09/27 20:48:16, benjhayden wrote: > On 2017/09/27 at 00:43:35, johnchen wrote: > > Thanks ...
3 years, 2 months ago (2017-09-27 21:13:33 UTC) #23
benjhayden
On 2017/09/27 at 21:13:33, johnchen wrote: > On 2017/09/27 20:48:16, benjhayden wrote: > > On ...
3 years, 2 months ago (2017-09-27 22:13:19 UTC) #24
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/3020433002/120001
3 years, 2 months ago (2017-09-27 22:14:09 UTC) #27
commit-bot: I haz the power
3 years, 2 months ago (2017-09-27 23:18:48 UTC) #30
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/catapult/+/e79820f09d19a90d428f9534ce80e472...

Powered by Google App Engine
This is Rietveld 408576698