Take GL version and extensions correctly into account when binding functions

gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc

 // Copyright (c) 2012 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 "gpu/command_buffer/service/gles2_cmd_decoder.h"
 
 #include "base/command_line.h"
 #include "base/strings/string_number_conversions.h"
 #include "gpu/command_buffer/common/gles2_cmd_format.h"
 #include "gpu/command_buffer/common/gles2_cmd_utils.h"
@@ skipping 4721 matching lines @@
       false,   // request alpha
       false,   // request depth
       false,   // request stencil
       true);   // bind generates resource
   DoBindRenderbuffer(GL_RENDERBUFFER, client_renderbuffer_id_,
                     kServiceRenderbufferId);
   EXPECT_CALL(*gl_, GetError())
       .WillOnce(Return(GL_NO_ERROR))
       .WillOnce(Return(GL_OUT_OF_MEMORY))
       .RetiresOnSaturation();
-  EXPECT_CALL(*gl_, RenderbufferStorageMultisampleEXT(
+  EXPECT_CALL(*gl_, RenderbufferStorageMultisample(
       GL_RENDERBUFFER, 1, GL_RGBA, 100, 50))
       .Times(1)
       .RetiresOnSaturation();
   RenderbufferStorageMultisampleCHROMIUM cmd;
   cmd.Init(GL_RENDERBUFFER, 1, GL_RGBA4, 100, 50);
   EXPECT_EQ(error::kNoError, ExecuteCmd(cmd));
   EXPECT_EQ(GL_OUT_OF_MEMORY, GetGLError());
 }
 
 TEST_F(GLES2DecoderManualInitTest,
        RenderbufferStorageMultisampleCHROMIUMBadArgs) {
   InitDecoder(
       "GL_EXT_framebuffer_multisample",  // extensions
       false,   // has alpha
       false,   // has depth
       false,   // has stencil
       false,   // request alpha
       false,   // request depth
       false,   // request stencil
       true);   // bind generates resource
   DoBindRenderbuffer(GL_RENDERBUFFER, client_renderbuffer_id_,
                     kServiceRenderbufferId);
-  EXPECT_CALL(*gl_, RenderbufferStorageMultisampleEXT(_, _, _, _, _))
+  EXPECT_CALL(*gl_, RenderbufferStorageMultisample(_, _, _, _, _))
       .Times(0)
       .RetiresOnSaturation();
   RenderbufferStorageMultisampleCHROMIUM cmd;
   cmd.Init(GL_RENDERBUFFER, TestHelper::kMaxSamples + 1,
            GL_RGBA4, TestHelper::kMaxRenderbufferSize, 1);
   EXPECT_EQ(error::kNoError, ExecuteCmd(cmd));
   EXPECT_EQ(GL_INVALID_VALUE, GetGLError());
   cmd.Init(GL_RENDERBUFFER, TestHelper::kMaxSamples,
            GL_RGBA4, TestHelper::kMaxRenderbufferSize + 1, 1);
   EXPECT_EQ(error::kNoError, ExecuteCmd(cmd));
@@ skipping 15 matching lines @@
       false,   // request stencil
       false);  // bind generates resource
   DoBindRenderbuffer(GL_RENDERBUFFER, client_renderbuffer_id_,
                     kServiceRenderbufferId);
   InSequence sequence;
   EXPECT_CALL(*gl_, GetError())
       .WillOnce(Return(GL_NO_ERROR))
       .RetiresOnSaturation();
   EXPECT_CALL(
       *gl_,
-      RenderbufferStorageMultisampleEXT(GL_RENDERBUFFER,
-                                        TestHelper::kMaxSamples,
-                                        GL_RGBA,
-                                        TestHelper::kMaxRenderbufferSize,
-                                        1))
+      RenderbufferStorageMultisample(GL_RENDERBUFFER,

[Ken Russell (switch to Gerrit), 2013/12/10 00:07:07]

This change is problematic. This test explicitly tries to initialize with the
GL_EXT_framebuffer_multisample extension. A fix is needed for this issue.

+                                     TestHelper::kMaxSamples,
+                                     GL_RGBA,
+                                     TestHelper::kMaxRenderbufferSize,
+                                     1))
       .Times(1)
       .RetiresOnSaturation();
   EXPECT_CALL(*gl_, GetError())
       .WillOnce(Return(GL_NO_ERROR))
       .RetiresOnSaturation();
   RenderbufferStorageMultisampleCHROMIUM cmd;
   cmd.Init(GL_RENDERBUFFER, TestHelper::kMaxSamples,
            GL_RGBA4, TestHelper::kMaxRenderbufferSize, 1);
   EXPECT_EQ(error::kNoError, ExecuteCmd(cmd));
   EXPECT_EQ(GL_NO_ERROR, GetGLError());
