Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2035383002:
Implementing auto pausing of video streams. (Closed)
|
Created:
4 years, 6 months ago by mflodman Modified:
4 years, 6 months ago CC:
webrtc-reviews_webrtc.org, video-team_agora.io, yujie_mao (webrtc), zhengzhonghou_agora.io, stefan-webrtc, tterriberry_mozilla.com, the sun, perkj_webrtc Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionImplementing auto pausing of video streams.
This CL implements auto pausing video streams per stream with logic to
avoid toggling state too often.
Also re-enabling tests disabled for Mac, with the assumption the new
logic removes flakiness.
BUG=webrtc:5868, webrtc:5407
R=pbos@webrtc.org, stefan@webrtc.org
Committed: https://crrev.com/101f250a30d3f5d0e157cf4569af9384a98f6696
Cr-Commit-Position: refs/heads/master@{#13092}
Patch Set 1 #
Total comments: 35
Patch Set 2 : #Patch Set 3 : Win build fix. #
Total comments: 1
Patch Set 4 : Inverted EnoughBitrateForAllObservers return value. #Patch Set 5 : Removed comment. #
Messages
Total messages: 25 (7 generated)
mflodman@webrtc.org changed reviewers: + stefan@webrtc.org
Stefan, Please take a look.
pbos@webrtc.org changed reviewers: + pbos@webrtc.org
Some drive-by comments, not sure if they're useful for this CL, so at your discretion. Also I couldn't figure out where the suspension option got hooked in. Where's that now? (Since it got removed from webrtc/video/video_send_stream.cc.) https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator.cc File webrtc/call/bitrate_allocator.cc (right): https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... webrtc/call/bitrate_allocator.cc:113: remove https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... webrtc/call/bitrate_allocator.cc:120: uint32_t sum_min_bitrates = 0; _bps https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... webrtc/call/bitrate_allocator.cc:154: int64_t remaining_bitrate = bitrate; _bps https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... webrtc/call/bitrate_allocator.cc:156: int32_t allocated_bitrate = 0; _bps https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... webrtc/call/bitrate_allocator.cc:202: uint32_t bitrate, _bps here and everywhere https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator.h File webrtc/call/bitrate_allocator.h (right): https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... webrtc/call/bitrate_allocator.h:69: // Removes a previously added observer, but will not trigger a new bitrate Should this not trigger a new bitrate allocation? If so why? https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... webrtc/call/bitrate_allocator.h:104: ObserverAllocation NormalRateAllocation(uint32_t bitrate, _bps on everything here please https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... File webrtc/call/bitrate_allocator_unittest.cc (right): https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... webrtc/call/bitrate_allocator_unittest.cc:33: uint32_t last_bitrate_; can you add _bps_ here? https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... webrtc/call/bitrate_allocator_unittest.cc:35: int64_t last_rtt_; rtt_ms_ https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... webrtc/call/bitrate_allocator_unittest.cc:134: EXPECT_EQ(0u, bitrate_observer_1.last_bitrate_); Does this still verify EnforceMinBitrate() or should the comment above be updated? https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/call.cc#newcode681 webrtc/call/call.cc:681: int pad_up_to_bitrate_bps = 0; Should this be an uint32_t? https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/call.cc#newcode705 webrtc/call/call.cc:705: // Make sure to not ask for more padding than the current BWE allows for. Should this be controlled in here or capped inside? (E.g. does CongestionController know about the max bitrate?)
On 2016/06/06 15:26:29, pbos-webrtc wrote: > Some drive-by comments, not sure if they're useful for this CL, so at your > discretion. > > Also I couldn't figure out where the suspension option got hooked in. Where's > that now? (Since it got removed from webrtc/video/video_send_stream.cc.) > > https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator.cc > File webrtc/call/bitrate_allocator.cc (right): > > https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... > webrtc/call/bitrate_allocator.cc:113: > remove > > https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... > webrtc/call/bitrate_allocator.cc:120: uint32_t sum_min_bitrates = 0; > _bps > > https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... > webrtc/call/bitrate_allocator.cc:154: int64_t remaining_bitrate = bitrate; > _bps > > https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... > webrtc/call/bitrate_allocator.cc:156: int32_t allocated_bitrate = 0; > _bps > > https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... > webrtc/call/bitrate_allocator.cc:202: uint32_t bitrate, > _bps here and everywhere > > https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator.h > File webrtc/call/bitrate_allocator.h (right): > > https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... > webrtc/call/bitrate_allocator.h:69: // Removes a previously added observer, but > will not trigger a new bitrate > Should this not trigger a new bitrate allocation? If so why? > > https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... > webrtc/call/bitrate_allocator.h:104: ObserverAllocation > NormalRateAllocation(uint32_t bitrate, > _bps on everything here please > > https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... > File webrtc/call/bitrate_allocator_unittest.cc (right): > > https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... > webrtc/call/bitrate_allocator_unittest.cc:33: uint32_t last_bitrate_; > can you add _bps_ here? > > https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... > webrtc/call/bitrate_allocator_unittest.cc:35: int64_t last_rtt_; > rtt_ms_ > > https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... > webrtc/call/bitrate_allocator_unittest.cc:134: EXPECT_EQ(0u, > bitrate_observer_1.last_bitrate_); > Does this still verify EnforceMinBitrate() or should the comment above be > updated? > > https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/call.cc > File webrtc/call/call.cc (right): > > https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/call.cc#newcode681 > webrtc/call/call.cc:681: int pad_up_to_bitrate_bps = 0; > Should this be an uint32_t? > > https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/call.cc#newcode705 > webrtc/call/call.cc:705: // Make sure to not ask for more padding than the > current BWE allows for. > Should this be controlled in here or capped inside? (E.g. does > CongestionController know about the max bitrate?) Won't update the CL today, but a general quick reply. I'd prefer to avoid using bps at all places you suggest. There is a comment regarding that in the header and all public methods have bps in the variable name. It is still hooked up in VideoSendStream, but per observer. That is the main pint, to get it per observer and not for the allocator. I'll reply to the other comments once I update the code.
On 2016/06/06 15:49:07, mflodman wrote: > On 2016/06/06 15:26:29, pbos-webrtc wrote: > > Some drive-by comments, not sure if they're useful for this CL, so at your > > discretion. > > > > Also I couldn't figure out where the suspension option got hooked in. Where's > > that now? (Since it got removed from webrtc/video/video_send_stream.cc.) > > > > > https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator.cc > > File webrtc/call/bitrate_allocator.cc (right): > > > > > https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... > > webrtc/call/bitrate_allocator.cc:113: > > remove > > > > > https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... > > webrtc/call/bitrate_allocator.cc:120: uint32_t sum_min_bitrates = 0; > > _bps > > > > > https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... > > webrtc/call/bitrate_allocator.cc:154: int64_t remaining_bitrate = bitrate; > > _bps > > > > > https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... > > webrtc/call/bitrate_allocator.cc:156: int32_t allocated_bitrate = 0; > > _bps > > > > > https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... > > webrtc/call/bitrate_allocator.cc:202: uint32_t bitrate, > > _bps here and everywhere > > > > > https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator.h > > File webrtc/call/bitrate_allocator.h (right): > > > > > https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... > > webrtc/call/bitrate_allocator.h:69: // Removes a previously added observer, > but > > will not trigger a new bitrate > > Should this not trigger a new bitrate allocation? If so why? > > > > > https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... > > webrtc/call/bitrate_allocator.h:104: ObserverAllocation > > NormalRateAllocation(uint32_t bitrate, > > _bps on everything here please > > > > > https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... > > File webrtc/call/bitrate_allocator_unittest.cc (right): > > > > > https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... > > webrtc/call/bitrate_allocator_unittest.cc:33: uint32_t last_bitrate_; > > can you add _bps_ here? > > > > > https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... > > webrtc/call/bitrate_allocator_unittest.cc:35: int64_t last_rtt_; > > rtt_ms_ > > > > > https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... > > webrtc/call/bitrate_allocator_unittest.cc:134: EXPECT_EQ(0u, > > bitrate_observer_1.last_bitrate_); > > Does this still verify EnforceMinBitrate() or should the comment above be > > updated? > > > > https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/call.cc > > File webrtc/call/call.cc (right): > > > > https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/call.cc#newcode681 > > webrtc/call/call.cc:681: int pad_up_to_bitrate_bps = 0; > > Should this be an uint32_t? > > > > https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/call.cc#newcode705 > > webrtc/call/call.cc:705: // Make sure to not ask for more padding than the > > current BWE allows for. > > Should this be controlled in here or capped inside? (E.g. does > > CongestionController know about the max bitrate?) > > Won't update the CL today, but a general quick reply. > > I'd prefer to avoid using bps at all places you suggest. There is a comment > regarding that in the header and all public methods have bps in the variable > name. > > It is still hooked up in VideoSendStream, but per observer. That is the main > pint, to get it per observer and not for the allocator. > > I'll reply to the other comments once I update the code. Right, of course it's specified twice, missed the AddObserver call already having this. :) Will you remove the MediaOpt suspend-below-min-bitrate stuff in another CL or should this go here as well?
On 2016/06/06 16:05:33, pbos-webrtc wrote: > On 2016/06/06 15:49:07, mflodman wrote: > > On 2016/06/06 15:26:29, pbos-webrtc wrote: > > > Some drive-by comments, not sure if they're useful for this CL, so at your > > > discretion. > > > > > > Also I couldn't figure out where the suspension option got hooked in. > Where's > > > that now? (Since it got removed from webrtc/video/video_send_stream.cc.) > > > > > > > > > https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator.cc > > > File webrtc/call/bitrate_allocator.cc (right): > > > > > > > > > https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... > > > webrtc/call/bitrate_allocator.cc:113: > > > remove > > > > > > > > > https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... > > > webrtc/call/bitrate_allocator.cc:120: uint32_t sum_min_bitrates = 0; > > > _bps > > > > > > > > > https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... > > > webrtc/call/bitrate_allocator.cc:154: int64_t remaining_bitrate = bitrate; > > > _bps > > > > > > > > > https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... > > > webrtc/call/bitrate_allocator.cc:156: int32_t allocated_bitrate = 0; > > > _bps > > > > > > > > > https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... > > > webrtc/call/bitrate_allocator.cc:202: uint32_t bitrate, > > > _bps here and everywhere > > > > > > > > > https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator.h > > > File webrtc/call/bitrate_allocator.h (right): > > > > > > > > > https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... > > > webrtc/call/bitrate_allocator.h:69: // Removes a previously added observer, > > but > > > will not trigger a new bitrate > > > Should this not trigger a new bitrate allocation? If so why? > > > > > > > > > https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... > > > webrtc/call/bitrate_allocator.h:104: ObserverAllocation > > > NormalRateAllocation(uint32_t bitrate, > > > _bps on everything here please > > > > > > > > > https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... > > > File webrtc/call/bitrate_allocator_unittest.cc (right): > > > > > > > > > https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... > > > webrtc/call/bitrate_allocator_unittest.cc:33: uint32_t last_bitrate_; > > > can you add _bps_ here? > > > > > > > > > https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... > > > webrtc/call/bitrate_allocator_unittest.cc:35: int64_t last_rtt_; > > > rtt_ms_ > > > > > > > > > https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... > > > webrtc/call/bitrate_allocator_unittest.cc:134: EXPECT_EQ(0u, > > > bitrate_observer_1.last_bitrate_); > > > Does this still verify EnforceMinBitrate() or should the comment above be > > > updated? > > > > > > https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/call.cc > > > File webrtc/call/call.cc (right): > > > > > > > https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/call.cc#newcode681 > > > webrtc/call/call.cc:681: int pad_up_to_bitrate_bps = 0; > > > Should this be an uint32_t? > > > > > > > https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/call.cc#newcode705 > > > webrtc/call/call.cc:705: // Make sure to not ask for more padding than the > > > current BWE allows for. > > > Should this be controlled in here or capped inside? (E.g. does > > > CongestionController know about the max bitrate?) > > > > Won't update the CL today, but a general quick reply. > > > > I'd prefer to avoid using bps at all places you suggest. There is a comment > > regarding that in the header and all public methods have bps in the variable > > name. > > > > It is still hooked up in VideoSendStream, but per observer. That is the main > > pint, to get it per observer and not for the allocator. > > > > I'll reply to the other comments once I update the code. > > Right, of course it's specified twice, missed the AddObserver call already > having this. :) > > Will you remove the MediaOpt suspend-below-min-bitrate stuff in another CL or > should this go here as well? I'll follow up with that right after, trying to keep this one simple and focused. Also, let me now if you agree about not using _bps. That is what we're using everywhere but in the Call API, so I thought it would be enough with the public methods for this class.
https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator.cc File webrtc/call/bitrate_allocator.cc (right): https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... webrtc/call/bitrate_allocator.cc:130: if (ShouldDoLowRateAllocation(bitrate, sum_min_bitrates)) Should we call this something like EnoughBitrateForAllObservers() instead? It better describes the condition, which I think makes it easier to read this code. https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... webrtc/call/bitrate_allocator.cc:178: // Allocate bitrate to previously paused streams. remove extra space after Allocate https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... webrtc/call/bitrate_allocator.cc:184: // Add a hysteresis to avoid toggling. Maybe this comment should go into MinBitrateForObserver instead? Or rename the method MinBitrateWithHysteresis()? https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... webrtc/call/bitrate_allocator.cc:208: // DistributeBitrateEvenly. Why? https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... webrtc/call/bitrate_allocator.cc:224: ObserverSortingMap list_max_bitrates; Not used.
On 2016/06/07 04:15:32, mflodman wrote: > Also, let me now if you agree about not using _bps. That is what we're using > everywhere but in the Call API, so I thought it would be enough with the public > methods for this class. I'd prefer using _bps everywhere, like we use time_ms or time_us for time. We have kbps code (even though we don't like it), and I'd prefer _bps for fast readability even if everything should be and is bps. New team members will also read _bps faster than finding any comments/practice.
Comments addressed, PTAL Peter and Stefan. https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator.cc File webrtc/call/bitrate_allocator.cc (right): https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... webrtc/call/bitrate_allocator.cc:113: On 2016/06/06 15:26:28, pbos-webrtc wrote: > remove Done. https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... webrtc/call/bitrate_allocator.cc:120: uint32_t sum_min_bitrates = 0; On 2016/06/06 15:26:28, pbos-webrtc wrote: > _bps See my previous reply and question, ignore the similar comments until we've talked about it. https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... webrtc/call/bitrate_allocator.cc:130: if (ShouldDoLowRateAllocation(bitrate, sum_min_bitrates)) On 2016/06/08 10:18:31, stefan-webrtc (holmer) wrote: > Should we call this something like EnoughBitrateForAllObservers() instead? It > better describes the condition, which I think makes it easier to read this code. That is a good suggestion. https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... webrtc/call/bitrate_allocator.cc:178: // Allocate bitrate to previously paused streams. On 2016/06/08 10:18:31, stefan-webrtc (holmer) wrote: > remove extra space after Allocate Done. https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... webrtc/call/bitrate_allocator.cc:184: // Add a hysteresis to avoid toggling. On 2016/06/08 10:18:31, stefan-webrtc (holmer) wrote: > Maybe this comment should go into MinBitrateForObserver instead? Or rename the > method MinBitrateWithHysteresis()? Yes, I'll rename to this suggestion. https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... webrtc/call/bitrate_allocator.cc:208: // DistributeBitrateEvenly. On 2016/06/08 10:18:31, stefan-webrtc (holmer) wrote: > Why? Bc DistributeBitrateEvenly only adds bitrate to observers with a non-zero allocation. That was a 'Sunday-before-morning-coffee-implementation' and I've updated it now to be a bit nicer. https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... webrtc/call/bitrate_allocator.cc:224: ObserverSortingMap list_max_bitrates; On 2016/06/08 10:18:31, stefan-webrtc (holmer) wrote: > Not used. Thanks, leftovers after I created DistributeBitrateEvenly. https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator.h File webrtc/call/bitrate_allocator.h (right): https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... webrtc/call/bitrate_allocator.h:69: // Removes a previously added observer, but will not trigger a new bitrate On 2016/06/06 15:26:28, pbos-webrtc wrote: > Should this not trigger a new bitrate allocation? If so why? It has never done it and there is no urgency to actually trigger it for this case, nothing is getting worse on the network and we might as well wait for the next "real" update. https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... webrtc/call/bitrate_allocator.h:104: ObserverAllocation NormalRateAllocation(uint32_t bitrate, On 2016/06/06 15:26:28, pbos-webrtc wrote: > _bps on everything here please See my previous reply. https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... File webrtc/call/bitrate_allocator_unittest.cc (right): https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... webrtc/call/bitrate_allocator_unittest.cc:33: uint32_t last_bitrate_; On 2016/06/06 15:26:29, pbos-webrtc wrote: > can you add _bps_ here? Done. https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... webrtc/call/bitrate_allocator_unittest.cc:35: int64_t last_rtt_; On 2016/06/06 15:26:29, pbos-webrtc wrote: > rtt_ms_ Done. https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... webrtc/call/bitrate_allocator_unittest.cc:134: EXPECT_EQ(0u, bitrate_observer_1.last_bitrate_); On 2016/06/06 15:26:29, pbos-webrtc wrote: > Does this still verify EnforceMinBitrate() or should the comment above be > updated? Comment updated. https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/call.cc#newcode681 webrtc/call/call.cc:681: int pad_up_to_bitrate_bps = 0; On 2016/06/06 15:26:29, pbos-webrtc wrote: > Should this be an uint32_t? I can take care of this in a follow up to not make this CL too large, it would require changes in VideoSendStream to return correct type from there. https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/call.cc#newcode705 webrtc/call/call.cc:705: // Make sure to not ask for more padding than the current BWE allows for. On 2016/06/06 15:26:29, pbos-webrtc wrote: > Should this be controlled in here or capped inside? (E.g. does > CongestionController know about the max bitrate?) That is a good question, but Per is reworking this entirely in another CL (already up for review) and I'd prefer to keep it this way to make his change easier. It will go in very soon. OK?
lgtm https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator.h File webrtc/call/bitrate_allocator.h (right): https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... webrtc/call/bitrate_allocator.h:69: // Removes a previously added observer, but will not trigger a new bitrate On 2016/06/09 13:23:13, mflodman wrote: > On 2016/06/06 15:26:28, pbos-webrtc wrote: > > Should this not trigger a new bitrate allocation? If so why? > > It has never done it and there is no urgency to actually trigger it for this > case, nothing is getting worse on the network and we might as well wait for the > next "real" update. Is there a guarantee that a new estimate will come? https://codereview.webrtc.org/2035383002/diff/40001/webrtc/call/bitrate_alloc... File webrtc/call/bitrate_allocator.cc (right): https://codereview.webrtc.org/2035383002/diff/40001/webrtc/call/bitrate_alloc... webrtc/call/bitrate_allocator.cc:129: if (EnoughBitrateForAllObservers(bitrate, sum_min_bitrates)) !Enough, but preferably !BitrateSufficientForAllObservers or !BitrateEnoughForAllObservers
https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator.cc File webrtc/call/bitrate_allocator.cc (right): https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... webrtc/call/bitrate_allocator.cc:130: if (ShouldDoLowRateAllocation(bitrate, sum_min_bitrates)) On 2016/06/09 13:23:12, mflodman wrote: > On 2016/06/08 10:18:31, stefan-webrtc (holmer) wrote: > > Should we call this something like EnoughBitrateForAllObservers() instead? It > > better describes the condition, which I think makes it easier to read this > code. > > That is a good suggestion. Probably should be inverted so that the check is: if (!EnoughBitrate...()) LowRateAllocation(bitrate); https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... webrtc/call/bitrate_allocator.cc:208: // DistributeBitrateEvenly. On 2016/06/09 13:23:12, mflodman wrote: > On 2016/06/08 10:18:31, stefan-webrtc (holmer) wrote: > > Why? > > Bc DistributeBitrateEvenly only adds bitrate to observers with a non-zero > allocation. That was a 'Sunday-before-morning-coffee-implementation' and I've > updated it now to be a bit nicer. > Thanks! Should you also update the comment now?
lgtm
lgtm
lgtm
https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator.cc File webrtc/call/bitrate_allocator.cc (right): https://codereview.webrtc.org/2035383002/diff/1/webrtc/call/bitrate_allocator... webrtc/call/bitrate_allocator.cc:208: // DistributeBitrateEvenly. On 2016/06/09 13:51:25, stefan-webrtc (holmer) wrote: > On 2016/06/09 13:23:12, mflodman wrote: > > On 2016/06/08 10:18:31, stefan-webrtc (holmer) wrote: > > > Why? > > > > Bc DistributeBitrateEvenly only adds bitrate to observers with a non-zero > > allocation. That was a 'Sunday-before-morning-coffee-implementation' and I've > > updated it now to be a bit nicer. > > > > Thanks! Should you also update the comment now? Done.
The CQ bit was checked by mflodman@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/2035383002/#ps80001 (title: "Removed comment.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2035383002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/14029)
Description was changed from ========== Implementing auto pausing of video streams. This CL implements auto pausing video streams per stream with logic to avoid toggling state too often. Also re-enabling tests disabled for Mac, with the assumption the new logic removes flakiness. BUG=webrtc:5868,webrtc:5407 ========== to ========== Implementing auto pausing of video streams. This CL implements auto pausing video streams per stream with logic to avoid toggling state too often. Also re-enabling tests disabled for Mac, with the assumption the new logic removes flakiness. BUG=webrtc:5868,webrtc:5407 R=pbos@webrtc.org, stefan@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/101f250a30d3f5d0e157cf456... ==========
Message was sent while issue was closed.
Description was changed from ========== Implementing auto pausing of video streams. This CL implements auto pausing video streams per stream with logic to avoid toggling state too often. Also re-enabling tests disabled for Mac, with the assumption the new logic removes flakiness. BUG=webrtc:5868,webrtc:5407 R=pbos@webrtc.org, stefan@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/101f250a30d3f5d0e157cf456... ========== to ========== Implementing auto pausing of video streams. This CL implements auto pausing video streams per stream with logic to avoid toggling state too often. Also re-enabling tests disabled for Mac, with the assumption the new logic removes flakiness. BUG=webrtc:5868,webrtc:5407 R=pbos@webrtc.org, stefan@webrtc.org Committed: https://crrev.com/101f250a30d3f5d0e157cf4569af9384a98f6696 Cr-Commit-Position: refs/heads/master@{#13092} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/101f250a30d3f5d0e157cf4569af9384a98f6696 Cr-Commit-Position: refs/heads/master@{#13092}
Message was sent while issue was closed.
Committed patchset #5 (id:80001) manually as 101f250a30d3f5d0e157cf4569af9384a98f6696 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
