Android MediaCodecVideoDecoder: Manage lifetime of texture frames

Android MediaCodecVideoDecoder: Manage lifetime of texture frames

This CL should be the last one in a series to finally unblock camera texture capture.

The SurfaceTexture.updateTexImage() calls are moved from the video renderers into MediaCodecVideoDecoder, and the destructor of the texture frames will signal MediaCodecVideoDecoder that the frame has returned. This CL also removes the SurfaceTexture from the native handle and only exposes the texture matrix instead, because only the video source should access the SurfaceTexture.

BUG=webrtc:4993
R=glaznev@webrtc.org, perkj@webrtc.org

Committed: https://chromium.googlesource.com/external/webrtc/+/91b348c7029d843e06868ed12b728a809c53176c

[magjed_webrtc, 2015-10-05 06:48:36]

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

[magjed_webrtc, 2015-10-05 06:49:19]

Please take a look.

[perkj_webrtc, 2015-10-05 07:28:34]

Me like !

lgtm with nits

https://codereview.webrtc.org/1378033003/diff/1/talk/app/webrtc/java/jni/andr...
File talk/app/webrtc/java/jni/androidmediadecoder_jni.cc (right):

https://codereview.webrtc.org/1378033003/diff/1/talk/app/webrtc/java/jni/andr...
talk/app/webrtc/java/jni/androidmediadecoder_jni.cc:487: if
(!DeliverPendingOutputs(jni, kMediaCodecTimeoutMs * 1000)) {
This is already in Ms- so why have it in US in java just to convert back...

https://codereview.webrtc.org/1378033003/diff/1/talk/app/webrtc/java/jni/peer...
File talk/app/webrtc/java/jni/peerconnection_jni.cc (right):

https://codereview.webrtc.org/1378033003/diff/1/talk/app/webrtc/java/jni/peer...
talk/app/webrtc/java/jni/peerconnection_jni.cc:725: class VideoRendererWrapper :
public VideoRendererInterface {
Can we split this out in  a separate file in a follow up cl?

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

https://codereview.webrtc.org/1378033003/diff/1/talk/app/webrtc/java/src/org/...
talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:102: private
static class TextureListener
move to below DecodedTextureBuffer  ?

https://codereview.webrtc.org/1378033003/diff/1/talk/app/webrtc/java/src/org/...
talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:124: public
synchronized DecodedTextureBuffer dequeueTextureFrame(int timeoutUs) {
timesoutMs - why timeoutUs? Fix in jni class?

[magjed_webrtc, 2015-10-05 09:49:55]

https://codereview.webrtc.org/1378033003/diff/1/talk/app/webrtc/java/jni/andr...
File talk/app/webrtc/java/jni/androidmediadecoder_jni.cc (right):

https://codereview.webrtc.org/1378033003/diff/1/talk/app/webrtc/java/jni/andr...
talk/app/webrtc/java/jni/androidmediadecoder_jni.cc:487: if
(!DeliverPendingOutputs(jni, kMediaCodecTimeoutMs * 1000)) {
On 2015/10/05 07:28:34, perkj1 wrote:
> This is already in Ms- so why have it in US in java just to convert back...

Done, I changed it. I think the reason for US is that the timeout argument to
MediaCodec.dequeueOutputBuffer() is in microseconds.

https://codereview.webrtc.org/1378033003/diff/1/talk/app/webrtc/java/jni/peer...
File talk/app/webrtc/java/jni/peerconnection_jni.cc (right):

https://codereview.webrtc.org/1378033003/diff/1/talk/app/webrtc/java/jni/peer...
talk/app/webrtc/java/jni/peerconnection_jni.cc:725: class VideoRendererWrapper :
public VideoRendererInterface {
On 2015/10/05 07:28:34, perkj1 wrote:
> Can we split this out in  a separate file in a follow up cl? 

Yes.

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

https://codereview.webrtc.org/1378033003/diff/1/talk/app/webrtc/java/src/org/...
talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:102: private
static class TextureListener
On 2015/10/05 07:28:34, perkj1 wrote:
> move to below DecodedTextureBuffer  ? 

Done.

https://codereview.webrtc.org/1378033003/diff/1/talk/app/webrtc/java/src/org/...
talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:124: public
synchronized DecodedTextureBuffer dequeueTextureFrame(int timeoutUs) {
On 2015/10/05 07:28:34, perkj1 wrote:
> timesoutMs - why timeoutUs? Fix in jni class? 

Done.

[magjed_webrtc, 2015-10-06 17:55:42]

Alex - ping.

[AlexG, 2015-10-06 23:27:35]

https://codereview.webrtc.org/1378033003/diff/20001/talk/app/webrtc/java/src/...
File talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java (right):

https://codereview.webrtc.org/1378033003/diff/20001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:333: throw new
IllegalStateException("Already holding a texture.");
Print error message in the log

https://codereview.webrtc.org/1378033003/diff/20001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:343:
wait(timeoutMs);
I think it will be cleaner to add separate Object field for wait() and
notifyAll() rather then using "this"

https://codereview.webrtc.org/1378033003/diff/20001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:345:
Thread.currentThread().interrupt();
This is bad fallback - we need to exit gracefully for this case and fallback to
SW decoder. Throw an exception here which will be caught by native code?

https://codereview.webrtc.org/1378033003/diff/20001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:355:
synchronized (this) {
Declare release() as synchronized for parity with other class methods?

[magjed_webrtc, 2015-10-07 07:50:57]

https://codereview.webrtc.org/1378033003/diff/20001/talk/app/webrtc/java/src/...
File talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java (right):

https://codereview.webrtc.org/1378033003/diff/20001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:333: throw new
IllegalStateException("Already holding a texture.");
On 2015/10/06 23:27:35, AlexG wrote:
> Print error message in the log

Done.

https://codereview.webrtc.org/1378033003/diff/20001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:343:
wait(timeoutMs);
On 2015/10/06 23:27:35, AlexG wrote:
> I think it will be cleaner to add separate Object field for wait() and
> notifyAll() rather then using "this"

Done.

https://codereview.webrtc.org/1378033003/diff/20001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:345:
Thread.currentThread().interrupt();
On 2015/10/06 23:27:35, AlexG wrote:
> This is bad fallback - we need to exit gracefully for this case and fallback
to
> SW decoder. Throw an exception here which will be caught by native code?

I think reinterrupting the thread to restore the interrupt status is an ok way
to handle this. I have tried to follow the recommendations in
http://www.ibm.com/developerworks/library/j-jtp05236/. If I reinterrupt, it will
be like wait() was never called and code higher up has chance to respond to it.
Calling Thread.interrupt() is not fatal and is just a way to ask the thread
nicely to unblock and return early.

I don't think it really makes sense to convert InterruptedException to a fatal
exception and propagate it through dequeueOutputBuffer() -> JNI
DeliverPendingOutputs() and fallback to SW. MediaCodec.dequeueOutputBuffer()
does not throw InterruptedException so it will not behave in the same way.

In practice, Thread.interrupt() will never be called (in the current code)
because the thread is created in C++ MediaCodecVideoDecoder and no one else has
a reference to the thread.

https://codereview.webrtc.org/1378033003/diff/20001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:355:
synchronized (this) {
On 2015/10/06 23:27:35, AlexG wrote:
> Declare release() as synchronized for parity with other class methods?

There is a risk for deadlock if we call disconnect() while holding the lock
since disconnect() is also a blocking method. disconnect() will wait until any
onTextureFrameAvailable() in progress is done. There is a risk
|surfaceTextureHelper| is just about to call onTextureFrameAvailable() while we
call disconnect(), and if we hold the lock then it will deadlock.

[AlexG, 2015-10-07 17:04:30]

lgtm

https://codereview.webrtc.org/1378033003/diff/20001/talk/app/webrtc/java/src/...
File talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java (right):

https://codereview.webrtc.org/1378033003/diff/20001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:355:
synchronized (this) {
On 2015/10/07 07:50:57, magjed_webrtc wrote:
> On 2015/10/06 23:27:35, AlexG wrote:
> > Declare release() as synchronized for parity with other class methods?
> 
> There is a risk for deadlock if we call disconnect() while holding the lock
> since disconnect() is also a blocking method. disconnect() will wait until any
> onTextureFrameAvailable() in progress is done. There is a risk
> |surfaceTextureHelper| is just about to call onTextureFrameAvailable() while
we
> call disconnect(), and if we hold the lock then it will deadlock.

Add comment on it?

[magjed_webrtc, 2015-10-07 20:56:56]

https://codereview.webrtc.org/1378033003/diff/20001/talk/app/webrtc/java/src/...
File talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java (right):

https://codereview.webrtc.org/1378033003/diff/20001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:355:
synchronized (this) {
On 2015/10/07 17:04:30, AlexG wrote:
> On 2015/10/07 07:50:57, magjed_webrtc wrote:
> > On 2015/10/06 23:27:35, AlexG wrote:
> > > Declare release() as synchronized for parity with other class methods?
> > 
> > There is a risk for deadlock if we call disconnect() while holding the lock
> > since disconnect() is also a blocking method. disconnect() will wait until
any
> > onTextureFrameAvailable() in progress is done. There is a risk
> > |surfaceTextureHelper| is just about to call onTextureFrameAvailable() while
> we
> > call disconnect(), and if we hold the lock then it will deadlock.
> 
> Add comment on it?

Done.

[commit-bot: I haz the power, 2015-10-07 20:57:23]

Patchset 4 (id:??) landed as
https://crrev.com/91b348c7029d843e06868ed12b728a809c53176c
Cr-Commit-Position: refs/heads/master@{#10203}

[magjed_webrtc, 2015-10-07 20:57:47]

Committed patchset #4 (id:60001) manually as
91b348c7029d843e06868ed12b728a809c53176c (presubmit successful).