Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2484503002:
exo: Add useful flags and benchmarking 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 useful flags and benchmarking to motion event client.
FPS will now be printed to stdout every 5 seconds and the
following flags have been added:
--size=NxM, specifies the client buffer size.
--scale=N, specifies the client scale factor (ie. number
of physical pixels per DIP).
--num-rects=N, specifies the number of rotating rects to draw.
--fullscreen, specifies if client should be fullscreen.
BUG=661010
TEST=wayland-motion-events
Committed: https://crrev.com/381ce176825fb27f1ccf9a6bd24980fac3cbd030
Cr-Commit-Position: refs/heads/master@{#430180}
Patch Set 1 #Patch Set 2 : remove printf #Patch Set 3 : minor cleanup #
Total comments: 10
Patch Set 4 : address feedback #Messages
Total messages: 19 (13 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: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
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.
LGTM. Can you mention in the CL description that this CL adds fullscreen support, code to draw multiple rects and logic to print out fps statistics at intervals? https://codereview.chromium.org/2484503002/diff/40001/components/exo/wayland/... File components/exo/wayland/clients/motion_events.cc (right): https://codereview.chromium.org/2484503002/diff/40001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:214: size_t rects, nit: maybe rect_num? https://codereview.chromium.org/2484503002/diff/40001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:232: const size_t rects_; rect_num_? https://codereview.chromium.org/2484503002/diff/40001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:379: std::cout << frames << " frames in " << benchmark_interval.InSeconds() Couldn't we just print this info to the canvas itself? https://codereview.chromium.org/2484503002/diff/40001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:396: int h = (height_ + (event_times.size() / 2)) / event_times.size(); Is the + event_time.size/2 to make sure it'll cover the whole canvas? https://codereview.chromium.org/2484503002/diff/40001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:453: // Specifies the client scale factor. You might want to expand a little on that. If I were just reading this I'd expect that setting scale to 2 would make the client bigger, but it actually has the inverse effect.
Description was changed from ========== exo: Add useful flags and benchmarking to motion event client. BUG=661010 TEST=wayland-motion-events ========== to ========== exo: Add useful flags and benchmarking to motion event client. FPS will now be printed to stdout every 5 seconds and the following flags have been added: --size=NxM, specifies the client buffer size. --scale=N, specifies the client scale factor (ie. number of physical pixels per DIP). --num-rects=N, specifies the number of rotating rects to draw. --fullscreen, specifies if client should be fullscreen. BUG=661010 TEST=wayland-motion-events ==========
The CQ bit was checked by reveman@chromium.org
On 2016/11/05 at 21:49:20, dcastagna wrote: > LGTM. > > Can you mention in the CL description that this CL adds fullscreen support, code to draw multiple rects and logic to print out fps statistics at intervals? Done. https://codereview.chromium.org/2484503002/diff/40001/components/exo/wayland/... File components/exo/wayland/clients/motion_events.cc (right): https://codereview.chromium.org/2484503002/diff/40001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:214: size_t rects, On 2016/11/05 at 21:49:20, Daniele Castagna wrote: > nit: maybe rect_num? Done. "num_rects" now https://codereview.chromium.org/2484503002/diff/40001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:232: const size_t rects_; On 2016/11/05 at 21:49:19, Daniele Castagna wrote: > rect_num_? Done. https://codereview.chromium.org/2484503002/diff/40001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:379: std::cout << frames << " frames in " << benchmark_interval.InSeconds() On 2016/11/05 at 21:49:19, Daniele Castagna wrote: > Couldn't we just print this info to the canvas itself? Yes, lets add a --fps-counter flag for that in a follow up patch. Console output is still useful when running this remotely and it's guaranteed to not interfere with measurements. https://codereview.chromium.org/2484503002/diff/40001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:396: int h = (height_ + (event_times.size() / 2)) / event_times.size(); On 2016/11/05 at 21:49:19, Daniele Castagna wrote: > Is the + event_time.size/2 to make sure it'll cover the whole canvas? Yes, added a comment to make that clear in latest patch. https://codereview.chromium.org/2484503002/diff/40001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:453: // Specifies the client scale factor. On 2016/11/05 at 21:49:20, Daniele Castagna wrote: > You might want to expand a little on that. If I were just reading this I'd expect that setting scale to 2 would make the client bigger, but it actually has the inverse effect. Done.
The patchset sent to the CQ was uploaded after l-g-t-m from dcastagna@chromium.org Link to the patchset: https://codereview.chromium.org/2484503002/#ps60001 (title: "address feedback")
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 #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== exo: Add useful flags and benchmarking to motion event client. FPS will now be printed to stdout every 5 seconds and the following flags have been added: --size=NxM, specifies the client buffer size. --scale=N, specifies the client scale factor (ie. number of physical pixels per DIP). --num-rects=N, specifies the number of rotating rects to draw. --fullscreen, specifies if client should be fullscreen. BUG=661010 TEST=wayland-motion-events ========== to ========== exo: Add useful flags and benchmarking to motion event client. FPS will now be printed to stdout every 5 seconds and the following flags have been added: --size=NxM, specifies the client buffer size. --scale=N, specifies the client scale factor (ie. number of physical pixels per DIP). --num-rects=N, specifies the number of rotating rects to draw. --fullscreen, specifies if client should be fullscreen. BUG=661010 TEST=wayland-motion-events Committed: https://crrev.com/381ce176825fb27f1ccf9a6bd24980fac3cbd030 Cr-Commit-Position: refs/heads/master@{#430180} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/381ce176825fb27f1ccf9a6bd24980fac3cbd030 Cr-Commit-Position: refs/heads/master@{#430180} |
