Adding drawable to CRD andorid and iOS gl rendering pipeline.

remoting/client/gl_canvas.cc

Index: remoting/client/gl_canvas.cc
diff --git a/remoting/client/gl_canvas.cc b/remoting/client/gl_canvas.cc
index 0a49f13ec26b8f404700d8aa31a9cbf5605f2fed..65e263e1789df7f54b217ac4a411b81f88505f33 100644
--- a/remoting/client/gl_canvas.cc
+++ b/remoting/client/gl_canvas.cc
@@ -65,7 +65,17 @@ const char kDrawTexFrag[] =
 
 namespace remoting {
 
+GlCanvas::GlCanvas() {}
+
 GlCanvas::GlCanvas(int gl_version) : gl_version_(gl_version) {
+#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

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

Can you reuse the Clear() method after everything is initialized?  Then you
wouldn't need duplicated code.

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

removing the clear function

+
   glGetIntegerv(GL_MAX_TEXTURE_SIZE, &max_texture_size_);
 
   vertex_shader_ = CompileShader(GL_VERTEX_SHADER, kTexCoordToViewVert);
@@ -84,16 +94,19 @@ GlCanvas::GlCanvas(int gl_version) : gl_version_(gl_version) {
   glEnableVertexAttribArray(tex_cord_location_);
   glEnable(GL_BLEND);
   glBlendFunc(GL_SRC_ALPHA, GL_ONE_MINUS_SRC_ALPHA);
+  gl_constructed_ = true;
 }
 
 GlCanvas::~GlCanvas() {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  glDisable(GL_BLEND);
-  glDisableVertexAttribArray(tex_cord_location_);
-  glDisableVertexAttribArray(position_location_);
-  glDeleteProgram(program_);
-  glDeleteShader(vertex_shader_);
-  glDeleteShader(fragment_shader_);
+  if (gl_constructed_) {

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

If you have a separate GlCanvas interface then I don't think you need
|gl_constructed_|...

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

This feels a bit odd.  Is there a way to guarantee the object is valid when it
is constructed?  I think that would be cleaner.

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

It is odd because this is not dependency injection based code so I had to do
funky stuff to test or rewrite it.

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

I don't think DI is required to prevent having test and production code in this
class.

A common approach is to use a factory, production uses a factory that provides a
fully initialized object, the test factory provides one specific to the task at
hand.  The class that uses the factory provides a "SetFactoryForTesting" method
which allows the test to pass in a factory that does what it wants.

[Sergey Ulanov, 2016/12/22 22:38:45]

In this case I don't think we need any of it. It should be possible to use real
OpenGL in unittests. We already do that on Android.

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

I would prefer to not test OpenGL and the renderer loop at the same time. It is
OS dependent and my preference while adding renderer tests (making sure the
stack/drawables has been created correctly) to be a true unit test rather than
an integration test.

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

I added an interface and moved to a more dependency injection model to allow for
testing.

[Yuwei, 2017/01/09 20:28:51]

The previous tests work based on these preconditions:
* Test binary can be successfully built with the corresponding
  OpenGL dependencies.
* Test code path doesn't call any gl* functions. For previous
  implementation that is OnSurfaceCreated never being called or
  canvas_ == nullptr

You can get it work as long as your changes/new tests fulfill these
preconditions. The extra interface/DI may or may not be required.

+    DCHECK(thread_checker_.CalledOnValidThread());
+    glDisable(GL_BLEND);
+    glDisableVertexAttribArray(tex_cord_location_);
+    glDisableVertexAttribArray(position_location_);
+    glDeleteProgram(program_);
+    glDeleteShader(vertex_shader_);
+    glDeleteShader(fragment_shader_);
+  }
 }
 
 void GlCanvas::SetTransformationMatrix(const std::array<float, 9>& matrix) {
@@ -113,6 +126,15 @@ void GlCanvas::SetViewSize(int width, int height) {
   view_size_set_ = true;
 }
 
+void GlCanvas::Clear() {
+  glClear(GL_COLOR_BUFFER_BIT);

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

glClearColor only specifies the color and glClear will actually clear the buffer
with the color so I think the order is wrong?

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

Fixed.

+
+#ifndef NDEBUG
+  // Set the background clear color to bright green for debugging purposes.
+  glClearColor(0.0f, 1.0f, 0.0f, 1.0f);
+#endif

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

Why is the NDEBUG case not here?

+}
+
 void GlCanvas::DrawTexture(int texture_id,
                            GLuint texture_handle,
                            GLuint vertex_buffer,
@@ -145,4 +167,12 @@ int GlCanvas::GetMaxTextureSize() const {
   return max_texture_size_;
 }
 
+GlCanvas* GlCanvas::CreateGlCanvas(int gl_version) {
+  if (gl_version > 0) {
+    return new GlCanvas(gl_version);
+  } else {
+    return new FakeGlCanvas();

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

I don't think this logic should be included in production code.  We should use a
factory or similar mechanism to provide fake instances for testing.

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

Removing.

+  }
+}
+
 }  // namespace remoting