Reland again Android MediaCodecVideoDecoder: Manage lifetime of texture frames

Patchset 1 yet again relands without modification https://codereview.webrtc.org/1422963003/
It do the following:

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.
It moves the responsibility of calculating the decode time to Java.

Patchset2 Refactor MediaCodecVideoDecoder to drop frames if a texture is not released.

R=magjed@webrtc.org

Committed: https://crrev.com/488e75f11b840dfbe636a9ea9bbc18252e7c59f0
Cr-Commit-Position: refs/heads/master@{#10706}

[perkj_webrtc, 2015-11-13 16:40:41]

Description was changed from

==========
Revert "Revert of Android MediaCodecVideoDecoder: Manage lifetime of texture
frames (patchset #12 id:320001 of https://codereview.webrtc.org/1422963003/ )"

This reverts commit c01c25434ba92f6ea32cdfdcde77ec8278182851.

BUG=
==========

to

==========
Patchset 1 yet again relands without modification
https://codereview.webrtc.org/1422963003/
It do the following:

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.
It moves the responsibility of calculating the decode time to Java.

Patchset2 Refactor MediaCodecVideoDecoder to drop frames if a texture is not
released.
==========

[perkj_webrtc, 2015-11-13 16:44:19]

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

[perkj_webrtc, 2015-11-13 16:44:20]

Please?

[magjed_webrtc, 2015-11-16 12:56:48]

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

https://codereview.webrtc.org/1440343002/diff/20001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:463: 
You can set |isWaitingForTexture| to false here instead of in
dequeueTextureBuffer(). Actually, I would prefer if you set |bufferToRender| to
null here, remove |isWaitingForTexture|, and in isWaitingForTexture() you return
'bufferToRender != null'.

https://codereview.webrtc.org/1440343002/diff/20001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:582: if
(!textureListener.isWaitingForTexture()) {
I would prefer if you add !dequeuedSurfaceOutputBuffers.isEmpty() to the
if-statement and split this out in a helper function that you call here and
below.

https://codereview.webrtc.org/1440343002/diff/20001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:604:
MAX_QUEUED_OUTPUTBUFFERS, outputBuffers.length)) || dequeueTimeoutMs > 0) {
I think for the case 'dequeueTimeoutMs > 0', |dequeuedSurfaceOutputBuffers| can
be empty here? Then it will crash on dequeuedSurfaceOutputBuffers.remove()
below.
Also, it's not obvious that the decoder will start to drop frames if you pass a
non-zero timeout value. Please comment this behaviour in the
dequeueTextureBuffer() comment.

https://codereview.webrtc.org/1440343002/diff/20001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:615: //
mediaCodec.releaseOutputBuffer has already been called. Dropping the this frame
will lead
I think you should remove the timestamp queue in C++ and instead send the
timestamps to queueInputBuffer() and store them in DecodedOutputBuffer to keep
the frame information consistent.
Also, as a todo in the future, some of the stats in C++ and Java are redundant,
e.g. dequeuedSurfaceOutputBuffers.size() is the same as C++ (frames_received_ -
frames_decoded_) which is used to trigger the decoder draining. If you moved the
remaining stats to Java (I think it is: frames_decoded_,
current_decoding_time_ms_, and current_bytes_) you wouldn't need to return
dropped dummy texture frames to keep the state in sync. I think mixing the state
here and in C++ is error prone.

[perkj_webrtc, 2015-11-16 16:00:09]

Still working on magjeds comments.
Also, we can not wait on each frame. After starting dropping, we should drop
everything until the texture has been returned.

[AlexG, 2015-11-17 00:47:35]

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

https://codereview.webrtc.org/1440343002/diff/20001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:573:
DecodedOutputBuffer outputBuffer = dequeueOutputBuffer(dequeueTimeoutMs);
If dequeueTimeoutMs > 0 (in case decoder is draining its output) and decoder has
already decoded all its input and is waiting for texture callback there is no
need in this call - it may just result in non necessary 1000 ms wait for output
buffer which decoder is not supposed to generate

https://codereview.webrtc.org/1440343002/diff/20001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:606: if
(dequeueTimeoutMs > 0) {
Have you checked that you can still receive onTextureFrameAvailable callbacks
when phone is locked? If these callbacks are not invoked will it result in 3
second decoder pause while it tries to get 3 texture output before it will start
dropping the frames?

https://codereview.webrtc.org/1440343002/diff/20001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:615: //
mediaCodec.releaseOutputBuffer has already been called. Dropping the this frame
will lead
On 2015/11/16 12:56:47, magjed_webrtc wrote:
> I think you should remove the timestamp queue in C++ and instead send the
> timestamps to queueInputBuffer() and store them in DecodedOutputBuffer to keep
> the frame information consistent.
> Also, as a todo in the future, some of the stats in C++ and Java are
redundant,
> e.g. dequeuedSurfaceOutputBuffers.size() is the same as C++ (frames_received_
-
> frames_decoded_) which is used to trigger the decoder draining. If you moved
the
> remaining stats to Java (I think it is: frames_decoded_,
> current_decoding_time_ms_, and current_bytes_) you wouldn't need to return
> dropped dummy texture frames to keep the state in sync. I think mixing the
state
> here and in C++ is error prone.

"frames_received_ - frames_decoded_" are used by C++ to toggle decoder draining
- this decision should be moved to Java level as well? I think this may worth a
separate follow up CL - this one is already too big change :)

[perkj_webrtc, 2015-11-17 11:02:19]

PTAL

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

https://codereview.webrtc.org/1440343002/diff/20001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:573:
DecodedOutputBuffer outputBuffer = dequeueOutputBuffer(dequeueTimeoutMs);
On 2015/11/17 00:47:35, AlexG wrote:
> If dequeueTimeoutMs > 0 (in case decoder is draining its output) and decoder
has
> already decoded all its input and is waiting for texture callback there is no
> need in this call - it may just result in non necessary 1000 ms wait for
output
> buffer which decoder is not supposed to generate

That is what I also discovered. And that in turns lead to that the jitter buffer
flash and the decoder is reset. Not so good. Instead I changed the draining code
in c++ to make sure we call this method more frequent but with shorter timouts.

https://codereview.webrtc.org/1440343002/diff/20001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:582: if
(!textureListener.isWaitingForTexture()) {
On 2015/11/16 12:56:47, magjed_webrtc wrote:
> I would prefer if you add !dequeuedSurfaceOutputBuffers.isEmpty() to the
> if-statement and split this out in a helper function that you call here and
> below.

Done.

https://codereview.webrtc.org/1440343002/diff/20001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:615: //
mediaCodec.releaseOutputBuffer has already been called. Dropping the this frame
will lead
On 2015/11/17 00:47:35, AlexG wrote:
> On 2015/11/16 12:56:47, magjed_webrtc wrote:
> > I think you should remove the timestamp queue in C++ and instead send the
> > timestamps to queueInputBuffer() and store them in DecodedOutputBuffer to
keep
> > the frame information consistent.
> > Also, as a todo in the future, some of the stats in C++ and Java are
> redundant,
> > e.g. dequeuedSurfaceOutputBuffers.size() is the same as C++
(frames_received_
> -
> > frames_decoded_) which is used to trigger the decoder draining. If you moved
> the
> > remaining stats to Java (I think it is: frames_decoded_,
> > current_decoding_time_ms_, and current_bytes_) you wouldn't need to return
> > dropped dummy texture frames to keep the state in sync. I think mixing the
> state
> > here and in C++ is error prone.
> 
> "frames_received_ - frames_decoded_" are used by C++ to toggle decoder
draining
> - this decision should be moved to Java level as well? I think this may worth
a
> separate follow up CL - this one is already too big change :)

Acknowledged.

https://codereview.webrtc.org/1440343002/diff/20001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:615: //
mediaCodec.releaseOutputBuffer has already been called. Dropping the this frame
will lead
On 2015/11/16 12:56:47, magjed_webrtc wrote:
> I think you should remove the timestamp queue in C++ and instead send the
> timestamps to queueInputBuffer() and store them in DecodedOutputBuffer to keep
> the frame information consistent.
> Also, as a todo in the future, some of the stats in C++ and Java are
redundant,
> e.g. dequeuedSurfaceOutputBuffers.size() is the same as C++ (frames_received_
-
> frames_decoded_) which is used to trigger the decoder draining. If you moved
the
> remaining stats to Java (I think it is: frames_decoded_,
> current_decoding_time_ms_, and current_bytes_) you wouldn't need to return
> dropped dummy texture frames to keep the state in sync. I think mixing the
state
> here and in C++ is error prone.

Done.

[magjed_webrtc, 2015-11-17 11:58:19]

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

https://codereview.webrtc.org/1440343002/diff/20001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:426: //
|isWaitingForTexture| is true when waiting for the transition:
Keep comment that bufferToRender is non-null when waiting between
addBufferToRender() to onTextureFrameAvailable().

https://codereview.webrtc.org/1440343002/diff/60001/talk/app/webrtc/java/jni/...
File talk/app/webrtc/java/jni/androidmediadecoder_jni.cc (right):

https://codereview.webrtc.org/1440343002/diff/60001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/androidmediadecoder_jni.cc:719:
decoded_frame.set_timestamp(output_timestamps_ms);
You can set this as the second argument in the ctor above. I don't know why the
ntp time is not included in the ctor.

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

https://codereview.webrtc.org/1440343002/diff/80001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:363: timeStampMs
,ntpTimeStamp));
incorrect formatting

