Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1732533002:
Add an option to soft reset HW decoder. (Closed)
DescriptionAdd an option to soft reset HW decoder.
Soft reset can be used when input frame resolution changes
to avoid re creating MediaCodec instance.
Instead MediaCodec is flushed and some variables are reset.
R=pbos@webrtc.org, perkj@webrtc.org
Committed: https://crrev.com/6a4a03c59c711fa423d72560edbe63d51136ae4a
Cr-Commit-Position: refs/heads/master@{#11878}
Patch Set 1 #Patch Set 2 : Add separate function for soft reset #Patch Set 3 : Add TODO item #
Total comments: 6
Patch Set 4 : Address comments #
Total comments: 10
Patch Set 5 : Address comments - 2 #
Messages
Total messages: 24 (9 generated)
pbos@webrtc.org changed reviewers: + pbos@webrtc.org
Does it matter to flush/reset any of these variables or could we just pass the data into MediaCodec?
On 2016/02/24 16:10:44, pbos-webrtc wrote: > Does it matter to flush/reset any of these variables or could we just pass the > data into MediaCodec? E.g. does the decoder instance really need to care about width/height at all?
glaznev@webrtc.org changed reviewers: + perkj@webrtc.org
PTAL
On 2016/02/24 16:19:38, pbos-webrtc wrote: > On 2016/02/24 16:10:44, pbos-webrtc wrote: > > Does it matter to flush/reset any of these variables or could we just pass the > > data into MediaCodec? > > E.g. does the decoder instance really need to care about width/height at all? MediaCodec.flush() is necessary as well as clearing buffer queues. Width/height are not necessary, but I think it is cleaner to set them since we know new frame size. It may be useful later for byte buffer decoding (for now soft reset is supported for surface decoding only). I updated code to move soft reset to a separate function.
Can you add some kind of test that triggers this? Preferably by addding a new loopback test in PeerConnectionTest.java where the resolution can be changed by changing the VideoCapturerAndroid resolution. Even better but much harder- add a c++ test that tests androids hardware encoder and decoder wrapper and nothing else. (peerconnection_unittests) https://codereview.webrtc.org/1732533002/diff/40001/webrtc/api/java/jni/andro... File webrtc/api/java/jni/androidmediadecoder_jni.cc (right): https://codereview.webrtc.org/1732533002/diff/40001/webrtc/api/java/jni/andro... webrtc/api/java/jni/androidmediadecoder_jni.cc:405: ResetDecodeOnCodecThread? https://codereview.webrtc.org/1732533002/diff/40001/webrtc/api/java/jni/andro... webrtc/api/java/jni/androidmediadecoder_jni.cc:419: key_frame_required_ = true; Move all these to a helper method and call from InitDecodeOnCodecThread as well please. Call it ResetState or something ? https://codereview.webrtc.org/1732533002/diff/40001/webrtc/api/java/src/org/w... File webrtc/api/java/src/org/webrtc/MediaCodecVideoDecoder.java (right): https://codereview.webrtc.org/1732533002/diff/40001/webrtc/api/java/src/org/w... webrtc/api/java/src/org/webrtc/MediaCodecVideoDecoder.java:313: private void softResetDecode(int width, int height) { call this just reset() and please add a comment what this does.
PTAL https://codereview.webrtc.org/1732533002/diff/40001/webrtc/api/java/jni/andro... File webrtc/api/java/jni/androidmediadecoder_jni.cc (right): https://codereview.webrtc.org/1732533002/diff/40001/webrtc/api/java/jni/andro... webrtc/api/java/jni/androidmediadecoder_jni.cc:405: On 2016/03/02 12:33:22, perkj_webrtc wrote: > ResetDecodeOnCodecThread? Done. https://codereview.webrtc.org/1732533002/diff/40001/webrtc/api/java/jni/andro... webrtc/api/java/jni/androidmediadecoder_jni.cc:419: key_frame_required_ = true; On 2016/03/02 12:33:22, perkj_webrtc wrote: > Move all these to a helper method and call from InitDecodeOnCodecThread as well > please. Call it ResetState or something ? Done. https://codereview.webrtc.org/1732533002/diff/40001/webrtc/api/java/src/org/w... File webrtc/api/java/src/org/webrtc/MediaCodecVideoDecoder.java (right): https://codereview.webrtc.org/1732533002/diff/40001/webrtc/api/java/src/org/w... webrtc/api/java/src/org/webrtc/MediaCodecVideoDecoder.java:313: private void softResetDecode(int width, int height) { On 2016/03/02 12:33:22, perkj_webrtc wrote: > call this just reset() and please add a comment what this does. Done.
lgtm
lgtm https://codereview.webrtc.org/1732533002/diff/60001/webrtc/api/java/jni/andro... File webrtc/api/java/jni/androidmediadecoder_jni.cc (right): https://codereview.webrtc.org/1732533002/diff/60001/webrtc/api/java/jni/andro... webrtc/api/java/jni/androidmediadecoder_jni.cc:315: key_frame_required_ = true; CheckOnCodecThread? https://codereview.webrtc.org/1732533002/diff/60001/webrtc/api/java/jni/andro... webrtc/api/java/jni/androidmediadecoder_jni.cc:544: // TODO(glaznev): try to use similar approach for H.264. (and buffer decoding?) https://codereview.webrtc.org/1732533002/diff/60001/webrtc/api/java/src/org/w... File webrtc/api/java/src/org/webrtc/MediaCodecVideoDecoder.java (right): https://codereview.webrtc.org/1732533002/diff/60001/webrtc/api/java/src/org/w... webrtc/api/java/src/org/webrtc/MediaCodecVideoDecoder.java:245: throw new RuntimeException("initDecode: forgot to release()?"); Forgot https://codereview.webrtc.org/1732533002/diff/60001/webrtc/api/java/src/org/w... webrtc/api/java/src/org/webrtc/MediaCodecVideoDecoder.java:261: throw new RuntimeException("initDecode: non supported codec " + type); Non-supported codec https://codereview.webrtc.org/1732533002/diff/60001/webrtc/api/java/src/org/w... webrtc/api/java/src/org/webrtc/MediaCodecVideoDecoder.java:318: throw new RuntimeException("reset for non initialized decoder."); Incorrect reset call for non-initialized decoder.
https://codereview.webrtc.org/1732533002/diff/60001/webrtc/api/java/jni/andro... File webrtc/api/java/jni/androidmediadecoder_jni.cc (right): https://codereview.webrtc.org/1732533002/diff/60001/webrtc/api/java/jni/andro... webrtc/api/java/jni/androidmediadecoder_jni.cc:315: key_frame_required_ = true; On 2016/03/04 10:23:13, pbos-webrtc wrote: > CheckOnCodecThread? Done. https://codereview.webrtc.org/1732533002/diff/60001/webrtc/api/java/jni/andro... webrtc/api/java/jni/androidmediadecoder_jni.cc:544: // TODO(glaznev): try to use similar approach for H.264. On 2016/03/04 10:23:13, pbos-webrtc wrote: > (and buffer decoding?) Done. https://codereview.webrtc.org/1732533002/diff/60001/webrtc/api/java/src/org/w... File webrtc/api/java/src/org/webrtc/MediaCodecVideoDecoder.java (right): https://codereview.webrtc.org/1732533002/diff/60001/webrtc/api/java/src/org/w... webrtc/api/java/src/org/webrtc/MediaCodecVideoDecoder.java:245: throw new RuntimeException("initDecode: forgot to release()?"); On 2016/03/04 10:23:13, pbos-webrtc wrote: > Forgot Done. https://codereview.webrtc.org/1732533002/diff/60001/webrtc/api/java/src/org/w... webrtc/api/java/src/org/webrtc/MediaCodecVideoDecoder.java:261: throw new RuntimeException("initDecode: non supported codec " + type); On 2016/03/04 10:23:13, pbos-webrtc wrote: > Non-supported codec Done. https://codereview.webrtc.org/1732533002/diff/60001/webrtc/api/java/src/org/w... webrtc/api/java/src/org/webrtc/MediaCodecVideoDecoder.java:318: throw new RuntimeException("reset for non initialized decoder."); On 2016/03/04 10:23:13, pbos-webrtc wrote: > Incorrect reset call for non-initialized decoder. Done.
The CQ bit was checked by glaznev@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org, perkj@webrtc.org Link to the patchset: https://codereview.webrtc.org/1732533002/#ps80001 (title: "Address comments - 2")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1732533002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1732533002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_baremetal on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/9585)
The CQ bit was checked by glaznev@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1732533002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1732533002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_baremetal on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/9587)
Description was changed from ========== Add an option to soft reset HW decoder. Soft reset can be used when input frame resolution changes to avoid re creating MediaCodec instance. Instead MediaCodec is flushed and some variables are reset. ========== to ========== Add an option to soft reset HW decoder. Soft reset can be used when input frame resolution changes to avoid re creating MediaCodec instance. Instead MediaCodec is flushed and some variables are reset. R=pbos@webrtc.org, perkj@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/6a4a03c59c711fa423d72560e... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) manually as 6a4a03c59c711fa423d72560edbe63d51136ae4a (presubmit successful).
Message was sent while issue was closed.
Description was changed from ========== Add an option to soft reset HW decoder. Soft reset can be used when input frame resolution changes to avoid re creating MediaCodec instance. Instead MediaCodec is flushed and some variables are reset. R=pbos@webrtc.org, perkj@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/6a4a03c59c711fa423d72560e... ========== to ========== Add an option to soft reset HW decoder. Soft reset can be used when input frame resolution changes to avoid re creating MediaCodec instance. Instead MediaCodec is flushed and some variables are reset. R=pbos@webrtc.org, perkj@webrtc.org Committed: https://crrev.com/6a4a03c59c711fa423d72560edbe63d51136ae4a Cr-Commit-Position: refs/heads/master@{#11878} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/6a4a03c59c711fa423d72560edbe63d51136ae4a Cr-Commit-Position: refs/heads/master@{#11878} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
