Android SurfaceViewRenderer: Allow to re-init after release() has been called

Android SurfaceViewRenderer: Allow to re-init after release() has been called

This CL makes a thorough reset of all variables in release() and clears the last rendered image so that the SurfaceViewRenderer object can be reinitialized with init() and work properly. This CL also removes an implicit assumption that init() is called before surfaceCreated() - now they can be called in any order.

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

Committed: https://crrev.com/4382d800d225a3d6ce93011ff9a573e4ff613f35
Cr-Commit-Position: refs/heads/master@{#10217}

[magjed_webrtc, 2015-10-07 21:35:00]

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

[magjed_webrtc, 2015-10-07 21:36:36]

hbos - Please take a look.

[hbos, 2015-10-08 07:54:36]

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

https://codereview.webrtc.org/1389203003/diff/1/talk/app/webrtc/java/android/...
talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java:148: public
void init(
You may call init() while it is already initialized and it does things before it
does the is initialized check. This would surely leak resources (rendererThread,
drawer, eglBase)?
Should init() do release()?

Throwing an exception does not guarantee that the program will crash (it can
easily be caught) so I think the resource leak matters even when throwing
IllegalStateException.

https://codereview.webrtc.org/1389203003/diff/1/talk/app/webrtc/java/android/...
talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java:155: eglBase =
new EglBase(sharedContext, EglBase.ConfigType.PLAIN);
What about threading swag? These variables are set, and used elsewhere, without
locks.

[magjed_webrtc, 2015-10-08 09:54:27]

Patchset #2 (id:20001) has been deleted

[magjed_webrtc, 2015-10-08 09:56:08]

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

https://codereview.webrtc.org/1389203003/diff/1/talk/app/webrtc/java/android/...
talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java:148: public
void init(
On 2015/10/08 07:54:36, hbos wrote:
> You may call init() while it is already initialized and it does things before
it
> does the is initialized check. This would surely leak resources
(rendererThread,
> drawer, eglBase)?
> Should init() do release()?
> 
> Throwing an exception does not guarantee that the program will crash (it can
> easily be caught) so I think the resource leak matters even when throwing
> IllegalStateException.

Done, I moved the check back to the top. I was thinking it is ok to leak
resources if I throw a fatal exception, but it's better to check first. Calling
release() here would also fix it, but I don't want to encourage sloppy use, so
better to throw I think.

https://codereview.webrtc.org/1389203003/diff/1/talk/app/webrtc/java/android/...
talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java:155: eglBase =
new EglBase(sharedContext, EglBase.ConfigType.PLAIN);
On 2015/10/08 07:54:36, hbos wrote:
> What about threading swag? These variables are set, and used elsewhere,
without
> locks.

They are only accessed from |renderThread|, which was created above, so it
should be safe.

[hbos, 2015-10-08 11:19:35]

lgtm

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

https://codereview.webrtc.org/1389203003/diff/1/talk/app/webrtc/java/android/...
talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java:148: public
void init(
On 2015/10/08 09:56:08, magjed_webrtc wrote:
> On 2015/10/08 07:54:36, hbos wrote:
> > You may call init() while it is already initialized and it does things
before
> it
> > does the is initialized check. This would surely leak resources
> (rendererThread,
> > drawer, eglBase)?
> > Should init() do release()?
> > 
> > Throwing an exception does not guarantee that the program will crash (it can
> > easily be caught) so I think the resource leak matters even when throwing
> > IllegalStateException.
> 
> Done, I moved the check back to the top. I was thinking it is ok to leak
> resources if I throw a fatal exception, but it's better to check first.
Calling
> release() here would also fix it, but I don't want to encourage sloppy use, so
> better to throw I think.

You probably shouldn't consider exceptions in java fatal though since they can
be caught, IMO. Unless you have the infamous ChuckNorrisException at your
disposal
(http://stackoverflow.com/questions/13883166/uncatchable-chucknorrisexception).

https://codereview.webrtc.org/1389203003/diff/1/talk/app/webrtc/java/android/...
talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java:155: eglBase =
new EglBase(sharedContext, EglBase.ConfigType.PLAIN);
On 2015/10/08 09:56:08, magjed_webrtc wrote:
> On 2015/10/08 07:54:36, hbos wrote:
> > What about threading swag? These variables are set, and used elsewhere,
> without
> > locks.
> 
> They are only accessed from |renderThread|, which was created above, so it
> should be safe.

Acknowledged.

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

https://codereview.webrtc.org/1389203003/diff/40001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java:436:
RendererCommon.rotateTextureMatrix(frame.samplingMatrix, frame.rotationDegree);
Is this diff here a merge diff or cleanup? Woah only shows up in diff-with-ps-1
not "show" what is this sorcery.

[magjed_webrtc, 2015-10-08 12:44:43]

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

https://codereview.webrtc.org/1389203003/diff/40001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java:436:
RendererCommon.rotateTextureMatrix(frame.samplingMatrix, frame.rotationDegree);
On 2015/10/08 11:19:35, hbos wrote:
> Is this diff here a merge diff or cleanup? Woah only shows up in
diff-with-ps-1
> not "show" what is this sorcery.

You see unrelated rebase stuff when you diff patches for some reason.

[magjed_webrtc, 2015-10-08 12:45:48]

Committed patchset #2 (id:40001) manually as
4382d800d225a3d6ce93011ff9a573e4ff613f35 (presubmit successful).

[commit-bot: I haz the power, 2015-10-08 12:45:52]

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