Android GLShader: Check return value of glCreateShader()

Android GLShader: Check return value of glCreateShader()

This CL adds a check to see if the return value of GLES20.glCreateShader() is zero. Also, shaders are flagged for deletion immediately after glLinkProgram() instead of doing it in release().

BUG=b/27197590

Committed: https://crrev.com/347c0bb5b50878d16a80732302707ccf90bd77cf
Cr-Commit-Position: refs/heads/master@{#11668}

[magjed_webrtc, 2016-02-17 11:24:29]

Description was changed from

==========
Android GLShader: Check return value of glCreateShader()

This CL adds a check to see if the return value of GLES20.glCreateShader() is
zero. Also, shaders are flagged for deletion immediately after glLinkProgram()
instead of doing it in release().

BUG=b/27197590
==========

to

==========
Android GLShader: Check return value of glCreateShader()

This CL adds a check to see if the return value of GLES20.glCreateShader() is
zero. Also, shaders are flagged for deletion immediately after glLinkProgram()
instead of doing it in release().

BUG=b/27197590
==========

[magjed_webrtc, 2016-02-17 11:24:29]

magjed@webrtc.org changed reviewers:
+ hbos@webrtc.org

[magjed_webrtc, 2016-02-17 11:24:42]

hbos - Please take a look.

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

https://codereview.webrtc.org/1702953002/diff/1/webrtc/api/java/android/org/w...
File webrtc/api/java/android/org/webrtc/GlShader.java (right):

https://codereview.webrtc.org/1702953002/diff/1/webrtc/api/java/android/org/w...
webrtc/api/java/android/org/webrtc/GlShader.java:72:
GLES20.glDeleteShader(fragmentShader);
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]

https://codereview.webrtc.org/1702953002/diff/1/webrtc/api/java/android/org/w...
File webrtc/api/java/android/org/webrtc/GlShader.java (right):

https://codereview.webrtc.org/1702953002/diff/1/webrtc/api/java/android/org/w...
webrtc/api/java/android/org/webrtc/GlShader.java:72:
GLES20.glDeleteShader(fragmentShader);
On 2016/02/17 14:23:06, hbos wrote:
> 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()?

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]

lgtm

https://codereview.webrtc.org/1702953002/diff/1/webrtc/api/java/android/org/w...
File webrtc/api/java/android/org/webrtc/GlShader.java (right):

https://codereview.webrtc.org/1702953002/diff/1/webrtc/api/java/android/org/w...
webrtc/api/java/android/org/webrtc/GlShader.java:72:
GLES20.glDeleteShader(fragmentShader);
On 2016/02/18 10:05:19, magjed_webrtc wrote:
> On 2016/02/17 14:23:06, hbos wrote:
> > 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()?
> 
> 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.

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

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

https://codereview.webrtc.org/1702953002/diff/1/webrtc/api/java/android/org/w...
File webrtc/api/java/android/org/webrtc/GlShader.java (right):

https://codereview.webrtc.org/1702953002/diff/1/webrtc/api/java/android/org/w...
webrtc/api/java/android/org/webrtc/GlShader.java:72:
GLES20.glDeleteShader(fragmentShader);
On 2016/02/18 10:35:54, hbos wrote:
> On 2016/02/18 10:05:19, magjed_webrtc wrote:
> > On 2016/02/17 14:23:06, hbos wrote:
> > > 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()?
> > 
> > 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.
> 
> And detaching occurs automagically at glDeleteProgram? In that case, lgtm.

Yes, detaching occurs automagically in glDeleteProgram.

[magjed_webrtc, 2016-02-18 11:04:11]

The CQ bit was checked by magjed@webrtc.org

[commit-bot: I haz the power, 2016-02-18 11:04:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1702953002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1702953002/1

[commit-bot: I haz the power, 2016-02-18 11:47:48]

Description was changed from

==========
Android GLShader: Check return value of glCreateShader()

This CL adds a check to see if the return value of GLES20.glCreateShader() is
zero. Also, shaders are flagged for deletion immediately after glLinkProgram()
instead of doing it in release().

BUG=b/27197590
==========

to

==========
Android GLShader: Check return value of glCreateShader()

This CL adds a check to see if the return value of GLES20.glCreateShader() is
zero. Also, shaders are flagged for deletion immediately after glLinkProgram()
instead of doing it in release().

BUG=b/27197590
==========

[commit-bot: I haz the power, 2016-02-18 11:47:49]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-02-18 11:47:56]

Description was changed from

==========
Android GLShader: Check return value of glCreateShader()

This CL adds a check to see if the return value of GLES20.glCreateShader() is
zero. Also, shaders are flagged for deletion immediately after glLinkProgram()
instead of doing it in release().

BUG=b/27197590
==========

to

==========
Android GLShader: Check return value of glCreateShader()

This CL adds a check to see if the return value of GLES20.glCreateShader() is
zero. Also, shaders are flagged for deletion immediately after glLinkProgram()
instead of doing it in release().

BUG=b/27197590

Committed: https://crrev.com/347c0bb5b50878d16a80732302707ccf90bd77cf
Cr-Commit-Position: refs/heads/master@{#11668}
==========

[commit-bot: I haz the power, 2016-02-18 11:47:57]

Patchset 1 (id:??) landed as
https://crrev.com/347c0bb5b50878d16a80732302707ccf90bd77cf
Cr-Commit-Position: refs/heads/master@{#11668}