Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2517243005:
Remove overhead from video bitrate. (Closed)
DescriptionRemove overhead from video bitrate.
BUG=webrtc:6638
Committed: https://crrev.com/a33287761af733d41d8c0a082f3d2c2cefaa9a68
Cr-Commit-Position: refs/heads/master@{#15303}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Add Test and field trial. #
Total comments: 17
Patch Set 3 : Respond to comments. #
Total comments: 6
Patch Set 4 : Respond to comments. #
Total comments: 4
Patch Set 5 : Respond to comment. #Patch Set 6 : Respond to comment. #Patch Set 7 : Fix implicite casting problems. #Patch Set 8 : Fix for unittests. #Patch Set 9 : Rebased #
Total comments: 1
Patch Set 10 : Add cmath include. #
Messages
Total messages: 75 (41 generated)
Here the Cl, to subtract the overhead from the video bitrate.
Description was changed from ========== Remove overhead from video bitrate. BUG=webrtc:6638 ========== to ========== Remove overhead from video bitrate. BUG=webrtc:6638 ==========
michaelt@webrtc.org changed reviewers: + minyue@webrtc.org, stefan@webrtc.org
https://codereview.webrtc.org/2517243005/diff/1/webrtc/video/video_send_strea... File webrtc/video/video_send_stream.cc (left): https://codereview.webrtc.org/2517243005/diff/1/webrtc/video/video_send_strea... webrtc/video/video_send_stream.cc:779: RTC_DCHECK_LE(config_->rtp.max_packet_size, 0xFFFFu + kRtpPacketSizeOverhead); Is there still a reasonable check we should make on the max_packet_size? https://codereview.webrtc.org/2517243005/diff/1/webrtc/video/video_send_strea... File webrtc/video/video_send_stream.cc (right): https://codereview.webrtc.org/2517243005/diff/1/webrtc/video/video_send_strea... webrtc/video/video_send_stream.cc:1229: (config_->rtp.max_packet_size + transport_overhead_per_packet_)); This is an approximation assuming only max sized packets, right? I think it would be good to improve it a bit since the overhead is a bit larger for low bitrates (say 50 kbps)... Maybe do something like this in OnBitrateUpdated(): framerate = stats_proxy_->GetSendFrameRate(); bits_per_frame = bitrate_bps / framerate packets_per_second = ceil(bits_per_frame / (8 * config_->rtp.max_packet_size)) * framerate overhead_per_second = 8 * overhead_bytes_per_packet * packets_per_second I probably made some mistake, so please double check :)
https://codereview.webrtc.org/2517243005/diff/1/webrtc/video/video_send_strea... File webrtc/video/video_send_stream.cc (left): https://codereview.webrtc.org/2517243005/diff/1/webrtc/video/video_send_strea... webrtc/video/video_send_stream.cc:779: RTC_DCHECK_LE(config_->rtp.max_packet_size, 0xFFFFu + kRtpPacketSizeOverhead); We could check for lower than IP_MTU(1500) https://codereview.webrtc.org/2517243005/diff/1/webrtc/video/video_send_strea... File webrtc/video/video_send_stream.cc (right): https://codereview.webrtc.org/2517243005/diff/1/webrtc/video/video_send_strea... webrtc/video/video_send_stream.cc:1229: (config_->rtp.max_packet_size + transport_overhead_per_packet_)); Yes I assumed only max packets. I think we can simplify you calculation a little. Framerate seams obsolete to me. I would do: packets_per_second = ceil(bitrate_bps / (8 * (config_->rtp.max_packet_size + transport_overhead_per_packet_))); overhead_per_second = 8 * overhead_bytes_per_packet * packets_per_second; The ceil should make the difference to my old approach.
https://codereview.webrtc.org/2517243005/diff/1/webrtc/video/video_send_strea... File webrtc/video/video_send_stream.cc (right): https://codereview.webrtc.org/2517243005/diff/1/webrtc/video/video_send_strea... webrtc/video/video_send_stream.cc:1229: (config_->rtp.max_packet_size + transport_overhead_per_packet_)); On 2016/11/23 09:19:13, michaelt wrote: > Yes I assumed only max packets. > I think we can simplify you calculation a little. > Framerate seams obsolete to me. > I would do: > packets_per_second = ceil(bitrate_bps / (8 * (config_->rtp.max_packet_size + > transport_overhead_per_packet_))); > overhead_per_second = 8 * overhead_bytes_per_packet * packets_per_second; > > The ceil should make the difference to my old approach. > > Yes, you are probably right, the ceil is what's important.
https://codereview.webrtc.org/2517243005/diff/1/webrtc/video/video_send_strea... File webrtc/video/video_send_stream.cc (right): https://codereview.webrtc.org/2517243005/diff/1/webrtc/video/video_send_strea... webrtc/video/video_send_stream.cc:1164: bitrate_bps *= payload_fraction_; I think we need to create a global flag to make clear transition from no-overhead era to overhead era. I think of "BANDWITDH_INCLUDE_OVERHEAD"
https://codereview.webrtc.org/2517243005/diff/1/webrtc/video/video_send_strea... File webrtc/video/video_send_stream.cc (right): https://codereview.webrtc.org/2517243005/diff/1/webrtc/video/video_send_strea... webrtc/video/video_send_stream.cc:1164: bitrate_bps *= payload_fraction_; On 2016/11/23 10:48:05, minyue-webrtc wrote: > I think we need to create a global flag to make clear transition from > no-overhead era to overhead era. > > I think of "BANDWITDH_INCLUDE_OVERHEAD" I don't think that's enough. We have to only subtract overhead if we're using send-side BWE. Call and CongestionController knows about this at least. Once we have added a check like that I think we can enable this either under a field trial, so that it's only used when audio frame length can change. This is the more cautious approach. The other option is to enable it by default as it doesn't really depend on varying audio frame lengths. This however requires that all code related to overhead changes is landed, ideally in one CL. You could land this behind a flag as suggested if you want to land it in separate CLs, but then I'd suggest using a field trial anyway so that it's easier to test it.
On 2016/11/23 11:08:12, stefan-webrtc (holmer) wrote: > https://codereview.webrtc.org/2517243005/diff/1/webrtc/video/video_send_strea... > File webrtc/video/video_send_stream.cc (right): > > https://codereview.webrtc.org/2517243005/diff/1/webrtc/video/video_send_strea... > webrtc/video/video_send_stream.cc:1164: bitrate_bps *= payload_fraction_; > On 2016/11/23 10:48:05, minyue-webrtc wrote: > > I think we need to create a global flag to make clear transition from > > no-overhead era to overhead era. > > > > I think of "BANDWITDH_INCLUDE_OVERHEAD" > > I don't think that's enough. We have to only subtract overhead if we're using > send-side BWE. Call and CongestionController knows about this at least. > > Once we have added a check like that I think we can enable this either under a > field trial, so that it's only used when audio frame length can change. This is > the more cautious approach. The other option is to enable it by default as it > doesn't really depend on varying audio frame lengths. This however requires that > all code related to overhead changes is landed, ideally in one CL. > > You could land this behind a flag as suggested if you want to land it in > separate CLs, but then I'd suggest using a field trial anyway so that it's > easier to test it. Hi Stefan, Thanks for the suggestion. If we need to test it as experiment, field trial can be very good. On the other hand, I feel that this may not need to go through the whole experiment procedure. When all code landed, unittests passed, it should work (as part of send side bwe). or are there possible pitfalls that requires us to push it out gradually? WDYT? What I suggested was mainly for development procedure. Before we hook up everything, we need to use the old code.
On 2016/11/23 11:28:52, minyue-webrtc wrote: > On 2016/11/23 11:08:12, stefan-webrtc (holmer) wrote: > > > https://codereview.webrtc.org/2517243005/diff/1/webrtc/video/video_send_strea... > > File webrtc/video/video_send_stream.cc (right): > > > > > https://codereview.webrtc.org/2517243005/diff/1/webrtc/video/video_send_strea... > > webrtc/video/video_send_stream.cc:1164: bitrate_bps *= payload_fraction_; > > On 2016/11/23 10:48:05, minyue-webrtc wrote: > > > I think we need to create a global flag to make clear transition from > > > no-overhead era to overhead era. > > > > > > I think of "BANDWITDH_INCLUDE_OVERHEAD" > > > > I don't think that's enough. We have to only subtract overhead if we're using > > send-side BWE. Call and CongestionController knows about this at least. > > > > Once we have added a check like that I think we can enable this either under a > > field trial, so that it's only used when audio frame length can change. This > is > > the more cautious approach. The other option is to enable it by default as it > > doesn't really depend on varying audio frame lengths. This however requires > that > > all code related to overhead changes is landed, ideally in one CL. > > > > You could land this behind a flag as suggested if you want to land it in > > separate CLs, but then I'd suggest using a field trial anyway so that it's > > easier to test it. > > Hi Stefan, > Thanks for the suggestion. > > If we need to test it as experiment, field trial can be very good. On the other > hand, I feel that this may not need to go through the whole experiment > procedure. When all code landed, unittests passed, it should work (as part of > send side bwe). or are there possible pitfalls that requires us to push it out > gradually? WDYT? No I don't think we should be using an experiment and roll it out. > > What I suggested was mainly for development procedure. Before we hook up > everything, we need to use the old code. That's my suggestion too, but I think it's preferred to not use #ifdefs if possible so that the still can run unittests on this code. By enabling/disabling using a field trial that's possible (can be removed later). We can also use a parameter when instantiating the class. On the other hand, maybe it's not necessary to test it... Although I would appreciate a test of some kind. Michael, do you think it's possible to write some kind of unittest for this?
On 2016/11/23 11:47:26, stefan-webrtc (holmer) wrote: > On 2016/11/23 11:28:52, minyue-webrtc wrote: > > On 2016/11/23 11:08:12, stefan-webrtc (holmer) wrote: > > > > > > https://codereview.webrtc.org/2517243005/diff/1/webrtc/video/video_send_strea... > > > File webrtc/video/video_send_stream.cc (right): > > > > > > > > > https://codereview.webrtc.org/2517243005/diff/1/webrtc/video/video_send_strea... > > > webrtc/video/video_send_stream.cc:1164: bitrate_bps *= payload_fraction_; > > > On 2016/11/23 10:48:05, minyue-webrtc wrote: > > > > I think we need to create a global flag to make clear transition from > > > > no-overhead era to overhead era. > > > > > > > > I think of "BANDWITDH_INCLUDE_OVERHEAD" > > > > > > I don't think that's enough. We have to only subtract overhead if we're > using > > > send-side BWE. Call and CongestionController knows about this at least. > > > > > > Once we have added a check like that I think we can enable this either under > a > > > field trial, so that it's only used when audio frame length can change. This > > is > > > the more cautious approach. The other option is to enable it by default as > it > > > doesn't really depend on varying audio frame lengths. This however requires > > that > > > all code related to overhead changes is landed, ideally in one CL. > > > > > > You could land this behind a flag as suggested if you want to land it in > > > separate CLs, but then I'd suggest using a field trial anyway so that it's > > > easier to test it. > > > > Hi Stefan, > > Thanks for the suggestion. > > > > If we need to test it as experiment, field trial can be very good. On the > other > > hand, I feel that this may not need to go through the whole experiment > > procedure. When all code landed, unittests passed, it should work (as part of > > send side bwe). or are there possible pitfalls that requires us to push it out > > gradually? WDYT? > > No I don't think we should be using an experiment and roll it out. > > > > > What I suggested was mainly for development procedure. Before we hook up > > everything, we need to use the old code. > > That's my suggestion too, but I think it's preferred to not use #ifdefs if > possible so that the still can run unittests on this code. By enabling/disabling > using a field trial that's possible (can be removed later). We can also use a > parameter when instantiating the class. > > On the other hand, maybe it's not necessary to test it... Although I would > appreciate a test of some kind. Michael, do you think it's possible to write > some kind of unittest for this? Might be better to write a test at a higher level later?
https://codereview.webrtc.org/2517243005/diff/1/webrtc/video/video_send_strea... File webrtc/video/video_send_stream.cc (right): https://codereview.webrtc.org/2517243005/diff/1/webrtc/video/video_send_strea... webrtc/video/video_send_stream.cc:1164: bitrate_bps *= payload_fraction_; Would prefer a field trial as well. I even think we may should test this without without changing frame length. So that we would be able the to detect a bugs in the "BWE with overhead" impl.
A integration test for the feature sounds like a good idea as well.
Added a test and the field trial.
https://codereview.webrtc.org/2517243005/diff/20001/webrtc/video/video_send_s... File webrtc/video/video_send_stream.cc (right): https://codereview.webrtc.org/2517243005/diff/20001/webrtc/video/video_send_s... webrtc/video/video_send_stream.cc:409: size_t overhead_per_packet_ GUARDED_BY(overhead_per_packet_crit_); I think better to make the unit clearer: _bytes_per_packet https://codereview.webrtc.org/2517243005/diff/20001/webrtc/video/video_send_s... webrtc/video/video_send_stream.cc:1172: bitrate_bps -= overhead_bps; clamp by zero, can become really large otherwise https://codereview.webrtc.org/2517243005/diff/20001/webrtc/video/video_send_s... webrtc/video/video_send_stream.cc:1240: int transport_overhead_per_packet) { I think better to make the unit clearer: _bytes_per_packet https://codereview.webrtc.org/2517243005/diff/20001/webrtc/video/video_send_s... File webrtc/video/video_send_stream_tests.cc (right): https://codereview.webrtc.org/2517243005/diff/20001/webrtc/video/video_send_s... webrtc/video/video_send_stream_tests.cc:3178: TEST_F(VideoSendStreamTest, RemoveOverheadFromBandwith) { Bandwi"d"th https://codereview.webrtc.org/2517243005/diff/20001/webrtc/video/video_send_s... webrtc/video/video_send_stream_tests.cc:3178: TEST_F(VideoSendStreamTest, RemoveOverheadFromBandwith) { Add comment that "This test verifies that overhead is removed from the bandwidth estimate by testing that the maximum possible target payload rate is smaller than the maximum bandwidth estimate by the overhead rate." https://codereview.webrtc.org/2517243005/diff/20001/webrtc/video/video_send_s... webrtc/video/video_send_stream_tests.cc:3193: if (target_bitrate_kbps_ > bitrate.get_sum_kbps()) { prefer if (bitrate.get_sum_kbps() >= target_bitrate_kbps_) { reached_bitrate_event_.Set(); } return FakeEncoder::SetRateAllocation(bitrate, frameRate); https://codereview.webrtc.org/2517243005/diff/20001/webrtc/video/video_send_s... webrtc/video/video_send_stream_tests.cc:3206: return reached_bitrate_event_.Wait(1000 * 5); why 1000 * 5? https://codereview.webrtc.org/2517243005/diff/20001/webrtc/video/video_send_s... webrtc/video/video_send_stream_tests.cc:3234: // At a bitrate of 60kbps with a packet size of 1200B video and a overhead a overhead -> an overhead https://codereview.webrtc.org/2517243005/diff/20001/webrtc/video/video_send_s... webrtc/video/video_send_stream_tests.cc:3238: EXPECT_FALSE(WaitForBitrate(kMaxBitrateBps)); should test FALSE on kMaxBitrateBps - 1999 But I prefer saving the max bitrate encountered and check if it equals kMaxBitrateBps - 2000
https://codereview.webrtc.org/2517243005/diff/20001/webrtc/video/video_send_s... File webrtc/video/video_send_stream.cc (right): https://codereview.webrtc.org/2517243005/diff/20001/webrtc/video/video_send_s... webrtc/video/video_send_stream.cc:1172: bitrate_bps -= overhead_bps; On 2016/11/24 10:09:49, minyue-webrtc wrote: > clamp by zero, can become really large otherwise Done. https://codereview.webrtc.org/2517243005/diff/20001/webrtc/video/video_send_s... webrtc/video/video_send_stream.cc:1240: int transport_overhead_per_packet) { On 2016/11/24 10:09:49, minyue-webrtc wrote: > I think better to make the unit clearer: _bytes_per_packet Done. https://codereview.webrtc.org/2517243005/diff/20001/webrtc/video/video_send_s... File webrtc/video/video_send_stream_tests.cc (right): https://codereview.webrtc.org/2517243005/diff/20001/webrtc/video/video_send_s... webrtc/video/video_send_stream_tests.cc:3178: TEST_F(VideoSendStreamTest, RemoveOverheadFromBandwith) { On 2016/11/24 10:09:49, minyue-webrtc wrote: > Add comment that "This test verifies that overhead is removed from the bandwidth > estimate by testing that the maximum possible target payload rate is smaller > than the maximum bandwidth estimate by the overhead rate." Done. https://codereview.webrtc.org/2517243005/diff/20001/webrtc/video/video_send_s... webrtc/video/video_send_stream_tests.cc:3178: TEST_F(VideoSendStreamTest, RemoveOverheadFromBandwith) { On 2016/11/24 10:09:49, minyue-webrtc wrote: > Bandwi"d"th Done. https://codereview.webrtc.org/2517243005/diff/20001/webrtc/video/video_send_s... webrtc/video/video_send_stream_tests.cc:3193: if (target_bitrate_kbps_ > bitrate.get_sum_kbps()) { Changed the impl. to wait for Max. https://codereview.webrtc.org/2517243005/diff/20001/webrtc/video/video_send_s... webrtc/video/video_send_stream_tests.cc:3206: return reached_bitrate_event_.Wait(1000 * 5); Changed the impl. to wait for Max. https://codereview.webrtc.org/2517243005/diff/20001/webrtc/video/video_send_s... webrtc/video/video_send_stream_tests.cc:3234: // At a bitrate of 60kbps with a packet size of 1200B video and a overhead On 2016/11/24 10:09:49, minyue-webrtc wrote: > a overhead -> an overhead Done. https://codereview.webrtc.org/2517243005/diff/20001/webrtc/video/video_send_s... webrtc/video/video_send_stream_tests.cc:3238: EXPECT_FALSE(WaitForBitrate(kMaxBitrateBps)); On 2016/11/24 10:09:49, minyue-webrtc wrote: > should test FALSE on kMaxBitrateBps - 1999 > > But I prefer saving the max bitrate encountered and check if it equals > kMaxBitrateBps - 2000 Done.
Patchset #3 (id:30001) has been deleted
nits found, lgtm otherwise https://codereview.webrtc.org/2517243005/diff/50001/webrtc/video/video_send_s... File webrtc/video/video_send_stream_tests.cc (right): https://codereview.webrtc.org/2517243005/diff/50001/webrtc/video/video_send_s... webrtc/video/video_send_stream_tests.cc:3180: // maximum bandwidth estimate by the overhead rate." remove " https://codereview.webrtc.org/2517243005/diff/50001/webrtc/video/video_send_s... webrtc/video/video_send_stream_tests.cc:3198: void WaitForMax() { Wait(); } do we need this function https://codereview.webrtc.org/2517243005/diff/50001/webrtc/video/video_send_s... webrtc/video/video_send_stream_tests.cc:3226: // overhead merge this line with below
https://codereview.webrtc.org/2517243005/diff/50001/webrtc/video/video_send_s... File webrtc/video/video_send_stream_tests.cc (right): https://codereview.webrtc.org/2517243005/diff/50001/webrtc/video/video_send_s... webrtc/video/video_send_stream_tests.cc:3180: // maximum bandwidth estimate by the overhead rate." On 2016/11/24 15:45:46, minyue-webrtc wrote: > remove " Done. https://codereview.webrtc.org/2517243005/diff/50001/webrtc/video/video_send_s... webrtc/video/video_send_stream_tests.cc:3198: void WaitForMax() { Wait(); } Removed the function. I just use Wait() in PerformTest(). https://codereview.webrtc.org/2517243005/diff/50001/webrtc/video/video_send_s... webrtc/video/video_send_stream_tests.cc:3226: // overhead On 2016/11/24 15:45:46, minyue-webrtc wrote: > merge this line with below Done.
found another nit https://codereview.webrtc.org/2517243005/diff/70001/webrtc/video/video_send_s... File webrtc/video/video_send_stream_tests.cc (right): https://codereview.webrtc.org/2517243005/diff/70001/webrtc/video/video_send_s... webrtc/video/video_send_stream_tests.cc:3219: test::ScopedFieldTrials override_field_trials_( nit: override_field_trials
https://codereview.webrtc.org/2517243005/diff/70001/webrtc/video/video_send_s... File webrtc/video/video_send_stream_tests.cc (right): https://codereview.webrtc.org/2517243005/diff/70001/webrtc/video/video_send_s... webrtc/video/video_send_stream_tests.cc:3219: test::ScopedFieldTrials override_field_trials_( On 2016/11/25 10:50:33, minyue-webrtc wrote: > nit: override_field_trials Done.
still lgtm Hi Stefan, Do you have more comments?
lgtm https://codereview.webrtc.org/2517243005/diff/70001/webrtc/video/video_send_s... File webrtc/video/video_send_stream.cc (right): https://codereview.webrtc.org/2517243005/diff/70001/webrtc/video/video_send_s... webrtc/video/video_send_stream.cc:33: #include "webrtc/system_wrappers/include/field_trial.h" Sort
https://codereview.webrtc.org/2517243005/diff/70001/webrtc/video/video_send_s... File webrtc/video/video_send_stream.cc (right): https://codereview.webrtc.org/2517243005/diff/70001/webrtc/video/video_send_s... webrtc/video/video_send_stream.cc:33: #include "webrtc/system_wrappers/include/field_trial.h" On 2016/11/28 15:14:14, stefan-webrtc (holmer) wrote: > Sort Done.
The CQ bit was checked by michaelt@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org, minyue@webrtc.org Link to the patchset: https://codereview.webrtc.org/2517243005/#ps110001 (title: "Respond to comment.")
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
Try jobs failed on following builders: mac_asan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/19759)
The CQ bit was checked by michaelt@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org, minyue@webrtc.org Link to the patchset: https://codereview.webrtc.org/2517243005/#ps130001 (title: "Fix implicite casting problems.")
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
Try jobs failed on following builders: mac_asan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/19801)
The CQ bit was checked by michaelt@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_tsan2 on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/17461)
The CQ bit was checked by michaelt@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_tsan2 on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/17468)
Patchset #8 (id:150001) has been deleted
The CQ bit was checked by michaelt@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_tsan2 on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/17471)
Patchset #8 (id:170001) has been deleted
The CQ bit was checked by michaelt@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_tsan2 on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/17474)
Patchset #8 (id:190001) has been deleted
The CQ bit was checked by michaelt@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_asan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/19904)
The CQ bit was checked by michaelt@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org, minyue@webrtc.org Link to the patchset: https://codereview.webrtc.org/2517243005/#ps210001 (title: "Fix for unittests.")
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
Failed to apply patch for webrtc/video/video_send_stream.cc:
While running git apply --index -p1;
error: patch failed: webrtc/video/video_send_stream.cc:49
error: webrtc/video/video_send_stream.cc: patch does not apply
Patch: webrtc/video/video_send_stream.cc
Index: webrtc/video/video_send_stream.cc
diff --git a/webrtc/video/video_send_stream.cc
b/webrtc/video/video_send_stream.cc
index
51c9dad13160a4cbbc454e6576db27fb4274b3aa..32cc1a23106ab64a5c6f347d16eeaed24a8ad90d
100644
--- a/webrtc/video/video_send_stream.cc
+++ b/webrtc/video/video_send_stream.cc
@@ -27,6 +27,7 @@
#include "webrtc/modules/rtp_rtcp/include/rtp_rtcp.h"
#include "webrtc/modules/utility/include/process_thread.h"
#include "webrtc/modules/video_coding/utility/ivf_file_writer.h"
+#include "webrtc/system_wrappers/include/field_trial.h"
#include "webrtc/video/call_stats.h"
#include "webrtc/video/vie_remb.h"
#include "webrtc/video_send_stream.h"
@@ -49,6 +50,7 @@ std::vector<RtpRtcp*> CreateRtpRtcpModules(
SendDelayStats* send_delay_stats,
RtcEventLog* event_log,
RateLimiter* retransmission_rate_limiter,
+ OverheadObserver* overhead_observer,
size_t num_modules) {
RTC_DCHECK_GT(num_modules, 0u);
RtpRtcp::Configuration configuration;
@@ -72,7 +74,7 @@ std::vector<RtpRtcp*> CreateRtpRtcpModules(
configuration.send_packet_observer = send_delay_stats;
configuration.event_log = event_log;
configuration.retransmission_rate_limiter = retransmission_rate_limiter;
-
+ configuration.overhead_observer = overhead_observer;
std::vector<RtpRtcp*> modules;
for (size_t i = 0; i < num_modules; ++i) {
RtpRtcp* rtp_rtcp = RtpRtcp::CreateRtpRtcp(configuration);
@@ -284,6 +286,7 @@ namespace internal {
// An encoder may deliver frames through the EncodedImageCallback on an
// arbitrary thread.
class VideoSendStreamImpl : public webrtc::BitrateAllocatorObserver,
+ public webrtc::OverheadObserver,
public webrtc::VCMProtectionCallback,
public ViEEncoder::EncoderSink {
public:
@@ -337,6 +340,9 @@ class VideoSendStreamImpl : public
webrtc::BitrateAllocatorObserver,
uint32_t* sent_nack_rate_bps,
uint32_t* sent_fec_rate_bps) override;
+ // Implements OverheadObserver.
+ void OnOverheadChanged(size_t overhead_bytes_per_packet) override;
+
void OnEncoderConfigurationChanged(std::vector<VideoStream> streams,
int min_transmit_bitrate_bps) override;
@@ -398,6 +404,10 @@ class VideoSendStreamImpl : public
webrtc::BitrateAllocatorObserver,
// |weak_ptr_factory_| must be declared last to make sure all WeakPtr's are
// invalidated before any other members are destroyed.
rtc::WeakPtrFactory<VideoSendStreamImpl> weak_ptr_factory_;
+
+ rtc::CriticalSection overhead_bytes_per_packet_crit_;
+ size_t overhead_bytes_per_packet_
GUARDED_BY(overhead_bytes_per_packet_crit_);
+ size_t transport_overhead_bytes_per_packet_;
};
// TODO(tommi): See if there's a more elegant way to create a task that creates
@@ -734,10 +744,13 @@ VideoSendStreamImpl::VideoSendStreamImpl(
send_delay_stats,
event_log,
congestion_controller_->GetRetransmissionRateLimiter(),
+ this,
config_->rtp.ssrcs.size())),
payload_router_(rtp_rtcp_modules_,
config_->encoder_settings.payload_type),
- weak_ptr_factory_(this) {
+ weak_ptr_factory_(this),
+ overhead_bytes_per_packet_(0),
+ transport_overhead_bytes_per_packet_(0) {
RTC_DCHECK_RUN_ON(worker_queue_);
LOG(LS_INFO) << "VideoSendStreamInternal: " << config_->ToString();
weak_ptr_ = weak_ptr_factory_.GetWeakPtr();
@@ -774,15 +787,12 @@ VideoSendStreamImpl::VideoSendStreamImpl(
// TODO(pbos): Should we set CNAME on all RTP modules?
rtp_rtcp_modules_.front()->SetCNAME(config_->rtp.c_name.c_str());
- // 28 to match packet overhead in ModuleRtpRtcpImpl.
- static const size_t kRtpPacketSizeOverhead = 28;
- RTC_DCHECK_LE(config_->rtp.max_packet_size, 0xFFFFu +
kRtpPacketSizeOverhead);
- const uint16_t mtu = static_cast<uint16_t>(config_->rtp.max_packet_size +
- kRtpPacketSizeOverhead);
+
for (RtpRtcp* rtp_rtcp : rtp_rtcp_modules_) {
rtp_rtcp->RegisterRtcpStatisticsCallback(stats_proxy_);
rtp_rtcp->RegisterSendChannelRtpStatisticsCallback(stats_proxy_);
- rtp_rtcp->SetMaxTransferUnit(mtu);
+ rtp_rtcp->SetMaxTransferUnit(
+ static_cast<uint16_t>(config_->rtp.max_packet_size));
rtp_rtcp->RegisterVideoSendPayload(
config_->encoder_settings.payload_type,
config_->encoder_settings.payload_name.c_str());
@@ -1152,6 +1162,19 @@ uint32_t VideoSendStreamImpl::OnBitrateUpdated(uint32_t
bitrate_bps,
RTC_DCHECK_RUN_ON(worker_queue_);
RTC_DCHECK(payload_router_.active())
<< "VideoSendStream::Start has not been called.";
+
+ if (webrtc::field_trial::FindFullName("WebRTC-SendSideBwe-WithOverhead") ==
+ "Enabled") {
+ // Substract overhead from bitrate.
+ rtc::CritScope lock(&overhead_bytes_per_packet_crit_);
+ int packets_per_second =
+ std::ceil(bitrate_bps / (8 * (config_->rtp.max_packet_size +
+ transport_overhead_bytes_per_packet_)));
+ uint32_t overhead_bps = static_cast<uint32_t>(
+ 8 * overhead_bytes_per_packet_ * packets_per_second);
+ bitrate_bps = overhead_bps > bitrate_bps ? 0u : bitrate_bps - overhead_bps;
+ }
+
// Get the encoder target rate. It is the estimated network rate -
// protection overhead.
encoder_target_rate_bps_ = protection_bitrate_calculator_.SetTargetRates(
@@ -1211,10 +1234,22 @@ int VideoSendStreamImpl::ProtectionRequest(
return 0;
}
+void VideoSendStreamImpl::OnOverheadChanged(size_t overhead_bytes_per_packet) {
+ rtc::CritScope lock(&overhead_bytes_per_packet_crit_);
+ overhead_bytes_per_packet_ = overhead_bytes_per_packet;
+}
+
void VideoSendStreamImpl::SetTransportOverhead(
- int transport_overhead_per_packet) {
- for (RtpRtcp* rtp_rtcp : rtp_rtcp_modules_)
- rtp_rtcp->SetTransportOverhead(transport_overhead_per_packet);
+ int transport_overhead_bytes_per_packet) {
+ transport_overhead_bytes_per_packet_ = transport_overhead_bytes_per_packet;
+ RTC_DCHECK_LE(config_->rtp.max_packet_size,
+ 0xFFFFu + transport_overhead_bytes_per_packet);
+ const uint16_t mtu = static_cast<uint16_t>(
+ config_->rtp.max_packet_size + transport_overhead_bytes_per_packet);
+ for (RtpRtcp* rtp_rtcp : rtp_rtcp_modules_) {
+ rtp_rtcp->SetTransportOverhead(transport_overhead_bytes_per_packet);
+ rtp_rtcp->SetMaxTransferUnit(mtu);
+ }
}
} // namespace internal
The CQ bit was checked by michaelt@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org, minyue@webrtc.org Link to the patchset: https://codereview.webrtc.org/2517243005/#ps230001 (title: "Rebased")
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": 230001, "attempt_start_ts": 1480438161923850,
"parent_rev": "e58bd56c747a7da638f8b1c59a192a8580e7a396", "commit_rev":
"76fea84d514e4239177f9bf3c686ba2c0150f294"}
Message was sent while issue was closed.
Description was changed from ========== Remove overhead from video bitrate. BUG=webrtc:6638 ========== to ========== Remove overhead from video bitrate. BUG=webrtc:6638 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:230001)
Message was sent while issue was closed.
Description was changed from ========== Remove overhead from video bitrate. BUG=webrtc:6638 ========== to ========== Remove overhead from video bitrate. BUG=webrtc:6638 Committed: https://crrev.com/a33287761af733d41d8c0a082f3d2c2cefaa9a68 Cr-Commit-Position: refs/heads/master@{#15303} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/a33287761af733d41d8c0a082f3d2c2cefaa9a68 Cr-Commit-Position: refs/heads/master@{#15303}
Message was sent while issue was closed.
hbos@webrtc.org changed reviewers: + hbos@webrtc.org
Message was sent while issue was closed.
https://codereview.webrtc.org/2517243005/diff/230001/webrtc/video/video_send_... File webrtc/video/video_send_stream.cc (right): https://codereview.webrtc.org/2517243005/diff/230001/webrtc/video/video_send_... webrtc/video/video_send_stream.cc:1175: transport_overhead_bytes_per_packet_))); This is causing compile failures in the waterfall: https://build.chromium.org/p/chromium.webrtc.fyi/builders/Mac%20Builder/build... https://build.chromium.org/p/chromium.webrtc.fyi/builders/Android%20Builder%2... https://build.chromium.org/p/chromium.webrtc.fyi/builders/Android%20Builder%2... Maybe you need to #include <cmath>?
Message was sent while issue was closed.
On 2016/11/30 09:29:32, hbos wrote: > https://codereview.webrtc.org/2517243005/diff/230001/webrtc/video/video_send_... > File webrtc/video/video_send_stream.cc (right): > > https://codereview.webrtc.org/2517243005/diff/230001/webrtc/video/video_send_... > webrtc/video/video_send_stream.cc:1175: transport_overhead_bytes_per_packet_))); > This is causing compile failures in the waterfall: > > https://build.chromium.org/p/chromium.webrtc.fyi/builders/Mac%20Builder/build... > > https://build.chromium.org/p/chromium.webrtc.fyi/builders/Android%20Builder%2... > > https://build.chromium.org/p/chromium.webrtc.fyi/builders/Android%20Builder%2... > > Maybe you need to #include <cmath>? Hi Michael, Please add the missing cmath as soon as you can. We may need to revert this CL otherwise.
Message was sent while issue was closed.
On 2016/11/30 09:42:03, minyue-webrtc wrote: > On 2016/11/30 09:29:32, hbos wrote: > > > https://codereview.webrtc.org/2517243005/diff/230001/webrtc/video/video_send_... > > File webrtc/video/video_send_stream.cc (right): > > > > > https://codereview.webrtc.org/2517243005/diff/230001/webrtc/video/video_send_... > > webrtc/video/video_send_stream.cc:1175: > transport_overhead_bytes_per_packet_))); > > This is causing compile failures in the waterfall: > > > > > https://build.chromium.org/p/chromium.webrtc.fyi/builders/Mac%20Builder/build... > > > > > https://build.chromium.org/p/chromium.webrtc.fyi/builders/Android%20Builder%2... > > > > > https://build.chromium.org/p/chromium.webrtc.fyi/builders/Android%20Builder%2... > > > > Maybe you need to #include <cmath>? > > Hi Michael, > > Please add the missing cmath as soon as you can. We may need to revert this CL > otherwise. Added cmath include.
Message was sent while issue was closed.
On 2016/11/30 09:42:03, minyue-webrtc wrote: > On 2016/11/30 09:29:32, hbos wrote: > > > https://codereview.webrtc.org/2517243005/diff/230001/webrtc/video/video_send_... > > File webrtc/video/video_send_stream.cc (right): > > > > > https://codereview.webrtc.org/2517243005/diff/230001/webrtc/video/video_send_... > > webrtc/video/video_send_stream.cc:1175: > transport_overhead_bytes_per_packet_))); > > This is causing compile failures in the waterfall: > > > > > https://build.chromium.org/p/chromium.webrtc.fyi/builders/Mac%20Builder/build... > > > > > https://build.chromium.org/p/chromium.webrtc.fyi/builders/Android%20Builder%2... > > > > > https://build.chromium.org/p/chromium.webrtc.fyi/builders/Android%20Builder%2... > > > > Maybe you need to #include <cmath>? > > Hi Michael, > > Please add the missing cmath as soon as you can. We may need to revert this CL > otherwise. No worries, I uploaded: https://codereview.webrtc.org/2544473002 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