https://codereview.webrtc.org/1440343002/diff/80001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:379: private
final long decodeStartTimeMs; // Time when this frame was
incomplete comment

https://codereview.webrtc.org/1440343002/diff/80001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:458: return
bufferToRender != null;
You need synchronized (newFrameLock) here.

https://codereview.webrtc.org/1440343002/diff/80001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:475: 
Set bufferToRender to null here.

https://codereview.webrtc.org/1440343002/diff/80001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:482: if
(!isWaitingForTexture()) {
Remove this and add it to the if-statement below instead:
if (renderedBuffer == null && timeoutMs > 0 && isWaitingForTexture())

https://codereview.webrtc.org/1440343002/diff/80001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:497:
bufferToRender = null;
You should set this to null in onTextureFrameAvailable() instead.

[perkj_webrtc, 2015-11-17 12:44:20]

Looks pretty good now right? PTAL

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

https://codereview.webrtc.org/1440343002/diff/20001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:426: //
|isWaitingForTexture| is true when waiting for the transition:
On 2015/11/17 11:58:19, magjed_webrtc wrote:
> Keep comment that bufferToRender is non-null when waiting between
> addBufferToRender() to onTextureFrameAvailable().

Done.

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

https://codereview.webrtc.org/1440343002/diff/80001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:363: timeStampMs
,ntpTimeStamp));
On 2015/11/17 11:58:19, magjed_webrtc wrote:
> incorrect formatting

