Android GLShader: Check return value of glCreateShader()

webrtc/api/java/android/org/webrtc/GlShader.java

Index: webrtc/api/java/android/org/webrtc/GlShader.java
diff --git a/webrtc/api/java/android/org/webrtc/GlShader.java b/webrtc/api/java/android/org/webrtc/GlShader.java
index ed14c7115d8f548f81e98bd1266fe271cc34e9d3..fb5e4c2f67759d7a0f83ba70a1a2c9b51f87d894 100644
--- a/webrtc/api/java/android/org/webrtc/GlShader.java
+++ b/webrtc/api/java/android/org/webrtc/GlShader.java
@@ -21,14 +21,17 @@ public class GlShader {
   private static final String TAG = "GlShader";
 
   private static int compileShader(int shaderType, String source) {
-    int[] result = new int[] {
-        GLES20.GL_FALSE
-    };
-    int shader = GLES20.glCreateShader(shaderType);
+    final int shader = GLES20.glCreateShader(shaderType);
+    if (shader == 0) {
+      throw new RuntimeException("glCreateShader() failed. GLES20 error: " + GLES20.glGetError());
+    }
     GLES20.glShaderSource(shader, source);
     GLES20.glCompileShader(shader);
-    GLES20.glGetShaderiv(shader, GLES20.GL_COMPILE_STATUS, result, 0);
-    if (result[0] != GLES20.GL_TRUE) {
+    int[] compileStatus = new int[] {
+        GLES20.GL_FALSE
+    };
+    GLES20.glGetShaderiv(shader, GLES20.GL_COMPILE_STATUS, compileStatus, 0);
+    if (compileStatus[0] != GLES20.GL_TRUE) {
       Logging.e(TAG, "Could not compile shader " + shaderType + ":" +
           GLES20.glGetShaderInfoLog(shader));
       throw new RuntimeException(GLES20.glGetShaderInfoLog(shader));
@@ -37,16 +40,14 @@ public class GlShader {
     return shader;
   }
 
-  private int vertexShader;
-  private int fragmentShader;
   private int program;
 
   public GlShader(String vertexSource, String fragmentSource) {
-    vertexShader = compileShader(GLES20.GL_VERTEX_SHADER, vertexSource);
-    fragmentShader = compileShader(GLES20.GL_FRAGMENT_SHADER, fragmentSource);
+    final int vertexShader = compileShader(GLES20.GL_VERTEX_SHADER, vertexSource);
+    final int fragmentShader = compileShader(GLES20.GL_FRAGMENT_SHADER, fragmentSource);
     program = GLES20.glCreateProgram();
     if (program == 0) {
-      throw new RuntimeException("Could not create program");
+      throw new RuntimeException("glCreateProgram() failed. GLES20 error: " + GLES20.glGetError());
     }
     GLES20.glAttachShader(program, vertexShader);
     GLES20.glAttachShader(program, fragmentShader);
@@ -60,6 +61,15 @@ public class GlShader {
           GLES20.glGetProgramInfoLog(program));
       throw new RuntimeException(GLES20.glGetProgramInfoLog(program));
     }
+    // According to the documentation of glLinkProgram():
+    // "After the link operation, applications are free to modify attached shader objects, compile
+    // attached shader objects, detach shader objects, delete shader objects, and attach additional
+    // shader objects. None of these operations affects the information log or the program that is
+    // part of the program object."
+    // But in practice, detaching shaders from the program seems to break some devices. Deleting the
+    // shaders are fine however - it will delete them when they are no longer attached to a program.
+    GLES20.glDeleteShader(vertexShader);
+    GLES20.glDeleteShader(fragmentShader);

[hbos, 2016/02/17 14:23:06]

The fact that it will be deleted when no longer attached explains why you can do
this, but it looks confusing. What is the benefit to deleting here as opposed to
in release()?

[magjed_webrtc, 2016/02/18 10:05:19]

The fact that they will be deleted when no longer attached is not the reason
that I can delete them here. In fact, I SHOULD be able detach them as well so
that they are in fact deleted, but some ill-behaved drivers don't like that.

Shader objects should exist for just long enough to successfully link a program
object, and they are not needed after that. It's cleaner and less maintenance to
delete them directly, because then we don't need to keep track of them.

[hbos, 2016/02/18 10:35:54]

And detaching occurs automagically at glDeleteProgram? In that case, lgtm.

[magjed_webrtc, 2016/02/18 11:03:42]

Yes, detaching occurs automagically in glDeleteProgram.

     GlUtil.checkNoGLES2Error("Creating GlShader");
   }
 
@@ -109,15 +119,6 @@ public class GlShader {
 
   public void release() {
     Logging.d(TAG, "Deleting shader.");
-    // Flag shaders for deletion (does not delete until no longer attached to a program).
-    if (vertexShader != -1) {
-      GLES20.glDeleteShader(vertexShader);
-      vertexShader = -1;
-    }
-    if (fragmentShader != -1) {
-      GLES20.glDeleteShader(fragmentShader);
-      fragmentShader = -1;
-    }
     // Delete program, automatically detaching any shaders from it.
     if (program != -1) {
       GLES20.glDeleteProgram(program);