Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2477383002:
exo: Add max frames pending flag 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 max frames pending flag and better benchmarking to motion event client.
--max-frames-pending flag can now be used to have the client
queue pending frames.
Synchronization has also been improved to avoid calling glFinish().
Stride is also used correctly with --use-drm flag.
BUG=661010
Committed: https://crrev.com/d052b1ae3efc8a67a48ef025c82d6cbf8b3e8edd
Cr-Commit-Position: refs/heads/master@{#431742}
Patch Set 1 #Patch Set 2 : fixes #
Total comments: 2
Patch Set 3 : fixes #Patch Set 4 : timer #Patch Set 5 : rebase #
Total comments: 18
Patch Set 6 : address feedback #Messages
Total messages: 17 (8 generated)
reveman@chromium.org changed reviewers: + dcastagna@chromium.org
https://codereview.chromium.org/2477383002/diff/20001/components/exo/wayland/... File components/exo/wayland/clients/motion_events.cc (right): https://codereview.chromium.org/2477383002/diff/20001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:504: Buffer* buffer = As discussed offline, we can create an helper function for the code in this block and refactor the code in the do while loop to use that. https://codereview.chromium.org/2477383002/diff/20001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:512: base::TimeTicks current_time = base::TimeTicks::Now(); Can we try re-using the code gpu/perftests/measurements.h?
Description was changed from ========== exo: Add max frames pending flag to motion event client. BUG= ========== to ========== exo: Add max frames pending flag and better benchmarking to motion event client. --max-frames-pending flag can now be used to have the client queue pending frames. Synchronization has also been improved to avoid calling glFinish(). Stride is also used correctly with --use-drm flag. BUG=661010 ==========
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...
ptal
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2477383002/diff/80001/components/exo/wayland/... File components/exo/wayland/clients/motion_events.cc (right): https://codereview.chromium.org/2477383002/diff/80001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:360: gl_surface_.get(), gl::GLContextAttribs()); What does this do? https://codereview.chromium.org/2477383002/diff/80001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:445: egl_sync_type = EGL_SYNC_FENCE_KHR; Nit: Can you use g_driver_egl.ext.b_EGL_EXT_image_flush_external later instead? https://codereview.chromium.org/2477383002/diff/80001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:537: base::TimeDelta benchmark_wall_time; Isn't this going be ~benchmark_interval? https://codereview.chromium.org/2477383002/diff/80001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:542: do { I think it'd be much more readable if we could manage to keep the do while body in a pag https://codereview.chromium.org/2477383002/diff/80001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:551: LOG(WARNING) << "Can't find free buffer"; nit: LOG(ERROR)? https://codereview.chromium.org/2477383002/diff/80001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:563: if (gpu_timer->IsAvailable()) { If we produce frames as fast as we can just flushing in between and as the last thing we just check if the timers are available, isn't it really likely the timers won't be available since the command buffer won't have executed till the point we inserted the timers? https://codereview.chromium.org/2477383002/diff/80001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:569: std::cout << frames << " frames in " << benchmark_interval.InSeconds() Should this mention that this is the number of frames produced and not necessarily displayed? https://codereview.chromium.org/2477383002/diff/80001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:589: base::ThreadTicks::WaitUntilInitialized(); Are you sure this is necessary? And if it is, can these two lines be before the main loop? https://codereview.chromium.org/2477383002/diff/80001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:646: DCHECK(sync != EGL_NO_SYNC_KHR); nit: DCHECK_NE? Nevermind, you'd need the cast to unsigned.
ptal https://codereview.chromium.org/2477383002/diff/80001/components/exo/wayland/... File components/exo/wayland/clients/motion_events.cc (right): https://codereview.chromium.org/2477383002/diff/80001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:360: gl_surface_.get(), gl::GLContextAttribs()); On 2016/11/12 at 00:04:05, Daniele Castagna wrote: > What does this do? needed for ToT to compile https://codereview.chromium.org/2477383002/diff/80001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:445: egl_sync_type = EGL_SYNC_FENCE_KHR; On 2016/11/12 at 00:04:05, Daniele Castagna wrote: > Nit: Can you use g_driver_egl.ext.b_EGL_EXT_image_flush_external later instead? There's no g_driver_egl.ext.b_ANDROID_native_fence_sync yet https://codereview.chromium.org/2477383002/diff/80001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:537: base::TimeDelta benchmark_wall_time; On 2016/11/12 at 00:04:05, Daniele Castagna wrote: > Isn't this going be ~benchmark_interval? This is different as discussed. https://codereview.chromium.org/2477383002/diff/80001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:542: do { On 2016/11/12 at 00:04:05, Daniele Castagna wrote: > I think it'd be much more readable if we could manage to keep the do while body in a pag Let's clean this up in a follow up instead. https://codereview.chromium.org/2477383002/diff/80001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:551: LOG(WARNING) << "Can't find free buffer"; On 2016/11/12 at 00:04:05, Daniele Castagna wrote: > nit: LOG(ERROR)? Done. https://codereview.chromium.org/2477383002/diff/80001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:563: if (gpu_timer->IsAvailable()) { On 2016/11/12 at 00:04:04, Daniele Castagna wrote: > If we produce frames as fast as we can just flushing in between and as the last thing we just check if the timers are available, isn't it really likely the timers won't be available since the command buffer won't have executed till the point we inserted the timers? Remove the GPU timers from latest patch. https://codereview.chromium.org/2477383002/diff/80001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:569: std::cout << frames << " frames in " << benchmark_interval.InSeconds() On 2016/11/12 at 00:04:05, Daniele Castagna wrote: > Should this mention that this is the number of frames produced and not necessarily displayed? Added a comment here about it. https://codereview.chromium.org/2477383002/diff/80001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:589: base::ThreadTicks::WaitUntilInitialized(); On 2016/11/12 at 00:04:05, Daniele Castagna wrote: > Are you sure this is necessary? And if it is, can these two lines be before the main loop? Not needed.
lgtm https://codereview.chromium.org/2477383002/diff/80001/components/exo/wayland/... File components/exo/wayland/clients/motion_events.cc (right): https://codereview.chromium.org/2477383002/diff/80001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:360: gl_surface_.get(), gl::GLContextAttribs()); On 2016/11/12 at 01:18:13, reveman wrote: > On 2016/11/12 at 00:04:05, Daniele Castagna wrote: > > What does this do? > > needed for ToT to compile We should at least build this target on the trybots.
The CQ bit was checked by reveman@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 #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== exo: Add max frames pending flag and better benchmarking to motion event client. --max-frames-pending flag can now be used to have the client queue pending frames. Synchronization has also been improved to avoid calling glFinish(). Stride is also used correctly with --use-drm flag. BUG=661010 ========== to ========== exo: Add max frames pending flag and better benchmarking to motion event client. --max-frames-pending flag can now be used to have the client queue pending frames. Synchronization has also been improved to avoid calling glFinish(). Stride is also used correctly with --use-drm flag. BUG=661010 Committed: https://crrev.com/d052b1ae3efc8a67a48ef025c82d6cbf8b3e8edd Cr-Commit-Position: refs/heads/master@{#431742} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/d052b1ae3efc8a67a48ef025c82d6cbf8b3e8edd Cr-Commit-Position: refs/heads/master@{#431742} |