Done.

https://codereview.webrtc.org/1440343002/diff/80001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:379: private
final long decodeStartTimeMs; // Time when this frame was
On 2015/11/17 11:58:19, magjed_webrtc wrote:
> incomplete comment

Done.

https://codereview.webrtc.org/1440343002/diff/80001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:458: return
bufferToRender != null;
On 2015/11/17 11:58:19, magjed_webrtc wrote:
> You need synchronized (newFrameLock) here.

Done.

https://codereview.webrtc.org/1440343002/diff/80001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:475: 
On 2015/11/17 11:58:19, magjed_webrtc wrote:
> Set bufferToRender to null here.

Done.

https://codereview.webrtc.org/1440343002/diff/80001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:482: if
(!isWaitingForTexture()) {
On 2015/11/17 11:58:19, magjed_webrtc wrote:
> Remove this and add it to the if-statement below instead:
> if (renderedBuffer == null && timeoutMs > 0 && isWaitingForTexture())

Done.

https://codereview.webrtc.org/1440343002/diff/80001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:497:
bufferToRender = null;
On 2015/11/17 11:58:19, magjed_webrtc wrote:
> You should set this to null in onTextureFrameAvailable() instead.

Done.

[magjed_webrtc, 2015-11-17 12:53:12]

lgtm

[perkj_webrtc, 2015-11-19 06:59:42]

The CQ bit was checked by perkj@webrtc.org

[commit-bot: I haz the power, 2015-11-19 06:59:51]

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

[commit-bot: I haz the power, 2015-11-19 07:31:03]

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

[commit-bot: I haz the power, 2015-11-19 07:31:04]

Try jobs failed on following builders:
  android_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/9128)

[perkj_webrtc, 2015-11-19 09:10:30]

The CQ bit was checked by perkj@webrtc.org

[commit-bot: I haz the power, 2015-11-19 09:10:38]

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

[commit-bot: I haz the power, 2015-11-19 09:17:34]

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

[commit-bot: I haz the power, 2015-11-19 09:17:35]

Try jobs failed on following builders:
  android_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/9134)

[perkj_webrtc, 2015-11-19 09:43:51]

Committed patchset #7 (id:120001) manually as
488e75f11b840dfbe636a9ea9bbc18252e7c59f0 (presubmit successful).

[commit-bot: I haz the power, 2015-11-19 09:43:59]

Patchset 7 (id:??) landed as
https://crrev.com/488e75f11b840dfbe636a9ea9bbc18252e7c59f0
Cr-Commit-Position: refs/heads/master@{#10706}