Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1266033005:
Added send-thresholding and fixed text packet dumping.
DescriptionAdded send-thresholding and fixed text packet dumping. Also a little squelch for the over-max-MTU log spam we see in there.
BUG=https://code.google.com/p/webrtc/issues/detail?id=4468
R=pthatcher@chromium.org, pthatcher@webrtc.org
Committed: https://chromium.googlesource.com/external/webrtc/+/d838d2791979bb50f464a61c557d55c6a324621e
Patch Set 1 #
Total comments: 18
Patch Set 2 : Response to pthatcher@ comments. #
Total comments: 12
Patch Set 3 : A bit more cleanup. #
Total comments: 8
Patch Set 4 : Added a test and more pthatcher@ comments. #
Total comments: 19
Patch Set 5 : With a critsec around the vector's use, and the vector static init delayed until first use. #Patch Set 6 : Took out the vector, moved to usrsctp_getladdrs. No more critsecs. #Patch Set 7 : (Post rebase on master) #
Messages
Total messages: 32 (10 generated)
lally@webrtc.org changed reviewers: + bemasc@webrtc.org, pthatcher@webrtc.org
On 2015/08/03 20:40:03, lally1 wrote: > mailto:lally@webrtc.org changed reviewers: > + mailto:bemasc@webrtc.org, mailto:pthatcher@webrtc.org Maybe this doesn't send email when I add reviewers? Odd.
I think you have to hit "publish" even if you add reviewers.
On 2015/08/05 21:28:17, pthatcher1 wrote: > I think you have to hit "publish" even if you add reviewers. In any case, I didn't get it until now.
https://codereview.webrtc.org/1266033005/diff/1/talk/media/sctp/sctpdataengin... File talk/media/sctp/sctpdataengine.cc (right): https://codereview.webrtc.org/1266033005/diff/1/talk/media/sctp/sctpdataengin... talk/media/sctp/sctpdataengine.cc:348: int SctpDataMediaChannel::SendThresholdCallback(struct socket* sock, I think it would make more sense to put this callback on the engine as a non-static method. https://codereview.webrtc.org/1266033005/diff/1/talk/media/sctp/sctpdataengin... talk/media/sctp/sctpdataengine.cc:349: uint32_t sb_free) { No ulp_info, like with reading a packet? That's lame. https://codereview.webrtc.org/1266033005/diff/1/talk/media/sctp/sctpdataengin... talk/media/sctp/sctpdataengine.cc:350: SocketChannelMap::iterator it = sock_channel_map_.find(sock); You can use auto it = sock_channel_map_.find(sock); And then you don't need to have the SocketChannelMap typedef. https://codereview.webrtc.org/1266033005/diff/1/talk/media/sctp/sctpdataengin... talk/media/sctp/sctpdataengine.cc:351: if (it == sock_channel_map_.end()) { Since there are almost always a very low number of SctpDataMediaChannels at one time (1 or 2), it would be usually be more efficient and perhaps easier to manager to just iterate over a vector of channels to find the one that has the matching socket. https://codereview.webrtc.org/1266033005/diff/1/talk/media/sctp/sctpdataengin... talk/media/sctp/sctpdataengine.cc:470: } I think this works, and it shorter and only does one lookup: if (!sock_channel_map_.erase(sock_)) { LOG(LS_ERROR) << "CloseSctpSocket: the socket wasn't registered."; } https://codereview.webrtc.org/1266033005/diff/1/talk/media/sctp/sctpdataengin... talk/media/sctp/sctpdataengine.cc:939: const int kSctpOverhead = 76; This could use a little more documentation, even if to explain why it can't be 0. https://codereview.webrtc.org/1266033005/diff/1/talk/media/sctp/sctpdataengin... talk/media/sctp/sctpdataengine.cc:944: << " vs max of " << kSctpMtu; Should probably include something like "assuming X bytes of SCTP overhead". https://codereview.webrtc.org/1266033005/diff/1/talk/media/sctp/sctpdataengine.h File talk/media/sctp/sctpdataengine.h (right): https://codereview.webrtc.org/1266033005/diff/1/talk/media/sctp/sctpdataengin... talk/media/sctp/sctpdataengine.h:200: static SocketChannelMap sock_channel_map_; Instead of being a static member of the SctpDataMediaChannel, I think it would be cleaner to make this a member of SctpDataMediaEngine.
Patchset #2 (id:20001) has been deleted
Thanks. I couldn't move the map (now a vector) to SctpDataEngine without adding a pointer back from the Channel to the Engine -- to notify the Engine when the Channel's getting destroyed. But I did make it a vector. I can move it and add the pointer, if you wish. https://codereview.webrtc.org/1266033005/diff/1/talk/media/sctp/sctpdataengin... File talk/media/sctp/sctpdataengine.cc (right): https://codereview.webrtc.org/1266033005/diff/1/talk/media/sctp/sctpdataengin... talk/media/sctp/sctpdataengine.cc:348: int SctpDataMediaChannel::SendThresholdCallback(struct socket* sock, On 2015/08/05 21:42:55, pthatcher1 wrote: > I think it would make more sense to put this callback on the engine as a > non-static method. I don't know where I'd get an instance pointer. https://codereview.webrtc.org/1266033005/diff/1/talk/media/sctp/sctpdataengin... talk/media/sctp/sctpdataengine.cc:349: uint32_t sb_free) { On 2015/08/05 21:42:55, pthatcher1 wrote: > No ulp_info, like with reading a packet? That's lame. Yeah, that's the real issue here. I had a prior version of this CL that used usrsctp_getladdrs(sock,..)+usrsctp_freeladdrs to get the full sockaddr_conn, but that's a full malloc/free cycle. https://codereview.webrtc.org/1266033005/diff/1/talk/media/sctp/sctpdataengin... talk/media/sctp/sctpdataengine.cc:350: SocketChannelMap::iterator it = sock_channel_map_.find(sock); On 2015/08/05 21:42:55, pthatcher1 wrote: > You can use > > auto it = sock_channel_map_.find(sock); > > > And then you don't need to have the SocketChannelMap typedef. Thanks! https://codereview.webrtc.org/1266033005/diff/1/talk/media/sctp/sctpdataengin... talk/media/sctp/sctpdataengine.cc:351: if (it == sock_channel_map_.end()) { On 2015/08/05 21:42:55, pthatcher1 wrote: > Since there are almost always a very low number of SctpDataMediaChannels at one > time (1 or 2), it would be usually be more efficient and perhaps easier to > manager to just iterate over a vector of channels to find the one that has the > matching socket. Done. https://codereview.webrtc.org/1266033005/diff/1/talk/media/sctp/sctpdataengin... talk/media/sctp/sctpdataengine.cc:470: } On 2015/08/05 21:42:55, pthatcher1 wrote: > I think this works, and it shorter and only does one lookup: > > if (!sock_channel_map_.erase(sock_)) { > LOG(LS_ERROR) << "CloseSctpSocket: the socket wasn't registered."; > } Thanks. Moved above into ~SctpDataMediaChannel(), but as it's a vector<>, we can't use this form of erase(). https://codereview.webrtc.org/1266033005/diff/1/talk/media/sctp/sctpdataengin... talk/media/sctp/sctpdataengine.cc:939: const int kSctpOverhead = 76; On 2015/08/05 21:42:55, pthatcher1 wrote: > This could use a little more documentation, even if to explain why it can't be > 0. Done. https://codereview.webrtc.org/1266033005/diff/1/talk/media/sctp/sctpdataengin... talk/media/sctp/sctpdataengine.cc:944: << " vs max of " << kSctpMtu; On 2015/08/05 21:42:55, pthatcher1 wrote: > Should probably include something like "assuming X bytes of SCTP overhead". Done. https://codereview.webrtc.org/1266033005/diff/1/talk/media/sctp/sctpdataengine.h File talk/media/sctp/sctpdataengine.h (right): https://codereview.webrtc.org/1266033005/diff/1/talk/media/sctp/sctpdataengin... talk/media/sctp/sctpdataengine.h:200: static SocketChannelMap sock_channel_map_; On 2015/08/05 21:42:55, pthatcher1 wrote: > Instead of being a static member of the SctpDataMediaChannel, I think it would > be cleaner to make this a member of SctpDataMediaEngine. > That makes a lot of sense, but the SctpDataMediaChannel doesn't have an instance pointer back to the SctpDataMediaEngine. I think we'd need it to notify SctpDataMediaEngine when the Channel's being destroyed. We could certainly pass one in construction. Is it worth it?
https://codereview.webrtc.org/1266033005/diff/1/talk/media/sctp/sctpdataengin... File talk/media/sctp/sctpdataengine.cc (right): https://codereview.webrtc.org/1266033005/diff/1/talk/media/sctp/sctpdataengin... talk/media/sctp/sctpdataengine.cc:348: int SctpDataMediaChannel::SendThresholdCallback(struct socket* sock, The instance pointer to the engine? Could you use rtc::Bind to get a callback that has a pointer to the engine? https://codereview.webrtc.org/1266033005/diff/1/talk/media/sctp/sctpdataengine.h File talk/media/sctp/sctpdataengine.h (right): https://codereview.webrtc.org/1266033005/diff/1/talk/media/sctp/sctpdataengin... talk/media/sctp/sctpdataengine.h:200: static SocketChannelMap sock_channel_map_; On 2015/08/18 20:38:56, lally1 wrote: > On 2015/08/05 21:42:55, pthatcher1 wrote: > > Instead of being a static member of the SctpDataMediaChannel, I think it would > > be cleaner to make this a member of SctpDataMediaEngine. > > > > That makes a lot of sense, but the SctpDataMediaChannel doesn't have an instance > pointer back to the SctpDataMediaEngine. I think we'd need it to notify > SctpDataMediaEngine when the Channel's being destroyed. We could certainly pass > one in construction. Is it worth it? We could add SctpDataMediaChannel::SignalDestroyed that fires in the destructor, and the MediaEngine could listen to that and remove it from the collection. We use a this pattern in other places in our code. Down underneath, that is a pointer back, but in a cleaner way. https://codereview.webrtc.org/1266033005/diff/40001/talk/media/sctp/sctpdatae... File talk/media/sctp/sctpdataengine.cc (right): https://codereview.webrtc.org/1266033005/diff/40001/talk/media/sctp/sctpdatae... talk/media/sctp/sctpdataengine.cc:113: static const int kSendBufferSize = 262144; Can you comment on how this number was chosen? https://codereview.webrtc.org/1266033005/diff/40001/talk/media/sctp/sctpdatae... talk/media/sctp/sctpdataengine.cc:326: std::vector<SctpDataMediaChannel*> SctpDataMediaChannel::sock_channel_map_; Since it's not a map anymore, perhaps channels_ would make more sense as a name? https://codereview.webrtc.org/1266033005/diff/40001/talk/media/sctp/sctpdatae... talk/media/sctp/sctpdataengine.cc:348: } An early return might look a little better: if (it == end()) { LOG(LS_ERROR) << "~SctpDataMediaChannel: the channel wasn't registered."; return; } erase(it); https://codereview.webrtc.org/1266033005/diff/40001/talk/media/sctp/sctpdatae... talk/media/sctp/sctpdataengine.cc:367: if (p->socket() == sock) { Did you mean to put this in GetSctpChannel? https://codereview.webrtc.org/1266033005/diff/40001/talk/media/sctp/sctpdatae... talk/media/sctp/sctpdataengine.cc:386: // something reasonable here. This comment looks like it belongs up by where kSendBufferSize is used. Did you mean to put it there? https://codereview.webrtc.org/1266033005/diff/40001/talk/media/sctp/sctpdatae... File talk/media/sctp/sctpdataengine.h (right): https://codereview.webrtc.org/1266033005/diff/40001/talk/media/sctp/sctpdatae... talk/media/sctp/sctpdataengine.h:229: static SctpDataMediaChannel* GetSctpChannel(struct socket* sock); This looks like it's never called or implemented. Did you mean to remove it?
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
Patchset #3 (id:100001) has been deleted
https://codereview.webrtc.org/1266033005/diff/40001/talk/media/sctp/sctpdatae... File talk/media/sctp/sctpdataengine.cc (right): https://codereview.webrtc.org/1266033005/diff/40001/talk/media/sctp/sctpdatae... talk/media/sctp/sctpdataengine.cc:113: static const int kSendBufferSize = 262144; On 2015/08/18 21:51:52, pthatcher1 wrote: > Can you comment on how this number was chosen? Done. https://codereview.webrtc.org/1266033005/diff/40001/talk/media/sctp/sctpdatae... talk/media/sctp/sctpdataengine.cc:326: std::vector<SctpDataMediaChannel*> SctpDataMediaChannel::sock_channel_map_; On 2015/08/18 21:51:52, pthatcher1 wrote: > Since it's not a map anymore, perhaps channels_ would make more sense as a name? Done. https://codereview.webrtc.org/1266033005/diff/40001/talk/media/sctp/sctpdatae... talk/media/sctp/sctpdataengine.cc:348: } On 2015/08/18 21:51:53, pthatcher1 wrote: > An early return might look a little better: > > if (it == end()) { > LOG(LS_ERROR) << "~SctpDataMediaChannel: the channel wasn't registered."; > return; > } > erase(it); Done, in OnChannelDestroyed(), where this code was moved. https://codereview.webrtc.org/1266033005/diff/40001/talk/media/sctp/sctpdatae... talk/media/sctp/sctpdataengine.cc:367: if (p->socket() == sock) { On 2015/08/18 21:51:52, pthatcher1 wrote: > Did you mean to put this in GetSctpChannel? I took that method out. Did you want it back in? https://codereview.webrtc.org/1266033005/diff/40001/talk/media/sctp/sctpdatae... talk/media/sctp/sctpdataengine.cc:386: // something reasonable here. On 2015/08/18 21:51:52, pthatcher1 wrote: > This comment looks like it belongs up by where kSendBufferSize is used. Did you > mean to put it there? No I just wrote it poorly. Rewritten. https://codereview.webrtc.org/1266033005/diff/40001/talk/media/sctp/sctpdatae... File talk/media/sctp/sctpdataengine.h (right): https://codereview.webrtc.org/1266033005/diff/40001/talk/media/sctp/sctpdatae... talk/media/sctp/sctpdataengine.h:229: static SctpDataMediaChannel* GetSctpChannel(struct socket* sock); On 2015/08/18 21:51:53, pthatcher1 wrote: > This looks like it's never called or implemented. Did you mean to remove it? Yup. Removed.
Nits left for the code. But could we add a unit test that fires the threshold event on the SctpDataEngine and makes sure that the right SctpDataMediaChannel as OnReadyToSend fired? https://codereview.webrtc.org/1266033005/diff/120001/talk/media/sctp/sctpdata... File talk/media/sctp/sctpdataengine.cc (right): https://codereview.webrtc.org/1266033005/diff/120001/talk/media/sctp/sctpdata... talk/media/sctp/sctpdataengine.cc:249: // Our registrar of sockets to their channels. Used for callbacks. This might be more clear as "All the channels created by this engine, used for callbacks from usrsctplib that only contain socket pointers. Channels are removed when SignalDestroyed is fired." https://codereview.webrtc.org/1266033005/diff/120001/talk/media/sctp/sctpdata... talk/media/sctp/sctpdataengine.cc:340: // static Is it really static? https://codereview.webrtc.org/1266033005/diff/120001/talk/media/sctp/sctpdata... talk/media/sctp/sctpdataengine.cc:347: } Would a GetChannelFromSocket method make sense? https://codereview.webrtc.org/1266033005/diff/120001/talk/media/sctp/sctpdata... talk/media/sctp/sctpdataengine.cc:389: // If kSendBufferSize isn't reflective of reality, we log an error, still still => but we still ?
Patchset #4 (id:140001) has been deleted
Ok, how's that? https://codereview.webrtc.org/1266033005/diff/120001/talk/media/sctp/sctpdata... File talk/media/sctp/sctpdataengine.cc (right): https://codereview.webrtc.org/1266033005/diff/120001/talk/media/sctp/sctpdata... talk/media/sctp/sctpdataengine.cc:249: // Our registrar of sockets to their channels. Used for callbacks. On 2015/08/24 19:09:31, pthatcher1 wrote: > This might be more clear as "All the channels created by this engine, used for > callbacks from usrsctplib that only contain socket pointers. Channels are > removed when SignalDestroyed is fired." Done. https://codereview.webrtc.org/1266033005/diff/120001/talk/media/sctp/sctpdata... talk/media/sctp/sctpdataengine.cc:340: // static On 2015/08/24 19:09:31, pthatcher1 wrote: > Is it really static? Yup. https://codereview.webrtc.org/1266033005/diff/120001/talk/media/sctp/sctpdata... talk/media/sctp/sctpdataengine.cc:347: } On 2015/08/24 19:09:31, pthatcher1 wrote: > Would a GetChannelFromSocket method make sense? I added it in. Whether it makes sense: it feels like better practice, even though it adds no utility here. https://codereview.webrtc.org/1266033005/diff/120001/talk/media/sctp/sctpdata... talk/media/sctp/sctpdataengine.cc:389: // If kSendBufferSize isn't reflective of reality, we log an error, still On 2015/08/24 19:09:31, pthatcher1 wrote: > still => but we still > > ? Done.
pthatcher@chromium.org changed reviewers: + pthatcher@chromium.org
lgtm
On 2015/08/25 17:11:13, pthatcher2 wrote: > lgtm Could pthatcher1@ lgtm it too? He's an owner.
On 2015/08/26 16:11:41, lally1 wrote: > On 2015/08/25 17:11:13, pthatcher2 wrote: > > lgtm > > Could pthatcher1@ lgtm it too? He's an owner. lgtm (How many versions of me are there?)
Message was sent while issue was closed.
Committed patchset #4 (id:160001) manually as d838d2791979bb50f464a61c557d55c6a324621e (presubmit successful).
Message was sent while issue was closed.
thakis@chromium.org changed reviewers: + thakis@chromium.org
Message was sent while issue was closed.
https://codereview.webrtc.org/1266033005/diff/160001/talk/media/sctp/sctpdata... File talk/media/sctp/sctpdataengine.h (right): https://codereview.webrtc.org/1266033005/diff/160001/talk/media/sctp/sctpdata... talk/media/sctp/sctpdataengine.h:105: static std::vector<SctpDataMediaChannel*> channels_; This adds a static initializer, that's no good. (I'm not sure why codecs_ doesn't add one too – I guess it's not referenced from any code used in chromium and gets dead-stripped?) # sctpdataengine.cc _GLOBAL__sub_I_sctpdataengine.cc+0xf # sctpdataengine.cc __cxa_atexit@plt [registers a dtor to run at exit]
Message was sent while issue was closed.
On 2015/08/27 03:56:46, Nico wrote: > https://codereview.webrtc.org/1266033005/diff/160001/talk/media/sctp/sctpdata... > File talk/media/sctp/sctpdataengine.h (right): > > https://codereview.webrtc.org/1266033005/diff/160001/talk/media/sctp/sctpdata... > talk/media/sctp/sctpdataengine.h:105: static std::vector<SctpDataMediaChannel*> > channels_; > This adds a static initializer, that's no good. (I'm not sure why codecs_ > doesn't add one too – I guess it's not referenced from any code used in chromium > and gets dead-stripped?) > > # sctpdataengine.cc _GLOBAL__sub_I_sctpdataengine.cc+0xf > # sctpdataengine.cc __cxa_atexit@plt [registers a dtor to run at exit] lally/pthatcher, can you guys fix this immediately? The static initializer blocks updating webrtc which in turn means the CrOS tree has a broken compile until this is fixed.
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:160001) has been created in https://codereview.webrtc.org/1315923003/ by tommi@webrtc.org. The reason for reverting is: The CL adds a global variable..
Message was sent while issue was closed.
tommi@webrtc.org changed reviewers: + tommi@webrtc.org
Message was sent while issue was closed.
I had to revert the cl due to the global variable. Also took a quick look that would be good to address before landing a new version. https://codereview.webrtc.org/1266033005/diff/160001/talk/media/sctp/sctpdata... File talk/media/sctp/sctpdataengine.cc (right): https://codereview.webrtc.org/1266033005/diff/160001/talk/media/sctp/sctpdata... talk/media/sctp/sctpdataengine.cc:252: std::vector<SctpDataMediaChannel*> SctpDataEngine::channels_; how is thread safety guaranteed? https://codereview.webrtc.org/1266033005/diff/160001/talk/media/sctp/sctpdata... talk/media/sctp/sctpdataengine.cc:334: SctpDataMediaChannel* SctpDataEngine::GetChannelFromSocket( On what thread does this method run and how do you know it's the same thread for all data engines? https://codereview.webrtc.org/1266033005/diff/160001/talk/media/sctp/sctpdata... talk/media/sctp/sctpdataengine.cc:336: for (auto p:channels_) { nit: spaces around : nit: would be good to use the actual type and not auto. Also though, doesn't this create a copy of every channel when iterating? https://codereview.webrtc.org/1266033005/diff/160001/talk/media/sctp/sctpdata... talk/media/sctp/sctpdataengine.cc:357: SctpDataMediaChannel *channel = GetChannelFromSocket(sock); SctpDataMediaChannel* channel https://codereview.webrtc.org/1266033005/diff/160001/talk/media/sctp/sctpdata... talk/media/sctp/sctpdataengine.cc:366: nit: one empty line https://codereview.webrtc.org/1266033005/diff/160001/talk/media/sctp/sctpdata... talk/media/sctp/sctpdataengine.cc:405: const static int send_threshold = usrsctp_sysctl_get_sctp_sendspace() / 2; nit: kSendThreshold https://codereview.webrtc.org/1266033005/diff/160001/talk/media/sctp/sctpdata... talk/media/sctp/sctpdataengine.cc:455: struct sctp_paddrparams params; prefer this the way it was (without memset) https://codereview.webrtc.org/1266033005/diff/160001/talk/media/sctp/sctpdata... File talk/media/sctp/sctpdataengine_unittest.cc (right): https://codereview.webrtc.org/1266033005/diff/160001/talk/media/sctp/sctpdata... talk/media/sctp/sctpdataengine_unittest.cc:353: void OnChan1ReadyToSend(bool send) { if (send) chan1_ready_to_send_count_++; } nit: use postfix ++ (style guide thing) it's also a good thing to not have if() and the body of the if, on the same line. It makes it harder to set breakpoints.
Message was sent while issue was closed.
https://codereview.webrtc.org/1266033005/diff/160001/talk/media/sctp/sctpdata... File talk/media/sctp/sctpdataengine.h (right): https://codereview.webrtc.org/1266033005/diff/160001/talk/media/sctp/sctpdata... talk/media/sctp/sctpdataengine.h:105: static std::vector<SctpDataMediaChannel*> channels_; On 2015/08/27 03:56:46, Nico wrote: > This adds a static initializer, that's no good. (I'm not sure why codecs_ > doesn't add one too – I guess it's not referenced from any code used in chromium > and gets dead-stripped?) > > # sctpdataengine.cc _GLOBAL__sub_I_sctpdataengine.cc+0xf > # sctpdataengine.cc __cxa_atexit@plt [registers a dtor to run at exit] codecs_ isn't static. And when I reviewed this code I didn't notice that channels_ was static. I don't see any reason why it needs to be.
Message was sent while issue was closed.
Hi, if this delayed-init approach isn't OK (and the synchronization that's required), lemme know and I'll put up a version that looks up the SctpDataMediaChannel* that we've saved with usrsctp. It's another malloc/free cycle per callback, which is why I've avoided it so far. That technique avoids adding a global, and thusly avoids the synchronization needed to maintain it. https://codereview.webrtc.org/1266033005/diff/160001/talk/media/sctp/sctpdata... File talk/media/sctp/sctpdataengine.cc (right): https://codereview.webrtc.org/1266033005/diff/160001/talk/media/sctp/sctpdata... talk/media/sctp/sctpdataengine.cc:252: std::vector<SctpDataMediaChannel*> SctpDataEngine::channels_; On 2015/08/27 11:38:02, tommi (webrtc) wrote: > how is thread safety guaranteed? Accesses to the member are via message-posting to single threads that manage this. Normally. In this case, the library invoking the callback I added may be using its own generated thread, so it's good that you reverted! https://codereview.webrtc.org/1266033005/diff/160001/talk/media/sctp/sctpdata... talk/media/sctp/sctpdataengine.cc:334: SctpDataMediaChannel* SctpDataEngine::GetChannelFromSocket( On 2015/08/27 11:38:02, tommi (webrtc) wrote: > On what thread does this method run and how do you know it's the same thread for > all data engines? I/O thread for usrsctplib, which only has once instance ever loaded (controlled via static int). But it's all CritSec'd now. https://codereview.webrtc.org/1266033005/diff/160001/talk/media/sctp/sctpdata... talk/media/sctp/sctpdataengine.cc:336: for (auto p:channels_) { On 2015/08/27 11:38:02, tommi (webrtc) wrote: > nit: spaces around : > nit: would be good to use the actual type and not auto. > > Also though, doesn't this create a copy of every channel when iterating? Done and done. It's a vector of pointers to channels. https://codereview.webrtc.org/1266033005/diff/160001/talk/media/sctp/sctpdata... talk/media/sctp/sctpdataengine.cc:357: SctpDataMediaChannel *channel = GetChannelFromSocket(sock); On 2015/08/27 11:38:02, tommi (webrtc) wrote: > SctpDataMediaChannel* channel Acknowledged. https://codereview.webrtc.org/1266033005/diff/160001/talk/media/sctp/sctpdata... talk/media/sctp/sctpdataengine.cc:366: On 2015/08/27 11:38:02, tommi (webrtc) wrote: > nit: one empty line Done. https://codereview.webrtc.org/1266033005/diff/160001/talk/media/sctp/sctpdata... talk/media/sctp/sctpdataengine.cc:405: const static int send_threshold = usrsctp_sysctl_get_sctp_sendspace() / 2; On 2015/08/27 11:38:02, tommi (webrtc) wrote: > nit: kSendThreshold Done. https://codereview.webrtc.org/1266033005/diff/160001/talk/media/sctp/sctpdata... talk/media/sctp/sctpdataengine.cc:455: struct sctp_paddrparams params; On 2015/08/27 11:38:02, tommi (webrtc) wrote: > prefer this the way it was (without memset) The old way was leaving some bytes uninitialized, that triggered some sanitizer. I forget the bug (and can't find it right now), but that's why I changed it. https://codereview.webrtc.org/1266033005/diff/160001/talk/media/sctp/sctpdata... File talk/media/sctp/sctpdataengine.h (right): https://codereview.webrtc.org/1266033005/diff/160001/talk/media/sctp/sctpdata... talk/media/sctp/sctpdataengine.h:105: static std::vector<SctpDataMediaChannel*> channels_; On 2015/08/27 03:56:46, Nico wrote: > This adds a static initializer, that's no good. (I'm not sure why codecs_ > doesn't add one too – I guess it's not referenced from any code used in chromium > and gets dead-stripped?) > > # sctpdataengine.cc _GLOBAL__sub_I_sctpdataengine.cc+0xf > # sctpdataengine.cc __cxa_atexit@plt [registers a dtor to run at exit] Ah, I didn't know that wasn't allowed. I've wrapped it in a static function, so the initializer's only run when the function's first invoked. codecs_ isn't static. Is that allowed? If not, then the only other way to get the relevant data for us involves a malloc/free cycle on each callback invocation. https://codereview.webrtc.org/1266033005/diff/160001/talk/media/sctp/sctpdata... File talk/media/sctp/sctpdataengine_unittest.cc (right): https://codereview.webrtc.org/1266033005/diff/160001/talk/media/sctp/sctpdata... talk/media/sctp/sctpdataengine_unittest.cc:353: void OnChan1ReadyToSend(bool send) { if (send) chan1_ready_to_send_count_++; } On 2015/08/27 11:38:02, tommi (webrtc) wrote: > nit: use postfix ++ (style guide thing) > > it's also a good thing to not have if() and the body of the if, on the same > line. It makes it harder to set breakpoints. Done.
Patchset #6 (id:200001) has been deleted
Ok, all changed. No more statics, no crit-secs. PTAL, thanks.
On 2015/08/27 18:35:03, lally1 wrote: > Ok, all changed. No more statics, no crit-secs. PTAL, thanks. Can you make it a new CL? Otherwise, I think it's confusing.
Ok! Moved over to https://codereview.webrtc.org/1304063006/ |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
