Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
3012863002:
Add stats for forced software encoder fallback for VP8. (Closed)
|
Created:
3 years, 3 months ago by åsapersson Modified:
3 years, 3 months ago CC:
webrtc-reviews_webrtc.org, video-team_agora.io, yujie_mao (webrtc), zhengzhonghou_agora.io, stefan-webrtc, tterriberry_mozilla.com, mflodman Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd stats for forced software encoder fallback for VP8.
Stats added for number of forced SW fallback changes per minute and percentage of time fallback is enabled for sent video streams:
- "WebRTC.Video.Encoder.ForcedSwFallbackChangesPerMinute.Vp8"
- "WebRTC.Video.Encoder.ForcedSwFallbackTimeInPercent.Vp8"
BUG=webrtc:6634
Review-Url: https://codereview.webrtc.org/3012863002
Cr-Commit-Position: refs/heads/master@{#19862}
Committed: https://webrtc.googlesource.com/src/+/8d75ac7e3fb0246a9d946c70237971e28bd074b9
Patch Set 1 #Patch Set 2 : add unittests #Patch Set 3 : rebase #Patch Set 4 #Patch Set 5 #
Total comments: 16
Patch Set 6 : rebase #Patch Set 7 : address comments #Patch Set 8 : address comments #Patch Set 9 : rebase #
Total comments: 4
Patch Set 10 #
Messages
Total messages: 49 (33 generated)
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #3 (id:80001) has been deleted
Description was changed from ========== Add stats for forced software encoder fallback for VP8. Stats added for number of forced SW fallback changes per minute and percentage of time fallback is enabled for sent video streams: - "WebRTC.Video.Encoder.ForcedSwFallbackChangesPerMinute.Vp8" - "WebRTC.Video.Encoder.ForcedSwFallbackTimeInPercent.Vp8" BUG=webrtc:6634 ========== to ========== Add stats for forced software encoder fallback for VP8. Stats added for number of forced SW fallback changes per minute and percentage of time fallback is enabled for sent video streams: - "WebRTC.Video.Encoder.ForcedSwFallbackChangesPerMinute.Vp8" - "WebRTC.Video.Encoder.ForcedSwFallbackTimeInPercent.Vp8" BUG=webrtc:6634 ==========
asapersson@webrtc.org changed reviewers: + brandtr@webrtc.org
Patchset #4 (id:120001) has been deleted
lgtm with some comments. https://codereview.webrtc.org/3012863002/diff/160001/webrtc/video/send_statis... File webrtc/video/send_statistics_proxy.cc (right): https://codereview.webrtc.org/3012863002/diff/160001/webrtc/video/send_statis... webrtc/video/send_statistics_proxy.cc:1: /* It looks like this member variable is unused: https://cs.chromium.org/chromium/src/third_party/webrtc/media/engine/videoenc... Would you mind removing it here, or in a followup CL? To avoid confusion about how the implementation name is set for the SW fallback. https://codereview.webrtc.org/3012863002/diff/160001/webrtc/video/send_statis... webrtc/video/send_statistics_proxy.cc:698: // The minimum set time should have passed for the first fallback. Can you extend this comment to explain why it's important to not count this case? https://codereview.webrtc.org/3012863002/diff/160001/webrtc/video/send_statis... webrtc/video/send_statistics_proxy.cc:713: // video is considered paused and the change is not included. Is this to avoid recording real failures, where it takes a long time before the HW wrapper returns an error code? https://codereview.webrtc.org/3012863002/diff/160001/webrtc/video/send_statis... webrtc/video/send_statistics_proxy.cc:738: UpdateEncoderFallbackStats(codec_info, &uma_container_->fallback_info_); Is there a benefit to passing in |fallback_info_| as a pointer, instead of just using it as a member in UpdateEncoderFallbackStats? https://codereview.webrtc.org/3012863002/diff/160001/webrtc/video/send_statis... File webrtc/video/send_statistics_proxy.h (right): https://codereview.webrtc.org/3012863002/diff/160001/webrtc/video/send_statis... webrtc/video/send_statistics_proxy.h:187: EXCLUSIVE_LOCKS_REQUIRED(crit_); I think you have to rebase this to use the RTC_ prefixed version now. https://codereview.webrtc.org/3012863002/diff/160001/webrtc/video/send_statis... File webrtc/video/send_statistics_proxy_unittest.cc (right): https://codereview.webrtc.org/3012863002/diff/160001/webrtc/video/send_statis... webrtc/video/send_statistics_proxy_unittest.cc:1818: Add unit test checking that FallbackTimeInPercent.Vp8 is not outputed when the field trial is not set. https://codereview.webrtc.org/3012863002/diff/160001/webrtc/video/send_statis... webrtc/video/send_statistics_proxy_unittest.cc:1841: statistics_proxy_->OnSendEncodedImage(encoded_image_, &codec_info_); Add comment. https://codereview.webrtc.org/3012863002/diff/160001/webrtc/video/send_statis... webrtc/video/send_statistics_proxy_unittest.cc:1905: codec_info_.codecSpecific.VP8.temporalIdx = kNoTemporalIdx; Delete this line?
https://codereview.webrtc.org/3012863002/diff/160001/webrtc/video/send_statis... File webrtc/video/send_statistics_proxy.cc (right): https://codereview.webrtc.org/3012863002/diff/160001/webrtc/video/send_statis... webrtc/video/send_statistics_proxy.cc:1: /* On 2017/09/14 12:30:11, brandtr wrote: > It looks like this member variable is unused: > https://cs.chromium.org/chromium/src/third_party/webrtc/media/engine/videoenc... > > Would you mind removing it here, or in a followup CL? To avoid confusion about > how the implementation name is set for the SW fallback. Ok, will remove in a separate CL. https://codereview.webrtc.org/3012863002/diff/160001/webrtc/video/send_statis... webrtc/video/send_statistics_proxy.cc:698: // The minimum set time should have passed for the first fallback. On 2017/09/14 12:30:11, brandtr wrote: > Can you extend this comment to explain why it's important to not count this > case? Done. https://codereview.webrtc.org/3012863002/diff/160001/webrtc/video/send_statis... webrtc/video/send_statistics_proxy.cc:713: // video is considered paused and the change is not included. On 2017/09/14 12:30:11, brandtr wrote: > Is this to avoid recording real failures, where it takes a long time before the > HW wrapper returns an error code? Updated comment a bit, it is to exclude time when video is muted. https://codereview.webrtc.org/3012863002/diff/160001/webrtc/video/send_statis... webrtc/video/send_statistics_proxy.cc:738: UpdateEncoderFallbackStats(codec_info, &uma_container_->fallback_info_); On 2017/09/14 12:30:12, brandtr wrote: > Is there a benefit to passing in |fallback_info_| as a pointer, instead of just > using it as a member in UpdateEncoderFallbackStats? No, removed. https://codereview.webrtc.org/3012863002/diff/160001/webrtc/video/send_statis... File webrtc/video/send_statistics_proxy.h (right): https://codereview.webrtc.org/3012863002/diff/160001/webrtc/video/send_statis... webrtc/video/send_statistics_proxy.h:187: EXCLUSIVE_LOCKS_REQUIRED(crit_); On 2017/09/14 12:30:12, brandtr wrote: > I think you have to rebase this to use the RTC_ prefixed version now. Done. https://codereview.webrtc.org/3012863002/diff/160001/webrtc/video/send_statis... File webrtc/video/send_statistics_proxy_unittest.cc (right): https://codereview.webrtc.org/3012863002/diff/160001/webrtc/video/send_statis... webrtc/video/send_statistics_proxy_unittest.cc:1818: On 2017/09/14 12:30:12, brandtr wrote: > Add unit test checking that FallbackTimeInPercent.Vp8 is not outputed when the > field trial is not set. Done. https://codereview.webrtc.org/3012863002/diff/160001/webrtc/video/send_statis... webrtc/video/send_statistics_proxy_unittest.cc:1841: statistics_proxy_->OnSendEncodedImage(encoded_image_, &codec_info_); On 2017/09/14 12:30:12, brandtr wrote: > Add comment. Done. https://codereview.webrtc.org/3012863002/diff/160001/webrtc/video/send_statis... webrtc/video/send_statistics_proxy_unittest.cc:1905: codec_info_.codecSpecific.VP8.temporalIdx = kNoTemporalIdx; On 2017/09/14 12:30:12, brandtr wrote: > Delete this line? Added comment..
lgtm
The CQ bit was checked by asapersson@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/...
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 asapersson@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from brandtr@webrtc.org Link to the patchset: https://codereview.webrtc.org/3012863002/#ps220001 (title: "address comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/21661)
The CQ bit was checked by asapersson@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/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/21664)
The CQ bit was checked by asapersson@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from brandtr@webrtc.org Link to the patchset: https://codereview.webrtc.org/3012863002/#ps240001 (title: "rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/21678)
The CQ bit was checked by asapersson@webrtc.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/21698)
The CQ bit was checked by asapersson@webrtc.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/21703)
mbonadei@webrtc.org changed reviewers: + mbonadei@webrtc.org
https://codereview.webrtc.org/3012863002/diff/240001/video/send_statistics_pr... File video/send_statistics_proxy.cc (right): https://codereview.webrtc.org/3012863002/diff/240001/video/send_statistics_pr... video/send_statistics_proxy.cc:18: #include "common_types.h" Please change this line with: #include "common_types.h" // NOLINT(build/include) https://codereview.webrtc.org/3012863002/diff/240001/video/send_statistics_pr... File video/send_statistics_proxy.h (right): https://codereview.webrtc.org/3012863002/diff/240001/video/send_statistics_pr... video/send_statistics_proxy.h:19: #include "common_types.h" Please change this line with: #include "common_types.h" // NOLINT(build/include)
The CQ bit was checked by asapersson@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/...
https://codereview.webrtc.org/3012863002/diff/240001/video/send_statistics_pr... File video/send_statistics_proxy.cc (right): https://codereview.webrtc.org/3012863002/diff/240001/video/send_statistics_pr... video/send_statistics_proxy.cc:18: #include "common_types.h" On 2017/09/15 11:36:34, mbonadei wrote: > Please change this line with: > #include "common_types.h" // NOLINT(build/include) Done. https://codereview.webrtc.org/3012863002/diff/240001/video/send_statistics_pr... File video/send_statistics_proxy.h (right): https://codereview.webrtc.org/3012863002/diff/240001/video/send_statistics_pr... video/send_statistics_proxy.h:19: #include "common_types.h" On 2017/09/15 11:36:34, mbonadei wrote: > Please change this line with: > #include "common_types.h" // NOLINT(build/include) Done.
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 asapersson@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from brandtr@webrtc.org Link to the patchset: https://codereview.webrtc.org/3012863002/#ps260001 (title: "")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 260001, "attempt_start_ts": 1505482708887640,
"parent_rev": "34f303cf58178d3602bca924e7ae78cd2f0eb8f1", "commit_rev":
"8d75ac7e3fb0246a9d946c70237971e28bd074b9"}
Message was sent while issue was closed.
Description was changed from ========== Add stats for forced software encoder fallback for VP8. Stats added for number of forced SW fallback changes per minute and percentage of time fallback is enabled for sent video streams: - "WebRTC.Video.Encoder.ForcedSwFallbackChangesPerMinute.Vp8" - "WebRTC.Video.Encoder.ForcedSwFallbackTimeInPercent.Vp8" BUG=webrtc:6634 ========== to ========== Add stats for forced software encoder fallback for VP8. Stats added for number of forced SW fallback changes per minute and percentage of time fallback is enabled for sent video streams: - "WebRTC.Video.Encoder.ForcedSwFallbackChangesPerMinute.Vp8" - "WebRTC.Video.Encoder.ForcedSwFallbackTimeInPercent.Vp8" BUG=webrtc:6634 Review-Url: https://codereview.webrtc.org/3012863002 Cr-Commit-Position: refs/heads/master@{#19862} Committed: https://webrtc.googlesource.com/src/+/8d75ac7e3fb0246a9d946c70237971e28bd074b9 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:260001) as https://webrtc.googlesource.com/src/+/8d75ac7e3fb0246a9d946c70237971e28bd074b9 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
