Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1397123003:
Add AudioSendStream to Call. (Closed)
|
Created:
5 years, 2 months ago by the sun Modified:
5 years, 2 months ago CC:
webrtc-reviews_webrtc.org, interface-changes_webrtc.org, rwolff_gocast.it, yujie_mao (webrtc), Andrew MacDonald, tterriberry_mozilla.com, qiang.lu, niklas.enbom, peah-webrtc, kwiberg-webrtc, stefan-webrtc Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd webrtc::AudioSendStream and methods on webrtc::Call to create and delete AudioSendStreams.
AudioSendStream will be replacing the send side of VoiceEngine channels and associated APIs. Hence, they will be used transform recorded audio into RTP/RTCP packets that can be transmitted to another party, according to given parameters.
BUG=webrtc:4690
Committed: https://crrev.com/c7a8b08a7cd8d8f37d7f5fb9930d0cdc74baba35
Cr-Commit-Position: refs/heads/master@{#10307}
Patch Set 1 #Patch Set 2 : more tests #
Total comments: 24
Patch Set 3 : comments #Patch Set 4 : missing file #Patch Set 5 : comments+merge #
Total comments: 8
Patch Set 6 : merge+comments #
Total comments: 11
Patch Set 7 : merge+comments #
Total comments: 7
Patch Set 8 : comment #Patch Set 9 : moar coment #Patch Set 10 : rebase #
Dependent Patchsets: Messages
Total messages: 46 (11 generated)
solenberg@webrtc.org changed reviewers: + stefan@webrtc.org, tommi@webrtc.org
Can you add some more documentation about what the audio send stream is and why it's needed? https://codereview.webrtc.org/1397123003/diff/20001/webrtc/audio/audio_send_s... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/1397123003/diff/20001/webrtc/audio/audio_send_s... webrtc/audio/audio_send_stream.cc:18: std::string AudioSendStream::Config::Rtp::ToString() const { is this needed for debugging purposes? If so, can we included only in such builds? https://codereview.webrtc.org/1397123003/diff/20001/webrtc/audio/audio_send_s... webrtc/audio/audio_send_stream.cc:22: for (size_t i = 0; i < extensions.size(); ++i) { use range based loop? https://codereview.webrtc.org/1397123003/diff/20001/webrtc/audio_send_stream.h File webrtc/audio_send_stream.h (right): https://codereview.webrtc.org/1397123003/diff/20001/webrtc/audio_send_stream.... webrtc/audio_send_stream.h:49: Transport* send_transport = nullptr; if you could also explain ownership rules for this variable, that would be good. https://codereview.webrtc.org/1397123003/diff/20001/webrtc/audio_send_stream.... webrtc/audio_send_stream.h:59: AudioEncoder* encoder = nullptr; I'm having a hard time understanding how the comment explains this encoder variable and why it should not be a scoped_ptr. Does ownership ever lie with Config? How is the objects lifetime and this pointer's value synchronized? (I.e. how do we guarantee that we never use dangling pointers?) https://codereview.webrtc.org/1397123003/diff/20001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/1397123003/diff/20001/webrtc/call/call.cc#newco... webrtc/call/call.cc:115: std::map<uint32_t, AudioSendStream*> audio_send_ssrcs_ GUARDED_BY(send_crit_); document where ownership of the send stream lies? https://codereview.webrtc.org/1397123003/diff/20001/webrtc/call/call.cc#newco... webrtc/call/call.cc:167: RTC_CHECK_EQ(0u, audio_send_ssrcs_.size()); nit: RTC_CHECK(audio_send_ssrcs_.empty()); I see you're just repeating the pattern, but if you don't mind fixing all of these checks, that would be appreciated. https://codereview.webrtc.org/1397123003/diff/20001/webrtc/call/call.cc#newco... webrtc/call/call.cc:195: return send_stream; is it guaranteed that CreateAudioSendStream() and DestroyAudioSendStream() are always called on the same thread? If so, please add a thread checker. If not, then it looks to me that we have a race since we're returning a pointer here to an object that may have been deleted. https://codereview.webrtc.org/1397123003/diff/20001/webrtc/call/call.cc#newco... webrtc/call/call.cc:205: static_cast<AudioSendStream*>(send_stream); I'm not groking this cast :) https://codereview.webrtc.org/1397123003/diff/20001/webrtc/call/call.cc#newco... webrtc/call/call.cc:433: for (auto& kv : audio_send_ssrcs_) { no chance ov avoiding holding these locks (network_enabled_crit_ and send_crit_) while calling out?
solenberg@webrtc.org changed reviewers: + pbos@webrtc.org - stefan@webrtc.org
https://codereview.webrtc.org/1397123003/diff/20001/webrtc/audio/audio_send_s... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/1397123003/diff/20001/webrtc/audio/audio_send_s... webrtc/audio/audio_send_stream.cc:18: std::string AudioSendStream::Config::Rtp::ToString() const { On 2015/10/14 13:21:18, tommi (webrtc) wrote: > is this needed for debugging purposes? If so, can we included only in such > builds? I like that idea, but it runs deeper than just this function, or its siblings. The use is usually something like this: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... In other words, the function will be called in any build, so we can a) make it an empty function and hope that the compiler removes the implementation for us, or b) change the logging semantics so no cycles are wasted on manipulating strings in release builds. Thoughts? https://codereview.webrtc.org/1397123003/diff/20001/webrtc/audio/audio_send_s... webrtc/audio/audio_send_stream.cc:22: for (size_t i = 0; i < extensions.size(); ++i) { On 2015/10/14 13:21:18, tommi (webrtc) wrote: > use range based loop? With the conditional "ss << ", ";" below, I don't see much point. Need to keep an iterator or an index anyway. https://codereview.webrtc.org/1397123003/diff/20001/webrtc/audio_send_stream.h File webrtc/audio_send_stream.h (right): https://codereview.webrtc.org/1397123003/diff/20001/webrtc/audio_send_stream.... webrtc/audio_send_stream.h:49: Transport* send_transport = nullptr; On 2015/10/14 13:21:18, tommi (webrtc) wrote: > if you could also explain ownership rules for this variable, that would be good. Done. https://codereview.webrtc.org/1397123003/diff/20001/webrtc/audio_send_stream.... webrtc/audio_send_stream.h:59: AudioEncoder* encoder = nullptr; On 2015/10/14 13:21:18, tommi (webrtc) wrote: > I'm having a hard time understanding how the comment explains this encoder > variable and why it should not be a scoped_ptr. Does ownership ever lie with > Config? How is the objects lifetime and this pointer's value synchronized? > (I.e. how do we guarantee that we never use dangling pointers?) The config is *copied* into webrtc::internal::AudioSendStream, so a scoped_ptr can't be used, unless swapping the whole config into the AudioSendStream, which might not be a bad idea, although currently a bit different from how it's done for video. https://codereview.webrtc.org/1397123003/diff/20001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/1397123003/diff/20001/webrtc/call/call.cc#newco... webrtc/call/call.cc:115: std::map<uint32_t, AudioSendStream*> audio_send_ssrcs_ GUARDED_BY(send_crit_); On 2015/10/14 13:21:18, tommi (webrtc) wrote: > document where ownership of the send stream lies? Done. https://codereview.webrtc.org/1397123003/diff/20001/webrtc/call/call.cc#newco... webrtc/call/call.cc:167: RTC_CHECK_EQ(0u, audio_send_ssrcs_.size()); On 2015/10/14 13:21:18, tommi (webrtc) wrote: > nit: RTC_CHECK(audio_send_ssrcs_.empty()); I see you're just repeating the > pattern, but if you don't mind fixing all of these checks, that would be > appreciated. Done. https://codereview.webrtc.org/1397123003/diff/20001/webrtc/call/call.cc#newco... webrtc/call/call.cc:195: return send_stream; On 2015/10/14 13:21:18, tommi (webrtc) wrote: > is it guaranteed that CreateAudioSendStream() and DestroyAudioSendStream() are > always called on the same thread? > If so, please add a thread checker. If not, then it looks to me that we have a > race since we're returning a pointer here to an object that may have been > deleted. Well, from what I can make out, in libjingle only Call::SignalNetworkState() is the only call not happening on the worker thread, so another option would be to marshall that call as well. Then we could get rid of all these locks and only have a thread checker. pbos, mflodman? https://codereview.webrtc.org/1397123003/diff/20001/webrtc/call/call.cc#newco... webrtc/call/call.cc:205: static_cast<AudioSendStream*>(send_stream); On 2015/10/14 13:21:18, tommi (webrtc) wrote: > I'm not groking this cast :) We're casting a webrtc::AudioSendStream to a webrtc::internal::AudioSendStream. I can make it explicit if you want, but then I'll change the corresponding casts in all functions below, and send pbos your way when he complains. :) https://codereview.webrtc.org/1397123003/diff/20001/webrtc/call/call.cc#newco... webrtc/call/call.cc:433: for (auto& kv : audio_send_ssrcs_) { On 2015/10/14 13:21:18, tommi (webrtc) wrote: > no chance ov avoiding holding these locks (network_enabled_crit_ and send_crit_) > while calling out? See above comment...
stefan@webrtc.org changed reviewers: + stefan@webrtc.org
https://codereview.webrtc.org/1397123003/diff/20001/webrtc/audio/audio_send_s... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/1397123003/diff/20001/webrtc/audio/audio_send_s... webrtc/audio/audio_send_stream.cc:18: std::string AudioSendStream::Config::Rtp::ToString() const { On 2015/10/14 14:25:24, the sun wrote: > On 2015/10/14 13:21:18, tommi (webrtc) wrote: > > is this needed for debugging purposes? If so, can we included only in such > > builds? > > I like that idea, but it runs deeper than just this function, or its siblings. > The use is usually something like this: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... > > In other words, the function will be called in any build, so we can > a) make it an empty function and hope that the compiler removes the > implementation for us, or > b) change the logging semantics so no cycles are wasted on manipulating strings > in release builds. > > Thoughts? This string is used to LOG the stream configurations into the webrtc log in a call, and is very useful for debugging real calls. Is it really worth getting rid of?
On 2015/10/14 14:29:22, stefan-webrtc (holmer) wrote: > https://codereview.webrtc.org/1397123003/diff/20001/webrtc/audio/audio_send_s... > File webrtc/audio/audio_send_stream.cc (right): > > https://codereview.webrtc.org/1397123003/diff/20001/webrtc/audio/audio_send_s... > webrtc/audio/audio_send_stream.cc:18: std::string > AudioSendStream::Config::Rtp::ToString() const { > On 2015/10/14 14:25:24, the sun wrote: > > On 2015/10/14 13:21:18, tommi (webrtc) wrote: > > > is this needed for debugging purposes? If so, can we included only in such > > > builds? > > > > I like that idea, but it runs deeper than just this function, or its siblings. > > The use is usually something like this: > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... > > > > In other words, the function will be called in any build, so we can > > a) make it an empty function and hope that the compiler removes the > > implementation for us, or > > b) change the logging semantics so no cycles are wasted on manipulating > strings > > in release builds. > > > > Thoughts? > > This string is used to LOG the stream configurations into the webrtc log in a > call, and is very useful for debugging real calls. Is it really worth getting > rid of? Well, if it meets the requirement of being very useful for debugging real calls, then I guess not :)
Thanks, I'll wait for feedback wrt to threading https://codereview.webrtc.org/1397123003/diff/20001/webrtc/audio/audio_send_s... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/1397123003/diff/20001/webrtc/audio/audio_send_s... webrtc/audio/audio_send_stream.cc:22: for (size_t i = 0; i < extensions.size(); ++i) { On 2015/10/14 14:25:24, the sun wrote: > On 2015/10/14 13:21:18, tommi (webrtc) wrote: > > use range based loop? > > With the conditional "ss << ", ";" below, I don't see much point. Need to keep > an iterator or an index anyway. Acknowledged. https://codereview.webrtc.org/1397123003/diff/20001/webrtc/audio_send_stream.h File webrtc/audio_send_stream.h (right): https://codereview.webrtc.org/1397123003/diff/20001/webrtc/audio_send_stream.... webrtc/audio_send_stream.h:59: AudioEncoder* encoder = nullptr; On 2015/10/14 14:25:24, the sun wrote: > On 2015/10/14 13:21:18, tommi (webrtc) wrote: > > I'm having a hard time understanding how the comment explains this encoder > > variable and why it should not be a scoped_ptr. Does ownership ever lie with > > Config? How is the objects lifetime and this pointer's value synchronized? > > (I.e. how do we guarantee that we never use dangling pointers?) > > The config is *copied* into webrtc::internal::AudioSendStream, so a scoped_ptr > can't be used, unless swapping the whole config into the AudioSendStream, which > might not be a bad idea, although currently a bit different from how it's done > for video. I see... is it copied into the internal one then? Would it make sense to hold the Config itself as a scoped_ptr and pass ownership via Pass() instead of copying? https://codereview.webrtc.org/1397123003/diff/20001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/1397123003/diff/20001/webrtc/call/call.cc#newco... webrtc/call/call.cc:205: static_cast<AudioSendStream*>(send_stream); On 2015/10/14 14:25:24, the sun wrote: > On 2015/10/14 13:21:18, tommi (webrtc) wrote: > > I'm not groking this cast :) > > We're casting a webrtc::AudioSendStream to a webrtc::internal::AudioSendStream. > I can make it explicit if you want, but then I'll change the corresponding casts > in all functions below, and send pbos your way when he complains. :) Ah :) that wasn't obvious to me especially since we're in webrtc:: already. I guess I'd prefer to have the internal class use a different name (e.g. AudioSendStreamInternal), but you could also for now just comment what the cast is for.
https://codereview.webrtc.org/1397123003/diff/20001/webrtc/audio_send_stream.h File webrtc/audio_send_stream.h (right): https://codereview.webrtc.org/1397123003/diff/20001/webrtc/audio_send_stream.... webrtc/audio_send_stream.h:59: AudioEncoder* encoder = nullptr; On 2015/10/14 14:49:28, tommi (webrtc) wrote: > On 2015/10/14 14:25:24, the sun wrote: > > On 2015/10/14 13:21:18, tommi (webrtc) wrote: > > > I'm having a hard time understanding how the comment explains this encoder > > > variable and why it should not be a scoped_ptr. Does ownership ever lie > with > > > Config? How is the objects lifetime and this pointer's value synchronized? > > > (I.e. how do we guarantee that we never use dangling pointers?) > > > > The config is *copied* into webrtc::internal::AudioSendStream, so a scoped_ptr > > can't be used, unless swapping the whole config into the AudioSendStream, > which > > might not be a bad idea, although currently a bit different from how it's done > > for video. > > I see... is it copied into the internal one then? Would it make sense to hold > the Config itself as a scoped_ptr and pass ownership via Pass() instead of > copying? Not clearcut what is the best approach here. I decided to comment out for now and make the final decision when we get to actual use that part of the config struct. I'm with you 100% that ownership should be explicit and fail safe. https://codereview.webrtc.org/1397123003/diff/20001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/1397123003/diff/20001/webrtc/call/call.cc#newco... webrtc/call/call.cc:205: static_cast<AudioSendStream*>(send_stream); On 2015/10/14 14:49:28, tommi (webrtc) wrote: > On 2015/10/14 14:25:24, the sun wrote: > > On 2015/10/14 13:21:18, tommi (webrtc) wrote: > > > I'm not groking this cast :) > > > > We're casting a webrtc::AudioSendStream to a > webrtc::internal::AudioSendStream. > > I can make it explicit if you want, but then I'll change the corresponding > casts > > in all functions below, and send pbos your way when he complains. :) > > Ah :) that wasn't obvious to me especially since we're in webrtc:: already. I > guess I'd prefer to have the internal class use a different name (e.g. > AudioSendStreamInternal), but you could also for now just comment what the cast > is for. I've added explicit casts for the audio functions; I think readable code trumps comments.
I don't think we should enforce that stream configuration and network is done on the same thread, especially when that thread is "the worker thread" used for everything (I think that's an implementation defect we have today), so I prefer keeping the locks. It makes sense to deliver stuff straight from the socket here and not enforce thread jumps. https://codereview.webrtc.org/1397123003/diff/80001/webrtc/audio/audio_send_s... File webrtc/audio/audio_send_stream.h (right): https://codereview.webrtc.org/1397123003/diff/80001/webrtc/audio/audio_send_s... webrtc/audio/audio_send_stream.h:17: remove empty line https://codereview.webrtc.org/1397123003/diff/80001/webrtc/audio/audio_send_s... File webrtc/audio/audio_send_stream_unittest.cc (right): https://codereview.webrtc.org/1397123003/diff/80001/webrtc/audio/audio_send_s... webrtc/audio/audio_send_stream_unittest.cc:26: EXPECT_LT(0u, config.ToString().size()); pref GT(size, 0u) https://codereview.webrtc.org/1397123003/diff/80001/webrtc/call/call_unittest.cc File webrtc/call/call_unittest.cc (right): https://codereview.webrtc.org/1397123003/diff/80001/webrtc/call/call_unittest... webrtc/call/call_unittest.cc:100: } // namespace webrtc Do you want to add some of these for Video equivalents? https://codereview.webrtc.org/1397123003/diff/80001/webrtc/webrtc_tests.gypi File webrtc/webrtc_tests.gypi (right): https://codereview.webrtc.org/1397123003/diff/80001/webrtc/webrtc_tests.gypi#... webrtc/webrtc_tests.gypi:149: # TODO(solenberg): Rename to webrtc_call_tests? Imo split up between audio, video, common. It's useful for local testing and "we" generally don't touch "audio" tests, so they add runtime for us.
https://codereview.webrtc.org/1397123003/diff/80001/webrtc/webrtc_tests.gypi File webrtc/webrtc_tests.gypi (right): https://codereview.webrtc.org/1397123003/diff/80001/webrtc/webrtc_tests.gypi#... webrtc/webrtc_tests.gypi:149: # TODO(solenberg): Rename to webrtc_call_tests? On 2015/10/15 15:32:12, pbos-webrtc wrote: > Imo split up between audio, video, common. It's useful for local testing and > "we" generally don't touch "audio" tests, so they add runtime for us. Checked with kjellander@; they are not happy about adding more test binaries.
On 2015/10/15 15:34:44, the sun wrote: > https://codereview.webrtc.org/1397123003/diff/80001/webrtc/webrtc_tests.gypi > File webrtc/webrtc_tests.gypi (right): > > https://codereview.webrtc.org/1397123003/diff/80001/webrtc/webrtc_tests.gypi#... > webrtc/webrtc_tests.gypi:149: # TODO(solenberg): Rename to webrtc_call_tests? > On 2015/10/15 15:32:12, pbos-webrtc wrote: > > Imo split up between audio, video, common. It's useful for local testing and > > "we" generally don't touch "audio" tests, so they add runtime for us. > > Checked with kjellander@; they are not happy about adding more test binaries. I think slicing test binaries based on teams make sense.
On 2015/10/15 16:35:00, pbos-webrtc wrote: > On 2015/10/15 15:34:44, the sun wrote: > > https://codereview.webrtc.org/1397123003/diff/80001/webrtc/webrtc_tests.gypi > > File webrtc/webrtc_tests.gypi (right): > > > > > https://codereview.webrtc.org/1397123003/diff/80001/webrtc/webrtc_tests.gypi#... > > webrtc/webrtc_tests.gypi:149: # TODO(solenberg): Rename to webrtc_call_tests? > > On 2015/10/15 15:32:12, pbos-webrtc wrote: > > > Imo split up between audio, video, common. It's useful for local testing and > > > "we" generally don't touch "audio" tests, so they add runtime for us. > > > > Checked with kjellander@; they are not happy about adding more test binaries. > > I think slicing test binaries based on teams make sense. That doesn't sound right to me. If you followed that through, BWE+video+call.cc tests would sit in one binary, while audio stuff (except ADM) would sit in another binary. While convenient for some people, at some point, it would seem random to everyone else.
On 2015/10/15 15:32:12, pbos-webrtc wrote: > I don't think we should enforce that stream configuration and network is done on > the same thread, especially when that thread is "the worker thread" used for > everything (I think that's an implementation defect we have today), so I prefer > keeping the locks. It makes sense to deliver stuff straight from the socket here > and not enforce thread jumps. Having a well understood thread model is good and I don't see assumptions about libjingle's worker thread (can you point it out?). Keeping the locks means potential of contention. Do you know what threads could content? Is it acceptable to block the network thread or any of the other threads? In my opinion it's a good thing to enforce correct thread usage and the more strict the threading requirements are, the better we understand how things actually hang together. If that means enforcing thread jumps, fine (see Chromium). If this doesn't make sense in this particular case though, it would be good to understand why not. > > https://codereview.webrtc.org/1397123003/diff/80001/webrtc/audio/audio_send_s... > File webrtc/audio/audio_send_stream.h (right): > > https://codereview.webrtc.org/1397123003/diff/80001/webrtc/audio/audio_send_s... > webrtc/audio/audio_send_stream.h:17: > remove empty line > > https://codereview.webrtc.org/1397123003/diff/80001/webrtc/audio/audio_send_s... > File webrtc/audio/audio_send_stream_unittest.cc (right): > > https://codereview.webrtc.org/1397123003/diff/80001/webrtc/audio/audio_send_s... > webrtc/audio/audio_send_stream_unittest.cc:26: EXPECT_LT(0u, > config.ToString().size()); > pref GT(size, 0u) > > https://codereview.webrtc.org/1397123003/diff/80001/webrtc/call/call_unittest.cc > File webrtc/call/call_unittest.cc (right): > > https://codereview.webrtc.org/1397123003/diff/80001/webrtc/call/call_unittest... > webrtc/call/call_unittest.cc:100: } // namespace webrtc > Do you want to add some of these for Video equivalents? > > https://codereview.webrtc.org/1397123003/diff/80001/webrtc/webrtc_tests.gypi > File webrtc/webrtc_tests.gypi (right): > > https://codereview.webrtc.org/1397123003/diff/80001/webrtc/webrtc_tests.gypi#... > webrtc/webrtc_tests.gypi:149: # TODO(solenberg): Rename to webrtc_call_tests? > Imo split up between audio, video, common. It's useful for local testing and > "we" generally don't touch "audio" tests, so they add runtime for us. yeah, not sure I agree with this, definitely not about splitting the code based on sub teams. You and I are on the same team, right? :) Btw, by 'runtime' are you referring to something like a runtime library or the time it takes to run the tests? I don't think the former is true and the latter can be fixed in other ways than adding more executables.
On 2015/10/15 21:09:53, tommi (webrtc) wrote: > On 2015/10/15 15:32:12, pbos-webrtc wrote: > > I don't think we should enforce that stream configuration and network is done > on > > the same thread, especially when that thread is "the worker thread" used for > > everything (I think that's an implementation defect we have today), so I > prefer > > keeping the locks. It makes sense to deliver stuff straight from the socket > here > > and not enforce thread jumps. > > Having a well understood thread model is good and I don't see assumptions about > libjingle's worker thread (can you point it out?). > > Keeping the locks means potential of contention. Do you know what threads could > content? Is it acceptable to block the network thread or any of the other > threads? > > In my opinion it's a good thing to enforce correct thread usage and the more > strict the threading requirements are, the better we understand how things > actually hang together. If that means enforcing thread jumps, fine (see > Chromium). If this doesn't make sense in this particular case though, it would > be good to understand why not. > > > > > > https://codereview.webrtc.org/1397123003/diff/80001/webrtc/audio/audio_send_s... > > File webrtc/audio/audio_send_stream.h (right): > > > > > https://codereview.webrtc.org/1397123003/diff/80001/webrtc/audio/audio_send_s... > > webrtc/audio/audio_send_stream.h:17: > > remove empty line > > > > > https://codereview.webrtc.org/1397123003/diff/80001/webrtc/audio/audio_send_s... > > File webrtc/audio/audio_send_stream_unittest.cc (right): > > > > > https://codereview.webrtc.org/1397123003/diff/80001/webrtc/audio/audio_send_s... > > webrtc/audio/audio_send_stream_unittest.cc:26: EXPECT_LT(0u, > > config.ToString().size()); > > pref GT(size, 0u) > > > > > https://codereview.webrtc.org/1397123003/diff/80001/webrtc/call/call_unittest.cc > > File webrtc/call/call_unittest.cc (right): > > > > > https://codereview.webrtc.org/1397123003/diff/80001/webrtc/call/call_unittest... > > webrtc/call/call_unittest.cc:100: } // namespace webrtc > > Do you want to add some of these for Video equivalents? > > > > https://codereview.webrtc.org/1397123003/diff/80001/webrtc/webrtc_tests.gypi > > File webrtc/webrtc_tests.gypi (right): > > > > > https://codereview.webrtc.org/1397123003/diff/80001/webrtc/webrtc_tests.gypi#... > > webrtc/webrtc_tests.gypi:149: # TODO(solenberg): Rename to webrtc_call_tests? > > Imo split up between audio, video, common. It's useful for local testing and > > "we" generally don't touch "audio" tests, so they add runtime for us. > > yeah, not sure I agree with this, definitely not about splitting the code based > on sub teams. You and I are on the same team, right? :) > > Btw, by 'runtime' are you referring to something like a runtime library or the > time it takes to run the tests? I don't think the former is true and the latter > can be fixed in other ways than adding more executables. Regarding locks vs thread checker, how about keeping the locks but adding a thread checker but keep the locks? That way multithreading assumptions would at least be documented (functions called on network thread would do a false-check) and we can make an effort to remove the locks later on. Just keeping the locks in undocumented state will give the impression that "anything goes" and we're heading towards the cliff...
https://codereview.webrtc.org/1397123003/diff/80001/webrtc/audio/audio_send_s... File webrtc/audio/audio_send_stream.h (right): https://codereview.webrtc.org/1397123003/diff/80001/webrtc/audio/audio_send_s... webrtc/audio/audio_send_stream.h:17: On 2015/10/15 15:32:11, pbos-webrtc wrote: > remove empty line Done. Thanks. Important. https://codereview.webrtc.org/1397123003/diff/80001/webrtc/audio/audio_send_s... File webrtc/audio/audio_send_stream_unittest.cc (right): https://codereview.webrtc.org/1397123003/diff/80001/webrtc/audio/audio_send_s... webrtc/audio/audio_send_stream_unittest.cc:26: EXPECT_LT(0u, config.ToString().size()); On 2015/10/15 15:32:12, pbos-webrtc wrote: > pref GT(size, 0u) Done. https://codereview.webrtc.org/1397123003/diff/80001/webrtc/call/call_unittest.cc File webrtc/call/call_unittest.cc (right): https://codereview.webrtc.org/1397123003/diff/80001/webrtc/call/call_unittest... webrtc/call/call_unittest.cc:100: } // namespace webrtc On 2015/10/15 15:32:12, pbos-webrtc wrote: > Do you want to add some of these for Video equivalents? Makes more sense if you do that.
On 2015/10/16 08:46:55, the sun wrote: > On 2015/10/15 21:09:53, tommi (webrtc) wrote: > > On 2015/10/15 15:32:12, pbos-webrtc wrote: > > > I don't think we should enforce that stream configuration and network is > done > > on > > > the same thread, especially when that thread is "the worker thread" used for > > > everything (I think that's an implementation defect we have today), so I > > prefer > > > keeping the locks. It makes sense to deliver stuff straight from the socket > > here > > > and not enforce thread jumps. > > > > Having a well understood thread model is good and I don't see assumptions > about > > libjingle's worker thread (can you point it out?). > > > > Keeping the locks means potential of contention. Do you know what threads > could > > content? Is it acceptable to block the network thread or any of the other > > threads? > > > > In my opinion it's a good thing to enforce correct thread usage and the more > > strict the threading requirements are, the better we understand how things > > actually hang together. If that means enforcing thread jumps, fine (see > > Chromium). If this doesn't make sense in this particular case though, it > would > > be good to understand why not. > > > > > > > > > > > https://codereview.webrtc.org/1397123003/diff/80001/webrtc/audio/audio_send_s... > > > File webrtc/audio/audio_send_stream.h (right): > > > > > > > > > https://codereview.webrtc.org/1397123003/diff/80001/webrtc/audio/audio_send_s... > > > webrtc/audio/audio_send_stream.h:17: > > > remove empty line > > > > > > > > > https://codereview.webrtc.org/1397123003/diff/80001/webrtc/audio/audio_send_s... > > > File webrtc/audio/audio_send_stream_unittest.cc (right): > > > > > > > > > https://codereview.webrtc.org/1397123003/diff/80001/webrtc/audio/audio_send_s... > > > webrtc/audio/audio_send_stream_unittest.cc:26: EXPECT_LT(0u, > > > config.ToString().size()); > > > pref GT(size, 0u) > > > > > > > > > https://codereview.webrtc.org/1397123003/diff/80001/webrtc/call/call_unittest.cc > > > File webrtc/call/call_unittest.cc (right): > > > > > > > > > https://codereview.webrtc.org/1397123003/diff/80001/webrtc/call/call_unittest... > > > webrtc/call/call_unittest.cc:100: } // namespace webrtc > > > Do you want to add some of these for Video equivalents? > > > > > > https://codereview.webrtc.org/1397123003/diff/80001/webrtc/webrtc_tests.gypi > > > File webrtc/webrtc_tests.gypi (right): > > > > > > > > > https://codereview.webrtc.org/1397123003/diff/80001/webrtc/webrtc_tests.gypi#... > > > webrtc/webrtc_tests.gypi:149: # TODO(solenberg): Rename to > webrtc_call_tests? > > > Imo split up between audio, video, common. It's useful for local testing and > > > "we" generally don't touch "audio" tests, so they add runtime for us. > > > > yeah, not sure I agree with this, definitely not about splitting the code > based > > on sub teams. You and I are on the same team, right? :) > > > > Btw, by 'runtime' are you referring to something like a runtime library or the > > time it takes to run the tests? I don't think the former is true and the > latter > > can be fixed in other ways than adding more executables. > > Regarding locks vs thread checker, how about keeping the locks but adding a > thread checker but keep the locks? > sgtm > That way multithreading assumptions would at least be documented (functions > called on network thread would do a false-check) and we can make an effort to > remove the locks later on. > > Just keeping the locks in undocumented state will give the impression that > "anything goes" and we're heading towards the cliff... Exactly :) if we can move towards a "only contend on write" model later, then that would at least be a win.
On 2015/10/16 08:54:02, tommi (webrtc) wrote: > On 2015/10/16 08:46:55, the sun wrote: > > On 2015/10/15 21:09:53, tommi (webrtc) wrote: > > > On 2015/10/15 15:32:12, pbos-webrtc wrote: > > > > I don't think we should enforce that stream configuration and network is > > done > > > on > > > > the same thread, especially when that thread is "the worker thread" used > for > > > > everything (I think that's an implementation defect we have today), so I > > > prefer > > > > keeping the locks. It makes sense to deliver stuff straight from the > socket > > > here > > > > and not enforce thread jumps. > > > > > > Having a well understood thread model is good and I don't see assumptions > > about > > > libjingle's worker thread (can you point it out?). > > > > > > Keeping the locks means potential of contention. Do you know what threads > > could > > > content? Is it acceptable to block the network thread or any of the other > > > threads? > > > > > > In my opinion it's a good thing to enforce correct thread usage and the more > > > strict the threading requirements are, the better we understand how things > > > actually hang together. If that means enforcing thread jumps, fine (see > > > Chromium). If this doesn't make sense in this particular case though, it > > would > > > be good to understand why not. > > > > > > > > > > > > > > > > > https://codereview.webrtc.org/1397123003/diff/80001/webrtc/audio/audio_send_s... > > > > File webrtc/audio/audio_send_stream.h (right): > > > > > > > > > > > > > > https://codereview.webrtc.org/1397123003/diff/80001/webrtc/audio/audio_send_s... > > > > webrtc/audio/audio_send_stream.h:17: > > > > remove empty line > > > > > > > > > > > > > > https://codereview.webrtc.org/1397123003/diff/80001/webrtc/audio/audio_send_s... > > > > File webrtc/audio/audio_send_stream_unittest.cc (right): > > > > > > > > > > > > > > https://codereview.webrtc.org/1397123003/diff/80001/webrtc/audio/audio_send_s... > > > > webrtc/audio/audio_send_stream_unittest.cc:26: EXPECT_LT(0u, > > > > config.ToString().size()); > > > > pref GT(size, 0u) > > > > > > > > > > > > > > https://codereview.webrtc.org/1397123003/diff/80001/webrtc/call/call_unittest.cc > > > > File webrtc/call/call_unittest.cc (right): > > > > > > > > > > > > > > https://codereview.webrtc.org/1397123003/diff/80001/webrtc/call/call_unittest... > > > > webrtc/call/call_unittest.cc:100: } // namespace webrtc > > > > Do you want to add some of these for Video equivalents? > > > > > > > > > https://codereview.webrtc.org/1397123003/diff/80001/webrtc/webrtc_tests.gypi > > > > File webrtc/webrtc_tests.gypi (right): > > > > > > > > > > > > > > https://codereview.webrtc.org/1397123003/diff/80001/webrtc/webrtc_tests.gypi#... > > > > webrtc/webrtc_tests.gypi:149: # TODO(solenberg): Rename to > > webrtc_call_tests? > > > > Imo split up between audio, video, common. It's useful for local testing > and > > > > "we" generally don't touch "audio" tests, so they add runtime for us. > > > > > > yeah, not sure I agree with this, definitely not about splitting the code > > based > > > on sub teams. You and I are on the same team, right? :) > > > > > > Btw, by 'runtime' are you referring to something like a runtime library or > the > > > time it takes to run the tests? I don't think the former is true and the > > latter > > > can be fixed in other ways than adding more executables. > > > > Regarding locks vs thread checker, how about keeping the locks but adding a > > thread checker but keep the locks? > > > > sgtm > > > That way multithreading assumptions would at least be documented (functions > > called on network thread would do a false-check) and we can make an effort to > > remove the locks later on. > > > > Just keeping the locks in undocumented state will give the impression that > > "anything goes" and we're heading towards the cliff... > > Exactly :) if we can move towards a "only contend on write" model later, then > that would at least be a win. https://codereview.webrtc.org/1403353003/
Ping, can we move ahead?
https://codereview.webrtc.org/1397123003/diff/100001/webrtc/call/call_unittes... File webrtc/call/call_unittest.cc (right): https://codereview.webrtc.org/1397123003/diff/100001/webrtc/call/call_unittes... webrtc/call/call_unittest.cc:30: }; I haven't seen this pattern used in many other unittests before. Typically people use test fixtures instead. Would that not make sense here?
solenberg@google.com changed reviewers: + solenberg@google.com
https://codereview.webrtc.org/1397123003/diff/100001/webrtc/call/call_unittes... File webrtc/call/call_unittest.cc (right): https://codereview.webrtc.org/1397123003/diff/100001/webrtc/call/call_unittes... webrtc/call/call_unittest.cc:30: }; On 2015/10/16 09:25:41, stefan-webrtc (holmer) wrote: > I haven't seen this pattern used in many other unittests before. Typically > people use test fixtures instead. Would that not make sense here? gtest supports tests with/without fixtures. Generally I think tests that don't use fixtures is more straightforward because there is less happening behind the scenes. We should use less of them and instead make it easy to set up the required state inline.
lgtm https://codereview.webrtc.org/1397123003/diff/100001/webrtc/call/call_unittes... File webrtc/call/call_unittest.cc (right): https://codereview.webrtc.org/1397123003/diff/100001/webrtc/call/call_unittes... webrtc/call/call_unittest.cc:30: }; On 2015/10/16 09:41:46, solenberg wrote: > On 2015/10/16 09:25:41, stefan-webrtc (holmer) wrote: > > I haven't seen this pattern used in many other unittests before. Typically > > people use test fixtures instead. Would that not make sense here? > > gtest supports tests with/without fixtures. > > Generally I think tests that don't use fixtures is more straightforward because > there is less happening behind the scenes. We should use less of them and > instead make it easy to set up the required state inline. Ok, I'm not going to block the CL on this, I was mostly curious as of why it was done differently.
solenberg@webrtc.org changed reviewers: - solenberg@google.com
https://codereview.webrtc.org/1397123003/diff/100001/webrtc/call/call_unittes... File webrtc/call/call_unittest.cc (right): https://codereview.webrtc.org/1397123003/diff/100001/webrtc/call/call_unittes... webrtc/call/call_unittest.cc:64: for (uint32_t ssrc = 0; ssrc < 1234567; ssrc += 34567) { Is there any point to this pattern? Also, can you test that you can re-add these streams again (e.g. that the SSRC isn't "taken" permanently)? https://codereview.webrtc.org/1397123003/diff/100001/webrtc/call/call_unittes... webrtc/call/call_unittest.cc:74: while (!streams.empty()) { for (AudioSendStream* stream : streams), clear after or skip pop_front() completely. https://codereview.webrtc.org/1397123003/diff/100001/webrtc/call/call_unittes... webrtc/call/call_unittest.cc:95: while (!streams.empty()) { same
(lgtm, I trust you take care of the rest)
solenberg@google.com changed reviewers: + solenberg@google.com
https://codereview.webrtc.org/1397123003/diff/100001/webrtc/call/call_unittes... File webrtc/call/call_unittest.cc (right): https://codereview.webrtc.org/1397123003/diff/100001/webrtc/call/call_unittes... webrtc/call/call_unittest.cc:64: for (uint32_t ssrc = 0; ssrc < 1234567; ssrc += 34567) { On 2015/10/16 10:48:29, pbos-webrtc wrote: > Is there any point to this pattern? > > Also, can you test that you can re-add these streams again (e.g. that the SSRC > isn't "taken" permanently)? 1. No. The intent is to add a bunch of streams and remove them in a different order. 2. Ok. https://codereview.webrtc.org/1397123003/diff/100001/webrtc/call/call_unittes... webrtc/call/call_unittest.cc:74: while (!streams.empty()) { On 2015/10/16 10:48:29, pbos-webrtc wrote: > for (AudioSendStream* stream : streams), clear after or skip pop_front() > completely. Done. https://codereview.webrtc.org/1397123003/diff/100001/webrtc/call/call_unittes... webrtc/call/call_unittest.cc:95: while (!streams.empty()) { On 2015/10/16 10:48:29, pbos-webrtc wrote: > same Done.
https://codereview.webrtc.org/1397123003/diff/100001/webrtc/call/call_unittes... File webrtc/call/call_unittest.cc (right): https://codereview.webrtc.org/1397123003/diff/100001/webrtc/call/call_unittes... webrtc/call/call_unittest.cc:26: webrtc::Call* operator()() { return call_.get(); } what about overloading operator -> instead? looks like that would change call()->Foo() to call->Foo()
https://codereview.webrtc.org/1397123003/diff/120001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/1397123003/diff/120001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:52: LOG(LS_INFO) << "~AudioSendStream: " << config_.ToString(); does this go into the diagnostic log or are these just debug traces? It feels a bit unfortunate that we're generating the same string multiple times. https://codereview.webrtc.org/1397123003/diff/120001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/1397123003/diff/120001/webrtc/call/call.cc#newc... webrtc/call/call.cc:193: audio_send_ssrcs_[config.rtp.ssrc] = send_stream; nit: could use insert() since the lookup should not be necessary https://codereview.webrtc.org/1397123003/diff/120001/webrtc/call/call.cc#newc... webrtc/call/call.cc:208: static_cast<webrtc::internal::AudioSendStream*>(send_stream); nice :) https://codereview.webrtc.org/1397123003/diff/120001/webrtc/webrtc_tests.gypi File webrtc/webrtc_tests.gypi (right): https://codereview.webrtc.org/1397123003/diff/120001/webrtc/webrtc_tests.gypi... webrtc/webrtc_tests.gypi:149: # TODO(solenberg): Rename to webrtc_call_tests? it sounded to me like this was not desired. Is it still a todo or should we just remove it?
oh and if you can make the CL description a little bit more verbose (see my first review comment) that would be good.
solenberg@webrtc.org changed reviewers: - solenberg@google.com
https://codereview.webrtc.org/1397123003/diff/100001/webrtc/call/call_unittes... File webrtc/call/call_unittest.cc (right): https://codereview.webrtc.org/1397123003/diff/100001/webrtc/call/call_unittes... webrtc/call/call_unittest.cc:26: webrtc::Call* operator()() { return call_.get(); } On 2015/10/16 11:08:43, tommi (webrtc) wrote: > what about overloading operator -> instead? > looks like that would change call()->Foo() to call->Foo() correct; silly moi https://codereview.webrtc.org/1397123003/diff/120001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/1397123003/diff/120001/webrtc/call/call.cc#newc... webrtc/call/call.cc:193: audio_send_ssrcs_[config.rtp.ssrc] = send_stream; On 2015/10/16 11:15:33, tommi (webrtc) wrote: > nit: could use insert() since the lookup should not be necessary I'm keeping it consistent with how the other methods do... https://codereview.webrtc.org/1397123003/diff/120001/webrtc/call/call.cc#newc... webrtc/call/call.cc:208: static_cast<webrtc::internal::AudioSendStream*>(send_stream); On 2015/10/16 11:15:33, tommi (webrtc) wrote: > nice :) Acknowledged. https://codereview.webrtc.org/1397123003/diff/120001/webrtc/webrtc_tests.gypi File webrtc/webrtc_tests.gypi (right): https://codereview.webrtc.org/1397123003/diff/120001/webrtc/webrtc_tests.gypi... webrtc/webrtc_tests.gypi:149: # TODO(solenberg): Rename to webrtc_call_tests? On 2015/10/16 11:15:33, tommi (webrtc) wrote: > it sounded to me like this was not desired. Is it still a todo or should we just > remove it? I think the name is bad and should be changed, but we can live with it until infrastructure allows us to do that more easily.
On 2015/10/16 11:30:01, the sun wrote: > https://codereview.webrtc.org/1397123003/diff/100001/webrtc/call/call_unittes... > File webrtc/call/call_unittest.cc (right): > > https://codereview.webrtc.org/1397123003/diff/100001/webrtc/call/call_unittes... > webrtc/call/call_unittest.cc:26: webrtc::Call* operator()() { return > call_.get(); } > On 2015/10/16 11:08:43, tommi (webrtc) wrote: > > what about overloading operator -> instead? > > looks like that would change call()->Foo() to call->Foo() > > correct; silly moi > > https://codereview.webrtc.org/1397123003/diff/120001/webrtc/call/call.cc > File webrtc/call/call.cc (right): > > https://codereview.webrtc.org/1397123003/diff/120001/webrtc/call/call.cc#newc... > webrtc/call/call.cc:193: audio_send_ssrcs_[config.rtp.ssrc] = send_stream; > On 2015/10/16 11:15:33, tommi (webrtc) wrote: > > nit: could use insert() since the lookup should not be necessary > > I'm keeping it consistent with how the other methods do... > > https://codereview.webrtc.org/1397123003/diff/120001/webrtc/call/call.cc#newc... > webrtc/call/call.cc:208: > static_cast<webrtc::internal::AudioSendStream*>(send_stream); > On 2015/10/16 11:15:33, tommi (webrtc) wrote: > > nice :) > > Acknowledged. > > https://codereview.webrtc.org/1397123003/diff/120001/webrtc/webrtc_tests.gypi > File webrtc/webrtc_tests.gypi (right): > > https://codereview.webrtc.org/1397123003/diff/120001/webrtc/webrtc_tests.gypi... > webrtc/webrtc_tests.gypi:149: # TODO(solenberg): Rename to webrtc_call_tests? > On 2015/10/16 11:15:33, tommi (webrtc) wrote: > > it sounded to me like this was not desired. Is it still a todo or should we > just > > remove it? > > I think the name is bad and should be changed, but we can live with it until > infrastructure allows us to do that more easily. Any further objections, or can I commit this today?
ping...
The CQ bit was checked by tommi@webrtc.org
lgtm
The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/1397123003/#ps180001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1397123003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1397123003/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_dbg on tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by solenberg@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1397123003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1397123003/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/c7a8b08a7cd8d8f37d7f5fb9930d0cdc74baba35 Cr-Commit-Position: refs/heads/master@{#10307} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
