Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2051403002:
Always on statistics for AndroidMediaEncoder. (Closed)
DescriptionAlways on statistics for AndroidMediaEncoder.
Earlier, no statistics were reported if no frames were being delivered
for encoding. This makes statics always be reported regardless of if
there are frames being delivered to the encoder.
Committed: https://crrev.com/1fc48100064f5a46cd0254bac898fdbff0f8ab4d
Cr-Commit-Position: refs/heads/master@{#13122}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Remove DeliverPendingOuputs statistics #
Total comments: 6
Patch Set 3 : Changes according to AlexG's comments #
Total comments: 1
Patch Set 4 : Change negative condition into a positive one. #Patch Set 5 : Rebase #
Messages
Total messages: 36 (19 generated)
Description was changed from ========== Always on statistics for AndroidMediaEncoder. Earlier, no statistics were reported if no frames were being delivered for encoding. This makes statics always be reported regardless of if there are frames being delivered to the encoder. I also added statistics for how many times DeliverPendingOutputs has been called in the last statistics interval since polling code is now more complicated. ========== to ========== Always on statistics for AndroidMediaEncoder. Earlier, no statistics were reported if no frames were being delivered for encoding. This makes statics always be reported regardless of if there are frames being delivered to the encoder. I also added statistics for how many times DeliverPendingOutputs has been called in the last statistics interval since polling code is now more complicated. ==========
sakal@webrtc.org changed reviewers: + glaznev@webrtc.org, perkj@webrtc.org
On 2016/06/10 09:07:20, sakal wrote: > mailto:sakal@webrtc.org changed reviewers: > + mailto:glaznev@webrtc.org, mailto:perkj@webrtc.org Perkj and glaznev, please take a look.
I don't think you need the extra stats for how many times DeliverPendingOutputs has been called. Please just remove it and update the description. lgtm % that. https://codereview.webrtc.org/2051403002/diff/1/webrtc/api/java/jni/androidme... File webrtc/api/java/jni/androidmediaencoder_jni.cc (right): https://codereview.webrtc.org/2051403002/diff/1/webrtc/api/java/jni/androidme... webrtc/api/java/jni/androidmediaencoder_jni.cc:1153: ", DeliverPendingOutputs: " << deliver_pending_outputs_calls_ << I don't think we care about how many times DeliverPendingOutputs has actually been called. Please just remove. https://codereview.webrtc.org/2051403002/diff/1/webrtc/api/java/jni/androidme... webrtc/api/java/jni/androidmediaencoder_jni.cc:1159: deliver_pending_outputs_calls_ = 0; And remove DeliverPendingOutputs.
Description was changed from ========== Always on statistics for AndroidMediaEncoder. Earlier, no statistics were reported if no frames were being delivered for encoding. This makes statics always be reported regardless of if there are frames being delivered to the encoder. I also added statistics for how many times DeliverPendingOutputs has been called in the last statistics interval since polling code is now more complicated. ========== to ========== Always on statistics for AndroidMediaEncoder. Earlier, no statistics were reported if no frames were being delivered for encoding. This makes statics always be reported regardless of if there are frames being delivered to the encoder. ==========
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/patch-status/2051403002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/14067)
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/patch-status/2051403002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/14078)
https://codereview.webrtc.org/2051403002/diff/20001/webrtc/api/java/jni/andro... File webrtc/api/java/jni/androidmediaencoder_jni.cc (right): https://codereview.webrtc.org/2051403002/diff/20001/webrtc/api/java/jni/andro... webrtc/api/java/jni/androidmediaencoder_jni.cc:485: codec_thread_->PostDelayed(kMediaCodecStatisticsIntervalMs, this); I think this stopping or increasing polling interval may be dangerous for the case when encoder generates 2 frames for a single output - suppose for H.264 encoder it generates first frame with SPS and PPS NAL and then actual encoded frame. Or something happens in HW that it by error generates 2 outputs. I suggest instead of complete polling stop or increasing polling interval to 3 second increase it to 50 or 100 ms - it should not affect the performance, but will make code more robust against encoder mis behaviour. https://codereview.webrtc.org/2051403002/diff/20001/webrtc/api/java/jni/andro... webrtc/api/java/jni/androidmediaencoder_jni.cc:606: codec_thread_->PostDelayed(kMediaCodecStatisticsIntervalMs, this); This is not needed - first 10 frames are logged, so we can detect jam for them. https://codereview.webrtc.org/2051403002/diff/20001/webrtc/api/java/jni/andro... webrtc/api/java/jni/androidmediaencoder_jni.cc:752: if (!output_delivery_loop_running_) { It is safer and less code to just remove this if condition - always schedule DeliverPendingOutptu when input frame arrives
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
https://codereview.webrtc.org/2051403002/diff/20001/webrtc/api/java/jni/andro... File webrtc/api/java/jni/androidmediaencoder_jni.cc (right): https://codereview.webrtc.org/2051403002/diff/20001/webrtc/api/java/jni/andro... webrtc/api/java/jni/androidmediaencoder_jni.cc:485: codec_thread_->PostDelayed(kMediaCodecStatisticsIntervalMs, this); On 2016/06/10 20:16:25, AlexG wrote: > I think this stopping or increasing polling interval may be dangerous for the > case when encoder generates 2 frames for a single output - suppose for H.264 > encoder it generates first frame with SPS and PPS NAL and then actual encoded > frame. Or something happens in HW that it by error generates 2 outputs. I > suggest instead of complete polling stop or increasing polling interval to 3 > second increase it to 50 or 100 ms - it should not affect the performance, but > will make code more robust against encoder mis behaviour. Done. https://codereview.webrtc.org/2051403002/diff/20001/webrtc/api/java/jni/andro... webrtc/api/java/jni/androidmediaencoder_jni.cc:606: codec_thread_->PostDelayed(kMediaCodecStatisticsIntervalMs, this); On 2016/06/10 20:16:25, AlexG wrote: > This is not needed - first 10 frames are logged, so we can detect jam for them. Done. https://codereview.webrtc.org/2051403002/diff/20001/webrtc/api/java/jni/andro... webrtc/api/java/jni/androidmediaencoder_jni.cc:752: if (!output_delivery_loop_running_) { On 2016/06/10 20:16:25, AlexG wrote: > It is safer and less code to just remove this if condition - always schedule > DeliverPendingOutptu when input frame arrives Done.
lgtm https://codereview.webrtc.org/2051403002/diff/100001/webrtc/api/java/jni/andr... File webrtc/api/java/jni/androidmediaencoder_jni.cc (right): https://codereview.webrtc.org/2051403002/diff/100001/webrtc/api/java/jni/andr... webrtc/api/java/jni/androidmediaencoder_jni.cc:478: if (!input_frame_infos_.empty()) { nit: better to use positive condition if (input_frame_infos_.empty()) ... else ...
The CQ bit was checked by sakal@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from perkj@webrtc.org, glaznev@webrtc.org Link to the patchset: https://codereview.webrtc.org/2051403002/#ps120001 (title: "Change negative condition into a positive one.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2051403002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_arm64_rel/build...) android_clang_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_clang_dbg/build...) android_compile_x64_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x64_dbg...) ios64_gn_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_gn_dbg/builds/492) ios64_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_gn_rel/builds/495) ios64_sim_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/8235) ios_api_framework on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_api_framework/build...) ios_arm64_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/10550) ios_arm64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/10475) ios_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/15777) ios_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/14424) linux_asan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/15453) linux_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_rel/builds/15894) linux_ubsan_vptr on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...) mac_baremetal on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/12076) mac_compile_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...) mac_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_rel/builds/10158) win_x64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/15443)
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/patch-status/2051403002/140001
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 sakal@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from glaznev@webrtc.org, perkj@webrtc.org Link to the patchset: https://codereview.webrtc.org/2051403002/#ps140001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2051403002/140001
Message was sent while issue was closed.
Description was changed from ========== Always on statistics for AndroidMediaEncoder. Earlier, no statistics were reported if no frames were being delivered for encoding. This makes statics always be reported regardless of if there are frames being delivered to the encoder. ========== to ========== Always on statistics for AndroidMediaEncoder. Earlier, no statistics were reported if no frames were being delivered for encoding. This makes statics always be reported regardless of if there are frames being delivered to the encoder. ==========
Message was sent while issue was closed.
Committed patchset #5 (id:140001)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== Always on statistics for AndroidMediaEncoder. Earlier, no statistics were reported if no frames were being delivered for encoding. This makes statics always be reported regardless of if there are frames being delivered to the encoder. ========== to ========== Always on statistics for AndroidMediaEncoder. Earlier, no statistics were reported if no frames were being delivered for encoding. This makes statics always be reported regardless of if there are frames being delivered to the encoder. Committed: https://crrev.com/1fc48100064f5a46cd0254bac898fdbff0f8ab4d Cr-Commit-Position: refs/heads/master@{#13122} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/1fc48100064f5a46cd0254bac898fdbff0f8ab4d Cr-Commit-Position: refs/heads/master@{#13122} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
