Make channel preemption not require view contexts for hookup

Make channel preemption not require view contexts for hookup

Instead have a single preemption flag on the GpuChannelManager, and make the
browser channel always be the one preempting, and the child channels always be
the ones being preempted (if --ui-prioritize-in-gpu-process is set). This
reflects our current usage, and is much simpler. It means we don't need
parent_client_id in GLSurfaceHandle any more, and all the renderer view surfaces
are identical, so we don't need their surface ids.

BUG=497658
Committed: https://crrev.com/4db52c1398011b124745567b5383572c2f319135
Cr-Commit-Position: refs/heads/master@{#350663}

[piman, 2015-09-23 04:43:04]

Patchset #1 (id:1) has been deleted

[piman, 2015-09-23 04:50:51]

piman@chromium.org changed reviewers:
+ danakj@chromium.org, kenrb@chromium.org, sievers@chromium.org

[piman, 2015-09-23 04:50:51]

sievers: please review
danakj: ui/gfx/native_widget_types.h
kenrb: content/common/gpu/gpu_messages.h

https://codereview.chromium.org/1365563002/diff/20001/content/browser/gpu/gpu...
File content/browser/gpu/gpu_surface_tracker.h (left):

https://codereview.chromium.org/1365563002/diff/20001/content/browser/gpu/gpu...
content/browser/gpu/gpu_surface_tracker.h:43: class SurfaceRef : public
base::RefCountedThreadSafe<SurfaceRef> {
This does nothing at all. I removed users.

https://codereview.chromium.org/1365563002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/1365563002/diff/20001/content/browser/rendere...
content/browser/renderer_host/compositor_impl_android.cc:408:
gfx::GLSurfaceHandle(surface_id_, gfx::NATIVE_DIRECT));
I moved the surface id into the handle slot, because that's more in-line with
the idea anyway, and it lets the GPU process have no notion of surface_id any
more.

https://codereview.chromium.org/1365563002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.cc (left):

https://codereview.chromium.org/1365563002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:313: surface_id_,
GetCompositingSurface());
I intend to remove the surface_id from the renderer altogether, but that's a
bigger change (this one is big already), so I will do that as a follow-up.

[danakj, 2015-09-23 18:46:37]

ui/gfx LGTM

[kenrb, 2015-09-23 19:11:07]

ipc lgtm

[no sievers, 2015-09-23 21:53:21]

lgtm. this is great.

https://codereview.chromium.org/1365563002/diff/80001/content/browser/composi...
File content/browser/compositor/gpu_process_transport_factory.cc (right):

https://codereview.chromium.org/1365563002/diff/80001/content/browser/composi...
content/browser/compositor/gpu_process_transport_factory.cc:559: #if
defined(OS_MACOSX)
While you're at it and are cleaning up the types, mind putting '||
(OS_ANDROID)'? :)
crbug.com/507792

https://codereview.chromium.org/1365563002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.cc (left):

https://codereview.chromium.org/1365563002/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:400: surface_id_,
GetCompositingSurface());
It looks to me like removing this would possibly also allow us to remove the
deferred logic to start the compositor in RenderWidget, which is for new widgets
created by the renderer (see RenderWiget::CompleteInit() and StartCompositor()).
Maybe put a TODO over there if you agree, so we can attempt this separately (it
might of course expose some other bug/race).

https://codereview.chromium.org/1365563002/diff/80001/content/common/gpu/gpu_...
File content/common/gpu/gpu_channel.cc (right):

https://codereview.chromium.org/1365563002/diff/80001/content/common/gpu/gpu_...
content/common/gpu/gpu_channel.cc:727: stub->SetPreemptByFlag(preempted_flag_);
Can you remove this method from GpuCommandBufferStub and pass it in the
constructor instead, now that we don't need the deferred setup anymore?

https://codereview.chromium.org/1365563002/diff/80001/content/common/gpu/gpu_...
File content/common/gpu/gpu_channel_manager.cc (right):

