Android VideoRendererGui: Add dispose function

Android VideoRendererGui: Add dispose function

There is currently no way to dispose VideoRendererGui or VideoRendererGui.YuvImageRenderer. This CL adds functions to do so.

BUG=webrtc:4892
Committed: https://crrev.com/d5031fcf928b615a1c9aa9f15e60d1c946e6d456
Cr-Commit-Position: refs/heads/master@{#9710}

[magjed_webrtc, 2015-08-05 14:50:38]

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

[magjed_webrtc, 2015-08-05 14:56:07]

The CQ bit was checked by magjed@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-05 14:56:14]

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

[magjed_webrtc, 2015-08-05 15:20:32]

Please take a look.

[commit-bot: I haz the power, 2015-08-05 17:02:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-05 17:02:54]

Dry run: This issue passed the CQ dry run.

[AlexG, 2015-08-05 18:19:23]

lgtm

https://codereview.webrtc.org/1273803002/diff/1/talk/app/webrtc/java/android/...
File talk/app/webrtc/java/android/org/webrtc/VideoRendererGui.java (right):

https://codereview.webrtc.org/1273803002/diff/1/talk/app/webrtc/java/android/...
talk/app/webrtc/java/android/org/webrtc/VideoRendererGui.java:104: public static
void dispose() {
May be better to declare this method as synchronized (will be synchronized on
class) and remove synchronization on instance, so all instance modifications are
synchronized on class?

https://codereview.webrtc.org/1273803002/diff/1/talk/app/webrtc/java/android/...
talk/app/webrtc/java/android/org/webrtc/VideoRendererGui.java:199: private void
release() {
synchronized?

https://codereview.webrtc.org/1273803002/diff/1/talk/app/webrtc/java/android/...
talk/app/webrtc/java/android/org/webrtc/VideoRendererGui.java:516: public static
void setView(GLSurfaceView surface,
add synchronized?

https://codereview.webrtc.org/1273803002/diff/1/talk/app/webrtc/java/android/...
talk/app/webrtc/java/android/org/webrtc/VideoRendererGui.java:523: public static
EGLContext getEGLContext() {
add synchronized?

https://codereview.webrtc.org/1273803002/diff/1/talk/app/webrtc/java/android/...
talk/app/webrtc/java/android/org/webrtc/VideoRendererGui.java:549: public static
YuvImageRenderer create(int x, int y, int width, int height,
declare this function as synchronized to deal with the case that instance can be
null?

https://codereview.webrtc.org/1273803002/diff/1/talk/app/webrtc/java/android/...
talk/app/webrtc/java/android/org/webrtc/VideoRendererGui.java:592: public static
void update(
ditto: declare this function as synchronized to deal with the case that instance
can be null?

https://codereview.webrtc.org/1273803002/diff/1/talk/app/webrtc/java/android/...
talk/app/webrtc/java/android/org/webrtc/VideoRendererGui.java:609: public static
void remove(VideoRenderer.Callbacks renderer) {
ditto: declare this function as synchronized to deal with the case that instance
can be null?

https://codereview.webrtc.org/1273803002/diff/1/talk/app/webrtc/java/android/...
talk/app/webrtc/java/android/org/webrtc/VideoRendererGui.java:616: if
(!instance.yuvImageRenderers.remove(renderer)) {
Call release() for removed renderer here?

https://codereview.webrtc.org/1273803002/diff/1/talk/app/webrtc/java/src/org/...
File talk/app/webrtc/java/src/org/webrtc/VideoRenderer.java (right):

https://codereview.webrtc.org/1273803002/diff/1/talk/app/webrtc/java/src/org/...
talk/app/webrtc/java/src/org/webrtc/VideoRenderer.java:208: }
set nativeVideoRenderer to 0 and add a check at the beginning of dispose() to
exit if nativeVideoRenderer == 0 - just to be safe if someone will call
dispose() twice?

https://codereview.webrtc.org/1273803002/diff/1/talk/examples/android/src/org...
File talk/examples/android/src/org/appspot/apprtc/CallActivity.java (right):

https://codereview.webrtc.org/1273803002/diff/1/talk/examples/android/src/org...
talk/examples/android/src/org/appspot/apprtc/CallActivity.java:441:
VideoRendererGui.dispose();
nit: Move before setResult() - we first dispose/close everythinh, then setResult
and exit.

[alaershov, 2015-08-06 05:19:45]

On 2015/08/05 15:20:32, magjed_webrtc wrote:
> Please take a look.

Sometimes (pretty often), when the call ends, the app crashes with the following
exception:

08-06 11:13:54.484  19398-21984/org.appspot.apprtc W/System.err﹕
java.lang.NullPointerException: Attempt to invoke virtual method 'void
android.opengl.GLSurfaceView.requestRender()' on a null object reference
08-06 11:13:54.484  19398-21984/org.appspot.apprtc W/System.err﹕ at
org.webrtc.VideoRendererGui$YuvImageRenderer.renderFrame(VideoRendererGui.java:505)
08-06 11:13:54.485  19398-21984/org.appspot.apprtc E/rtc﹕ #
    # Fatal error in ../../talk/app/webrtc/java/jni/peerconnection_jni.cc, line
777
    # Check failed: !jni()->ExceptionCheck()
    #
    #
08-06 11:13:54.485  19398-21984/org.appspot.apprtc A/libc﹕ Fatal signal 6
(SIGABRT), code -6 in tid 21984 (Thread-11183)

[alaershov, 2015-08-06 05:31:47]

On 2015/08/05 15:20:32, magjed_webrtc wrote:
> Please take a look.

And another thing: is it safe to call dispose() while webrtc connection is still
active? In my app the call lives longer than an Activity with GLSurfaceView,
user can go to different screens of an app, and when GLSurfaceView is not shown,
it should be freed. Should I call dispose() on every onDestroy() or something,
or there is another way, for example, just manually remove all the renderers?

[magjed_webrtc, 2015-08-11 10:01:43]

Patchset #2 (id:20001) has been deleted

[magjed_webrtc, 2015-08-11 10:38:16]

Please take another look. I reverted releasing |yuvTextures|, because they are
accessed in many different places, and I'm afraid I will break something. It
looks like they will always be indirectly synchronized on
|instance.yuvImageRenderers|, but that's kind of strange.

https://codereview.webrtc.org/1273803002/diff/1/talk/app/webrtc/java/android/...
File talk/app/webrtc/java/android/org/webrtc/VideoRendererGui.java (right):

https://codereview.webrtc.org/1273803002/diff/1/talk/app/webrtc/java/android/...
talk/app/webrtc/java/android/org/webrtc/VideoRendererGui.java:104: public static
void dispose() {
On 2015/08/05 18:19:23, AlexG wrote:
> May be better to declare this method as synchronized (will be synchronized on
> class) and remove synchronization on instance, so all instance modifications
are
> synchronized on class?

Done.

https://codereview.webrtc.org/1273803002/diff/1/talk/app/webrtc/java/android/...
talk/app/webrtc/java/android/org/webrtc/VideoRendererGui.java:199: private void
release() {
On 2015/08/05 18:19:23, AlexG wrote:
> synchronized?

Done.

https://codereview.webrtc.org/1273803002/diff/1/talk/app/webrtc/java/android/...
talk/app/webrtc/java/android/org/webrtc/VideoRendererGui.java:516: public static
void setView(GLSurfaceView surface,
On 2015/08/05 18:19:23, AlexG wrote:
> add synchronized?

Done.

https://codereview.webrtc.org/1273803002/diff/1/talk/app/webrtc/java/android/...
talk/app/webrtc/java/android/org/webrtc/VideoRendererGui.java:523: public static
EGLContext getEGLContext() {
On 2015/08/05 18:19:23, AlexG wrote:
> add synchronized?

Done.

https://codereview.webrtc.org/1273803002/diff/1/talk/app/webrtc/java/android/...
talk/app/webrtc/java/android/org/webrtc/VideoRendererGui.java:549: public static
YuvImageRenderer create(int x, int y, int width, int height,
On 2015/08/05 18:19:23, AlexG wrote:
> declare this function as synchronized to deal with the case that instance can
be
> null?

Done.

https://codereview.webrtc.org/1273803002/diff/1/talk/app/webrtc/java/android/...
talk/app/webrtc/java/android/org/webrtc/VideoRendererGui.java:592: public static
void update(
On 2015/08/05 18:19:23, AlexG wrote:
> ditto: declare this function as synchronized to deal with the case that
instance
> can be null?

Done.

https://codereview.webrtc.org/1273803002/diff/1/talk/app/webrtc/java/android/...
talk/app/webrtc/java/android/org/webrtc/VideoRendererGui.java:609: public static
void remove(VideoRenderer.Callbacks renderer) {
On 2015/08/05 18:19:23, AlexG wrote:
> ditto: declare this function as synchronized to deal with the case that
instance
> can be null?

Done.

https://codereview.webrtc.org/1273803002/diff/1/talk/app/webrtc/java/android/...
talk/app/webrtc/java/android/org/webrtc/VideoRendererGui.java:616: if
(!instance.yuvImageRenderers.remove(renderer)) {
On 2015/08/05 18:19:23, AlexG wrote:
> Call release() for removed renderer here?

Done.

https://codereview.webrtc.org/1273803002/diff/1/talk/app/webrtc/java/src/org/...
File talk/app/webrtc/java/src/org/webrtc/VideoRenderer.java (right):

https://codereview.webrtc.org/1273803002/diff/1/talk/app/webrtc/java/src/org/...
talk/app/webrtc/java/src/org/webrtc/VideoRenderer.java:208: }
On 2015/08/05 18:19:23, AlexG wrote:
> set nativeVideoRenderer to 0 and add a check at the beginning of dispose() to
> exit if nativeVideoRenderer == 0 - just to be safe if someone will call
> dispose() twice?

Done. I also changed |callbacks| to a boolean instead.

https://codereview.webrtc.org/1273803002/diff/1/talk/examples/android/src/org...
File talk/examples/android/src/org/appspot/apprtc/CallActivity.java (right):

https://codereview.webrtc.org/1273803002/diff/1/talk/examples/android/src/org...
talk/examples/android/src/org/appspot/apprtc/CallActivity.java:441:
VideoRendererGui.dispose();
On 2015/08/05 18:19:23, AlexG wrote:
> nit: Move before setResult() - we first dispose/close everythinh, then
setResult
> and exit.

I moved it to onDestroy() instead. I think that makes sense, because
VideoRendererGui.setView() and VideoRendererGui.create() is called in
onCreate().

[AlexG, 2015-08-13 23:25:00]

lgtm

[magjed_webrtc, 2015-08-14 08:56:50]

The CQ bit was checked by magjed@webrtc.org

[commit-bot: I haz the power, 2015-08-14 08:57:00]

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

[commit-bot: I haz the power, 2015-08-14 10:13:10]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2015-08-14 10:13:18]

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