Android SurfaceViewRenderer: Block in release() until frames are returned and cleanup is done

Android SurfaceViewRenderer: Block in release() until frames are returned and cleanup is done

BUG=webrtc:4742, webrtc:4909
R=hbos@webrtc.org

Committed: https://crrev.com/c19922c181f0df0b5f4a5c7ffe55f6a5eb2e6ced
Cr-Commit-Position: refs/heads/master@{#10000}

[magjed_webrtc, 2015-09-21 09:02:04]

Patchset #1 (id:1) has been deleted

[magjed_webrtc, 2015-09-21 09:02:42]

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

[magjed_webrtc, 2015-09-21 09:04:56]

magjed@webrtc.org changed reviewers:
- perkj@webrtc.org

[magjed_webrtc, 2015-09-21 09:04:56]

hbos - Please take a look. I have tried to handle the InterruptedException in a
mature way, following the recommendations in e.g.
http://www.ibm.com/developerworks/library/j-jtp05236/. I would appreciate if you
knew any more elegant way to handle it.

[hbos, 2015-09-21 12:06:57]

Considering interruptions are just hints that some thread wants us to quit, but
we really must ignore this and complete the release, I think looping until join
completes successfully is the way to go.

I'm debating with myself whether we want to set the interrupt status or if we
want to make release() throw interrupt exception and re-throw it in the end.
Either way we're ignoring the interrupt and asking the caller to deal with it -
either explicitly through a thrown InterruptException or implicitly with an
isInterrupted flag set...

https://codereview.webrtc.org/1354393002/diff/20001/talk/app/webrtc/java/andr...
File talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java (right):

https://codereview.webrtc.org/1354393002/diff/20001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java:161: *
function, the GL resources might leak.
Add a comment saying it is a blocking operation that will run until resources
have been released even if an interrupt occurs, but that if one occurs then the
interrupt status of the current thread will be set.

https://codereview.webrtc.org/1354393002/diff/20001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java:207:
wasInterrupted = true;
Can we skip |wasInterrupted| and do interrupt() here instead?

[hbos, 2015-09-21 12:08:54]

The comments are assuming we go with the interrupt() approach but I'd like to
discuss the throws InterruptException alternative, let's chat offline.

[hbos, 2015-09-21 12:22:48]

lgtm

https://codereview.webrtc.org/1354393002/diff/20001/talk/app/webrtc/java/andr...
File talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java (right):

https://codereview.webrtc.org/1354393002/diff/20001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java:207:
wasInterrupted = true;
On 2015/09/21 12:06:57, hbos wrote:
> Can we skip |wasInterrupted| and do interrupt() here instead?

Nvm :)

[magjed_webrtc, 2015-09-21 12:28:56]

https://codereview.webrtc.org/1354393002/diff/20001/talk/app/webrtc/java/andr...
File talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java (right):

https://codereview.webrtc.org/1354393002/diff/20001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java:161: *
function, the GL resources might leak.
On 2015/09/21 12:06:57, hbos wrote:
> Add a comment saying it is a blocking operation that will run until resources
> have been released even if an interrupt occurs, but that if one occurs then
the
> interrupt status of the current thread will be set.

Done.

https://codereview.webrtc.org/1354393002/diff/20001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java:207:
wasInterrupted = true;
On 2015/09/21 12:06:57, hbos wrote:
> Can we skip |wasInterrupted| and do interrupt() here instead?

I think we would get stuck in an infinite loop because .join() will throw
immediately if the interrupt flag is set.

[magjed_webrtc, 2015-09-21 12:41:18]

Committed patchset #2 (id:40001) to pending queue manually as
3adbe9c16893223625cb34e3cca3147485baa85c (presubmit successful).

[commit-bot: I haz the power, 2015-09-21 12:51:35]

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