Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2337103006:
Adding FecController to audio network adaptor. (Closed)
|
Created:
4 years, 3 months ago by minyue-webrtc Modified:
4 years, 3 months ago CC:
webrtc-reviews_webrtc.org, kwiberg-webrtc, tlegrand-webrtc, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, peah-webrtc, minyue-webrtc Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdding FecController to audio network adaptor.
BUG=webrtc:6303
Committed: https://crrev.com/d0ede4493e89f1e7c3adaf3a2ad66c64578e823e
Cr-Commit-Position: refs/heads/master@{#14351}
Patch Set 1 #Patch Set 2 : rebasing #Patch Set 3 : fixing some errors on bots #
Total comments: 14
Patch Set 4 : On Henrik's comments #
Total comments: 6
Patch Set 5 : unify threshold calculation #
Total comments: 5
Patch Set 6 : final polish #
Total comments: 2
Patch Set 7 : using actual packet loss fraction even when fec off. #Patch Set 8 : rebasing #Patch Set 9 : fixing death test #
Total comments: 1
Depends on Patchset: Messages
Total messages: 43 (22 generated)
Description was changed from ========== BUG= ========== to ========== Adding FecController to audio network adaptor. BUG=webrtc:6303 ==========
minyue@webrtc.org changed reviewers: + michaelt@webrtc.org
The CQ bit was checked by minyue@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/...
Hi Michael, Please take a look at this CL, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios64_gyp_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_gyp_rel/builds/499) ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/12885) ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/16798)
The CQ bit was checked by minyue@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: android_compile_x64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x64_dbg...) linux_gyp_dbg on master.tryserver.webrtc (JOB_FAILED, no build URL)
The CQ bit was checked by minyue@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: win_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_baremetal/builds/14269)
https://codereview.webrtc.org/2337103006/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.h (right): https://codereview.webrtc.org/2337103006/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.h:96: const ThresholdInfo fec_enabling_threshold_info; should this infos be marked as members ?
minyue@webrtc.org changed reviewers: + henrik.lundin@webrtc.org
+Henrik, Hi Henrik, Michael has taken a look, and he had no major concerns. I add you now to check the CL. Thanks! https://codereview.webrtc.org/2337103006/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.h (right): https://codereview.webrtc.org/2337103006/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.h:96: const ThresholdInfo fec_enabling_threshold_info; On 2016/09/15 15:12:52, michaelt wrote: > should this infos be marked as members ? you mean to add "_", yes, of course, that was my mistake.
Nice. See comments inline. https://codereview.webrtc.org/2337103006/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.cc (right): https://codereview.webrtc.org/2337103006/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.cc:14: #include "webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.h" fec_controller.h first of all. https://codereview.webrtc.org/2337103006/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.cc:45: RTC_DCHECK_LE(fec_enabling_threshold_info.slope, 0); Are these checks enough to make sure the lines don't intersect? https://codereview.webrtc.org/2337103006/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.cc:90: slope = packet_loss_diff / bandwidth_diff_bps; How is bandwidth_diff_bps protected from being 0? https://codereview.webrtc.org/2337103006/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.h (right): https://codereview.webrtc.org/2337103006/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.h:46: // enabled. |fec_enabling_threshold| defines a curve, under which FEC should fec_enabling_threshold -> fec_disabling_threshol https://codereview.webrtc.org/2337103006/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_unittest.cc (right): https://codereview.webrtc.org/2337103006/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_unittest.cc:72: void CheckDecision(FecControllerStates* states, It is a little bit unclear to the casual reader what is actually checked here, since uplink_packet_loss is both input and expected value. Please, write a comment to clarify. https://codereview.webrtc.org/2337103006/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_unittest.cc:93: EXPECT_EQ(rtc::Optional<float>(expected_enable_fec && uplink_packet_loss EXPECT_EQ(expected_enable_fec ? uplink_packet_loss : rtc::Optional<float>(0.0), ...); Seems somewhat simpler to me.
I updated the CL The biggest change there is adding intersection detection. PTAL again. Thanks! https://codereview.webrtc.org/2337103006/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.cc (right): https://codereview.webrtc.org/2337103006/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.cc:14: #include "webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.h" On 2016/09/19 14:46:43, hlundin-webrtc wrote: > fec_controller.h first of all. Done. https://codereview.webrtc.org/2337103006/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.cc:45: RTC_DCHECK_LE(fec_enabling_threshold_info.slope, 0); On 2016/09/19 14:46:43, hlundin-webrtc wrote: > Are these checks enough to make sure the lines don't intersect? No, this cannot guarantee no-intersection. they only checks the vertical and horizontal parts of the curve. Let me try to add intersection detection. https://codereview.webrtc.org/2337103006/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.cc:90: slope = packet_loss_diff / bandwidth_diff_bps; On 2016/09/19 14:46:43, hlundin-webrtc wrote: > How is bandwidth_diff_bps protected from being 0? good catch. https://codereview.webrtc.org/2337103006/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.h (right): https://codereview.webrtc.org/2337103006/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.h:46: // enabled. |fec_enabling_threshold| defines a curve, under which FEC should On 2016/09/19 14:46:43, hlundin-webrtc wrote: > fec_enabling_threshold -> fec_disabling_threshol my bad. thanks! https://codereview.webrtc.org/2337103006/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_unittest.cc (right): https://codereview.webrtc.org/2337103006/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_unittest.cc:72: void CheckDecision(FecControllerStates* states, On 2016/09/19 14:46:43, hlundin-webrtc wrote: > It is a little bit unclear to the casual reader what is actually checked here, > since uplink_packet_loss is both input and expected value. Please, write a > comment to clarify. good point. Thanks! https://codereview.webrtc.org/2337103006/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_unittest.cc:93: EXPECT_EQ(rtc::Optional<float>(expected_enable_fec && uplink_packet_loss On 2016/09/19 14:46:43, hlundin-webrtc wrote: > EXPECT_EQ(expected_enable_fec ? uplink_packet_loss : rtc::Optional<float>(0.0), > ...); > Seems somewhat simpler to me. also to me, thanks!
two nits found by myself. https://codereview.webrtc.org/2337103006/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.cc (right): https://codereview.webrtc.org/2337103006/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.cc:43: printf("%f\n", denom); remove https://codereview.webrtc.org/2337103006/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.cc:48: printf("%f, %f\n", alpha, beta); remove
two nits found by myself.
https://codereview.webrtc.org/2337103006/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.cc (right): https://codereview.webrtc.org/2337103006/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.cc:87: // Ensure that the two middle parts of the curves do not intersect. Please, simplify this as we discussed offline. (Too complicated to write here...). Also, please add a death test to verify that the DCHECK triggers when the areas are intersecting. I think one test is enough, just to verify that it actually is working. https://codereview.webrtc.org/2337103006/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_unittest.cc (right): https://codereview.webrtc.org/2337103006/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_unittest.cc:97: EXPECT_EQ(uplink_packet_loss ? uplink_packet_loss : rtc::Optional<float>(0.0), This does not match your original expression. I think it should be EXPECT_EQ(expected_enable_fec ? uplink_packet_loss : rtc::Optional<float>(0.0),...);
Thanks for the inspiration! It gets more improvements. See new patch! https://codereview.webrtc.org/2337103006/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.cc (right): https://codereview.webrtc.org/2337103006/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.cc:87: // Ensure that the two middle parts of the curves do not intersect. On 2016/09/20 12:40:37, hlundin-webrtc wrote: > Please, simplify this as we discussed offline. (Too complicated to write > here...). > > Also, please add a death test to verify that the DCHECK triggers when the areas > are intersecting. I think one test is enough, just to verify that it actually is > working. thanks the inspiration! FecDisablingDecision and FecEnablingDecision gets also simplified by that. https://codereview.webrtc.org/2337103006/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_unittest.cc (right): https://codereview.webrtc.org/2337103006/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_unittest.cc:97: EXPECT_EQ(uplink_packet_loss ? uplink_packet_loss : rtc::Optional<float>(0.0), On 2016/09/20 12:40:37, hlundin-webrtc wrote: > This does not match your original expression. I think it should be > EXPECT_EQ(expected_enable_fec ? uplink_packet_loss : > rtc::Optional<float>(0.0),...); oh, thanks. But this is not right either. it should be expected_enable_fec && uplink_packet_loss ? ...
Much improved! Two comments on the death test. https://codereview.webrtc.org/2337103006/diff/80001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_unittest.cc (right): https://codereview.webrtc.org/2337103006/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_unittest.cc:292: TEST(FecControllerTest, DeathOnWrongConfig) { FecControllerDeathTest is the recommended test name. See https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid.... https://codereview.webrtc.org/2337103006/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_unittest.cc:292: TEST(FecControllerTest, DeathOnWrongConfig) { I think you'll have to enclose the test in #if GTEST_HAS_DEATH_TEST ... #endif because death tests are not supported on all platforms. There is another test predicate called EXPECT_DEATH_IF_SUPPORTED, but that would instead have to check for debug builds, I think.
thanks! now changed. https://codereview.webrtc.org/2337103006/diff/80001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_unittest.cc (right): https://codereview.webrtc.org/2337103006/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_unittest.cc:292: TEST(FecControllerTest, DeathOnWrongConfig) { On 2016/09/21 10:59:23, hlundin-webrtc wrote: > I think you'll have to enclose the test in > #if GTEST_HAS_DEATH_TEST > ... > #endif > because death tests are not supported on all platforms. There is another test > predicate called EXPECT_DEATH_IF_SUPPORTED, but that would instead have to check > for debug builds, I think. follow other places of DeathTest, added #if RTC_DCHECK_IS_ON && GTEST_HAS_DEATH_TEST && !defined(WEBRTC_ANDROID) https://codereview.webrtc.org/2337103006/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_unittest.cc:292: TEST(FecControllerTest, DeathOnWrongConfig) { On 2016/09/21 10:59:23, hlundin-webrtc wrote: > FecControllerDeathTest is the recommended test name. > See > https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid.... Done.
LGTM https://codereview.webrtc.org/2337103006/diff/80001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_unittest.cc (right): https://codereview.webrtc.org/2337103006/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_unittest.cc:292: TEST(FecControllerTest, DeathOnWrongConfig) { On 2016/09/21 11:46:49, minyue-webrtc wrote: > On 2016/09/21 10:59:23, hlundin-webrtc wrote: > > I think you'll have to enclose the test in > > #if GTEST_HAS_DEATH_TEST > > ... > > #endif > > because death tests are not supported on all platforms. There is another test > > predicate called EXPECT_DEATH_IF_SUPPORTED, but that would instead have to > check > > for debug builds, I think. > > follow other places of DeathTest, added > #if RTC_DCHECK_IS_ON && GTEST_HAS_DEATH_TEST && !defined(WEBRTC_ANDROID) Acknowledged.
https://codereview.webrtc.org/2337103006/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.cc (right): https://codereview.webrtc.org/2337103006/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.cc:88: config->uplink_packet_loss_fraction = rtc::Optional<float>(0.0); OPUS uses the packet loss information not only for FEC. OPUS can educing dependencies among blocks while encoding the input audio, when FEC is off but packet loss is sent to OPUS. So it would be good to send packet loss even when FEC is off.
Hi Michael, Thanks for the nice point. I uploaded a new patch set to cover that case. https://codereview.webrtc.org/2337103006/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.cc (right): https://codereview.webrtc.org/2337103006/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.cc:88: config->uplink_packet_loss_fraction = rtc::Optional<float>(0.0); On 2016/09/21 12:51:54, michaelt wrote: > OPUS uses the packet loss information not only for FEC. OPUS can educing > dependencies among blocks while encoding the input audio, when FEC is off but > packet loss is sent to OPUS. > So it would be good to send packet loss even when FEC is off. Good point!
lgtm
The CQ bit was checked by minyue@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from henrik.lundin@webrtc.org, michaelt@webrtc.org Link to the patchset: https://codereview.webrtc.org/2337103006/#ps140001 (title: "rebasing")
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: win_x64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/18030)
https://codereview.webrtc.org/2337103006/diff/160001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_unittest.cc (right): https://codereview.webrtc.org/2337103006/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_unittest.cc:298: EXPECT_DEATH( It looks like that RTC_DCHECK_IS_ON for some non-debug builds.
The CQ bit was checked by minyue@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from henrik.lundin@webrtc.org, michaelt@webrtc.org Link to the patchset: https://codereview.webrtc.org/2337103006/#ps160001 (title: "fixing death test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Adding FecController to audio network adaptor. BUG=webrtc:6303 ========== to ========== Adding FecController to audio network adaptor. BUG=webrtc:6303 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Adding FecController to audio network adaptor. BUG=webrtc:6303 ========== to ========== Adding FecController to audio network adaptor. BUG=webrtc:6303 Committed: https://crrev.com/d0ede4493e89f1e7c3adaf3a2ad66c64578e823e Cr-Commit-Position: refs/heads/master@{#14351} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/d0ede4493e89f1e7c3adaf3a2ad66c64578e823e Cr-Commit-Position: refs/heads/master@{#14351} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