https://codereview.chromium.org/1365563002/diff/80001/content/common/gpu/gpu_...
content/common/gpu/gpu_channel_manager.cc:175: DCHECK(!params.preempts ||
!params.preempted);
nit: don't feel strongly at all, but you could also consider merging the two
bools into an enum describing the scheduling priority (UI vs. NORMAL or so) of
the channel.

[piman, 2015-09-23 23:33:37]

https://codereview.chromium.org/1365563002/diff/80001/content/browser/composi...
File content/browser/compositor/gpu_process_transport_factory.cc (right):

https://codereview.chromium.org/1365563002/diff/80001/content/browser/composi...
content/browser/compositor/gpu_process_transport_factory.cc:559: #if
defined(OS_MACOSX)
On 2015/09/23 21:53:21, sievers wrote:
> While you're at it and are cleaning up the types, mind putting '||
> (OS_ANDROID)'? :)
> crbug.com/507792

Yes, good point, done.

https://codereview.chromium.org/1365563002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.cc (left):

https://codereview.chromium.org/1365563002/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:400: surface_id_,
GetCompositingSurface());
On 2015/09/23 21:53:21, sievers wrote:
> It looks to me like removing this would possibly also allow us to remove the
> deferred logic to start the compositor in RenderWidget, which is for new
widgets
> created by the renderer (see RenderWiget::CompleteInit() and
StartCompositor()).
> Maybe put a TODO over there if you agree, so we can attempt this separately
(it
> might of course expose some other bug/race).

Added a TODO and filed a bug. I thought we still relied on it, but I can't find
the code to match, so it needs investigation, and we may be able to remove.

https://codereview.chromium.org/1365563002/diff/80001/content/common/gpu/gpu_...
File content/common/gpu/gpu_channel.cc (right):

https://codereview.chromium.org/1365563002/diff/80001/content/common/gpu/gpu_...
content/common/gpu/gpu_channel.cc:727: stub->SetPreemptByFlag(preempted_flag_);
On 2015/09/23 21:53:21, sievers wrote:
> Can you remove this method from GpuCommandBufferStub and pass it in the
> constructor instead, now that we don't need the deferred setup anymore?

Done.

https://codereview.chromium.org/1365563002/diff/80001/content/common/gpu/gpu_...
File content/common/gpu/gpu_channel_manager.cc (right):

https://codereview.chromium.org/1365563002/diff/80001/content/common/gpu/gpu_...
content/common/gpu/gpu_channel_manager.cc:175: DCHECK(!params.preempts ||
!params.preempted);
On 2015/09/23 21:53:21, sievers wrote:
> nit: don't feel strongly at all, but you could also consider merging the two
> bools into an enum describing the scheduling priority (UI vs. NORMAL or so) of
> the channel.

I'm expecting this to be superseded by the per-stream priorities, so I expect
this to be removed eventually. I didn't want to be too intrusive at the IPC
level by adding more complex enums (though in retrospect I'm not sure which is
better).

[piman, 2015-09-23 23:37:44]

The CQ bit was checked by piman@chromium.org

[piman, 2015-09-23 23:37:45]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
kenrb@chromium.org, sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/1365563002/#ps100001
(title: "review comments")

