Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2714393002:
ash: Low-latency laser pointer. (Closed)
Descriptionash: Low-latency laser pointer.
This makes the laser pointer view send frames directly to the
display compositor and make use of HW overlays for low-latency
updates when available.
BUG=696385
Review-Url: https://codereview.chromium.org/2714393002
Cr-Commit-Position: refs/heads/master@{#454252}
Committed: https://chromium.googlesource.com/chromium/src/+/4d5e2b9a2cff5e6c1d27d1256f241cf6ede10697
Patch Set 1 #
Total comments: 2
Patch Set 2 : defer buffer updates and some code cleanup #Patch Set 3 : use pending draw instead of submitted frames as a throttling mechanism #Patch Set 4 : fix ash build deps and rebase #Patch Set 5 : fix rebase #
Total comments: 2
Patch Set 6 : mus support #Patch Set 7 : fix deps #
Total comments: 12
Patch Set 8 : rebase, add comments and use ack #
Total comments: 2
Patch Set 9 : remove :surfaces #
Messages
Total messages: 48 (32 generated)
reveman@chromium.org changed reviewers: + dcastagna@chromium.org, sammiequon@chromium.org
fsamuel@chromium.org changed reviewers: + fsamuel@chromium.org
Drive-by comment. https://codereview.chromium.org/2714393002/diff/1/ash/laser/laser_pointer_vie... File ash/laser/laser_pointer_view.cc (right): https://codereview.chromium.org/2714393002/diff/1/ash/laser/laser_pointer_vie... ash/laser/laser_pointer_view.cc:522: local_surface_id_ = id_allocator_.GenerateId(); So you only allocate a surface ID once? Do you have ever need to resize this view or change its device scale factor?
https://codereview.chromium.org/2714393002/diff/1/ash/laser/laser_pointer_vie... File ash/laser/laser_pointer_view.cc (right): https://codereview.chromium.org/2714393002/diff/1/ash/laser/laser_pointer_vie... ash/laser/laser_pointer_view.cc:522: local_surface_id_ = id_allocator_.GenerateId(); On 2017/02/27 at 16:07:29, Fady Samuel wrote: > So you only allocate a surface ID once? Do you have ever need to resize this view or change its device scale factor? Correct, we allocate this ID once. The widget/view is destroyed on stylus up and recreated on stylus down. If the scale factor or display size changes while the stylus is down then I think it's fine for that to not be reflected until we receive a new stylus down. Makes the code much simpler.
lgtm
reveman@chromium.org changed reviewers: + jbauman@chromium.org, oshima@chromium.org
+jbauman for DEPS:+gpu/command_buffer/client +oshima for ash/
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: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2714393002/diff/80001/ash/laser/laser_pointer... File ash/laser/laser_pointer_view.h (right): https://codereview.chromium.org/2714393002/diff/80001/ash/laser/laser_pointer... ash/laser/laser_pointer_view.h:41: public cc::CompositorFrameSinkSupportClient { This won't work in mus right? You may disable this for now, but please file a bug to implement for mus. Many cros-UI folks aren't familiar with compositor/HW overlay stuff, so brief documentation + link/pointers would be helpful. Maybe we should have a view class that does HW overlay?
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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
https://codereview.chromium.org/2714393002/diff/80001/ash/laser/laser_pointer... File ash/laser/laser_pointer_view.h (right): https://codereview.chromium.org/2714393002/diff/80001/ash/laser/laser_pointer... ash/laser/laser_pointer_view.h:41: public cc::CompositorFrameSinkSupportClient { On 2017/02/28 at 07:40:16, oshima wrote: > This won't work in mus right? You may disable this for now, but please file a bug to implement for mus. I've updated the patch so it should now work with mus. Not sure how to verify that though. > > Many cros-UI folks aren't familiar with compositor/HW overlay stuff, so brief documentation + link/pointers would be helpful. > > Maybe we should have a view class that does HW overlay? HW overlays shouldn't really matter here. This works without HW overlays and the general concept is just to use a GpuMemoryBuffer backed surface for single buffered output with minimal latency. It happens that on platforms with HW overlay support this can result in very low latency. The single buffered GpuMemoryBuffer usage here is unique and I don't think we'll have other cases in Chrome OS UI where we're going to do something similar but having some form of SurfaceView that handles the logic in UpdateSurface might be nice. I'm going to leave the exercise of creating such a thing until we have another use-case for it if that's OK? Exo is another use case but that's such a special case as a result of how it needs to handle buffer release events that I'm not sure it makes sense to share code there. I'm going to document this type of single-buffered rendering on dev.chromium.org once we have a real app (pepper3d or arc++) using it and we can then link to that here. https://docs.google.com/a/google.com/document/d/1HNfCZF7xxYL2KrYPKTUfm6XWfzjj... is the best documentation we have until then.
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.
DEPS lgtm. Just a few questions about the code. https://codereview.chromium.org/2714393002/diff/120001/ash/laser/laser_pointe... File ash/laser/laser_pointer_view.cc (right): https://codereview.chromium.org/2714393002/diff/120001/ash/laser/laser_pointe... ash/laser/laser_pointer_view.cc:306: const gfx::Rect& damage_rect) { On think this should be triggered by DidReceiveCompositorFrameAck instead. https://codereview.chromium.org/2714393002/diff/120001/ash/laser/laser_pointe... ash/laser/laser_pointer_view.cc:354: ->GetGpuMemoryBufferManager() I think we need to listen to ContextFactoryObserver::OnLostResources to know when to recreate this. https://codereview.chromium.org/2714393002/diff/120001/ash/laser/laser_pointe... ash/laser/laser_pointer_view.cc:383: gfx::Canvas canvas(update_rect.size(), scale_factor_, false); You might want to have a cache for this to avoid the cost of allocating and clearing it. https://codereview.chromium.org/2714393002/diff/120001/ash/laser/laser_pointe... ash/laser/laser_pointer_view.cc:397: previous_point.location -= widget_offset + update_rect.OffsetFromOrigin(); Might be easier to just translate the canvas rather than change all the point locations. https://codereview.chromium.org/2714393002/diff/120001/ash/laser/laser_pointe... ash/laser/laser_pointer_view.cc:471: void LaserPointerView::UpdateSurface() { We'd normally want to trigger this from OnBeginFrame instead so it'd be rate-limited. I don't know if that would hurt latency too much, though. https://codereview.chromium.org/2714393002/diff/120001/ash/laser/laser_pointe... ash/laser/laser_pointer_view.cc:490: if (!resource->context_provider) { Check if ContextGL()->GetGraphicsResetStatusKHR() is GL_NO_ERROR and if not throw this resource away and create a new one.
On 2017/02/28 17:53:27, reveman wrote: > https://codereview.chromium.org/2714393002/diff/80001/ash/laser/laser_pointer... > File ash/laser/laser_pointer_view.h (right): > > https://codereview.chromium.org/2714393002/diff/80001/ash/laser/laser_pointer... > ash/laser/laser_pointer_view.h:41: public cc::CompositorFrameSinkSupportClient { > On 2017/02/28 at 07:40:16, oshima wrote: > > This won't work in mus right? You may disable this for now, but please file a > bug to implement for mus. > > I've updated the patch so it should now work with mus. Not sure how to verify > that though. > > > > > Many cros-UI folks aren't familiar with compositor/HW overlay stuff, so brief > documentation + link/pointers would be helpful. > > > > Maybe we should have a view class that does HW overlay? > > HW overlays shouldn't really matter here. This works without HW overlays and the > general concept is just to use a GpuMemoryBuffer backed surface for single > buffered output with minimal latency. It happens that on platforms with HW > overlay support this can result in very low latency. > > The single buffered GpuMemoryBuffer usage here is unique and I don't think we'll > have other cases in Chrome OS UI where we're going to do something similar but > having some form of SurfaceView that handles the logic in UpdateSurface might be > nice. I'm going to leave the exercise of creating such a thing until we have > another use-case for it if that's OK? Exo is another use case but that's such a > special case as a result of how it needs to handle buffer release events that > I'm not sure it makes sense to share code there. > > I'm going to document this type of single-buffered rendering on http://dev.chromium.org > once we have a real app (pepper3d or arc++) using it and we can then link to > that here. > https://docs.google.com/a/google.com/document/d/1HNfCZF7xxYL2KrYPKTUfm6XWfzjj... > is the best documentation we have until then. I was wondering if this can be used for softwarwe cursor. Anyhow, we can look into it later. lgtm
reveman@chromium.org changed reviewers: + sky@chromium.org
+sky for mash/ https://codereview.chromium.org/2714393002/diff/120001/ash/laser/laser_pointe... File ash/laser/laser_pointer_view.cc (right): https://codereview.chromium.org/2714393002/diff/120001/ash/laser/laser_pointe... ash/laser/laser_pointer_view.cc:306: const gfx::Rect& damage_rect) { On 2017/03/01 at 01:12:01, jbauman wrote: > On think this should be triggered by DidReceiveCompositorFrameAck instead. Done. Will throttle frames more aggressively. This is good in the HW overlay case where each frame end up as a no-op. Might have negative impact when HW overlays are not supported but I can't tell a difference from testing so let's start with this. https://codereview.chromium.org/2714393002/diff/120001/ash/laser/laser_pointe... ash/laser/laser_pointer_view.cc:354: ->GetGpuMemoryBufferManager() On 2017/03/01 at 01:12:01, jbauman wrote: > I think we need to listen to ContextFactoryObserver::OnLostResources to know when to recreate this. GpuMemoryBuffers survive GPU process crashes. The textures and the GLImage doesn't but the idea with the current code is that if that happens it's fine that a stylus up event (will destroy the instance of this class) and another stylus down is needed to get output again. That keeps the code simple. I added a comment about this to latest patch. https://codereview.chromium.org/2714393002/diff/120001/ash/laser/laser_pointe... ash/laser/laser_pointer_view.cc:383: gfx::Canvas canvas(update_rect.size(), scale_factor_, false); On 2017/03/01 at 01:12:01, jbauman wrote: > You might want to have a cache for this to avoid the cost of allocating and clearing it. I considered that but with a good system allocator it might not make much difference in allocation cost. Minimizing the clear is tricky as this is already reduced to only the size that was damaged. Anyhow, there might be room for improvement here and I added a comment to the code about this in latest patch. https://codereview.chromium.org/2714393002/diff/120001/ash/laser/laser_pointe... ash/laser/laser_pointer_view.cc:397: previous_point.location -= widget_offset + update_rect.OffsetFromOrigin(); On 2017/03/01 at 01:12:01, jbauman wrote: > Might be easier to just translate the canvas rather than change all the point locations. Good idea. I'll keep it as is in current patch to minimize the delta but will follow up with another patch that improves this. https://codereview.chromium.org/2714393002/diff/120001/ash/laser/laser_pointe... ash/laser/laser_pointer_view.cc:471: void LaserPointerView::UpdateSurface() { On 2017/03/01 at 01:12:01, jbauman wrote: > We'd normally want to trigger this from OnBeginFrame instead so it'd be rate-limited. I don't know if that would hurt latency too much, though. That's another good follow up that I also considered including in this patch but decided to save for later to keep the change small. https://codereview.chromium.org/2714393002/diff/120001/ash/laser/laser_pointe... ash/laser/laser_pointer_view.cc:490: if (!resource->context_provider) { On 2017/03/01 at 01:12:01, jbauman wrote: > Check if ContextGL()->GetGraphicsResetStatusKHR() is GL_NO_ERROR and if not throw this resource away and create a new one. As I mentioned in the other comment. We don't worry about this as this class is considered short-lived. Added a comment about this here.
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...
On 2017/03/01 at 08:59:38, oshima wrote: > On 2017/02/28 17:53:27, reveman wrote: > > https://codereview.chromium.org/2714393002/diff/80001/ash/laser/laser_pointer... > > File ash/laser/laser_pointer_view.h (right): > > > > https://codereview.chromium.org/2714393002/diff/80001/ash/laser/laser_pointer... > > ash/laser/laser_pointer_view.h:41: public cc::CompositorFrameSinkSupportClient { > > On 2017/02/28 at 07:40:16, oshima wrote: > > > This won't work in mus right? You may disable this for now, but please file a > > bug to implement for mus. > > > > I've updated the patch so it should now work with mus. Not sure how to verify > > that though. > > > > > > > > Many cros-UI folks aren't familiar with compositor/HW overlay stuff, so brief > > documentation + link/pointers would be helpful. > > > > > > Maybe we should have a view class that does HW overlay? > > > > HW overlays shouldn't really matter here. This works without HW overlays and the > > general concept is just to use a GpuMemoryBuffer backed surface for single > > buffered output with minimal latency. It happens that on platforms with HW > > overlay support this can result in very low latency. > > > > The single buffered GpuMemoryBuffer usage here is unique and I don't think we'll > > have other cases in Chrome OS UI where we're going to do something similar but > > having some form of SurfaceView that handles the logic in UpdateSurface might be > > nice. I'm going to leave the exercise of creating such a thing until we have > > another use-case for it if that's OK? Exo is another use case but that's such a > > special case as a result of how it needs to handle buffer release events that > > I'm not sure it makes sense to share code there. > > > > I'm going to document this type of single-buffered rendering on http://dev.chromium.org > > once we have a real app (pepper3d or arc++) using it and we can then link to > > that here. > > https://docs.google.com/a/google.com/document/d/1HNfCZF7xxYL2KrYPKTUfm6XWfzjj... > > is the best documentation we have until then. > > I was wondering if this can be used for softwarwe cursor. Anyhow, we can look into it later. lgtm Yes, I think that might be worth an experiment as it would result in a sw cursor with lower latency than our current hw cursor, wouldn't be limited in size as current hw cursor and we could do fancy things like motion blur. If it turns out that we want this then sharing code with the laser pointer is definitely something we want to do.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/03/01 18:57:15, reveman wrote: > +sky for mash/ > > https://codereview.chromium.org/2714393002/diff/120001/ash/laser/laser_pointe... > File ash/laser/laser_pointer_view.cc (right): > > https://codereview.chromium.org/2714393002/diff/120001/ash/laser/laser_pointe... > ash/laser/laser_pointer_view.cc:306: const gfx::Rect& damage_rect) { > On 2017/03/01 at 01:12:01, jbauman wrote: > > On think this should be triggered by DidReceiveCompositorFrameAck instead. > > Done. Will throttle frames more aggressively. This is good in the HW overlay > case where each frame end up as a no-op. Might have negative impact when HW > overlays are not supported but I can't tell a difference from testing so let's > start with this. > > https://codereview.chromium.org/2714393002/diff/120001/ash/laser/laser_pointe... > ash/laser/laser_pointer_view.cc:354: ->GetGpuMemoryBufferManager() > On 2017/03/01 at 01:12:01, jbauman wrote: > > I think we need to listen to ContextFactoryObserver::OnLostResources to know > when to recreate this. > > GpuMemoryBuffers survive GPU process crashes. The textures and the GLImage > doesn't but the idea with the current code is that if that happens it's fine > that a stylus up event (will destroy the instance of this class) and another > stylus down is needed to get output again. That keeps the code simple. I added a > comment about this to latest patch. lgtm, makes sense. > > https://codereview.chromium.org/2714393002/diff/120001/ash/laser/laser_pointe... > ash/laser/laser_pointer_view.cc:383: gfx::Canvas canvas(update_rect.size(), > scale_factor_, false); > On 2017/03/01 at 01:12:01, jbauman wrote: > > You might want to have a cache for this to avoid the cost of allocating and > clearing it. > > I considered that but with a good system allocator it might not make much > difference in allocation cost. Minimizing the clear is tricky as this is already > reduced to only the size that was damaged. Anyhow, there might be room for > improvement here and I added a comment to the code about this in latest patch. > Yeah, it was a big improvement for tiles in the renderer, but I don't know how much it matters here.
LGTM https://codereview.chromium.org/2714393002/diff/140001/mash/BUILD.gn File mash/BUILD.gn (right): https://codereview.chromium.org/2714393002/diff/140001/mash/BUILD.gn#newcode124 mash/BUILD.gn:124: "//cc/surfaces:surfaces", You don't need the :surfaces here.
The CQ bit was checked by reveman@chromium.org
https://codereview.chromium.org/2714393002/diff/140001/mash/BUILD.gn File mash/BUILD.gn (right): https://codereview.chromium.org/2714393002/diff/140001/mash/BUILD.gn#newcode124 mash/BUILD.gn:124: "//cc/surfaces:surfaces", On 2017/03/02 at 03:07:56, sky wrote: > You don't need the :surfaces here. Done.
The patchset sent to the CQ was uploaded after l-g-t-m from sammiequon@chromium.org, oshima@chromium.org, sky@chromium.org, jbauman@chromium.org Link to the patchset: https://codereview.chromium.org/2714393002/#ps160001 (title: "remove :surfaces")
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": 160001, "attempt_start_ts": 1488458652737700,
"parent_rev": "7a537bf4f18a9ddc3b105d49a57ee0b924068e8e", "commit_rev":
"4d5e2b9a2cff5e6c1d27d1256f241cf6ede10697"}
Message was sent while issue was closed.
Description was changed from ========== ash: Low-latency laser pointer. This makes the laser pointer view send frames directly to the display compositor and make use of HW overlays for low-latency updates when available. BUG=696385 ========== to ========== ash: Low-latency laser pointer. This makes the laser pointer view send frames directly to the display compositor and make use of HW overlays for low-latency updates when available. BUG=696385 Review-Url: https://codereview.chromium.org/2714393002 Cr-Commit-Position: refs/heads/master@{#454252} Committed: https://chromium.googlesource.com/chromium/src/+/4d5e2b9a2cff5e6c1d27d1256f24... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/4d5e2b9a2cff5e6c1d27d1256f24... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
