Use GLES2Interface for shaders and programs

Use GLES2Interface for shaders and programs

This uses the gpu::gles2::GLES2Interface type in the gl renderer's shader
and program code instead of WebGraphicsContext3D. For production code, the
GLES2Interface is a direct interface to the real command buffer in use. For
cc_unittests, the GLES2Interface is a stub that wraps TestWebGraphicsContext3D
so we can continue to use the same stubs/mocks for now. Once we port all of
the production code over to using GLES2Interface we should port the test context
classes over to the new base interface.

BUG=181120
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=239405

[jamesr, 2013-12-03 02:25:11]

Here's an example of wrapping TestWebGraphicsContext3D in a GLES2Interface.  I
wrapped these functions by hand since we need so few in order to run the unit
tests since we never actually run the shader programs in unit tests.  I think I
can just proceed in this fashion and think about autogenerating / macroifying
the forwarding class once I get tired of the copy+paste+edit cycle.

WDYT?

[jamesr, 2013-12-03 02:25:21]

This patch is on top of https://codereview.chromium.org/54463007/

[danakj, 2013-12-03 19:24:49]

LGTM

https://codereview.chromium.org/93433004/diff/1/cc/output/program_binding.cc
File cc/output/program_binding.cc (right):

https://codereview.chromium.org/93433004/diff/1/cc/output/program_binding.cc#...
cc/output/program_binding.cc:85: if (!shader)
If we're going to DCHECK in here, is there any reason to keep the if()
statements too?

https://codereview.chromium.org/93433004/diff/1/cc/output/shader_unittest.cc
File cc/output/shader_unittest.cc (right):

https://codereview.chromium.org/93433004/diff/1/cc/output/shader_unittest.cc#...
cc/output/shader_unittest.cc:15: // The FakeWebGraphicsContext3D always uses a
mediump precision of 10 bits
Comment needs updating, and verify this is still true for the stub?

[jamesr, 2013-12-04 01:50:21]

PTAL

piman - could you approve the
+gpu/command_buffer/client/gles2_interface{,_stub}.h additions to cc/DEPS?

https://codereview.chromium.org/93433004/diff/1/cc/output/program_binding.cc
File cc/output/program_binding.cc (right):

https://codereview.chromium.org/93433004/diff/1/cc/output/program_binding.cc#...
cc/output/program_binding.cc:85: if (!shader)
On 2013/12/03 19:24:49, danakj wrote:
> If we're going to DCHECK in here, is there any reason to keep the if()
> statements too?

I don't think there is - removed.

https://codereview.chromium.org/93433004/diff/1/cc/output/shader_unittest.cc
File cc/output/shader_unittest.cc (right):

https://codereview.chromium.org/93433004/diff/1/cc/output/shader_unittest.cc#...
cc/output/shader_unittest.cc:15: // The FakeWebGraphicsContext3D always uses a
mediump precision of 10 bits
On 2013/12/03 19:24:49, danakj wrote:
> Comment needs updating, and verify this is still true for the stub?

Good catch.  It isn't - I've changed this back to using the old test class and
forwarded the GLES2Interface::GetShaderPrecisionFormat() call to make sure it's
still hitting the old codepath.  Interestingly this test passes even if
GetShaderPrecisionFormat() is a no-op, indicating that it might not be a very
good test.

[piman, 2013-12-04 02:01:43]

lgtm

[danakj, 2013-12-04 16:39:43]

https://codereview.chromium.org/93433004/diff/1/cc/output/program_binding.cc
File cc/output/program_binding.cc (right):

https://codereview.chromium.org/93433004/diff/1/cc/output/program_binding.cc#...
cc/output/program_binding.cc:85: if (!shader)
On 2013/12/04 01:50:21, jamesr wrote:
> On 2013/12/03 19:24:49, danakj wrote:
> > If we're going to DCHECK in here, is there any reason to keep the if()
> > statements too?
> 
> I don't think there is - removed.

Ok cool, same question for all the other added DCHECKs in this file then.

[jamesr, 2013-12-05 23:30:03]

