Adding drawable to CRD andorid and iOS gl rendering pipeline.

remoting/client/gl_renderer.cc

Index: remoting/client/gl_renderer.cc
diff --git a/remoting/client/gl_renderer.cc b/remoting/client/gl_renderer.cc
index 64ce7d31d537b0db9870ebe1a8a77bcb631153f5..c422f33a7a0bad0d7e82c32f3e51387b353770e9 100644
--- a/remoting/client/gl_renderer.cc
+++ b/remoting/client/gl_renderer.cc
@@ -17,13 +17,20 @@ namespace remoting {
 
 GlRenderer::GlRenderer() :
     weak_factory_(this) {
-  weak_ptr_ = weak_factory_.GetWeakPtr();
   thread_checker_.DetachFromThread();
 }
 
 GlRenderer::~GlRenderer() {
 }
 
+GlRenderer* GlRenderer::CreateGlRendererWithDesktop() {

[Yuwei, 2016/12/21 23:41:59]

I think it makes more sense to create a new class that wraps GlRenderer and
handles everything related desktop and cursor and make GlRenderer a class that
simply renders a stack of drawables.

[nicholss, 2017/01/09 18:50:24]

I think that makes sense but not required for this pass. Maybe next time.

+  GlRenderer* renderer = new GlRenderer();
+  renderer->AddDrawable(renderer->desktop_.GetWeakPtr());
+  renderer->AddDrawable(renderer->cursor_.GetWeakPtr());
+  renderer->AddDrawable(renderer->cursor_feedback_.GetWeakPtr());
+  return renderer;
+}
+
 void GlRenderer::SetDelegate(base::WeakPtr<GlRendererDelegate> delegate) {
   DCHECK(!delegate_);
   delegate_ = delegate;
@@ -48,24 +55,28 @@ void GlRenderer::OnPixelTransformationChanged(
   RequestRender();
 }
 
+// TODO(nicholss): This method seems like it should not be in this class.
 void GlRenderer::OnCursorMoved(float x, float y) {
   DCHECK(thread_checker_.CalledOnValidThread());
   cursor_.SetCursorPosition(x, y);
   RequestRender();
 }
 
+// TODO(nicholss): This method seems like it should not be in this class.
 void GlRenderer::OnCursorInputFeedback(float x, float y, float diameter) {
   DCHECK(thread_checker_.CalledOnValidThread());
   cursor_feedback_.StartAnimation(x, y, diameter);
   RequestRender();
 }
 
+// TODO(nicholss): This method seems like it should not be in this class.
 void GlRenderer::OnCursorVisibilityChanged(bool visible) {
   DCHECK(thread_checker_.CalledOnValidThread());
   cursor_.SetCursorVisible(visible);
   RequestRender();
 }
 
+// TODO(nicholss): This method seems like it should not be in this class.
 void GlRenderer::OnFrameReceived(std::unique_ptr<webrtc::DesktopFrame> frame,
                                  const base::Closure& done) {
   DCHECK(thread_checker_.CalledOnValidThread());
@@ -84,6 +95,7 @@ void GlRenderer::OnFrameReceived(std::unique_ptr<webrtc::DesktopFrame> frame,
   RequestRender();
 }
 
+// TODO(nicholss): This method seems like it should not be in this class.
 void GlRenderer::OnCursorShapeChanged(const protocol::CursorShapeInfo& shape) {
   DCHECK(thread_checker_.CalledOnValidThread());
   cursor_.SetCursorShape(shape);
@@ -92,17 +104,10 @@ void GlRenderer::OnCursorShapeChanged(const protocol::CursorShapeInfo& shape) {
 
 void GlRenderer::OnSurfaceCreated(int gl_version) {
   DCHECK(thread_checker_.CalledOnValidThread());
-#ifndef NDEBUG
-  // Set the background clear color to bright green for debugging purposes.
-  glClearColor(0.0f, 1.0f, 0.0f, 1.0f);
-#else
-  // Set the background clear color to black.
-  glClearColor(0.0f, 0.0f, 0.0f, 1.0f);
-#endif
-  canvas_.reset(new GlCanvas(gl_version));
-  desktop_.SetCanvas(canvas_.get());
-  cursor_.SetCanvas(canvas_.get());
-  cursor_feedback_.SetCanvas(canvas_.get());
+  canvas_.reset(GlCanvas::CreateGlCanvas(gl_version));
+  for (auto& drawable : drawables_) {
+    drawable->SetCanvas(canvas_.get());
+  }
 }
 
 void GlRenderer::OnSurfaceChanged(int view_width, int view_height) {
@@ -117,14 +122,14 @@ void GlRenderer::OnSurfaceChanged(int view_width, int view_height) {
 
 void GlRenderer::OnSurfaceDestroyed() {
   DCHECK(thread_checker_.CalledOnValidThread());
-  cursor_feedback_.SetCanvas(nullptr);
-  cursor_.SetCanvas(nullptr);
-  desktop_.SetCanvas(nullptr);
+  for (auto& drawable : drawables_) {
+    drawable->SetCanvas(nullptr);
+  }
   canvas_.reset();
 }
 
 base::WeakPtr<GlRenderer> GlRenderer::GetWeakPtr() {
-  return weak_ptr_;
+  return weak_factory_.GetWeakPtr();

[Yuwei, 2016/12/21 23:41:59]

Why removing |weak_ptr_|? We used |weak_ptr_| since GetWeakPtr() is
thread-unsafe so we just call it once in ctor and store the pointer.

[nicholss, 2016/12/22 16:21:41]

Nothing in the documentation says this call is thread-unsafe. Are you sure?

[Yuwei, 2016/12/22 19:01:10]

Yep, this is an undocumented bug("feature"?). See https://crbug.com/627602

[joedow, 2016/12/22 19:18:09]

Thanks for pointing that out Yuwei!  That is a good point.  You want to make
sure all of the WeakPtr's are bound to the render thread.  I'm pretty sure they
are today but that doesn't prevent problems in the future.

[Sergey Ulanov, 2016/12/27 21:00:19]

weak ptrs get bound to a thread when they are dereferenced, not when they are
created. 
AFAICT GetWeakPtr() call is probably safe here. It may only be unsafe when it's
being called on multiple threads in parallel, which shouldn't be happening here.
Still agree with Yuwei it's better to keep weak_ptr_.
FWIW here is a related CL: https://codereview.chromium.org/1602443003 that will
clarify threading semantics for that method.

[nicholss, 2017/01/09 18:50:24]

reverted;

 }
 
 void GlRenderer::RequestRender() {
@@ -133,32 +138,32 @@ void GlRenderer::RequestRender() {
     return;
   }
   base::ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE, base::Bind(&GlRenderer::OnRender, weak_ptr_));
+      FROM_HERE, base::Bind(&GlRenderer::OnRender, GetWeakPtr()));
   render_scheduled_ = true;
 }
 
+void GlRenderer::AddDrawable(base::WeakPtr<GlDrawable> drawable) {
+  drawable->SetCanvas(canvas_.get());
+  drawables_.push_back(drawable);
+  std::sort(drawables_.begin(), drawables_.end(),
+            drawable::SortDrawablesZOrder);
+}
+
 void GlRenderer::OnRender() {
   DCHECK(thread_checker_.CalledOnValidThread());
   render_scheduled_ = false;
   if (!delegate_ || !delegate_->CanRenderFrame()) {
     return;
   }
-
   if (canvas_) {
-    glClear(GL_COLOR_BUFFER_BIT);
-
-    // Layers will be drawn from bottom to top.
-    desktop_.Draw();
-
-    // |cursor_feedback_| should be drawn before |cursor_| so that the cursor
-    // won't be covered by the feedback animation.
-    if (cursor_feedback_.Draw()) {
-      RequestRender();
+    canvas_->Clear();
+    // Draw each drawable in order.
+    for (auto& drawable : drawables_) {
+      if (drawable->Draw()) {
+        RequestRender();

[joedow, 2016/12/22 00:29:03]

Can you break out of the loop once a drawable indicates it should be drawn?

[nicholss, 2016/12/22 16:21:41]

No, request render means: next draw loop, I have something to draw. Like an
animation. Otherwise draw will only happen when an event causes a render to be
requested, like a new frame from the video decode is pushed in for example.

[Yuwei, 2016/12/22 19:01:10]

I think it worth it to add a comment about the return value to
GlDrawable::Draw().
FYI we only draw/swap buffer on demand since swapping buffer will block the
display thread for 0~16ms to vsync. The display thread is also used to do other
heavy works like uploading textures so we don't want it to be block want nothing
needs to be drawn.

[joedow, 2016/12/22 19:18:09]

Ah yeah, I misread that.  As long as RequestRender is a cheap operation then
this is fine.

+      }
     }
-
-    cursor_.Draw();
   }
-
   delegate_->OnFrameRendered();
 
   while (!pending_done_callbacks_.empty()) {