Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2243823002:
Added ProbeBitrate(bitrate_bps, num_probes) to BitrateProber. (Closed)
|
Created:
4 years, 4 months ago by philipel Modified:
4 years, 4 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, the sun, zhuangzesen_agora.io, stefan-webrtc, mflodman, Irfan Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdded ProbeBitrate(bitrate_bps, num_probes) to BitrateProber.
Also some minor cleanup.
BUG=webrtc:5859
Committed: https://crrev.com/4a1ec1e639b0e78ca1f189b19205782c4b9ab0af
Cr-Commit-Position: refs/heads/master@{#13760}
Patch Set 1 #Patch Set 2 : Clean up api #
Total comments: 13
Patch Set 3 : Feedback fixes. #
Total comments: 2
Patch Set 4 : Don't modify container while iterating over it. #Patch Set 5 : include for size_t #Patch Set 6 : ... in header file. #
Messages
Total messages: 23 (9 generated)
philipel@webrtc.org changed reviewers: + danilchap@webrtc.org, stefan@webrtc.org
https://codereview.webrtc.org/2243823002/diff/20001/webrtc/modules/pacing/bit... File webrtc/modules/pacing/bitrate_prober.cc (right): https://codereview.webrtc.org/2243823002/diff/20001/webrtc/modules/pacing/bit... webrtc/modules/pacing/bitrate_prober.cc:58: void BitrateProber::OnIncomingPacket(size_t packet_size) { Instead of creating clusters and trigger probing (setting state to active) we now use ProbeBitrate/2 to first setup clusters and then this function to trigger when we have large enough packets. https://codereview.webrtc.org/2243823002/diff/20001/webrtc/modules/pacing/bit... webrtc/modules/pacing/bitrate_prober.cc:88: } In the unittest BitrateProberTest.DoesntProbeWithoutRecentPackets we test that we retry the probing if there is a too large gap in between sent packets.
https://codereview.webrtc.org/2243823002/diff/20001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2243823002/diff/20001/webrtc/call/call.cc#newco... webrtc/call/call.cc:597: return; Is there a reason not to DCHECK? https://codereview.webrtc.org/2243823002/diff/20001/webrtc/modules/pacing/bit... File webrtc/modules/pacing/bitrate_prober.cc (right): https://codereview.webrtc.org/2243823002/diff/20001/webrtc/modules/pacing/bit... webrtc/modules/pacing/bitrate_prober.cc:84: // Reset all queued probing clusters. What does resetting mean? It seems odd to me that we keep the same number of clusters, but set new ids. Why not simply removing them and have the user create new probes if he wants? https://codereview.webrtc.org/2243823002/diff/20001/webrtc/modules/pacing/bit... webrtc/modules/pacing/bitrate_prober.cc:88: } On 2016/08/12 12:53:44, philipel wrote: > In the unittest BitrateProberTest.DoesntProbeWithoutRecentPackets we test that > we retry the probing if there is a too large gap in between sent packets. If this is the answer to my question above, then maybe we at least need to comment on it. https://codereview.webrtc.org/2243823002/diff/20001/webrtc/modules/pacing/bit... File webrtc/modules/pacing/bitrate_prober.h (right): https://codereview.webrtc.org/2243823002/diff/20001/webrtc/modules/pacing/bit... webrtc/modules/pacing/bitrate_prober.h:40: void ProbeBitrate(uint32_t bitrate_bps, int num_packets); ProbeAtBitrate or CreateProbeAtBitrate?
https://codereview.webrtc.org/2243823002/diff/20001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2243823002/diff/20001/webrtc/call/call.cc#newco... webrtc/call/call.cc:597: return; On 2016/08/12 13:19:58, stefan-webrtc (holmer) wrote: > Is there a reason not to DCHECK? Thought this might be user input, but i haven't looked into it completely so I'll undo this change. https://codereview.webrtc.org/2243823002/diff/20001/webrtc/modules/pacing/bit... File webrtc/modules/pacing/bitrate_prober.cc (right): https://codereview.webrtc.org/2243823002/diff/20001/webrtc/modules/pacing/bit... webrtc/modules/pacing/bitrate_prober.cc:84: // Reset all queued probing clusters. On 2016/08/12 13:19:58, stefan-webrtc (holmer) wrote: > What does resetting mean? It seems odd to me that we keep the same number of > clusters, but set new ids. Why not simply removing them and have the user create > new probes if he wants? Now use ProbeAtBitrate instead to recreate the probing clusters. https://codereview.webrtc.org/2243823002/diff/20001/webrtc/modules/pacing/bit... webrtc/modules/pacing/bitrate_prober.cc:88: } On 2016/08/12 13:19:58, stefan-webrtc (holmer) wrote: > On 2016/08/12 12:53:44, philipel wrote: > > In the unittest BitrateProberTest.DoesntProbeWithoutRecentPackets we test that > > we retry the probing if there is a too large gap in between sent packets. > > If this is the answer to my question above, then maybe we at least need to > comment on it. Yes, the current behavior is to retry probing if we reset. https://codereview.webrtc.org/2243823002/diff/20001/webrtc/modules/pacing/bit... File webrtc/modules/pacing/bitrate_prober.h (right): https://codereview.webrtc.org/2243823002/diff/20001/webrtc/modules/pacing/bit... webrtc/modules/pacing/bitrate_prober.h:40: void ProbeBitrate(uint32_t bitrate_bps, int num_packets); On 2016/08/12 13:19:59, stefan-webrtc (holmer) wrote: > ProbeAtBitrate or CreateProbeAtBitrate? Change to ProbeAtBitrate.
isheriff@chromium.org changed reviewers: + isheriff@chromium.org
Thanks for CCing on this change. I think the biggest change I make for expoential probing is to track the state and initiation explicitly and I should be able to rebase and adapt the change on top of this.
Thanks for CCing on this change. I think the biggest change I make for expoential probing is to track the state and initiation explicitly and I should be able to rebase and adapt the change on top of this.
https://codereview.webrtc.org/2243823002/diff/20001/webrtc/modules/pacing/bit... File webrtc/modules/pacing/bitrate_prober.cc (right): https://codereview.webrtc.org/2243823002/diff/20001/webrtc/modules/pacing/bit... webrtc/modules/pacing/bitrate_prober.cc:76: if (probing_state_ != ProbingState::kActive) wouldn't that activate probing even if it is in ProbingState::kDisable state? The comment in header file state probing wouldn't be activate from Disable state, only from Suspended. Can you ensure comments are behavior match. https://codereview.webrtc.org/2243823002/diff/20001/webrtc/modules/pacing/bit... File webrtc/modules/pacing/bitrate_prober.h (right): https://codereview.webrtc.org/2243823002/diff/20001/webrtc/modules/pacing/bit... webrtc/modules/pacing/bitrate_prober.h:26: void SetEnabled(bool enable); Is this function still in use? Seems dangerous to keep it around since it's behaviour silently changed: it no longer works without new ProbeBitrate function.
https://codereview.chromium.org/2243823002/diff/20001/webrtc/modules/pacing/b... File webrtc/modules/pacing/bitrate_prober.cc (right): https://codereview.chromium.org/2243823002/diff/20001/webrtc/modules/pacing/b... webrtc/modules/pacing/bitrate_prober.cc:76: if (probing_state_ != ProbingState::kActive) On 2016/08/12 15:13:34, danilchap wrote: > wouldn't that activate probing even if it is in ProbingState::kDisable state? > The comment in header file state probing wouldn't be activate from Disable > state, only from Suspended. > Can you ensure comments are behavior match. There should probably be an RTC_DCHECK() that ensures this function does not get called in disabled state. With exponential probing, where we track the state when probing needs to be initiated at paced_sender, I do refactor this to have control over when probing starts.
https://codereview.webrtc.org/2243823002/diff/40001/webrtc/modules/pacing/bit... File webrtc/modules/pacing/bitrate_prober.cc (right): https://codereview.webrtc.org/2243823002/diff/40001/webrtc/modules/pacing/bit... webrtc/modules/pacing/bitrate_prober.cc:89: ProbeAtBitrate(cluster.probe_bitrate_bps, cluster.max_probe_packets); Avoid implicitly modifying container you are iterating on. While this code looks technically correct, it is very easy to slightly adjust it to make it wrong. Consider clearing container first: std::queue<ProbeCluster> clusters; using std::swap; swap(clusters, clusters_); for (const auto& cluster : clusters) { ProbeAtBitrate(cluster.probe_bitrate_bps, cluster.max_probe_packets); } or add a helper function ResetCluster(ProbleCluster*) to keep old code where container is not recreated.
https://codereview.webrtc.org/2243823002/diff/40001/webrtc/modules/pacing/bit... File webrtc/modules/pacing/bitrate_prober.cc (right): https://codereview.webrtc.org/2243823002/diff/40001/webrtc/modules/pacing/bit... webrtc/modules/pacing/bitrate_prober.cc:89: ProbeAtBitrate(cluster.probe_bitrate_bps, cluster.max_probe_packets); On 2016/08/12 15:30:26, danilchap wrote: > Avoid implicitly modifying container you are iterating on. > While this code looks technically correct, it is very easy to slightly adjust it > to make it wrong. > Consider clearing container first: > std::queue<ProbeCluster> clusters; > using std::swap; > swap(clusters, clusters_); > for (const auto& cluster : clusters) { > ProbeAtBitrate(cluster.probe_bitrate_bps, cluster.max_probe_packets); > } > > or add a helper function ResetCluster(ProbleCluster*) to keep old code where > container is not recreated. Good point, now creats a new queue and then swap it.
lgtm
The CQ bit was checked by philipel@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_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_baremetal/builds/...)
The CQ bit was checked by philipel@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/2243823002/#ps100001 (title: "... in header file.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Added ProbeBitrate(bitrate_bps, num_probes) to BitrateProber. Also some minor cleanup. BUG=webrtc:5859 ========== to ========== Added ProbeBitrate(bitrate_bps, num_probes) to BitrateProber. Also some minor cleanup. BUG=webrtc:5859 Committed: https://crrev.com/4a1ec1e639b0e78ca1f189b19205782c4b9ab0af Cr-Commit-Position: refs/heads/master@{#13760} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/4a1ec1e639b0e78ca1f189b19205782c4b9ab0af Cr-Commit-Position: refs/heads/master@{#13760} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
