PPAPI: Make GLES2 calls resilient to bad/dead resources.

gpu/command_buffer/build_gles2_cmd_buffer.py

 #!/usr/bin/env python
 # 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.
 
 """code generator for GLES2 command buffers."""
 
 import itertools
 import os
 import os.path
@@ skipping 1194 matching lines @@
 # expectation:  If False the unit test will have no expected calls.
 # gen_func:     Name of function that generates GL resource for corresponding
 #               bind function.
 # states:       array of states that get set by this function corresponding to
 #               the given arguments
 # state_flag:   name of flag that is set to true when function is called.
 # no_gl:        no GL function is called.
 # valid_args:   A dictionary of argument indices to args to use in unit tests
 #               when they can not be automatically determined.
 # pepper_interface: The pepper interface that is used for this extension
+# pepper_args:  A string representing the argument list (what would appear in
+#               C/C++ between the parentheses for the function declaration)
+#               that the Pepper API expects for this function. Use this only if
+#               the stable Pepper API differs from the GLES2 argument list.
+# pepper_args_to_pass: A string representing the parameters to pass from the
+#               Pepper API to the CommandBuffer (i.e., what would appear in
+#               C/C++ between the parentheses for the function call). This is
+#               for cases where pepper_args is set, and there is some conversion
+#               (such as a type cast) required to call the CommandBuffer
+#               function.
 # invalid_test: False if no invalid test needed.
 # shadowed:     True = the value is shadowed so no glGetXXX call will be made.
 # first_element_only: For PUT types, True if only the first element of an
 #               array is used and we end up calling the single value
 #               corresponding function. eg. TexParameteriv -> TexParameteri
 
 _FUNCTION_INFO = {
   'ActiveTexture': {
     'decoder_func': 'DoActiveTexture',
     'unit_test': False,
@@ skipping 709 matching lines @@
     'client_test': False,
   },
   'ShaderSource': {
     'type': 'Manual',
     'immediate': True,
     'bucket': True,
     'needs_size': True,
     'client_test': False,
     'cmd_args':
         'GLuint shader, const char* data',
+    'pepper_args':
+        'GLuint shader, GLsizei count, const char** str, const GLint* length',
+    'pepper_args_to_pass':
+        'shader, count, const_cast<const char* const*>(str), length'

[piman, 2013/09/25 22:23:55]

Ah, sigh. A const got lost in translation and we shipped that...

[dmichael (off chromium), 2013/09/26 19:26:33]

Well, I realized that we don't actually need this cast (it adds a const). It's
still not great, but we can't change the Pepper API without revving, and it
doesn't seem worth dealing with the version skew for that (since we'd have to
support both versions anyway).

   },
   'StencilMask': {
     'type': 'StateSetFrontBack',
     'state': 'StencilMask',
     'no_gl': True,
     'expectation': False,
   },
   'StencilMaskSeparate': {
     'type': 'StateSetFrontBackSeparate',
     'state': 'StencilMask',
@@ skipping 344 matching lines @@
     'pepper_interface': 'Query',
   },
   'GetQueryObjectuivEXT': {
     'gen_cmd': False,
     'client_test': False,
     'gl_test_func': 'glGetQueryObjectuiv',
     'pepper_interface': 'Query',
   },
   'BindUniformLocationCHROMIUM': {
     'type': 'GLchar',
+    'extension': True,
     'bucket': True,
     'needs_size': True,
     'gl_test_func': 'DoBindUniformLocationCHROMIUM',
   },
   'InsertEventMarkerEXT': {
     'type': 'GLcharN',
     'decoder_func': 'DoInsertEventMarkerEXT',
     'expectation': False,
+    'extension': True,
   },
   'PushGroupMarkerEXT': {
     'type': 'GLcharN',
     'decoder_func': 'DoPushGroupMarkerEXT',
     'expectation': False,
+    'extension': True,
   },
   'PopGroupMarkerEXT': {
     'decoder_func': 'DoPopGroupMarkerEXT',
     'expectation': False,
+    'extension': True,
     'impl_func': False,
   },
 
   'GenVertexArraysOES': {
     'type': 'GENn',
+    'extension': True,
     'gl_test_func': 'glGenVertexArraysOES',
     'resource_type': 'VertexArray',
     'resource_types': 'VertexArrays',
     'unit_test': False,
   },
   'BindVertexArrayOES': {
     'type': 'Bind',
+    'extension': True,
     'gl_test_func': 'glBindVertexArrayOES',
     'decoder_func': 'DoBindVertexArrayOES',
     'gen_func': 'GenVertexArraysOES',
     'unit_test': False,
     'client_test': False,
   },
   'DeleteVertexArraysOES': {
     'type': 'DELn',
+    'extension': True,
     'gl_test_func': 'glDeleteVertexArraysOES',
     'resource_type': 'VertexArray',
     'resource_types': 'VertexArrays',
     'unit_test': False,
   },
   'IsVertexArrayOES': {
     'type': 'Is',
+    'extension': True,
     'gl_test_func': 'glIsVertexArrayOES',
     'decoder_func': 'DoIsVertexArrayOES',
     'expectation': False,
     'unit_test': False,
   },
   'BindTexImage2DCHROMIUM': {
     'decoder_func': 'DoBindTexImage2DCHROMIUM',
     'unit_test': False,
     'extension': True,
     'chromium': True,
@@ skipping 4025 matching lines @@
     return self.init_args
 
   def GetOriginalArgs(self):
     """Gets the original arguments to this function."""
     return self.original_args
 
   def GetLastOriginalArg(self):
     """Gets the last original argument to this function."""
     return self.original_args[len(self.original_args) - 1]
 
-  def __GetArgList(self, arg_string, add_comma):
+  def __MaybePrependComma(self, arg_string, add_comma):
     """Adds a comma if arg_string is not empty and add_comma is true."""
     comma = ""
     if add_comma and len(arg_string):
       comma = ", "
     return "%s%s" % (comma, arg_string)
 
   def MakeTypedOriginalArgString(self, prefix, add_comma = False):
-    """Gets a list of arguments as they arg in GL."""
+    """Gets a list of arguments as they are in GL."""
     args = self.GetOriginalArgs()
     arg_string = ", ".join(
         ["%s %s%s" % (arg.type, prefix, arg.name) for arg in args])
-    return self.__GetArgList(arg_string, add_comma)
+    return self.__MaybePrependComma(arg_string, add_comma)
 
   def MakeOriginalArgString(self, prefix, add_comma = False, separator = ", "):
     """Gets the list of arguments as they are in GL."""
     args = self.GetOriginalArgs()
     arg_string = separator.join(
         ["%s%s" % (prefix, arg.name) for arg in args])
-    return self.__GetArgList(arg_string, add_comma)
+    return self.__MaybePrependComma(arg_string, add_comma)
+
+  def MakeTypedPepperArgString(self, prefix):
+    """Gets a list of arguments as they need to be for Pepper."""
+    if self.GetInfo("pepper_args"):
+      return self.GetInfo("pepper_args")
+    else:
+      return self.MakeTypedOriginalArgString(prefix, False)
+
+  def MakePepperArgString(self, prefix):
+    """Gets the list of arguments to pass from a Pepper call."""
+    if self.GetInfo("pepper_args_to_pass"):
+      return self.GetInfo("pepper_args_to_pass")
+    else:
+      return self.MakeOriginalArgString(prefix, False)
 
   def MakeTypedCmdArgString(self, prefix, add_comma = False):
     """Gets a typed list of arguments as they need to be for command buffers."""
     args = self.GetCmdArgs()
     arg_string = ", ".join(
         ["%s %s%s" % (arg.type, prefix, arg.name) for arg in args])
-    return self.__GetArgList(arg_string, add_comma)
+    return self.__MaybePrependComma(arg_string, add_comma)
 
   def MakeCmdArgString(self, prefix, add_comma = False):
     """Gets the list of arguments as they need to be for command buffers."""
     args = self.GetCmdArgs()
     arg_string = ", ".join(
         ["%s%s" % (prefix, arg.name) for arg in args])
-    return self.__GetArgList(arg_string, add_comma)
+    return self.__MaybePrependComma(arg_string, add_comma)
 
   def MakeTypedInitString(self, prefix, add_comma = False):
     """Gets a typed list of arguments as they need to be for cmd Init/Set."""
     args = self.GetInitArgs()
     arg_string = ", ".join(
         ["%s %s%s" % (arg.type, prefix, arg.name) for arg in args])
-    return self.__GetArgList(arg_string, add_comma)
+    return self.__MaybePrependComma(arg_string, add_comma)
 
   def MakeInitString(self, prefix, add_comma = False):
     """Gets the list of arguments as they need to be for cmd Init/Set."""
     args = self.GetInitArgs()
     arg_string = ", ".join(
         ["%s%s" % (prefix, arg.name) for arg in args])
-    return self.__GetArgList(arg_string, add_comma)
+    return self.__MaybePrependComma(arg_string, add_comma)
 
   def MakeLogArgString(self):
     """Makes a string of the arguments for the LOG macros"""
     args = self.GetOriginalArgs()
     return ' << ", " << '.join([arg.GetLogArg() for arg in args])
 
   def WriteCommandDescription(self, file):
     """Writes a description of the command."""
     file.Write("//! Command that corresponds to gl%s.\n" % self.original_name)
 
@@ skipping 1131 matching lines @@
       file.Write("#define %s_1_0 \"%s;1.0\"\n" %
                  (interface.GetInterfaceName(), interface.GetInterfaceString()))
       file.Write("#define %s %s_1_0\n" %
                  (interface.GetInterfaceName(), interface.GetInterfaceName()))
 
       file.Write("\nstruct %s {\n" % interface.GetStructName())
       for func in self.original_functions:
         if not func.InPepperInterface(interface):
           continue
 
-        original_arg = func.MakeTypedOriginalArgString("")
+        original_arg = func.MakeTypedPepperArgString("")
         context_arg = "PP_Resource context"
         if len(original_arg):
           arg = context_arg + ", " + original_arg
         else:
           arg = context_arg
         file.Write("  %s (*%s)(%s);\n" % (func.return_type, func.name, arg))
       file.Write("};\n\n")
 
 
     file.Close()
 
   def WritePepperGLES2Implementation(self, filename):
     """Writes the Pepper OpenGLES interface implementation."""
 
     file = CWriter(filename)
     file.Write(_LICENSE)
     file.Write(_DO_NOT_EDIT_WARNING)
 
     file.Write("#include \"ppapi/shared_impl/ppb_opengles2_shared.h\"\n\n")
     file.Write("#include \"base/logging.h\"\n")
     file.Write("#include \"gpu/command_buffer/client/gles2_implementation.h\"\n")
     file.Write("#include \"ppapi/shared_impl/ppb_graphics_3d_shared.h\"\n")
     file.Write("#include \"ppapi/thunk/enter.h\"\n\n")
 
     file.Write("namespace ppapi {\n\n")
     file.Write("namespace {\n\n")
 
-    file.Write("gpu::gles2::GLES2Implementation*"
-               " GetGLES(PP_Resource context) {\n")
-    file.Write("  thunk::EnterResource<thunk::PPB_Graphics3D_API>"
-               " enter_g3d(context, false);\n")
-    file.Write("  DCHECK(enter_g3d.succeeded());\n")
-    file.Write("  return static_cast<PPB_Graphics3D_Shared*>"
-               "(enter_g3d.object())->gles2_impl();\n")
-    file.Write("}\n\n")
+    file.Write("typedef thunk::EnterResource<thunk::PPB_Graphics3D_API>"
+               " Enter3D;\n\n")
+
+    file.Write("gpu::gles2::GLES2Implementation* ToGles2Impl(Enter3D*"
+               " enter) {\n")
+    file.Write("  CHECK(enter);\n")
+    file.Write("  CHECK(enter->succeeded());\n")

[piman, 2013/09/25 22:23:55]

Why CHECK? This is done on every single GL call from the app, which can be a lot
(think 1000s per frame). I don't think we should have correctly written apps
suffer from badly written ones.

[dmichael (off chromium), 2013/09/26 19:26:33]

Yeah, that was probably overkill. The generated code basically guarantees it
doesn't happen. I'll make them DCHECKs.

+    file.Write("  return static_cast<PPB_Graphics3D_Shared*>(enter->object())->"
+               "gles2_impl();\n");
+    file.Write("}\n\n");
 
     for func in self.original_functions:
       if not func.InAnyPepperExtension():
         continue
 
-      original_arg = func.MakeTypedOriginalArgString("")
+      original_arg = func.MakeTypedPepperArgString("")
       context_arg = "PP_Resource context_id"
       if len(original_arg):
         arg = context_arg + ", " + original_arg
       else:
         arg = context_arg
       file.Write("%s %s(%s) {\n" % (func.return_type, func.name, arg))
+      file.Write("  Enter3D enter(context_id, true);\n")
+      file.Write("  if (enter.succeeded()) {\n")

[piman, 2013/09/25 22:23:55]

So this has a non-0 cost. Can we quantify it on some NaCl games?
The crash suggests Flash is obviously doing something wrong, and I don't think
we should have correct apps suffer from that (we should fix Flash). Some
implementations of GL crash if there is no current context, and I think it is
reasonable to do the same in Pepper (no current context == invalid resource), as
it is today.
If this has no measurable cost, then we can keep it.

[dmichael (off chromium), 2013/09/26 19:26:33]

For calls that use the command buffer, this CL is a net win. If you look at the
old code (GetGLES) it does EnterResource, which acquires the Pepper Proxy lock.
Then, it immediately drops it. Later, we had a "LockingCommandBuffer" which
grabbed the lock _again_. We had a race condition in between, and acquired the
lock twice. Now, we acquire the lock once for the duration of the call, so in
those cases we're definitely better off.

Even in the case where the GLES command does not use the command buffer (I don't
know how many of them that is; I poked around the code a little just now, but
it's not obvious to me), this is an inlined function that compares the result to
PP_OK (0). I suspect it's in the noise compared to the lock.

I went and compared with From Dust, and the FPS looks pretty similar with and
without on my HP Z620, FWIW. In Release, both are around 30fps. In Debug, the
new patch is possibly somewhat higher fps in some scenes. Not scientific, or
anything.

And I agree, it looks like Flash is doing something wrong, but I was going for
the tradeoff of greater stability, given that I think it's low/no cost. I
suspect most of the failures are benign. Flash may end up crashing elsewhere in
some of those cases, but it will be more obviously a Flash problem then.

 
       return_str = "" if func.return_type == "void" else "return "
-      file.Write("  %sGetGLES(context_id)->%s(%s);\n" %
+      file.Write("    %sToGles2Impl(&enter)->%s(%s);\n" %
                  (return_str, func.original_name,
-                  func.MakeOriginalArgString("")))
+                  func.MakePepperArgString("")))
+      file.Write("  }")
+      if func.return_type == "void":
+        file.Write("\n")
+      else:
+        file.Write(" else {\n")
+        error_return = "0"
+        if func.return_type == "GLboolean":
+          error_return = "GL_FALSE"
+        elif func.return_type == "GLenum":
+          if func.name == "CheckFramebufferStatus":
+            error_return = "GL_INVALID_ENUM"

[piman, 2013/09/25 22:23:55]

According to
http://www.opengl.org/sdk/docs/man3/xhtml/glCheckFramebufferStatus.xml I would
return 0. Note, this is the return value, not the GL error - the thing you get
with glGetError, which is what the "Errors" section of the man page refers to.
If you don't have a context, you can't get to the GL error anyway (glGetError
should also return 0).

[dmichael (off chromium), 2013/09/26 19:26:33]

Ah, thanks, I missed that sentence.

+          else:
+            error_return = "GL_INVALID_OPERATION"

[piman, 2013/09/25 22:23:55]

This should most likely be 0 too. If we want to be consistent with the spec, we
should audit each function. I haven't done so, but 0 seems like a reasonable
thing to do.

[dmichael (off chromium), 2013/09/26 19:26:33]

Done.

I went ahead and looked at all the functions that return stuff and added a
couple of exceptions to returning 0.

+        elif "*" in func.return_type:
+          error_return = "NULL"
+        file.Write("    return %s;\n" % error_return)
+        file.Write("  }\n")
       file.Write("}\n\n")
 
     file.Write("}  // namespace\n")
 
     for interface in self.pepper_interfaces:
       file.Write("const %s* PPB_OpenGLES2_Shared::Get%sInterface() {\n" %
                  (interface.GetStructName(), interface.GetName()))
       file.Write("  static const struct %s "
                  "ppb_opengles2 = {\n" % interface.GetStructName())
       file.Write("    &")
@@ skipping 146 matching lines @@
     gen.WriteGLES2Header("../GLES2/gl2chromium_autogen.h")
 
   if gen.errors > 0:
     print "%d errors" % gen.errors
     return 1
   return 0
 
 
 if __name__ == '__main__':
   sys.exit(main(sys.argv[1:]))