@@ skipping 49 matching lines @@
         RenderbufferStorageMultisampleIMG(GL_RENDERBUFFER,
                                           TestHelper::kMaxSamples,
                                           GL_RGBA,
                                           TestHelper::kMaxRenderbufferSize,
                                           1))
         .Times(1)
         .RetiresOnSaturation();
   } else {
     EXPECT_CALL(
         *gl_,
-        RenderbufferStorageMultisampleEXT(GL_RENDERBUFFER,
-                                          TestHelper::kMaxSamples,
-                                          GL_RGBA,
-                                          TestHelper::kMaxRenderbufferSize,
-                                          1))
+        RenderbufferStorageMultisample(GL_RENDERBUFFER,

[no sievers, 2013/12/05 20:51:57]

Here we'd still want RenderbufferStorageMultisampleEXT for the specific mobile
render_to_texture extension.

[oetuaho, 2013/12/09 17:17:07]

The root problem here is really that the unit tests don't initialize the
bindings with a proper extension string. If that would be fixed,
RenderbufferStorageMultisampleEXT would be called here. I think I'll try to see
if re-initializing the bindings in GLES2DecoderTestWithExtensions::SetUp could
work.

[no sievers, 2013/12/09 20:05:24]

Or maybe to make it simpler just initialize it with all extensions in
gpu/command_buffer/common/unittest_main.cc. That would be compatible with what
happens now, and I think good enough since at this level we don't really test
anything about the bindings but rather mock all of them. That means that we also
fail tests if we get unexpected mock calls.

You could use the mock version/extension trick you do in the other places. Maybe
the python script could generate a constant string with all known extensions for
this purpose?

[Ken Russell (switch to Gerrit), 2013/12/10 00:07:07]

There are attempts in other tests, like
GLES2DecoderManualInitTest::RenderbufferStorageMultisampleCHROMIUM, above, to
initialize the decoder with an appropriate extension string. Why isn't that
working any more?

The change of which function is called here and above -- from
RenderbufferStorageMultisampleEXT to RenderbufferStorageMultisample -- is really
problematic and needs to be resolved before this patch can land. These
multisampling APIs have very different semantics.

[oetuaho, 2013/12/10 10:39:05]

Yes, the decoder is initialized with the appropriate extension string in the
tests, but the bindings are not. I do intend to fix this.

Ah, and now I think I get the semantic difference. So to clarify, this binding
is a different function depending on whether we're using ES or desktop GL, since
in desktop GL RenderbufferStorageMultisample !=
RenderbufferStorageMultisampleEXT, but in ES RenderbufferStorageMultisample ==
RenderbufferStorageMultisampleEXT == RenderbufferStorageMultisampleIMG. In ES
world this fallback actually looks correct to me (this line), but in the desktop
world it is definitely not correct (the case above Ken commented on). That the
fallback exists in the bindings in the first place is wrong from the desktop
point of view, so it could use some further cleanup, but for now it should be
enough to fix the unit tests so that the bindings are done as expected.

And I don't think that initializing the unit test bindings once with all known
extensions is good enough, since the extensions can be in conflict with each
other like this. They need to be reinitialized per test.

+                                       TestHelper::kMaxSamples,
+                                       GL_RGBA,
+                                       TestHelper::kMaxRenderbufferSize,
+                                       1))
         .Times(1)
         .RetiresOnSaturation();
   }
   EXPECT_CALL(*gl_, GetError())
       .WillOnce(Return(GL_NO_ERROR))
       .RetiresOnSaturation();
   RenderbufferStorageMultisampleEXT cmd;
   cmd.Init(GL_RENDERBUFFER, TestHelper::kMaxSamples,
            GL_RGBA4, TestHelper::kMaxRenderbufferSize, 1);
   EXPECT_EQ(error::kNoError, ExecuteCmd(cmd));
@@ skipping 3959 matching lines @@
 // TODO(gman): TexImage2DImmediate
 
 // TODO(gman): TexSubImage2DImmediate
 
 // TODO(gman): UseProgram
 
 // TODO(gman): SwapBuffers
 
 }  // namespace gles2
 }  // namespace gpu