Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1474533006:
Fix bug in calculation of averge queue time in paced sender. (Closed)
|
Created:
5 years ago by sprang_webrtc Modified:
5 years ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhuangzesen_agora.io, stefan-webrtc, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionFix bug in calculation of averge queue time in paced sender.
Also work around a flaw in fake encoder which caused bogus perf
regression in rampup tests.
BUG=560434
R=mflodman@webrtc.org, stefan@webrtc.org
Committed: https://crrev.com/ad113e50d251c95adcf501ed29f8312ad1193a35
Cr-Commit-Position: refs/heads/master@{#10811}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Addressed comments #Patch Set 3 : Added unit test #Patch Set 4 : Cleanup #
Total comments: 6
Patch Set 5 : Updated fake encoder fix #Patch Set 6 : Removed magic const #
Total comments: 4
Patch Set 7 : Added a todo comment #
Total comments: 5
Patch Set 8 : Addressed comments #Patch Set 9 : Just one more dcheck... #Patch Set 10 : Fixed race condition #
Total comments: 2
Patch Set 11 : Added comment #
Messages
Total messages: 42 (12 generated)
sprang@webrtc.org changed reviewers: + mflodman@webrtc.org, stefan@webrtc.org
https://codereview.webrtc.org/1474533006/diff/1/webrtc/modules/pacing/paced_s... File webrtc/modules/pacing/paced_sender.cc (right): https://codereview.webrtc.org/1474533006/diff/1/webrtc/modules/pacing/paced_s... webrtc/modules/pacing/paced_sender.cc:107: UpdateQueueTime(); To me it makes more sense to call this before we add the packet. Btw, is there a risk of getting some glitch here since we use different timestamps in UpdateQueueTime and for enqueue_time_ms in Packet? https://codereview.webrtc.org/1474533006/diff/1/webrtc/modules/pacing/paced_s... webrtc/modules/pacing/paced_sender.cc:147: int64_t AverageQueueTimeMs() { Make this const... https://codereview.webrtc.org/1474533006/diff/1/webrtc/modules/pacing/paced_s... webrtc/modules/pacing/paced_sender.cc:148: UpdateQueueTime(); ... and call this separately?
Addressed comments. I'll upload a test case in a few moments. https://codereview.webrtc.org/1474533006/diff/1/webrtc/modules/pacing/paced_s... File webrtc/modules/pacing/paced_sender.cc (right): https://codereview.webrtc.org/1474533006/diff/1/webrtc/modules/pacing/paced_s... webrtc/modules/pacing/paced_sender.cc:107: UpdateQueueTime(); On 2015/11/25 15:28:52, stefan-webrtc (holmer) wrote: > To me it makes more sense to call this before we add the packet. Yes, I realized this just after uploading... > Btw, is there a > risk of getting some glitch here since we use different timestamps in > UpdateQueueTime and for enqueue_time_ms in Packet? Yes, good catch. Updating to use packet.enqueue_time instead. https://codereview.webrtc.org/1474533006/diff/1/webrtc/modules/pacing/paced_s... webrtc/modules/pacing/paced_sender.cc:147: int64_t AverageQueueTimeMs() { On 2015/11/25 15:28:52, stefan-webrtc (holmer) wrote: > Make this const... Done. https://codereview.webrtc.org/1474533006/diff/1/webrtc/modules/pacing/paced_s... webrtc/modules/pacing/paced_sender.cc:148: UpdateQueueTime(); On 2015/11/25 15:28:52, stefan-webrtc (holmer) wrote: > ... and call this separately? Done.
https://codereview.webrtc.org/1474533006/diff/60001/webrtc/modules/pacing/pac... File webrtc/modules/pacing/paced_sender.cc (right): https://codereview.webrtc.org/1474533006/diff/60001/webrtc/modules/pacing/pac... webrtc/modules/pacing/paced_sender.cc:328: int64_t PacedSender::AverageQueueTimeMs() const { Can this really be const if we call UpdateQueueTime below? https://codereview.webrtc.org/1474533006/diff/60001/webrtc/test/fake_encoder.cc File webrtc/test/fake_encoder.cc (right): https://codereview.webrtc.org/1474533006/diff/60001/webrtc/test/fake_encoder.... webrtc/test/fake_encoder.cc:53: if (time_since_last_encode_ms > 1000 / config_.maxFramerate) { I don't think this does much... :) You probably want this at line 65 or 64.
https://codereview.webrtc.org/1474533006/diff/60001/webrtc/modules/pacing/pac... File webrtc/modules/pacing/paced_sender.cc (right): https://codereview.webrtc.org/1474533006/diff/60001/webrtc/modules/pacing/pac... webrtc/modules/pacing/paced_sender.cc:328: int64_t PacedSender::AverageQueueTimeMs() const { On 2015/11/25 18:14:22, stefan-webrtc (holmer) wrote: > Can this really be const if we call UpdateQueueTime below? Ehh, yeah.. What am I missing? Why does this compile?! https://codereview.webrtc.org/1474533006/diff/60001/webrtc/test/fake_encoder.cc File webrtc/test/fake_encoder.cc (right): https://codereview.webrtc.org/1474533006/diff/60001/webrtc/test/fake_encoder.... webrtc/test/fake_encoder.cc:53: if (time_since_last_encode_ms > 1000 / config_.maxFramerate) { On 2015/11/25 18:14:22, stefan-webrtc (holmer) wrote: > I don't think this does much... :) > > You probably want this at line 65 or 64. Oops. Done.
https://codereview.webrtc.org/1474533006/diff/60001/webrtc/modules/pacing/pac... File webrtc/modules/pacing/paced_sender.cc (right): https://codereview.webrtc.org/1474533006/diff/60001/webrtc/modules/pacing/pac... webrtc/modules/pacing/paced_sender.cc:328: int64_t PacedSender::AverageQueueTimeMs() const { On 2015/11/25 19:40:42, språng wrote: > On 2015/11/25 18:14:22, stefan-webrtc (holmer) wrote: > > Can this really be const if we call UpdateQueueTime below? > > Ehh, yeah.. What am I missing? Why does this compile?! Did you figure out why it works? Should we remove the const?
https://codereview.webrtc.org/1474533006/diff/60001/webrtc/modules/pacing/pac... File webrtc/modules/pacing/paced_sender.cc (right): https://codereview.webrtc.org/1474533006/diff/60001/webrtc/modules/pacing/pac... webrtc/modules/pacing/paced_sender.cc:328: int64_t PacedSender::AverageQueueTimeMs() const { On 2015/11/26 08:02:46, stefan-webrtc (holmer) wrote: > On 2015/11/25 19:40:42, språng wrote: > > On 2015/11/25 18:14:22, stefan-webrtc (holmer) wrote: > > > Can this really be const if we call UpdateQueueTime below? > > > > Ehh, yeah.. What am I missing? Why does this compile?! > > Did you figure out why it works? Should we remove the const? Did not figure out why it works. Removed it anyways, cause is scares me.
lgtm
https://codereview.webrtc.org/1474533006/diff/100001/webrtc/modules/pacing/pa... File webrtc/modules/pacing/paced_sender.h (right): https://codereview.webrtc.org/1474533006/diff/100001/webrtc/modules/pacing/pa... webrtc/modules/pacing/paced_sender.h:118: virtual int64_t AverageQueueTimeMs(); Hm, btw, this is just for testing, right? I'm not a big fan of exposing methods for that reason... Is it possible to write the test in some other way?
https://codereview.webrtc.org/1474533006/diff/100001/webrtc/modules/pacing/pa... File webrtc/modules/pacing/paced_sender.h (right): https://codereview.webrtc.org/1474533006/diff/100001/webrtc/modules/pacing/pa... webrtc/modules/pacing/paced_sender.h:118: virtual int64_t AverageQueueTimeMs(); On 2015/11/26 08:26:50, stefan-webrtc (holmer) wrote: > Hm, btw, this is just for testing, right? I'm not a big fan of exposing methods > for that reason... Is it possible to write the test in some other way? Theoretically, but it'd become hugely convoluted. You'd have to try to trigger overshooting and calculate backwards from the amount of packets send to actual bitrate sent and then work out the estimated average queue time from that. Didn't quite seem sensible to me. Average queue time might possibly also be a nice to have, as a uma stats for instance? But I agree. I think we should probably move the pacer into a subdirectory and split it up pacer, packet queue and media budget to own files, so we can more easily test those separately. Preferrably as a follow-up cl then. :) wdyt?
lgtm, again. https://codereview.webrtc.org/1474533006/diff/100001/webrtc/modules/pacing/pa... File webrtc/modules/pacing/paced_sender.h (right): https://codereview.webrtc.org/1474533006/diff/100001/webrtc/modules/pacing/pa... webrtc/modules/pacing/paced_sender.h:118: virtual int64_t AverageQueueTimeMs(); On 2015/11/26 08:55:09, språng wrote: > On 2015/11/26 08:26:50, stefan-webrtc (holmer) wrote: > > Hm, btw, this is just for testing, right? I'm not a big fan of exposing > methods > > for that reason... Is it possible to write the test in some other way? > > Theoretically, but it'd become hugely convoluted. You'd have to try to trigger > overshooting and calculate backwards from the amount of packets send to actual > bitrate sent and then work out the estimated average queue time from that. > Didn't quite seem sensible to me. > Average queue time might possibly also be a nice to have, as a uma stats for > instance? > > But I agree. I think we should probably move the pacer into a subdirectory and > split it up pacer, packet queue and media budget to own files, so we can more > easily test those separately. Preferrably as a follow-up cl then. :) > > wdyt? Yes, I think splitting this up makes a lot of sense... Maybe add that TODO above.
https://codereview.webrtc.org/1474533006/diff/100001/webrtc/modules/pacing/pa... File webrtc/modules/pacing/paced_sender.h (right): https://codereview.webrtc.org/1474533006/diff/100001/webrtc/modules/pacing/pa... webrtc/modules/pacing/paced_sender.h:118: virtual int64_t AverageQueueTimeMs(); On 2015/11/26 09:15:22, stefan-webrtc (holmer) wrote: > On 2015/11/26 08:55:09, språng wrote: > > On 2015/11/26 08:26:50, stefan-webrtc (holmer) wrote: > > > Hm, btw, this is just for testing, right? I'm not a big fan of exposing > > methods > > > for that reason... Is it possible to write the test in some other way? > > > > Theoretically, but it'd become hugely convoluted. You'd have to try to trigger > > overshooting and calculate backwards from the amount of packets send to actual > > bitrate sent and then work out the estimated average queue time from that. > > Didn't quite seem sensible to me. > > Average queue time might possibly also be a nice to have, as a uma stats for > > instance? > > > > But I agree. I think we should probably move the pacer into a subdirectory and > > split it up pacer, packet queue and media budget to own files, so we can more > > easily test those separately. Preferrably as a follow-up cl then. :) > > > > wdyt? > > Yes, I think splitting this up makes a lot of sense... Maybe add that TODO > above. Done.
The CQ bit was checked by sprang@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/1474533006/#ps120001 (title: "Added a todo comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1474533006/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1474533006/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/1995)
https://codereview.webrtc.org/1474533006/diff/120001/webrtc/test/fake_encoder.cc File webrtc/test/fake_encoder.cc (right): https://codereview.webrtc.org/1474533006/diff/120001/webrtc/test/fake_encoder... webrtc/test/fake_encoder.cc:63: time_since_last_encode_ms = 3 * 1000 / config_.maxFramerate; This can still add too much, why isn't this the same as in the if statement above?
https://codereview.webrtc.org/1474533006/diff/120001/webrtc/test/fake_encoder.cc File webrtc/test/fake_encoder.cc (right): https://codereview.webrtc.org/1474533006/diff/120001/webrtc/test/fake_encoder... webrtc/test/fake_encoder.cc:63: time_since_last_encode_ms = 3 * 1000 / config_.maxFramerate; On 2015/11/26 10:23:38, mflodman wrote: > This can still add too much, why isn't this the same as in the if statement > above? Very good question :) It should be 3x in the if statement too.
LGTM with comments addressed. https://codereview.webrtc.org/1474533006/diff/120001/webrtc/modules/pacing/pa... File webrtc/modules/pacing/paced_sender.cc (right): https://codereview.webrtc.org/1474533006/diff/120001/webrtc/modules/pacing/pa... webrtc/modules/pacing/paced_sender.cc:127: } Can we add a DCHECK to verify queue_time_sum_ == 0 if packet_list_.empty(). Or is PacedSenderTest.AverageQueueTime() enough? https://codereview.webrtc.org/1474533006/diff/120001/webrtc/modules/pacing/pa... webrtc/modules/pacing/paced_sender.cc:151: return 0; Or here if it's easier logically to check here.
https://codereview.webrtc.org/1474533006/diff/120001/webrtc/test/fake_encoder.cc File webrtc/test/fake_encoder.cc (right): https://codereview.webrtc.org/1474533006/diff/120001/webrtc/test/fake_encoder... webrtc/test/fake_encoder.cc:63: time_since_last_encode_ms = 3 * 1000 / config_.maxFramerate; On 2015/11/26 11:23:38, stefan-webrtc (holmer) wrote: > On 2015/11/26 10:23:38, mflodman wrote: > > This can still add too much, why isn't this the same as in the if statement > > above? > > Very good question :) > > It should be 3x in the if statement too. Done.
The CQ bit was checked by sprang@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org, mflodman@webrtc.org Link to the patchset: https://codereview.webrtc.org/1474533006/#ps160001 (title: "Just one more dcheck...")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1474533006/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1474533006/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_asan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/10892)
PTAL again :( There was a race condition hidden in this: We used prio_queue_.size() when updating the sum of queue times. It can happen that we pop an element from the prio queue - try to send it (which temporarily releases the lock) - and then abort if sending fails, pushing the packet back again. During that gap it is possible for another packet to be added, which will update the sum using an incorrect queue size. The solution if to use packet_list_.size() instead, which isn't altered until the packet is actually removed. The reason I used the prio_queue_.size() was that std::list was allowed to have a complexity of O(n), but since this has changed with c++11 and our implementations (to my knowledge) have O(1) complexity, I'm going to ignore this potential issue. WDYT?
The CQ bit was checked by sprang@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1474533006/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1474533006/180001
Assuming we only build on compilers comply to this part of c++11, this lgtm, and I have a hard time seeing that even c++98 compilers would actually implement size() with a loop over all elements... :) https://codereview.webrtc.org/1474533006/diff/180001/webrtc/modules/pacing/pa... File webrtc/modules/pacing/paced_sender.cc (right): https://codereview.webrtc.org/1474533006/diff/180001/webrtc/modules/pacing/pa... webrtc/modules/pacing/paced_sender.cc:148: queue_time_sum_ += delta * packet_list_.size(); I think it is worth commenting on the importance of using packet_list_.size() and not prio_queue_.size()...
Assuming we only build on compilers comply to this part of c++11, this lgtm, and I have a hard time seeing that even c++98 compilers would actually implement size() with a loop over all elements... :) https://codereview.webrtc.org/1474533006/diff/180001/webrtc/modules/pacing/pa... File webrtc/modules/pacing/paced_sender.cc (right): https://codereview.webrtc.org/1474533006/diff/180001/webrtc/modules/pacing/pa... webrtc/modules/pacing/paced_sender.cc:148: queue_time_sum_ += delta * packet_list_.size(); I think it is worth commenting on the importance of using packet_list_.size() and not prio_queue_.size()...
https://codereview.webrtc.org/1474533006/diff/180001/webrtc/modules/pacing/pa... File webrtc/modules/pacing/paced_sender.cc (right): https://codereview.webrtc.org/1474533006/diff/180001/webrtc/modules/pacing/pa... webrtc/modules/pacing/paced_sender.cc:148: queue_time_sum_ += delta * packet_list_.size(); On 2015/11/26 14:18:20, stefan-webrtc (holmer) wrote: > I think it is worth commenting on the importance of using packet_list_.size() > and not prio_queue_.size()... Done.
The CQ bit was checked by sprang@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from mflodman@webrtc.org, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/1474533006/#ps190001 (title: "Added comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1474533006/190001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1474533006/190001
Description was changed from ========== Fix bug in calculation of averge queue time in paced sender. Also work around a flaw in fake encoder which caused bogus perf regression in rampup tests. BUG=560434 ========== to ========== Fix bug in calculation of averge queue time in paced sender. Also work around a flaw in fake encoder which caused bogus perf regression in rampup tests. BUG=560434 R=mflodman@webrtc.org, stefan@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/ad113e50d251c95adcf501ed2... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:190001) manually as ad113e50d251c95adcf501ed29f8312ad1193a35 (presubmit successful).
Message was sent while issue was closed.
Description was changed from ========== Fix bug in calculation of averge queue time in paced sender. Also work around a flaw in fake encoder which caused bogus perf regression in rampup tests. BUG=560434 R=mflodman@webrtc.org, stefan@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/ad113e50d251c95adcf501ed2... ========== to ========== Fix bug in calculation of averge queue time in paced sender. Also work around a flaw in fake encoder which caused bogus perf regression in rampup tests. BUG=560434 R=mflodman@webrtc.org, stefan@webrtc.org Committed: https://crrev.com/ad113e50d251c95adcf501ed29f8312ad1193a35 Cr-Commit-Position: refs/heads/master@{#10811} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/ad113e50d251c95adcf501ed29f8312ad1193a35 Cr-Commit-Position: refs/heads/master@{#10811} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