[commit-bot: I haz the power, 2015-09-24 01:02:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1365563002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1365563002/100001

[commit-bot: I haz the power, 2015-09-24 03:01:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-24 03:01:49]

Failed to apply patch for content/common/gpu/gpu_channel_manager.h:
While running git apply --index -3 -p1;
  error: patch failed: content/common/gpu/gpu_channel_manager.h:116
  error: repository lacks the necessary blob to fall back on 3-way merge.
  error: content/common/gpu/gpu_channel_manager.h: patch does not apply

Patch:       content/common/gpu/gpu_channel_manager.h
Index: content/common/gpu/gpu_channel_manager.h
diff --git a/content/common/gpu/gpu_channel_manager.h
b/content/common/gpu/gpu_channel_manager.h
index
ea3d94724de466e2a7e3f8bcc1480ab8f9c2d737..469d98cbc41881d306178a1748462d9d6ee9f258
100644
--- a/content/common/gpu/gpu_channel_manager.h
+++ b/content/common/gpu/gpu_channel_manager.h
@@ -33,6 +33,7 @@ class GLShareGroup;
 }
 
 namespace gpu {
+class PreemptionFlag;
 class SyncPointManager;
 union ValueState;
 namespace gles2 {
@@ -49,6 +50,7 @@ class SyncChannel;
 }
 
 struct GPUCreateCommandBufferConfig;
+struct GpuMsg_EstablishChannel_Params;
 
 namespace content {
 class GpuChannel;
@@ -116,13 +118,19 @@ class CONTENT_EXPORT GpuChannelManager : public
IPC::Listener,
 #endif
 
  protected:
-  virtual scoped_ptr<GpuChannel> CreateGpuChannel(
-      gfx::GLShareGroup* share_group,
-      gpu::gles2::MailboxManager* mailbox_manager,
-      int client_id,
-      uint64_t client_tracing_id,
-      bool allow_future_sync_points,
-      bool allow_real_time_streams);
+  virtual scoped_ptr<GpuChannel> CreateGpuChannel(int client_id,
+                                                  uint64_t client_tracing_id,
+                                                  bool preempts,
+                                                  bool
allow_future_sync_points,
+                                                  bool
allow_real_time_streams);
+
+  gfx::GLShareGroup* share_group() const { return share_group_.get(); }
+  gpu::gles2::MailboxManager* mailbox_manager() const {
+    return mailbox_manager_.get();
+  }
+  gpu::PreemptionFlag* preemption_flag() const {
+    return preemption_flag_.get();
+  }
 
   scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
   scoped_refptr<base::SingleThreadTaskRunner> io_task_runner_;
@@ -135,17 +143,11 @@ class CONTENT_EXPORT GpuChannelManager : public
IPC::Listener,
  private:
   // Message handlers.
   bool OnControlMessageReceived(const IPC::Message& msg);
-  void OnEstablishChannel(int client_id,
-                          uint64_t client_tracing_id,
-                          bool share_context,
-                          bool allow_future_sync_points,
-                          bool allow_real_time_streams);
+  void OnEstablishChannel(const GpuMsg_EstablishChannel_Params& params);
   void OnCloseChannel(const IPC::ChannelHandle& channel_handle);
-  void OnVisibilityChanged(
-      int32 render_view_id, int32 client_id, bool visible);
+  void OnVisibilityChanged(int32 render_view_id, int32 client_id, bool
visible);
   void OnCreateViewCommandBuffer(
       const gfx::GLSurfaceHandle& window,
-      int32 render_view_id,
       int32 client_id,
       const GPUCreateCommandBufferConfig& init_params,
       int32 route_id);
@@ -176,6 +178,7 @@ class CONTENT_EXPORT GpuChannelManager : public
IPC::Listener,
 
   scoped_refptr<gfx::GLShareGroup> share_group_;
   scoped_refptr<gpu::gles2::MailboxManager> mailbox_manager_;
+  scoped_refptr<gpu::PreemptionFlag> preemption_flag_;
   GpuMemoryManager gpu_memory_manager_;
   // SyncPointManager guaranteed to outlive running MessageLoop.
   gpu::SyncPointManager* sync_point_manager_;

[commit-bot: I haz the power, 2015-09-24 03:02:40]

The CQ bit was unchecked by commit-bot@chromium.org

[piman, 2015-09-24 19:45:50]

The CQ bit was checked by piman@chromium.org

[piman, 2015-09-24 19:45:50]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
kenrb@chromium.org, sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/1365563002/#ps120001
(title: "rebase")

[commit-bot: I haz the power, 2015-09-24 19:48:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1365563002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1365563002/120001

[commit-bot: I haz the power, 2015-09-24 21:12:38]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2015-09-24 21:13:44]

Patchset 6 (id:??) landed as
https://crrev.com/4db52c1398011b124745567b5383572c2f319135
Cr-Commit-Position: refs/heads/master@{#350663}