Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2795643002:
Add methods to register congestion controller observer after construction. (Closed)
|
Created:
3 years, 8 months ago by nisse-webrtc Modified:
3 years, 8 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd methods to register congestion controller observer after construction.
The point of this change is to make it possible to create the congestion
controller as part of creating RtpTransportController, later pass it to the
constructor of Call, and then let Call register itself as an observer.
BUG=webrtc:7135
Review-Url: https://codereview.webrtc.org/2795643002
Cr-Commit-Position: refs/heads/master@{#17504}
Committed: https://chromium.googlesource.com/external/webrtc/+/23425f9068aed6ba93d947f0ace17e55f2e1247d
Patch Set 1 #
Total comments: 3
Patch Set 2 : Added TODO comment. #
Messages
Total messages: 15 (6 generated)
nisse@webrtc.org changed reviewers: + perkj@webrtc.org, terelius@webrtc.org
PTAL. The point of this change is to make it possible to create the congestion controller as part of creating RtpTransportController, later pass it to the constructor of Call, and then let Call register itself as an observer. Not entirely clear how it should work in the longer term, when Call is refactored or eliminated. But the bitrate allocator which is the target of the OnNetworkChaged callback should stay separate from RtpTransportController. Another lock is unfortunate, but I see no easy way to avoid that. At least, I think contention is unlikely.
lgtm if you add your comment "The point of this change is to make it possible to create the congestion controller as part of creating RtpTransportController, later pass it to the constructor of Call, and then let Call register itself as an observer." to the cl description. I think it also make sense to have a tracker bug with a reasonable scope for this work.
Description was changed from ========== Add methods to register congestion controller observer after construction. BUG=webrtc:7135 ========== to ========== Add methods to register congestion controller observer after construction. The point of this change is to make it possible to create the congestion controller as part of creating RtpTransportController, later pass it to the constructor of Call, and then let Call register itself as an observer. BUG=webrtc:7135 ==========
On 2017/04/03 08:53:35, perkj_webrtc wrote: > lgtm if you add your comment > > "The point of this change is to make it possible to create the congestion > controller as part of creating RtpTransportController, later pass it to the > constructor of Call, and then let Call register itself as an observer." to the > cl description. Done. > I think it also make sense to have a tracker bug with a reasonable scope for > this work. I could file an additional tracker bug, but it's not clear to me what a reasonable scope or sub-objective is.
lgtm https://codereview.webrtc.org/2795643002/diff/1/webrtc/modules/congestion_con... File webrtc/modules/congestion_controller/send_side_congestion_controller.cc (right): https://codereview.webrtc.org/2795643002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/send_side_congestion_controller.cc:49: Observer* observer, Going forward, will we deprecate passing an observer to the constructor? If so, should there be a comment explaining this?
https://codereview.webrtc.org/2795643002/diff/1/webrtc/modules/congestion_con... File webrtc/modules/congestion_controller/send_side_congestion_controller.cc (right): https://codereview.webrtc.org/2795643002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/send_side_congestion_controller.cc:49: Observer* observer, On 2017/04/03 09:48:04, terelius wrote: > Going forward, will we deprecate passing an observer to the constructor? If so, > should there be a comment explaining this? Hmm. Maybe. The CongestionController class (old all-in-one-class, both send and receive side, which we should eventually delete) has a corresponding constructor argument, which I don't think it makes sense to touch. But we could delete the argument from *this* constructor, and let the CongestionController class call RegisterNetworkObserver. What do you think?
https://codereview.webrtc.org/2795643002/diff/1/webrtc/modules/congestion_con... File webrtc/modules/congestion_controller/send_side_congestion_controller.cc (right): https://codereview.webrtc.org/2795643002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/send_side_congestion_controller.cc:49: Observer* observer, On 2017/04/03 09:54:42, nisse-webrtc wrote: > On 2017/04/03 09:48:04, terelius wrote: > > Going forward, will we deprecate passing an observer to the constructor? If > so, > > should there be a comment explaining this? > > Hmm. Maybe. > > The CongestionController class (old all-in-one-class, both send and receive > side, which we should eventually delete) has a corresponding constructor > argument, which I don't think it makes sense to touch. But we could delete the > argument from *this* constructor, and let the CongestionController class call > RegisterNetworkObserver. What do you think? Agree about not spending time on changing something that we plan to delete anyway. I don't know what interface would be most useful in SendSideCongestionController. However, if we know that we're always going to set the oberser using the new functions, then I think it would be nice to add a TODO in the constructor.
The CQ bit was checked by nisse@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from terelius@webrtc.org, perkj@webrtc.org Link to the patchset: https://codereview.webrtc.org/2795643002/#ps20001 (title: "Added TODO comment.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
On 2017/04/03 10:55:28, terelius wrote: > https://codereview.webrtc.org/2795643002/diff/1/webrtc/modules/congestion_con... > File webrtc/modules/congestion_controller/send_side_congestion_controller.cc > (right): > > https://codereview.webrtc.org/2795643002/diff/1/webrtc/modules/congestion_con... > webrtc/modules/congestion_controller/send_side_congestion_controller.cc:49: > Observer* observer, > On 2017/04/03 09:54:42, nisse-webrtc wrote: > > On 2017/04/03 09:48:04, terelius wrote: > > > Going forward, will we deprecate passing an observer to the constructor? If > > so, > > > should there be a comment explaining this? > > > > Hmm. Maybe. > > > > The CongestionController class (old all-in-one-class, both send and receive > > side, which we should eventually delete) has a corresponding constructor > > argument, which I don't think it makes sense to touch. But we could delete the > > argument from *this* constructor, and let the CongestionController class call > > RegisterNetworkObserver. What do you think? > > Agree about not spending time on changing something that we plan to delete > anyway. I don't know what interface would be most useful in > SendSideCongestionController. However, if we know that we're always going to set > the oberser using the new functions, then I think it would be nice to add a TODO > in the constructor. Added TODO comment. Attempting to land now.
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1491219063565000,
"parent_rev": "d197cd9ad94b1b9b59b6f54e19694ff9a9d3ac4e", "commit_rev":
"23425f9068aed6ba93d947f0ace17e55f2e1247d"}
Message was sent while issue was closed.
Description was changed from ========== Add methods to register congestion controller observer after construction. The point of this change is to make it possible to create the congestion controller as part of creating RtpTransportController, later pass it to the constructor of Call, and then let Call register itself as an observer. BUG=webrtc:7135 ========== to ========== Add methods to register congestion controller observer after construction. The point of this change is to make it possible to create the congestion controller as part of creating RtpTransportController, later pass it to the constructor of Call, and then let Call register itself as an observer. BUG=webrtc:7135 Review-Url: https://codereview.webrtc.org/2795643002 Cr-Commit-Position: refs/heads/master@{#17504} Committed: https://chromium.googlesource.com/external/webrtc/+/23425f9068aed6ba93d947f0a... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/external/webrtc/+/23425f9068aed6ba93d947f0a... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
