Android: Split out EGL rendering from SurfaceViewRenderer to separate class

Android: Split out EGL rendering from SurfaceViewRenderer to separate class

The purpose is to prepare for a TextureViewRenderer that will share the
EGL rendering code.

Two functional changes are also included:
* The implementation of SurfaceHolder.Callback.surfaceDestroyed will now
  block until the EGL surface is released. This is done in order to
  comply with the documentation that says: "If you have a rendering
  thread that directly accesses the surface, you must ensure that thread
  is no longer touching the Surface before returning from this function."
* We will no longer try to hide render glitches during layout changes.
  This was a lost cause anyway.

BUG=webrtc:6470
Committed: https://crrev.com/df494b09085986ba980b7128ed7bdd90636a9c08
Cr-Commit-Position: refs/heads/master@{#14570}

[magjed_webrtc, 2016-10-06 08:54:40]

Patchset #1 (id:1) has been deleted

[magjed_webrtc, 2016-10-06 08:58:31]

Patchset #1 (id:20001) has been deleted

[magjed_webrtc, 2016-10-06 09:05:55]

Description was changed from

==========
Android: Split out EGL rendering from SurfaceViewRenderer to separate class

The purpose is to prepare for a TextureViewRenderer that will share the
EGL rendering code.

BUG=webrtc:6470
==========

to

==========
Android: Split out EGL rendering from SurfaceViewRenderer to separate class

The purpose is to prepare for a TextureViewRenderer that will share the
EGL rendering code.

Two functional changes are also included:
* The implementation of SurfaceHolder.Callback.surfaceDestroyed will now
  block until the EGL surface is released. This is done in order to
  comply with the documentation that says: "If you have a rendering
  thread that directly accesses the surface, you must ensure that thread
  is no longer touching the Surface before returning from this function."
* We will no longer try to hide render glitches during layout changes.
  This was a lost cause anyway.

BUG=webrtc:6407
==========

[magjed_webrtc, 2016-10-06 09:05:55]

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

[magjed_webrtc, 2016-10-06 09:06:13]

Sami - please take a look.

[sakal, 2016-10-06 11:32:51]

https://codereview.webrtc.org/2399463006/diff/40001/webrtc/api/android/java/s...
File webrtc/api/android/java/src/org/webrtc/EglRenderer.java (left):

https://codereview.webrtc.org/2399463006/diff/40001/webrtc/api/android/java/s...
webrtc/api/android/java/src/org/webrtc/EglRenderer.java:440: throw new
IllegalStateException(getResourceName() + "Wrong thread.");
Why this was removed?

https://codereview.webrtc.org/2399463006/diff/40001/webrtc/api/android/java/s...
File webrtc/api/android/java/src/org/webrtc/EglRenderer.java (right):

https://codereview.webrtc.org/2399463006/diff/40001/webrtc/api/android/java/s...
webrtc/api/android/java/src/org/webrtc/EglRenderer.java:20: import
android.opengl.GLES20;
Please move this together with the rest of the android imports.

https://codereview.webrtc.org/2399463006/diff/40001/webrtc/api/android/java/s...
webrtc/api/android/java/src/org/webrtc/EglRenderer.java:32: private Object
surface;
Please change Object to Surface.

https://codereview.webrtc.org/2399463006/diff/40001/webrtc/api/android/java/s...
webrtc/api/android/java/src/org/webrtc/EglRenderer.java:126: throw new
IllegalStateException(name + "Already initialized");
nit: Formatting of this error could be improved.

https://codereview.webrtc.org/2399463006/diff/40001/webrtc/api/android/java/s...
webrtc/api/android/java/src/org/webrtc/EglRenderer.java:419: Logging.d(TAG, name
+ string);
nit: Improve log formatting, i.e. add a space between the name and the string

https://codereview.webrtc.org/2399463006/diff/40001/webrtc/api/android/java/s...
File webrtc/api/android/java/src/org/webrtc/SurfaceViewRenderer.java (right):

https://codereview.webrtc.org/2399463006/diff/40001/webrtc/api/android/java/s...
webrtc/api/android/java/src/org/webrtc/SurfaceViewRenderer.java:210:
Logging.d(TAG, resourceName + string);
nit: Log formatting

[magjed_webrtc, 2016-10-06 13:36:07]

https://codereview.webrtc.org/2399463006/diff/40001/webrtc/api/android/java/s...
File webrtc/api/android/java/src/org/webrtc/EglRenderer.java (left):

https://codereview.webrtc.org/2399463006/diff/40001/webrtc/api/android/java/s...
webrtc/api/android/java/src/org/webrtc/EglRenderer.java:440: throw new
IllegalStateException(getResourceName() + "Wrong thread.");
On 2016/10/06 11:32:51, sakal wrote:
> Why this was removed?

renderThreadHandler can be null when this is executed. Also, it's statically
possible to see that this function is always executed on the right thread.

https://codereview.webrtc.org/2399463006/diff/40001/webrtc/api/android/java/s...
File webrtc/api/android/java/src/org/webrtc/EglRenderer.java (right):

https://codereview.webrtc.org/2399463006/diff/40001/webrtc/api/android/java/s...
webrtc/api/android/java/src/org/webrtc/EglRenderer.java:20: import
android.opengl.GLES20;
On 2016/10/06 11:32:51, sakal wrote:
> Please move this together with the rest of the android imports.

Done.

https://codereview.webrtc.org/2399463006/diff/40001/webrtc/api/android/java/s...
webrtc/api/android/java/src/org/webrtc/EglRenderer.java:32: private Object
surface;
On 2016/10/06 11:32:50, sakal wrote:
> Please change Object to Surface.

Done.

https://codereview.webrtc.org/2399463006/diff/40001/webrtc/api/android/java/s...
webrtc/api/android/java/src/org/webrtc/EglRenderer.java:126: throw new
IllegalStateException(name + "Already initialized");
On 2016/10/06 11:32:51, sakal wrote:
> nit: Formatting of this error could be improved.

|name| already includes ": ", and it might also be empty. It's not obvious, but
it's how it's currently done, and it saves some string concatenation.

https://codereview.webrtc.org/2399463006/diff/40001/webrtc/api/android/java/s...
webrtc/api/android/java/src/org/webrtc/EglRenderer.java:419: Logging.d(TAG, name
+ string);
On 2016/10/06 11:32:51, sakal wrote:
> nit: Improve log formatting, i.e. add a space between the name and the string

ditto, a space and colon is already included, or it's empty.

https://codereview.webrtc.org/2399463006/diff/40001/webrtc/api/android/java/s...
File webrtc/api/android/java/src/org/webrtc/SurfaceViewRenderer.java (right):

https://codereview.webrtc.org/2399463006/diff/40001/webrtc/api/android/java/s...
webrtc/api/android/java/src/org/webrtc/SurfaceViewRenderer.java:210:
Logging.d(TAG, resourceName + string);
On 2016/10/06 11:32:51, sakal wrote:
> nit: Log formatting

ditto

[sakal, 2016-10-07 11:59:15]

lgtm

https://codereview.webrtc.org/2399463006/diff/60001/webrtc/api/BUILD.gn
File webrtc/api/BUILD.gn (right):

https://codereview.webrtc.org/2399463006/diff/60001/webrtc/api/BUILD.gn#newco...
webrtc/api/BUILD.gn:264: "android/java/src/org/webrtc/EglRenderer.java",
Do we need to modify a gyp file?

[magjed_webrtc, 2016-10-07 12:06:21]

https://codereview.webrtc.org/2399463006/diff/60001/webrtc/api/BUILD.gn
File webrtc/api/BUILD.gn (right):

https://codereview.webrtc.org/2399463006/diff/60001/webrtc/api/BUILD.gn#newco...
webrtc/api/BUILD.gn:264: "android/java/src/org/webrtc/EglRenderer.java",
On 2016/10/07 11:59:14, sakal wrote:
> Do we need to modify a gyp file?

I don't think so. No gyp file contains the .java files.

[magjed_webrtc, 2016-10-07 12:06:24]

The CQ bit was checked by magjed@webrtc.org

[commit-bot: I haz the power, 2016-10-07 12:06:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-10-07 12:32:37]

Description was changed from

==========
Android: Split out EGL rendering from SurfaceViewRenderer to separate class

The purpose is to prepare for a TextureViewRenderer that will share the
EGL rendering code.

Two functional changes are also included:
* The implementation of SurfaceHolder.Callback.surfaceDestroyed will now
  block until the EGL surface is released. This is done in order to
  comply with the documentation that says: "If you have a rendering
  thread that directly accesses the surface, you must ensure that thread
  is no longer touching the Surface before returning from this function."
* We will no longer try to hide render glitches during layout changes.
  This was a lost cause anyway.

BUG=webrtc:6407
==========

to

==========
Android: Split out EGL rendering from SurfaceViewRenderer to separate class

The purpose is to prepare for a TextureViewRenderer that will share the
EGL rendering code.

Two functional changes are also included:
* The implementation of SurfaceHolder.Callback.surfaceDestroyed will now
  block until the EGL surface is released. This is done in order to
  comply with the documentation that says: "If you have a rendering
  thread that directly accesses the surface, you must ensure that thread
  is no longer touching the Surface before returning from this function."
* We will no longer try to hide render glitches during layout changes.
  This was a lost cause anyway.

BUG=webrtc:6407
==========

[commit-bot: I haz the power, 2016-10-07 12:32:38]

Committed patchset #2 (id:60001)

[commit-bot: I haz the power, 2016-10-07 12:32:45]

Description was changed from

==========
Android: Split out EGL rendering from SurfaceViewRenderer to separate class

The purpose is to prepare for a TextureViewRenderer that will share the
EGL rendering code.

Two functional changes are also included:
* The implementation of SurfaceHolder.Callback.surfaceDestroyed will now
  block until the EGL surface is released. This is done in order to
  comply with the documentation that says: "If you have a rendering
  thread that directly accesses the surface, you must ensure that thread
  is no longer touching the Surface before returning from this function."
* We will no longer try to hide render glitches during layout changes.
  This was a lost cause anyway.

BUG=webrtc:6407
==========

to

==========
Android: Split out EGL rendering from SurfaceViewRenderer to separate class

The purpose is to prepare for a TextureViewRenderer that will share the
EGL rendering code.

Two functional changes are also included:
* The implementation of SurfaceHolder.Callback.surfaceDestroyed will now
  block until the EGL surface is released. This is done in order to
  comply with the documentation that says: "If you have a rendering
  thread that directly accesses the surface, you must ensure that thread
  is no longer touching the Surface before returning from this function."
* We will no longer try to hide render glitches during layout changes.
  This was a lost cause anyway.

BUG=webrtc:6407

Committed: https://crrev.com/df494b09085986ba980b7128ed7bdd90636a9c08
Cr-Commit-Position: refs/heads/master@{#14570}
==========

[commit-bot: I haz the power, 2016-10-07 12:32:46]

Patchset 2 (id:??) landed as
https://crrev.com/df494b09085986ba980b7128ed7bdd90636a9c08
Cr-Commit-Position: refs/heads/master@{#14570}

[ehmaldonado_webrtc, 2016-10-07 13:21:33]

On 2016/10/07 12:32:46, commit-bot: I haz the power wrote:
> Patchset 2 (id:??) landed as
> https://crrev.com/df494b09085986ba980b7128ed7bdd90636a9c08
> Cr-Commit-Position: refs/heads/master@{#14570}

Are you sure you have the right bug?

[magjed_webrtc, 2016-10-18 08:44:19]

On 2016/10/07 13:21:33, ehmaldonado_webrtc wrote:
> On 2016/10/07 12:32:46, commit-bot: I haz the power wrote:
> > Patchset 2 (id:??) landed as
> > https://crrev.com/df494b09085986ba980b7128ed7bdd90636a9c08
> > Cr-Commit-Position: refs/heads/master@{#14570}
> 
> Are you sure you have the right bug?

Sorry, no should be webrtc:6470 instead of webrtc:6407.

[magjed_webrtc, 2016-10-18 15:25:03]

Description was changed from

==========
Android: Split out EGL rendering from SurfaceViewRenderer to separate class

The purpose is to prepare for a TextureViewRenderer that will share the
EGL rendering code.

Two functional changes are also included:
* The implementation of SurfaceHolder.Callback.surfaceDestroyed will now
  block until the EGL surface is released. This is done in order to
  comply with the documentation that says: "If you have a rendering
  thread that directly accesses the surface, you must ensure that thread
  is no longer touching the Surface before returning from this function."
* We will no longer try to hide render glitches during layout changes.
  This was a lost cause anyway.

BUG=webrtc:6407

Committed: https://crrev.com/df494b09085986ba980b7128ed7bdd90636a9c08
Cr-Commit-Position: refs/heads/master@{#14570}
==========

to

==========
Android: Split out EGL rendering from SurfaceViewRenderer to separate class

The purpose is to prepare for a TextureViewRenderer that will share the
EGL rendering code.

Two functional changes are also included:
* The implementation of SurfaceHolder.Callback.surfaceDestroyed will now
  block until the EGL surface is released. This is done in order to
  comply with the documentation that says: "If you have a rendering
  thread that directly accesses the surface, you must ensure that thread
  is no longer touching the Surface before returning from this function."
* We will no longer try to hide render glitches during layout changes.
  This was a lost cause anyway.

BUG=webrtc:6470

Committed: https://crrev.com/df494b09085986ba980b7128ed7bdd90636a9c08
Cr-Commit-Position: refs/heads/master@{#14570}
==========