Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1422963003:
Android MediaCodecVideoDecoder: Manage lifetime of texture frames (Closed)
|
Created:
5 years, 1 month ago by perkj_webrtc Modified:
5 years, 1 month ago CC:
webrtc-reviews_webrtc.org, yujie_mao (webrtc), Andrew MacDonald, tterriberry_mozilla.com, qiang.lu, niklas.enbom, peah-webrtc Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionPatchset 1 is a pure
revert of "Revert of "Android MediaCodecVideoDecoder: Manage lifetime of texture frames" https://codereview.webrtc.org/1378033003/
Following patchsets move the responsibility of calculating the decode time to Java.
TESTED= Apprtc loopback using H264 and VP8 on N5, N6, N7, S5
Committed: https://crrev.com/9cb8982e64f08d3d630bf7c3d2bcc78c10db88e2
Cr-Commit-Position: refs/heads/master@{#10597}
Patch Set 1 : Pure revert of the revert. #Patch Set 2 : #
Total comments: 29
Patch Set 3 : Addressed comments and further refactored. #
Total comments: 15
Patch Set 4 : Rebased #Patch Set 5 : Fix #
Total comments: 29
Patch Set 6 : Addressed comments. #Patch Set 7 : Added frame dropping. #
Total comments: 6
Patch Set 8 : Addressed comments. #
Total comments: 18
Patch Set 9 : rebased #Patch Set 10 : Addressed comments. #
Total comments: 2
Patch Set 11 : Moved FrameDelay to Java. #
Total comments: 4
Patch Set 12 : Addressed magjeds comments. #Messages
Total messages: 34 (12 generated)
Patchset #2 (id:20001) has been deleted
Description was changed from ========== Revert "Revert of "Android MediaCodecVideoDecoder: Manage lifetime of texture frames" https://codereview.webrtc.org/1378033003/" This reverts commit 543b6ca30a43eeb069c699291460ce6bacc7959d. BUG= ========== to ========== Patchset 1 is a pure revert of "Revert of "Android MediaCodecVideoDecoder: Manage lifetime of texture frames" https://codereview.webrtc.org/1378033003/" Following patchset move the responsibility of calculating the decode time to Java. BUG= ==========
perkj@webrtc.org changed reviewers: + magjed@webrtc.org
Hi, can you take a look and we can discuss the idea? https://codereview.webrtc.org/1422963003/diff/40001/talk/app/webrtc/java/src/... File talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java (right): https://codereview.webrtc.org/1422963003/diff/40001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:334: private long decodeTimeMs; decodeTime will be needed in the byte buffer as well. https://codereview.webrtc.org/1422963003/diff/40001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:348: private long decodeTime; remove, unused. https://codereview.webrtc.org/1422963003/diff/40001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:408: private Object dequeueOutputBuffer(int dequeueTimeoutMs) I think this should be completely separate from the useSurface method. It can then also return the correct type. https://codereview.webrtc.org/1422963003/diff/40001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:413: return dequeueTexture(dequeueTimeoutMs); remove and use dequeueTexture from c++ https://codereview.webrtc.org/1422963003/diff/40001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:467: do { Move all of do {} into a separate method and use it from both dequeueTexture and dequeueOutputBuffer
Description was changed from ========== Patchset 1 is a pure revert of "Revert of "Android MediaCodecVideoDecoder: Manage lifetime of texture frames" https://codereview.webrtc.org/1378033003/" Following patchset move the responsibility of calculating the decode time to Java. BUG= ========== to ========== Patchset 1 is a pure revert of "Revert of "Android MediaCodecVideoDecoder: Manage lifetime of texture frames" https://codereview.webrtc.org/1378033003/ Following patchset move the responsibility of calculating the decode time to Java. BUG= ==========
This approach looks good. https://codereview.webrtc.org/1422963003/diff/40001/talk/app/webrtc/java/jni/... File talk/app/webrtc/java/jni/androidmediadecoder_jni.cc (right): https://codereview.webrtc.org/1422963003/diff/40001/talk/app/webrtc/java/jni/... talk/app/webrtc/java/jni/androidmediadecoder_jni.cc:601: output_timestamps_ms = timestamp_ns / rtc::kNumNanosecsPerMillisec; This is a duplicated line. https://codereview.webrtc.org/1422963003/diff/40001/talk/app/webrtc/java/jni/... talk/app/webrtc/java/jni/androidmediadecoder_jni.cc:696: " DelayTime: %lld", frames_decoded_, width, height, stride, slice_height, I think you should print both latency and decode time. https://codereview.webrtc.org/1422963003/diff/40001/talk/app/webrtc/java/jni/... talk/app/webrtc/java/jni/androidmediadecoder_jni.cc:702: current_decoding_time_ms_ += decode_time_ms; You need to set |decode_time_ms| for ByteBuffer output as well. You can probably use |frame_delayed_ms|. https://codereview.webrtc.org/1422963003/diff/40001/talk/app/webrtc/java/src/... File talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java (right): https://codereview.webrtc.org/1422963003/diff/40001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:104: private final List<Long> decodeStartTime = new ArrayList<Long>(); These Lists should be queues. Also add ms suffix to the variable names. https://codereview.webrtc.org/1422963003/diff/40001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:334: private long decodeTimeMs; On 2015/10/27 20:46:15, perkj1 wrote: > decodeTime will be needed in the byte buffer as well. Or use the decode latency calculated in jni as before. https://codereview.webrtc.org/1422963003/diff/40001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:461: if (decodeStartTime.size() > 0) { s/decodeStartTime.size() > 0/!decodeStartTime.isEmpty()/g https://codereview.webrtc.org/1422963003/diff/40001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:472: outputBuffers = mediaCodec.getOutputBuffers(); You need to update |dequeuedOutputBuffers| and |decodeTime| here as well now. https://codereview.webrtc.org/1422963003/diff/40001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:505: DecodedTextureBuffer textureBuffer = null; You can write the remaining code simpler like this: if (!isWaitingForTextureToFinishRendering && !decodeStartTime.isEmpty()) { mediaCodec.releaseOutputBuffer(dequeuedOutputBuffers.remove(), true); isWaitingForTextureToFinishRendering = true; } final DecodedTextureBuffer textureBuffer = textureListener.dequeueTextureFrame(dequeueTimeoutMs); if (textureBuffer != null) { textureBuffer.decodeTimeMs = decodeTimeMs.remove(); isWaitingForTextureToFinishRendering = false; } return textureBuffer; https://codereview.webrtc.org/1422963003/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/main/source/generic_decoder.cc (right): https://codereview.webrtc.org/1422963003/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/main/source/generic_decoder.cc:68: (decodeTimeInMs != 0) ? frameInfo->decodeStartTimeMs + decodeTimeInMs : I think you should make this lessy hacky and land separately.
PTAL I will do the generic_decoder changes in a separate cl. https://codereview.webrtc.org/1422963003/diff/40001/talk/app/webrtc/java/jni/... File talk/app/webrtc/java/jni/androidmediadecoder_jni.cc (right): https://codereview.webrtc.org/1422963003/diff/40001/talk/app/webrtc/java/jni/... talk/app/webrtc/java/jni/androidmediadecoder_jni.cc:601: output_timestamps_ms = timestamp_ns / rtc::kNumNanosecsPerMillisec; On 2015/10/28 11:57:16, magjed_webrtc wrote: > This is a duplicated line. Done. https://codereview.webrtc.org/1422963003/diff/40001/talk/app/webrtc/java/jni/... talk/app/webrtc/java/jni/androidmediadecoder_jni.cc:696: " DelayTime: %lld", frames_decoded_, width, height, stride, slice_height, On 2015/10/28 11:57:16, magjed_webrtc wrote: > I think you should print both latency and decode time. Done. https://codereview.webrtc.org/1422963003/diff/40001/talk/app/webrtc/java/jni/... talk/app/webrtc/java/jni/androidmediadecoder_jni.cc:702: current_decoding_time_ms_ += decode_time_ms; On 2015/10/28 11:57:16, magjed_webrtc wrote: > You need to set |decode_time_ms| for ByteBuffer output as well. You can probably > use |frame_delayed_ms|. Done. https://codereview.webrtc.org/1422963003/diff/40001/talk/app/webrtc/java/src/... File talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java (right): https://codereview.webrtc.org/1422963003/diff/40001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:104: private final List<Long> decodeStartTime = new ArrayList<Long>(); On 2015/10/28 11:57:16, magjed_webrtc wrote: > These Lists should be queues. Also add ms suffix to the variable names. Done. https://codereview.webrtc.org/1422963003/diff/40001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:334: private long decodeTimeMs; On 2015/10/28 11:57:16, magjed_webrtc wrote: > On 2015/10/27 20:46:15, perkj1 wrote: > > decodeTime will be needed in the byte buffer as well. > > Or use the decode latency calculated in jni as before. Done. https://codereview.webrtc.org/1422963003/diff/40001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:348: private long decodeTime; On 2015/10/27 20:46:15, perkj1 wrote: > remove, unused. Done. https://codereview.webrtc.org/1422963003/diff/40001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:408: private Object dequeueOutputBuffer(int dequeueTimeoutMs) On 2015/10/27 20:46:15, perkj1 wrote: > I think this should be completely separate from the useSurface method. It can > then also return the correct type. Done. https://codereview.webrtc.org/1422963003/diff/40001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:413: return dequeueTexture(dequeueTimeoutMs); On 2015/10/27 20:46:15, perkj1 wrote: > remove and use dequeueTexture from c++ Done. https://codereview.webrtc.org/1422963003/diff/40001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:461: if (decodeStartTime.size() > 0) { On 2015/10/28 11:57:16, magjed_webrtc wrote: > s/decodeStartTime.size() > 0/!decodeStartTime.isEmpty()/g Done. https://codereview.webrtc.org/1422963003/diff/40001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:467: do { On 2015/10/27 20:46:15, perkj1 wrote: > Move all of do {} into a separate method and use it from both dequeueTexture and > dequeueOutputBuffer Done. https://codereview.webrtc.org/1422963003/diff/40001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:472: outputBuffers = mediaCodec.getOutputBuffers(); On 2015/10/28 11:57:16, magjed_webrtc wrote: > You need to update |dequeuedOutputBuffers| and |decodeTime| here as well now. I hope I don't have to. This variable is actually never used, it just follows the pattern documented in the SDK. If a buffer have been dequeued, it should be available until released in mediaCodec.releaseOutputBuffer(bufferIndex, Also, it seems like the decoder is torn down whenever the resolution change. https://codereview.webrtc.org/1422963003/diff/40001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:478: width = format.getInteger(MediaFormat.KEY_WIDTH); width and height need to be stored with the frames since it can change after decoding and before rendering from the output buffers. https://codereview.webrtc.org/1422963003/diff/40001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:505: DecodedTextureBuffer textureBuffer = null; On 2015/10/28 11:57:16, magjed_webrtc wrote: > You can write the remaining code simpler like this: > if (!isWaitingForTextureToFinishRendering && !decodeStartTime.isEmpty()) { > mediaCodec.releaseOutputBuffer(dequeuedOutputBuffers.remove(), true); > isWaitingForTextureToFinishRendering = true; > } > final DecodedTextureBuffer textureBuffer = > textureListener.dequeueTextureFrame(dequeueTimeoutMs); > if (textureBuffer != null) { > textureBuffer.decodeTimeMs = decodeTimeMs.remove(); > isWaitingForTextureToFinishRendering = false; > } > return textureBuffer; Yes, but the idea was that if isWaitingForTextureToFinishRendering == true we should first wait for the frame so we can render the next frame in the same loop so that hopefully, the next time this is called, the frame is ready. Also, we don't want to wait for textureListener.dequeueTextureFrame(dequeueTimeoutMs) if we are not waiting for a frame and there is nothing to render and dequeueTimeoutMs > 0 For some reason, VP8 only allows 1 one frame in the pipe in DecodeOnCodecThread before it tries to drain. https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin... (I will try to increase that in a separate cl, for now, its a good test case.) I need to store the widht and height for each frame so I also added a state and rewrote this much easier to read I think.
https://codereview.webrtc.org/1422963003/diff/40001/talk/app/webrtc/java/src/... File talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java (right): https://codereview.webrtc.org/1422963003/diff/40001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:472: outputBuffers = mediaCodec.getOutputBuffers(); On 2015/10/28 21:12:39, perkj1 wrote: > On 2015/10/28 11:57:16, magjed_webrtc wrote: > > You need to update |dequeuedOutputBuffers| and |decodeTime| here as well now. > > I hope I don't have to. This variable is actually never used, it just follows > the pattern documented in the SDK. > If a buffer have been dequeued, it should be available until released in > mediaCodec.releaseOutputBuffer(bufferIndex, > > Also, it seems like the decoder is torn down whenever the resolution change. If INFO_OUTPUT_BUFFERS_CHANGED should never happen if a buffer has been dequeued, check this with: if (!dequeuedOutputBuffers.isEmpty()) { throw new IllegalStateException(...); } https://codereview.webrtc.org/1422963003/diff/40001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:505: DecodedTextureBuffer textureBuffer = null; On 2015/10/28 21:12:39, perkj1 wrote: > On 2015/10/28 11:57:16, magjed_webrtc wrote: > > You can write the remaining code simpler like this: > > if (!isWaitingForTextureToFinishRendering && !decodeStartTime.isEmpty()) { > > mediaCodec.releaseOutputBuffer(dequeuedOutputBuffers.remove(), true); > > isWaitingForTextureToFinishRendering = true; > > } > > final DecodedTextureBuffer textureBuffer = > > textureListener.dequeueTextureFrame(dequeueTimeoutMs); > > if (textureBuffer != null) { > > textureBuffer.decodeTimeMs = decodeTimeMs.remove(); > > isWaitingForTextureToFinishRendering = false; > > } > > return textureBuffer; > > Yes, but the idea was that if isWaitingForTextureToFinishRendering == true we > should first wait for the frame so we can render the next frame in the same loop > so that hopefully, the next time this is called, the frame is ready. > Also, we don't want to wait for > textureListener.dequeueTextureFrame(dequeueTimeoutMs) if we are not waiting for > a frame and there is nothing to render and dequeueTimeoutMs > 0 > > For some reason, VP8 only allows 1 one frame in the pipe in DecodeOnCodecThread > before it tries to drain. > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin... > > (I will try to increase that in a separate cl, for now, its a good test case.) > > I need to store the widht and height for each frame so I also added a state and > rewrote this much easier to read I think. I think you should trust the C++ class to poll frequent enough. If you want to optimize the pipeline here, you can change the order: final DecodedTextureBuffer textureBuffer = isWaitingForTextureToFinishRendering ? textureListener.dequeueTextureFrame(dequeueTimeoutMs) : null; if (textureBuffer != null) { textureBuffer.decodeTimeMs = decodeTimeMs.remove(); isWaitingForTextureToFinishRendering = false; } if (!isWaitingForTextureToFinishRendering && !decodeStartTime.isEmpty()) { mediaCodec.releaseOutputBuffer(dequeuedOutputBuffers.remove(), true); isWaitingForTextureToFinishRendering = true; } https://codereview.webrtc.org/1422963003/diff/60001/talk/app/webrtc/java/jni/... File talk/app/webrtc/java/jni/androidmediadecoder_jni.cc (right): https://codereview.webrtc.org/1422963003/diff/60001/talk/app/webrtc/java/jni/... talk/app/webrtc/java/jni/androidmediadecoder_jni.cc:206: jni, *j_media_codec_video_decoder_class_, "dequeueTextureBuffer", this is a 5 space indent, remove one space https://codereview.webrtc.org/1422963003/diff/60001/talk/app/webrtc/java/jni/... talk/app/webrtc/java/jni/androidmediadecoder_jni.cc:573: jobject j_decoder_output_buffer = You can write this like: jobject j_decoder_output_buffer = jni->CallObjectMethod(*j_media_codec_video_decoder_, use_surface_ ? j_dequeue_texture_buffer_method_ : j_dequeue_byte_buffer_method_, dequeue_timeout_ms); https://codereview.webrtc.org/1422963003/diff/60001/talk/app/webrtc/java/jni/... talk/app/webrtc/java/jni/androidmediadecoder_jni.cc:718: ALOGV("Decoder frame out # %d. %d x %d. %d x %d. Color: 0x%x. TS: %ld." |slice_height| etc is not defined here anymore. Maybe move this log inside the big if-statement surface vs non-surface above. |slice_height|, |stride| and |color_format| is not interesting to print for surface decoding anyway. https://codereview.webrtc.org/1422963003/diff/60001/talk/app/webrtc/java/src/... File talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java (right): https://codereview.webrtc.org/1422963003/diff/60001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:327: private static class DecodedTextureBuffer { I think this class has become bloated with too much state and with mutable variables that you have to set outside the constructor. I think you should just add final |width|, |height|, and |decodeTimeMs| to this class and nothing more. https://codereview.webrtc.org/1422963003/diff/60001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:336: public enum State { I don't think this State makes sense, because it's not a per-buffer state, it's a per-MediaCodec state. Either the MediaCodec is currently rendering to the single Surface output or else it's not. I prefer the old code with a single boolean flag to represent this. https://codereview.webrtc.org/1422963003/diff/60001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:450: private DecodedTextureBuffer dequeueTextureBuffer(int dequeueTimeoutMs) { This function has become too bloated and complicated. https://codereview.webrtc.org/1422963003/diff/60001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:502: // Release a dequeued output byte buffer back to the codec for re-use. Should only be called for Move this back to the old place so it's easier to review https://codereview.webrtc.org/1422963003/diff/60001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:556: return result; I think you should add a |decodedTimeMs| to DecodedByteBuffer and revert to returning a DecodedByteBuffer here. Then you can set the decode time here for both surface/non-surface output: return new DecodedByteBuffer(result, info.offset, info.size, info.presentationTimeUs, SystemClock.elapsedRealtime() - decodeStartTimeMs.remove()); Then you can also read the decode time in C++ for both surface/non-surface more consistently.
Patchset #5 (id:100001) has been deleted
PTAL https://codereview.webrtc.org/1422963003/diff/60001/talk/app/webrtc/java/jni/... File talk/app/webrtc/java/jni/androidmediadecoder_jni.cc (right): https://codereview.webrtc.org/1422963003/diff/60001/talk/app/webrtc/java/jni/... talk/app/webrtc/java/jni/androidmediadecoder_jni.cc:206: jni, *j_media_codec_video_decoder_class_, "dequeueTextureBuffer", On 2015/10/29 09:44:13, magjed_webrtc wrote: > this is a 5 space indent, remove one space Done. https://codereview.webrtc.org/1422963003/diff/60001/talk/app/webrtc/java/jni/... talk/app/webrtc/java/jni/androidmediadecoder_jni.cc:573: jobject j_decoder_output_buffer = On 2015/10/29 09:44:12, magjed_webrtc wrote: > You can write this like: > jobject j_decoder_output_buffer = > jni->CallObjectMethod(*j_media_codec_video_decoder_, > use_surface_ ? j_dequeue_texture_buffer_method_ > : j_dequeue_byte_buffer_method_, > dequeue_timeout_ms); Done. https://codereview.webrtc.org/1422963003/diff/60001/talk/app/webrtc/java/jni/... talk/app/webrtc/java/jni/androidmediadecoder_jni.cc:718: ALOGV("Decoder frame out # %d. %d x %d. %d x %d. Color: 0x%x. TS: %ld." On 2015/10/29 09:44:12, magjed_webrtc wrote: > |slice_height| etc is not defined here anymore. Maybe move this log inside the > big if-statement surface vs non-surface above. |slice_height|, |stride| and > |color_format| is not interesting to print for surface decoding anyway. Changed back to read widht and height as fields from the codec. https://codereview.webrtc.org/1422963003/diff/60001/talk/app/webrtc/java/src/... File talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java (right): https://codereview.webrtc.org/1422963003/diff/60001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:327: private static class DecodedTextureBuffer { On 2015/10/29 09:44:13, magjed_webrtc wrote: > I think this class has become bloated with too much state and with mutable > variables that you have to set outside the constructor. I think you should just > add final |width|, |height|, and |decodeTimeMs| to this class and nothing more. Acknowledged. https://codereview.webrtc.org/1422963003/diff/60001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:336: public enum State { On 2015/10/29 09:44:13, magjed_webrtc wrote: > I don't think this State makes sense, because it's not a per-buffer state, it's > a per-MediaCodec state. Either the MediaCodec is currently rendering to the > single Surface output or else it's not. I prefer the old code with a single > boolean flag to represent this. Done. https://codereview.webrtc.org/1422963003/diff/60001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:450: private DecodedTextureBuffer dequeueTextureBuffer(int dequeueTimeoutMs) { On 2015/10/29 09:44:13, magjed_webrtc wrote: > This function has become too bloated and complicated. Done. https://codereview.webrtc.org/1422963003/diff/60001/talk/app/webrtc/java/src/... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:556: return result; On 2015/10/29 09:44:13, magjed_webrtc wrote: > I think you should add a |decodedTimeMs| to DecodedByteBuffer and revert to > returning a DecodedByteBuffer here. Then you can set the decode time here for > both surface/non-surface output: > return new DecodedByteBuffer(result, info.offset, info.size, > info.presentationTimeUs, SystemClock.elapsedRealtime() - > decodeStartTimeMs.remove()); > Then you can also read the decode time in C++ for both surface/non-surface more > consistently. Done.
https://codereview.webrtc.org/1422963003/diff/120001/talk/app/webrtc/java/jni... File talk/app/webrtc/java/jni/androidmediadecoder_jni.cc (right): https://codereview.webrtc.org/1422963003/diff/120001/talk/app/webrtc/java/jni... talk/app/webrtc/java/jni/androidmediadecoder_jni.cc:501: frames_received_, frames_decoded_); Revert indentation change? https://codereview.webrtc.org/1422963003/diff/120001/talk/app/webrtc/java/jni... talk/app/webrtc/java/jni/androidmediadecoder_jni.cc:719: ALOGV("Decoder frame out # %d. %d x %d. %d x %d. Color: 0x%x. TS: %lld." Has this log been removed from master? https://codereview.webrtc.org/1422963003/diff/120001/talk/app/webrtc/java/src... File talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java (left): https://codereview.webrtc.org/1422963003/diff/120001/talk/app/webrtc/java/src... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:355: throws IllegalStateException, MediaCodec.CodecException { Put 'throws IllegalStateException, MediaCodec.CodecException' back. https://codereview.webrtc.org/1422963003/diff/120001/talk/app/webrtc/java/src... File talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java (right): https://codereview.webrtc.org/1422963003/diff/120001/talk/app/webrtc/java/src... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:309: I don't think we need this empty line. https://codereview.webrtc.org/1422963003/diff/120001/talk/app/webrtc/java/src... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:438: case MediaCodec.INFO_OUTPUT_BUFFERS_CHANGED: Revert switch case indentation change - it was correct before. https://codereview.webrtc.org/1422963003/diff/120001/talk/app/webrtc/java/src... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:479: SystemClock.elapsedRealtime() - decodeStartTimeMs.poll()); Use remove() instead of poll() because you are not polling. It's an exceptional circumstance if |decodeStartTimeMs is empty here and throwing a NoSuchElementException is more appropriate than misleading poll() call followed by null pointer exception crash. https://codereview.webrtc.org/1422963003/diff/120001/talk/app/webrtc/java/src... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:507: mediaCodec.releaseOutputBuffer(firstBuffer.index, true /* render */); I would prefer to remove |firstBuffer| and write it like this instead: if (!isWaitingForTexture) { // releaseOutputBuffer renders to the decoder output surface. mediaCodec.releaseOutputBuffer( dequeuedSurfaceOutputBuffers.peek().index, true /* render */); isWaitingForTexture = true; } and below: ... final DecodedOutputBuffer outputBuffer = dequeuedSurfaceOutputBuffers.remove(); return new DecodedTextureBuffer(info.textureID, info.transformMatrix, outputBuffer.presentationTimestampUs, outputBuffer.decodeTimeMs); } If you feel strongly about reducing decode time caused by polling latency, you can duplicate the first 5 lines or put the two if-statements in a for-loop with 2 runs. https://codereview.webrtc.org/1422963003/diff/120001/talk/app/webrtc/java/src... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:513: // rendered at the time. s/rendered at the time/rendererd at a time/g
perkj@webrtc.org changed reviewers: + glaznev@webrtc.org
Hi, Alex, can you take a first pass as well even though I have not addressed magjeds last comments? Seems like we are down to minor details now. I will do the generic_decoder changes in a separate cl.
https://codereview.webrtc.org/1422963003/diff/120001/talk/app/webrtc/java/jni... File talk/app/webrtc/java/jni/androidmediadecoder_jni.cc (right): https://codereview.webrtc.org/1422963003/diff/120001/talk/app/webrtc/java/jni... talk/app/webrtc/java/jni/androidmediadecoder_jni.cc:719: ALOGV("Decoder frame out # %d. %d x %d. %d x %d. Color: 0x%x. TS: %lld." On 2015/10/30 12:28:10, magjed_webrtc wrote: > Has this log been removed from master? I think this can be added back or alternatively in case buffer tracking is defined set kMaxDecodedLogFrames to something very big and then ALOGD above will log information for each input/output buffers https://codereview.webrtc.org/1422963003/diff/120001/talk/app/webrtc/java/jni... talk/app/webrtc/java/jni/androidmediadecoder_jni.cc:745: callback_->Decoded(decoded_frame, decode_time_ms); I am not sure if this is taken care of, but sometimes H.264 decoder will not start generating output until it accumulates some amount of frames - just got a report that one decoder accumulates 11 frames before it starts generating output. So decode_time_ms (decode latency) may be quite high for this case even if actual frame decoding time is low https://codereview.webrtc.org/1422963003/diff/120001/talk/app/webrtc/java/src... File talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java (right): https://codereview.webrtc.org/1422963003/diff/120001/talk/app/webrtc/java/src... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:334: private final long decodeTimeMs; I would call it decodeLatency - it can be bigger than actual decode time. https://codereview.webrtc.org/1422963003/diff/120001/talk/app/webrtc/java/src... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:441: if (hasDecodedFirstFrame) { May be just ignore it? I think it will be ok if we can swallow up some codec misbehavior without reporting error. https://codereview.webrtc.org/1422963003/diff/120001/talk/app/webrtc/java/src... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:450: if (hasDecodedFirstFrame && (new_width != width || new_height != height)) { Should we throw an exception here? Why we don't allow on-fly resolution change? https://codereview.webrtc.org/1422963003/diff/120001/talk/app/webrtc/java/src... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:499: if (dequeuedSurfaceOutputBuffers.isEmpty()) nit: add brackets https://codereview.webrtc.org/1422963003/diff/120001/talk/app/webrtc/java/src... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:503: DecodedOutputBuffer firstBuffer = dequeuedSurfaceOutputBuffers.peek(); I think you may add a condition to call releaseOutputBuffer(render = false) if dequeuedSurfaceOutputBuffers size is more than 2 or 3.
Patchset #7 (id:160001) has been deleted
Patchset #7 (id:180001) has been deleted
PTAL https://codereview.webrtc.org/1422963003/diff/120001/talk/app/webrtc/java/jni... File talk/app/webrtc/java/jni/androidmediadecoder_jni.cc (right): https://codereview.webrtc.org/1422963003/diff/120001/talk/app/webrtc/java/jni... talk/app/webrtc/java/jni/androidmediadecoder_jni.cc:501: frames_received_, frames_decoded_); On 2015/10/30 12:28:10, magjed_webrtc wrote: > Revert indentation change? Done. https://codereview.webrtc.org/1422963003/diff/120001/talk/app/webrtc/java/jni... talk/app/webrtc/java/jni/androidmediadecoder_jni.cc:719: ALOGV("Decoder frame out # %d. %d x %d. %d x %d. Color: 0x%x. TS: %lld." On 2015/10/30 21:06:10, AlexG wrote: > On 2015/10/30 12:28:10, magjed_webrtc wrote: > > Has this log been removed from master? > > I think this can be added back or alternatively in case buffer tracking is > defined set kMaxDecodedLogFrames to something very big and then ALOGD above will > log information for each input/output buffers Right, this looks like a merge mistake. I will remove and rely on ALOGD above. https://codereview.webrtc.org/1422963003/diff/120001/talk/app/webrtc/java/jni... talk/app/webrtc/java/jni/androidmediadecoder_jni.cc:745: callback_->Decoded(decoded_frame, decode_time_ms); On 2015/10/30 21:06:10, AlexG wrote: > I am not sure if this is taken care of, but sometimes H.264 decoder will not > start generating output until it accumulates some amount of frames - just got a > report that one decoder accumulates 11 frames before it starts generating > output. So decode_time_ms (decode latency) may be quite high for this case even > if actual frame decoding time is low So if that is just an initial problem we should recover. You will get a long delay while it happens of course but it should eventually catch up. Note that the decode time in generic_decoder is,before this change, calculating the decode time as the time from a frame was sent to the decoder until the same frame is decoded and callback_->Decoded invoked so the decode time here would be the same as before for that case. https://codereview.webrtc.org/1422963003/diff/120001/talk/app/webrtc/java/src... File talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java (right): https://codereview.webrtc.org/1422963003/diff/120001/talk/app/webrtc/java/src... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:309: On 2015/10/30 12:28:10, magjed_webrtc wrote: > I don't think we need this empty line. Done. https://codereview.webrtc.org/1422963003/diff/120001/talk/app/webrtc/java/src... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:334: private final long decodeTimeMs; On 2015/10/30 21:06:10, AlexG wrote: > I would call it decodeLatency - it can be bigger than actual decode time. Do you feel strong about it ? I can change here, but it is called decode_time in webrtc/modules/video_coding/main/source/timing.cc where this eventually end up. Also the statistics call this decode time. webrtc/video/receive_statistics_proxy.cc https://codereview.webrtc.org/1422963003/diff/120001/talk/app/webrtc/java/src... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:438: case MediaCodec.INFO_OUTPUT_BUFFERS_CHANGED: On 2015/10/30 12:28:10, magjed_webrtc wrote: > Revert switch case indentation change - it was correct before. Done. https://codereview.webrtc.org/1422963003/diff/120001/talk/app/webrtc/java/src... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:441: if (hasDecodedFirstFrame) { On 2015/10/30 21:06:10, AlexG wrote: > May be just ignore it? I think it will be ok if we can swallow up some codec > misbehavior without reporting error. Well, what does that mean for the output buffers we are currently using in dequeuedSurfaceOutputBuffers? I guess we then also must support dropping them all. https://codereview.webrtc.org/1422963003/diff/120001/talk/app/webrtc/java/src... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:450: if (hasDecodedFirstFrame && (new_width != width || new_height != height)) { On 2015/10/30 21:06:10, AlexG wrote: > Should we throw an exception here? Why we don't allow on-fly resolution change? We reset the decoder from c++ https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin... So this is here to make sure that behavior is not changed without modifying the code in Java. If the width and height changes, we need to read the width and height into each output frame now that we might render the output buffer to the Surface at a later time in dequeTextureFrame. I must admit that I have no idea why this class previously did not support on the fly size change, I hoped you knew? :-) But I assume that if size change, so would the outputBuffers which would be a problem for all items in dequeuedOutputBuffers. https://codereview.webrtc.org/1422963003/diff/120001/talk/app/webrtc/java/src... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:479: SystemClock.elapsedRealtime() - decodeStartTimeMs.poll()); On 2015/10/30 12:28:10, magjed_webrtc wrote: > Use remove() instead of poll() because you are not polling. It's an exceptional > circumstance if |decodeStartTimeMs is empty here and throwing a > NoSuchElementException is more appropriate than misleading poll() call followed > by null pointer exception crash. Done. https://codereview.webrtc.org/1422963003/diff/120001/talk/app/webrtc/java/src... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:499: if (dequeuedSurfaceOutputBuffers.isEmpty()) On 2015/10/30 21:06:10, AlexG wrote: > nit: add brackets Done. https://codereview.webrtc.org/1422963003/diff/120001/talk/app/webrtc/java/src... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:503: DecodedOutputBuffer firstBuffer = dequeuedSurfaceOutputBuffers.peek(); On 2015/10/30 21:06:10, AlexG wrote: > I think you may add a condition to call releaseOutputBuffer(render = false) if > dequeuedSurfaceOutputBuffers size is more than 2 or 3. done https://codereview.webrtc.org/1422963003/diff/120001/talk/app/webrtc/java/src... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:507: mediaCodec.releaseOutputBuffer(firstBuffer.index, true /* render */); On 2015/10/30 12:28:10, magjed_webrtc wrote: > I would prefer to remove |firstBuffer| and write it like this instead: > if (!isWaitingForTexture) { > // releaseOutputBuffer renders to the decoder output surface. > mediaCodec.releaseOutputBuffer( > dequeuedSurfaceOutputBuffers.peek().index, true /* render */); > isWaitingForTexture = true; > } > and below: > ... > final DecodedOutputBuffer outputBuffer = > dequeuedSurfaceOutputBuffers.remove(); > return new DecodedTextureBuffer(info.textureID, info.transformMatrix, > outputBuffer.presentationTimestampUs, outputBuffer.decodeTimeMs); > } > > If you feel strongly about reducing decode time caused by polling latency, you > can duplicate the first 5 lines or put the two if-statements in a for-loop with > 2 runs. ok, I am leaning towards reducing the poll intervall in a follow up cl. https://codereview.webrtc.org/1422963003/diff/120001/talk/app/webrtc/java/src... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:513: // rendered at the time. On 2015/10/30 12:28:10, magjed_webrtc wrote: > s/rendered at the time/rendererd at a time/g Done.
https://codereview.webrtc.org/1422963003/diff/120001/talk/app/webrtc/java/src... File talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java (right): https://codereview.webrtc.org/1422963003/diff/120001/talk/app/webrtc/java/src... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:513: // rendered at the time. On 2015/11/06 14:31:12, perkj1 wrote: > On 2015/10/30 12:28:10, magjed_webrtc wrote: > > s/rendered at the time/rendererd at a time/g > > Done. No? https://codereview.webrtc.org/1422963003/diff/200001/talk/app/webrtc/java/and... File talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java (right): https://codereview.webrtc.org/1422963003/diff/200001/talk/app/webrtc/java/and... talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java:32: import android.graphics.SurfaceTexture; This import is unnecessary now - remove it. https://codereview.webrtc.org/1422963003/diff/200001/talk/app/webrtc/java/jni... File talk/app/webrtc/java/jni/androidmediadecoder_jni.cc (right): https://codereview.webrtc.org/1422963003/diff/200001/talk/app/webrtc/java/jni... talk/app/webrtc/java/jni/androidmediadecoder_jni.cc:607: if (texture_id > 0) { // |texture_id| == 0 represents a dropped frame. texture_id is a GLuint, so negative values can in theory be ok. Zero is the only special texture id, so change this to 'texture_id != 0'. https://codereview.webrtc.org/1422963003/diff/200001/talk/app/webrtc/java/src... File talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java (right): https://codereview.webrtc.org/1422963003/diff/200001/talk/app/webrtc/java/src... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:102: // The below variables are only used when the decode decodes to a Surface. Strange sentence, maybe "when the decoder decodes" or just "when decoding to a Surface".
Patchset #8 (id:220001) has been deleted
PTAL I will land the generic_decoder changes in https://codereview.webrtc.org/1414693006/. Except for that, are you ok with this? https://codereview.webrtc.org/1422963003/diff/200001/talk/app/webrtc/java/and... File talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java (right): https://codereview.webrtc.org/1422963003/diff/200001/talk/app/webrtc/java/and... talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java:32: import android.graphics.SurfaceTexture; On 2015/11/09 16:30:52, magjed_webrtc wrote: > This import is unnecessary now - remove it. Done. https://codereview.webrtc.org/1422963003/diff/200001/talk/app/webrtc/java/jni... File talk/app/webrtc/java/jni/androidmediadecoder_jni.cc (right): https://codereview.webrtc.org/1422963003/diff/200001/talk/app/webrtc/java/jni... talk/app/webrtc/java/jni/androidmediadecoder_jni.cc:607: if (texture_id > 0) { // |texture_id| == 0 represents a dropped frame. On 2015/11/09 16:30:52, magjed_webrtc wrote: > texture_id is a GLuint, so negative values can in theory be ok. Zero is the only > special texture id, so change this to 'texture_id != 0'. Done. https://codereview.webrtc.org/1422963003/diff/200001/talk/app/webrtc/java/src... File talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java (right): https://codereview.webrtc.org/1422963003/diff/200001/talk/app/webrtc/java/src... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:102: // The below variables are only used when the decode decodes to a Surface. On 2015/11/09 16:30:52, magjed_webrtc wrote: > Strange sentence, maybe "when the decoder decodes" or just "when decoding to a > Surface". Done.
lgtm
With this change will updateTexImage be called on decoder thread? Can it lead to video tearing when using VideoRendererGui? https://codereview.webrtc.org/1422963003/diff/240001/talk/app/webrtc/java/jni... File talk/app/webrtc/java/jni/androidmediadecoder_jni.cc (right): https://codereview.webrtc.org/1422963003/diff/240001/talk/app/webrtc/java/jni... talk/app/webrtc/java/jni/androidmediadecoder_jni.cc:708: int64_t frame_delayed_ms = 0; Is it different from decode_time_ms_? It is calculated as diff between frame output time and frame input time - as I understood this calculation is now handled by Java. do you observe similar values for "DecTime: and "DelayTime"? If yes probably worth to remove frame_delayd_ms and frame_rtc_times_ms_? https://codereview.webrtc.org/1422963003/diff/240001/talk/app/webrtc/java/jni... talk/app/webrtc/java/jni/androidmediadecoder_jni.cc:740: if (!decoded_frame.IsZeroSize()) { Can you please add a comment when decoder_frame.IsZeroSize can be true https://codereview.webrtc.org/1422963003/diff/240001/talk/app/webrtc/java/src... File talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java (right): https://codereview.webrtc.org/1422963003/diff/240001/talk/app/webrtc/java/src... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:316: decodeStartTimeMs.clear(); nit: it looks a little more clear for me to call clear() in initDecode() https://codereview.webrtc.org/1422963003/diff/240001/talk/app/webrtc/java/src... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:436: newFrameLock.wait(timeoutMs); This may be not a safe wait() as I was recently learned from Magnus :) Use something like ThreadUtils.awaitUninterruptibly()? https://codereview.webrtc.org/1422963003/diff/240001/talk/app/webrtc/java/src... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:536: if (dequeuedSurfaceOutputBuffers.size() > 2) { Make limitation dependent on output buffers amount to avoid hang in case we have only 2 output buffer. May be use as a limit: min(2, outbutBuffers.length - 1) https://codereview.webrtc.org/1422963003/diff/240001/talk/app/webrtc/java/src... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:537: Logging.w(TAG, "Too many output buffers. Dropping frame."); I think it is worth to add stat variable on how many decoded frames were dropped and print it when decoder release and on every frame drop https://codereview.webrtc.org/1422963003/diff/240001/talk/app/webrtc/java/src... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:551: nit: extra line https://codereview.webrtc.org/1422963003/diff/240001/talk/app/webrtc/java/src... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:566: dequeuedSurfaceOutputBuffers.remove(); call mediaCodec.releaseOutputBuffer(true) for the next buffer in a queue here to speed up buffer pipeline.
I will try to reproduce the tearing issue with the old renderer tomorrow. If I can repro, I will test Magnus suggestion to make sure the texture copy is synchronized using eglWait. magjed is still working on fixing a bug (unrelated to this on Nexus 5x and Nexus 6) in the SurfaceViewRenderer so we can not use it everywhere yet. PTAL. https://codereview.webrtc.org/1422963003/diff/240001/talk/app/webrtc/java/jni... File talk/app/webrtc/java/jni/androidmediadecoder_jni.cc (right): https://codereview.webrtc.org/1422963003/diff/240001/talk/app/webrtc/java/jni... talk/app/webrtc/java/jni/androidmediadecoder_jni.cc:708: int64_t frame_delayed_ms = 0; On 2015/11/10 01:21:39, AlexG wrote: > Is it different from decode_time_ms_? It is calculated as diff between frame > output time and frame input time - as I understood this calculation is now > handled by Java. do you observe similar values for "DecTime: and "DelayTime"? If > yes probably worth to remove frame_delayd_ms and frame_rtc_times_ms_? The difference is that in Java we calculate the decode time. |frame_delayed_ms| also include the time until we actually output the frame which can be higher in the case when the frame is rendered later and thus blocks the output frame for a short period of time. I am happy to remove this. Keep or remove ? https://codereview.webrtc.org/1422963003/diff/240001/talk/app/webrtc/java/jni... talk/app/webrtc/java/jni/androidmediadecoder_jni.cc:740: if (!decoded_frame.IsZeroSize()) { On 2015/11/10 01:21:39, AlexG wrote: > Can you please add a comment when decoder_frame.IsZeroSize can be true Done. https://codereview.webrtc.org/1422963003/diff/240001/talk/app/webrtc/java/src... File talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java (right): https://codereview.webrtc.org/1422963003/diff/240001/talk/app/webrtc/java/src... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:316: decodeStartTimeMs.clear(); On 2015/11/10 01:21:39, AlexG wrote: > nit: it looks a little more clear for me to call clear() in initDecode() Done. https://codereview.webrtc.org/1422963003/diff/240001/talk/app/webrtc/java/src... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:436: newFrameLock.wait(timeoutMs); On 2015/11/10 01:21:39, AlexG wrote: > This may be not a safe wait() as I was recently learned from Magnus :) > Use something like ThreadUtils.awaitUninterruptibly()? Discussed with magnus. Spurious wake up is not dangerous here, we will simply get the texture next time we call dequeueTextureInfo. https://codereview.webrtc.org/1422963003/diff/240001/talk/app/webrtc/java/src... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:536: if (dequeuedSurfaceOutputBuffers.size() > 2) { On 2015/11/10 01:21:39, AlexG wrote: > Make limitation dependent on output buffers amount to avoid hang in case we have > only 2 output buffer. > May be use as a limit: > min(2, outbutBuffers.length - 1) Done. https://codereview.webrtc.org/1422963003/diff/240001/talk/app/webrtc/java/src... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:537: Logging.w(TAG, "Too many output buffers. Dropping frame."); On 2015/11/10 01:21:39, AlexG wrote: > I think it is worth to add stat variable on how many decoded frames were dropped > and print it when decoder release and on every frame drop Done. https://codereview.webrtc.org/1422963003/diff/240001/talk/app/webrtc/java/src... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:551: On 2015/11/10 01:21:39, AlexG wrote: > nit: extra line Done. https://codereview.webrtc.org/1422963003/diff/240001/talk/app/webrtc/java/src... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:566: dequeuedSurfaceOutputBuffers.remove(); On 2015/11/10 01:21:39, AlexG wrote: > call mediaCodec.releaseOutputBuffer(true) for the next buffer in a queue here to > speed up buffer pipeline. :-) I did a few rounds back but Magnus wanted me to remove it. He later accepted to do it but I said I would reduce the poll intervall instead. Ok- I will do it.
lgtm Hopefully last comments. Please also check if tearing problem is solved before submitting this CL. https://codereview.webrtc.org/1422963003/diff/240001/talk/app/webrtc/java/jni... File talk/app/webrtc/java/jni/androidmediadecoder_jni.cc (right): https://codereview.webrtc.org/1422963003/diff/240001/talk/app/webrtc/java/jni... talk/app/webrtc/java/jni/androidmediadecoder_jni.cc:708: int64_t frame_delayed_ms = 0; On 2015/11/10 13:58:15, perkj1 wrote: > On 2015/11/10 01:21:39, AlexG wrote: > > Is it different from decode_time_ms_? It is calculated as diff between frame > > output time and frame input time - as I understood this calculation is now > > handled by Java. do you observe similar values for "DecTime: and "DelayTime"? > If > > yes probably worth to remove frame_delayd_ms and frame_rtc_times_ms_? > > The difference is that in Java we calculate the decode time. |frame_delayed_ms| > also include the time until we actually output the frame which can be higher in > the case when the frame is rendered later and thus blocks the output frame for a > short period of time. > > I am happy to remove this. Keep or remove ? It is probably better to keep it then - at least for some time to see the amount of delay introduced by renderer and waiting for texture available callback. But it will be probably more clearer to get this delays from a single place - for now decTime is from Java and delayTime from C++. I suggest to move delayTime to Java - probably one more field in OutputBuffer class? https://codereview.webrtc.org/1422963003/diff/280001/talk/app/webrtc/java/src... File talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java (right): https://codereview.webrtc.org/1422963003/diff/280001/talk/app/webrtc/java/src... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:539: Logging.w(TAG, "Too many output buffers. Dropping frame."); nit: print droppedFrame value in this message?
https://codereview.webrtc.org/1422963003/diff/240001/talk/app/webrtc/java/jni... File talk/app/webrtc/java/jni/androidmediadecoder_jni.cc (right): https://codereview.webrtc.org/1422963003/diff/240001/talk/app/webrtc/java/jni... talk/app/webrtc/java/jni/androidmediadecoder_jni.cc:708: int64_t frame_delayed_ms = 0; On 2015/11/11 01:54:06, AlexG wrote: > On 2015/11/10 13:58:15, perkj1 wrote: > > On 2015/11/10 01:21:39, AlexG wrote: > > > Is it different from decode_time_ms_? It is calculated as diff between frame > > > output time and frame input time - as I understood this calculation is now > > > handled by Java. do you observe similar values for "DecTime: and > "DelayTime"? > > If > > > yes probably worth to remove frame_delayd_ms and frame_rtc_times_ms_? > > > > The difference is that in Java we calculate the decode time. > |frame_delayed_ms| > > also include the time until we actually output the frame which can be higher > in > > the case when the frame is rendered later and thus blocks the output frame for > a > > short period of time. > > > > I am happy to remove this. Keep or remove ? > > It is probably better to keep it then - at least for some time to see the amount > of delay introduced by renderer and waiting for texture available callback. But > it will be probably more clearer to get this delays from a single place - for > now decTime is from Java and delayTime from C++. I suggest to move delayTime to > Java - probably one more field in OutputBuffer class? Done. https://codereview.webrtc.org/1422963003/diff/280001/talk/app/webrtc/java/src... File talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java (right): https://codereview.webrtc.org/1422963003/diff/280001/talk/app/webrtc/java/src... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:539: Logging.w(TAG, "Too many output buffers. Dropping frame."); On 2015/11/11 01:54:06, AlexG wrote: > nit: print droppedFrame value in this message? Done.
https://codereview.webrtc.org/1422963003/diff/300001/talk/app/webrtc/java/src... File talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java (right): https://codereview.webrtc.org/1422963003/diff/300001/talk/app/webrtc/java/src... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:388: // Since there there is only one texture, this interval depend on the time from when Remove extra 'there' https://codereview.webrtc.org/1422963003/diff/300001/talk/app/webrtc/java/src... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:549: if (dequeuedSurfaceOutputBuffers.size() > Math.min(2, outputBuffers.length - 1)) { I would prefer: dequeuedSurfaceOutputBuffers.size() >= Math.min(3, outputBuffers.length) maybe should also give the constant here a name.
Description was changed from ========== Patchset 1 is a pure revert of "Revert of "Android MediaCodecVideoDecoder: Manage lifetime of texture frames" https://codereview.webrtc.org/1378033003/ Following patchset move the responsibility of calculating the decode time to Java. BUG= ========== to ========== Patchset 1 is a pure revert of "Revert of "Android MediaCodecVideoDecoder: Manage lifetime of texture frames" https://codereview.webrtc.org/1378033003/ Following patchsets move the responsibility of calculating the decode time to Java. TESTED= Apprtc loopback using H264 and VP8 on N5, N6, N7, S5 ==========
https://codereview.webrtc.org/1422963003/diff/300001/talk/app/webrtc/java/src... File talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java (right): https://codereview.webrtc.org/1422963003/diff/300001/talk/app/webrtc/java/src... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:388: // Since there there is only one texture, this interval depend on the time from when On 2015/11/11 10:09:53, magjed_webrtc wrote: > Remove extra 'there' Done. https://codereview.webrtc.org/1422963003/diff/300001/talk/app/webrtc/java/src... talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:549: if (dequeuedSurfaceOutputBuffers.size() > Math.min(2, outputBuffers.length - 1)) { On 2015/11/11 10:09:53, magjed_webrtc wrote: > I would prefer: > dequeuedSurfaceOutputBuffers.size() >= Math.min(3, outputBuffers.length) > maybe should also give the constant here a name. Done.
The CQ bit was checked by perkj@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org, glaznev@webrtc.org Link to the patchset: https://codereview.webrtc.org/1422963003/#ps320001 (title: "Addressed magjeds comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1422963003/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1422963003/320001
Message was sent while issue was closed.
Committed patchset #12 (id:320001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/9cb8982e64f08d3d630bf7c3d2bcc78c10db88e2 Cr-Commit-Position: refs/heads/master@{#10597}
Message was sent while issue was closed.
A revert of this CL (patchset #12 id:320001) has been created in https://codereview.webrtc.org/1441363002/ by perkj@webrtc.org. The reason for reverting is: Causes fallback to SW decoder if a renderer is put in the background.. |
