Adding drawable to CRD andorid and iOS gl rendering pipeline.

remoting/client/gl_canvas.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/gl_canvas.h"
 
 #include "base/logging.h"
 #include "remoting/client/gl_helpers.h"
 #include "remoting/client/gl_math.h"
 
@@ skipping 47 matching lines @@
     "uniform float u_alpha_multiplier;\n"
     "void main() {\n"
     "  gl_FragColor = texture2D(u_texture, v_texCoord);\n"
     "  gl_FragColor.a *= u_alpha_multiplier;\n"
     "}";
 
 }  // namespace
 
 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);
   fragment_shader_ = CompileShader(GL_FRAGMENT_SHADER, kDrawTexFrag);
   program_ = CreateProgram(vertex_shader_, fragment_shader_);
   glUseProgram(program_);
 
   transform_location_ = glGetUniformLocation(program_, "u_transform");
   view_size_location_ = glGetUniformLocation(program_, "u_viewSize");
   texture_location_ = glGetUniformLocation(program_, "u_texture");
   alpha_multiplier_location_ =
       glGetUniformLocation(program_, "u_alpha_multiplier");
   position_location_ = glGetAttribLocation(program_, "a_position");
   tex_cord_location_ = glGetAttribLocation(program_, "a_texCoord");
   glEnableVertexAttribArray(position_location_);
   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) {
   DCHECK(thread_checker_.CalledOnValidThread());
   std::array<float, 9> transposed_matrix = matrix;
   TransposeTransformationMatrix(&transposed_matrix);
   glUniformMatrix3fv(transform_location_, 1, GL_FALSE,
                      transposed_matrix.data());
   transformation_set_ = true;
 }
 
 void GlCanvas::SetViewSize(int width, int height) {
   DCHECK(width > 0 && height > 0);
   glViewport(0, 0, width, height);
   float view_size[2] {width, height};
   glUniform2fv(view_size_location_, 1, view_size);
   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,
                            float alpha_multiplier) {
   DCHECK(thread_checker_.CalledOnValidThread());
   if (!view_size_set_ || !transformation_set_) {
     return;
   }
   glActiveTexture(GL_TEXTURE0 + texture_id);
   glBindTexture(GL_TEXTURE_2D, texture_handle);
@@ skipping 12 matching lines @@
 }
 
 int GlCanvas::GetGlVersion() const {
   return gl_version_;
 }
 
 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