[Remoting Android] Fix thread issue with OpenGL drawable

remoting/client/display/gl_renderer.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "remoting/client/display/gl_renderer.h"
 
 #include <algorithm>
 
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "remoting/client/display/drawable.h"
 #include "remoting/client/display/gl_canvas.h"
 #include "remoting/client/display/gl_math.h"
 #include "remoting/client/display/gl_renderer_delegate.h"
 #include "remoting/client/display/sys_opengl.h"
 #include "third_party/webrtc/modules/desktop_capture/desktop_frame.h"
 
 namespace remoting {
 
 namespace {
 
-bool CompareDrawableZOrder(base::WeakPtr<Drawable> a,
-                           base::WeakPtr<Drawable> b) {
+template <typename DrawablePtr>

[nicholss, 2017/01/13 16:45:01]

I don't understand why this is better.

+bool CompareDrawableZOrder(DrawablePtr a, DrawablePtr b) {
   return a->GetZIndex() < b->GetZIndex();
 }
 
 }  // namespace
 
 GlRenderer::GlRenderer() :
     weak_factory_(this) {
   weak_ptr_ = weak_factory_.GetWeakPtr();
   thread_checker_.DetachFromThread();
 }
@@ skipping 89 matching lines @@
   cursor_feedback_.SetCanvas(nullptr);
   cursor_.SetCanvas(nullptr);
   desktop_.SetCanvas(nullptr);
   canvas_.reset();
 }
 
 base::WeakPtr<GlRenderer> GlRenderer::GetWeakPtr() {
   return weak_ptr_;
 }
 
+// static
+std::unique_ptr<GlRenderer> GlRenderer::CreateGlRendererWithDesktop() {

[Sergey Ulanov, 2017/01/13 02:03:10]

This code looks weird. If the GlRenderer owns all the Drawables, why do we have
to add them to drawables_ here instead of in the constructor?

It feels like GlRenderer should really be split into two layers as follows:
 1. GlRender: holds list of drawables and knows how to draw them, but doesn't
know what they are
 2. DesktopRenderer: owns GlRenderer and all the drawables we need. Knows how to
update the drawables (e.g. move the cursor, etc.)

WDYT?

[Yuwei, 2017/01/13 02:21:08]

That's exactly what I plan to do.

[joedow, 2017/01/13 16:09:34]

To my knowledge Scott is actively working on this code, have you talked to
him/are coordinating your changes and plans with him?

[nicholss, 2017/01/13 16:45:01]

This is close to what the plan is. A single object that is the endpoint for the
events I think is the wrong strategy. I would like to see it decoupled so each
object that understands how to draw a component is tied to the outlet of that
event from the decode/protocol side maybe with a router of some type to help
shuffle the messages into where they need to go. 

In the end I would like to add the ability to add other drawables, like a FPS
stat display or help message bubble into the rendering layer with very little
codechange. 

+  std::unique_ptr<GlRenderer> renderer(new GlRenderer());
+  std::vector<Drawable*> drawables{&renderer->desktop_, &renderer->cursor_,
+                                   &renderer->cursor_feedback_};
+  std::sort(drawables.begin(), drawables.end(),

[Sergey Ulanov, 2017/01/13 02:03:10]

This is way more complicated than necessary. We have just 3 drawables and the
order is known in advance. Is it not possible to just initialize the drawables_
list in the proper order without sorting anything?

[nicholss, 2017/01/13 16:45:01]

that is if you assume nothing can be added or removed at runtime, which will not
be true always.

+            CompareDrawableZOrder<Drawable*>);
+  for (auto* drawable : drawables) {

[nicholss, 2017/01/13 16:45:01]

Why would you sort them, they are going to be sorted as they are added. This is
just duplicating code to avoid sorting a list of a very small amount of elements
a couple times. No need to optimize this at all as it only happens once per
session. webRTC is way slower than this operation. 

+    renderer->drawables_.push_back(drawable->GetWeakPtr());
+  }
+  return renderer;
+}
+
 void GlRenderer::RequestRender() {
   DCHECK(thread_checker_.CalledOnValidThread());
   if (render_scheduled_) {
     return;
   }
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE, base::Bind(&GlRenderer::OnRender, weak_ptr_));
   render_scheduled_ = true;
 }
 
 void GlRenderer::AddDrawable(base::WeakPtr<Drawable> drawable) {

[Sergey Ulanov, 2017/01/13 02:03:10]

Is this function still used anywhere?

[Yuwei, 2017/01/13 02:21:08]

I think the unit test is still using it. Also IIRC Scott has some fancy demo
Drawable for debugging that has not been checked in?

[nicholss, 2017/01/13 16:45:01]

This method is the entire point of my last commit. I am attempting to make the
entire renderer dynamic with layers than can be added and removed at will. If
this is removed then my work has been for nothing.

+  DCHECK(thread_checker_.CalledOnValidThread());
   drawable->SetCanvas(canvas_ ? canvas_->GetWeakPtr() : nullptr);
   drawables_.push_back(drawable);
-  std::sort(drawables_.begin(), drawables_.end(), CompareDrawableZOrder);
+  std::sort(drawables_.begin(), drawables_.end(),
+            CompareDrawableZOrder<base::WeakPtr<Drawable>>);
 }
 
 void GlRenderer::OnRender() {
   DCHECK(thread_checker_.CalledOnValidThread());
   render_scheduled_ = false;
   if (!delegate_ || !delegate_->CanRenderFrame()) {
     return;
   }
 
   if (canvas_) {
     canvas_->Clear();
     // Draw each drawable in order.
     for (auto& drawable : drawables_) {
       if (drawable->Draw()) {
         RequestRender();
       }
     }
   }
 
   delegate_->OnFrameRendered();
 
   while (!pending_done_callbacks_.empty()) {
     pending_done_callbacks_.front().Run();
     pending_done_callbacks_.pop();
   }
 }
 
-std::unique_ptr<GlRenderer> GlRenderer::CreateGlRendererWithDesktop() {

[nicholss, 2017/01/13 16:45:01]

I think it is a mistake to remove this and use the other version. too much
duplicated logic and it breaks the future expandability of the renderer.

-  std::unique_ptr<GlRenderer> renderer(new GlRenderer());
-  renderer->AddDrawable(renderer->desktop_.GetWeakPtr());
-  renderer->AddDrawable(renderer->cursor_.GetWeakPtr());
-  renderer->AddDrawable(renderer->cursor_feedback_.GetWeakPtr());
-  return renderer;
-}
-
 }  // namespace remoting