Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years, 7 months ago by perkj_webrtc Modified:
4 years, 6 months ago Reviewers:
NELAS.SEXISLIMNWIX, pbos-webrtc, stefan-webrtc, mflodman CC:
webrtc-reviews_webrtc.org, video-team_agora.io, yujie_mao (webrtc), zhengzhonghou_agora.io, stefan-webrtc, tterriberry_mozilla.com, the sun, pbos-webrtc Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionThis cl:
1. It moves calculation of the needed padding to VideoSendStream instead of ViEEncoder and only does it once per send Stream instead of every time the network estimate changes.
2. The maximum amount of padding sent was prior to this cl calculated and updated based on network estimate changes. However, it can only change based on encoder configuration changes and if send streams are added or removed. This cl change the VideoSendStream/VieEncoder to notify the BitrateAllocator of changes to the needed padding bitrate and for BitrateAllocator to notify Call of these changes.
3. Fixed an issue in the SendPacer where it could send a padding packet before sending a real packet. This caused the test EndToEndTest.RestartingSendStreamPreservesRtpStatesWithRtx to fail with these refactorings since the pacer suddenly could send a padding packet before the encoder had produced its first frame.
BUG=webrtc:5687
Committed: https://crrev.com/71ee44cc6d3049763da69e8e42a08d4b796b97db
Cr-Commit-Position: refs/heads/master@{#13149}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : More work. Seems to work now. #Patch Set 4 : Self review #
Total comments: 1
Patch Set 5 : Rebased #Patch Set 6 : Rebased #Patch Set 7 : Fixed unitialized memory. #
Total comments: 30
Patch Set 8 : Addressed review comments. #Patch Set 9 : #
Total comments: 16
Patch Set 10 : Rebased #Patch Set 11 : Addressed review comments. #
Created: 4 years, 6 months ago
Dependent Patchsets: Messages
Total messages: 31 (14 generated)
Description was changed from ========== Calculate the needed padding in BitrateAllocator instead of ViEEncoder. BUG= ========== to ========== Calculate the needed padding in BitrateAllocator instead of ViEEncoder. BUG= ==========
Description was changed from ========== Calculate the needed padding in BitrateAllocator instead of ViEEncoder. BUG= ========== to ========== Calculate the needed padding in VideoSendStream instead of ViEEncoder. BUG= ==========
Description was changed from ========== Calculate the needed padding in VideoSendStream instead of ViEEncoder. BUG= ========== to ========== This cl fixes a few issues. 1. It moves calculation of the needed padding to VideoSendStream instead of ViEEncoder and only does it once per send Stream instead of every time the network estimate changes. 2. The maximum amount of padding sent was prior to this cl calculated and updated based on network estimate changes. However, it can only change based on encoder configuration changes and if send streams are added or removed. This cl change the VideoSendStream/VieEncoder to notify the BitrateAllocator of changes to the needed padding bitrate and for BitrateAllocator to notify Call of these changes. BUG=webrtc:5687 ==========
Description was changed from ========== This cl fixes a few issues. 1. It moves calculation of the needed padding to VideoSendStream instead of ViEEncoder and only does it once per send Stream instead of every time the network estimate changes. 2. The maximum amount of padding sent was prior to this cl calculated and updated based on network estimate changes. However, it can only change based on encoder configuration changes and if send streams are added or removed. This cl change the VideoSendStream/VieEncoder to notify the BitrateAllocator of changes to the needed padding bitrate and for BitrateAllocator to notify Call of these changes. BUG=webrtc:5687 ========== to ========== This cl: 1. It moves calculation of the needed padding to VideoSendStream instead of ViEEncoder and only does it once per send Stream instead of every time the network estimate changes. 2. The maximum amount of padding sent was prior to this cl calculated and updated based on network estimate changes. However, it can only change based on encoder configuration changes and if send streams are added or removed. This cl change the VideoSendStream/VieEncoder to notify the BitrateAllocator of changes to the needed padding bitrate and for BitrateAllocator to notify Call of these changes. 3. Fixed an issue in the SendPacer where it could send a padding packet before sending a real packet. This caused the test EndToEndTest.RestartingSendStreamPreservesRtpStatesWithRtx to fail with these refactorings since the pacer suddenly could send a padding packet before the encoder had produced its first frame. BUG=webrtc:5687 ==========
perkj@webrtc.org changed reviewers: + mflodman@webrtc.org, pbos@webrtc.org, stefan@webrtc.org
Please?
friendly ping Magnus and Stefan.
nelas.sexislimnwix@gmail.com changed reviewers: + NELAS.SEXISLIMNWIX@gmail.com
lgtm
perkj@webrtc.org changed reviewers: + nelas.sexislimnwix@gmail.com - NELAS.SEXISLIMNWIX@gmail.com
Pretty please when there are no other emergencies? Two weeks has passed without a review?
On 2016/06/08 08:59:30, perkj_webrtc wrote: > Pretty please when there are no other emergencies? Two weeks has passed without > a review? Sorry Per, this fell off when you were on vacation. Reviewing...
https://codereview.webrtc.org/1993113003/diff/60001/webrtc/call/bitrate_alloc... File webrtc/call/bitrate_allocator.h (right): https://codereview.webrtc.org/1993113003/diff/60001/webrtc/call/bitrate_alloc... webrtc/call/bitrate_allocator.h:35: protected: Why is this needed? https://codereview.webrtc.org/1993113003/diff/120001/webrtc/call/bitrate_allo... File webrtc/call/bitrate_allocator.cc (right): https://codereview.webrtc.org/1993113003/diff/120001/webrtc/call/bitrate_allo... webrtc/call/bitrate_allocator.cc:128: crit_sect_.Leave(); I don't like this... Can we make sure we can OnAllocationLimitsChanged() after the crit is no longer active? You should be able to scope the crit in AddObserver() so that it is no longer active once you call OnAllocationLimitsChanged at the end. Same for NotifyObserverInactive. https://codereview.webrtc.org/1993113003/diff/120001/webrtc/call/bitrate_allo... File webrtc/call/bitrate_allocator.h (right): https://codereview.webrtc.org/1993113003/diff/120001/webrtc/call/bitrate_allo... webrtc/call/bitrate_allocator.h:87: // not be sent. I know this is how it's behaving today, but is it what we want? I'm thinking whether we may actually want to remove the observer instead and simply add it back once the encoder becomes active? What would be the implications of that? Magnus, what's your feeling about that? https://codereview.webrtc.org/1993113003/diff/120001/webrtc/modules/pacing/pa... File webrtc/modules/pacing/paced_sender.cc (right): https://codereview.webrtc.org/1993113003/diff/120001/webrtc/modules/pacing/pa... webrtc/modules/pacing/paced_sender.cc:372: std::min(estimated_bitrate_bps_ / 1000, max_padding_bitrate_kbps_)); Why was this change needed? Seems like we're setting it more often than needed now? https://codereview.webrtc.org/1993113003/diff/120001/webrtc/modules/pacing/pa... webrtc/modules/pacing/paced_sender.cc:424: // do, time stamps gets messed up. "timestamps get messsed up" https://codereview.webrtc.org/1993113003/diff/120001/webrtc/video/video_send_... File webrtc/video/video_send_stream.cc (right): https://codereview.webrtc.org/1993113003/diff/120001/webrtc/video/video_send_... webrtc/video/video_send_stream.cc:350: // Calulate max padding bitrate for a multi layer codec. Calculate https://codereview.webrtc.org/1993113003/diff/120001/webrtc/video/video_send_... webrtc/video/video_send_stream.cc:355: // + target_bitrate_bps of the lower layers. Add instead of +. https://codereview.webrtc.org/1993113003/diff/120001/webrtc/video/video_send_... webrtc/video/video_send_stream.cc:356: for (size_t i = 0; i < config.streams.size() - 1; ++i) { Remove {} https://codereview.webrtc.org/1993113003/diff/120001/webrtc/video/video_send_... webrtc/video/video_send_stream.cc:561: const int kEncodeCheckForActiviyPeriodMs = 1000; Activity And please add a comment why we have to wake up periodically. https://codereview.webrtc.org/1993113003/diff/120001/webrtc/video/video_send_... webrtc/video/video_send_stream.cc:625: // TODO(perkj): This is temporary. Activity should be notified by ViEncoder ViEEncoder https://codereview.webrtc.org/1993113003/diff/120001/webrtc/video/vie_encoder.cc File webrtc/video/vie_encoder.cc (right): https://codereview.webrtc.org/1993113003/diff/120001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.cc:223: // VieEncoder::GetPaddingBitrate was by Call for each OnBitrateUpdated. This comment needs to be rewritten https://codereview.webrtc.org/1993113003/diff/120001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.cc:234: LOG(LS_INFO) << "ViEncoder timed out."; ViEEncoder
perkj@webrtc.org changed reviewers: - nelas.sexislimnwix@gmail.com
An initial review more about the high level than the details. I haven't been part of all the discussions you've had, so you might have talked about it already. https://codereview.webrtc.org/1993113003/diff/120001/webrtc/call/bitrate_allo... File webrtc/call/bitrate_allocator.cc (right): https://codereview.webrtc.org/1993113003/diff/120001/webrtc/call/bitrate_allo... webrtc/call/bitrate_allocator.cc:104: void BitrateAllocator::NotifyObserverInactive( Can we in this case just remove the observer instead? Or in another way, if it is inactive, wouldn't it be better to remove it from the allocation? https://codereview.webrtc.org/1993113003/diff/120001/webrtc/video/video_send_... File webrtc/video/video_send_stream.cc (right): https://codereview.webrtc.org/1993113003/diff/120001/webrtc/video/video_send_... webrtc/video/video_send_stream.cc:627: vie_encoder_.CheckForActivity(); Can this method return true / false instead and we remove the observer in case it's inactive? That way we don't need the sink implementation and we can avoid adding the active/inactive state in the allocator. https://codereview.webrtc.org/1993113003/diff/120001/webrtc/video/video_send_... webrtc/video/video_send_stream.cc:627: vie_encoder_.CheckForActivity(); If that is done, can we also update the congestion controller from here rather than from the allocator? The CC API would then be to add min bitrate and padding needed per stream rather than once for all. Doing that, we don't need to add the padding concept at all to the allocator. WDYT?
PTAL https://codereview.webrtc.org/1993113003/diff/120001/webrtc/call/bitrate_allo... File webrtc/call/bitrate_allocator.cc (right): https://codereview.webrtc.org/1993113003/diff/120001/webrtc/call/bitrate_allo... webrtc/call/bitrate_allocator.cc:104: void BitrateAllocator::NotifyObserverInactive( On 2016/06/08 10:18:31, mflodman wrote: > Can we in this case just remove the observer instead? > > Or in another way, if it is inactive, wouldn't it be better to remove it from > the allocation? Seems like that is what stefan suggests too. https://codereview.webrtc.org/1993113003/diff/120001/webrtc/call/bitrate_allo... webrtc/call/bitrate_allocator.cc:128: crit_sect_.Leave(); On 2016/06/08 09:25:13, stefan-webrtc (holmer) wrote: > I don't like this... Can we make sure we can OnAllocationLimitsChanged() after > the crit is no longer active? You should be able to scope the crit in > AddObserver() so that it is no longer active once you call > OnAllocationLimitsChanged at the end. Same for NotifyObserverInactive. Yes, but with code duplication. Do you have a suggestion? https://codereview.webrtc.org/1993113003/diff/120001/webrtc/call/bitrate_allo... File webrtc/call/bitrate_allocator.h (right): https://codereview.webrtc.org/1993113003/diff/120001/webrtc/call/bitrate_allo... webrtc/call/bitrate_allocator.h:87: // not be sent. On 2016/06/08 09:25:13, stefan-webrtc (holmer) wrote: > I know this is how it's behaving today, but is it what we want? I'm thinking > whether we may actually want to remove the observer instead and simply add it > back once the encoder becomes active? What would be the implications of that? > > Magnus, what's your feeling about that? ok- NotifyObserverInactive has been removed. https://codereview.webrtc.org/1993113003/diff/120001/webrtc/modules/pacing/pa... File webrtc/modules/pacing/paced_sender.cc (right): https://codereview.webrtc.org/1993113003/diff/120001/webrtc/modules/pacing/pa... webrtc/modules/pacing/paced_sender.cc:372: std::min(estimated_bitrate_bps_ / 1000, max_padding_bitrate_kbps_)); On 2016/06/08 09:25:13, stefan-webrtc (holmer) wrote: > Why was this change needed? Seems like we're setting it more often than needed > now? Done. https://codereview.webrtc.org/1993113003/diff/120001/webrtc/modules/pacing/pa... webrtc/modules/pacing/paced_sender.cc:424: // do, time stamps gets messed up. On 2016/06/08 09:25:13, stefan-webrtc (holmer) wrote: > "timestamps get messsed up" Done. https://codereview.webrtc.org/1993113003/diff/120001/webrtc/video/video_send_... File webrtc/video/video_send_stream.cc (right): https://codereview.webrtc.org/1993113003/diff/120001/webrtc/video/video_send_... webrtc/video/video_send_stream.cc:350: // Calulate max padding bitrate for a multi layer codec. On 2016/06/08 09:25:13, stefan-webrtc (holmer) wrote: > Calculate Done. https://codereview.webrtc.org/1993113003/diff/120001/webrtc/video/video_send_... webrtc/video/video_send_stream.cc:355: // + target_bitrate_bps of the lower layers. On 2016/06/08 09:25:13, stefan-webrtc (holmer) wrote: > Add instead of +. Done. https://codereview.webrtc.org/1993113003/diff/120001/webrtc/video/video_send_... webrtc/video/video_send_stream.cc:356: for (size_t i = 0; i < config.streams.size() - 1; ++i) { On 2016/06/08 09:25:13, stefan-webrtc (holmer) wrote: > Remove {} Done. https://codereview.webrtc.org/1993113003/diff/120001/webrtc/video/video_send_... webrtc/video/video_send_stream.cc:561: const int kEncodeCheckForActiviyPeriodMs = 1000; On 2016/06/08 09:25:13, stefan-webrtc (holmer) wrote: > Activity > > And please add a comment why we have to wake up periodically. Done. https://codereview.webrtc.org/1993113003/diff/120001/webrtc/video/video_send_... webrtc/video/video_send_stream.cc:625: // TODO(perkj): This is temporary. Activity should be notified by ViEncoder On 2016/06/08 09:25:13, stefan-webrtc (holmer) wrote: > ViEEncoder Acknowledged. https://codereview.webrtc.org/1993113003/diff/120001/webrtc/video/video_send_... webrtc/video/video_send_stream.cc:627: vie_encoder_.CheckForActivity(); On 2016/06/08 10:18:31, mflodman wrote: > If that is done, can we also update the congestion controller from here rather > than from the allocator? The CC API would then be to add min bitrate and padding > needed per stream rather than once for all. Doing that, we don't need to add the > padding concept at all to the allocator. > > WDYT? I don't think I can call the congestion controller directly. Its only the BitrateAllocator that can sum all observers requested padding rates. - the CongestionController does not know about the streams. Call however, do know about the streams so it can potentially sum if we really want. But I don't think it make sense to add Call* as a dependency to VideoSendStream as well. https://codereview.webrtc.org/1993113003/diff/120001/webrtc/video/vie_encoder.cc File webrtc/video/vie_encoder.cc (right): https://codereview.webrtc.org/1993113003/diff/120001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.cc:223: // VieEncoder::GetPaddingBitrate was by Call for each OnBitrateUpdated. On 2016/06/08 09:25:13, stefan-webrtc (holmer) wrote: > This comment needs to be rewritten Acknowledged. https://codereview.webrtc.org/1993113003/diff/120001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.cc:234: LOG(LS_INFO) << "ViEncoder timed out."; On 2016/06/08 09:25:13, stefan-webrtc (holmer) wrote: > ViEEncoder Acknowledged.
On 2016/06/08 15:35:27, perkj_webrtc wrote: > PTAL > > https://codereview.webrtc.org/1993113003/diff/120001/webrtc/call/bitrate_allo... > File webrtc/call/bitrate_allocator.cc (right): > > https://codereview.webrtc.org/1993113003/diff/120001/webrtc/call/bitrate_allo... > webrtc/call/bitrate_allocator.cc:104: void > BitrateAllocator::NotifyObserverInactive( > On 2016/06/08 10:18:31, mflodman wrote: > > Can we in this case just remove the observer instead? > > > > Or in another way, if it is inactive, wouldn't it be better to remove it from > > the allocation? > > Seems like that is what stefan suggests too. > > https://codereview.webrtc.org/1993113003/diff/120001/webrtc/call/bitrate_allo... > webrtc/call/bitrate_allocator.cc:128: crit_sect_.Leave(); > On 2016/06/08 09:25:13, stefan-webrtc (holmer) wrote: > > I don't like this... Can we make sure we can OnAllocationLimitsChanged() after > > the crit is no longer active? You should be able to scope the crit in > > AddObserver() so that it is no longer active once you call > > OnAllocationLimitsChanged at the end. Same for NotifyObserverInactive. > > Yes, but with code duplication. Do you have a suggestion? > > https://codereview.webrtc.org/1993113003/diff/120001/webrtc/call/bitrate_allo... > File webrtc/call/bitrate_allocator.h (right): > > https://codereview.webrtc.org/1993113003/diff/120001/webrtc/call/bitrate_allo... > webrtc/call/bitrate_allocator.h:87: // not be sent. > On 2016/06/08 09:25:13, stefan-webrtc (holmer) wrote: > > I know this is how it's behaving today, but is it what we want? I'm thinking > > whether we may actually want to remove the observer instead and simply add it > > back once the encoder becomes active? What would be the implications of that? > > > > Magnus, what's your feeling about that? > > ok- NotifyObserverInactive has been removed. > > https://codereview.webrtc.org/1993113003/diff/120001/webrtc/modules/pacing/pa... > File webrtc/modules/pacing/paced_sender.cc (right): > > https://codereview.webrtc.org/1993113003/diff/120001/webrtc/modules/pacing/pa... > webrtc/modules/pacing/paced_sender.cc:372: std::min(estimated_bitrate_bps_ / > 1000, max_padding_bitrate_kbps_)); > On 2016/06/08 09:25:13, stefan-webrtc (holmer) wrote: > > Why was this change needed? Seems like we're setting it more often than needed > > now? > > Done. > > https://codereview.webrtc.org/1993113003/diff/120001/webrtc/modules/pacing/pa... > webrtc/modules/pacing/paced_sender.cc:424: // do, time stamps gets messed up. > On 2016/06/08 09:25:13, stefan-webrtc (holmer) wrote: > > "timestamps get messsed up" > > Done. > > https://codereview.webrtc.org/1993113003/diff/120001/webrtc/video/video_send_... > File webrtc/video/video_send_stream.cc (right): > > https://codereview.webrtc.org/1993113003/diff/120001/webrtc/video/video_send_... > webrtc/video/video_send_stream.cc:350: // Calulate max padding bitrate for a > multi layer codec. > On 2016/06/08 09:25:13, stefan-webrtc (holmer) wrote: > > Calculate > > Done. > > https://codereview.webrtc.org/1993113003/diff/120001/webrtc/video/video_send_... > webrtc/video/video_send_stream.cc:355: // + target_bitrate_bps of the lower > layers. > On 2016/06/08 09:25:13, stefan-webrtc (holmer) wrote: > > Add instead of +. > > Done. > > https://codereview.webrtc.org/1993113003/diff/120001/webrtc/video/video_send_... > webrtc/video/video_send_stream.cc:356: for (size_t i = 0; i < > config.streams.size() - 1; ++i) { > On 2016/06/08 09:25:13, stefan-webrtc (holmer) wrote: > > Remove {} > > Done. > > https://codereview.webrtc.org/1993113003/diff/120001/webrtc/video/video_send_... > webrtc/video/video_send_stream.cc:561: const int kEncodeCheckForActiviyPeriodMs > = 1000; > On 2016/06/08 09:25:13, stefan-webrtc (holmer) wrote: > > Activity > > > > And please add a comment why we have to wake up periodically. > > Done. > > https://codereview.webrtc.org/1993113003/diff/120001/webrtc/video/video_send_... > webrtc/video/video_send_stream.cc:625: // TODO(perkj): This is temporary. > Activity should be notified by ViEncoder > On 2016/06/08 09:25:13, stefan-webrtc (holmer) wrote: > > ViEEncoder > > Acknowledged. > > https://codereview.webrtc.org/1993113003/diff/120001/webrtc/video/video_send_... > webrtc/video/video_send_stream.cc:627: vie_encoder_.CheckForActivity(); > On 2016/06/08 10:18:31, mflodman wrote: > > If that is done, can we also update the congestion controller from here rather > > than from the allocator? The CC API would then be to add min bitrate and > padding > > needed per stream rather than once for all. Doing that, we don't need to add > the > > padding concept at all to the allocator. > > > > WDYT? > > I don't think I can call the congestion controller directly. Its only the > BitrateAllocator that can sum all observers requested padding rates. - the > CongestionController does not know about the streams. Call however, do know > about the streams so it can potentially sum if we really want. But I don't think > it make sense to add Call* as a dependency to VideoSendStream as well. > > https://codereview.webrtc.org/1993113003/diff/120001/webrtc/video/vie_encoder.cc > File webrtc/video/vie_encoder.cc (right): > > https://codereview.webrtc.org/1993113003/diff/120001/webrtc/video/vie_encoder... > webrtc/video/vie_encoder.cc:223: // VieEncoder::GetPaddingBitrate was by Call > for each OnBitrateUpdated. > On 2016/06/08 09:25:13, stefan-webrtc (holmer) wrote: > > This comment needs to be rewritten > > Acknowledged. > > https://codereview.webrtc.org/1993113003/diff/120001/webrtc/video/vie_encoder... > webrtc/video/vie_encoder.cc:234: LOG(LS_INFO) << "ViEncoder timed out."; > On 2016/06/08 09:25:13, stefan-webrtc (holmer) wrote: > > ViEEncoder > > Acknowledged. Hello ?
https://codereview.webrtc.org/1993113003/diff/120001/webrtc/modules/pacing/pa... File webrtc/modules/pacing/paced_sender.cc (right): https://codereview.webrtc.org/1993113003/diff/120001/webrtc/modules/pacing/pa... webrtc/modules/pacing/paced_sender.cc:424: // do, time stamps gets messed up. On 2016/06/08 15:35:27, perkj_webrtc wrote: > On 2016/06/08 09:25:13, stefan-webrtc (holmer) wrote: > > "timestamps get messsed up" > > Done. I was referring to that "gets" should be "get" :) https://codereview.webrtc.org/1993113003/diff/160001/webrtc/modules/pacing/pa... File webrtc/modules/pacing/paced_sender.cc (right): https://codereview.webrtc.org/1993113003/diff/160001/webrtc/modules/pacing/pa... webrtc/modules/pacing/paced_sender.cc:426: // do, timestamps gets messed up. This isnt true if we are using rtx. Is this really needed? https://codereview.webrtc.org/1993113003/diff/160001/webrtc/video/video_send_... File webrtc/video/video_send_stream.cc (right): https://codereview.webrtc.org/1993113003/diff/160001/webrtc/video/video_send_... webrtc/video/video_send_stream.cc:564: // active. If not, deregiser as BitrateAllocatorObserver. deregister https://codereview.webrtc.org/1993113003/diff/160001/webrtc/video/video_send_... webrtc/video/video_send_stream.cc:637: LOG(LS_INFO) << "Encoder timed out."; Do LOG_F or make it more clear why it has timed out. https://codereview.webrtc.org/1993113003/diff/160001/webrtc/video/video_send_... webrtc/video/video_send_stream.cc:644: LOG(LS_INFO) << "Encoder is active."; Same here. https://codereview.webrtc.org/1993113003/diff/160001/webrtc/video/vie_encoder.h File webrtc/video/vie_encoder.h (right): https://codereview.webrtc.org/1993113003/diff/160001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.h:88: // handled internally within VideoSendStream once an internal timer Two extra spaces on this line. https://codereview.webrtc.org/1993113003/diff/160001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.h:89: // once ViEncoder owns the encoder thread. And this comment doesnt seem correct...
Thanks for the changes, I think this looks good! A couple of more small comments. https://codereview.webrtc.org/1993113003/diff/120001/webrtc/video/video_send_... File webrtc/video/video_send_stream.cc (right): https://codereview.webrtc.org/1993113003/diff/120001/webrtc/video/video_send_... webrtc/video/video_send_stream.cc:627: vie_encoder_.CheckForActivity(); On 2016/06/08 15:35:27, perkj_webrtc wrote: > On 2016/06/08 10:18:31, mflodman wrote: > > If that is done, can we also update the congestion controller from here rather > > than from the allocator? The CC API would then be to add min bitrate and > padding > > needed per stream rather than once for all. Doing that, we don't need to add > the > > padding concept at all to the allocator. > > > > WDYT? > > I don't think I can call the congestion controller directly. Its only the > BitrateAllocator that can sum all observers requested padding rates. - the > CongestionController does not know about the streams. Call however, do know > about the streams so it can potentially sum if we really want. But I don't think > it make sense to add Call* as a dependency to VideoSendStream as well. My intention was to let the CongestionController do the addition of bitrates and the send stream update only its own min and pad bitrates at the same time as an observer is registered / deregistered. This might still be better as you've done it though. Let's just discuss for one minute to make sure we understand each other and agree. https://codereview.webrtc.org/1993113003/diff/160001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/include/congestion_controller.h (right): https://codereview.webrtc.org/1993113003/diff/160001/webrtc/modules/congestio... webrtc/modules/congestion_controller/include/congestion_controller.h:82: // SetAllocatedSendBitrate sets bitrates limits imposed by send code Maybe SetAllocatedSendBitrateThresholds / Limits? To tell this is not just an allocation happening during regular bitrate allocation. https://codereview.webrtc.org/1993113003/diff/160001/webrtc/video/video_send_... File webrtc/video/video_send_stream.cc (right): https://codereview.webrtc.org/1993113003/diff/160001/webrtc/video/video_send_... webrtc/video/video_send_stream.cc:633: if (send_stream_registered_as_observer_ && Instead of having 'send_stream_registered_as_observer_' as a class member, can we just add a local 'bool frame_encoded = false; at line 621, then set to true if we did encode a frame in the if statement below that line?
Patchset #12 (id:220001) has been deleted
Patchset #11 (id:200001) has been deleted
PTAL https://codereview.webrtc.org/1993113003/diff/120001/webrtc/modules/pacing/pa... File webrtc/modules/pacing/paced_sender.cc (right): https://codereview.webrtc.org/1993113003/diff/120001/webrtc/modules/pacing/pa... webrtc/modules/pacing/paced_sender.cc:424: // do, time stamps gets messed up. On 2016/06/10 13:54:13, stefan-webrtc (holmer) wrote: > On 2016/06/08 15:35:27, perkj_webrtc wrote: > > On 2016/06/08 09:25:13, stefan-webrtc (holmer) wrote: > > > "timestamps get messsed up" > > > > Done. > > I was referring to that "gets" should be "get" :) oops https://codereview.webrtc.org/1993113003/diff/160001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/include/congestion_controller.h (right): https://codereview.webrtc.org/1993113003/diff/160001/webrtc/modules/congestio... webrtc/modules/congestion_controller/include/congestion_controller.h:82: // SetAllocatedSendBitrate sets bitrates limits imposed by send code On 2016/06/13 05:02:43, mflodman wrote: > Maybe SetAllocatedSendBitrateThresholds / Limits? To tell this is not just an > allocation happening during regular bitrate allocation. Done. https://codereview.webrtc.org/1993113003/diff/160001/webrtc/modules/pacing/pa... File webrtc/modules/pacing/paced_sender.cc (right): https://codereview.webrtc.org/1993113003/diff/160001/webrtc/modules/pacing/pa... webrtc/modules/pacing/paced_sender.cc:426: // do, timestamps gets messed up. On 2016/06/10 13:54:13, stefan-webrtc (holmer) wrote: > This isnt true if we are using rtx. Is this really needed? There was a test that failed due to that it checked timestamps and that the timestamps for padding packets should be the same the media packets according to pbos. So that required a media packet to be sent first. Now I can not find the failing test..... But doesn't this make sence regardless? https://codereview.webrtc.org/1993113003/diff/160001/webrtc/video/video_send_... File webrtc/video/video_send_stream.cc (right): https://codereview.webrtc.org/1993113003/diff/160001/webrtc/video/video_send_... webrtc/video/video_send_stream.cc:564: // active. If not, deregiser as BitrateAllocatorObserver. On 2016/06/10 13:54:13, stefan-webrtc (holmer) wrote: > deregister Done. https://codereview.webrtc.org/1993113003/diff/160001/webrtc/video/video_send_... webrtc/video/video_send_stream.cc:633: if (send_stream_registered_as_observer_ && On 2016/06/13 05:02:43, mflodman wrote: > Instead of having 'send_stream_registered_as_observer_' as a class member, can > we just add a local 'bool frame_encoded = false; at line 621, then set to true > if we did encode a frame in the if statement below that line? discussed offline. https://codereview.webrtc.org/1993113003/diff/160001/webrtc/video/video_send_... webrtc/video/video_send_stream.cc:637: LOG(LS_INFO) << "Encoder timed out."; On 2016/06/10 13:54:13, stefan-webrtc (holmer) wrote: > Do LOG_F or make it more clear why it has timed out. Done. https://codereview.webrtc.org/1993113003/diff/160001/webrtc/video/video_send_... webrtc/video/video_send_stream.cc:644: LOG(LS_INFO) << "Encoder is active."; On 2016/06/10 13:54:13, stefan-webrtc (holmer) wrote: > Same here. Done. https://codereview.webrtc.org/1993113003/diff/160001/webrtc/video/vie_encoder.h File webrtc/video/vie_encoder.h (right): https://codereview.webrtc.org/1993113003/diff/160001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.h:88: // handled internally within VideoSendStream once an internal timer On 2016/06/10 13:54:13, stefan-webrtc (holmer) wrote: > Two extra spaces on this line. Acknowledged. https://codereview.webrtc.org/1993113003/diff/160001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.h:89: // once ViEncoder owns the encoder thread. On 2016/06/10 13:54:13, stefan-webrtc (holmer) wrote: > And this comment doesnt seem correct... The whole comment is wrong.. sorry. I still think this should be done internally in VideoSend stream so that we can handle preencoded data the same way as this. But I will remove the todo for now since its probably confusing.
LGTM
The CQ bit was checked by perkj@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from NELAS.SEXISLIMNWIX@gmail.com Link to the patchset: https://codereview.webrtc.org/1993113003/#ps240001 (title: "Addressed review comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1993113003/240001
Message was sent while issue was closed.
Description was changed from ========== This cl: 1. It moves calculation of the needed padding to VideoSendStream instead of ViEEncoder and only does it once per send Stream instead of every time the network estimate changes. 2. The maximum amount of padding sent was prior to this cl calculated and updated based on network estimate changes. However, it can only change based on encoder configuration changes and if send streams are added or removed. This cl change the VideoSendStream/VieEncoder to notify the BitrateAllocator of changes to the needed padding bitrate and for BitrateAllocator to notify Call of these changes. 3. Fixed an issue in the SendPacer where it could send a padding packet before sending a real packet. This caused the test EndToEndTest.RestartingSendStreamPreservesRtpStatesWithRtx to fail with these refactorings since the pacer suddenly could send a padding packet before the encoder had produced its first frame. BUG=webrtc:5687 ========== to ========== This cl: 1. It moves calculation of the needed padding to VideoSendStream instead of ViEEncoder and only does it once per send Stream instead of every time the network estimate changes. 2. The maximum amount of padding sent was prior to this cl calculated and updated based on network estimate changes. However, it can only change based on encoder configuration changes and if send streams are added or removed. This cl change the VideoSendStream/VieEncoder to notify the BitrateAllocator of changes to the needed padding bitrate and for BitrateAllocator to notify Call of these changes. 3. Fixed an issue in the SendPacer where it could send a padding packet before sending a real packet. This caused the test EndToEndTest.RestartingSendStreamPreservesRtpStatesWithRtx to fail with these refactorings since the pacer suddenly could send a padding packet before the encoder had produced its first frame. BUG=webrtc:5687 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:240001)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== This cl: 1. It moves calculation of the needed padding to VideoSendStream instead of ViEEncoder and only does it once per send Stream instead of every time the network estimate changes. 2. The maximum amount of padding sent was prior to this cl calculated and updated based on network estimate changes. However, it can only change based on encoder configuration changes and if send streams are added or removed. This cl change the VideoSendStream/VieEncoder to notify the BitrateAllocator of changes to the needed padding bitrate and for BitrateAllocator to notify Call of these changes. 3. Fixed an issue in the SendPacer where it could send a padding packet before sending a real packet. This caused the test EndToEndTest.RestartingSendStreamPreservesRtpStatesWithRtx to fail with these refactorings since the pacer suddenly could send a padding packet before the encoder had produced its first frame. BUG=webrtc:5687 ========== to ========== This cl: 1. It moves calculation of the needed padding to VideoSendStream instead of ViEEncoder and only does it once per send Stream instead of every time the network estimate changes. 2. The maximum amount of padding sent was prior to this cl calculated and updated based on network estimate changes. However, it can only change based on encoder configuration changes and if send streams are added or removed. This cl change the VideoSendStream/VieEncoder to notify the BitrateAllocator of changes to the needed padding bitrate and for BitrateAllocator to notify Call of these changes. 3. Fixed an issue in the SendPacer where it could send a padding packet before sending a real packet. This caused the test EndToEndTest.RestartingSendStreamPreservesRtpStatesWithRtx to fail with these refactorings since the pacer suddenly could send a padding packet before the encoder had produced its first frame. BUG=webrtc:5687 Committed: https://crrev.com/71ee44cc6d3049763da69e8e42a08d4b796b97db Cr-Commit-Position: refs/heads/master@{#13149} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/71ee44cc6d3049763da69e8e42a08d4b796b97db Cr-Commit-Position: refs/heads/master@{#13149} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
