Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2997363002:
Add experiment to disable ulpfec. (Closed)
|
Created:
3 years, 4 months ago by stefan-webrtc Modified:
3 years, 4 months ago Reviewers:
brandtr CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhuangzesen_agora.io, danilchap, stefan-webrtc, mflodman Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd experiment to disable ulpfec.
BUG=None
Review-Url: https://codereview.webrtc.org/2997363002
Cr-Commit-Position: refs/heads/master@{#19471}
Committed: https://chromium.googlesource.com/external/webrtc/+/60e10c794e8d5740e7295a2e0f0bc697eecdaa3b
Patch Set 1 #Patch Set 2 : . #
Total comments: 5
Patch Set 3 : Comments addressed. #Patch Set 4 : Cleanup #
Total comments: 6
Patch Set 5 : Comments addressed. #Messages
Total messages: 20 (7 generated)
stefan@webrtc.org changed reviewers: + brandtr@webrtc.org
https://codereview.webrtc.org/2997363002/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/ulpfec_generator.cc (right): https://codereview.webrtc.org/2997363002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/ulpfec_generator.cc:65: webrtc::field_trial::FindFullName(kDisableUlpfecExperiment); You can use this function instead: https://cs.chromium.org/chromium/src/third_party/webrtc/system_wrappers/inclu... https://codereview.webrtc.org/2997363002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/ulpfec_generator.cc:159: if (ulpfec_disabled_) Is it better to disable ULPFEC here, instead of somewhere higher up? E.g, where ULPFEC is configured: https://cs.chromium.org/chromium/src/third_party/webrtc/video/video_send_stre... If disabled there, ProtectionBitrateCalculator et. al will know that sending FEC is disabled. But it might be harder to test.
https://codereview.webrtc.org/2997363002/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/ulpfec_generator.cc (right): https://codereview.webrtc.org/2997363002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/ulpfec_generator.cc:159: if (ulpfec_disabled_) On 2017/08/22 14:58:08, brandtr wrote: > Is it better to disable ULPFEC here, instead of somewhere higher up? E.g, where > ULPFEC is configured: > https://cs.chromium.org/chromium/src/third_party/webrtc/video/video_send_stre... > > If disabled there, ProtectionBitrateCalculator et. al will know that sending FEC > is disabled. But it might be harder to test. Yes that's true. I'll consider that tomorrow.
Comments addressed.
Cleanup
https://codereview.webrtc.org/2997363002/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/ulpfec_generator.cc (right): https://codereview.webrtc.org/2997363002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/ulpfec_generator.cc:65: webrtc::field_trial::FindFullName(kDisableUlpfecExperiment); On 2017/08/22 14:58:08, brandtr wrote: > You can use this function instead: > https://cs.chromium.org/chromium/src/third_party/webrtc/system_wrappers/inclu... Done. https://codereview.webrtc.org/2997363002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/ulpfec_generator.cc:159: if (ulpfec_disabled_) On 2017/08/22 14:58:08, brandtr wrote: > Is it better to disable ULPFEC here, instead of somewhere higher up? E.g, where > ULPFEC is configured: > https://cs.chromium.org/chromium/src/third_party/webrtc/video/video_send_stre... > > If disabled there, ProtectionBitrateCalculator et. al will know that sending FEC > is disabled. But it might be harder to test. Done.
lgtm, but added some suggestions https://codereview.webrtc.org/2997363002/diff/60001/webrtc/video/video_send_s... File webrtc/video/video_send_stream.cc (right): https://codereview.webrtc.org/2997363002/diff/60001/webrtc/video/video_send_s... webrtc/video/video_send_stream.cc:991: LOG(LS_INFO) << "Experiment to disable FlexFEC is enabled."; FlexFEC -> Ulpfec. For clarity, mention that we only disable _sending_ Ulpfec. We will still negotiate it, so we might still receive it. https://codereview.webrtc.org/2997363002/diff/60001/webrtc/video/video_send_s... webrtc/video/video_send_stream.cc:1053: for (RtpRtcp* rtp_rtcp : rtp_rtcp_modules_) { Not your change, but would you mind removing this unnecessary double loop? :) https://codereview.webrtc.org/2997363002/diff/60001/webrtc/video/video_send_s... File webrtc/video/video_send_stream_tests.cc (right): https://codereview.webrtc.org/2997363002/diff/60001/webrtc/video/video_send_s... webrtc/video/video_send_stream_tests.cc:565: protected: Maybe put the ScopedFieldTrials as a member of this class, so you don't have to make changes to VideoSendStreamTest?
Comments addressed.
https://codereview.webrtc.org/2997363002/diff/60001/webrtc/video/video_send_s... File webrtc/video/video_send_stream.cc (right): https://codereview.webrtc.org/2997363002/diff/60001/webrtc/video/video_send_s... webrtc/video/video_send_stream.cc:991: LOG(LS_INFO) << "Experiment to disable FlexFEC is enabled."; On 2017/08/23 14:10:44, brandtr wrote: > FlexFEC -> Ulpfec. > > For clarity, mention that we only disable _sending_ Ulpfec. We will still > negotiate it, so we might still receive it. Done. https://codereview.webrtc.org/2997363002/diff/60001/webrtc/video/video_send_s... webrtc/video/video_send_stream.cc:1053: for (RtpRtcp* rtp_rtcp : rtp_rtcp_modules_) { On 2017/08/23 14:10:44, brandtr wrote: > Not your change, but would you mind removing this unnecessary double loop? :) Yes! https://codereview.webrtc.org/2997363002/diff/60001/webrtc/video/video_send_s... File webrtc/video/video_send_stream_tests.cc (right): https://codereview.webrtc.org/2997363002/diff/60001/webrtc/video/video_send_s... webrtc/video/video_send_stream_tests.cc:565: protected: On 2017/08/23 14:10:44, brandtr wrote: > Maybe put the ScopedFieldTrials as a member of this class, so you don't have to > make changes to VideoSendStreamTest? Yes, I don't know why I did it this way.
The CQ bit was checked by stefan@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/2997363002/#ps80001 (title: "Comments addressed.")
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: android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) android_compile_mips_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) android_compile_x64_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) android_compile_x86_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) android_compile_x86_rel on master.tryserver.webrtc (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) android_more_configs on master.tryserver.webrtc (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) android_rel on master.tryserver.webrtc (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux32_arm_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_arm64_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_asan on master.tryserver.webrtc (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_compile_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_gcc_rel on master.tryserver.webrtc (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_libfuzzer_rel on master.tryserver.webrtc (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_memcheck on master.tryserver.webrtc (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_more_configs on master.tryserver.webrtc (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_msan on master.tryserver.webrtc (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_rel on master.tryserver.webrtc (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_tsan2 on master.tryserver.webrtc (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_ubsan on master.tryserver.webrtc (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_ubsan_vptr on master.tryserver.webrtc (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) win_baremetal on master.tryserver.webrtc (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?))
The CQ bit was checked by stefan@webrtc.org
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": 80001, "attempt_start_ts": 1503505883109030,
"parent_rev": "45ca37c02214531b3fe88b8b0cc2cc0d36571b8b", "commit_rev":
"60e10c794e8d5740e7295a2e0f0bc697eecdaa3b"}
Message was sent while issue was closed.
Description was changed from ========== Add experiment to disable ulpfec. BUG=None ========== to ========== Add experiment to disable ulpfec. BUG=None Review-Url: https://codereview.webrtc.org/2997363002 Cr-Commit-Position: refs/heads/master@{#19471} Committed: https://chromium.googlesource.com/external/webrtc/+/60e10c794e8d5740e7295a2e0... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/external/webrtc/+/60e10c794e8d5740e7295a2e0... |
