Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2347023002:
BitrateProber: Support higher probing bitrates (Closed)
DescriptionBitrateProber: Support higher probing bitrates
Currently, BitrateProber does not scale higher than 2 Mbps to 6 Mbps. The actual
number is dependent on the size of the last packet. If a packet of around 250
bytes is used for probing, it fails above 2 Mbps.
BitrateProber now provides a recommendation on probe size instead of a
packet size. PacedSender utilizes this to decide on the number of packets
per probe. This enables BitrateProber to scale up-to higher bitrates.
Tests with chromoting show it stalls at about 10 Mbps (perhaps due to the
limitation on the simulation pipeline to deliver packets).
BUG=webrtc:6332
Committed: https://crrev.com/cc5903e15f47ec9e2b9282225da38d62aae5ada2
Cr-Commit-Position: refs/heads/master@{#14503}
Patch Set 1 : BitrateProber: Support higher probing bitrates #
Total comments: 13
Patch Set 2 : addressed comments #
Total comments: 4
Patch Set 3 : Add tests #
Total comments: 17
Patch Set 4 : Address comments #
Total comments: 5
Patch Set 5 : Add test for validating padding overhead #
Total comments: 1
Patch Set 6 : Fix test to turn on RTX #Patch Set 7 : Fix thread race and test flakiness on some platforms #
Total comments: 15
Patch Set 8 : Fix nits #Patch Set 9 : Rebased #
Dependent Patchsets: Messages
Total messages: 42 (17 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== BitrateProber: Support higher probing bitrates Currently, BitrateProber does not scale higher than 2 Mbps to 6 Mbps. The actual number is dependent on the size of the last packet. If a packet of around 250 bytes is used for probing, it fails above 2 Mbps. BitrateProber now provides a recommendation on probe size instead of a packet size. PacedSender utilizes this to decide on the number of packets per probe. This enables BitrateProber to scale up-to higher bitrates. Tests with chromoting show it stalls at about 10 Mbps (perhaps due to the limitation on the simulation pipeline to deliver packets). BUG=6332 ========== to ========== BitrateProber: Support higher probing bitrates Currently, BitrateProber does not scale higher than 2 Mbps to 6 Mbps. The actual number is dependent on the size of the last packet. If a packet of around 250 bytes is used for probing, it fails above 2 Mbps. BitrateProber now provides a recommendation on probe size instead of a packet size. PacedSender utilizes this to decide on the number of packets per probe. This enables BitrateProber to scale up-to higher bitrates. Tests with chromoting show it stalls at about 10 Mbps (perhaps due to the limitation on the simulation pipeline to deliver packets). BUG=6332 ==========
isheriff@chromium.org changed reviewers: + philipel@webrtc.org, stefan@webrtc.org
PTAL
Patchset #1 (id:20001) has been deleted
https://codereview.chromium.org/2347023002/diff/40001/webrtc/modules/pacing/b... File webrtc/modules/pacing/bitrate_prober.cc (right): https://codereview.chromium.org/2347023002/diff/40001/webrtc/modules/pacing/b... webrtc/modules/pacing/bitrate_prober.cc:148: size_t BitrateProber::RecommendedProbeSize() const { Isn't this the recommended minimum size? Larger sizes are still ok, and perhaps even recommended, right? https://codereview.chromium.org/2347023002/diff/40001/webrtc/modules/pacing/b... webrtc/modules/pacing/bitrate_prober.cc:155: RTC_DCHECK(probing_state_ == ProbingState::kActive); DCHECK_EQ https://codereview.chromium.org/2347023002/diff/40001/webrtc/modules/pacing/b... webrtc/modules/pacing/bitrate_prober.cc:156: assert(bytes > 0); RTC_DCHECK_GT https://codereview.chromium.org/2347023002/diff/40001/webrtc/modules/pacing/p... File webrtc/modules/pacing/paced_sender.cc (right): https://codereview.chromium.org/2347023002/diff/40001/webrtc/modules/pacing/p... webrtc/modules/pacing/paced_sender.cc:422: if (is_probing && bytes_sent > recommended_probe_size) { This may look like a bug to the reader as it seems like we are sending packets in the middle of a probe which are not part of the probe. It's by design because we may have small audio packets that must be sent, but maybe we should say that in a comment?
https://codereview.chromium.org/2347023002/diff/40001/webrtc/modules/pacing/b... File webrtc/modules/pacing/bitrate_prober.cc (right): https://codereview.chromium.org/2347023002/diff/40001/webrtc/modules/pacing/b... webrtc/modules/pacing/bitrate_prober.cc:148: size_t BitrateProber::RecommendedProbeSize() const { On 2016/09/16 07:42:53, stefan-webrtc (holmer) wrote: > Isn't this the recommended minimum size? Larger sizes are still ok, and perhaps > even recommended, right? Right, updated to reflect that. https://codereview.chromium.org/2347023002/diff/40001/webrtc/modules/pacing/b... webrtc/modules/pacing/bitrate_prober.cc:155: RTC_DCHECK(probing_state_ == ProbingState::kActive); On 2016/09/16 07:42:53, stefan-webrtc (holmer) wrote: > DCHECK_EQ DCHECK_EQ does not work on scoped enums https://codereview.chromium.org/2347023002/diff/40001/webrtc/modules/pacing/b... webrtc/modules/pacing/bitrate_prober.cc:156: assert(bytes > 0); On 2016/09/16 07:42:53, stefan-webrtc (holmer) wrote: > RTC_DCHECK_GT This assert is actually redundant given it is an unsigned type. Removed. https://codereview.chromium.org/2347023002/diff/40001/webrtc/modules/pacing/p... File webrtc/modules/pacing/paced_sender.cc (right): https://codereview.chromium.org/2347023002/diff/40001/webrtc/modules/pacing/p... webrtc/modules/pacing/paced_sender.cc:422: if (is_probing && bytes_sent > recommended_probe_size) { On 2016/09/16 07:42:53, stefan-webrtc (holmer) wrote: > This may look like a bug to the reader as it seems like we are sending packets > in the middle of a probe which are not part of the probe. It's by design because > we may have small audio packets that must be sent, but maybe we should say that > in a comment? Not sure I follow. The is_probing flag indicates whether we are in a probing state or not. The check here is for ensuring we send enough probe packets before we return. Can you elaborate what comment what you would like to see here ?
PTAL. I have restructured it a bit so that ProbeSent is always called at end. Still need to fix a test.
https://codereview.chromium.org/2347023002/diff/40001/webrtc/modules/pacing/b... File webrtc/modules/pacing/bitrate_prober.cc (right): https://codereview.chromium.org/2347023002/diff/40001/webrtc/modules/pacing/b... webrtc/modules/pacing/bitrate_prober.cc:155: RTC_DCHECK(probing_state_ == ProbingState::kActive); On 2016/09/19 06:05:11, Irfan wrote: > On 2016/09/16 07:42:53, stefan-webrtc (holmer) wrote: > > DCHECK_EQ > > DCHECK_EQ does not work on scoped enums Acknowledged. https://codereview.chromium.org/2347023002/diff/40001/webrtc/modules/pacing/b... webrtc/modules/pacing/bitrate_prober.cc:156: assert(bytes > 0); On 2016/09/19 06:05:11, Irfan wrote: > On 2016/09/16 07:42:53, stefan-webrtc (holmer) wrote: > > RTC_DCHECK_GT > > This assert is actually redundant given it is an unsigned type. Removed. Is 0 not still possible? https://codereview.chromium.org/2347023002/diff/40001/webrtc/modules/pacing/p... File webrtc/modules/pacing/paced_sender.cc (right): https://codereview.chromium.org/2347023002/diff/40001/webrtc/modules/pacing/p... webrtc/modules/pacing/paced_sender.cc:422: if (is_probing && bytes_sent > recommended_probe_size) { On 2016/09/19 06:05:11, Irfan wrote: > On 2016/09/16 07:42:53, stefan-webrtc (holmer) wrote: > > This may look like a bug to the reader as it seems like we are sending packets > > in the middle of a probe which are not part of the probe. It's by design > because > > we may have small audio packets that must be sent, but maybe we should say > that > > in a comment? > > Not sure I follow. The is_probing flag indicates whether we are in a probing > state or not. The check here is for ensuring we send enough probe packets before > we return. > > Can you elaborate what comment what you would like to see here ? I'm thinking of the case where we have paced out, e.g., an audio packet which is smaller than recommended_probe_size. In that case we're not reporting it as a ProbeSent, which looks odd if we don't explain with a comment. https://codereview.chromium.org/2347023002/diff/60001/webrtc/modules/pacing/p... File webrtc/modules/pacing/paced_sender.cc (right): https://codereview.chromium.org/2347023002/diff/60001/webrtc/modules/pacing/p... webrtc/modules/pacing/paced_sender.cc:423: break; Is this because you want to be able to fill up with padding or retransmits packets if this probe wasn't large enough? https://codereview.chromium.org/2347023002/diff/60001/webrtc/modules/pacing/p... webrtc/modules/pacing/paced_sender.cc:440: bytes_sent += SendPadding(padding_needed, probe_cluster_id); I'm starting to think that we may want a higher level test which verifies that when we do this type of probing (multiple packets per probe) we should ensure that retransmits are used so we don't end up sending a large number of padding packets. Maybe in video_send_stream_tests.cc?
https://codereview.chromium.org/2347023002/diff/40001/webrtc/modules/pacing/b... File webrtc/modules/pacing/bitrate_prober.cc (right): https://codereview.chromium.org/2347023002/diff/40001/webrtc/modules/pacing/b... webrtc/modules/pacing/bitrate_prober.cc:156: assert(bytes > 0); On 2016/09/19 07:24:55, stefan-webrtc (holmer) wrote: > On 2016/09/19 06:05:11, Irfan wrote: > > On 2016/09/16 07:42:53, stefan-webrtc (holmer) wrote: > > > RTC_DCHECK_GT > > > > This assert is actually redundant given it is an unsigned type. Removed. > > Is 0 not still possible? Ah yes, added https://codereview.chromium.org/2347023002/diff/40001/webrtc/modules/pacing/p... File webrtc/modules/pacing/paced_sender.cc (right): https://codereview.chromium.org/2347023002/diff/40001/webrtc/modules/pacing/p... webrtc/modules/pacing/paced_sender.cc:422: if (is_probing && bytes_sent > recommended_probe_size) { On 2016/09/19 07:24:56, stefan-webrtc (holmer) wrote: > On 2016/09/19 06:05:11, Irfan wrote: > > On 2016/09/16 07:42:53, stefan-webrtc (holmer) wrote: > > > This may look like a bug to the reader as it seems like we are sending > packets > > > in the middle of a probe which are not part of the probe. It's by design > > because > > > we may have small audio packets that must be sent, but maybe we should say > > that > > > in a comment? > > > > Not sure I follow. The is_probing flag indicates whether we are in a probing > > state or not. The check here is for ensuring we send enough probe packets > before > > we return. > > > > Can you elaborate what comment what you would like to see here ? > > I'm thinking of the case where we have paced out, e.g., an audio packet which is > smaller than recommended_probe_size. In that case we're not reporting it as a > ProbeSent, which looks odd if we don't explain with a comment. We report once the aggregate bytes is above the recommended size. In the latest PS, this should be more clear that we always report the byte size in the end before returning from Process(). If there is an audio packet here, it will actually add up in this aggregate of bytes_sent and be reported to be prober. https://codereview.chromium.org/2347023002/diff/60001/webrtc/modules/pacing/p... File webrtc/modules/pacing/paced_sender.cc (right): https://codereview.chromium.org/2347023002/diff/60001/webrtc/modules/pacing/p... webrtc/modules/pacing/paced_sender.cc:423: break; On 2016/09/19 07:24:56, stefan-webrtc (holmer) wrote: > Is this because you want to be able to fill up with padding or retransmits > packets if this probe wasn't large enough? I restructured this to ensure there is a single call to ProbeSent() towards the end. Basically, we had 3 different places where we would return and avoiding those helps so we do not duplicate (or forget) reporting bytes sent. This also helps further with a follow up CL where we report to AlrDetector. https://codereview.chromium.org/2347023002/diff/60001/webrtc/modules/pacing/p... webrtc/modules/pacing/paced_sender.cc:440: bytes_sent += SendPadding(padding_needed, probe_cluster_id); On 2016/09/19 07:24:56, stefan-webrtc (holmer) wrote: > I'm starting to think that we may want a higher level test which verifies that > when we do this type of probing (multiple packets per probe) we should ensure > that retransmits are used so we don't end up sending a large number of padding > packets. Maybe in video_send_stream_tests.cc? Yes, I did want to add a test once the CL looks good to you guys.
Patchset #3 (id:80001) has been deleted
Patchset #3 (id:100001) has been deleted
Patchset #3 (id:120001) has been deleted
Description was changed from ========== BitrateProber: Support higher probing bitrates Currently, BitrateProber does not scale higher than 2 Mbps to 6 Mbps. The actual number is dependent on the size of the last packet. If a packet of around 250 bytes is used for probing, it fails above 2 Mbps. BitrateProber now provides a recommendation on probe size instead of a packet size. PacedSender utilizes this to decide on the number of packets per probe. This enables BitrateProber to scale up-to higher bitrates. Tests with chromoting show it stalls at about 10 Mbps (perhaps due to the limitation on the simulation pipeline to deliver packets). BUG=6332 ========== to ========== BitrateProber: Support higher probing bitrates Currently, BitrateProber does not scale higher than 2 Mbps to 6 Mbps. The actual number is dependent on the size of the last packet. If a packet of around 250 bytes is used for probing, it fails above 2 Mbps. BitrateProber now provides a recommendation on probe size instead of a packet size. PacedSender utilizes this to decide on the number of packets per probe. This enables BitrateProber to scale up-to higher bitrates. Tests with chromoting show it stalls at about 10 Mbps (perhaps due to the limitation on the simulation pipeline to deliver packets). BUG=webrtc:6332 ==========
PTAL
PTAL
https://codereview.webrtc.org/2347023002/diff/140001/webrtc/modules/pacing/bi... File webrtc/modules/pacing/bitrate_prober.h (right): https://codereview.webrtc.org/2347023002/diff/140001/webrtc/modules/pacing/bi... webrtc/modules/pacing/bitrate_prober.h:41: void CreateProbeCluster(int bitrate_bps, int num_probes); Not for this CL, but I think it would be nice if we didn't have to specify how many probes we want to send, and instead let the BitrateProber determine how many bytes it needs to send in order to accurately probe for the given bitrate. https://codereview.webrtc.org/2347023002/diff/140001/webrtc/modules/pacing/pa... File webrtc/modules/pacing/paced_sender_unittest.cc (right): https://codereview.webrtc.org/2347023002/diff/140001/webrtc/modules/pacing/pa... webrtc/modules/pacing/paced_sender_unittest.cc:815: while (packet_sender.packets_sent() < kFirstClusterCount) { Simplify loop: int time_until_process = send_bucket_->TimeUntilNextProcess(); clock_.AdvanceTimeMilliseconds(time_until_process); send_bucket_->Process(); https://codereview.webrtc.org/2347023002/diff/140001/webrtc/modules/pacing/pa... webrtc/modules/pacing/paced_sender_unittest.cc:825: EXPECT_GT( Why do we EXPECT_GT? Shouldn't we EXPECT_NEAR instead? https://codereview.webrtc.org/2347023002/diff/140001/webrtc/modules/pacing/pa... webrtc/modules/pacing/paced_sender_unittest.cc:830: while (packet_sender.packets_sent() < simplfy loop https://codereview.webrtc.org/2347023002/diff/140001/webrtc/modules/pacing/pa... webrtc/modules/pacing/paced_sender_unittest.cc:841: EXPECT_GT( EXPECT_NEAR? https://codereview.webrtc.org/2347023002/diff/140001/webrtc/modules/pacing/pa... webrtc/modules/pacing/paced_sender_unittest.cc:865: while (process_count < kFirstClusterCount) { Simplify loop https://codereview.webrtc.org/2347023002/diff/140001/webrtc/modules/pacing/pa... webrtc/modules/pacing/paced_sender_unittest.cc:877: EXPECT_GT((packets_sent * kPacketSize * 8000 + padding_sent) / EXPECT_NEAR https://codereview.webrtc.org/2347023002/diff/140001/webrtc/modules/pacing/pa... webrtc/modules/pacing/paced_sender_unittest.cc:901: while (packet_sender.packets_sent() < kFirstClusterCount) { Simplify loop https://codereview.webrtc.org/2347023002/diff/140001/webrtc/modules/pacing/pa... webrtc/modules/pacing/paced_sender_unittest.cc:913: EXPECT_GT((packets_sent * kPacketSize * 8000) / EXPECT_NEAR
https://codereview.chromium.org/2347023002/diff/140001/webrtc/modules/pacing/... File webrtc/modules/pacing/paced_sender_unittest.cc (right): https://codereview.chromium.org/2347023002/diff/140001/webrtc/modules/pacing/... webrtc/modules/pacing/paced_sender_unittest.cc:815: while (packet_sender.packets_sent() < kFirstClusterCount) { On 2016/09/21 09:26:40, philipel wrote: > Simplify loop: > int time_until_process = send_bucket_->TimeUntilNextProcess(); > clock_.AdvanceTimeMilliseconds(time_until_process); > send_bucket_->Process(); Done. https://codereview.chromium.org/2347023002/diff/140001/webrtc/modules/pacing/... webrtc/modules/pacing/paced_sender_unittest.cc:825: EXPECT_GT( On 2016/09/21 09:26:40, philipel wrote: > Why do we EXPECT_GT? Shouldn't we EXPECT_NEAR instead? I have clarified this in the comments. Have switched to EXPECT_NEAR since there is value in validating the range. https://codereview.chromium.org/2347023002/diff/140001/webrtc/modules/pacing/... webrtc/modules/pacing/paced_sender_unittest.cc:830: while (packet_sender.packets_sent() < On 2016/09/21 09:26:40, philipel wrote: > simplfy loop Done. https://codereview.chromium.org/2347023002/diff/140001/webrtc/modules/pacing/... webrtc/modules/pacing/paced_sender_unittest.cc:841: EXPECT_GT( On 2016/09/21 09:26:40, philipel wrote: > EXPECT_NEAR? Done. https://codereview.chromium.org/2347023002/diff/140001/webrtc/modules/pacing/... webrtc/modules/pacing/paced_sender_unittest.cc:865: while (process_count < kFirstClusterCount) { On 2016/09/21 09:26:40, philipel wrote: > Simplify loop Done. https://codereview.chromium.org/2347023002/diff/140001/webrtc/modules/pacing/... webrtc/modules/pacing/paced_sender_unittest.cc:877: EXPECT_GT((packets_sent * kPacketSize * 8000 + padding_sent) / On 2016/09/21 09:26:40, philipel wrote: > EXPECT_NEAR Done. https://codereview.chromium.org/2347023002/diff/140001/webrtc/modules/pacing/... webrtc/modules/pacing/paced_sender_unittest.cc:901: while (packet_sender.packets_sent() < kFirstClusterCount) { On 2016/09/21 09:26:39, philipel wrote: > Simplify loop Done. https://codereview.chromium.org/2347023002/diff/140001/webrtc/modules/pacing/... webrtc/modules/pacing/paced_sender_unittest.cc:913: EXPECT_GT((packets_sent * kPacketSize * 8000) / On 2016/09/21 09:26:40, philipel wrote: > EXPECT_NEAR Done.
PTAL
lgtm
Stefan, can you PTAL ?
lg, but I'm missing one of the tests I requested. https://codereview.chromium.org/2347023002/diff/160001/webrtc/modules/pacing/... File webrtc/modules/pacing/paced_sender_unittest.cc (right): https://codereview.chromium.org/2347023002/diff/160001/webrtc/modules/pacing/... webrtc/modules/pacing/paced_sender_unittest.cc:869: process_count++; ++process_count; https://codereview.chromium.org/2347023002/diff/160001/webrtc/modules/pacing/... webrtc/modules/pacing/paced_sender_unittest.cc:882: TEST_F(PacedSenderTest, ProbingUsesRetransmissions) { This was not the test I was after in my earlier comments. I'd like to have an integration test which verifies that probing doesn't send a lot of padding in response to calls to TimeToSendPadding(). I simply want to ensure that, if we do probing at high rates, we don't end up with a huge number of padding packets for some reason. I think they should all be preemptive retransmits. This is not possible to test with the pacer alone, so I'd suggest implementing it in video_send_stream_tests.cc.
https://codereview.chromium.org/2347023002/diff/160001/webrtc/modules/pacing/... File webrtc/modules/pacing/paced_sender_unittest.cc (right): https://codereview.chromium.org/2347023002/diff/160001/webrtc/modules/pacing/... webrtc/modules/pacing/paced_sender_unittest.cc:869: process_count++; On 2016/09/23 13:29:11, stefan-webrtc (holmer) wrote: > ++process_count; Done. https://codereview.chromium.org/2347023002/diff/160001/webrtc/modules/pacing/... webrtc/modules/pacing/paced_sender_unittest.cc:882: TEST_F(PacedSenderTest, ProbingUsesRetransmissions) { On 2016/09/23 13:29:11, stefan-webrtc (holmer) wrote: > This was not the test I was after in my earlier comments. > > I'd like to have an integration test which verifies that probing doesn't send a > lot of padding in response to calls to TimeToSendPadding(). I simply want to > ensure that, if we do probing at high rates, we don't end up with a huge number > of padding packets for some reason. I think they should all be preemptive > retransmits. > > This is not possible to test with the pacer alone, so I'd suggest implementing > it in video_send_stream_tests.cc. I have added a test that measures the padding overhead from an end-to-end perspective. The reality is that when there are no packets that are outstanding for retransmission, probing will end up sending padded packets for measuring bitrate. I see about 40% padding for 10 Mbps ramp in the test. This will really depend on how many retransmissions or packets to send are available. The good news here is that padding is only sent when we know there is bitrate available on network and when there is nothing to send to avail that. We also only send a fixed amount of padding in terms of time (so more bits at higher bitrate).
https://codereview.chromium.org/2347023002/diff/160001/webrtc/modules/pacing/... File webrtc/modules/pacing/paced_sender_unittest.cc (right): https://codereview.chromium.org/2347023002/diff/160001/webrtc/modules/pacing/... webrtc/modules/pacing/paced_sender_unittest.cc:882: TEST_F(PacedSenderTest, ProbingUsesRetransmissions) { On 2016/09/26 23:00:29, Irfan wrote: > On 2016/09/23 13:29:11, stefan-webrtc (holmer) wrote: > > This was not the test I was after in my earlier comments. > > > > I'd like to have an integration test which verifies that probing doesn't send > a > > lot of padding in response to calls to TimeToSendPadding(). I simply want to > > ensure that, if we do probing at high rates, we don't end up with a huge > number > > of padding packets for some reason. I think they should all be preemptive > > retransmits. > > > > This is not possible to test with the pacer alone, so I'd suggest implementing > > it in video_send_stream_tests.cc. > > I have added a test that measures the padding overhead from an end-to-end > perspective. > > The reality is that when there are no packets that are outstanding for > retransmission, probing will end up sending padded packets for measuring > bitrate. I see about 40% padding for 10 Mbps ramp in the test. This will really > depend on how many retransmissions or packets to send are available. > > The good news here is that padding is only sent when we know there is bitrate > available on network and when there is nothing to send to avail that. We also > only send a fixed amount of padding in terms of time (so more bits at higher > bitrate). Right, that's expected in some situations. But in most situations there should be video packets available for preemptive retransmission, right? Is that not the case in your new test for some reason? We don't send padding packets to probe before we've sent our first video packet, right? https://codereview.chromium.org/2347023002/diff/180001/webrtc/video/video_sen... File webrtc/video/video_send_stream_tests.cc (right): https://codereview.chromium.org/2347023002/diff/180001/webrtc/video/video_sen... webrtc/video/video_send_stream_tests.cc:1094: SleepMs(5000); Should you instead run the test until Call::Stats::send_bandwidth_bps > 8000 kbps or something?
Thanks for the pointer on RTX. I now see about 2-3% of traffic being padding. Have updated the test. PTAL
Patchset #6 (id:200001) has been deleted
Patchset #7 (id:240001) has been deleted
Patchset #7 (id:260001) has been deleted
Patchset #7 (id:280001) has been deleted
Patchset #7 (id:300001) has been deleted
PTAL. I reverted the tight control on exceeding 8 Mbps since some of the platforms seem to have scheduling issues on sender side which prevents BW ramp up and causes flakiness. Instead use a time limit as before.
https://codereview.webrtc.org/2347023002/diff/320001/webrtc/modules/pacing/pa... File webrtc/modules/pacing/paced_sender_unittest.cc (right): https://codereview.webrtc.org/2347023002/diff/320001/webrtc/modules/pacing/pa... webrtc/modules/pacing/paced_sender_unittest.cc:831: EXPECT_EQ(0, padding_sent_)? https://codereview.webrtc.org/2347023002/diff/320001/webrtc/modules/pacing/pa... webrtc/modules/pacing/paced_sender_unittest.cc:882: TEST_F(PacedSenderTest, ProbingUsesRetransmissions) { Do you think this provides any value? A retransmission is more or less equivalent to any other packet from the pacer point of view. https://codereview.webrtc.org/2347023002/diff/320001/webrtc/video/video_send_... File webrtc/video/video_send_stream_tests.cc (right): https://codereview.webrtc.org/2347023002/diff/320001/webrtc/video/video_send_... webrtc/video/video_send_stream_tests.cc:1068: padding_length_ += header.paddingLength; You only have to protect padding_length_ and total_length_ with the crit. https://codereview.webrtc.org/2347023002/diff/320001/webrtc/video/video_send_... webrtc/video/video_send_stream_tests.cc:1091: // Turn on RTX End with . https://codereview.webrtc.org/2347023002/diff/320001/webrtc/video/video_send_... webrtc/video/video_send_stream_tests.cc:1110: EXPECT_LT(padding_length_, .1 * total_length_); Just a question: I would have expected 0% padding. Do you know why that is not the case? https://codereview.webrtc.org/2347023002/diff/320001/webrtc/video/video_send_... webrtc/video/video_send_stream_tests.cc:1117: size_t total_length_; Add GUARDED_BY
https://codereview.chromium.org/2347023002/diff/320001/webrtc/modules/pacing/... File webrtc/modules/pacing/paced_sender_unittest.cc (right): https://codereview.chromium.org/2347023002/diff/320001/webrtc/modules/pacing/... webrtc/modules/pacing/paced_sender_unittest.cc:831: On 2016/09/30 08:37:21, stefan-webrtc (holmer) wrote: > EXPECT_EQ(0, padding_sent_)? Done. https://codereview.chromium.org/2347023002/diff/320001/webrtc/modules/pacing/... webrtc/modules/pacing/paced_sender_unittest.cc:882: TEST_F(PacedSenderTest, ProbingUsesRetransmissions) { On 2016/09/30 08:37:21, stefan-webrtc (holmer) wrote: > Do you think this provides any value? A retransmission is more or less > equivalent to any other packet from the pacer point of view. ok, removed this https://codereview.chromium.org/2347023002/diff/320001/webrtc/video/video_sen... File webrtc/video/video_send_stream_tests.cc (right): https://codereview.chromium.org/2347023002/diff/320001/webrtc/video/video_sen... webrtc/video/video_send_stream_tests.cc:1068: padding_length_ += header.paddingLength; On 2016/09/30 08:37:21, stefan-webrtc (holmer) wrote: > You only have to protect padding_length_ and total_length_ with the crit. Right, but given the contention is a one time thing at end of test, having the CritScope at top keeps it a bit cleaner. https://codereview.chromium.org/2347023002/diff/320001/webrtc/video/video_sen... webrtc/video/video_send_stream_tests.cc:1091: // Turn on RTX On 2016/09/30 08:37:21, stefan-webrtc (holmer) wrote: > End with . Done. https://codereview.chromium.org/2347023002/diff/320001/webrtc/video/video_sen... webrtc/video/video_send_stream_tests.cc:1110: EXPECT_LT(padding_length_, .1 * total_length_); On 2016/09/30 08:37:21, stefan-webrtc (holmer) wrote: > Just a question: I would have expected 0% padding. Do you know why that is not > the case? Adding logs, I see that there are a small number of instances where there is nothing in packet_history to force rtx https://codereview.chromium.org/2347023002/diff/320001/webrtc/video/video_sen... webrtc/video/video_send_stream_tests.cc:1117: size_t total_length_; On 2016/09/30 08:37:21, stefan-webrtc (holmer) wrote: > Add GUARDED_BY Done.
lgtm https://codereview.webrtc.org/2347023002/diff/320001/webrtc/video/video_send_... File webrtc/video/video_send_stream_tests.cc (right): https://codereview.webrtc.org/2347023002/diff/320001/webrtc/video/video_send_... webrtc/video/video_send_stream_tests.cc:1068: padding_length_ += header.paddingLength; On 2016/10/03 18:45:26, Irfan wrote: > On 2016/09/30 08:37:21, stefan-webrtc (holmer) wrote: > > You only have to protect padding_length_ and total_length_ with the crit. > > Right, but given the contention is a one time thing at end of test, having the > CritScope at top keeps it a bit cleaner. Acknowledged. https://codereview.webrtc.org/2347023002/diff/320001/webrtc/video/video_send_... webrtc/video/video_send_stream_tests.cc:1110: EXPECT_LT(padding_length_, .1 * total_length_); On 2016/10/03 18:45:26, Irfan wrote: > On 2016/09/30 08:37:21, stefan-webrtc (holmer) wrote: > > Just a question: I would have expected 0% padding. Do you know why that is not > > the case? > > Adding logs, I see that there are a small number of instances where there is > nothing in packet_history to force rtx Out of curiosity, are those sent before any media packets are sent, or are they sent because the packets in the history are too old? Or maybe it's just that the requested padding size is too small for it to make sense to send a full sized media packet.
The CQ bit was checked by isheriff@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from philipel@webrtc.org Link to the patchset: https://codereview.chromium.org/2347023002/#ps360001 (title: "Rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
https://codereview.webrtc.org/2347023002/diff/320001/webrtc/video/video_send_... File webrtc/video/video_send_stream_tests.cc (right): https://codereview.webrtc.org/2347023002/diff/320001/webrtc/video/video_send_... webrtc/video/video_send_stream_tests.cc:1110: EXPECT_LT(padding_length_, .1 * total_length_); On 2016/10/04 14:56:40, stefan-webrtc (holmer) wrote: > On 2016/10/03 18:45:26, Irfan wrote: > > On 2016/09/30 08:37:21, stefan-webrtc (holmer) wrote: > > > Just a question: I would have expected 0% padding. Do you know why that is > not > > > the case? > > > > Adding logs, I see that there are a small number of instances where there is > > nothing in packet_history to force rtx > > Out of curiosity, are those sent before any media packets are sent, or are they > sent because the packets in the history are too old? Or maybe it's just that the > requested padding size is too small for it to make sense to send a full sized > media packet. Clarification here: Turns out, its actually because there is a small amount of pad is requested (< 50 bytes) and rtp_packet_history just returns nothing when that happens.
Message was sent while issue was closed.
Description was changed from ========== BitrateProber: Support higher probing bitrates Currently, BitrateProber does not scale higher than 2 Mbps to 6 Mbps. The actual number is dependent on the size of the last packet. If a packet of around 250 bytes is used for probing, it fails above 2 Mbps. BitrateProber now provides a recommendation on probe size instead of a packet size. PacedSender utilizes this to decide on the number of packets per probe. This enables BitrateProber to scale up-to higher bitrates. Tests with chromoting show it stalls at about 10 Mbps (perhaps due to the limitation on the simulation pipeline to deliver packets). BUG=webrtc:6332 ========== to ========== BitrateProber: Support higher probing bitrates Currently, BitrateProber does not scale higher than 2 Mbps to 6 Mbps. The actual number is dependent on the size of the last packet. If a packet of around 250 bytes is used for probing, it fails above 2 Mbps. BitrateProber now provides a recommendation on probe size instead of a packet size. PacedSender utilizes this to decide on the number of packets per probe. This enables BitrateProber to scale up-to higher bitrates. Tests with chromoting show it stalls at about 10 Mbps (perhaps due to the limitation on the simulation pipeline to deliver packets). BUG=webrtc:6332 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:360001)
Message was sent while issue was closed.
Description was changed from ========== BitrateProber: Support higher probing bitrates Currently, BitrateProber does not scale higher than 2 Mbps to 6 Mbps. The actual number is dependent on the size of the last packet. If a packet of around 250 bytes is used for probing, it fails above 2 Mbps. BitrateProber now provides a recommendation on probe size instead of a packet size. PacedSender utilizes this to decide on the number of packets per probe. This enables BitrateProber to scale up-to higher bitrates. Tests with chromoting show it stalls at about 10 Mbps (perhaps due to the limitation on the simulation pipeline to deliver packets). BUG=webrtc:6332 ========== to ========== BitrateProber: Support higher probing bitrates Currently, BitrateProber does not scale higher than 2 Mbps to 6 Mbps. The actual number is dependent on the size of the last packet. If a packet of around 250 bytes is used for probing, it fails above 2 Mbps. BitrateProber now provides a recommendation on probe size instead of a packet size. PacedSender utilizes this to decide on the number of packets per probe. This enables BitrateProber to scale up-to higher bitrates. Tests with chromoting show it stalls at about 10 Mbps (perhaps due to the limitation on the simulation pipeline to deliver packets). BUG=webrtc:6332 Committed: https://crrev.com/cc5903e15f47ec9e2b9282225da38d62aae5ada2 Cr-Commit-Position: refs/heads/master@{#14503} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/cc5903e15f47ec9e2b9282225da38d62aae5ada2 Cr-Commit-Position: refs/heads/master@{#14503} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
