Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2653183004:
Add QP for MediaCodec decoder. (Closed)
|
Created:
3 years, 11 months ago by sakal Modified:
3 years, 10 months ago Reviewers:
magjed_webrtc CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd QP for MediaCodec decoder.
BUG=webrtc:6541
Review-Url: https://codereview.webrtc.org/2653183004
Cr-Commit-Position: refs/heads/master@{#16662}
Committed: https://chromium.googlesource.com/external/webrtc/+/9c997a3b9e33e24f3b23ae69024d7436660c364d
Patch Set 1 : Rebase 2 #
Total comments: 10
Patch Set 2 : Changes according to magjed's comments. #1 #Patch Set 3 : Push back the conversion from int to rtc::Optional<uint8_t> #
Total comments: 2
Patch Set 4 : Move qp_int. #
Messages
Total messages: 23 (16 generated)
Patchset #4 (id:60001) has been deleted
The CQ bit was checked by sakal@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Description was changed from ========== Add QP for MediaCodec VP8/H264 decoder. BUG=webrtc:6541 ========== to ========== Add QP for MediaCodec decoder. BUG=webrtc:6541 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
sakal@webrtc.org changed reviewers: + magjed@webrtc.org
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
PTAL
https://codereview.webrtc.org/2653183004/diff/80001/webrtc/sdk/android/src/jn... File webrtc/sdk/android/src/jni/androidmediadecoder_jni.cc (right): https://codereview.webrtc.org/2653183004/diff/80001/webrtc/sdk/android/src/jn... webrtc/sdk/android/src/jni/androidmediadecoder_jni.cc:14: #include <deque> sort the list https://codereview.webrtc.org/2653183004/diff/80001/webrtc/sdk/android/src/jn... webrtc/sdk/android/src/jni/androidmediadecoder_jni.cc:35: #include "webrtc/modules/video_coding/utility/vp8_header_parser.h" sort the list https://codereview.webrtc.org/2653183004/diff/80001/webrtc/sdk/android/src/jn... webrtc/sdk/android/src/jni/androidmediadecoder_jni.cc:139: std::deque<int> pending_frame_qps_; Why not a regular queue? You don't need it to be double-ended. https://codereview.webrtc.org/2653183004/diff/80001/webrtc/sdk/android/src/jn... webrtc/sdk/android/src/jni/androidmediadecoder_jni.cc:662: int qp = -1; Use a 'rtc::Optional<uint8_t> qp' directly instead of magic -1 value. https://codereview.webrtc.org/2653183004/diff/80001/webrtc/sdk/android/src/jn... webrtc/sdk/android/src/jni/androidmediadecoder_jni.cc:882: callback_->Decoded( Why did you remove the return value check?
Patchset #2 (id:100001) has been deleted
Patchset #2 (id:120001) has been deleted
https://codereview.webrtc.org/2653183004/diff/80001/webrtc/sdk/android/src/jn... File webrtc/sdk/android/src/jni/androidmediadecoder_jni.cc (right): https://codereview.webrtc.org/2653183004/diff/80001/webrtc/sdk/android/src/jn... webrtc/sdk/android/src/jni/androidmediadecoder_jni.cc:14: #include <deque> On 2017/02/16 20:22:20, magjed_webrtc wrote: > sort the list Done. https://codereview.webrtc.org/2653183004/diff/80001/webrtc/sdk/android/src/jn... webrtc/sdk/android/src/jni/androidmediadecoder_jni.cc:35: #include "webrtc/modules/video_coding/utility/vp8_header_parser.h" On 2017/02/16 20:22:20, magjed_webrtc wrote: > sort the list Done. https://codereview.webrtc.org/2653183004/diff/80001/webrtc/sdk/android/src/jn... webrtc/sdk/android/src/jni/androidmediadecoder_jni.cc:139: std::deque<int> pending_frame_qps_; On 2017/02/16 20:22:20, magjed_webrtc wrote: > Why not a regular queue? You don't need it to be double-ended. queue type doesn't have clear method that is required to empty the queue. https://codereview.webrtc.org/2653183004/diff/80001/webrtc/sdk/android/src/jn... webrtc/sdk/android/src/jni/androidmediadecoder_jni.cc:662: int qp = -1; On 2017/02/16 20:22:20, magjed_webrtc wrote: > Use a 'rtc::Optional<uint8_t> qp' directly instead of magic -1 value. Done. https://codereview.webrtc.org/2653183004/diff/80001/webrtc/sdk/android/src/jn... webrtc/sdk/android/src/jni/androidmediadecoder_jni.cc:882: callback_->Decoded( On 2017/02/16 20:22:20, magjed_webrtc wrote: > Why did you remove the return value check? The new callback doesn't include a return value because it always returned success anyway.
lgtm https://codereview.webrtc.org/2653183004/diff/160001/webrtc/sdk/android/src/j... File webrtc/sdk/android/src/jni/androidmediadecoder_jni.cc (right): https://codereview.webrtc.org/2653183004/diff/160001/webrtc/sdk/android/src/j... webrtc/sdk/android/src/jni/androidmediadecoder_jni.cc:663: int qp_int; nit: move this variable into the if-statement and the else-if statement.
The CQ bit was checked by sakal@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org Link to the patchset: https://codereview.webrtc.org/2653183004/#ps180001 (title: "Move qp_int.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
https://codereview.webrtc.org/2653183004/diff/160001/webrtc/sdk/android/src/j... File webrtc/sdk/android/src/jni/androidmediadecoder_jni.cc (right): https://codereview.webrtc.org/2653183004/diff/160001/webrtc/sdk/android/src/j... webrtc/sdk/android/src/jni/androidmediadecoder_jni.cc:663: int qp_int; On 2017/02/17 09:50:19, magjed_webrtc wrote: > nit: move this variable into the if-statement and the else-if statement. Done.
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1487326135671940,
"parent_rev": "f9d91548084c39fae6c355fd03202fb8f66eb5a1", "commit_rev":
"9c997a3b9e33e24f3b23ae69024d7436660c364d"}
Message was sent while issue was closed.
Description was changed from ========== Add QP for MediaCodec decoder. BUG=webrtc:6541 ========== to ========== Add QP for MediaCodec decoder. BUG=webrtc:6541 Review-Url: https://codereview.webrtc.org/2653183004 Cr-Commit-Position: refs/heads/master@{#16662} Committed: https://chromium.googlesource.com/external/webrtc/+/9c997a3b9e33e24f3b23ae690... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:180001) as https://chromium.googlesource.com/external/webrtc/+/9c997a3b9e33e24f3b23ae690... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