Sorry, I remembered more of the history here and changed the DCHECK()s in
program_binding back to early returns.  The issue here is that when debugging an
issue related to lost context, calls that check if the compile/link succeeded
will return false.  It's super annoying to have a DCHECK fire in the renderer in
this case.  So this returns failure from program_binding.cc and then in init
verifies that if the init failed anywhere it was because the context was
actually lost.

[danakj, 2013-12-05 23:39:06]

On 2013/12/05 23:30:03, jamesr wrote:
> Sorry, I remembered more of the history here and changed the DCHECK()s in
> program_binding back to early returns.  The issue here is that when debugging
an
> issue related to lost context, calls that check if the compile/link succeeded
> will return false.  It's super annoying to have a DCHECK fire in the renderer
in
> this case.  So this returns failure from program_binding.cc and then in init
> verifies that if the init failed anywhere it was because the context was
> actually lost.

Ah, I see. Sounds good.

[commit-bot: I haz the power, 2013-12-06 00:04:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/93433004/60001

[commit-bot: I haz the power, 2013-12-06 00:04:27]

Failed to apply patch for cc/test/test_gles2_interface.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  can't find file to patch at input line 6
  Perhaps you used the wrong -p or --strip option?
  The text leading up to this was:
  --------------------------
  |Index: cc/test/test_gles2_interface.cc
  |diff --git a/cc/test/test_gles2_interface.cc
b/cc/test/test_gles2_interface.cc
  |index
d0706871aa693d6cd8c8bc368981dcafc8335896..c9502d5c82628d4576f0f2f75e6038a2494c8c4a
100644
  |--- a/cc/test/test_gles2_interface.cc
  |+++ b/cc/test/test_gles2_interface.cc
  --------------------------
  No file to patch.  Skipping patch.
  2 out of 2 hunks ignored

Patch:       cc/test/test_gles2_interface.cc
Index: cc/test/test_gles2_interface.cc
diff --git a/cc/test/test_gles2_interface.cc b/cc/test/test_gles2_interface.cc
index
d0706871aa693d6cd8c8bc368981dcafc8335896..c9502d5c82628d4576f0f2f75e6038a2494c8c4a
100644
--- a/cc/test/test_gles2_interface.cc
+++ b/cc/test/test_gles2_interface.cc
@@ -5,6 +5,7 @@
 #include "cc/test/test_gles2_interface.h"
 
 #include "base/logging.h"
+#include "cc/test/test_web_graphics_context_3d.h"
 
 namespace cc {
 
@@ -15,4 +16,36 @@
TestGLES2Interface::TestGLES2Interface(TestWebGraphicsContext3D* test_context)
 
 TestGLES2Interface::~TestGLES2Interface() {}
 
+GLuint TestGLES2Interface::CreateShader(GLenum type) {
+  return test_context_->createShader(type);
+}
+
+GLuint TestGLES2Interface::CreateProgram() {
+  return test_context_->createProgram();
+}
+
+void TestGLES2Interface::GetShaderiv(GLuint shader,
+                                     GLenum pname,
+                                     GLint* params) {
+  test_context_->getShaderiv(shader, pname, params);
+}
+
+void TestGLES2Interface::GetProgramiv(GLuint program,
+                                      GLenum pname,
+                                      GLint* params) {
+  test_context_->getProgramiv(program, pname, params);
+}
+
+void TestGLES2Interface::GetShaderPrecisionFormat(GLenum shadertype,
+                                                  GLenum precisiontype,
+                                                  GLint* range,
+                                                  GLint* precision) {
+  test_context_->getShaderPrecisionFormat(
+      shadertype, precisiontype, range, precision);
+}
+
+void TestGLES2Interface::UseProgram(GLuint program) {
+  test_context_->useProgram(program);
+}
+
 }  // namespace cc

[commit-bot: I haz the power, 2013-12-06 02:56:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/93433004/80001

[commit-bot: I haz the power, 2013-12-06 02:57:04]

Failed to apply patch for cc/test/test_gles2_interface.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  can't find file to patch at input line 6
  Perhaps you used the wrong -p or --strip option?
  The text leading up to this was:
  --------------------------
  |Index: cc/test/test_gles2_interface.cc
  |diff --git a/cc/test/test_gles2_interface.cc
b/cc/test/test_gles2_interface.cc
  |index
d0706871aa693d6cd8c8bc368981dcafc8335896..c9502d5c82628d4576f0f2f75e6038a2494c8c4a
100644
  |--- a/cc/test/test_gles2_interface.cc
  |+++ b/cc/test/test_gles2_interface.cc
  --------------------------
  No file to patch.  Skipping patch.
  2 out of 2 hunks ignored

Patch:       cc/test/test_gles2_interface.cc
Index: cc/test/test_gles2_interface.cc
diff --git a/cc/test/test_gles2_interface.cc b/cc/test/test_gles2_interface.cc
index
d0706871aa693d6cd8c8bc368981dcafc8335896..c9502d5c82628d4576f0f2f75e6038a2494c8c4a
100644
--- a/cc/test/test_gles2_interface.cc
+++ b/cc/test/test_gles2_interface.cc
@@ -5,6 +5,7 @@
 #include "cc/test/test_gles2_interface.h"
 
 #include "base/logging.h"
+#include "cc/test/test_web_graphics_context_3d.h"
 
 namespace cc {
 
@@ -15,4 +16,36 @@
TestGLES2Interface::TestGLES2Interface(TestWebGraphicsContext3D* test_context)
 
 TestGLES2Interface::~TestGLES2Interface() {}
 
+GLuint TestGLES2Interface::CreateShader(GLenum type) {
+  return test_context_->createShader(type);
+}
+
+GLuint TestGLES2Interface::CreateProgram() {
+  return test_context_->createProgram();
+}
+
+void TestGLES2Interface::GetShaderiv(GLuint shader,
+                                     GLenum pname,
+                                     GLint* params) {
+  test_context_->getShaderiv(shader, pname, params);
+}
+
+void TestGLES2Interface::GetProgramiv(GLuint program,
+                                      GLenum pname,
+                                      GLint* params) {
+  test_context_->getProgramiv(program, pname, params);
+}
+
+void TestGLES2Interface::GetShaderPrecisionFormat(GLenum shadertype,
+                                                  GLenum precisiontype,
+                                                  GLint* range,
+                                                  GLint* precision) {
+  test_context_->getShaderPrecisionFormat(
+      shadertype, precisiontype, range, precision);
+}
+
+void TestGLES2Interface::UseProgram(GLuint program) {
+  test_context_->useProgram(program);
+}
+
 }  // namespace cc

[commit-bot: I haz the power, 2013-12-06 20:28:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/93433004/90001

[commit-bot: I haz the power, 2013-12-06 20:28:46]

Failed to apply patch for cc/test/test_gles2_interface.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  can't find file to patch at input line 6
  Perhaps you used the wrong -p or --strip option?
  The text leading up to this was:
  --------------------------
  |Index: cc/test/test_gles2_interface.cc
  |diff --git a/cc/test/test_gles2_interface.cc
b/cc/test/test_gles2_interface.cc
  |index
d0706871aa693d6cd8c8bc368981dcafc8335896..c9502d5c82628d4576f0f2f75e6038a2494c8c4a
100644
  |--- a/cc/test/test_gles2_interface.cc
  |+++ b/cc/test/test_gles2_interface.cc
  --------------------------
  No file to patch.  Skipping patch.
  2 out of 2 hunks ignored

Patch:       cc/test/test_gles2_interface.cc
Index: cc/test/test_gles2_interface.cc
diff --git a/cc/test/test_gles2_interface.cc b/cc/test/test_gles2_interface.cc
index
d0706871aa693d6cd8c8bc368981dcafc8335896..c9502d5c82628d4576f0f2f75e6038a2494c8c4a
100644
--- a/cc/test/test_gles2_interface.cc
+++ b/cc/test/test_gles2_interface.cc
@@ -5,6 +5,7 @@
 #include "cc/test/test_gles2_interface.h"
 
 #include "base/logging.h"
+#include "cc/test/test_web_graphics_context_3d.h"
 
 namespace cc {
 
@@ -15,4 +16,36 @@
TestGLES2Interface::TestGLES2Interface(TestWebGraphicsContext3D* test_context)
 
 TestGLES2Interface::~TestGLES2Interface() {}
 
+GLuint TestGLES2Interface::CreateShader(GLenum type) {
+  return test_context_->createShader(type);
+}
+
+GLuint TestGLES2Interface::CreateProgram() {
+  return test_context_->createProgram();
+}
+
+void TestGLES2Interface::GetShaderiv(GLuint shader,
+                                     GLenum pname,
+                                     GLint* params) {
+  test_context_->getShaderiv(shader, pname, params);
+}
+
+void TestGLES2Interface::GetProgramiv(GLuint program,
+                                      GLenum pname,
+                                      GLint* params) {
+  test_context_->getProgramiv(program, pname, params);
+}
+
+void TestGLES2Interface::GetShaderPrecisionFormat(GLenum shadertype,
+                                                  GLenum precisiontype,
+                                                  GLint* range,
+                                                  GLint* precision) {
+  test_context_->getShaderPrecisionFormat(
+      shadertype, precisiontype, range, precision);
+}
+
+void TestGLES2Interface::UseProgram(GLuint program) {
+  test_context_->useProgram(program);
+}
+
 }  // namespace cc

[commit-bot: I haz the power, 2013-12-07 01:07:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/93433004/110001

[commit-bot: I haz the power, 2013-12-07 01:07:08]

Failed to apply patch for cc/test/test_gles2_interface.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  can't find file to patch at input line 6
  Perhaps you used the wrong -p or --strip option?
  The text leading up to this was:
  --------------------------
  |Index: cc/test/test_gles2_interface.cc
  |diff --git a/cc/test/test_gles2_interface.cc
b/cc/test/test_gles2_interface.cc
  |index
d0706871aa693d6cd8c8bc368981dcafc8335896..c9502d5c82628d4576f0f2f75e6038a2494c8c4a
100644
  |--- a/cc/test/test_gles2_interface.cc
  |+++ b/cc/test/test_gles2_interface.cc
  --------------------------
  No file to patch.  Skipping patch.
  2 out of 2 hunks ignored

Patch:       cc/test/test_gles2_interface.cc
Index: cc/test/test_gles2_interface.cc
diff --git a/cc/test/test_gles2_interface.cc b/cc/test/test_gles2_interface.cc
index
d0706871aa693d6cd8c8bc368981dcafc8335896..c9502d5c82628d4576f0f2f75e6038a2494c8c4a
100644
--- a/cc/test/test_gles2_interface.cc
+++ b/cc/test/test_gles2_interface.cc
@@ -5,6 +5,7 @@
 #include "cc/test/test_gles2_interface.h"
 
 #include "base/logging.h"
+#include "cc/test/test_web_graphics_context_3d.h"
 
 namespace cc {
 
@@ -15,4 +16,36 @@
TestGLES2Interface::TestGLES2Interface(TestWebGraphicsContext3D* test_context)
 
 TestGLES2Interface::~TestGLES2Interface() {}
 
+GLuint TestGLES2Interface::CreateShader(GLenum type) {
+  return test_context_->createShader(type);
+}
+
+GLuint TestGLES2Interface::CreateProgram() {
+  return test_context_->createProgram();
+}
+
+void TestGLES2Interface::GetShaderiv(GLuint shader,
+                                     GLenum pname,
+                                     GLint* params) {
+  test_context_->getShaderiv(shader, pname, params);
+}
+
+void TestGLES2Interface::GetProgramiv(GLuint program,
+                                      GLenum pname,
+                                      GLint* params) {
+  test_context_->getProgramiv(program, pname, params);
+}
+
+void TestGLES2Interface::GetShaderPrecisionFormat(GLenum shadertype,
+                                                  GLenum precisiontype,
+                                                  GLint* range,
+                                                  GLint* precision) {
+  test_context_->getShaderPrecisionFormat(
+      shadertype, precisiontype, range, precision);
+}
+
+void TestGLES2Interface::UseProgram(GLuint program) {
+  test_context_->useProgram(program);
+}
+
 }  // namespace cc

[commit-bot: I haz the power, 2013-12-07 02:15:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/93433004/150001

[commit-bot: I haz the power, 2013-12-07 04:02:25]

Retried try job too often on android_aosp for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_ao...

[commit-bot: I haz the power, 2013-12-08 01:12:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/93433004/170001

[commit-bot: I haz the power, 2013-12-08 22:14:13]

Change committed as 239405