Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2498883002:
exo: Add FPS counter to motion event client. (Closed)
|
Created:
4 years, 1 month ago by reveman Modified:
4 years, 1 month ago Reviewers:
Daniele Castagna CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionexo: Add FPS counter to motion event client.
This adds a fps counter and event time stamps to the frame
output.
BUG=661010
Committed: https://crrev.com/800d00b3c2ca7eedafe5cc9c4be4f56c860875ab
Cr-Commit-Position: refs/heads/master@{#431965}
Patch Set 1 #
Total comments: 17
Patch Set 2 : show ?? before we have fps to show\ #Messages
Total messages: 16 (7 generated)
reveman@chromium.org changed reviewers: + dcastagna@chromium.org
The CQ bit was checked by reveman@chromium.org 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.
https://codereview.chromium.org/2498883002/diff/1/components/exo/wayland/clie... File components/exo/wayland/clients/motion_events.cc (right): https://codereview.chromium.org/2498883002/diff/1/components/exo/wayland/clie... components/exo/wayland/clients/motion_events.cc:537: std::string fps_counter_text; Should it be initialized to something like "fps: N/A" in case show_fps_counter_ is true? https://codereview.chromium.org/2498883002/diff/1/components/exo/wayland/clie... components/exo/wayland/clients/motion_events.cc:541: text_paint.setColor(SK_ColorWHITE); It'd be much more readable if it had a black border. https://codereview.chromium.org/2498883002/diff/1/components/exo/wayland/clie... components/exo/wayland/clients/motion_events.cc:572: base::UintToString(frames / benchmark_interval.InSeconds()); Why not a floating point here? Should it also include something like "fps: " before the actual number? https://codereview.chromium.org/2498883002/diff/1/components/exo/wayland/clie... components/exo/wayland/clients/motion_events.cc:599: std::string text = base::UintToString(event_times.back()); Should we have something like "event ts: " before the timestamp? https://codereview.chromium.org/2498883002/diff/1/components/exo/wayland/clie... components/exo/wayland/clients/motion_events.cc:841: command_line->HasSwitch(switches::kShowFpsCounter), use_drm.get()); Why would you *not* want to see the fps? And if there is any reason for that, wouldn't it be better to have it on by default and have a flag to disable it?
https://codereview.chromium.org/2498883002/diff/1/components/exo/wayland/clie... File components/exo/wayland/clients/motion_events.cc (right): https://codereview.chromium.org/2498883002/diff/1/components/exo/wayland/clie... components/exo/wayland/clients/motion_events.cc:537: std::string fps_counter_text; On 2016/11/14 at 03:04:31, Daniele Castagna wrote: > Should it be initialized to something like "fps: N/A" in case show_fps_counter_ is true? I'd like to have the ability to run this without drawing any text for benchmark purposes. https://codereview.chromium.org/2498883002/diff/1/components/exo/wayland/clie... components/exo/wayland/clients/motion_events.cc:541: text_paint.setColor(SK_ColorWHITE); On 2016/11/14 at 03:04:32, Daniele Castagna wrote: > It'd be much more readable if it had a black border. The background is typically black and that makes this easy to read. I'd also like to keep this as simple as possible and I think this is good enough. https://codereview.chromium.org/2498883002/diff/1/components/exo/wayland/clie... components/exo/wayland/clients/motion_events.cc:572: base::UintToString(frames / benchmark_interval.InSeconds()); On 2016/11/14 at 03:04:32, Daniele Castagna wrote: > Why not a floating point here? > Should it also include something like "fps: " before the actual number? Just printing 2 digits makes it easy from a layout perspective and I think just the number is clean and good enough. https://codereview.chromium.org/2498883002/diff/1/components/exo/wayland/clie... components/exo/wayland/clients/motion_events.cc:599: std::string text = base::UintToString(event_times.back()); On 2016/11/14 at 03:04:32, Daniele Castagna wrote: > Should we have something like "event ts: " before the timestamp? Like to keep this consistent with above and just print the number. The user needs to look at the code to understand how what this time stamp is anyhow. https://codereview.chromium.org/2498883002/diff/1/components/exo/wayland/clie... components/exo/wayland/clients/motion_events.cc:841: command_line->HasSwitch(switches::kShowFpsCounter), use_drm.get()); On 2016/11/14 at 03:04:32, Daniele Castagna wrote: > Why would you *not* want to see the fps? And if there is any reason for that, wouldn't it be better to have it on by default and have a flag to disable it? I'd like to use this client as a benchmark that would produce the maximum number of bytes that can be transferred to screen per second without having any text rendering influence the results.
https://codereview.chromium.org/2498883002/diff/1/components/exo/wayland/clie... File components/exo/wayland/clients/motion_events.cc (right): https://codereview.chromium.org/2498883002/diff/1/components/exo/wayland/clie... components/exo/wayland/clients/motion_events.cc:537: std::string fps_counter_text; On 2016/11/14 at 15:00:49, reveman wrote: > On 2016/11/14 at 03:04:31, Daniele Castagna wrote: > > Should it be initialized to something like "fps: N/A" in case show_fps_counter_ is true? > > I'd like to have the ability to run this without drawing any text for benchmark purposes. Sure, but what I was suggesting is to have the N/A string in case you want the text (show_fps_counter_ is true), and it's the first benchmark period. https://codereview.chromium.org/2498883002/diff/1/components/exo/wayland/clie... components/exo/wayland/clients/motion_events.cc:541: text_paint.setColor(SK_ColorWHITE); On 2016/11/14 at 15:00:49, reveman wrote: > On 2016/11/14 at 03:04:32, Daniele Castagna wrote: > > It'd be much more readable if it had a black border. > > The background is typically black and that makes this easy to read. I'd also like to keep this as simple as possible and I think this is good enough. Acknowledged. https://codereview.chromium.org/2498883002/diff/1/components/exo/wayland/clie... components/exo/wayland/clients/motion_events.cc:572: base::UintToString(frames / benchmark_interval.InSeconds()); On 2016/11/14 at 15:00:49, reveman wrote: > On 2016/11/14 at 03:04:32, Daniele Castagna wrote: > > Why not a floating point here? > > Should it also include something like "fps: " before the actual number? > > Just printing 2 digits makes it easy from a layout perspective and I think just the number is clean and good enough. Should you round the number instead of getting floor(fps) then? https://codereview.chromium.org/2498883002/diff/1/components/exo/wayland/clie... components/exo/wayland/clients/motion_events.cc:841: command_line->HasSwitch(switches::kShowFpsCounter), use_drm.get()); On 2016/11/14 at 15:00:49, reveman wrote: > On 2016/11/14 at 03:04:32, Daniele Castagna wrote: > > Why would you *not* want to see the fps? And if there is any reason for that, wouldn't it be better to have it on by default and have a flag to disable it? > > I'd like to use this client as a benchmark that would produce the maximum number of bytes that can be transferred to screen per second without having any text rendering influence the results. Then you might want to be able not to print the event timestamps too.
https://codereview.chromium.org/2498883002/diff/1/components/exo/wayland/clie... File components/exo/wayland/clients/motion_events.cc (right): https://codereview.chromium.org/2498883002/diff/1/components/exo/wayland/clie... components/exo/wayland/clients/motion_events.cc:537: std::string fps_counter_text; On 2016/11/14 at 15:17:03, Daniele Castagna wrote: > On 2016/11/14 at 15:00:49, reveman wrote: > > On 2016/11/14 at 03:04:31, Daniele Castagna wrote: > > > Should it be initialized to something like "fps: N/A" in case show_fps_counter_ is true? > > > > I'd like to have the ability to run this without drawing any text for benchmark purposes. > > Sure, but what I was suggesting is to have the N/A string in case you want the text (show_fps_counter_ is true), and it's the first benchmark period. Done. But "??" as I want to keep text layout simple. https://codereview.chromium.org/2498883002/diff/1/components/exo/wayland/clie... components/exo/wayland/clients/motion_events.cc:572: base::UintToString(frames / benchmark_interval.InSeconds()); On 2016/11/14 at 15:17:03, Daniele Castagna wrote: > On 2016/11/14 at 15:00:49, reveman wrote: > > On 2016/11/14 at 03:04:32, Daniele Castagna wrote: > > > Why not a floating point here? > > > Should it also include something like "fps: " before the actual number? > > > > Just printing 2 digits makes it easy from a layout perspective and I think just the number is clean and good enough. > > Should you round the number instead of getting floor(fps) then? Done. https://codereview.chromium.org/2498883002/diff/1/components/exo/wayland/clie... components/exo/wayland/clients/motion_events.cc:841: command_line->HasSwitch(switches::kShowFpsCounter), use_drm.get()); On 2016/11/14 at 15:17:03, Daniele Castagna wrote: > On 2016/11/14 at 15:00:49, reveman wrote: > > On 2016/11/14 at 03:04:32, Daniele Castagna wrote: > > > Why would you *not* want to see the fps? And if there is any reason for that, wouldn't it be better to have it on by default and have a flag to disable it? > > > > I'd like to use this client as a benchmark that would produce the maximum number of bytes that can be transferred to screen per second without having any text rendering influence the results. > > Then you might want to be able not to print the event timestamps too. I think this is OK as these will only be printed as a result of motion events and that's not going to happen in the benchmark case I'm thinking of.
lgtm
The CQ bit was checked by dcastagna@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== exo: Add FPS counter to motion event client. This adds a fps counter and event time stamps to the frame output. BUG=661010 ========== to ========== exo: Add FPS counter to motion event client. This adds a fps counter and event time stamps to the frame output. BUG=661010 Committed: https://crrev.com/800d00b3c2ca7eedafe5cc9c4be4f56c860875ab Cr-Commit-Position: refs/heads/master@{#431965} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/800d00b3c2ca7eedafe5cc9c4be4f56c860875ab Cr-Commit-Position: refs/heads/master@{#431965} |
