Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2973363002:
Explicitly inform PacketRouter which RTP-RTCP modules are REMB-candidates (Closed)
|
Created:
3 years, 5 months ago by eladalon Modified:
3 years, 4 months ago CC:
webrtc-reviews_webrtc.org, video-team_agora.io, yujie_mao (webrtc), zhuangzesen_agora.io, zhengzhonghou_agora.io, stefan-webrtc, tterriberry_mozilla.com, mflodman Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionExplicitly inform PacketRouter which RTP-RTCP modules are REMB-candidates
BUG=webrtc:7860
Review-Url: https://codereview.webrtc.org/2973363002
Cr-Commit-Position: refs/heads/master@{#19201}
Committed: https://chromium.googlesource.com/external/webrtc/+/822ff2b794eb676541ef3cc6d5c7bc7380a590cc
Patch Set 1 #Patch Set 2 : Move |active_remb_module_ = nullptr| to correct location. #Patch Set 3 : Remove default values from functions. #Patch Set 4 : 1. Thread/lock annotations. 2. Starting to add UTs. #Patch Set 5 : Unit-tests added. #
Total comments: 2
Patch Set 6 : . #
Total comments: 28
Patch Set 7 : . #
Total comments: 15
Patch Set 8 : CR response #Patch Set 9 : . #
Messages
Total messages: 29 (6 generated)
Description was changed from ========== Explicitly inform PacketRouter which RTP-RTCP modules are REMB-candidates BUG=webrtc:7860 ========== to ========== Explicitly inform PacketRouter which RTP-RTCP modules are REMB-candidates BUG=webrtc:7860 ==========
eladalon@webrtc.org changed reviewers: + danilchap@webrtc.org, nisse@webrtc.org, sprang@webrtc.org, stefan@webrtc.org
PTAL
I don't have the full context here, but it looks a bit odd to hard code remb candidate status based media type alone. Can't we set this status based on if the abs send time extension is registered?
On 2017/07/12 09:11:27, sprang_webrtc wrote:
> I don't have the full context here, but it looks a bit odd to hard code remb
> candidate status based media type alone. Can't we set this status based on if
> the abs send time extension is registered?
Niels has the following TODO in the code:
// TODO(nisse): This remb setting is currently set but never
// applied. REMB logic is now the responsibility of
// PacketRouter, and it will generate REMB feedback if
// OnReceiveBitrateChanged is used, which depends on how the
// estimators belonging to the ReceiveSideCongestionController
// are configured. Decide if this setting should be deleted, and
// if it needs to be replaced by a setting in PacketRouter to
// disable REMB feedback.
I'll wait until I can discuss this with him before doing more changes.
On 2017/07/12 09:11:27, sprang_webrtc wrote: > I don't have the full context here, but it looks a bit odd to hard code remb > candidate status based media type alone. Can't we set this status based on if > the abs send time extension is registered? VideoReceiveStream::Config::remb may control if it is ok to use an ssrc to send remb.
https://codereview.webrtc.org/2973363002/diff/80001/webrtc/modules/pacing/pac... File webrtc/modules/pacing/packet_router.h (right): https://codereview.webrtc.org/2973363002/diff/80001/webrtc/modules/pacing/pac... webrtc/modules/pacing/packet_router.h:53: void AddSendRtpModule(RtpRtcp* rtp_module, bool remb_candidate); overload to make the change backward compatible. (but feel free to mark old signature RTC_DEPRECATED to discourage it) Though it might be better to add SupportsRemb / SupportsTransportCC to the RtpRtcp interface itself to avoid introducing odd parameters.
https://codereview.webrtc.org/2973363002/diff/80001/webrtc/modules/pacing/pac... File webrtc/modules/pacing/packet_router.h (right): https://codereview.webrtc.org/2973363002/diff/80001/webrtc/modules/pacing/pac... webrtc/modules/pacing/packet_router.h:53: void AddSendRtpModule(RtpRtcp* rtp_module, bool remb_candidate); On 2017/07/19 12:47:20, danilchap wrote: > overload to make the change backward compatible. > (but feel free to mark old signature RTC_DEPRECATED to discourage it) > > Though it might be better to add SupportsRemb / SupportsTransportCC to the > RtpRtcp interface itself to avoid introducing odd parameters. Done. (For posterity - keeping remb_candidate because the class currently manages REMB transmission.)
https://codereview.webrtc.org/2973363002/diff/100001/webrtc/modules/pacing/pa... File webrtc/modules/pacing/packet_router.cc (right): https://codereview.webrtc.org/2973363002/diff/100001/webrtc/modules/pacing/pa... webrtc/modules/pacing/packet_router.cc:248: // TODO(eladalon): !!! Before my change, RTX-enabled modules were also I can't see any connection between rtx and remb. (except that they both make sense for video and not so much for audio). i.e. I do not find that intentional, no reason to keep that. https://codereview.webrtc.org/2973363002/diff/100001/webrtc/modules/pacing/pa... File webrtc/modules/pacing/packet_router_unittest.cc (right): https://codereview.webrtc.org/2973363002/diff/100001/webrtc/modules/pacing/pa... webrtc/modules/pacing/packet_router_unittest.cc:74: // returns true even when no modules are found that match the SSRC. probably it mean that module for the packet was just removed. i.e. it is right thing to drop the packet. TimeToSendPacket probably should return void. https://codereview.webrtc.org/2973363002/diff/100001/webrtc/modules/pacing/pa... webrtc/modules/pacing/packet_router_unittest.cc:782: // TODO(eladalon): Tests for a module which is both receive as well as send? do not think we have those modules. At least for video.
https://codereview.webrtc.org/2973363002/diff/100001/webrtc/modules/pacing/pa... File webrtc/modules/pacing/packet_router.cc (right): https://codereview.webrtc.org/2973363002/diff/100001/webrtc/modules/pacing/pa... webrtc/modules/pacing/packet_router.cc:248: // TODO(eladalon): !!! Before my change, RTX-enabled modules were also On 2017/07/27 20:00:19, danilchap wrote: > I can't see any connection between rtx and remb. (except that they both make > sense for video and not so much for audio). > i.e. I do not find that intentional, no reason to keep that. Yeah, I couldn't think of a reason, but did not feel comfortable just chucking that part away without getting your or Niels to have a look. Thanks. https://codereview.webrtc.org/2973363002/diff/100001/webrtc/modules/pacing/pa... File webrtc/modules/pacing/packet_router_unittest.cc (right): https://codereview.webrtc.org/2973363002/diff/100001/webrtc/modules/pacing/pa... webrtc/modules/pacing/packet_router_unittest.cc:74: // returns true even when no modules are found that match the SSRC. On 2017/07/27 20:00:20, danilchap wrote: > probably it mean that module for the packet was just removed. > i.e. it is right thing to drop the packet. > TimeToSendPacket probably should return void. PacedSender::SendPacket uses that, so even if we end up removing it, that's work for a separate CL. I have the TODO there because it looks suspicious. If we look at that aforementioned SendPacket(), we can see the following (where |success| is the return value of TimeToSendPacket): if (success) { if (packet.priority != kHighPriority) { // Update media bytes sent. UpdateBudgetWithBytesSent(packet.bytes); } It seems suspicious to me that a packet that did not end up being sent anywhere could lead to an updating of a budget, and merits further investigation. Then again, it might also be that this is all highly theoretical, because the SSRC not matching might not be possible. Wdyt about filing a bug for this and removing this TODO from this CL? https://codereview.webrtc.org/2973363002/diff/100001/webrtc/modules/pacing/pa... webrtc/modules/pacing/packet_router_unittest.cc:782: // TODO(eladalon): Tests for a module which is both receive as well as send? On 2017/07/27 20:00:20, danilchap wrote: > do not think we have those modules. At least for video. Acknowledged; will remove this TODO with the next revision (which I'll upload after I have your feedback on the other comment).
https://codereview.webrtc.org/2973363002/diff/100001/webrtc/modules/pacing/pa... File webrtc/modules/pacing/packet_router.cc (right): https://codereview.webrtc.org/2973363002/diff/100001/webrtc/modules/pacing/pa... webrtc/modules/pacing/packet_router.cc:257: RtpRtcp* previous_active_remb_module_ = active_remb_module_; remove last _ (it is local variable, not a member variable) https://codereview.webrtc.org/2973363002/diff/100001/webrtc/modules/pacing/pa... File webrtc/modules/pacing/packet_router.h (right): https://codereview.webrtc.org/2973363002/diff/100001/webrtc/modules/pacing/pa... webrtc/modules/pacing/packet_router.h:56: constexpr bool remb_candidate = true; // Default to old behavior. May be remove this line: with called function just one line above, repeating parameter name with local variable is probably overkill. Comment feels like stating the obvious: to be backward compatible you have to default to old behavior. https://codereview.webrtc.org/2973363002/diff/100001/webrtc/modules/pacing/pa... webrtc/modules/pacing/packet_router.h:107: // TODO(eladalon): remb_crit_ only ever held from one function, and it's not you addressing it in another CL already, so probably remove the TODO from this one. https://codereview.webrtc.org/2973363002/diff/100001/webrtc/modules/pacing/pa... File webrtc/modules/pacing/packet_router_unittest.cc (right): https://codereview.webrtc.org/2973363002/diff/100001/webrtc/modules/pacing/pa... webrtc/modules/pacing/packet_router_unittest.cc:39: EXPECT_CALL(*mock_rtp_rtcp, SetREMBStatus(remb_state)) EXPECT_CALL in helper complicates reading error message when it fails - error message will contain line number of the helper instead of line number where expectation was set. https://codereview.webrtc.org/2973363002/diff/100001/webrtc/modules/pacing/pa... webrtc/modules/pacing/packet_router_unittest.cc:48: void OnSetREMBStatusAndSetRembAccordingly(MockRtpRtcp* mock_rtp_rtcp) { probably cleaner to use different Mock instead of free functions: class MockRtpRtcpRemb : public MockRtpRtcp { public: MockRtpRtcpRemb() { ON_CALL(*this, SetREMBStatus(_)).WillByDefault(SaveArg<0>(&remb_)); ON_CALL(*this, REMB()).WillByDefault(ReturnPointee(&remb_)); } private: bool remb_ = false; }; TEST(...) { // both EXPECT_CALL(module, SetRembStatus(false)) before call // and EXPECT_FALSE(module->REMB()) after the call with check what you need. // personally would prefer the latter: expect module end-up in correct remb state rather than how it was changed to it. // or, like you already doing, test with EXPECT_CALL(, SetREMBData) removing mentioning GetRembStatus/SetRembStatus from individual tests. } For more details: https://github.com/google/googletest/blob/master/googlemock/docs/CookBook.md#... https://codereview.webrtc.org/2973363002/diff/100001/webrtc/modules/pacing/pa... webrtc/modules/pacing/packet_router_unittest.cc:64: TEST_F(PacketRouterTest, Sanity_NoModuleRegistered_TimeToSendPacket) { this questionable sanity checks looks unlrelated to remb changes. (Do you know a use case that rely on this edge behaviour?) May be move discussion about them into dedicated CL https://codereview.webrtc.org/2973363002/diff/100001/webrtc/modules/pacing/pa... webrtc/modules/pacing/packet_router_unittest.cc:616: clock.AdvanceTime(rtc::TimeDelta::FromMilliseconds(1000)); probably better to advance time after OnReceiveBitrateChanged: remb sending normally happen within a second _after_ bitrate is changed. https://codereview.webrtc.org/2973363002/diff/100001/webrtc/modules/pacing/pa... webrtc/modules/pacing/packet_router_unittest.cc:635: clock.AdvanceTime(rtc::TimeDelta::FromMilliseconds(1000)); either do not advance time at all (to stress remb is sent immidiately) or advance it after new bitrate estimate.
https://codereview.webrtc.org/2973363002/diff/100001/webrtc/modules/pacing/pa... File webrtc/modules/pacing/packet_router.cc (right): https://codereview.webrtc.org/2973363002/diff/100001/webrtc/modules/pacing/pa... webrtc/modules/pacing/packet_router.cc:257: RtpRtcp* previous_active_remb_module_ = active_remb_module_; On 2017/07/28 11:35:03, danilchap wrote: > remove last _ (it is local variable, not a member variable) Done. https://codereview.webrtc.org/2973363002/diff/100001/webrtc/modules/pacing/pa... File webrtc/modules/pacing/packet_router.h (right): https://codereview.webrtc.org/2973363002/diff/100001/webrtc/modules/pacing/pa... webrtc/modules/pacing/packet_router.h:56: constexpr bool remb_candidate = true; // Default to old behavior. On 2017/07/28 11:35:04, danilchap wrote: > May be remove this line: > with called function just one line above, repeating parameter name with local > variable is probably overkill. > > Comment feels like stating the obvious: to be backward compatible you have to > default to old behavior. Done. https://codereview.webrtc.org/2973363002/diff/100001/webrtc/modules/pacing/pa... webrtc/modules/pacing/packet_router.h:107: // TODO(eladalon): remb_crit_ only ever held from one function, and it's not On 2017/07/28 11:35:04, danilchap wrote: > you addressing it in another CL already, so probably remove the TODO from this > one. 1. I think you're referring to receive_cs_? I don't remember handling remb_crit_ (though I might be mistaken). 2. Speaking of receive_cs_, I am not sure when (/if) that CL can be landed, since it's causing problems with some unit-tests which are multi-threaded. So I think I should fix the TODO (% fixing my typo over "caleld"). https://codereview.webrtc.org/2973363002/diff/100001/webrtc/modules/pacing/pa... File webrtc/modules/pacing/packet_router_unittest.cc (right): https://codereview.webrtc.org/2973363002/diff/100001/webrtc/modules/pacing/pa... webrtc/modules/pacing/packet_router_unittest.cc:39: EXPECT_CALL(*mock_rtp_rtcp, SetREMBStatus(remb_state)) On 2017/07/28 11:35:04, danilchap wrote: > EXPECT_CALL in helper complicates reading error message when it fails - error > message will contain line number of the helper instead of line number where > expectation was set. Removed in favor of MockRtpRtcpRemb (under a different name) and explicitly checking REMB after the function under test is called. https://codereview.webrtc.org/2973363002/diff/100001/webrtc/modules/pacing/pa... webrtc/modules/pacing/packet_router_unittest.cc:48: void OnSetREMBStatusAndSetRembAccordingly(MockRtpRtcp* mock_rtp_rtcp) { On 2017/07/28 11:35:04, danilchap wrote: > probably cleaner to use different Mock instead of free functions: > class MockRtpRtcpRemb : public MockRtpRtcp { > public: > MockRtpRtcpRemb() { > ON_CALL(*this, SetREMBStatus(_)).WillByDefault(SaveArg<0>(&remb_)); > ON_CALL(*this, REMB()).WillByDefault(ReturnPointee(&remb_)); > } > > private: > bool remb_ = false; > }; > > TEST(...) { > // both EXPECT_CALL(module, SetRembStatus(false)) before call > // and EXPECT_FALSE(module->REMB()) after the call with check what you need. > // personally would prefer the latter: expect module end-up in correct remb > state rather than how it was changed to it. > // or, like you already doing, test with EXPECT_CALL(, SetREMBData) removing > mentioning GetRembStatus/SetRembStatus from individual tests. > } > > For more details: > https://github.com/google/googletest/blob/master/googlemock/docs/CookBook.md#... MockRtpRtcpRemb - Good suggestions, I'll use that. About the rest - discussed offline. https://codereview.webrtc.org/2973363002/diff/100001/webrtc/modules/pacing/pa... webrtc/modules/pacing/packet_router_unittest.cc:64: TEST_F(PacketRouterTest, Sanity_NoModuleRegistered_TimeToSendPacket) { On 2017/07/28 11:35:04, danilchap wrote: > this questionable sanity checks looks unlrelated to remb changes. > (Do you know a use case that rely on this edge behaviour?) > May be move discussion about them into dedicated CL I'll move to its own CL (https://codereview.webrtc.org/2986093003). As for questionability, I'd say that at the very least we want to make sure we don't crash - hence the "sanity". Whether it should return false, return true, or RTC_DCHECK (then the sanity test would be a death-test) is indeed up for discussion. https://codereview.webrtc.org/2973363002/diff/100001/webrtc/modules/pacing/pa... webrtc/modules/pacing/packet_router_unittest.cc:616: clock.AdvanceTime(rtc::TimeDelta::FromMilliseconds(1000)); On 2017/07/28 11:35:04, danilchap wrote: > probably better to advance time after OnReceiveBitrateChanged: > remb sending normally happen within a second _after_ bitrate is changed. Done. https://codereview.webrtc.org/2973363002/diff/100001/webrtc/modules/pacing/pa... webrtc/modules/pacing/packet_router_unittest.cc:616: clock.AdvanceTime(rtc::TimeDelta::FromMilliseconds(1000)); On 2017/07/28 11:35:04, danilchap wrote: > probably better to advance time after OnReceiveBitrateChanged: > remb sending normally happen within a second _after_ bitrate is changed. What effect would moving the clock have on the packet_router if does after the last manipulation of packet_router? (This is a single-threaded test.) https://codereview.webrtc.org/2973363002/diff/100001/webrtc/modules/pacing/pa... webrtc/modules/pacing/packet_router_unittest.cc:635: clock.AdvanceTime(rtc::TimeDelta::FromMilliseconds(1000)); On 2017/07/28 11:35:04, danilchap wrote: > either do not advance time at all (to stress remb is sent immidiately) or > advance it after new bitrate estimate. OnReceiveBitrateChanged() won't have the expected effect if the clock is not advanced beforehand. This might be seen as edge behavior, because last_remb_time_ms_ is 0 and the fake clock is also 0. I could change last_remb_time_ms_ to be an optional, but that doesn't seem to me to be reasonable; we probably don't really want to send an REMB on the very first call to PacketRouter::OnReceiveBitrateChanged(), because we probably don't have a valid estimation yet. Maybe moving AdvanceTime() to the beginning of the test would make things clearer? I am not sure about this. Wdyt?
lgtm with nits https://codereview.webrtc.org/2973363002/diff/100001/webrtc/modules/pacing/pa... File webrtc/modules/pacing/packet_router_unittest.cc (right): https://codereview.webrtc.org/2973363002/diff/100001/webrtc/modules/pacing/pa... webrtc/modules/pacing/packet_router_unittest.cc:616: clock.AdvanceTime(rtc::TimeDelta::FromMilliseconds(1000)); On 2017/07/28 15:47:11, eladalon wrote: > On 2017/07/28 11:35:04, danilchap wrote: > > probably better to advance time after OnReceiveBitrateChanged: > > remb sending normally happen within a second _after_ bitrate is changed. > > What effect would moving the clock have on the packet_router if does after the > last manipulation of packet_router? (This is a single-threaded test.) Hm, true, it shouldn't. until clock will learn to run pending tasks during AdvanceTime. Let me reread tests keeping that in mind... https://codereview.webrtc.org/2973363002/diff/120001/webrtc/modules/pacing/pa... File webrtc/modules/pacing/packet_router.cc (right): https://codereview.webrtc.org/2973363002/diff/120001/webrtc/modules/pacing/pa... webrtc/modules/pacing/packet_router.cc:56: const auto& it = auto it = (iterators normally passed by value) https://codereview.webrtc.org/2973363002/diff/120001/webrtc/modules/pacing/pa... webrtc/modules/pacing/packet_router.cc:59: rtp_send_modules_.remove(rtp_module); may be erase(it) to avoid double lookup https://codereview.webrtc.org/2973363002/diff/120001/webrtc/modules/pacing/pa... webrtc/modules/pacing/packet_router.cc:217: std::vector<RtpRtcp*>& candidates_vector = may be just candidates (function is short enough to see it is a vector) https://codereview.webrtc.org/2973363002/diff/120001/webrtc/modules/pacing/pa... webrtc/modules/pacing/packet_router.cc:237: if (*it == active_remb_module_) { is this block needed? (DetermineActiveRembModule seems already deactivate the active module) https://codereview.webrtc.org/2973363002/diff/120001/webrtc/modules/pacing/pa... File webrtc/modules/pacing/packet_router_unittest.cc (right): https://codereview.webrtc.org/2973363002/diff/120001/webrtc/modules/pacing/pa... webrtc/modules/pacing/packet_router_unittest.cc:290: packet_router.RemoveSendRtpModule(&module); may be use packet_router_ or do not use fixture, e.g. TEST(PacketRouterDeathTest, Double...
https://codereview.webrtc.org/2973363002/diff/120001/webrtc/modules/pacing/pa... File webrtc/modules/pacing/packet_router.cc (right): https://codereview.webrtc.org/2973363002/diff/120001/webrtc/modules/pacing/pa... webrtc/modules/pacing/packet_router.cc:56: const auto& it = On 2017/07/31 13:18:03, danilchap wrote: > auto it = > (iterators normally passed by value) Done. https://codereview.webrtc.org/2973363002/diff/120001/webrtc/modules/pacing/pa... webrtc/modules/pacing/packet_router.cc:59: rtp_send_modules_.remove(rtp_module); On 2017/07/31 13:18:04, danilchap wrote: > may be erase(it) to avoid double lookup Done. https://codereview.webrtc.org/2973363002/diff/120001/webrtc/modules/pacing/pa... webrtc/modules/pacing/packet_router.cc:217: std::vector<RtpRtcp*>& candidates_vector = On 2017/07/31 13:18:04, danilchap wrote: > may be just candidates > (function is short enough to see it is a vector) Done. https://codereview.webrtc.org/2973363002/diff/120001/webrtc/modules/pacing/pa... webrtc/modules/pacing/packet_router.cc:237: if (*it == active_remb_module_) { On 2017/07/31 13:18:04, danilchap wrote: > is this block needed? (DetermineActiveRembModule seems already deactivate the > active module) You're right that it would work, but I find it to be a bit confusing to only have: candidates.erase(it); DetermineActiveRembModule(); Because that splits the removal between two functions. With this (somewhat redundant) block, though, removal is done completely within MaybeRemoveRembModuleCandidate(), and DetermineActiveRembModule() only sets the new candidate (if any), as its name suggests. https://codereview.webrtc.org/2973363002/diff/120001/webrtc/modules/pacing/pa... File webrtc/modules/pacing/packet_router_unittest.cc (right): https://codereview.webrtc.org/2973363002/diff/120001/webrtc/modules/pacing/pa... webrtc/modules/pacing/packet_router_unittest.cc:290: packet_router.RemoveSendRtpModule(&module); On 2017/07/31 13:18:04, danilchap wrote: > may be use packet_router_ > or do not use fixture, e.g. > TEST(PacketRouterDeathTest, Double... Let's land as-is, and I'll push a CL on top of this that removes the fixture from this file.
https://codereview.webrtc.org/2973363002/diff/120001/webrtc/modules/pacing/pa... File webrtc/modules/pacing/packet_router_unittest.cc (right): https://codereview.webrtc.org/2973363002/diff/120001/webrtc/modules/pacing/pa... webrtc/modules/pacing/packet_router_unittest.cc:290: packet_router.RemoveSendRtpModule(&module); On 2017/07/31 14:08:26, eladalon wrote: > On 2017/07/31 13:18:04, danilchap wrote: > > may be use packet_router_ > > or do not use fixture, e.g. > > TEST(PacketRouterDeathTest, Double... > > Let's land as-is, and I'll push a CL on top of this that removes the fixture > from this file. (I've changed in some places, but I didn't think it would be worth the work to hunt down only the tests I've added in this CL and change only there. I'll just remove the fixture entirely in a CL I'll now push for review.)
https://codereview.webrtc.org/2973363002/diff/120001/webrtc/modules/pacing/pa... File webrtc/modules/pacing/packet_router.cc (right): https://codereview.webrtc.org/2973363002/diff/120001/webrtc/modules/pacing/pa... webrtc/modules/pacing/packet_router.cc:237: if (*it == active_remb_module_) { On 2017/07/31 14:08:26, eladalon wrote: > On 2017/07/31 13:18:04, danilchap wrote: > > is this block needed? (DetermineActiveRembModule seems already deactivate the > > active module) > > You're right that it would work, but I find it to be a bit confusing to only > have: > candidates.erase(it); > DetermineActiveRembModule(); > Because that splits the removal between two functions. With this (somewhat > redundant) block, though, removal is done completely within > MaybeRemoveRembModuleCandidate(), and DetermineActiveRembModule() only sets the > new candidate (if any), as its name suggests. yep, removal (and addition) are naturally split between two functions: Add/Remove are responsible to update candidates_ containers where DetermineActiveRembModule responsible for updating active_remb_module_. It is easy to imaging that the way you turn remb off in the module might change. Currently you have to remember to update 2 places. With this block remove there will be single block that is responsible for deactivating module.
https://codereview.webrtc.org/2973363002/diff/120001/webrtc/modules/pacing/pa... File webrtc/modules/pacing/packet_router.cc (right): https://codereview.webrtc.org/2973363002/diff/120001/webrtc/modules/pacing/pa... webrtc/modules/pacing/packet_router.cc:237: if (*it == active_remb_module_) { On 2017/07/31 14:37:17, danilchap wrote: > On 2017/07/31 14:08:26, eladalon wrote: > > On 2017/07/31 13:18:04, danilchap wrote: > > > is this block needed? (DetermineActiveRembModule seems already deactivate > the > > > active module) > > > > You're right that it would work, but I find it to be a bit confusing to only > > have: > > candidates.erase(it); > > DetermineActiveRembModule(); > > Because that splits the removal between two functions. With this (somewhat > > redundant) block, though, removal is done completely within > > MaybeRemoveRembModuleCandidate(), and DetermineActiveRembModule() only sets > the > > new candidate (if any), as its name suggests. > > yep, removal (and addition) are naturally split between two functions: > Add/Remove are responsible to update candidates_ containers > where DetermineActiveRembModule responsible for updating active_remb_module_. > > It is easy to imaging that the way you turn remb off in the module might change. > Currently you have to remember to update 2 places. With this block remove there > will be single block that is responsible for deactivating module. That's one side of the coin. The other side is that by removing this block, I rely the update of |candidates| to be sufficient to trigger the right behavior in DetermineActiveRembModule(). Let's try to have the best of both worlds by adding a helper function, UnsetActiveRembModule(). Wdyt of that? (See in code.)
https://codereview.webrtc.org/2973363002/diff/120001/webrtc/modules/pacing/pa... File webrtc/modules/pacing/packet_router.cc (right): https://codereview.webrtc.org/2973363002/diff/120001/webrtc/modules/pacing/pa... webrtc/modules/pacing/packet_router.cc:237: if (*it == active_remb_module_) { On 2017/07/31 14:51:20, eladalon wrote: > On 2017/07/31 14:37:17, danilchap wrote: > > On 2017/07/31 14:08:26, eladalon wrote: > > > On 2017/07/31 13:18:04, danilchap wrote: > > > > is this block needed? (DetermineActiveRembModule seems already deactivate > > the > > > > active module) > > > > > > You're right that it would work, but I find it to be a bit confusing to only > > > have: > > > candidates.erase(it); > > > DetermineActiveRembModule(); > > > Because that splits the removal between two functions. With this (somewhat > > > redundant) block, though, removal is done completely within > > > MaybeRemoveRembModuleCandidate(), and DetermineActiveRembModule() only sets > > the > > > new candidate (if any), as its name suggests. > > > > yep, removal (and addition) are naturally split between two functions: > > Add/Remove are responsible to update candidates_ containers > > where DetermineActiveRembModule responsible for updating active_remb_module_. > > > > It is easy to imaging that the way you turn remb off in the module might > change. > > Currently you have to remember to update 2 places. With this block remove > there > > will be single block that is responsible for deactivating module. > > That's one side of the coin. The other side is that by removing this block, I > rely the update of |candidates| to be sufficient to trigger the right behavior > in DetermineActiveRembModule(). Looking at implmentation of the DetermineActiveRembModule, this guarantee seems easy to provide: active_remb_module_ shouldn't be deleted. Everything else is responsibility of that last helper. > Let's try to have the best of both worlds by adding a helper function, > UnsetActiveRembModule(). Wdyt of that? (See in code.) I find it about same, but that my taste you do not need to agree with. counter-proposal: Will it look better if you rename DetermingActiveRembModule into e.g. UpdateActiveRembModule? (or some better name that hints it responsible both for unsetting old value and configuring new one)
lgtm
https://codereview.webrtc.org/2973363002/diff/120001/webrtc/modules/pacing/pa... File webrtc/modules/pacing/packet_router.cc (right): https://codereview.webrtc.org/2973363002/diff/120001/webrtc/modules/pacing/pa... webrtc/modules/pacing/packet_router.cc:237: if (*it == active_remb_module_) { On 2017/07/31 15:10:05, danilchap wrote: > On 2017/07/31 14:51:20, eladalon wrote: > > On 2017/07/31 14:37:17, danilchap wrote: > > > On 2017/07/31 14:08:26, eladalon wrote: > > > > On 2017/07/31 13:18:04, danilchap wrote: > > > > > is this block needed? (DetermineActiveRembModule seems already > deactivate > > > the > > > > > active module) > > > > > > > > You're right that it would work, but I find it to be a bit confusing to > only > > > > have: > > > > candidates.erase(it); > > > > DetermineActiveRembModule(); > > > > Because that splits the removal between two functions. With this (somewhat > > > > redundant) block, though, removal is done completely within > > > > MaybeRemoveRembModuleCandidate(), and DetermineActiveRembModule() only > sets > > > the > > > > new candidate (if any), as its name suggests. > > > > > > yep, removal (and addition) are naturally split between two functions: > > > Add/Remove are responsible to update candidates_ containers > > > where DetermineActiveRembModule responsible for updating > active_remb_module_. > > > > > > It is easy to imaging that the way you turn remb off in the module might > > change. > > > Currently you have to remember to update 2 places. With this block remove > > there > > > will be single block that is responsible for deactivating module. > > > > That's one side of the coin. The other side is that by removing this block, I > > rely the update of |candidates| to be sufficient to trigger the right behavior > > in DetermineActiveRembModule(). > Looking at implmentation of the DetermineActiveRembModule, this guarantee seems > easy to provide: active_remb_module_ shouldn't be deleted. Everything else is > responsibility of that last helper. > > > Let's try to have the best of both worlds by adding a helper function, > > UnsetActiveRembModule(). Wdyt of that? (See in code.) > I find it about same, but that my taste you do not need to agree with. > > counter-proposal: Will it look better if you rename DetermingActiveRembModule > into e.g. UpdateActiveRembModule? > (or some better name that hints it responsible both for unsetting old value and > configuring new one) My reservation against Update vs. Determine is that it makes it less clear that the following two are also an option: 1. The active has not changed (e.g. when adding a new candidate which is not of higher priority than the current). 2. Last candidate removed - "update" (weakly) suggests that there would be an active module after the function terminates. I prefer to keep the removal here. I'm interpreting your latest comment as "this is my opinion, but I don't mind you landing this with your version." I'll land, then. Let me know if I misunderstood, and we can fix this.
eladalon-macbookpro:src eladalon$ cat webrtc/voice_engine/OWNERS henrikg@webrtc.org henrika@webrtc.org niklas.enbom@webrtc.org solenberg@webrtc.org Stefan, Danil, whom do you suggest to accept the voice_engine/ part of the CL? I see Fredrick is marked as OOO until the 21st.
lgtm for webrtc/video https://codereview.webrtc.org/2973363002/diff/100001/webrtc/video/rtp_video_s... File webrtc/video/rtp_video_stream_receiver.cc (right): https://codereview.webrtc.org/2973363002/diff/100001/webrtc/video/rtp_video_s... webrtc/video/rtp_video_stream_receiver.cc:116: constexpr bool remb_candidate = true; nit: Can you just add a comment instead of a named temp? https://codereview.webrtc.org/2973363002/diff/100001/webrtc/video/video_send_... File webrtc/video/video_send_stream.cc (right): https://codereview.webrtc.org/2973363002/diff/100001/webrtc/video/video_send_... webrtc/video/video_send_stream.cc:850: constexpr bool remb_candidate = true; dito
https://codereview.webrtc.org/2973363002/diff/100001/webrtc/video/rtp_video_s... File webrtc/video/rtp_video_stream_receiver.cc (right): https://codereview.webrtc.org/2973363002/diff/100001/webrtc/video/rtp_video_s... webrtc/video/rtp_video_stream_receiver.cc:116: constexpr bool remb_candidate = true; On 2017/08/01 09:51:54, sprang_webrtc wrote: > nit: Can you just add a comment instead of a named temp? Discussed offline; Erik is OK with keeping it. https://codereview.webrtc.org/2973363002/diff/100001/webrtc/video/video_send_... File webrtc/video/video_send_stream.cc (right): https://codereview.webrtc.org/2973363002/diff/100001/webrtc/video/video_send_... webrtc/video/video_send_stream.cc:850: constexpr bool remb_candidate = true; On 2017/08/01 09:51:54, sprang_webrtc wrote: > dito Same here.
The CQ bit was checked by eladalon@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from danilchap@webrtc.org Link to the patchset: https://codereview.webrtc.org/2973363002/#ps160001 (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": 160001, "attempt_start_ts": 1501592768274740,
"parent_rev": "ddfa252b50953e2a0e3b35af21739e70e4345208", "commit_rev":
"822ff2b794eb676541ef3cc6d5c7bc7380a590cc"}
Message was sent while issue was closed.
Description was changed from ========== Explicitly inform PacketRouter which RTP-RTCP modules are REMB-candidates BUG=webrtc:7860 ========== to ========== Explicitly inform PacketRouter which RTP-RTCP modules are REMB-candidates BUG=webrtc:7860 Review-Url: https://codereview.webrtc.org/2973363002 Cr-Commit-Position: refs/heads/master@{#19201} Committed: https://chromium.googlesource.com/external/webrtc/+/822ff2b794eb676541ef3cc6d... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/external/webrtc/+/822ff2b794eb676541ef3cc6d... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
