[Remoting Android] Fix thread issue with OpenGL drawable

remoting/client/display/gl_renderer.cc

Index: remoting/client/display/gl_renderer.cc
diff --git a/remoting/client/display/gl_renderer.cc b/remoting/client/display/gl_renderer.cc
index 8b2ee234a8dbae570d56f5e88bc5b4864c942e95..e88c3af8f05c99cf9fdc464bf2b5bc1673d2f559 100644
--- a/remoting/client/display/gl_renderer.cc
+++ b/remoting/client/display/gl_renderer.cc
@@ -20,8 +20,8 @@ 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();
 }
 
@@ -131,6 +131,19 @@ 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_) {
@@ -142,9 +155,11 @@ void GlRenderer::RequestRender() {
 }
 
 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() {
@@ -172,12 +187,4 @@ void GlRenderer::OnRender() {
   }
 }
 
-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