Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2594573002:
Change smoothness frame-times metrics on CrOS (Closed)
DescriptionChange smoothness frame-times metrics on CrOS
This patch changes smoothness metrics on CrOS. The new metrics uses accurate
page flip timestamp from ozone to calculate frame times, which improves
smoothness display stats. It's based on when the frame becomes visible on
screen, closer to Android SurfaceFlinger based metrics.
BUG=chromium:675846
Review-Url: https://codereview.chromium.org/2594573002
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/20ccc20120bdc5c1faa3eed43429a144f5a7b8ce
Patch Set 1 #
Messages
Total messages: 30 (15 generated)
Description was changed from ========== Change smoothness frame-times metrics on CrOS This patch changes smoothness metrics on CrOS. The new metrics uses accurate page flip timestamp from ozone to calculate frame times, which improves smoothness display stats. It's based on when the frame becomes visible on screen, closer to Android SurfaceFlinger based metrics. BUG=chromium:675846 ========== to ========== Change smoothness frame-times metrics on CrOS This patch changes smoothness metrics on CrOS. The new metrics uses accurate page flip timestamp from ozone to calculate frame times, which improves smoothness display stats. It's based on when the frame becomes visible on screen, closer to Android SurfaceFlinger based metrics. BUG=chromium:675846 ==========
On 2016/12/20 08:18:42, RafaelC wrote: This is one CL in a set of two: - https://codereview.chromium.org/2593513002/ - https://codereview.chromium.org/2594573002/ (this one) Please take a look, thanks!
llozano@google.com changed reviewers: + laszio@chromium.org, manojgupta@chromium.org, yunlian@chromium.org
Could you please take a look ? Thank you
Can you be more specific about who you want to review what part of the change? The reviewers list is quite long, and I suspect I'm not the best reviewer here.
On 2017/01/04 13:33:44, tdresser wrote: > Can you be more specific about who you want to review what part of the change? > > The reviewers list is quite long, and I suspect I'm not the best reviewer here. Thx, i'll update the list.
Description was changed from ========== Change smoothness frame-times metrics on CrOS This patch changes smoothness metrics on CrOS. The new metrics uses accurate page flip timestamp from ozone to calculate frame times, which improves smoothness display stats. It's based on when the frame becomes visible on screen, closer to Android SurfaceFlinger based metrics. BUG=chromium:675846 ========== to ========== Change smoothness frame-times metrics on CrOS This patch changes smoothness metrics on CrOS. The new metrics uses accurate page flip timestamp from ozone to calculate frame times, which improves smoothness display stats. It's based on when the frame becomes visible on screen, closer to Android SurfaceFlinger based metrics. BUG=chromium:675846 ==========
Reviewers list updated. Please take a look. Thanks
Description was changed from ========== Change smoothness frame-times metrics on CrOS This patch changes smoothness metrics on CrOS. The new metrics uses accurate page flip timestamp from ozone to calculate frame times, which improves smoothness display stats. It's based on when the frame becomes visible on screen, closer to Android SurfaceFlinger based metrics. BUG=chromium:675846 ========== to ========== Change smoothness frame-times metrics on CrOS This patch changes smoothness metrics on CrOS. The new metrics uses accurate page flip timestamp from ozone to calculate frame times, which improves smoothness display stats. It's based on when the frame becomes visible on screen, closer to Android SurfaceFlinger based metrics. BUG=chromium:675846 ==========
eakuefner@chromium.org changed reviewers: + nednguyen@google.com, vmiura@chromium.org
+vmiura, nednguyen Victor, as owner of the smoothness benchmarks can you vet these changes, or defer to someone else who can? I'm happy to sign off as an owner of Telemetry if smoothness stakeholders think this is an okay change, but I'm curious about one thing up front: the title/description seems to imply that this is a CrOS-only change, but the code seems to change the general behavior of the smoothness metric. Can you clarify what that's about? Is this supposed to modify the behavior on all platforms, or just Chrome OS? If it's only supposed to modify the behavior on Chrome OS, are we comfortable with the implementations being platform-divergent?
nednguyen@google.com changed reviewers: + tdresser@chromium.org
On 2017/01/05 18:01:25, eakuefner wrote: > +vmiura, nednguyen > > Victor, as owner of the smoothness benchmarks can you vet these changes, or > defer to someone else who can? > > I'm happy to sign off as an owner of Telemetry if smoothness stakeholders think > this is an okay change, but I'm curious about one thing up front: the > title/description seems to imply that this is a CrOS-only change, but the code > seems to change the general behavior of the smoothness metric. Can you clarify > what that's about? Is this supposed to modify the behavior on all platforms, or > just Chrome OS? If it's only supposed to modify the behavior on Chrome OS, are > we comfortable with the implementations being platform-divergent? Yes it is CrOS-only change, the new frame-time metric is based on DrmEventFlipComplete event generated in Ozone, which only exists on CrOS. (rendering_stats.py line 151: HasDrmStats(gpu_process) returns True and timestamp_event_name is DrmEventFlipComplete on CrOS) I have forwarded the background email to you for your reference. Smoothness metric on android is based on when it becomes visible (timestamp_process is SurfaceFlinger, timestamp_event_name is vsync_before. Patch related: https://codereview.chromium.org/765773004, BUG=351916), while CrOS metric is based on when the call to SwapBuffers returns (timestamp_event_name is BenchmarkInstrumentation). There's gap between them. So we try to improve CrOS display stats and align it with android, making timestamp close to when a frame gets displayed on the screen. DrmEventFlipComplete has a timestamp that's done in the kernel, at the exact time the page flip happend, thus it's more accurate and closer.
On 2017/01/09 08:53:36, RafaelC wrote: > On 2017/01/05 18:01:25, eakuefner wrote: > > +vmiura, nednguyen > > > > Victor, as owner of the smoothness benchmarks can you vet these changes, or > > defer to someone else who can? > > > > I'm happy to sign off as an owner of Telemetry if smoothness stakeholders > think > > this is an okay change, but I'm curious about one thing up front: the > > title/description seems to imply that this is a CrOS-only change, but the code > > seems to change the general behavior of the smoothness metric. Can you clarify > > what that's about? Is this supposed to modify the behavior on all platforms, > or > > just Chrome OS? If it's only supposed to modify the behavior on Chrome OS, are > > we comfortable with the implementations being platform-divergent? > > > Yes it is CrOS-only change, the new frame-time metric is based on > DrmEventFlipComplete event generated in Ozone, which only exists on CrOS. > (rendering_stats.py line 151: HasDrmStats(gpu_process) returns True and > timestamp_event_name is DrmEventFlipComplete on CrOS) > > I have forwarded the background email to you for your reference. Smoothness > metric on android is based on when it becomes visible (timestamp_process is > SurfaceFlinger, timestamp_event_name is vsync_before. Patch related: > https://codereview.chromium.org/765773004, BUG=351916), while CrOS metric is > based on when the call to SwapBuffers returns (timestamp_event_name is > BenchmarkInstrumentation). There's gap between them. So we try to improve CrOS > display stats and align it with android, making timestamp close to when a frame > gets displayed on the screen. DrmEventFlipComplete has a timestamp that's done > in the kernel, at the exact time the page flip happend, thus it's more accurate > and closer. lgtm, this is a welcome improvement.
telemetry/ rs-lgtm. Since this is going to cause some movement in the metrics, you may want to see if achuith@ can help you make sure that movement is accounted for by folks interested in and/or sheriffing on these metrics from the CrOS side.
On 2017/01/09 18:52:52, eakuefner wrote: > telemetry/ rs-lgtm. > > Since this is going to cause some movement in the metrics, you may want to see > if achuith@ can help you make sure that movement is accounted for by folks > interested in and/or sheriffing on these metrics from the CrOS side. The patch will impact the frame_time related metrics(frame_times, frame_time_discrepancy, mean_frame_time and percentage_smooth), with new values being more accurate. Smoothness data on CrOS is given below to show the change. Chromium 55.0.2859.0, Platform 8793, Cyan metrics | score_before | score_after | frame_times (ms) | 18.91 | 18.84 | mean_frame_times (ms)| 20.67 | 20.42 | percentage_smooth(%) | 48.33 | 88.82 | New DrmPageFlip timestamp is vsync aligned, so there's less variance than SwapBuffers based timestamp. Percentage_smooth will go up significantly on CrOS, similar as that on Android.
lgtm
i have to sign the CLA before commit. But unfortunately, the original staff in charge of processing CLA request has left intel, we are trying to find a right person to deal with this. Sorry for the convenience.
The CQ bit was checked by zheda.chen@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by zheda.chen@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 1, "attempt_start_ts": 1485241006074820, "parent_rev":
"d1653ccfb81e5464f584c398548a8f3c4de4ed7f", "commit_rev":
"20ccc20120bdc5c1faa3eed43429a144f5a7b8ce"}
Message was sent while issue was closed.
Description was changed from ========== Change smoothness frame-times metrics on CrOS This patch changes smoothness metrics on CrOS. The new metrics uses accurate page flip timestamp from ozone to calculate frame times, which improves smoothness display stats. It's based on when the frame becomes visible on screen, closer to Android SurfaceFlinger based metrics. BUG=chromium:675846 ========== to ========== Change smoothness frame-times metrics on CrOS This patch changes smoothness metrics on CrOS. The new metrics uses accurate page flip timestamp from ozone to calculate frame times, which improves smoothness display stats. It's based on when the frame becomes visible on screen, closer to Android SurfaceFlinger based metrics. BUG=chromium:675846 Review-Url: https://codereview.chromium.org/2594573002 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
