Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
276783002:
Cast: Reduce bitrate drastically when close to unacked frame limit (Closed)
|
Created:
6 years, 7 months ago by hubbe Modified:
6 years, 7 months ago CC:
chromium-reviews, hclam+watch_chromium.org, imcheng+watch_chromium.org, hguihot+watch_chromium.org, jasonroberts+watch_google.com, avayvod+watch_chromium.org, pwestin+watch_google.com, feature-media-reviews_chromium.org, miu+watch_chromium.org, hubbe+watch_chromium.org, mikhal+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionCast: Reduce bitrate drastically when close to unacked frame limit
Without this CL, I observed that the sender sometimes gets stuck trying to catch up.
I could provoke this by using the "bad" netork profile in the udp_proxy in combination
with the "Extreme" (4Mbit/s) sender profile. I could provoke the behaviour by killing
the proxy for a few seconds, and then observe that the mirrored video would remain
slow and jerky for a very long time. (Perhaps forever) With this patch, the video
catches up again after a few seconds.
BUG=374027
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269475
Patch Set 1 #
Total comments: 6
Patch Set 2 : helper function added #
Total comments: 3
Patch Set 3 : added dcheck #
Total comments: 2
Patch Set 4 : sign fix #
Messages
Total messages: 21 (0 generated)
Before we land this change. Do we understand why it is stuck? Is it because that bandwidth is throttled or the pacer is not allowing more out flow? I'm also a bit worried about the impact on quality, have we tested this and shows good quality under normal network condition? https://codereview.chromium.org/276783002/diff/1/media/cast/video_sender/vide... File media/cast/video_sender/video_sender.cc (right): https://codereview.chromium.org/276783002/diff/1/media/cast/video_sender/vide... media/cast/video_sender/video_sender.cc:189: current_requested_bitrate_ / current_bitrate_divider_); Can we have a helper method to compute this instead of duplicating the divide? https://codereview.chromium.org/276783002/diff/1/media/cast/video_sender/vide... media/cast/video_sender/video_sender.cc:392: video_encoder_->SetBitRate(new_bitrate / current_bitrate_divider_); This should really be a helper method. https://codereview.chromium.org/276783002/diff/1/media/cast/video_sender/vide... media/cast/video_sender/video_sender.cc:419: video_encoder_->SetBitRate(new_bitrate / current_bitrate_divider_); Same here this should be a helper method.
On 2014/05/08 23:22:33, Alpha wrote: > Before we land this change. Do we understand why it is stuck? Is it because that > bandwidth is throttled or the pacer is not allowing more out flow? > > I'm also a bit worried about the impact on quality, have we tested this and > shows good quality under normal network condition? > > https://codereview.chromium.org/276783002/diff/1/media/cast/video_sender/vide... > File media/cast/video_sender/video_sender.cc (right): In my (limited) testing, it seems to have no impact at all on the quality once it is running normally again. The reason it doesn't catch up seems to be that every time it tries to stop dropping frames, the new frame is very large (since it's been a while since it last encoded a frame) and so it immediately gets into trouble again. If I switch the network to "perfect" then it is able to catch up again. When running normally, it uses ~4.6 mbit/s. When trying to catch up, it was using double that amount. I don't think the pacer is a problem since 9Mbit/s is still a lot less than 24Mbit/s.
On 2014/05/08 23:31:26, hubbe wrote: > On 2014/05/08 23:22:33, Alpha wrote: > > Before we land this change. Do we understand why it is stuck? Is it because > that > > bandwidth is throttled or the pacer is not allowing more out flow? > > > > I'm also a bit worried about the impact on quality, have we tested this and > > shows good quality under normal network condition? > > > > > https://codereview.chromium.org/276783002/diff/1/media/cast/video_sender/vide... > > File media/cast/video_sender/video_sender.cc (right): > > In my (limited) testing, it seems to have no impact at all on the quality once > it is running normally again. The reason it doesn't catch up seems to be that > every time it tries to stop dropping frames, the new frame is very large (since > it's been a while since it last encoded a frame) and so it immediately gets into > trouble again. If I switch the network to "perfect" then it is able to catch up > again. > > When running normally, it uses ~4.6 mbit/s. When trying to catch up, it was > using double that amount. I don't think the pacer is a problem since 9Mbit/s is > still a lot less than 24Mbit/s. I see. While I like this change, I'm hopping that we can have a test for these kind of stuff with the fake video encoder with a simulated network profile. Is it possible?
https://codereview.chromium.org/276783002/diff/1/media/cast/video_sender/vide... File media/cast/video_sender/video_sender.cc (right): https://codereview.chromium.org/276783002/diff/1/media/cast/video_sender/vide... media/cast/video_sender/video_sender.cc:189: current_requested_bitrate_ / current_bitrate_divider_); On 2014/05/08 23:22:34, Alpha wrote: > Can we have a helper method to compute this instead of duplicating the divide? Done. https://codereview.chromium.org/276783002/diff/1/media/cast/video_sender/vide... media/cast/video_sender/video_sender.cc:392: video_encoder_->SetBitRate(new_bitrate / current_bitrate_divider_); On 2014/05/08 23:22:34, Alpha wrote: > This should really be a helper method. Done. https://codereview.chromium.org/276783002/diff/1/media/cast/video_sender/vide... media/cast/video_sender/video_sender.cc:419: video_encoder_->SetBitRate(new_bitrate / current_bitrate_divider_); On 2014/05/08 23:22:34, Alpha wrote: > Same here this should be a helper method. Done.
On 2014/05/08 23:42:38, Alpha wrote: > On 2014/05/08 23:31:26, hubbe wrote: > > On 2014/05/08 23:22:33, Alpha wrote: > > > Before we land this change. Do we understand why it is stuck? Is it because > > that > > > bandwidth is throttled or the pacer is not allowing more out flow? > > > > > > I'm also a bit worried about the impact on quality, have we tested this and > > > shows good quality under normal network condition? > > > > > > > > > https://codereview.chromium.org/276783002/diff/1/media/cast/video_sender/vide... > > > File media/cast/video_sender/video_sender.cc (right): > > > > In my (limited) testing, it seems to have no impact at all on the quality once > > it is running normally again. The reason it doesn't catch up seems to be that > > every time it tries to stop dropping frames, the new frame is very large > (since > > it's been a while since it last encoded a frame) and so it immediately gets > into > > trouble again. If I switch the network to "perfect" then it is able to catch > up > > again. > > > > When running normally, it uses ~4.6 mbit/s. When trying to catch up, it was > > using double that amount. I don't think the pacer is a problem since 9Mbit/s > is > > still a lot less than 24Mbit/s. > > I see. While I like this change, I'm hopping that we can have a test for these > kind of stuff with the fake video encoder with a simulated network profile. Is > it possible? It is possible, and we should definitely have it. However, it is not possible before the branch point.
Okay. Let's have the test after the branch point then. https://codereview.chromium.org/276783002/diff/20001/media/cast/video_sender/... File media/cast/video_sender/video_sender.cc (right): https://codereview.chromium.org/276783002/diff/20001/media/cast/video_sender/... media/cast/video_sender/video_sender.cc:492: new_bitrate /= current_bitrate_divider_; Please have a check here such that we don't go too low for the bitrate.
https://codereview.chromium.org/276783002/diff/20001/media/cast/video_sender/... File media/cast/video_sender/video_sender.cc (right): https://codereview.chromium.org/276783002/diff/20001/media/cast/video_sender/... media/cast/video_sender/video_sender.cc:492: new_bitrate /= current_bitrate_divider_; On 2014/05/08 23:48:30, Alpha wrote: > Please have a check here such that we don't go too low for the bitrate. What is "too low" ?
https://codereview.chromium.org/276783002/diff/20001/media/cast/video_sender/... File media/cast/video_sender/video_sender.cc (right): https://codereview.chromium.org/276783002/diff/20001/media/cast/video_sender/... media/cast/video_sender/video_sender.cc:492: new_bitrate /= current_bitrate_divider_; On 2014/05/08 23:48:30, Alpha wrote: > Please have a check here such that we don't go too low for the bitrate. Done.
On 2014/05/08 23:58:17, hubbe wrote: > https://codereview.chromium.org/276783002/diff/20001/media/cast/video_sender/... > File media/cast/video_sender/video_sender.cc (right): > > https://codereview.chromium.org/276783002/diff/20001/media/cast/video_sender/... > media/cast/video_sender/video_sender.cc:492: new_bitrate /= > current_bitrate_divider_; > On 2014/05/08 23:48:30, Alpha wrote: > > Please have a check here such that we don't go too low for the bitrate. > > Done. LGTM.
The CQ bit was checked by hclam@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hubbe@chromium.org/276783002/40001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
https://codereview.chromium.org/276783002/diff/40001/media/cast/video_sender/... File media/cast/video_sender/video_sender.cc (right): https://codereview.chromium.org/276783002/diff/40001/media/cast/video_sender/... media/cast/video_sender/video_sender.cc:470: } else if (frames_in_flight > max_unacked_frames_ * 4 / 5) { Looks like this gets a compiler warning turned into error. If |frames_in_flight| is int then this problem will go away.
https://codereview.chromium.org/276783002/diff/40001/media/cast/video_sender/... File media/cast/video_sender/video_sender.cc (right): https://codereview.chromium.org/276783002/diff/40001/media/cast/video_sender/... media/cast/video_sender/video_sender.cc:470: } else if (frames_in_flight > max_unacked_frames_ * 4 / 5) { On 2014/05/09 05:19:03, Alpha wrote: > Looks like this gets a compiler warning turned into error. If |frames_in_flight| > is int then this problem will go away. away away away away
The CQ bit was checked by hubbe@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hubbe@chromium.org/276783002/60001
Message was sent while issue was closed.
Change committed as 269475
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/296513008/ by hubbe@chromium.org. The reason for reverting is: It seems this causes problems with video quality. Reverting this until more testing has been done. . |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
