Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2994513002:
Add PacketRouter::SetMaxDesiredReceiveBitrate for application limited receive bandwidth (Closed)
DescriptionAdd PacketRouter::SetMaxDesiredReceiveBitrate for application limited receive bandwidth
BUG=webrtc:7135
Review-Url: https://codereview.webrtc.org/2994513002
Cr-Commit-Position: refs/heads/master@{#19307}
Committed: https://chromium.googlesource.com/external/webrtc/+/4708537f0d299bed59cad6f0917f608e1f8569ab
Patch Set 1 #
Total comments: 26
Patch Set 2 : comments #Patch Set 3 : test comments #
Total comments: 4
Patch Set 4 : Rename the function #
Messages
Total messages: 20 (8 generated)
Description was changed from ========== Add PacketRouter::SetMaxEstimatedBandwidth for application-limited receive bandwidth BUG=None ========== to ========== Add PacketRouter::SetMaxEstimatedBandwidth for application-limited receive bandwidth BUG=webrtc:7135 ==========
Description was changed from ========== Add PacketRouter::SetMaxEstimatedBandwidth for application-limited receive bandwidth BUG=webrtc:7135 ========== to ========== Add PacketRouter::SetMaxEstimatedBandwidth for application limited receive bandwidth BUG=webrtc:7135 ==========
danilchap@webrtc.org changed reviewers: + eladalon@webrtc.org, stefan@webrtc.org
ptal
https://codereview.webrtc.org/2994513002/diff/1/webrtc/modules/pacing/packet_... File webrtc/modules/pacing/packet_router.cc (right): https://codereview.webrtc.org/2994513002/diff/1/webrtc/modules/pacing/packet_... webrtc/modules/pacing/packet_router.cc:25: nit: I'd have preferred with an empty line between the two "namespace" lines, but then without the empty lines around the "constexpr" line. Of course, this is just my own preference. https://codereview.webrtc.org/2994513002/diff/1/webrtc/modules/pacing/packet_... webrtc/modules/pacing/packet_router.cc:34: max_bitrate_bps_(std::numeric_limits<uint32_t>::max()), Suggestion: max_bitrate_bps_(std::numeric_limits<decltype(max_bitrate_bps_)>::max()), 1. This way, nobody can change the type only in the .h file and introduce an error. 2. Also, the reader doesn't need to check if uint32_t is the actual type, or if the value has a special meaning. https://codereview.webrtc.org/2994513002/diff/1/webrtc/modules/pacing/packet_... webrtc/modules/pacing/packet_router.cc:195: rtc::TimeMillis() - last_remb_time_ms_ < kRembSendIntervalMs && 1. This is a bit confusing; it would be more natural, IMHO, to suppress the REMB if (a || b || c), where B is "not enough time has passed since the last sent REMB." 2. The way it's written now, if SetMaxEstimatedBandwidth() is called from, say, the ctor, wouldn't the (last_send_bitrate_bps_ > 0) fail, causing us to *not* supress the REMB, and therefore send out an REMB before we have any valid estimate? (It could be that it's suppressed from inside SendRemb then, but it's suboptimal to call SendRemb at all.)
https://codereview.webrtc.org/2994513002/diff/1/webrtc/modules/pacing/packet_... File webrtc/modules/pacing/packet_router_unittest.cc (right): https://codereview.webrtc.org/2994513002/diff/1/webrtc/modules/pacing/packet_... webrtc/modules/pacing/packet_router_unittest.cc:32: using ::testing::_; Is this move meaningfull? We're inside of a .cc file, so this shouldn't really matter, I'm guessing? But perhaps it's a general policy to keep things to the minimum possible scope? https://codereview.webrtc.org/2994513002/diff/1/webrtc/modules/pacing/packet_... webrtc/modules/pacing/packet_router_unittest.cc:574: packet_router.AddSendRtpModule(&remb_sender, /*remb_candidate=*/true); nit: Prefer keeping files self-consistent, and either use |constexpr bool remb_candidate| everywhere (current implementation) or nowhere (possible change by you). I personally prefer the former (everywhere), but either would be better than a mix of the two. https://codereview.webrtc.org/2994513002/diff/1/webrtc/modules/pacing/packet_... webrtc/modules/pacing/packet_router_unittest.cc:579: EXPECT_CALL(remb_sender, SetREMBData(Gt(cap_bitrate), _)).Times(0); On the one hand, you illuminate the intention of your test. On the other, extremely wrong behavior of the code under test could still be perceived as legal - suppose the bitrate is capped at 0, 10, or anything else that's way below cap_bitrate. That would clearly be wrong, but it would pass. IMHO, it would be better to EXPECT exactly what we expect here. https://codereview.webrtc.org/2994513002/diff/1/webrtc/modules/pacing/packet_... webrtc/modules/pacing/packet_router_unittest.cc:586: // Cleanup. nit #1: Empty line before the cleanup block. nit #2: "Cleanup" everywhere or "test tear-down" everywhere, imho, would be better than a mix. https://codereview.webrtc.org/2994513002/diff/1/webrtc/modules/pacing/packet_... webrtc/modules/pacing/packet_router_unittest.cc:618: const uint32_t measured_bitrate_bps = 150000; nit: constexpr https://codereview.webrtc.org/2994513002/diff/1/webrtc/modules/pacing/packet_... webrtc/modules/pacing/packet_router_unittest.cc:624: packet_router.SetMaxEstimatedBandwidth(measured_bitrate_bps); Suggestion - if you created another const(expr), called it "max...", and set it to be equal to |measured_bitrate_bps|, nothing would change in the binary, but it would be clear this is not a typo. https://codereview.webrtc.org/2994513002/diff/1/webrtc/modules/pacing/packet_... webrtc/modules/pacing/packet_router_unittest.cc:670: TEST(PacketRouterRembTest, SetMaxEstimatedBandwidthTriggersRembWhenNoMeasures) { So this is intentional? 1. Might be good to document this in PacketRouter. 2. Do we mean to let this override the minimum time between REMB messages?
https://codereview.webrtc.org/2994513002/diff/1/webrtc/modules/pacing/packet_... File webrtc/modules/pacing/packet_router.cc (right): https://codereview.webrtc.org/2994513002/diff/1/webrtc/modules/pacing/packet_... webrtc/modules/pacing/packet_router.cc:25: On 2017/08/03 12:40:52, eladalon wrote: > nit: I'd have preferred with an empty line between the two "namespace" lines, > but then without the empty lines around the "constexpr" line. Of course, this is > just my own preference. Acknowledged. https://codereview.webrtc.org/2994513002/diff/1/webrtc/modules/pacing/packet_... webrtc/modules/pacing/packet_router.cc:34: max_bitrate_bps_(std::numeric_limits<uint32_t>::max()), On 2017/08/03 12:40:52, eladalon wrote: > Suggestion: > max_bitrate_bps_(std::numeric_limits<decltype(max_bitrate_bps_)>::max()), > 1. This way, nobody can change the type only in the .h file and introduce an > error. > 2. Also, the reader doesn't need to check if uint32_t is the actual type, or if > the value has a special meaning. Done. https://codereview.webrtc.org/2994513002/diff/1/webrtc/modules/pacing/packet_... webrtc/modules/pacing/packet_router.cc:195: rtc::TimeMillis() - last_remb_time_ms_ < kRembSendIntervalMs && On 2017/08/03 12:40:52, eladalon wrote: > 1. This is a bit confusing; it would be more natural, IMHO, to suppress the REMB > if (a || b || c), where B is "not enough time has passed since the last sent > REMB." > 2. The way it's written now, if SetMaxEstimatedBandwidth() is called from, say, > the ctor, wouldn't the (last_send_bitrate_bps_ > 0) fail, causing us to *not* > supress the REMB, and therefore send out an REMB before we have any valid > estimate? (It could be that it's suppressed from inside SendRemb then, but it's > suboptimal to call SendRemb at all.) 1. Recent measure bitrate below the cap seems like the only reason not to send remb. can you please expand what a and c in your suggestion? For now I swapped conditions to match order of the words in the comment below. Alternatively it can be simplified by changing last_remb_time_ms_ to be infinite past initially instead of current time. then 'recent' check would include 'had measurement'. 2. This function explicitly expected to send remb before any measurements are done (last_send_bitrate_bps_ > 0 is there exactly for that feature). In particular it expect to work with send side bwe where no measurements will be provided.
https://codereview.webrtc.org/2994513002/diff/1/webrtc/modules/pacing/packet_... File webrtc/modules/pacing/packet_router_unittest.cc (right): https://codereview.webrtc.org/2994513002/diff/1/webrtc/modules/pacing/packet_... webrtc/modules/pacing/packet_router_unittest.cc:32: using ::testing::_; On 2017/08/03 12:59:02, eladalon wrote: > Is this move meaningfull? We're inside of a .cc file, so this shouldn't really > matter, I'm guessing? But perhaps it's a general policy to keep things to the > minimum possible scope? yes, it is general policy, this style might help to avoid hard-to find name collisions without introducing downsides. moved it because extended it. https://codereview.webrtc.org/2994513002/diff/1/webrtc/modules/pacing/packet_... webrtc/modules/pacing/packet_router_unittest.cc:574: packet_router.AddSendRtpModule(&remb_sender, /*remb_candidate=*/true); On 2017/08/03 12:59:02, eladalon wrote: > nit: Prefer keeping files self-consistent, and either use |constexpr bool > remb_candidate| everywhere (current implementation) or nowhere (possible change > by you). I personally prefer the former (everywhere), but either would be better > than a mix of the two. changed to keep old style for consistency, thanks. https://codereview.webrtc.org/2994513002/diff/1/webrtc/modules/pacing/packet_... webrtc/modules/pacing/packet_router_unittest.cc:579: EXPECT_CALL(remb_sender, SetREMBData(Gt(cap_bitrate), _)).Times(0); On 2017/08/03 12:59:02, eladalon wrote: > On the one hand, you illuminate the intention of your test. On the other, > extremely wrong behavior of the code under test could still be perceived as > legal - suppose the bitrate is capped at 0, 10, or anything else that's way > below cap_bitrate. That would clearly be wrong, but it would pass. IMHO, it > would be better to EXPECT exactly what we expect here. Acknowledged. I do that in test below, where there are no OnReceiveBitrateChanged calls. Here I prefer to test exactly what test promise and what is the main feature of the function. https://codereview.webrtc.org/2994513002/diff/1/webrtc/modules/pacing/packet_... webrtc/modules/pacing/packet_router_unittest.cc:586: // Cleanup. On 2017/08/03 12:59:02, eladalon wrote: > nit #1: Empty line before the cleanup block. > nit #2: "Cleanup" everywhere or "test tear-down" everywhere, imho, would be > better than a mix. Done. https://codereview.webrtc.org/2994513002/diff/1/webrtc/modules/pacing/packet_... webrtc/modules/pacing/packet_router_unittest.cc:618: const uint32_t measured_bitrate_bps = 150000; On 2017/08/03 12:59:02, eladalon wrote: > nit: constexpr not sure why is it better in this case, checked tests around - some use constexpr, some const, some neither. personally when it doesn't matter prefer to use constexpr for values that never every really need to change (e.g. number of bits in byte), but keep const for values that can have different value, but just locally happen to be same. https://codereview.webrtc.org/2994513002/diff/1/webrtc/modules/pacing/packet_... webrtc/modules/pacing/packet_router_unittest.cc:624: packet_router.SetMaxEstimatedBandwidth(measured_bitrate_bps); On 2017/08/03 12:59:02, eladalon wrote: > Suggestion - if you created another const(expr), called it "max...", and set it > to be equal to |measured_bitrate_bps|, nothing would change in the binary, but > it would be clear this is not a typo. Done. https://codereview.webrtc.org/2994513002/diff/1/webrtc/modules/pacing/packet_... webrtc/modules/pacing/packet_router_unittest.cc:670: TEST(PacketRouterRembTest, SetMaxEstimatedBandwidthTriggersRembWhenNoMeasures) { On 2017/08/03 12:59:02, eladalon wrote: > So this is intentional? > 1. Might be good to document this in PacketRouter. > 2. Do we mean to let this override the minimum time between REMB messages? 1. It sort of implicit - if you set a limit how much you can receive, you expect sender will be notified about the limit. I've update documentation a bit. Any suggestion how I can explain it better? 2. Yes, if application apply the limit, it likely want to propagate it asap, but do not want to give those promises in the documentation, e.g. SetREMBData doesn't actually sends a remb, it can have own delay and own throttle.
lgtm % suggestions https://codereview.webrtc.org/2994513002/diff/1/webrtc/modules/pacing/packet_... File webrtc/modules/pacing/packet_router.cc (right): https://codereview.webrtc.org/2994513002/diff/1/webrtc/modules/pacing/packet_... webrtc/modules/pacing/packet_router.cc:195: rtc::TimeMillis() - last_remb_time_ms_ < kRembSendIntervalMs && On 2017/08/03 13:25:33, danilchap wrote: > On 2017/08/03 12:40:52, eladalon wrote: > > 1. This is a bit confusing; it would be more natural, IMHO, to suppress the > REMB > > if (a || b || c), where B is "not enough time has passed since the last sent > > REMB." > > 2. The way it's written now, if SetMaxEstimatedBandwidth() is called from, > say, > > the ctor, wouldn't the (last_send_bitrate_bps_ > 0) fail, causing us to *not* > > supress the REMB, and therefore send out an REMB before we have any valid > > estimate? (It could be that it's suppressed from inside SendRemb then, but > it's > > suboptimal to call SendRemb at all.) > > 1. Recent measure bitrate below the cap seems like the only reason not to send > remb. can you please expand what a and c in your suggestion? > For now I swapped conditions to match order of the words in the comment below. > > Alternatively it can be simplified by changing last_remb_time_ms_ to be infinite > past initially instead of current time. then 'recent' check would include 'had > measurement'. > > 2. This function explicitly expected to send remb before any measurements are > done (last_send_bitrate_bps_ > 0 is there exactly for that feature). In > particular it expect to work with send side bwe where no measurements will be > provided. > 1. |a| was |last_send_bitrate_bps_ > 0|, etc. But if you really intended to send REMB whenever SetMaxEstimatedBandwidth() was called, except for very specific cases where you'd suppress, then never mind. 2. I had initially suspected this to be a bug, until I had seen you testing for it in the unit-tests. 3. This means calling SetMaxEstimatedBandwidth() too often might produce too many REMBs. I wonder if we could not just let the next expected REMB be sent normally, instead of triggering an early REMB? Even if SetMaxEstimatedBandwidth() being called too often is highly unlikely, not sending the REMB from yet another code-path would reduce complexity. Wdyt? https://codereview.webrtc.org/2994513002/diff/1/webrtc/modules/pacing/packet_... File webrtc/modules/pacing/packet_router_unittest.cc (right): https://codereview.webrtc.org/2994513002/diff/1/webrtc/modules/pacing/packet_... webrtc/modules/pacing/packet_router_unittest.cc:32: using ::testing::_; On 2017/08/03 14:16:19, danilchap wrote: > On 2017/08/03 12:59:02, eladalon wrote: > > Is this move meaningfull? We're inside of a .cc file, so this shouldn't really > > matter, I'm guessing? But perhaps it's a general policy to keep things to the > > minimum possible scope? > > yes, it is general policy, this style might help to avoid hard-to find name > collisions without introducing downsides. > moved it because extended it. Acknowledged. https://codereview.webrtc.org/2994513002/diff/1/webrtc/modules/pacing/packet_... webrtc/modules/pacing/packet_router_unittest.cc:618: const uint32_t measured_bitrate_bps = 150000; On 2017/08/03 14:16:19, danilchap wrote: > On 2017/08/03 12:59:02, eladalon wrote: > > nit: constexpr > > not sure why is it better in this case, > checked tests around - some use constexpr, some const, some neither. > > personally when it doesn't matter prefer to use constexpr for values that never > every really need to change (e.g. number of bits in byte), > but keep const for values that can have different value, but just locally happen > to be same. That could have been a good convention, but there are problems with it. 1. The natural problem with introducing new conventions - not everybody uses it, so it can't be relied on. 2. As you no-doubt know, constexpr/const are not interchangeable for the code, so we can't choose between them purely to fit the convention. |const| is needed whenever you want an address, for example; constexpr whenever you want things to be set in compile-time. I prefer using constexpr whenever possible. A good compiler would probably not even need either keyword in the kind of cases we're discussing at the moment (local variables in a function - the compiler can see everything we can see, and optimize better than us), but a developer seeing constexpr would immediately know that the address of the const in question is never used - and that's independent of convention. But excuse this long discussion; I guess it's not really important, one way or the other. https://codereview.webrtc.org/2994513002/diff/1/webrtc/modules/pacing/packet_... webrtc/modules/pacing/packet_router_unittest.cc:670: TEST(PacketRouterRembTest, SetMaxEstimatedBandwidthTriggersRembWhenNoMeasures) { On 2017/08/03 14:16:18, danilchap wrote: > On 2017/08/03 12:59:02, eladalon wrote: > > So this is intentional? > > 1. Might be good to document this in PacketRouter. > > 2. Do we mean to let this override the minimum time between REMB messages? > > 1. It sort of implicit - if you set a limit how much you can receive, you expect > sender will be notified about the limit. > I've update documentation a bit. Any suggestion how I can explain it better? > 2. Yes, if application apply the limit, it likely want to propagate it asap, but > do not want to give those promises in the documentation, e.g. SetREMBData > doesn't actually sends a remb, it can have own delay and own throttle. Acknowledged. I've suggested a rephrasing next to the function's documentation. https://codereview.webrtc.org/2994513002/diff/40001/webrtc/modules/pacing/pac... File webrtc/modules/pacing/packet_router.h (right): https://codereview.webrtc.org/2994513002/diff/40001/webrtc/modules/pacing/pac... webrtc/modules/pacing/packet_router.h:87: // Ensures remote party notified of the estimate no larger than |bitrate_bps|. I find this a bit confusing. It's not easy to find a better wording that doesn't become too long, though. I suggest - and this is just a nit - to let the reader figure out what REMB is using other comments in this file, and here only write: // Cap the estimate sent to the other side. // If [fill in here], a new REMB message will be produced immediately.
https://codereview.webrtc.org/2994513002/diff/40001/webrtc/modules/pacing/pac... File webrtc/modules/pacing/packet_router.h (right): https://codereview.webrtc.org/2994513002/diff/40001/webrtc/modules/pacing/pac... webrtc/modules/pacing/packet_router.h:88: void SetMaxEstimatedBandwidth(uint32_t bitrate_bps); Isn't it better to give this a name that reflects what it's used for? E.g., SetMaxDesiredReceiveBitrate()
Description was changed from ========== Add PacketRouter::SetMaxEstimatedBandwidth for application limited receive bandwidth BUG=webrtc:7135 ========== to ========== Add PacketRouter::SetMaxDesiredReceiveBitrate for application limited receive bandwidth BUG=webrtc:7135 ==========
https://codereview.webrtc.org/2994513002/diff/1/webrtc/modules/pacing/packet_... File webrtc/modules/pacing/packet_router.cc (right): https://codereview.webrtc.org/2994513002/diff/1/webrtc/modules/pacing/packet_... webrtc/modules/pacing/packet_router.cc:195: rtc::TimeMillis() - last_remb_time_ms_ < kRembSendIntervalMs && On 2017/08/04 08:32:48, eladalon wrote: > On 2017/08/03 13:25:33, danilchap wrote: > > On 2017/08/03 12:40:52, eladalon wrote: > > > 1. This is a bit confusing; it would be more natural, IMHO, to suppress the > > REMB > > > if (a || b || c), where B is "not enough time has passed since the last sent > > > REMB." > > > 2. The way it's written now, if SetMaxEstimatedBandwidth() is called from, > > say, > > > the ctor, wouldn't the (last_send_bitrate_bps_ > 0) fail, causing us to > *not* > > > supress the REMB, and therefore send out an REMB before we have any valid > > > estimate? (It could be that it's suppressed from inside SendRemb then, but > > it's > > > suboptimal to call SendRemb at all.) > > > > 1. Recent measure bitrate below the cap seems like the only reason not to send > > remb. can you please expand what a and c in your suggestion? > > For now I swapped conditions to match order of the words in the comment below. > > > > Alternatively it can be simplified by changing last_remb_time_ms_ to be > infinite > > past initially instead of current time. then 'recent' check would include 'had > > measurement'. > > > > 2. This function explicitly expected to send remb before any measurements are > > done (last_send_bitrate_bps_ > 0 is there exactly for that feature). In > > particular it expect to work with send side bwe where no measurements will be > > provided. > > > > 1. |a| was |last_send_bitrate_bps_ > 0|, etc. But if you really intended to send > REMB whenever SetMaxEstimatedBandwidth() was called, except for very specific > cases where you'd suppress, then never mind. > 2. I had initially suspected this to be a bug, until I had seen you testing for > it in the unit-tests. > 3. This means calling SetMaxEstimatedBandwidth() too often might produce too > many REMBs. I wonder if we could not just let the next expected REMB be sent > normally, instead of triggering an early REMB? Even if > SetMaxEstimatedBandwidth() being called too often is highly unlikely, not > sending the REMB from yet another code-path would reduce complexity. Wdyt? to reduce complexity it might make sense to move all remb throttling to rtcp module from packet router: it has more knowledge if throttling is desired. But that is out of scope of this CL. This CL introduce new feature trying not to change any behavior if it is not used and with minimalistic prod code change. https://codereview.webrtc.org/2994513002/diff/1/webrtc/modules/pacing/packet_... File webrtc/modules/pacing/packet_router_unittest.cc (right): https://codereview.webrtc.org/2994513002/diff/1/webrtc/modules/pacing/packet_... webrtc/modules/pacing/packet_router_unittest.cc:618: const uint32_t measured_bitrate_bps = 150000; On 2017/08/04 08:32:49, eladalon wrote: > On 2017/08/03 14:16:19, danilchap wrote: > > On 2017/08/03 12:59:02, eladalon wrote: > > > nit: constexpr > > > > not sure why is it better in this case, > > checked tests around - some use constexpr, some const, some neither. > > > > personally when it doesn't matter prefer to use constexpr for values that > never > > every really need to change (e.g. number of bits in byte), > > but keep const for values that can have different value, but just locally > happen > > to be same. > > That could have been a good convention, but there are problems with it. > 1. The natural problem with introducing new conventions - not everybody uses it, > so it can't be relied on. > 2. As you no-doubt know, constexpr/const are not interchangeable for the code, > so we can't choose between them purely to fit the convention. |const| is needed > whenever you want an address, for example; constexpr whenever you want things to > be set in compile-time. > > I prefer using constexpr whenever possible. A good compiler would probably not > even need either keyword in the kind of cases we're discussing at the moment > (local variables in a function - the compiler can see everything we can see, and > optimize better than us), but a developer seeing constexpr would immediately > know that the address of the const in question is never used - and that's > independent of convention. > > But excuse this long discussion; I guess it's not really important, one way or > the other. Acknowledged. https://codereview.webrtc.org/2994513002/diff/40001/webrtc/modules/pacing/pac... File webrtc/modules/pacing/packet_router.h (right): https://codereview.webrtc.org/2994513002/diff/40001/webrtc/modules/pacing/pac... webrtc/modules/pacing/packet_router.h:87: // Ensures remote party notified of the estimate no larger than |bitrate_bps|. On 2017/08/04 08:32:49, eladalon wrote: > I find this a bit confusing. It's not easy to find a better wording that doesn't > become too long, though. I suggest - and this is just a nit - to let the reader > figure out what REMB is using other comments in this file, and here only write: > // Cap the estimate sent to the other side. > // If [fill in here], a new REMB message will be produced immediately. Rephrased a bit hoping to reduce confusion. (estimate -> receive bitrate limit) Would like to avoid be too specific about the current implementation. https://google.github.io/styleguide/cppguide.html#Function_Comments "Declaration comments describe use of the function" e.g. it might make sense to notify using tmmbr instead of remb. https://codereview.webrtc.org/2994513002/diff/40001/webrtc/modules/pacing/pac... webrtc/modules/pacing/packet_router.h:88: void SetMaxEstimatedBandwidth(uint32_t bitrate_bps); On 2017/08/04 08:58:14, stefan-webrtc wrote: > Isn't it better to give this a name that reflects what it's used for? E.g., > SetMaxDesiredReceiveBitrate() Done. That looks better, thank you.
Stefan, do you have more concerns with this feature?
lgtm
The CQ bit was checked by danilchap@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from eladalon@webrtc.org Link to the patchset: https://codereview.webrtc.org/2994513002/#ps60001 (title: "Rename the function")
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": 60001, "attempt_start_ts": 1502367731414380,
"parent_rev": "541280a8ca21033b495c3a1c543049f60dd2febe", "commit_rev":
"4708537f0d299bed59cad6f0917f608e1f8569ab"}
Message was sent while issue was closed.
Description was changed from ========== Add PacketRouter::SetMaxDesiredReceiveBitrate for application limited receive bandwidth BUG=webrtc:7135 ========== to ========== Add PacketRouter::SetMaxDesiredReceiveBitrate for application limited receive bandwidth BUG=webrtc:7135 Review-Url: https://codereview.webrtc.org/2994513002 Cr-Commit-Position: refs/heads/master@{#19307} Committed: https://chromium.googlesource.com/external/webrtc/+/4708537f0d299bed59cad6f09... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/external/webrtc/+/4708537f0d299bed59cad6f09... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
