Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2066373002:
Improve encoding time calculation for Android HW encoder. (Closed)
DescriptionImprove encoding time calculation for Android HW encoder.
BUG=b/29359403
Committed: https://crrev.com/ce5a874674b6029985c2c5317feba44782435c99
Cr-Commit-Position: refs/heads/master@{#13202}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Address comments #Messages
Total messages: 18 (9 generated)
glaznev@webrtc.org changed reviewers: + sakal@webrtc.org
PTAL
Overall, looks good. https://codereview.webrtc.org/2066373002/diff/1/webrtc/api/java/jni/androidme... File webrtc/api/java/jni/androidmediaencoder_jni.cc (left): https://codereview.webrtc.org/2066373002/diff/1/webrtc/api/java/jni/androidme... webrtc/api/java/jni/androidmediaencoder_jni.cc:716: if (j_input_buffer_index == -2) { nit: Not related to your change, but while we are touching code here, might as well make this if .. else if, right? https://codereview.webrtc.org/2066373002/diff/1/webrtc/api/java/jni/androidme... File webrtc/api/java/jni/androidmediaencoder_jni.cc (right): https://codereview.webrtc.org/2066373002/diff/1/webrtc/api/java/jni/androidme... webrtc/api/java/jni/androidmediaencoder_jni.cc:712: if (frames_received_ > 1) { Why is this code needed? Do we always report one dropped frame in the beginning otherwise? Is the first frame some configuration frame that is okay to drop?
Description was changed from ========== Improve encoding time calculation for Android HW encoder. BUG=b/29359403 ========== to ========== Improve encoding time calculation for Android HW encoder. BUG=b/29359403 ==========
sakal@webrtc.org changed reviewers: + pbos@webrtc.org
lgtm https://codereview.webrtc.org/2066373002/diff/1/webrtc/api/java/jni/androidme... File webrtc/api/java/jni/androidmediaencoder_jni.cc (right): https://codereview.webrtc.org/2066373002/diff/1/webrtc/api/java/jni/androidme... webrtc/api/java/jni/androidmediaencoder_jni.cc:603: const int64_t frame_arrival_time) { frame_input_time_ms https://codereview.webrtc.org/2066373002/diff/1/webrtc/api/java/jni/androidme... webrtc/api/java/jni/androidmediaencoder_jni.cc:717: // Input buffers are not ready after codec initialization - this is .. why is this expected?
https://codereview.webrtc.org/2066373002/diff/1/webrtc/api/java/jni/androidme... File webrtc/api/java/jni/androidmediaencoder_jni.cc (left): https://codereview.webrtc.org/2066373002/diff/1/webrtc/api/java/jni/androidme... webrtc/api/java/jni/androidmediaencoder_jni.cc:716: if (j_input_buffer_index == -2) { On 2016/06/16 08:07:43, sakal wrote: > nit: Not related to your change, but while we are touching code here, might as > well make this if .. else if, right? Done. https://codereview.webrtc.org/2066373002/diff/1/webrtc/api/java/jni/androidme... File webrtc/api/java/jni/androidmediaencoder_jni.cc (right): https://codereview.webrtc.org/2066373002/diff/1/webrtc/api/java/jni/androidme... webrtc/api/java/jni/androidmediaencoder_jni.cc:603: const int64_t frame_arrival_time) { On 2016/06/16 11:32:17, pbos-webrtc wrote: > frame_input_time_ms Done. https://codereview.webrtc.org/2066373002/diff/1/webrtc/api/java/jni/androidme... webrtc/api/java/jni/androidmediaencoder_jni.cc:712: if (frames_received_ > 1) { On 2016/06/16 08:07:43, sakal wrote: > Why is this code needed? Do we always report one dropped frame in the beginning > otherwise? Is the first frame some configuration frame that is okay to drop? This is dropping of camera non encoded frame. When codec is initialized it make take some time for codec buffers to be allocated by HW. So by the time frame arrives buffers may not be ready. It is not an error/warning sign to drop frame in this case. When buffers are not available after encoding has been started then it is a bad sign and worth logging - encoder is running out of its input buffers. https://codereview.webrtc.org/2066373002/diff/1/webrtc/api/java/jni/androidme... webrtc/api/java/jni/androidmediaencoder_jni.cc:717: // Input buffers are not ready after codec initialization - this is On 2016/06/16 11:32:17, pbos-webrtc wrote: > .. why is this expected? Added more comment.
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/2066373002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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 Link to the patchset: https://codereview.webrtc.org/2066373002/#ps20001 (title: "Address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2066373002/20001
Message was sent while issue was closed.
Description was changed from ========== Improve encoding time calculation for Android HW encoder. BUG=b/29359403 ========== to ========== Improve encoding time calculation for Android HW encoder. BUG=b/29359403 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Improve encoding time calculation for Android HW encoder. BUG=b/29359403 ========== to ========== Improve encoding time calculation for Android HW encoder. BUG=b/29359403 Committed: https://crrev.com/ce5a874674b6029985c2c5317feba44782435c99 Cr-Commit-Position: refs/heads/master@{#13202} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/ce5a874674b6029985c2c5317feba44782435c99 Cr-Commit-Position: refs/heads/master@{#13202} |
