Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
3 years, 11 months ago by elad.alon Modified:
3 years, 9 months ago Reviewers:
ossu, michaelt, sprang_webrtc, stefan-webrtc, terelius, minyue-webrtc, the sun CC:
webrtc-reviews_webrtc.org, danilchap, zhuangzesen_agora.io, Andrew MacDonald, henrika_webrtc, stefan-webrtc, tlegrand-webrtc, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, peah-webrtc, mflodman Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAttach TransportFeedbackPacketLossTracker to ANA (PLR only)
This CL is one in a series. To finish the work, the following CLs will be added:
1. CL for connecting RPLR as well
2. CL for RPLR-based FecController
3. CL for allowing experiment-driven configuration of the above (through both field-trials and protobuf)
BUG=webrtc:7058
Review-Url: https://codereview.webrtc.org/2638083002
Cr-Commit-Position: refs/heads/master@{#17365}
Committed: https://chromium.googlesource.com/external/webrtc/+/d12a8e1c8eff2f721da702b4d22c44d7d7b1a13a
Patch Set 1 #Patch Set 2 : Rebased #Patch Set 3 : nit #Patch Set 4 : Fix UT #
Total comments: 6
Patch Set 5 : Rebased #
Total comments: 7
Patch Set 6 : TODO replaced by task (7173). #
Total comments: 12
Patch Set 7 : Rebased #Patch Set 8 : event_log_visualizer #
Total comments: 12
Patch Set 9 : 1. Rebased. 2. Move tracker ownership. #Patch Set 10 : . #Patch Set 11 : SSRC() #
Total comments: 14
Patch Set 12 : Rebased #
Total comments: 2
Patch Set 13 : Rebased #Patch Set 14 : Rebased #
Total comments: 5
Patch Set 15 : Check added #
Total comments: 9
Patch Set 16 : TODO shuffle #
Total comments: 2
Patch Set 17 : . #Patch Set 18 : 1. Rebased. 2. Observer UT added. #
Total comments: 4
Patch Set 19 : DeathTest fix. #
Total comments: 4
Patch Set 20 : CR response #
Total comments: 49
Patch Set 21 : CR response #Patch Set 22 : git cl format :-) #Patch Set 23 : Style nit #Patch Set 24 : #include <vector> #
Total comments: 2
Patch Set 25 : Rebased (and minor changes to comments) #Patch Set 26 : Fix UT #
Total comments: 2
Created: 3 years, 9 months ago
Messages
Total messages: 81 (26 generated)
elad.alon@webrtc.org changed reviewers: + michaelt@webrtc.org, minyue@webrtc.org, solenberg@webrtc.org, sprang@webrtc.org, stefan@webrtc.org
Description was changed from ========== Attach TransportFeedbackPacketLossTracker to ANA ========== to ========== Attach TransportFeedbackPacketLossTracker to ANA BUG=webrtc:7058 ==========
Description was changed from ========== Attach TransportFeedbackPacketLossTracker to ANA BUG=webrtc:7058 ========== to ========== Attach TransportFeedbackPacketLossTracker to ANA (PLR only) This CL is one in a series. To finish the work, the following CLs will be added: 1. CL for connecting RPLR as well 2. CL for RPLR-based FecController 3. CLR for allowing experiment-driven configuration of the above (through both field-trials and protobuf) BUG=webrtc:7058 ==========
Review, please? :-) Please note the description, which explains the upcoming CLs in the series - but itself, this CL would not have an effect, as a hard-coded boolean blocks it. In the upcoming CLs, more functionality would be introduced, then experimental configuration that would replace the hard-coded block and some hard-coded values.
Description was changed from ========== Attach TransportFeedbackPacketLossTracker to ANA (PLR only) This CL is one in a series. To finish the work, the following CLs will be added: 1. CL for connecting RPLR as well 2. CL for RPLR-based FecController 3. CLR for allowing experiment-driven configuration of the above (through both field-trials and protobuf) BUG=webrtc:7058 ========== to ========== Attach TransportFeedbackPacketLossTracker to ANA (PLR only) This CL is one in a series. To finish the work, the following CLs will be added: 1. CL for connecting RPLR as well 2. CL for RPLR-based FecController 3. CL for allowing experiment-driven configuration of the above (through both field-trials and protobuf) BUG=webrtc:7058 ==========
https://codereview.webrtc.org/2638083002/diff/60001/webrtc/audio/audio_send_s... File webrtc/audio/audio_send_stream.h (right): https://codereview.webrtc.org/2638083002/diff/60001/webrtc/audio/audio_send_s... webrtc/audio/audio_send_stream.h:89: // TODO(elad.alon): How come the GUARDED_BY has no effect here? how do you mean? https://codereview.webrtc.org/2638083002/diff/60001/webrtc/voice_engine/chann... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2638083002/diff/60001/webrtc/voice_engine/chann... webrtc/voice_engine/channel.cc:947: use_twcc_plr_for_ana_(false), This is always false? https://codereview.webrtc.org/2638083002/diff/60001/webrtc/voice_engine/chann... webrtc/voice_engine/channel.cc:2927: packet_loss_tracker_.OnPacketAdded(sent_sequence_number); Will this really work? Maybe I'm missing something, but it looks to me like you are filtering out the transport wide sequence numbers/packed id for the packets belonging to the ssrc associated with this channel. But they will then contain "gaps" for packets sent on other ssrcs. Won't they then be interpreted as packet losses by the tracker?
https://codereview.webrtc.org/2638083002/diff/60001/webrtc/audio/audio_send_s... File webrtc/audio/audio_send_stream.h (right): https://codereview.webrtc.org/2638083002/diff/60001/webrtc/audio/audio_send_s... webrtc/audio/audio_send_stream.h:89: // TODO(elad.alon): How come the GUARDED_BY has no effect here? On 2017/02/03 11:57:04, språng wrote: > how do you mean? The compiler does not complain - though it should - when |packets_sent_since_last_feedback_| is accessed from a context which does not hold |packets_sent_since_last_feedback_cs_|. Probably something about the BUILD.gn files of this module? https://codereview.webrtc.org/2638083002/diff/60001/webrtc/voice_engine/chann... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2638083002/diff/60001/webrtc/voice_engine/chann... webrtc/voice_engine/channel.cc:947: use_twcc_plr_for_ana_(false), On 2017/02/03 11:57:04, språng wrote: > This is always false? For now, yes. This is one CL in a series; one of the next ones (probably the last one) will make this configuration experiment-driven. (Please note the TODO above this line.) https://codereview.webrtc.org/2638083002/diff/60001/webrtc/voice_engine/chann... webrtc/voice_engine/channel.cc:2927: packet_loss_tracker_.OnPacketAdded(sent_sequence_number); On 2017/02/03 11:57:04, språng wrote: > Will this really work? > Maybe I'm missing something, but it looks to me like you are filtering out the > transport wide sequence numbers/packed id for the packets belonging to the ssrc > associated with this channel. But they will then contain "gaps" for packets sent > on other ssrcs. Won't they then be interpreted as packet losses by the tracker? You've reviewed 2629883003, but skipped 2632203002 - that's what you're missing. :-) (Would it be helpful to give the stacked CLs sequence numbers, perhaps?)
https://codereview.webrtc.org/2638083002/diff/80001/webrtc/audio/audio_send_s... File webrtc/audio/audio_send_stream.cc (left): https://codereview.webrtc.org/2638083002/diff/80001/webrtc/audio/audio_send_s... webrtc/audio/audio_send_stream.cc:255: Woudnt it be simpler to let AudioSendStream own packet_loss_tracker_ and the just pass down the pl and plr value ? https://codereview.webrtc.org/2638083002/diff/80001/webrtc/audio/audio_send_s... File webrtc/audio/audio_send_stream.h (right): https://codereview.webrtc.org/2638083002/diff/80001/webrtc/audio/audio_send_s... webrtc/audio/audio_send_stream.h:89: // TODO(elad.alon): How come the GUARDED_BY has no effect here? Works fine for me. So i think we should remove this comment.
https://codereview.webrtc.org/2638083002/diff/80001/webrtc/audio/audio_send_s... File webrtc/audio/audio_send_stream.h (right): https://codereview.webrtc.org/2638083002/diff/80001/webrtc/audio/audio_send_s... webrtc/audio/audio_send_stream.h:89: // TODO(elad.alon): How come the GUARDED_BY has no effect here? On 2017/02/09 13:29:00, michaelt wrote: > Works fine for me. > So i think we should remove this comment. Offline discussion - both Michael and I encounter the same issue - building for iOS/Android encounters the problem (GUARDED_BY has no effect); building for Mac (UT/App) exhibits no issue. I'll open an issue for this, and remove the TODO.
solenberg@webrtc.org changed reviewers: + ossu@webrtc.org - solenberg@webrtc.org
https://codereview.webrtc.org/2638083002/diff/80001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/transport_feedback_adapter.cc (right): https://codereview.webrtc.org/2638083002/diff/80001/webrtc/modules/congestion... webrtc/modules/congestion_controller/transport_feedback_adapter.cc:88: send_time_history_.OnSentPacket(sequence_number, send_time_ms); why not adding observer here?
https://codereview.webrtc.org/2638083002/diff/80001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/transport_feedback_adapter.cc (right): https://codereview.webrtc.org/2638083002/diff/80001/webrtc/modules/congestion... webrtc/modules/congestion_controller/transport_feedback_adapter.cc:88: send_time_history_.OnSentPacket(sequence_number, send_time_ms); On 2017/02/16 09:59:09, minyue-webrtc wrote: > why not adding observer here? Because OnSentPacket() is called after the packet has been sent to the network, which theoretically gives us a race condition with the feedback from the other side.
On 2017/02/16 17:02:10, elad.alon wrote: > https://codereview.webrtc.org/2638083002/diff/80001/webrtc/modules/congestion... > File webrtc/modules/congestion_controller/transport_feedback_adapter.cc (right): > > https://codereview.webrtc.org/2638083002/diff/80001/webrtc/modules/congestion... > webrtc/modules/congestion_controller/transport_feedback_adapter.cc:88: > send_time_history_.OnSentPacket(sequence_number, send_time_ms); > On 2017/02/16 09:59:09, minyue-webrtc wrote: > > why not adding observer here? > > Because OnSentPacket() is called after the packet has been sent to the network, > which theoretically gives us a race condition with the feedback from the other > side. I feel that OnSentPacket() is more correct to use. 1) Is everything put to AddPaceket will be sent? Of course, this might be the case, but if we have a more direct info of packets being sent, why not going for that instead. 2) More importantly, we care about when the packet is sent to calculate packet loss rate. and which send timestamp is given as an argument of OnSentPacket(). So simple. If you agree, we can try to fix (or intentionally won't fix) the data race. 1) Feedbacks come before the sent packets is registered a sent packet is extremely rare. So I don't even think we should care. 2) There should be a lock. And OnSentPacket should certainly acquires the lock before the feedback comes.
On 2017/02/17 10:11:44, minyue-webrtc wrote: > On 2017/02/16 17:02:10, elad.alon wrote: > > > https://codereview.webrtc.org/2638083002/diff/80001/webrtc/modules/congestion... > > File webrtc/modules/congestion_controller/transport_feedback_adapter.cc > (right): > > > > > https://codereview.webrtc.org/2638083002/diff/80001/webrtc/modules/congestion... > > webrtc/modules/congestion_controller/transport_feedback_adapter.cc:88: > > send_time_history_.OnSentPacket(sequence_number, send_time_ms); > > On 2017/02/16 09:59:09, minyue-webrtc wrote: > > > why not adding observer here? > > > > Because OnSentPacket() is called after the packet has been sent to the > network, > > which theoretically gives us a race condition with the feedback from the other > > side. > > I feel that OnSentPacket() is more correct to use. > 1) Is everything put to AddPaceket will be sent? Of course, this might be the > case, but if we have a more direct info of packets being sent, why not going for > that instead. > 2) More importantly, we care about when the packet is sent to calculate packet > loss rate. and which send timestamp is given as an argument of OnSentPacket(). > So simple. > > If you agree, we can try to fix (or intentionally won't fix) the data race. > 1) Feedbacks come before the sent packets is registered a sent packet is > extremely rare. So I don't even think we should care. > 2) There should be a lock. And OnSentPacket should certainly acquires the lock > before the feedback comes. 1. OnSentPacket() is called on the pacer's thread. Namely, from RTPSender::PrepareAndSendPacket(). So it's called right before sending to the network, which is exactly when we'd like to check the time. 2. At that point, it shouldn't really be possible for the packet to not be sent (modulo errors). Note that even if somehow it won't be sent, it wouldn't really affect us (except for very corner cases [we can discuss offline], where it would still be correct behavior).
I think the most important consideration here is the design: Current solution sounds to re-invent a TransportFeedbackObserver, with the name of TransportFeedbackAdaptorObserver, but IMO is supposed to the same thing: "AddPacket to RTP_sender and OnTransportFeedback to RTCP_receiver." In addition, I do not see a point to go through congestion controller. The TWCC packet loss tracker can stay by itself. Since we essentially hook more than one TransportFeedbackObserver to the RTP RTCP module, someone has to do a broadcasting. RTP RTCP may be one of them. I think it would be good to do it in VoE. Simplest, we can let TransportFeedbackProxy hold a list of TransportFeedbackObserver(s). One come from the congestion controller, and the other is the TWCC packet loss tracker. https://codereview.webrtc.org/2638083002/diff/100001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2638083002/diff/100001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:271: // Prevent unbounded memory consumption if OnTransportFeedback ends up why should we buffer sent packets? since the packet loss tracker buffers it cleverly, this buffering here looks redundant. https://codereview.webrtc.org/2638083002/diff/100001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/transport_feedback_adapter.cc (right): https://codereview.webrtc.org/2638083002/diff/100001/webrtc/modules/congestio... webrtc/modules/congestion_controller/transport_feedback_adapter.cc:116: if (std::find(observers_.begin(), I would the sanity check be debug only: RTC_DCHECK_NE(observers_.end(), observers_.find(...)) https://codereview.webrtc.org/2638083002/diff/100001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h (right): https://codereview.webrtc.org/2638083002/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h:317: class TransportFeedbackAdapterObserver { Why cannot this be a TransportFeedbackObserver? See more detailed discussion on this. https://codereview.webrtc.org/2638083002/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h:319: TransportFeedbackAdapterObserver() = default; no ctor needed
TransportFeedbackProxy doesn't seem to me like the right place to me, because it's an object that only exists to solve an order-of-creation problem, and since that problem is about to be resolved soon (according to my discussion with Stefan), that object would go away, too. Once TransportFeedbackProxy is removed, TransportFeedbackObserver will be removed too (CongestionController will be passed in all its full glory where it is now being passed as that type of observer), and then we can reuse the TransportFeedbackObserver name for our own observer. Also, offline discussions have led Stefan and me to agree that delay_based_bwe_ should for now remain inside of the CongestionController, and be given special treatment, rather than be registered as an observer of the adapter. My apologies that I could not summarize all of the significant discussion which has gone into the topic. Please ping me for offline discussion if any more clarifications are needed. https://codereview.webrtc.org/2638083002/diff/100001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2638083002/diff/100001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:271: // Prevent unbounded memory consumption if OnTransportFeedback ends up On 2017/02/22 11:47:55, minyue-webrtc(OOO_until_Mar19) wrote: > why should we buffer sent packets? since the packet loss tracker buffers it > cleverly, this buffering here looks redundant. Please see the previous comment (lines 266-268 on PS6, but might change when I upload PS7). We want everything to happen on the same thread, so we don't have to acquire locks along the route from AudioSendStream to the tracker. https://codereview.webrtc.org/2638083002/diff/100001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:302: channel_proxy_->HandleTransportFeedback( FYI, I've also considered doing this with two separate functions called one after the other, but this seems better to me. I'm open to discussion, though. https://codereview.webrtc.org/2638083002/diff/100001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/transport_feedback_adapter.cc (right): https://codereview.webrtc.org/2638083002/diff/100001/webrtc/modules/congestio... webrtc/modules/congestion_controller/transport_feedback_adapter.cc:116: if (std::find(observers_.begin(), On 2017/02/22 11:47:55, minyue-webrtc(OOO_until_Mar19) wrote: > I would the sanity check be debug only: > > RTC_DCHECK_NE(observers_.end(), observers_.find(...)) Are you sure? Observer registration is not an often-occurring event, so the inefficiency of searching through the handful of observers seems worth the extra safety we get here. Wdyt? https://codereview.webrtc.org/2638083002/diff/100001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h (right): https://codereview.webrtc.org/2638083002/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h:317: class TransportFeedbackAdapterObserver { On 2017/02/22 11:47:55, minyue-webrtc(OOO_until_Mar19) wrote: > Why cannot this be a TransportFeedbackObserver? See more detailed discussion on > this. (For posterity - some offline discussions were made, and this seems like the best approach for now. A TODO in the code introduced in PS7 sheds some light on the approach, though not on all of the reasons for it.) https://codereview.webrtc.org/2638083002/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h:319: TransportFeedbackAdapterObserver() = default; On 2017/02/22 11:47:55, minyue-webrtc(OOO_until_Mar19) wrote: > no ctor needed Done.
https://codereview.webrtc.org/2638083002/diff/80001/webrtc/audio/audio_send_s... File webrtc/audio/audio_send_stream.cc (left): https://codereview.webrtc.org/2638083002/diff/80001/webrtc/audio/audio_send_s... webrtc/audio/audio_send_stream.cc:255: Is this still under consideration ?
https://codereview.webrtc.org/2638083002/diff/80001/webrtc/audio/audio_send_s... File webrtc/audio/audio_send_stream.cc (left): https://codereview.webrtc.org/2638083002/diff/80001/webrtc/audio/audio_send_s... webrtc/audio/audio_send_stream.cc:255: On 2017/03/13 09:57:43, michaelt wrote: > Is this still under consideration ? > I would prefer that too, but, IIRC it was Minyue who wanted the ownership of the tracker to be in Channel, to keep it close to ModifyEncoder(). It's been a while since this decision was made, so I might be wrong. Minyue?
A few comments for now https://codereview.webrtc.org/2638083002/diff/140001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2638083002/diff/140001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:264: // which is called on the thread which channel_proxy_ mostly works on. second "which" -> that https://codereview.webrtc.org/2638083002/diff/140001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:267: // Prevent unbounded memory consumption if OnTransportFeedback ends up I think this may be better taken care inside the packet loss tracker https://codereview.webrtc.org/2638083002/diff/140001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.h (right): https://codereview.webrtc.org/2638083002/diff/140001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.h:92: std::vector<SentTransportPacketRecord> packets_sent_since_last_feedback_ I don't understand why we need this buffer given that the packet loss tracker has its internal buffer. https://codereview.webrtc.org/2638083002/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h (right): https://codereview.webrtc.org/2638083002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h:315: struct SentTransportPacketRecord { I don't see the need of this struct, because first of all, the buffering of SentTransportPacketRecord isn't necessary https://codereview.webrtc.org/2638083002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h:320: // TODO(elad.alon): The only implementers of this interface are I think this comment should be placed above TransportFeedbackAdapterObserver, and reword it a bit, starting with "TODO(elad.alon): Merge TransportFeedbackAdapterObserver into TransportFeedbackObserver when xxx." add additional info after this summary sentence.
The previous design, with the buffering in AudioSendStream, was necessary because I was working with the design-constraint that Channel must be the owner of the tracker (and didn't want to introduce new threading assumptions along the path between AudioSendStream and Channel). Now that we've discussed this offline, and have agreed to move the ownership of the tracker into AudioSendStream itself, the buffering is indeed no longer necessary, and I've removed it. https://codereview.webrtc.org/2638083002/diff/140001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2638083002/diff/140001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:264: // which is called on the thread which channel_proxy_ mostly works on. On 2017/03/15 10:54:13, minyue-webrtc(OOO_until_Mar19) wrote: > second "which" -> that This code no longer exists after the redesign. https://codereview.webrtc.org/2638083002/diff/140001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:267: // Prevent unbounded memory consumption if OnTransportFeedback ends up On 2017/03/15 10:54:13, minyue-webrtc(OOO_until_Mar19) wrote: > I think this may be better taken care inside the packet loss tracker No longer relevant after redesign, but for posterity, this needed to be done like this because we wanted the ownership of the tracker in Channel, and to no introduce new threading constraints on the objects along the way to Channel. https://codereview.webrtc.org/2638083002/diff/140001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.h (right): https://codereview.webrtc.org/2638083002/diff/140001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.h:92: std::vector<SentTransportPacketRecord> packets_sent_since_last_feedback_ On 2017/03/15 10:54:13, minyue-webrtc(OOO_until_Mar19) wrote: > I don't understand why we need this buffer given that the packet loss tracker > has its internal buffer. Because of the threading issue. Please see my other comment about this, in the .cc file. https://codereview.webrtc.org/2638083002/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h (right): https://codereview.webrtc.org/2638083002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h:315: struct SentTransportPacketRecord { On 2017/03/15 10:54:13, minyue-webrtc(OOO_until_Mar19) wrote: > I don't see the need of this struct, because first of all, the buffering of > SentTransportPacketRecord isn't necessary This was necessary when the design was to keep the tracker ownership in Channel. Now that we've redesigned, it's no longer necessary. https://codereview.webrtc.org/2638083002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h:320: // TODO(elad.alon): The only implementers of this interface are On 2017/03/15 10:54:13, minyue-webrtc(OOO_until_Mar19) wrote: > I think this comment should be placed above TransportFeedbackAdapterObserver, > and reword it a bit, starting with > > "TODO(elad.alon): Merge TransportFeedbackAdapterObserver into > TransportFeedbackObserver when xxx." add additional info after this summary > sentence. 1. I've reworded to begin with the action, then provide the background to it. 2. I feel it's weird that a comment that talks a lot about TransportFeedbackObserver is placed above TransportFeedbackAdapterObserver instead. Also, that a TODO whose resolution would be triggered by the deletion of TransportFeedbackObserver is placed elsewhere, where a person removing TransportFeedbackObserver would not see it. Please reconsider.
This is much cleaner. Thanks for the work! some comments from me. https://codereview.webrtc.org/2638083002/diff/100001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/transport_feedback_adapter.cc (right): https://codereview.webrtc.org/2638083002/diff/100001/webrtc/modules/congestio... webrtc/modules/congestion_controller/transport_feedback_adapter.cc:116: if (std::find(observers_.begin(), On 2017/03/10 12:03:26, elad.alon wrote: > On 2017/02/22 11:47:55, minyue-webrtc(OOO_until_Mar19) wrote: > > I would the sanity check be debug only: > > > > RTC_DCHECK_NE(observers_.end(), observers_.find(...)) > > Are you sure? Observer registration is not an often-occurring event, so the > inefficiency of searching through the handful of observers seems worth the extra > safety we get here. Wdyt? My points are: 1. no user actions seem to trigger RegisterTransportFeedbackAdapterObserver, so runtime check may not be needed. 2. if you think there is really a need to take care of re-registration, we don't need the else part. https://codereview.webrtc.org/2638083002/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h (right): https://codereview.webrtc.org/2638083002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h:320: // TODO(elad.alon): The only implementers of this interface are On 2017/03/16 18:37:35, elad.alon wrote: > On 2017/03/15 10:54:13, minyue-webrtc(OOO_until_Mar19) wrote: > > I think this comment should be placed above TransportFeedbackAdapterObserver, > > and reword it a bit, starting with > > > > "TODO(elad.alon): Merge TransportFeedbackAdapterObserver into > > TransportFeedbackObserver when xxx." add additional info after this summary > > sentence. > > 1. I've reworded to begin with the action, then provide the background to it. > 2. I feel it's weird that a comment that talks a lot about > TransportFeedbackObserver is placed above TransportFeedbackAdapterObserver > instead. Not a big deal. And it will be more weird to talk about TransportFeedbackAdapterObserver before it is defined. Also, that a TODO whose resolution would be triggered by the deletion > of TransportFeedbackObserver is placed elsewhere, where a person removing > TransportFeedbackObserver would not see it. Please reconsider. Is TransportFeedbackAdapterObserver or TransportFeedbackObserver to be removed? I am confused. You wrote "Then, we'll remove TransportFeedbackAdapterObserver". So the action is "to remove TransportFeedbackAdapterObserver", isn't it? And therefore, I suggested putting the comment next to TransportFeedbackAdapterObserver. Otherwise, the comment can be forgotten when you remove TransportFeedbackAdapterObserver. https://codereview.webrtc.org/2638083002/diff/200001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/transport_feedback_adapter.cc (right): https://codereview.webrtc.org/2638083002/diff/200001/webrtc/modules/congestio... webrtc/modules/congestion_controller/transport_feedback_adapter.cc:59: auto elem = std::find(observers_.begin(), observers_.end(), observer); elem -> it https://codereview.webrtc.org/2638083002/diff/200001/webrtc/modules/congestio... webrtc/modules/congestion_controller/transport_feedback_adapter.cc:60: if (elem != observers_.end()) { RTC_DCHECK_NE(observers_.end(), it); https://codereview.webrtc.org/2638083002/diff/200001/webrtc/modules/congestio... webrtc/modules/congestion_controller/transport_feedback_adapter.cc:72: rtc::CritScope cs(&lock_); lock_ as a name becomes unclear https://codereview.webrtc.org/2638083002/diff/200001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.h (right): https://codereview.webrtc.org/2638083002/diff/200001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.h:385: const rtc::Optional<float>& packet_loss_rate); I think the arg should be "float packet_loss_rate", if there is no packet_loss_rate, why should we call this function?
https://codereview.webrtc.org/2638083002/diff/100001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/transport_feedback_adapter.cc (right): https://codereview.webrtc.org/2638083002/diff/100001/webrtc/modules/congestio... webrtc/modules/congestion_controller/transport_feedback_adapter.cc:116: if (std::find(observers_.begin(), On 2017/03/17 09:20:31, minyue-webrtc(OOO_until_Mar19) wrote: > On 2017/03/10 12:03:26, elad.alon wrote: > > On 2017/02/22 11:47:55, minyue-webrtc(OOO_until_Mar19) wrote: > > > I would the sanity check be debug only: > > > > > > RTC_DCHECK_NE(observers_.end(), observers_.find(...)) > > > > Are you sure? Observer registration is not an often-occurring event, so the > > inefficiency of searching through the handful of observers seems worth the > extra > > safety we get here. Wdyt? > > My points are: > 1. no user actions seem to trigger RegisterTransportFeedbackAdapterObserver, so > runtime check may not be needed. > 2. if you think there is really a need to take care of re-registration, we don't > need the else part. The else-clause is there to alert a programmer running freshly modified code that breaks this assumption (just like the RTC_DCHECK you suggest). I'll change the code according to your suggestion, although my personal preference is for the extra check. https://codereview.webrtc.org/2638083002/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h (right): https://codereview.webrtc.org/2638083002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h:320: // TODO(elad.alon): The only implementers of this interface are On 2017/03/17 09:20:31, minyue-webrtc(OOO_until_Mar19) wrote: > On 2017/03/16 18:37:35, elad.alon wrote: > > On 2017/03/15 10:54:13, minyue-webrtc(OOO_until_Mar19) wrote: > > > I think this comment should be placed above > TransportFeedbackAdapterObserver, > > > and reword it a bit, starting with > > > > > > "TODO(elad.alon): Merge TransportFeedbackAdapterObserver into > > > TransportFeedbackObserver when xxx." add additional info after this summary > > > sentence. > > > > 1. I've reworded to begin with the action, then provide the background to it. > > 2. I feel it's weird that a comment that talks a lot about > > TransportFeedbackObserver is placed above TransportFeedbackAdapterObserver > > instead. > > Not a big deal. And it will be more weird to talk about > TransportFeedbackAdapterObserver before it is defined. > > Also, that a TODO whose resolution would be triggered by the deletion > > of TransportFeedbackObserver is placed elsewhere, where a person removing > > TransportFeedbackObserver would not see it. Please reconsider. > Is TransportFeedbackAdapterObserver or TransportFeedbackObserver to be removed? > I am confused. > > You wrote "Then, we'll remove TransportFeedbackAdapterObserver". So the action > is "to remove TransportFeedbackAdapterObserver", isn't it? And therefore, I > suggested putting the comment next to TransportFeedbackAdapterObserver. > Otherwise, the comment can be forgotten when you remove > TransportFeedbackAdapterObserver. I think I see the source of the misunderstanding. There are two observers - the one *currently* called "TransportFeedbackObserver" (let's call it O1) and the one *currently* called "TransportFeedbackAdapterObserver" (let's call it O2). As per Stefan, plans are in motion that will remove the necessity for O1. At that point, O1 will be removed, and O2 can assume its name - TransportFeedbackObserver. Therefore, I've placed the TODO next to O1, so that whoever removes O1 either resolves the TODO or alerts me about it. Given that, wdyt? https://codereview.webrtc.org/2638083002/diff/200001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/transport_feedback_adapter.cc (right): https://codereview.webrtc.org/2638083002/diff/200001/webrtc/modules/congestio... webrtc/modules/congestion_controller/transport_feedback_adapter.cc:59: auto elem = std::find(observers_.begin(), observers_.end(), observer); On 2017/03/17 09:20:31, minyue-webrtc(OOO_until_Mar19) wrote: > elem -> it Will do. https://codereview.webrtc.org/2638083002/diff/200001/webrtc/modules/congestio... webrtc/modules/congestion_controller/transport_feedback_adapter.cc:60: if (elem != observers_.end()) { On 2017/03/17 09:20:31, minyue-webrtc(OOO_until_Mar19) wrote: > RTC_DCHECK_NE(observers_.end(), it); Will do. https://codereview.webrtc.org/2638083002/diff/200001/webrtc/modules/congestio... webrtc/modules/congestion_controller/transport_feedback_adapter.cc:72: rtc::CritScope cs(&lock_); On 2017/03/17 09:20:31, minyue-webrtc(OOO_until_Mar19) wrote: > lock_ as a name becomes unclear Stefan, do you have a specific name you'd like? https://codereview.webrtc.org/2638083002/diff/200001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.h (right): https://codereview.webrtc.org/2638083002/diff/200001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.h:385: const rtc::Optional<float>& packet_loss_rate); On 2017/03/17 09:20:32, minyue-webrtc(OOO_until_Mar19) wrote: > I think the arg should be "float packet_loss_rate", if there is no > packet_loss_rate, why should we call this function? An unknown is possible when a new feedback arrives after a long period of inactivity, making the window go back down below the thresholds for determining "statistically significant" (kind of) values. In such a case, ANA should decide what to do, not the tracker (not passing new input = pushing to keep the last decision).
https://codereview.webrtc.org/2638083002/diff/200001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/transport_feedback_adapter.cc (right): https://codereview.webrtc.org/2638083002/diff/200001/webrtc/modules/congestio... webrtc/modules/congestion_controller/transport_feedback_adapter.cc:60: if (elem != observers_.end()) { On 2017/03/17 10:10:33, elad.alon wrote: > On 2017/03/17 09:20:31, minyue-webrtc(OOO_until_Mar19) wrote: > > RTC_DCHECK_NE(observers_.end(), it); > > Will do. Minyue, FYI, the RTC_DCHECK_EQ/_NE doesn't seem to work well with these iterators, so I'll be using a generic RTC_DCHECK. Something with the stringification of the arguments; I've not dug deeper because I don't think it's in-scope. Let me know if you think this should be looked more into.
https://codereview.webrtc.org/2638083002/diff/100001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/transport_feedback_adapter.cc (right): https://codereview.webrtc.org/2638083002/diff/100001/webrtc/modules/congestio... webrtc/modules/congestion_controller/transport_feedback_adapter.cc:116: if (std::find(observers_.begin(), On 2017/03/17 10:10:32, elad.alon wrote: > On 2017/03/17 09:20:31, minyue-webrtc(OOO_until_Mar19) wrote: > > On 2017/03/10 12:03:26, elad.alon wrote: > > > On 2017/02/22 11:47:55, minyue-webrtc(OOO_until_Mar19) wrote: > > > > I would the sanity check be debug only: > > > > > > > > RTC_DCHECK_NE(observers_.end(), observers_.find(...)) > > > > > > Are you sure? Observer registration is not an often-occurring event, so the > > > inefficiency of searching through the handful of observers seems worth the > > extra > > > safety we get here. Wdyt? > > > > My points are: > > 1. no user actions seem to trigger RegisterTransportFeedbackAdapterObserver, > so > > runtime check may not be needed. > > 2. if you think there is really a need to take care of re-registration, we > don't > > need the else part. > > The else-clause is there to alert a programmer running freshly modified code > that breaks this assumption (just like the RTC_DCHECK you suggest). > I'll change the code according to your suggestion, although my personal > preference is for the extra check. As a reader's point of view, I would like to see that you either forbid it or allow misuse. It is a bit confusing if we say "we forbid it, but in case you forget ..." That's why for my suggested code. https://codereview.webrtc.org/2638083002/diff/200001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/transport_feedback_adapter.cc (right): https://codereview.webrtc.org/2638083002/diff/200001/webrtc/modules/congestio... webrtc/modules/congestion_controller/transport_feedback_adapter.cc:60: if (elem != observers_.end()) { On 2017/03/17 13:40:06, elad.alon wrote: > On 2017/03/17 10:10:33, elad.alon wrote: > > On 2017/03/17 09:20:31, minyue-webrtc(OOO_until_Mar19) wrote: > > > RTC_DCHECK_NE(observers_.end(), it); > > > > Will do. > > Minyue, FYI, the RTC_DCHECK_EQ/_NE doesn't seem to work well with these > iterators, so I'll be using a generic RTC_DCHECK. Something with the > stringification of the arguments; I've not dug deeper because I don't think it's > in-scope. Let me know if you think this should be looked more into. RTC_DCHECK( xx != xx) is fine. Thanks. https://codereview.webrtc.org/2638083002/diff/220001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/transport_feedback_adapter.cc (right): https://codereview.webrtc.org/2638083002/diff/220001/webrtc/modules/congestio... webrtc/modules/congestion_controller/transport_feedback_adapter.cc:58: observers_.erase(it); extra check RTC_DCHECK(std::find(observers_.begin(), observers_.end(), observer) == observers_.end());
elad.alon@webrtc.org changed reviewers: + solenberg@webrtc.org
https://codereview.webrtc.org/2638083002/diff/260001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h (right): https://codereview.webrtc.org/2638083002/diff/260001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h:341: // and is removed, use that name for this observer instead. Ok. What you wrote in the other comment, please say, "replace TransportFeedbackObserver with TransportFeedbackAdapterObserver".
PTAL https://codereview.webrtc.org/2638083002/diff/220001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/transport_feedback_adapter.cc (right): https://codereview.webrtc.org/2638083002/diff/220001/webrtc/modules/congestio... webrtc/modules/congestion_controller/transport_feedback_adapter.cc:58: observers_.erase(it); On 2017/03/21 09:22:58, minyue-webrtc wrote: > extra check > > RTC_DCHECK(std::find(observers_.begin(), observers_.end(), observer) == > observers_.end()); Done.
https://codereview.webrtc.org/2638083002/diff/260001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2638083002/diff/260001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:267: // potentially reset the window, sending both PLR and RPLR to UNKNOWN. sending -> making to UNKNOWN -> unknown https://codereview.webrtc.org/2638083002/diff/260001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:278: // to unknown, no indication is given, which leads the lower layers to think A little bit misleading, "leads the lower layers to think the old value is still correct" is not a necessary case. You may reword it to something like if |plr| is empty, it may mean that the old value is no longer valid. We currently do not explicitly pass such information.
https://codereview.webrtc.org/2638083002/diff/280001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2638083002/diff/280001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:57: : clock_(Clock::GetRealTimeClock()), I think the code would be better testable when we inject the clock rather than construct it here. https://codereview.webrtc.org/2638083002/diff/280001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:261: void AudioSendStream::OnPacketAdded(uint32_t ssrc, uint16_t seq_num) { Can you add some unit test for these functions ? https://codereview.webrtc.org/2638083002/diff/300001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/transport_feedback_adapter.cc (right): https://codereview.webrtc.org/2638083002/diff/300001/webrtc/modules/congestio... webrtc/modules/congestion_controller/transport_feedback_adapter.cc:43: void TransportFeedbackAdapter::RegisterTransportFeedbackAdapterObserver( Pleas add some unit test for the added functionality.
On 2017/03/21 10:53:26, michaelt wrote: > https://codereview.webrtc.org/2638083002/diff/280001/webrtc/audio/audio_send_... > File webrtc/audio/audio_send_stream.cc (right): > > https://codereview.webrtc.org/2638083002/diff/280001/webrtc/audio/audio_send_... > webrtc/audio/audio_send_stream.cc:57: : clock_(Clock::GetRealTimeClock()), > I think the code would be better testable when we inject the clock rather than > construct it here. > > https://codereview.webrtc.org/2638083002/diff/280001/webrtc/audio/audio_send_... > webrtc/audio/audio_send_stream.cc:261: void > AudioSendStream::OnPacketAdded(uint32_t ssrc, uint16_t seq_num) { > Can you add some unit test for these functions ? > > https://codereview.webrtc.org/2638083002/diff/300001/webrtc/modules/congestio... > File webrtc/modules/congestion_controller/transport_feedback_adapter.cc (right): > > https://codereview.webrtc.org/2638083002/diff/300001/webrtc/modules/congestio... > webrtc/modules/congestion_controller/transport_feedback_adapter.cc:43: void > TransportFeedbackAdapter::RegisterTransportFeedbackAdapterObserver( > Pleas add some unit test for the added functionality. +1
https://codereview.webrtc.org/2638083002/diff/260001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2638083002/diff/260001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:267: // potentially reset the window, sending both PLR and RPLR to UNKNOWN. On 2017/03/21 10:50:39, minyue-webrtc wrote: > sending -> making > > to UNKNOWN -> unknown I've used "setting". https://codereview.webrtc.org/2638083002/diff/260001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:278: // to unknown, no indication is given, which leads the lower layers to think On 2017/03/21 10:50:39, minyue-webrtc wrote: > A little bit misleading, "leads the lower layers to think the old value is still > correct" is not a necessary case. > > You may reword it to something like > > if |plr| is empty, it may mean that the old value is no longer valid. We > currently do not explicitly pass such information. Handled.
https://codereview.webrtc.org/2638083002/diff/200001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/transport_feedback_adapter.cc (right): https://codereview.webrtc.org/2638083002/diff/200001/webrtc/modules/congestio... webrtc/modules/congestion_controller/transport_feedback_adapter.cc:72: rtc::CritScope cs(&lock_); On 2017/03/17 10:10:33, elad.alon wrote: > On 2017/03/17 09:20:31, minyue-webrtc(OOO_until_Mar19) wrote: > > lock_ as a name becomes unclear > > Stefan, do you have a specific name you'd like? Not really since it's a generic lock used for a bunch of things. feedback_lock_ maybe... Not much better. https://codereview.webrtc.org/2638083002/diff/200001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/transport_feedback_adapter.h (right): https://codereview.webrtc.org/2638083002/diff/200001/webrtc/modules/congestio... webrtc/modules/congestion_controller/transport_feedback_adapter.h:69: uint16_t remote_net_id_; Seems like I missed adding GUARDED_BY(&lock_) here. Could you add that?
https://codereview.webrtc.org/2638083002/diff/200001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/transport_feedback_adapter.cc (right): https://codereview.webrtc.org/2638083002/diff/200001/webrtc/modules/congestio... webrtc/modules/congestion_controller/transport_feedback_adapter.cc:72: rtc::CritScope cs(&lock_); On 2017/03/21 11:35:47, stefan-webrtc wrote: > On 2017/03/17 10:10:33, elad.alon wrote: > > On 2017/03/17 09:20:31, minyue-webrtc(OOO_until_Mar19) wrote: > > > lock_ as a name becomes unclear > > > > Stefan, do you have a specific name you'd like? > > Not really since it's a generic lock used for a bunch of things. feedback_lock_ > maybe... Not much better. Acknowledged.
PTAL https://codereview.webrtc.org/2638083002/diff/200001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/transport_feedback_adapter.h (right): https://codereview.webrtc.org/2638083002/diff/200001/webrtc/modules/congestio... webrtc/modules/congestion_controller/transport_feedback_adapter.h:69: uint16_t remote_net_id_; On 2017/03/21 11:35:47, stefan-webrtc wrote: > Seems like I missed adding GUARDED_BY(&lock_) here. Could you add that? Done. (I've assumed both local_net_id_ and remote_net_id_ were meant, but nothing else, based on the code. Let me know if more.) https://codereview.webrtc.org/2638083002/diff/280001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2638083002/diff/280001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:57: : clock_(Clock::GetRealTimeClock()), On 2017/03/21 10:53:26, michaelt wrote: > I think the code would be better testable when we inject the clock rather than > construct it here. It's equally testable both ways; please see how the UT is providing the timestamps. Otherwise, I'd have had to use a SimulatedClock that I'd have to manually increment in the UT, achieving the same effect. I have no significant preference, but I lean towards keep it as is, thereby minimizing changes. Minyue? https://codereview.webrtc.org/2638083002/diff/280001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:261: void AudioSendStream::OnPacketAdded(uint32_t ssrc, uint16_t seq_num) { On 2017/03/21 10:53:26, michaelt wrote: > Can you add some unit test for these functions ? This is just piping. Also, keep in mind that we plan to move it out of here soon, so I think creating a new UT for this piping, then removing it, is unnecessary. https://codereview.webrtc.org/2638083002/diff/300001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/transport_feedback_adapter.cc (right): https://codereview.webrtc.org/2638083002/diff/300001/webrtc/modules/congestio... webrtc/modules/congestion_controller/transport_feedback_adapter.cc:43: void TransportFeedbackAdapter::RegisterTransportFeedbackAdapterObserver( On 2017/03/21 10:53:26, michaelt wrote: > Pleas add some unit test for the added functionality. Added.
The CQ bit was checked by elad.alon@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: ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...) ios64_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios9_dbg/buil...)
one comment + one nit But I would say lgtm. https://codereview.webrtc.org/2638083002/diff/280001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2638083002/diff/280001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:57: : clock_(Clock::GetRealTimeClock()), On 2017/03/21 17:23:14, elad.alon wrote: > On 2017/03/21 10:53:26, michaelt wrote: > > I think the code would be better testable when we inject the clock rather than > > construct it here. > > It's equally testable both ways; please see how the UT is providing the > timestamps. Otherwise, I'd have had to use a SimulatedClock that I'd have to > manually increment in the UT, achieving the same effect. I have no significant > preference, but I lean towards keep it as is, thereby minimizing changes. > Minyue? I think it is fine to keep it as it is, since we cannot test how clock interacts with packet_loss_tracker_ here (otherwise, packet_loss_tracker_ needs to be injected too), and the logic between clock and packet_loss_tracker_ is simple. But Elad, what UT do you talk about in your comment. I see no new audio_send_stream_unittest, and think you meant packet_loss_tracker_unittests. https://codereview.webrtc.org/2638083002/diff/280001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:261: void AudioSendStream::OnPacketAdded(uint32_t ssrc, uint16_t seq_num) { On 2017/03/21 17:23:14, elad.alon wrote: > On 2017/03/21 10:53:26, michaelt wrote: > > Can you add some unit test for these functions ? > > This is just piping. Also, keep in mind that we plan to move it out of here > soon, so I think creating a new UT for this piping, then removing it, is > unnecessary. I agree to omit a test in AudioSendStream. https://codereview.webrtc.org/2638083002/diff/340001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.h (right): https://codereview.webrtc.org/2638083002/diff/340001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.h:25: class SendSideCongestionController; just a note: please try rebasing in a separate patch set. usually, such a patch set will require minimum review. https://codereview.webrtc.org/2638083002/diff/340001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/transport_feedback_adapter_unittest.cc (right): https://codereview.webrtc.org/2638083002/diff/340001/webrtc/modules/congestio... webrtc/modules/congestion_controller/transport_feedback_adapter_unittest.cc:86: std::vector<PacketFeedback> packets = { const https://codereview.webrtc.org/2638083002/diff/360001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/transport_feedback_adapter_unittest.cc (right): https://codereview.webrtc.org/2638083002/diff/360001/webrtc/modules/congestio... webrtc/modules/congestion_controller/transport_feedback_adapter_unittest.cc:106: adapter_->DeRegisterTransportFeedbackAdapterObserver(&mock); I hope you do another adapter_->OnTransportFeedback(feedback); after DeRegister. Add .Times(1) after EXPECT_CALL(mock, OnNewTransportFeedbackVector(_)). (it is times(1) by default but it is easier to read, which I learned from michaelt) https://codereview.webrtc.org/2638083002/diff/360001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h (right): https://codereview.webrtc.org/2638083002/diff/360001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h:368: // TransportFeedbackProxy therefore removed. Then, we'll remove I still think the last sentence "Then, we'll remove TransportFeedbackAdapterObserver, reuse the name TransportFeedbackObserver for TransportFeedbackAdapterObserver, and only have one observer." is confusing. The first sentence said "TransportFeedbackObserver will be removed" but now it becomes "TransportFeedbackAdapterObserver", I think it is a typo.
https://codereview.webrtc.org/2638083002/diff/280001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2638083002/diff/280001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:57: : clock_(Clock::GetRealTimeClock()), On 2017/03/22 07:51:39, minyue-webrtc wrote: > On 2017/03/21 17:23:14, elad.alon wrote: > > On 2017/03/21 10:53:26, michaelt wrote: > > > I think the code would be better testable when we inject the clock rather > than > > > construct it here. > > > > It's equally testable both ways; please see how the UT is providing the > > timestamps. Otherwise, I'd have had to use a SimulatedClock that I'd have to > > manually increment in the UT, achieving the same effect. I have no significant > > preference, but I lean towards keep it as is, thereby minimizing changes. > > Minyue? > > I think it is fine to keep it as it is, since we cannot test how clock interacts > with packet_loss_tracker_ here (otherwise, packet_loss_tracker_ needs to be > injected too), and the logic between clock and packet_loss_tracker_ is simple. > > But Elad, what UT do you talk about in your comment. I see no new > audio_send_stream_unittest, and think you meant packet_loss_tracker_unittests. Yes, that's what I meant. I thought Michael was talking about packet_loss_tracker_unittests. If he's talking about audio_send_stream_unittest, then I agree with him in principle, but since the clock_ is not really used for anything except for the tracker, and the tracker would soon move to the encoder, I think it's simpler to simply construct (as you've said). https://codereview.webrtc.org/2638083002/diff/340001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.h (right): https://codereview.webrtc.org/2638083002/diff/340001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.h:25: class SendSideCongestionController; On 2017/03/22 07:51:39, minyue-webrtc wrote: > just a note: please try rebasing in a separate patch set. usually, such a patch > set will require minimum review. Sure, will do. https://codereview.webrtc.org/2638083002/diff/340001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/transport_feedback_adapter_unittest.cc (right): https://codereview.webrtc.org/2638083002/diff/340001/webrtc/modules/congestio... webrtc/modules/congestion_controller/transport_feedback_adapter_unittest.cc:86: std::vector<PacketFeedback> packets = { On 2017/03/22 07:51:39, minyue-webrtc wrote: > const Done. https://codereview.webrtc.org/2638083002/diff/360001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/transport_feedback_adapter_unittest.cc (right): https://codereview.webrtc.org/2638083002/diff/360001/webrtc/modules/congestio... webrtc/modules/congestion_controller/transport_feedback_adapter_unittest.cc:106: adapter_->DeRegisterTransportFeedbackAdapterObserver(&mock); On 2017/03/22 07:51:39, minyue-webrtc wrote: > I hope you do another > > adapter_->OnTransportFeedback(feedback); > > after DeRegister. > > Add .Times(1) after EXPECT_CALL(mock, OnNewTransportFeedbackVector(_)). (it is > times(1) by default but it is easier to read, which I learned from michaelt) Done. https://codereview.webrtc.org/2638083002/diff/360001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h (right): https://codereview.webrtc.org/2638083002/diff/360001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h:368: // TransportFeedbackProxy therefore removed. Then, we'll remove On 2017/03/22 07:51:39, minyue-webrtc wrote: > I still think the last sentence > > "Then, we'll remove TransportFeedbackAdapterObserver, reuse the name > TransportFeedbackObserver for TransportFeedbackAdapterObserver, and only have > one observer." > > is confusing. > > The first sentence said "TransportFeedbackObserver will be removed" but now it > becomes "TransportFeedbackAdapterObserver", I think it is a typo. Done.
https://codereview.webrtc.org/2638083002/diff/380001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.h (right): https://codereview.webrtc.org/2638083002/diff/380001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.h:70: void OnNewTransportFeedbackVector( Let's drop "New" if you don't think it provides value. Btw, in another CL I think we used OnPacketFeedbacks() for the same type of method? Or was this changed? I don't really like that we call it TransportFeedback when it no longer has to be. We could follow up on this though (and perhaps rename TransportFeedbackAdapterObserver (puh) to something like PacketFeedbackObserver? https://codereview.webrtc.org/2638083002/diff/380001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/transport_feedback_adapter.cc (right): https://codereview.webrtc.org/2638083002/diff/380001/webrtc/modules/congestio... webrtc/modules/congestion_controller/transport_feedback_adapter.cc:60: observers_.end()); This seems redundant to me. Should be enough to check that we only add an observer once and that it exists when we remove it. https://codereview.webrtc.org/2638083002/diff/380001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/transport_feedback_adapter_unittest.cc (right): https://codereview.webrtc.org/2638083002/diff/380001/webrtc/modules/congestio... webrtc/modules/congestion_controller/transport_feedback_adapter_unittest.cc:123: TEST_F(TransportFeedbackAdapterTest, ObserverDoubleRegistrationDeathTest) { Not sure about the value of these tests, but if you think they pull their own weight... :) https://codereview.webrtc.org/2638083002/diff/380001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2638083002/diff/380001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:399: owner_->OnUplinkPacketLossRate(weighted_fraction_lost / 255.0f); Is this a bug fix? Should be mentioned somewhere in that case. https://codereview.webrtc.org/2638083002/diff/380001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel_proxy.cc (right): https://codereview.webrtc.org/2638083002/diff/380001/webrtc/voice_engine/chan... webrtc/voice_engine/channel_proxy.cc:370: void ChannelProxy::OnTwccBasedUplinkPacketLossRate(float packet_loss_rate) { Most methods here have thread checks. Should this one have one?
https://codereview.webrtc.org/2638083002/diff/380001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2638083002/diff/380001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:45: constexpr size_t kPlrMinNumAckedPackets = 50; In this context, the abbreviations PLR and RPLR will be difficult to understand. Maybe it is better to type the words out? https://codereview.webrtc.org/2638083002/diff/380001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:57: : clock_(Clock::GetRealTimeClock()), Remove. https://codereview.webrtc.org/2638083002/diff/380001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:261: void AudioSendStream::OnPacketAdded(uint32_t ssrc, uint16_t seq_num) { Add thread checkers to document on which thread(s) this may be called. https://codereview.webrtc.org/2638083002/diff/380001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:266: // TODO(elad.alon): Take care of the following known issue - this could Which following issues? https://codereview.webrtc.org/2638083002/diff/380001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:274: rtc::CritScope lock(&packet_loss_tracker_cs_); packet_loss_tracker_ and channel_proxy_ are both set up in ctor and never changed during the lifetime of the stream. So, what are you protecting with this lock? Internals in the PLT (potential race between OnNewTransportFeedbackVector() and GetPacketLossRate(), or some subtle dependency between the PLT and the voe::Channel? https://codereview.webrtc.org/2638083002/diff/380001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.h (right): https://codereview.webrtc.org/2638083002/diff/380001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.h:68: // From TransportFeedbackAdapterObserver super nit: comments end in . https://codereview.webrtc.org/2638083002/diff/380001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.h:81: const Clock* const clock_; Please, no more Clock*. Use the functions in webrtc/base/timeutils.h instead. https://codereview.webrtc.org/2638083002/diff/380001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/transport_feedback_adapter.cc (right): https://codereview.webrtc.org/2638083002/diff/380001/webrtc/modules/congestio... webrtc/modules/congestion_controller/transport_feedback_adapter.cc:44: TransportFeedbackAdapterObserver* observer) { Please add thread checkers to document multithread usage in these functions. We'd like to move away from a world full of locks to one where we know which threads touch what data. https://codereview.webrtc.org/2638083002/diff/380001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2638083002/diff/380001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:908: use_twcc_plr_for_ana_(false) { Ok, but does it really need to be a class member? Will the experiment affect separate channel instances differently? https://codereview.webrtc.org/2638083002/diff/380001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:1307: void Channel::OnTwccBasedUplinkPacketLossRate(float packet_loss_rate) { ossu@ - should this be done differently, considering your upcoming changes? Would that affect the relationships in this CL in any way? https://codereview.webrtc.org/2638083002/diff/380001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.h (right): https://codereview.webrtc.org/2638083002/diff/380001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.h:37: #include "webrtc/voice_engine/transport_feedback_packet_loss_tracker.h" No need to include this here. https://codereview.webrtc.org/2638083002/diff/380001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.h:382: // Note: The existence of this function alongside OnUplinkPacketLossRate is "Note:" is superfluous. https://codereview.webrtc.org/2638083002/diff/380001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.h:388: protected: Nobody inherits from this class so you should move the below in under private: https://codereview.webrtc.org/2638083002/diff/380001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel_proxy.cc (right): https://codereview.webrtc.org/2638083002/diff/380001/webrtc/voice_engine/chan... webrtc/voice_engine/channel_proxy.cc:370: void ChannelProxy::OnTwccBasedUplinkPacketLossRate(float packet_loss_rate) { On 2017/03/22 12:03:01, stefan-webrtc wrote: > Most methods here have thread checks. Should this one have one? Yes.
https://codereview.webrtc.org/2638083002/diff/380001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/transport_feedback_adapter.cc (right): https://codereview.webrtc.org/2638083002/diff/380001/webrtc/modules/congestio... webrtc/modules/congestion_controller/transport_feedback_adapter.cc:60: observers_.end()); On 2017/03/22 12:03:01, stefan-webrtc wrote: > This seems redundant to me. Should be enough to check that we only add an > observer once and that it exists when we remove it. I suggested it. It is clearly redundant, but just an extra dcheck, in case observers_ is touched at other place than Register... I am fine if you strongly advice removing it.
https://codereview.webrtc.org/2638083002/diff/380001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2638083002/diff/380001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:1307: void Channel::OnTwccBasedUplinkPacketLossRate(float packet_loss_rate) { On 2017/03/22 12:06:42, the sun wrote: > ossu@ - should this be done differently, considering your upcoming changes? > Would that affect the relationships in this CL in any way? Thanks for the heads-up! We're still handing the encoder over to the ACM, so this should continue to work. Eventually, we'll want to move stuff from Channel and possibly have the encoder listen directly to this kind of feedback, rather than having to plumb it manually. That's outside the scope of this and of my current CLs, though.
https://codereview.webrtc.org/2638083002/diff/380001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2638083002/diff/380001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:45: constexpr size_t kPlrMinNumAckedPackets = 50; On 2017/03/22 12:06:42, the sun wrote: > In this context, the abbreviations PLR and RPLR will be difficult to understand. > Maybe it is better to type the words out? Done. https://codereview.webrtc.org/2638083002/diff/380001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:57: : clock_(Clock::GetRealTimeClock()), On 2017/03/22 12:06:42, the sun wrote: > Remove. Done. https://codereview.webrtc.org/2638083002/diff/380001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:261: void AudioSendStream::OnPacketAdded(uint32_t ssrc, uint16_t seq_num) { On 2017/03/22 12:06:42, the sun wrote: > Add thread checkers to document on which thread(s) this may be called. Done. https://codereview.webrtc.org/2638083002/diff/380001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:266: // TODO(elad.alon): Take care of the following known issue - this could On 2017/03/22 12:06:42, the sun wrote: > Which following issues? The one immediately after the hyphen. Minyue has asked me to use the imperative mood when formulating TODOs. (The "this" refers to the function call right above the TODO. Moved it to before and reworded a bit now.) https://codereview.webrtc.org/2638083002/diff/380001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:274: rtc::CritScope lock(&packet_loss_tracker_cs_); On 2017/03/22 12:06:42, the sun wrote: > packet_loss_tracker_ and channel_proxy_ are both set up in ctor and never > changed during the lifetime of the stream. So, what are you protecting with this > lock? Internals in the PLT (potential race between > OnNewTransportFeedbackVector() and GetPacketLossRate(), or some subtle > dependency between the PLT and the voe::Channel? PLT internals. OnPacketAdded() and OnNewTransportFeedbackVector() are called on separate threads. I'll move the channel_proxy_ out of the critical section, though. https://codereview.webrtc.org/2638083002/diff/380001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.h (right): https://codereview.webrtc.org/2638083002/diff/380001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.h:68: // From TransportFeedbackAdapterObserver On 2017/03/22 12:06:42, the sun wrote: > super nit: comments end in . Since both you an Minyue comment on this, I'll align with the local style, but IMHO, full sentences end in full stops, and titles don't. (Example: "War and Peace", not "War and Peace.") https://codereview.webrtc.org/2638083002/diff/380001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.h:70: void OnNewTransportFeedbackVector( On 2017/03/22 12:03:01, stefan-webrtc wrote: > Let's drop "New" if you don't think it provides value. Btw, in another CL I > think we used OnPacketFeedbacks() for the same type of method? Or was this > changed? I don't really like that we call it TransportFeedback when it no longer > has to be. We could follow up on this though (and perhaps rename > TransportFeedbackAdapterObserver (puh) to something like PacketFeedbackObserver? (I've renamed elsewhere as a follow-up to Fredrick's offline comment that "feedback" in non-countable.) Done. I've used OnPacketFeedbackVector() and PacketFeedbackObserver. https://codereview.webrtc.org/2638083002/diff/380001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.h:81: const Clock* const clock_; On 2017/03/22 12:06:42, the sun wrote: > Please, no more Clock*. Use the functions in webrtc/base/timeutils.h instead. Done. https://codereview.webrtc.org/2638083002/diff/380001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/transport_feedback_adapter.cc (right): https://codereview.webrtc.org/2638083002/diff/380001/webrtc/modules/congestio... webrtc/modules/congestion_controller/transport_feedback_adapter.cc:44: TransportFeedbackAdapterObserver* observer) { On 2017/03/22 12:06:42, the sun wrote: > Please add thread checkers to document multithread usage in these functions. > We'd like to move away from a world full of locks to one where we know which > threads touch what data. We've tried that before with this class - https://codereview.webrtc.org/2710213003/ - but ran into UT problems. We decided that in this case, it wouldn't be worth the trouble. https://codereview.webrtc.org/2638083002/diff/380001/webrtc/modules/congestio... webrtc/modules/congestion_controller/transport_feedback_adapter.cc:60: observers_.end()); On 2017/03/22 12:14:48, minyue-webrtc wrote: > On 2017/03/22 12:03:01, stefan-webrtc wrote: > > This seems redundant to me. Should be enough to check that we only add an > > observer once and that it exists when we remove it. > > I suggested it. It is clearly redundant, but just an extra dcheck, in case > observers_ is touched at other place than Register... > > I am fine if you strongly advice removing it. Waiting for further input. https://codereview.webrtc.org/2638083002/diff/380001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/transport_feedback_adapter_unittest.cc (right): https://codereview.webrtc.org/2638083002/diff/380001/webrtc/modules/congestio... webrtc/modules/congestion_controller/transport_feedback_adapter_unittest.cc:123: TEST_F(TransportFeedbackAdapterTest, ObserverDoubleRegistrationDeathTest) { On 2017/03/22 12:03:01, stefan-webrtc wrote: > Not sure about the value of these tests, but if you think they pull their own > weight... :) I've added them on the basis of reviewers' request. Or do you just mean the death-tests specifically? https://codereview.webrtc.org/2638083002/diff/380001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2638083002/diff/380001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:399: owner_->OnUplinkPacketLossRate(weighted_fraction_lost / 255.0f); On 2017/03/22 12:03:01, stefan-webrtc wrote: > Is this a bug fix? Should be mentioned somewhere in that case. It's not. I'm just moving the place where it divides by 255.0f, to make things clearer. Otherwise, OnTwccBasedUplinkPacketLossRate() and OnUplinkPacketLossRate() get different inputs, which is confusing. https://codereview.webrtc.org/2638083002/diff/380001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:908: use_twcc_plr_for_ana_(false) { On 2017/03/22 12:06:42, the sun wrote: > Ok, but does it really need to be a class member? Will the experiment affect > separate channel instances differently? If it ends up populated from a FieldTrial - please see https://codereview.webrtc.org/2684773002. However, one does not want to call webrtc::field_trial::FindFullName() multiple times, as it involves string-comparison, and is thus wasteful. The alternative of a global is, IMHO, inferior to a member variable. It's true that multiple instances would mean memory gets allocated for this multiple times, but if we go down to that resolution, I'd argue that cache-locality would be better with a member than with a global. https://codereview.webrtc.org/2638083002/diff/380001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.h (right): https://codereview.webrtc.org/2638083002/diff/380001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.h:37: #include "webrtc/voice_engine/transport_feedback_packet_loss_tracker.h" On 2017/03/22 12:06:42, the sun wrote: > No need to include this here. Done. (FYI, relic from an older revision where Channel was the owner of the tracker, as per Minyue's decree.) https://codereview.webrtc.org/2638083002/diff/380001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.h:382: // Note: The existence of this function alongside OnUplinkPacketLossRate is On 2017/03/22 12:06:42, the sun wrote: > "Note:" is superfluous. I've removed, but IMHO, it made it immediately clear that this is a side-note about the function, rather than documentation of what the function does, which is what one would normally expect in such a location. https://codereview.webrtc.org/2638083002/diff/380001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.h:388: protected: On 2017/03/22 12:06:42, the sun wrote: > Nobody inherits from this class so you should move the below in under private: Done. https://codereview.webrtc.org/2638083002/diff/380001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel_proxy.cc (right): https://codereview.webrtc.org/2638083002/diff/380001/webrtc/voice_engine/chan... webrtc/voice_engine/channel_proxy.cc:370: void ChannelProxy::OnTwccBasedUplinkPacketLossRate(float packet_loss_rate) { On 2017/03/22 12:06:42, the sun wrote: > On 2017/03/22 12:03:01, stefan-webrtc wrote: > > Most methods here have thread checks. Should this one have one? > > Yes. Done.
terelius@webrtc.org changed reviewers: + terelius@webrtc.org
lgtm for analyzer https://codereview.webrtc.org/2638083002/diff/460001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/congestion_controller.cc (right): https://codereview.webrtc.org/2638083002/diff/460001/webrtc/modules/congestio... webrtc/modules/congestion_controller/congestion_controller.cc:109: void CongestionController::AddPacket(uint32_t ssrc, If we're changing this method signature, could we also update the name to make the meaning more clear? It is not immediately clear to me whether "adding a packet to the congestion controller" means 1) adding one incoming RTCP packet (containing feedback about many RTP packets), 2) adding received feedback about one RTP packet, or 3) adding one outgoing RTP packet. Maybe AddOutgoingPacket, AddPacketToSendTimeHistory or OnPacedPacket? Stefan, WDYT?
https://codereview.webrtc.org/2638083002/diff/460001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/congestion_controller.cc (right): https://codereview.webrtc.org/2638083002/diff/460001/webrtc/modules/congestio... webrtc/modules/congestion_controller/congestion_controller.cc:109: void CongestionController::AddPacket(uint32_t ssrc, On 2017/03/22 16:03:32, terelius wrote: > If we're changing this method signature, could we also update the name to make > the meaning more clear? It is not immediately clear to me whether "adding a > packet to the congestion controller" means > 1) adding one incoming RTCP packet (containing feedback about many RTP packets), > 2) adding received feedback about one RTP packet, or > 3) adding one outgoing RTP packet. > > Maybe AddOutgoingPacket, AddPacketToSendTimeHistory or OnPacedPacket? Stefan, > WDYT? +1, though I'd prefer doing it in a separate CL. Wdys?
https://codereview.webrtc.org/2638083002/diff/380001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2638083002/diff/380001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:266: // TODO(elad.alon): Take care of the following known issue - this could On 2017/03/22 14:34:46, elad.alon wrote: > On 2017/03/22 12:06:42, the sun wrote: > > Which following issues? > > The one immediately after the hyphen. Minyue has asked me to use the imperative > mood when formulating TODOs. (The "this" refers to the function call right above > the TODO. Moved it to before and reworded a bit now.) The work "following" makes people think an issue outside the TODO. So Take care of the issue that, after OnPacketAdded() xxx can become unknown, but currently such information is not passed down.
https://codereview.webrtc.org/2638083002/diff/380001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2638083002/diff/380001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:266: // TODO(elad.alon): Take care of the following known issue - this could On 2017/03/22 16:27:40, minyue-webrtc wrote: > On 2017/03/22 14:34:46, elad.alon wrote: > > On 2017/03/22 12:06:42, the sun wrote: > > > Which following issues? > > > > The one immediately after the hyphen. Minyue has asked me to use the > imperative > > mood when formulating TODOs. (The "this" refers to the function call right > above > > the TODO. Moved it to before and reworded a bit now.) > > The work "following" makes people think an issue outside the TODO. So > > Take care of the issue that, after OnPacketAdded() xxx can become unknown, but > currently such information is not passed down. I think the wording of the latest patchset (PS24) is clear enough. I'll keep that, unless someone has a strong objection.
https://codereview.webrtc.org/2638083002/diff/280001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2638083002/diff/280001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:57: : clock_(Clock::GetRealTimeClock()), I was talking about audio_send_stream_unittest :). I'm ok with not testing the functionality in audio_send_stream if we move the tracker later. But as "the sun" said we should remove "clock_" anyway an replace it by the new time interface "webrtc/base/timeutils.h"
https://codereview.webrtc.org/2638083002/diff/280001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2638083002/diff/280001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:57: : clock_(Clock::GetRealTimeClock()), On 2017/03/23 09:57:45, michaelt wrote: > I was talking about audio_send_stream_unittest :). > I'm ok with not testing the functionality in audio_send_stream if we move the > tracker later. > > But as "the sun" said we should remove "clock_" anyway an replace it by the new > time interface "webrtc/base/timeutils.h" > Done in latest patchset.
lgtm
https://codereview.webrtc.org/2638083002/diff/380001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2638083002/diff/380001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:266: // TODO(elad.alon): Take care of the following known issue - this could On 2017/03/22 16:34:19, elad.alon wrote: > On 2017/03/22 16:27:40, minyue-webrtc wrote: > > On 2017/03/22 14:34:46, elad.alon wrote: > > > On 2017/03/22 12:06:42, the sun wrote: > > > > Which following issues? > > > > > > The one immediately after the hyphen. Minyue has asked me to use the > > imperative > > > mood when formulating TODOs. (The "this" refers to the function call right > > above > > > the TODO. Moved it to before and reworded a bit now.) > > > > The work "following" makes people think an issue outside the TODO. So > > > > Take care of the issue that, after OnPacketAdded() xxx can become unknown, but > > currently such information is not passed down. > > I think the wording of the latest patchset (PS24) is clear enough. I'll keep > that, unless someone has a strong objection. The wording has not changed in the sense that the use of hyphen is still confusing. In fact, you could just remove the "Take care of the following known issue" part - TODO() already means that. Same goes below ("Resolve the following known issue"). https://codereview.webrtc.org/2638083002/diff/380001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:274: rtc::CritScope lock(&packet_loss_tracker_cs_); On 2017/03/22 14:34:46, elad.alon wrote: > On 2017/03/22 12:06:42, the sun wrote: > > packet_loss_tracker_ and channel_proxy_ are both set up in ctor and never > > changed during the lifetime of the stream. So, what are you protecting with > this > > lock? Internals in the PLT (potential race between > > OnNewTransportFeedbackVector() and GetPacketLossRate(), or some subtle > > dependency between the PLT and the voe::Channel? > > PLT internals. OnPacketAdded() and OnNewTransportFeedbackVector() are called on > separate threads. I'll move the channel_proxy_ out of the critical section, > though. This one is interesting and tricky. On one hand I'd argue that since the shared resource lives in the PLT, that's where the lock should be. We cannot know at this level what the PLT functions may access. Another argument for keeping the lock in the PLT is to grab it for the shortest time possible, and only in the PLT do we know exactly what resource is accessed from multiple threads. We'd then also need to add thread checkers in the PLT (which may be a good idea anyway, given the ambiguous threading situation). On the other hand, I could also argue the same way it appears that you have: let the PLT make no promises about thread safety and handle that in the client. This is appealing because why should we impose a threading model on a class unnecessarily? But, IIUC OnNewTransportFeedbackVector() and OnPacketAdded() are only called from SendSideCongestionController, so by the same reasoning, why should SSCC assume that the TransportFeedbackAdapterObserver is anything but single threaded? So then the lock should be in SSCC. In other words, it seems AudioSendStream is not the right place for this lock. Either you should add it in the caller, SSCC, or you should add a lock to PLT (which is the approach most WebRTC code takes). https://codereview.webrtc.org/2638083002/diff/380001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/transport_feedback_adapter.cc (right): https://codereview.webrtc.org/2638083002/diff/380001/webrtc/modules/congestio... webrtc/modules/congestion_controller/transport_feedback_adapter.cc:60: observers_.end()); On 2017/03/22 14:34:47, elad.alon wrote: > On 2017/03/22 12:14:48, minyue-webrtc wrote: > > On 2017/03/22 12:03:01, stefan-webrtc wrote: > > > This seems redundant to me. Should be enough to check that we only add an > > > observer once and that it exists when we remove it. > > > > I suggested it. It is clearly redundant, but just an extra dcheck, in case > > observers_ is touched at other place than Register... > > > > I am fine if you strongly advice removing it. > > Waiting for further input. Agree that we don't need to unit test std::vector::erase() here.
https://codereview.webrtc.org/2638083002/diff/380001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2638083002/diff/380001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:266: // TODO(elad.alon): Take care of the following known issue - this could On 2017/03/23 11:11:14, the sun wrote: > On 2017/03/22 16:34:19, elad.alon wrote: > > On 2017/03/22 16:27:40, minyue-webrtc wrote: > > > On 2017/03/22 14:34:46, elad.alon wrote: > > > > On 2017/03/22 12:06:42, the sun wrote: > > > > > Which following issues? > > > > > > > > The one immediately after the hyphen. Minyue has asked me to use the > > > imperative > > > > mood when formulating TODOs. (The "this" refers to the function call right > > > above > > > > the TODO. Moved it to before and reworded a bit now.) > > > > > > The work "following" makes people think an issue outside the TODO. So > > > > > > Take care of the issue that, after OnPacketAdded() xxx can become unknown, > but > > > currently such information is not passed down. > > > > I think the wording of the latest patchset (PS24) is clear enough. I'll keep > > that, unless someone has a strong objection. > > The wording has not changed in the sense that the use of hyphen is still > confusing. In fact, you could just remove the "Take care of the following known > issue" part - TODO() already means that. Same goes below ("Resolve the following > known issue"). Minyue, I'm going to take up this suggestion, as it fits my personal preference. I don't think TODOs have to be formulated in the imperative. The presence of a TODO, followed by a description of a problem, implicitly indicates the problem is supposed to be taken care of at a later time. https://codereview.webrtc.org/2638083002/diff/380001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:274: rtc::CritScope lock(&packet_loss_tracker_cs_); On 2017/03/23 11:11:14, the sun wrote: > On 2017/03/22 14:34:46, elad.alon wrote: > > On 2017/03/22 12:06:42, the sun wrote: > > > packet_loss_tracker_ and channel_proxy_ are both set up in ctor and never > > > changed during the lifetime of the stream. So, what are you protecting with > > this > > > lock? Internals in the PLT (potential race between > > > OnNewTransportFeedbackVector() and GetPacketLossRate(), or some subtle > > > dependency between the PLT and the voe::Channel? > > > > PLT internals. OnPacketAdded() and OnNewTransportFeedbackVector() are called > on > > separate threads. I'll move the channel_proxy_ out of the critical section, > > though. > > This one is interesting and tricky. > > On one hand I'd argue that since the shared resource lives in the PLT, that's > where the lock should be. We cannot know at this level what the PLT functions > may access. Another argument for keeping the lock in the PLT is to grab it for > the shortest time possible, and only in the PLT do we know exactly what resource > is accessed from multiple threads. We'd then also need to add thread checkers in > the PLT (which may be a good idea anyway, given the ambiguous threading > situation). > > On the other hand, I could also argue the same way it appears that you have: let > the PLT make no promises about thread safety and handle that in the client. This > is appealing because why should we impose a threading model on a class > unnecessarily? But, IIUC OnNewTransportFeedbackVector() and OnPacketAdded() are > only called from SendSideCongestionController, so by the same reasoning, why > should SSCC assume that the TransportFeedbackAdapterObserver is anything but > single threaded? So then the lock should be in SSCC. > > In other words, it seems AudioSendStream is not the right place for this lock. > Either you should add it in the caller, SSCC, or you should add a lock to PLT > (which is the approach most WebRTC code takes). One additional thing to consider is that the CL immediately on top of this one introduces a read of the RPLR. The two metrics are read using separate functions, which I think is more appropriate than reading both metrics at the same time. By placing the lock here, we reap two rewards: 1. Lesser reward - only need to acquire the lock a single time. 2. Greater reward - the state is guaranteed to not change between reading the PLR and RPLR, so someone who looks at their relation could not get a distorted picture. You might argue that this is solvable using an API that reads both at the same time, but something about that feels a bit off to me. Wdyt, though? https://codereview.webrtc.org/2638083002/diff/380001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/transport_feedback_adapter.cc (right): https://codereview.webrtc.org/2638083002/diff/380001/webrtc/modules/congestio... webrtc/modules/congestion_controller/transport_feedback_adapter.cc:60: observers_.end()); On 2017/03/23 11:11:14, the sun wrote: > On 2017/03/22 14:34:47, elad.alon wrote: > > On 2017/03/22 12:14:48, minyue-webrtc wrote: > > > On 2017/03/22 12:03:01, stefan-webrtc wrote: > > > > This seems redundant to me. Should be enough to check that we only add an > > > > observer once and that it exists when we remove it. > > > > > > I suggested it. It is clearly redundant, but just an extra dcheck, in case > > > observers_ is touched at other place than Register... > > > > > > I am fine if you strongly advice removing it. > > > > Waiting for further input. > > Agree that we don't need to unit test std::vector::erase() here. Removed.
https://codereview.webrtc.org/2638083002/diff/380001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2638083002/diff/380001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:266: // TODO(elad.alon): Take care of the following known issue - this could On 2017/03/23 13:37:30, elad.alon wrote: > On 2017/03/23 11:11:14, the sun wrote: > > On 2017/03/22 16:34:19, elad.alon wrote: > > > On 2017/03/22 16:27:40, minyue-webrtc wrote: > > > > On 2017/03/22 14:34:46, elad.alon wrote: > > > > > On 2017/03/22 12:06:42, the sun wrote: > > > > > > Which following issues? > > > > > > > > > > The one immediately after the hyphen. Minyue has asked me to use the > > > > imperative > > > > > mood when formulating TODOs. (The "this" refers to the function call > right > > > > above > > > > > the TODO. Moved it to before and reworded a bit now.) > > > > > > > > The work "following" makes people think an issue outside the TODO. So > > > > > > > > Take care of the issue that, after OnPacketAdded() xxx can become unknown, > > but > > > > currently such information is not passed down. > > > > > > I think the wording of the latest patchset (PS24) is clear enough. I'll keep > > > that, unless someone has a strong objection. > > > > The wording has not changed in the sense that the use of hyphen is still > > confusing. In fact, you could just remove the "Take care of the following > known > > issue" part - TODO() already means that. Same goes below ("Resolve the > following > > known issue"). > > Minyue, I'm going to take up this suggestion, as it fits my personal preference. > I don't think TODOs have to be formulated in the imperative. The presence of a > TODO, followed by a description of a problem, implicitly indicates the problem > is supposed to be taken care of at a later time. You are to make it // TODO(elad.alon): This could potentially reset the window, setting both PLR and RPLR to unknown. ? This is a fact, I don't know what the action to it will be. You may need to add "Consider signal such an event to lower layer." https://codereview.webrtc.org/2638083002/diff/380001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:274: rtc::CritScope lock(&packet_loss_tracker_cs_); On 2017/03/23 13:37:30, elad.alon wrote: > On 2017/03/23 11:11:14, the sun wrote: > > On 2017/03/22 14:34:46, elad.alon wrote: > > > On 2017/03/22 12:06:42, the sun wrote: > > > > packet_loss_tracker_ and channel_proxy_ are both set up in ctor and never > > > > changed during the lifetime of the stream. So, what are you protecting > with > > > this > > > > lock? Internals in the PLT (potential race between > > > > OnNewTransportFeedbackVector() and GetPacketLossRate(), or some subtle > > > > dependency between the PLT and the voe::Channel? > > > > > > PLT internals. OnPacketAdded() and OnNewTransportFeedbackVector() are called > > on > > > separate threads. I'll move the channel_proxy_ out of the critical section, > > > though. > > > > This one is interesting and tricky. > > > > On one hand I'd argue that since the shared resource lives in the PLT, that's > > where the lock should be. We cannot know at this level what the PLT functions > > may access. Another argument for keeping the lock in the PLT is to grab it for > > the shortest time possible, and only in the PLT do we know exactly what > resource > > is accessed from multiple threads. We'd then also need to add thread checkers > in > > the PLT (which may be a good idea anyway, given the ambiguous threading > > situation). > > > > On the other hand, I could also argue the same way it appears that you have: > let > > the PLT make no promises about thread safety and handle that in the client. > This > > is appealing because why should we impose a threading model on a class > > unnecessarily? But, IIUC OnNewTransportFeedbackVector() and OnPacketAdded() > are > > only called from SendSideCongestionController, so by the same reasoning, why > > should SSCC assume that the TransportFeedbackAdapterObserver is anything but > > single threaded? So then the lock should be in SSCC. > > > > In other words, it seems AudioSendStream is not the right place for this lock. > > Either you should add it in the caller, SSCC, or you should add a lock to PLT > > (which is the approach most WebRTC code takes). > > One additional thing to consider is that the CL immediately on top of this one > introduces a read of the RPLR. The two metrics are read using separate > functions, which I think is more appropriate than reading both metrics at the > same time. > > By placing the lock here, we reap two rewards: > 1. Lesser reward - only need to acquire the lock a single time. > 2. Greater reward - the state is guaranteed to not change between reading the > PLR and RPLR, so someone who looks at their relation could not get a distorted > picture. The "greater reward" isn't big. It is not a big deal if they are queried at slightly different time. (if they are to be compared in a stricter sense, it might be an issue, but not the case now). > > You might argue that this is solvable using an API that reads both at the same > time, but something about that feels a bit off to me. Wdyt, though? I also like letting PLT to handle data races.
On 2017/03/23 13:57:16, minyue-webrtc wrote: > https://codereview.webrtc.org/2638083002/diff/380001/webrtc/audio/audio_send_... > File webrtc/audio/audio_send_stream.cc (right): > > https://codereview.webrtc.org/2638083002/diff/380001/webrtc/audio/audio_send_... > webrtc/audio/audio_send_stream.cc:266: // TODO(elad.alon): Take care of the > following known issue - this could > On 2017/03/23 13:37:30, elad.alon wrote: > > On 2017/03/23 11:11:14, the sun wrote: > > > On 2017/03/22 16:34:19, elad.alon wrote: > > > > On 2017/03/22 16:27:40, minyue-webrtc wrote: > > > > > On 2017/03/22 14:34:46, elad.alon wrote: > > > > > > On 2017/03/22 12:06:42, the sun wrote: > > > > > > > Which following issues? > > > > > > > > > > > > The one immediately after the hyphen. Minyue has asked me to use the > > > > > imperative > > > > > > mood when formulating TODOs. (The "this" refers to the function call > > right > > > > > above > > > > > > the TODO. Moved it to before and reworded a bit now.) > > > > > > > > > > The work "following" makes people think an issue outside the TODO. So > > > > > > > > > > Take care of the issue that, after OnPacketAdded() xxx can become > unknown, > > > but > > > > > currently such information is not passed down. > > > > > > > > I think the wording of the latest patchset (PS24) is clear enough. I'll > keep > > > > that, unless someone has a strong objection. > > > > > > The wording has not changed in the sense that the use of hyphen is still > > > confusing. In fact, you could just remove the "Take care of the following > > known > > > issue" part - TODO() already means that. Same goes below ("Resolve the > > following > > > known issue"). > > > > Minyue, I'm going to take up this suggestion, as it fits my personal > preference. > > I don't think TODOs have to be formulated in the imperative. The presence of a > > TODO, followed by a description of a problem, implicitly indicates the problem > > is supposed to be taken care of at a later time. > > You are to make it > // TODO(elad.alon): This could potentially reset the window, setting both PLR > and RPLR to unknown. > ? > This is a fact, I don't know what the action to it will be. You may need to add > "Consider signal such an event to lower layer." > > https://codereview.webrtc.org/2638083002/diff/380001/webrtc/audio/audio_send_... > webrtc/audio/audio_send_stream.cc:274: rtc::CritScope > lock(&packet_loss_tracker_cs_); > On 2017/03/23 13:37:30, elad.alon wrote: > > On 2017/03/23 11:11:14, the sun wrote: > > > On 2017/03/22 14:34:46, elad.alon wrote: > > > > On 2017/03/22 12:06:42, the sun wrote: > > > > > packet_loss_tracker_ and channel_proxy_ are both set up in ctor and > never > > > > > changed during the lifetime of the stream. So, what are you protecting > > with > > > > this > > > > > lock? Internals in the PLT (potential race between > > > > > OnNewTransportFeedbackVector() and GetPacketLossRate(), or some subtle > > > > > dependency between the PLT and the voe::Channel? > > > > > > > > PLT internals. OnPacketAdded() and OnNewTransportFeedbackVector() are > called > > > on > > > > separate threads. I'll move the channel_proxy_ out of the critical > section, > > > > though. > > > > > > This one is interesting and tricky. > > > > > > On one hand I'd argue that since the shared resource lives in the PLT, > that's > > > where the lock should be. We cannot know at this level what the PLT > functions > > > may access. Another argument for keeping the lock in the PLT is to grab it > for > > > the shortest time possible, and only in the PLT do we know exactly what > > resource > > > is accessed from multiple threads. We'd then also need to add thread > checkers > > in > > > the PLT (which may be a good idea anyway, given the ambiguous threading > > > situation). > > > > > > On the other hand, I could also argue the same way it appears that you have: > > let > > > the PLT make no promises about thread safety and handle that in the client. > > This > > > is appealing because why should we impose a threading model on a class > > > unnecessarily? But, IIUC OnNewTransportFeedbackVector() and OnPacketAdded() > > are > > > only called from SendSideCongestionController, so by the same reasoning, why > > > should SSCC assume that the TransportFeedbackAdapterObserver is anything but > > > single threaded? So then the lock should be in SSCC. > > > > > > In other words, it seems AudioSendStream is not the right place for this > lock. > > > Either you should add it in the caller, SSCC, or you should add a lock to > PLT > > > (which is the approach most WebRTC code takes). > > > > One additional thing to consider is that the CL immediately on top of this one > > introduces a read of the RPLR. The two metrics are read using separate > > functions, which I think is more appropriate than reading both metrics at the > > same time. > > > > By placing the lock here, we reap two rewards: > > 1. Lesser reward - only need to acquire the lock a single time. > > 2. Greater reward - the state is guaranteed to not change between reading the > > PLR and RPLR, so someone who looks at their relation could not get a distorted > > picture. > The "greater reward" isn't big. It is not a big deal if they are queried at > slightly different time. (if they are to be compared in a stricter sense, it > might be an issue, but not the case now). > > > > You might argue that this is solvable using an API that reads both at the same > > time, but something about that feels a bit off to me. Wdyt, though? > > I also like letting PLT to handle data races. Me and Elad just talked this over offline and I was agreeing to have the lock in AudioSendStream. If, as you say Minyue, there is no requirement on strictness as mentioned in #2, then the lock should probably go in the PLT.
On 2017/03/23 14:15:32, the sun wrote: > On 2017/03/23 13:57:16, minyue-webrtc wrote: > > > https://codereview.webrtc.org/2638083002/diff/380001/webrtc/audio/audio_send_... > > File webrtc/audio/audio_send_stream.cc (right): > > > > > https://codereview.webrtc.org/2638083002/diff/380001/webrtc/audio/audio_send_... > > webrtc/audio/audio_send_stream.cc:266: // TODO(elad.alon): Take care of the > > following known issue - this could > > On 2017/03/23 13:37:30, elad.alon wrote: > > > On 2017/03/23 11:11:14, the sun wrote: > > > > On 2017/03/22 16:34:19, elad.alon wrote: > > > > > On 2017/03/22 16:27:40, minyue-webrtc wrote: > > > > > > On 2017/03/22 14:34:46, elad.alon wrote: > > > > > > > On 2017/03/22 12:06:42, the sun wrote: > > > > > > > > Which following issues? > > > > > > > > > > > > > > The one immediately after the hyphen. Minyue has asked me to use the > > > > > > imperative > > > > > > > mood when formulating TODOs. (The "this" refers to the function call > > > right > > > > > > above > > > > > > > the TODO. Moved it to before and reworded a bit now.) > > > > > > > > > > > > The work "following" makes people think an issue outside the TODO. So > > > > > > > > > > > > Take care of the issue that, after OnPacketAdded() xxx can become > > unknown, > > > > but > > > > > > currently such information is not passed down. > > > > > > > > > > I think the wording of the latest patchset (PS24) is clear enough. I'll > > keep > > > > > that, unless someone has a strong objection. > > > > > > > > The wording has not changed in the sense that the use of hyphen is still > > > > confusing. In fact, you could just remove the "Take care of the following > > > known > > > > issue" part - TODO() already means that. Same goes below ("Resolve the > > > following > > > > known issue"). > > > > > > Minyue, I'm going to take up this suggestion, as it fits my personal > > preference. > > > I don't think TODOs have to be formulated in the imperative. The presence of > a > > > TODO, followed by a description of a problem, implicitly indicates the > problem > > > is supposed to be taken care of at a later time. > > > > You are to make it > > // TODO(elad.alon): This could potentially reset the window, setting both PLR > > and RPLR to unknown. > > ? > > This is a fact, I don't know what the action to it will be. You may need to > add > > "Consider signal such an event to lower layer." > > > > > https://codereview.webrtc.org/2638083002/diff/380001/webrtc/audio/audio_send_... > > webrtc/audio/audio_send_stream.cc:274: rtc::CritScope > > lock(&packet_loss_tracker_cs_); > > On 2017/03/23 13:37:30, elad.alon wrote: > > > On 2017/03/23 11:11:14, the sun wrote: > > > > On 2017/03/22 14:34:46, elad.alon wrote: > > > > > On 2017/03/22 12:06:42, the sun wrote: > > > > > > packet_loss_tracker_ and channel_proxy_ are both set up in ctor and > > never > > > > > > changed during the lifetime of the stream. So, what are you protecting > > > with > > > > > this > > > > > > lock? Internals in the PLT (potential race between > > > > > > OnNewTransportFeedbackVector() and GetPacketLossRate(), or some subtle > > > > > > dependency between the PLT and the voe::Channel? > > > > > > > > > > PLT internals. OnPacketAdded() and OnNewTransportFeedbackVector() are > > called > > > > on > > > > > separate threads. I'll move the channel_proxy_ out of the critical > > section, > > > > > though. > > > > > > > > This one is interesting and tricky. > > > > > > > > On one hand I'd argue that since the shared resource lives in the PLT, > > that's > > > > where the lock should be. We cannot know at this level what the PLT > > functions > > > > may access. Another argument for keeping the lock in the PLT is to grab it > > for > > > > the shortest time possible, and only in the PLT do we know exactly what > > > resource > > > > is accessed from multiple threads. We'd then also need to add thread > > checkers > > > in > > > > the PLT (which may be a good idea anyway, given the ambiguous threading > > > > situation). > > > > > > > > On the other hand, I could also argue the same way it appears that you > have: > > > let > > > > the PLT make no promises about thread safety and handle that in the > client. > > > This > > > > is appealing because why should we impose a threading model on a class > > > > unnecessarily? But, IIUC OnNewTransportFeedbackVector() and > OnPacketAdded() > > > are > > > > only called from SendSideCongestionController, so by the same reasoning, > why > > > > should SSCC assume that the TransportFeedbackAdapterObserver is anything > but > > > > single threaded? So then the lock should be in SSCC. > > > > > > > > In other words, it seems AudioSendStream is not the right place for this > > lock. > > > > Either you should add it in the caller, SSCC, or you should add a lock to > > PLT > > > > (which is the approach most WebRTC code takes). > > > > > > One additional thing to consider is that the CL immediately on top of this > one > > > introduces a read of the RPLR. The two metrics are read using separate > > > functions, which I think is more appropriate than reading both metrics at > the > > > same time. > > > > > > By placing the lock here, we reap two rewards: > > > 1. Lesser reward - only need to acquire the lock a single time. > > > 2. Greater reward - the state is guaranteed to not change between reading > the > > > PLR and RPLR, so someone who looks at their relation could not get a > distorted > > > picture. > > The "greater reward" isn't big. It is not a big deal if they are queried at > > slightly different time. (if they are to be compared in a stricter sense, it > > might be an issue, but not the case now). > > > > > > You might argue that this is solvable using an API that reads both at the > same > > > time, but something about that feels a bit off to me. Wdyt, though? > > > > I also like letting PLT to handle data races. > > Me and Elad just talked this over offline and I was agreeing to have the lock in > AudioSendStream. If, as you say Minyue, there is no requirement on strictness as > mentioned in #2, then the lock should probably go in the PLT. I have no objection to locking inside of PLT, except for the read-PLR-then-RPLR issue. Shall we add an interface to read both at the same time, then? If so, should it be instead of the current single-use interfaces, or in addition?
On 2017/03/23 14:17:58, elad.alon wrote: > On 2017/03/23 14:15:32, the sun wrote: > > On 2017/03/23 13:57:16, minyue-webrtc wrote: > > > > > > https://codereview.webrtc.org/2638083002/diff/380001/webrtc/audio/audio_send_... > > > File webrtc/audio/audio_send_stream.cc (right): > > > > > > > > > https://codereview.webrtc.org/2638083002/diff/380001/webrtc/audio/audio_send_... > > > webrtc/audio/audio_send_stream.cc:266: // TODO(elad.alon): Take care of the > > > following known issue - this could > > > On 2017/03/23 13:37:30, elad.alon wrote: > > > > On 2017/03/23 11:11:14, the sun wrote: > > > > > On 2017/03/22 16:34:19, elad.alon wrote: > > > > > > On 2017/03/22 16:27:40, minyue-webrtc wrote: > > > > > > > On 2017/03/22 14:34:46, elad.alon wrote: > > > > > > > > On 2017/03/22 12:06:42, the sun wrote: > > > > > > > > > Which following issues? > > > > > > > > > > > > > > > > The one immediately after the hyphen. Minyue has asked me to use > the > > > > > > > imperative > > > > > > > > mood when formulating TODOs. (The "this" refers to the function > call > > > > right > > > > > > > above > > > > > > > > the TODO. Moved it to before and reworded a bit now.) > > > > > > > > > > > > > > The work "following" makes people think an issue outside the TODO. > So > > > > > > > > > > > > > > Take care of the issue that, after OnPacketAdded() xxx can become > > > unknown, > > > > > but > > > > > > > currently such information is not passed down. > > > > > > > > > > > > I think the wording of the latest patchset (PS24) is clear enough. > I'll > > > keep > > > > > > that, unless someone has a strong objection. > > > > > > > > > > The wording has not changed in the sense that the use of hyphen is still > > > > > confusing. In fact, you could just remove the "Take care of the > following > > > > known > > > > > issue" part - TODO() already means that. Same goes below ("Resolve the > > > > following > > > > > known issue"). > > > > > > > > Minyue, I'm going to take up this suggestion, as it fits my personal > > > preference. > > > > I don't think TODOs have to be formulated in the imperative. The presence > of > > a > > > > TODO, followed by a description of a problem, implicitly indicates the > > problem > > > > is supposed to be taken care of at a later time. > > > > > > You are to make it > > > // TODO(elad.alon): This could potentially reset the window, setting both > PLR > > > and RPLR to unknown. > > > ? > > > This is a fact, I don't know what the action to it will be. You may need to > > add > > > "Consider signal such an event to lower layer." > > > > > > > > > https://codereview.webrtc.org/2638083002/diff/380001/webrtc/audio/audio_send_... > > > webrtc/audio/audio_send_stream.cc:274: rtc::CritScope > > > lock(&packet_loss_tracker_cs_); > > > On 2017/03/23 13:37:30, elad.alon wrote: > > > > On 2017/03/23 11:11:14, the sun wrote: > > > > > On 2017/03/22 14:34:46, elad.alon wrote: > > > > > > On 2017/03/22 12:06:42, the sun wrote: > > > > > > > packet_loss_tracker_ and channel_proxy_ are both set up in ctor and > > > never > > > > > > > changed during the lifetime of the stream. So, what are you > protecting > > > > with > > > > > > this > > > > > > > lock? Internals in the PLT (potential race between > > > > > > > OnNewTransportFeedbackVector() and GetPacketLossRate(), or some > subtle > > > > > > > dependency between the PLT and the voe::Channel? > > > > > > > > > > > > PLT internals. OnPacketAdded() and OnNewTransportFeedbackVector() are > > > called > > > > > on > > > > > > separate threads. I'll move the channel_proxy_ out of the critical > > > section, > > > > > > though. > > > > > > > > > > This one is interesting and tricky. > > > > > > > > > > On one hand I'd argue that since the shared resource lives in the PLT, > > > that's > > > > > where the lock should be. We cannot know at this level what the PLT > > > functions > > > > > may access. Another argument for keeping the lock in the PLT is to grab > it > > > for > > > > > the shortest time possible, and only in the PLT do we know exactly what > > > > resource > > > > > is accessed from multiple threads. We'd then also need to add thread > > > checkers > > > > in > > > > > the PLT (which may be a good idea anyway, given the ambiguous threading > > > > > situation). > > > > > > > > > > On the other hand, I could also argue the same way it appears that you > > have: > > > > let > > > > > the PLT make no promises about thread safety and handle that in the > > client. > > > > This > > > > > is appealing because why should we impose a threading model on a class > > > > > unnecessarily? But, IIUC OnNewTransportFeedbackVector() and > > OnPacketAdded() > > > > are > > > > > only called from SendSideCongestionController, so by the same reasoning, > > why > > > > > should SSCC assume that the TransportFeedbackAdapterObserver is anything > > but > > > > > single threaded? So then the lock should be in SSCC. > > > > > > > > > > In other words, it seems AudioSendStream is not the right place for this > > > lock. > > > > > Either you should add it in the caller, SSCC, or you should add a lock > to > > > PLT > > > > > (which is the approach most WebRTC code takes). > > > > > > > > One additional thing to consider is that the CL immediately on top of this > > one > > > > introduces a read of the RPLR. The two metrics are read using separate > > > > functions, which I think is more appropriate than reading both metrics at > > the > > > > same time. > > > > > > > > By placing the lock here, we reap two rewards: > > > > 1. Lesser reward - only need to acquire the lock a single time. > > > > 2. Greater reward - the state is guaranteed to not change between reading > > the > > > > PLR and RPLR, so someone who looks at their relation could not get a > > distorted > > > > picture. > > > The "greater reward" isn't big. It is not a big deal if they are queried at > > > slightly different time. (if they are to be compared in a stricter sense, it > > > might be an issue, but not the case now). > > > > > > > > You might argue that this is solvable using an API that reads both at the > > same > > > > time, but something about that feels a bit off to me. Wdyt, though? > > > > > > I also like letting PLT to handle data races. > > > > Me and Elad just talked this over offline and I was agreeing to have the lock > in > > AudioSendStream. If, as you say Minyue, there is no requirement on strictness > as > > mentioned in #2, then the lock should probably go in the PLT. > > I have no objection to locking inside of PLT, except for the read-PLR-then-RPLR > issue. Shall we add an interface to read both at the same time, then? If so, > should it be instead of the current single-use interfaces, or in addition? I let Minyue decide; lock in AudioSendStream or PLT is ok. LGTM
The CQ bit was checked by elad.alon@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from minyue@webrtc.org, michaelt@webrtc.org, terelius@webrtc.org, solenberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/2638083002/#ps480001 (title: "Rebased (and minor changes to comments)")
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: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/15294)
The CQ bit was checked by elad.alon@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_ubsan_vptr on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...)
The CQ bit was checked by elad.alon@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: This issue passed the CQ dry run.
lgtm Feel free to add the test separately if you agree that it makes sense to have it. https://codereview.webrtc.org/2638083002/diff/500001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/congestion_controller_unittest.cc (right): https://codereview.webrtc.org/2638083002/diff/500001/webrtc/modules/congestio... webrtc/modules/congestion_controller/congestion_controller_unittest.cc:82: constexpr uint32_t ssrc = 0; I think it would make sense to have a unittest which verifies that this actually is stored correctly.
The CQ bit was checked by elad.alon@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaelt@webrtc.org, terelius@webrtc.org, minyue@webrtc.org, solenberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/2638083002/#ps500001 (title: "Fix UT")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
https://codereview.webrtc.org/2638083002/diff/380001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2638083002/diff/380001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:266: // TODO(elad.alon): Take care of the following known issue - this could On 2017/03/23 13:57:16, minyue-webrtc wrote: > On 2017/03/23 13:37:30, elad.alon wrote: > > On 2017/03/23 11:11:14, the sun wrote: > > > On 2017/03/22 16:34:19, elad.alon wrote: > > > > On 2017/03/22 16:27:40, minyue-webrtc wrote: > > > > > On 2017/03/22 14:34:46, elad.alon wrote: > > > > > > On 2017/03/22 12:06:42, the sun wrote: > > > > > > > Which following issues? > > > > > > > > > > > > The one immediately after the hyphen. Minyue has asked me to use the > > > > > imperative > > > > > > mood when formulating TODOs. (The "this" refers to the function call > > right > > > > > above > > > > > > the TODO. Moved it to before and reworded a bit now.) > > > > > > > > > > The work "following" makes people think an issue outside the TODO. So > > > > > > > > > > Take care of the issue that, after OnPacketAdded() xxx can become > unknown, > > > but > > > > > currently such information is not passed down. > > > > > > > > I think the wording of the latest patchset (PS24) is clear enough. I'll > keep > > > > that, unless someone has a strong objection. > > > > > > The wording has not changed in the sense that the use of hyphen is still > > > confusing. In fact, you could just remove the "Take care of the following > > known > > > issue" part - TODO() already means that. Same goes below ("Resolve the > > following > > > known issue"). > > > > Minyue, I'm going to take up this suggestion, as it fits my personal > preference. > > I don't think TODOs have to be formulated in the imperative. The presence of a > > TODO, followed by a description of a problem, implicitly indicates the problem > > is supposed to be taken care of at a later time. > > You are to make it > // TODO(elad.alon): This could potentially reset the window, setting both PLR > and RPLR to unknown. > ? > This is a fact, I don't know what the action to it will be. You may need to add > "Consider signal such an event to lower layer." OK https://codereview.webrtc.org/2638083002/diff/500001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/congestion_controller_unittest.cc (right): https://codereview.webrtc.org/2638083002/diff/500001/webrtc/modules/congestio... webrtc/modules/congestion_controller/congestion_controller_unittest.cc:82: constexpr uint32_t ssrc = 0; On 2017/03/23 17:55:06, stefan-webrtc wrote: > I think it would make sense to have a unittest which verifies that this actually > is stored correctly. We don't store this, but rather make sure that we don't do anything in AudioSendStream::OnSentPacket() if the supplied SSRC in not the same as the stream's configured SSRC. So the AddPacket() interface just pipes it. But maybe I've misunderstood you?
CQ is committing da patch.
Bot data: {"patchset_id": 500001, "attempt_start_ts": 1490292089286520,
"parent_rev": "7b3ce5b87239c50bf6a3f1a36099c32176bbe45e", "commit_rev":
"d12a8e1c8eff2f721da702b4d22c44d7d7b1a13a"}
Message was sent while issue was closed.
Description was changed from ========== Attach TransportFeedbackPacketLossTracker to ANA (PLR only) This CL is one in a series. To finish the work, the following CLs will be added: 1. CL for connecting RPLR as well 2. CL for RPLR-based FecController 3. CL for allowing experiment-driven configuration of the above (through both field-trials and protobuf) BUG=webrtc:7058 ========== to ========== Attach TransportFeedbackPacketLossTracker to ANA (PLR only) This CL is one in a series. To finish the work, the following CLs will be added: 1. CL for connecting RPLR as well 2. CL for RPLR-based FecController 3. CL for allowing experiment-driven configuration of the above (through both field-trials and protobuf) BUG=webrtc:7058 Review-Url: https://codereview.webrtc.org/2638083002 Cr-Commit-Position: refs/heads/master@{#17365} Committed: https://chromium.googlesource.com/external/webrtc/+/d12a8e1c8eff2f721da702b4d... ==========
Message was sent while issue was closed.
Committed patchset #26 (id:500001) as https://chromium.googlesource.com/external/webrtc/+/d12a8e1c8eff2f721da702b4d... |
