Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2630333002:
Drop frames until specified bitrate is achieved. (Closed)
|
Created:
3 years, 11 months ago by kthelgason Modified:
3 years, 10 months ago CC:
webrtc-reviews_webrtc.org, video-team_agora.io, yujie_mao (webrtc), zhengzhonghou_agora.io, stefan-webrtc, tterriberry_mozilla.com, the sun, mflodman, magjed_webrtc Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionDrop frames until specified bitrate is achieved.
This CL fixes a regression introduced with the new quality scaler
where the video would no longer start in a scaled mode. This CL adds
code that compares incoming captured frames to the target bitrate,
and if they are found to be too large, they are dropped and sinkWants
set to a lower resolution. The number of dropped frames should be low
(0-4 in most cases) and should not introduce a noticeable delay, or
at least should be preferrable to having the first 2-4 seconds of video
have very low quality.
BUG=webrtc:6953
Review-Url: https://codereview.webrtc.org/2630333002
Cr-Commit-Position: refs/heads/master@{#16391}
Committed: https://chromium.googlesource.com/external/webrtc/+/83399caec5762d2dad038b8e9d86163e92c18c9f
Patch Set 1 #Patch Set 2 : keep initial start bitrate at 0 #
Total comments: 5
Patch Set 3 : Initialize |initial_rampup_| in ctor #
Total comments: 14
Patch Set 4 : Add more thorough tests and limit max framedrops. #Patch Set 5 : cleanup #Patch Set 6 : unsign things #Patch Set 7 : rebase #
Total comments: 19
Patch Set 8 : code review #
Total comments: 9
Patch Set 9 : Only drop frames at beginning of call #
Total comments: 14
Patch Set 10 : configure quality scaler also on SetSource #Patch Set 11 : Disable scaling in unit tests #Patch Set 12 #Patch Set 13 : Turn off quality scaling for screensharing in tests #
Messages
Total messages: 67 (45 generated)
The CQ bit was checked by kthelgason@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was checked by kthelgason@webrtc.org
The CQ bit was unchecked by kthelgason@webrtc.org
The CQ bit was checked by kthelgason@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
kthelgason@webrtc.org changed reviewers: + perkj@webrtc.org, sprang@webrtc.org
PTAL.
https://codereview.webrtc.org/2630333002/diff/20001/webrtc/video/vie_encoder.h File webrtc/video/vie_encoder.h (right): https://codereview.webrtc.org/2630333002/diff/20001/webrtc/video/vie_encoder.... webrtc/video/vie_encoder.h:120: bool initial_rampup_ = false; Please initialize in ctor instead. https://codereview.webrtc.org/2630333002/diff/20001/webrtc/video/vie_encoder_... File webrtc/video/vie_encoder_unittest.cc (right): https://codereview.webrtc.org/2630333002/diff/20001/webrtc/video/vie_encoder_... webrtc/video/vie_encoder_unittest.cc:989: ev.Wait(10); What exactly does this sleep accomplish, and why 10ms?
https://codereview.webrtc.org/2630333002/diff/20001/webrtc/video/vie_encoder.h File webrtc/video/vie_encoder.h (right): https://codereview.webrtc.org/2630333002/diff/20001/webrtc/video/vie_encoder.... webrtc/video/vie_encoder.h:120: bool initial_rampup_ = false; On 2017/01/16 13:45:23, språng wrote: > Please initialize in ctor instead. Done. https://codereview.webrtc.org/2630333002/diff/20001/webrtc/video/vie_encoder_... File webrtc/video/vie_encoder_unittest.cc (right): https://codereview.webrtc.org/2630333002/diff/20001/webrtc/video/vie_encoder_... webrtc/video/vie_encoder_unittest.cc:989: ev.Wait(10); On 2017/01/16 13:45:23, språng wrote: > What exactly does this sleep accomplish, and why 10ms? The idea is to simulate a delay between frames, as the propagation of sinkWants to the capturer is not instantaneous. 10ms chosen arbitrarily, purely because 10 is a nice number.
lgtm, given that you verify that this won't cause any bot flakiness https://codereview.webrtc.org/2630333002/diff/20001/webrtc/video/vie_encoder_... File webrtc/video/vie_encoder_unittest.cc (right): https://codereview.webrtc.org/2630333002/diff/20001/webrtc/video/vie_encoder_... webrtc/video/vie_encoder_unittest.cc:989: ev.Wait(10); On 2017/01/16 14:08:45, kthelgason wrote: > On 2017/01/16 13:45:23, språng wrote: > > What exactly does this sleep accomplish, and why 10ms? > > The idea is to simulate a delay between frames, as the propagation of sinkWants > to the capturer is not instantaneous. 10ms chosen arbitrarily, purely because 10 > is a nice number. So does this risk being racy? I like 1000/30 as sleep time :P https://codereview.webrtc.org/2630333002/diff/40001/webrtc/video/vie_encoder_... File webrtc/video/vie_encoder_unittest.cc (right): https://codereview.webrtc.org/2630333002/diff/40001/webrtc/video/vie_encoder_... webrtc/video/vie_encoder_unittest.cc:992: dropped_frames++; nit: {} for multi-line if
https://codereview.webrtc.org/2630333002/diff/40001/webrtc/api/video/video_fr... File webrtc/api/video/video_frame.h (right): https://codereview.webrtc.org/2630333002/diff/40001/webrtc/api/video/video_fr... webrtc/api/video/video_frame.h:53: // Get frame size in pixels. nit: empty line https://codereview.webrtc.org/2630333002/diff/40001/webrtc/video/vie_encoder.cc File webrtc/video/vie_encoder.cc (right): https://codereview.webrtc.org/2630333002/diff/40001/webrtc/video/vie_encoder.... webrtc/video/vie_encoder.cc:58: int MaximumFrameSizeForBitrate(int kbps) { uint32_t encoder_start_bitrate_bps_ MaximumFrameSizeForBitrate can never get a negative bitrate and thus does not have to check for that. Param should also be a uint. https://codereview.webrtc.org/2630333002/diff/40001/webrtc/video/vie_encoder.... webrtc/video/vie_encoder.cc:577: int expected_pixel_count = nit: please remove this unnecessary temp. https://codereview.webrtc.org/2630333002/diff/40001/webrtc/video/vie_encoder.... webrtc/video/vie_encoder.cc:578: MaximumFrameSizeForBitrate(encoder_start_bitrate_bps_ / 1000); nit: Why not use unit bps here too since that is what we already have. https://codereview.webrtc.org/2630333002/diff/40001/webrtc/video/vie_encoder.... webrtc/video/vie_encoder.cc:579: if (initial_rampup_ && video_frame.size() > expected_pixel_count) { instead of initial_rampup being a bool- can we let it be a counter and drop the first say 3 frames after a reconfiguration if the configured bitrate is too low ? Otherwise, what happens if the source can not scale down further? Then we will never encode these frames and drop all. something like if( video_frame.size() > MaximumFrameSizeForBitrate(encoder_start_bitrate_bps_) && frames_dropped_due_to_bitrate_ > ....) { https://codereview.webrtc.org/2630333002/diff/40001/webrtc/video/vie_encoder_... File webrtc/video/vie_encoder_unittest.cc (right): https://codereview.webrtc.org/2630333002/diff/40001/webrtc/video/vie_encoder_... webrtc/video/vie_encoder_unittest.cc:996: sink_.WaitForEncodedFrame(10); So you only expect one frame to be encoded. Can you write this a bit more readable? Skip the for- you only need two frames and check the expectations for each frame you insert. If you do as I suggest and change so that a frame is encoded even if it is too large you should also test that (maybe in a separate test since multiple simple tests are prefered).
https://codereview.webrtc.org/2630333002/diff/40001/webrtc/video/vie_encoder.cc File webrtc/video/vie_encoder.cc (right): https://codereview.webrtc.org/2630333002/diff/40001/webrtc/video/vie_encoder.... webrtc/video/vie_encoder.cc:577: int expected_pixel_count = oh, we should probably do this before Reconfiguring the encoder too since will drop the frame and wait for a new resolution anyway.
https://codereview.webrtc.org/2630333002/diff/40001/webrtc/api/video/video_fr... File webrtc/api/video/video_frame.h (right): https://codereview.webrtc.org/2630333002/diff/40001/webrtc/api/video/video_fr... webrtc/api/video/video_frame.h:53: // Get frame size in pixels. On 2017/01/17 08:53:10, perkj_webrtc wrote: > nit: empty line Acknowledged. https://codereview.webrtc.org/2630333002/diff/40001/webrtc/video/vie_encoder.cc File webrtc/video/vie_encoder.cc (right): https://codereview.webrtc.org/2630333002/diff/40001/webrtc/video/vie_encoder.... webrtc/video/vie_encoder.cc:58: int MaximumFrameSizeForBitrate(int kbps) { On 2017/01/17 08:53:10, perkj_webrtc wrote: > uint32_t encoder_start_bitrate_bps_ > > MaximumFrameSizeForBitrate can never get a negative bitrate and thus does not > have to check for that. Param should also be a uint. Done. https://codereview.webrtc.org/2630333002/diff/40001/webrtc/video/vie_encoder.... webrtc/video/vie_encoder.cc:577: int expected_pixel_count = On 2017/01/17 10:30:02, perkj_webrtc wrote: > oh, we should probably do this before Reconfiguring the encoder too since will > drop the frame and wait for a new resolution anyway. Done. https://codereview.webrtc.org/2630333002/diff/40001/webrtc/video/vie_encoder.... webrtc/video/vie_encoder.cc:578: MaximumFrameSizeForBitrate(encoder_start_bitrate_bps_ / 1000); On 2017/01/17 08:53:10, perkj_webrtc wrote: > nit: Why not use unit bps here too since that is what we already have. I started out implementing it that way, but changed it to kbps for readability. It's really hard (at least for me) to quickly tell the difference between 100000 and 1000000. https://codereview.webrtc.org/2630333002/diff/40001/webrtc/video/vie_encoder.... webrtc/video/vie_encoder.cc:579: if (initial_rampup_ && video_frame.size() > expected_pixel_count) { On 2017/01/17 08:53:10, perkj_webrtc wrote: > instead of initial_rampup being a bool- can we let it be a counter and drop the > first say 3 frames after a reconfiguration if the configured bitrate is too low > ? > Otherwise, what happens if the source can not scale down further? Then we will > never encode these frames and drop all. > > something like > if( video_frame.size() > MaximumFrameSizeForBitrate(encoder_start_bitrate_bps_) > && frames_dropped_due_to_bitrate_ > ....) { That's a very good point, thanks. Done! https://codereview.webrtc.org/2630333002/diff/40001/webrtc/video/vie_encoder_... File webrtc/video/vie_encoder_unittest.cc (right): https://codereview.webrtc.org/2630333002/diff/40001/webrtc/video/vie_encoder_... webrtc/video/vie_encoder_unittest.cc:996: sink_.WaitForEncodedFrame(10); On 2017/01/17 08:53:10, perkj_webrtc wrote: > So you only expect one frame to be encoded. > Can you write this a bit more readable? Skip the for- you only need two frames > and check the expectations for each frame you insert. > > If you do as I suggest and change so that a frame is encoded even if it is too > large you should also test that (maybe in a separate test since multiple simple > tests are prefered). Good comments, thanks. I added some more thorough tests.
The CQ bit was checked by kthelgason@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...) ios64_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios9_dbg/buil...) ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/16569) ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/20557)
The CQ bit was checked by kthelgason@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
https://codereview.webrtc.org/2630333002/diff/120001/webrtc/modules/video_cod... File webrtc/modules/video_coding/utility/default_video_bitrate_allocator.cc (left): https://codereview.webrtc.org/2630333002/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/utility/default_video_bitrate_allocator.cc:38: This snuck in by accident, sorry about that.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Tests looks much better. I wonder if you should talk to stefan about this as well. This will not only change the behaviour in the beginning but every time the bwe drops below your thresholds, scaling will be requested. That is a behaviour change and you might want to test that more and talk to Stefan and flodman before we dare to land this. https://codereview.webrtc.org/2630333002/diff/120001/webrtc/video/vie_encoder.cc File webrtc/video/vie_encoder.cc (right): https://codereview.webrtc.org/2630333002/diff/120001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.cc:45: // The maximum number of frames to drop at beginning of stream Isnt this comment a bit confusing? This will happen as soon as the resolution change and the bitrate is lower than what we think is good. https://codereview.webrtc.org/2630333002/diff/120001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.cc:583: uint32_t expected_pixel_count = min_pixel_count? https://codereview.webrtc.org/2630333002/diff/120001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.cc:585: if (initial_rampup_ < kMaxInitialFramedrop && please name more appropriate. Maybe something like frames_dropped_due_to_high_resolution_ ? https://codereview.webrtc.org/2630333002/diff/120001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.cc:585: if (initial_rampup_ < kMaxInitialFramedrop && Should kMaxInitilFrameDrop also change change name since frames are dropped at anytime the bitrate drops below the threshold? https://codereview.webrtc.org/2630333002/diff/120001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.cc:585: if (initial_rampup_ < kMaxInitialFramedrop && This will drop frames at anytime time kMaxInitialFrameDrop times if the bitrate estimates suddenly drops and a new lower resolution is requested. Is that ok with sprang and stefan? Does that look good? Will the quality scaler request a higher resolution later, even if the resolution was lowered due to this? Can you add a test for this case as well please. https://codereview.webrtc.org/2630333002/diff/120001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.cc:717: encoder_start_bitrate_bps_ = this is where encoder_start_bitrate_bps_ is set. it only exist since we might reconfigure the encoder even though its paused. It might be better and make more sense to use last_observed_bitrate_bps_ for this. https://codereview.webrtc.org/2630333002/diff/120001/webrtc/video/vie_encoder.h File webrtc/video/vie_encoder.h (right): https://codereview.webrtc.org/2630333002/diff/120001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.h:178: int initial_rampup_; Add comment what this means. https://codereview.webrtc.org/2630333002/diff/120001/webrtc/video/vie_encoder... File webrtc/video/vie_encoder_unittest.cc (right): https://codereview.webrtc.org/2630333002/diff/120001/webrtc/video/vie_encoder... webrtc/video/vie_encoder_unittest.cc:1061: TEST_F(ViEEncoderTest, NrOfInitiallyDroppedFramesLimited) { NrOfDroppedFramesLimited? https://codereview.webrtc.org/2630333002/diff/120001/webrtc/video/vie_encoder... webrtc/video/vie_encoder_unittest.cc:1074: // The n+1th frame should not, even though it's size is too large. /s not be dropped, even .... https://codereview.webrtc.org/2630333002/diff/120001/webrtc/video/vie_encoder... webrtc/video/vie_encoder_unittest.cc:1086: TEST_F(ViEEncoderTest, InitialFramedropOffWithMaintainResolutionPreference) { FrameDropOffWithMain.. ?
Patchset #9 (id:160001) has been deleted
Thanks for the review perkj. Can you take another look? I changed it to work the way we had initially discussed, so that we will only ever drop frames at the beginning of a call. https://codereview.webrtc.org/2630333002/diff/120001/webrtc/video/vie_encoder.cc File webrtc/video/vie_encoder.cc (right): https://codereview.webrtc.org/2630333002/diff/120001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.cc:45: // The maximum number of frames to drop at beginning of stream On 2017/01/23 08:54:22, perkj_webrtc wrote: > Isnt this comment a bit confusing? This will happen as soon as the resolution > change and the bitrate is lower than what we think is good. I'm not sure I understand what you mean. This should only happen in the beginning when an encoder is configured. https://codereview.webrtc.org/2630333002/diff/120001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.cc:583: uint32_t expected_pixel_count = On 2017/01/23 08:54:22, perkj_webrtc wrote: > min_pixel_count? Shouldn't it rather be max_pixel_count? It's the maximum amount of pixels we allow for this bitrate. https://codereview.webrtc.org/2630333002/diff/120001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.cc:585: if (initial_rampup_ < kMaxInitialFramedrop && On 2017/01/23 08:54:22, perkj_webrtc wrote: > Should kMaxInitilFrameDrop also change change name since frames are dropped at > anytime the bitrate drops below the threshold? I understand what you mean. That should not be how this works. If a frame ever passes this condition |initial_rampup_| should be set to kMaxInitialFrameDrop so that we never drop frames once one has been sent. https://codereview.webrtc.org/2630333002/diff/120001/webrtc/video/vie_encoder.h File webrtc/video/vie_encoder.h (right): https://codereview.webrtc.org/2630333002/diff/120001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.h:178: int initial_rampup_; On 2017/01/23 08:54:22, perkj_webrtc wrote: > Add comment what this means. Done. https://codereview.webrtc.org/2630333002/diff/120001/webrtc/video/vie_encoder... File webrtc/video/vie_encoder_unittest.cc (right): https://codereview.webrtc.org/2630333002/diff/120001/webrtc/video/vie_encoder... webrtc/video/vie_encoder_unittest.cc:1061: TEST_F(ViEEncoderTest, NrOfInitiallyDroppedFramesLimited) { On 2017/01/23 08:54:22, perkj_webrtc wrote: > NrOfDroppedFramesLimited? Done. https://codereview.webrtc.org/2630333002/diff/120001/webrtc/video/vie_encoder... webrtc/video/vie_encoder_unittest.cc:1074: // The n+1th frame should not, even though it's size is too large. On 2017/01/23 08:54:22, perkj_webrtc wrote: > /s not be dropped, even .... Done. https://codereview.webrtc.org/2630333002/diff/120001/webrtc/video/vie_encoder... webrtc/video/vie_encoder_unittest.cc:1086: TEST_F(ViEEncoderTest, InitialFramedropOffWithMaintainResolutionPreference) { On 2017/01/23 08:54:22, perkj_webrtc wrote: > FrameDropOffWithMain.. ? Done.
The CQ bit was checked by kthelgason@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
I think you should add a test for the behavior you wanted that frames are not dropped after a frame has been encoded. Except for that I think this is fine. https://codereview.webrtc.org/2630333002/diff/120001/webrtc/video/vie_encoder.cc File webrtc/video/vie_encoder.cc (right): https://codereview.webrtc.org/2630333002/diff/120001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.cc:45: // The maximum number of frames to drop at beginning of stream On 2017/01/23 12:39:06, kthelgason wrote: > On 2017/01/23 08:54:22, perkj_webrtc wrote: > > Isnt this comment a bit confusing? This will happen as soon as the resolution > > change and the bitrate is lower than what we think is good. > > I'm not sure I understand what you mean. This should only happen in the > beginning when an encoder is configured. but an encoder is reconfigured every time the resolution change. ie. after the cpu adapter or quality scaler kicks in. Just to double check, Will the quality scaler request a scale up if the initial bitrate is low so that ViEEncoder has requested a scale down. https://codereview.webrtc.org/2630333002/diff/140001/webrtc/video/vie_encoder.cc File webrtc/video/vie_encoder.cc (right): https://codereview.webrtc.org/2630333002/diff/140001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.cc:45: // The maximum number of frames to drop at beginning of stream same here. It happens every time the resolution change. https://codereview.webrtc.org/2630333002/diff/140001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.cc:63: if (kbps > 0) { kbps can never be 0 right? RTC_DCHECK(kbps >0) instead of if. https://codereview.webrtc.org/2630333002/diff/140001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.cc:583: uint32_t max_pixel_count = Move all this to above the check for if(pending_encoder_recon..) There is no need to reconfigure if the frame will be dropped anyway and lower resolution requested. https://codereview.webrtc.org/2630333002/diff/140001/webrtc/video/vie_encoder.h File webrtc/video/vie_encoder.h (right): https://codereview.webrtc.org/2630333002/diff/140001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.h:178: // Counts how many frames we've dropped in the initial rampup phase. This still happens every time the encoder it reconfigured. And the encoder is reconfigured every time the incoming frame size change. Is initil_rampup_ still a good name? /s since the last time the frame size changed. https://codereview.webrtc.org/2630333002/diff/140001/webrtc/video/vie_encoder... File webrtc/video/vie_encoder_unittest.cc (right): https://codereview.webrtc.org/2630333002/diff/140001/webrtc/video/vie_encoder... webrtc/video/vie_encoder_unittest.cc:1102: } Add test for not dropping frames if bitrate drops after a frame has been encoded.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The main change in this patchset is that we don't reset |initial_rampup_| on encoder reconfiguration. https://codereview.webrtc.org/2630333002/diff/140001/webrtc/video/vie_encoder.cc File webrtc/video/vie_encoder.cc (right): https://codereview.webrtc.org/2630333002/diff/140001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.cc:45: // The maximum number of frames to drop at beginning of stream On 2017/01/24 09:06:16, perkj_webrtc wrote: > same here. It happens every time the resolution change. Acknowledged. https://codereview.webrtc.org/2630333002/diff/140001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.cc:63: if (kbps > 0) { On 2017/01/24 09:06:16, perkj_webrtc wrote: > kbps can never be 0 right? > RTC_DCHECK(kbps >0) instead of if. It can actually be zero. |encoder_start_bitrate_| is initialized to 0 and it seems that it's used to signal no bandwidth constraint or something. At least it happens in practice, and the old code that used to handle this in the quality scaler included this guard. https://codereview.webrtc.org/2630333002/diff/140001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.cc:583: uint32_t max_pixel_count = On 2017/01/24 09:06:16, perkj_webrtc wrote: > Move all this to above the check for if(pending_encoder_recon..) > > There is no need to reconfigure if the frame will be dropped anyway and lower > resolution requested. Done. https://codereview.webrtc.org/2630333002/diff/140001/webrtc/video/vie_encoder.h File webrtc/video/vie_encoder.h (right): https://codereview.webrtc.org/2630333002/diff/140001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.h:178: // Counts how many frames we've dropped in the initial rampup phase. On 2017/01/24 09:06:16, perkj_webrtc wrote: > This still happens every time the encoder it reconfigured. And the encoder is > reconfigured every time the incoming frame size change. > Is initil_rampup_ still a good name? > > /s since the last time the frame size changed. You're right, but we can't have that behaviour. In that case we would get completely stuck at the limited resolution, and not be able to scale above it when the quality is good. We need to find another place to reset this variable.
The CQ bit was checked by kthelgason@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_ubsan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan/builds/8910)
https://codereview.webrtc.org/2630333002/diff/180001/webrtc/video/vie_encoder.cc File webrtc/video/vie_encoder.cc (right): https://codereview.webrtc.org/2630333002/diff/180001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.cc:358: scaling_enabled_ = (degradation_preference != Changing the source change scaling_enabled_ but does not reconfigure the encoder unless the frame size also change. https://codereview.webrtc.org/2630333002/diff/180001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.cc:460: quality_scaler_.reset( quality scaler is only created when the codec is reconfigured. it can be due to frame size changes or that the the public method ViEEncoder::ConfigureEncoder is called. https://codereview.webrtc.org/2630333002/diff/180001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.cc:466: initial_rampup_ = kMaxInitialFramedrop; This will not necessarily be called when scaling_enabled_ change. Ie, if the source change. You should maybe not change this here either then. https://codereview.webrtc.org/2630333002/diff/180001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.cc:568: if (initial_rampup_ < kMaxInitialFramedrop && I guess you will also need to check if (scaling_enabled_ && quality_scaling_ ) to handle this, otherwise if !scaling_settings.enabled but scaling_enabled_ you will never request a higher resolution again. This gets a bit too complicated. Any chance we can start over a bit and clean up the states? https://codereview.webrtc.org/2630333002/diff/180001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.cc:569: video_frame.size() > max_pixel_count) { nit: prefer video_frame_size() > MaximumFrameSizeForBitrate(encoder_start_bitrate_bps_ / 1000); https://codereview.webrtc.org/2630333002/diff/180001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.cc:577: nit: remove extra empty line. https://codereview.webrtc.org/2630333002/diff/180001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.cc:734: if (!scaling_enabled_) so if the source change scaling_enabled_ can change but quality scaler may or may not exist regardless.
The CQ bit was checked by kthelgason@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_ubsan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan/builds/8919)
I tried moving the initialization of the quality scaler in to ViEEncoder::SetSource but that won't work as VideoEncoder::GetScalingSettings depends on the video encoder being initialized, which doesn't happen until ReconfigureEncoder() is called. https://codereview.webrtc.org/2630333002/diff/180001/webrtc/video/vie_encoder.cc File webrtc/video/vie_encoder.cc (right): https://codereview.webrtc.org/2630333002/diff/180001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.cc:358: scaling_enabled_ = (degradation_preference != On 2017/01/24 12:58:23, perkj_webrtc wrote: > Changing the source change scaling_enabled_ but does not reconfigure the encoder > unless the frame size also change. Acknowledged. https://codereview.webrtc.org/2630333002/diff/180001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.cc:460: quality_scaler_.reset( On 2017/01/24 12:58:22, perkj_webrtc wrote: > quality scaler is only created when the codec is reconfigured. it can be due to > frame size changes or that the the public method ViEEncoder::ConfigureEncoder is > called. Acknowledged. https://codereview.webrtc.org/2630333002/diff/180001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.cc:466: initial_rampup_ = kMaxInitialFramedrop; On 2017/01/24 12:58:23, perkj_webrtc wrote: > This will not necessarily be called when scaling_enabled_ change. Ie, if the > source change. > You should maybe not change this here either then. Acknowledged. https://codereview.webrtc.org/2630333002/diff/180001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.cc:568: if (initial_rampup_ < kMaxInitialFramedrop && On 2017/01/24 12:58:22, perkj_webrtc wrote: > I guess you will also need to check if (scaling_enabled_ && quality_scaling_ ) > to handle this, otherwise > if !scaling_settings.enabled but scaling_enabled_ you will never request a > higher resolution again. This gets a bit too complicated. > > Any chance we can start over a bit and clean up the states? Yes, this sucks. https://codereview.webrtc.org/2630333002/diff/180001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.cc:569: video_frame.size() > max_pixel_count) { On 2017/01/24 12:58:23, perkj_webrtc wrote: > nit: prefer video_frame_size() > > MaximumFrameSizeForBitrate(encoder_start_bitrate_bps_ / 1000); Done. https://codereview.webrtc.org/2630333002/diff/180001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.cc:577: On 2017/01/24 12:58:23, perkj_webrtc wrote: > nit: remove extra empty line. Done. https://codereview.webrtc.org/2630333002/diff/180001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.cc:734: if (!scaling_enabled_) On 2017/01/24 12:58:23, perkj_webrtc wrote: > so if the source change scaling_enabled_ can change but quality scaler may or > may not exist regardless. This has now been changed such that the quality scaler is reinitialized both when re =configuring the encoder and when changing the source.
I tried moving the initialization of the quality scaler in to ViEEncoder::SetSource but that won't work as VideoEncoder::GetScalingSettings depends on the video encoder being initialized, which doesn't happen until ReconfigureEncoder() is called.
The CQ bit was checked by kthelgason@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/22062)
perkj@webrtc.org changed reviewers: + stefan@webrtc.org
looks good to me but you need to fix the failing tests. Note that you till restart the "initial_rampup" every time you switch source which I think is fine. Looks like you have a few tests to fix https://build.chromium.org/p/tryserver.webrtc/builders/win_x64_dbg/builds/559... The initial bitrate is too low I guess. Stefan was interested in reviewing this as well. And as magjed pointed out, we can just set the initial sink wants if it wasn't for the stats.
The CQ bit was checked by kthelgason@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_rel/builds/22384)
The CQ bit was checked by kthelgason@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Please take a look. I have not moved anything into the quality scaler, as when I started doing so it felt completely unrelated to other logic that's there already, and so I didn't feel like it was a benefit. If anyone feels strongly that this belongs there however I can move it.
Ping stefan, perkj
lgtm
The CQ bit was checked by kthelgason@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from sprang@webrtc.org Link to the patchset: https://codereview.webrtc.org/2630333002/#ps240001 (title: "")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 240001, "attempt_start_ts": 1485939058109960,
"parent_rev": "fdd9b85652cd91798559c62cb867ac1a93bca479", "commit_rev":
"83399caec5762d2dad038b8e9d86163e92c18c9f"}
Message was sent while issue was closed.
Description was changed from ========== Drop frames until specified bitrate is achieved. This CL fixes a regression introduced with the new quality scaler where the video would no longer start in a scaled mode. This CL adds code that compares incoming captured frames to the target bitrate, and if they are found to be too large, they are dropped and sinkWants set to a lower resolution. The number of dropped frames should be low (0-4 in most cases) and should not introduce a noticeable delay, or at least should be preferrable to having the first 2-4 seconds of video have very low quality. BUG=webrtc:6953 ========== to ========== Drop frames until specified bitrate is achieved. This CL fixes a regression introduced with the new quality scaler where the video would no longer start in a scaled mode. This CL adds code that compares incoming captured frames to the target bitrate, and if they are found to be too large, they are dropped and sinkWants set to a lower resolution. The number of dropped frames should be low (0-4 in most cases) and should not introduce a noticeable delay, or at least should be preferrable to having the first 2-4 seconds of video have very low quality. BUG=webrtc:6953 Review-Url: https://codereview.webrtc.org/2630333002 Cr-Commit-Position: refs/heads/master@{#16391} Committed: https://chromium.googlesource.com/external/webrtc/+/83399caec5762d2dad038b8e9... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:240001) as https://chromium.googlesource.com/external/webrtc/+/83399caec5762d2dad038b8e9...
Message was sent while issue was closed.
A revert of this CL (patchset #12 id:240001) has been created in https://codereview.webrtc.org/2666303002/ by minyue@webrtc.org. The reason for reverting is: due to failures on perf tests (not on perf stats, but fails running due to dcheck failures), see e.g., https://build.chromium.org/p/client.webrtc.perf/builders/Android32%20Tests%20.... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
