Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2286343002:
Less lock acquisitions for AudioMixer. (Closed)
|
Created:
4 years, 3 months ago by aleloi Modified:
4 years, 3 months ago CC:
webrtc-reviews_webrtc.org, interface-changes_webrtc.org, yujie_mao (webrtc), Andrew MacDonald, henrika_webrtc, hlundin-webrtc, tlegrand-webrtc, tterriberry_mozilla.com, audio-team_agora.io, qiang.lu, niklas.enbom, peah-webrtc, minyue-webrtc, the sun, aluebs-webrtc, bjornv1 Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionSeveral lock acquisitions and one of the two lock members are removed. ENSURE_LOCKS_REQUIRED and CalledOnValidThread annotations are added.
NOTRY=True
Committed: https://crrev.com/311525e71559227e0cc762236450cd0cbdbde636
Cr-Commit-Position: refs/heads/master@{#14106}
Patch Set 1 : A huge pile of messy changes (left because of the comments) #
Total comments: 25
Patch Set 2 #
Total comments: 9
Patch Set 3 : Changes in response to reviewer comments. #
Total comments: 9
Patch Set 4 : GUARDED_BY, ACCESS_ON, C header and EXPECT_EQ order. #
Total comments: 5
Patch Set 5 : Added ACCESS_ON to limiter and added threading unit test. #
Total comments: 1
Patch Set 6 : Construction and initialization. #
Total comments: 6
Patch Set 7 : Removed 'explicit' and changed lock type. #Patch Set 8 : Rebase. #
Dependent Patchsets: Messages
Total messages: 59 (29 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== Fix Chromium clang plugin warnings. This adds a new file, //webrtc/common.cc, and a new target //webrtc:config_options and also BUG=webrtc:163 ========== to ========== Fix Chromium clang plugin warnings. This adds a new file, //webrtc/common.cc, and a new target //webrtc:config_options. BUG=webrtc:163 ==========
aleloi@webrtc.org changed reviewers: + kjellander@webrtc.org
https://codereview.webrtc.org/2286343002/diff/20001/webrtc/BUILD.gn File webrtc/BUILD.gn (right): https://codereview.webrtc.org/2286343002/diff/20001/webrtc/BUILD.gn#newcode357 webrtc/BUILD.gn:357: source_set("config_options") { New target for common.h / common.cc. I decided to name it config_options rather than |common| or |options_common|. https://codereview.webrtc.org/2286343002/diff/20001/webrtc/BUILD.gn#newcode367 webrtc/BUILD.gn:367: "base:rtc_base_approved", Dependency since it uses disable constructors macro. https://codereview.webrtc.org/2286343002/diff/20001/webrtc/common.cc File webrtc/common.cc (left): https://codereview.webrtc.org/2286343002/diff/20001/webrtc/common.cc#oldcode11 webrtc/common.cc:11: #include "webrtc/test/run_test.h" Bogus copy, I'll try tune --similarity for eventual coming patch sets. https://codereview.webrtc.org/2286343002/diff/20001/webrtc/common.h File webrtc/common.h (left): https://codereview.webrtc.org/2286343002/diff/20001/webrtc/common.h#oldcode76 webrtc/common.h:76: // Note: this method is inline so webrtc public API depends only This file gets included almost everywhere, and therefore inlining prevents from enabling the clang warnings. I guess clang warnings are more important than depending on headers only.
kjellander@webrtc.org changed reviewers: + kwiberg@webrtc.org
I'm afraid kwiberg beat you to the party here with his https://codereview.webrtc.org/2288593004/ + a series of other CLs. I guess it's easiest to let him land his (mostly done already) and then rebase this?
On 2016/08/29 19:24:08, kjellander_webrtc wrote: > I'm afraid kwiberg beat you to the party here with his > https://codereview.webrtc.org/2288593004/ + a series of other CLs. > > I guess it's easiest to let him land his (mostly done already) and then rebase > this? sgtm
On 2016/08/29 21:05:01, kwiberg-webrtc wrote: > On 2016/08/29 19:24:08, kjellander_webrtc wrote: > > I'm afraid kwiberg beat you to the party here with his > > https://codereview.webrtc.org/2288593004/ + a series of other CLs. > > > > I guess it's easiest to let him land his (mostly done already) and then rebase > > this? > > sgtm Sounds good to me too!
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Description was changed from ========== Fix Chromium clang plugin warnings. This adds a new file, //webrtc/common.cc, and a new target //webrtc:config_options. BUG=webrtc:163 ========== to ========== TODO fill in later. ==========
aleloi@webrtc.org changed reviewers: - kjellander@webrtc.org, kwiberg@webrtc.org
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
Patchset #1 (id:120001) has been deleted
Description was changed from ========== TODO fill in later. ========== to ========== Several lock acquisitions are removed. Methods are named more consistently and have a more consistent signatures. The call structure of mixing is slightly simplified. Anonymous participants are also ramped up. ==========
aleloi@webrtc.org changed reviewers: + ivoc@webrtc.org, kwiberg@webrtc.org
Hi! Could you take a look at this? It's more mixer improvements. It's a bit largish, but I tried to describe the reason for the changes in comments. https://codereview.webrtc.org/2286343002/diff/140001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/BUILD.gn (left): https://codereview.webrtc.org/2286343002/diff/140001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/BUILD.gn:40: Now that //webrtc/common.h is fixed by kwiberg@, I can enable the clang warnings here. https://codereview.webrtc.org/2286343002/diff/140001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_mixer.h (left): https://codereview.webrtc.org/2286343002/diff/140001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer.h:31: kLowestPossible = -1, Not used any longer. https://codereview.webrtc.org/2286343002/diff/140001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_mixer.h (right): https://codereview.webrtc.org/2286343002/diff/140001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer.h:74: RTC_DISALLOW_COPY_AND_ASSIGN(AudioMixer); Copying a mixer should be forbidden, as the mixer expects only it can ask AudioSources for audio. There hasn't been any use case for assignment. https://codereview.webrtc.org/2286343002/diff/140001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_mixer_impl.cc (left): https://codereview.webrtc.org/2286343002/diff/140001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.cc:84: *mixed_frame += *frame; MixFrames is now done in MixFromList. https://codereview.webrtc.org/2286343002/diff/140001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.cc:193: std::map<int, MixerAudioSource*> mixedAudioSourcesMap; Isn't used any longer. https://codereview.webrtc.org/2286343002/diff/140001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.cc:214: } The frequency is no longer guarded by a lock. This is because only the mixing thread accesses it. This is enforced by CalledOnValidThread() https://codereview.webrtc.org/2286343002/diff/140001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.cc:239: // We only use the limiter if it supports the output sample rate and It should support all sample rates now. https://codereview.webrtc.org/2286343002/diff/140001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.cc:254: } There is no access to members that can be modified by another theard. Therefore no lock. https://codereview.webrtc.org/2286343002/diff/140001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.cc:317: // Note: The scratch buffer may only be updated in Process(). Scratch buffer and Process() comment seems stale https://codereview.webrtc.org/2286343002/diff/140001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.cc:319: num_mixed_audio_sources_ = numMixedAudioSources; AFAIK, no need to guard |num_sources| with other lock https://codereview.webrtc.org/2286343002/diff/140001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.cc:454: continue; Replaced this with a RTC_DCHECK_EQ in MixFromList. Frames should never be empty. https://codereview.webrtc.org/2286343002/diff/140001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.cc:508: Was unused. https://codereview.webrtc.org/2286343002/diff/140001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.cc:532: // TODO(andrew): consolidate this function with MixFromList. Done :) https://codereview.webrtc.org/2286343002/diff/140001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_mixer_impl.cc (right): https://codereview.webrtc.org/2286343002/diff/140001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.cc:49: Differs from the above by not calculating energy. A structure with source, frame and previous mixed status was needed for |Ramp()|. I reused |SourceFrame|. https://codereview.webrtc.org/2286343002/diff/140001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.cc:94: } Ramping in/out should be done both for anonymous and ordinary participants. I broke it out to a function to avoid code duplication. https://codereview.webrtc.org/2286343002/diff/140001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.cc:231: } This is the only section that accesses the participant lists. https://codereview.webrtc.org/2286343002/diff/140001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.cc:371: std::vector<SourceFrame> ramp_list; See comment about 2:nd SourceFrame constructor. https://codereview.webrtc.org/2286343002/diff/140001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_mixer_impl.h (left): https://codereview.webrtc.org/2286343002/diff/140001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.h:98: const MixerAudioSourceList& mixList) const; These two have not been used for the last 3-4 mixer changes. https://codereview.webrtc.org/2286343002/diff/140001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.h:126: Merged with MixFromList https://codereview.webrtc.org/2286343002/diff/140001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.h:135: std::unique_ptr<CriticalSectionWrapper> cb_crit_; I think only one lock is needed. Adding/removing/querying status of participants can happen on different threads. In addition the mixing is done on another thread. The public adding/removing/querying methods are all guarded by cb_crit_. Getting audio from participants and updating participants mixing history is all done guarded by cb_crit_. The other parts of mixing do not hold locks. AFAIK this is enough, since the mixing is done on a single thread. https://codereview.webrtc.org/2286343002/diff/140001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_mixer_impl.h (right): https://codereview.webrtc.org/2286343002/diff/140001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.h:35: typedef std::list<AudioFrame*> AudioFrameList; Mixing structure is a little simplified. Mute information is only used inside of Get*AnonymousAudio. Muted frames are filtered out one step earlier. Therefore a |muted| field is no longer needed here. https://codereview.webrtc.org/2286343002/diff/140001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.h:96: AudioFrameList GetAnonymousAudio() const; Made naming and signature more consistent between the Get*Audio methods. https://codereview.webrtc.org/2286343002/diff/140001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc (right): https://codereview.webrtc.org/2286343002/diff/140001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:216: participant.fake_frame()->samples_per_channel_ = frequency / 100; There are now RTC_DCHECKs for receiving correct rate and samples/channel from audio sources.
It could just be me, but I find this CL hard to follow. It's large, and tries to do several things. Is there a way to split it up into a sequence of CLs that each do one thing? https://codereview.webrtc.org/2286343002/diff/140001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_mixer_impl.h (right): https://codereview.webrtc.org/2286343002/diff/140001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.h:33: }; Is this struct still used somewhere, or can you remove it? https://codereview.webrtc.org/2286343002/diff/140001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.h:91: // many participants are allowed to be mixed. This comment needs updating.
Description was changed from ========== Several lock acquisitions are removed. Methods are named more consistently and have a more consistent signatures. The call structure of mixing is slightly simplified. Anonymous participants are also ramped up. ========== to ========== Several lock acquisitions and one of the two lock members are removed. ENSURE_LOCKS_REQUIRED and CalledOnValidThread annotations are added. ==========
I removed everything except the lock and threading parts. The other changes are more simple and will be uploaded as part of the next 2-3 CLs.
Good! Now it's *much* easier to read. https://codereview.webrtc.org/2286343002/diff/160001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_mixer_impl.cc (left): https://codereview.webrtc.org/2286343002/diff/160001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.cc:245: MixAnonomouslyFromList(audio_frame_for_mixing, additionalFramesList); Where did this line go? https://codereview.webrtc.org/2286343002/diff/160001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_mixer_impl.cc (right): https://codereview.webrtc.org/2286343002/diff/160001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.cc:358: EXCLUSIVE_LOCKS_REQUIRED(crit_) { IIRC we always put these annotations in the .h file. That way, all lock annotations (both method and member variable) are in the same place. Also, it makes sense since the required locks are part of the interface when you call the method. Actually, now that I think of it it can't be safe to have the annotation only in the .cc file. If someone calls this method from another compilation unit, the compiler won't know that the caller needs to hold the lock. I'm guessing this is a private method, so we won't hit that problem in this case, but it's another reason for just doing the simple thing and put all the annotations in the header. Ooooh. Even if it's private, it can still be problematic. Consider if another compilation unit calls a public inline method of your class, which in turn calls this private method. https://codereview.webrtc.org/2286343002/diff/160001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.cc:416: const EXCLUSIVE_LOCKS_REQUIRED(crit_) { Move to .h file. https://codereview.webrtc.org/2286343002/diff/160001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_mixer_impl.h (left): https://codereview.webrtc.org/2286343002/diff/160001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.h:135: std::unique_ptr<CriticalSectionWrapper> cb_crit_; It's troubling that there aren't lock annotations on the member variables that tell the compiler what the locks are supposed to protect. To prevent mistakes, it's probably best to start by adding GUARDED_BY annotations everywhere... (base/thread_checker.h seems to have corresponding annotations for thread checkers.)
https://codereview.webrtc.org/2286343002/diff/160001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_mixer_impl.cc (right): https://codereview.webrtc.org/2286343002/diff/160001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.cc:191: switch (sample_rate) { It's not really related to the threading/locking issue in this CL, but I find this switch statement a bit silly. I think it would be much simpler to write it like this: if (sample_rate != kNbInHz && sample_rate != kWbInHz && ...) { WEBRTC_TRACE(...); RTC_NOTREACHED(); return; } mixing_frequency = sample_rate;
Please take another look! https://codereview.webrtc.org/2286343002/diff/160001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_mixer_impl.cc (left): https://codereview.webrtc.org/2286343002/diff/160001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.cc:245: MixAnonomouslyFromList(audio_frame_for_mixing, additionalFramesList); On 2016/08/31 09:10:36, kwiberg-webrtc wrote: > Where did this line go? It should still be here, but be removed in the next dependent CL. The tests didn't catch it. I've put the line back now, and added a test. https://codereview.webrtc.org/2286343002/diff/160001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_mixer_impl.cc (right): https://codereview.webrtc.org/2286343002/diff/160001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.cc:191: switch (sample_rate) { On 2016/08/31 09:22:45, ivoc wrote: > It's not really related to the threading/locking issue in this CL, but I find > this switch statement a bit silly. I think it would be much simpler to write it > like this: > > if (sample_rate != kNbInHz && sample_rate != kWbInHz && ...) { > WEBRTC_TRACE(...); > RTC_NOTREACHED(); > return; > } > mixing_frequency = sample_rate; This is indeed simpler! Thanks! https://codereview.webrtc.org/2286343002/diff/160001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.cc:358: EXCLUSIVE_LOCKS_REQUIRED(crit_) { On 2016/08/31 09:10:36, kwiberg-webrtc wrote: > IIRC we always put these annotations in the .h file. That way, all lock > annotations (both method and member variable) are in the same place. Also, it > makes sense since the required locks are part of the interface when you call the > method. > > Actually, now that I think of it it can't be safe to have the annotation only in > the .cc file. If someone calls this method from another compilation unit, the > compiler won't know that the caller needs to hold the lock. I'm guessing this is > a private method, so we won't hit that problem in this case, but it's another > reason for just doing the simple thing and put all the annotations in the > header. > > Ooooh. Even if it's private, it can still be problematic. Consider if another > compilation unit calls a public inline method of your class, which in turn calls > this private method. This makes sense! Thanks! I've moved the annotation to the header. https://codereview.webrtc.org/2286343002/diff/160001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.cc:416: const EXCLUSIVE_LOCKS_REQUIRED(crit_) { On 2016/08/31 09:10:36, kwiberg-webrtc wrote: > Move to .h file. Done.
https://codereview.webrtc.org/2286343002/diff/180001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc (right): https://codereview.webrtc.org/2286343002/diff/180001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:235: TEST(AudioMixer, FrameNotModifiedForSingleAnonymousParticipant) { This test compares mixer output in the anonymous case. It fails if the line |MixAnonomouslyFromList(audio_frame_for_mixing, additionalFramesList);| is removed. Previously, there were a few tests that compared audio output with the old mixer. They didn't use anonymous participants.
https://codereview.webrtc.org/2286343002/diff/180001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_mixer_impl.h (right): https://codereview.webrtc.org/2286343002/diff/180001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.h:136: std::unique_ptr<CriticalSectionWrapper> crit_; Please also add GUARDED_BY(crit_) to any members that are protected by this critical section. https://codereview.webrtc.org/2286343002/diff/180001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc (right): https://codereview.webrtc.org/2286343002/diff/180001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:11: #include <cstring> I think we generally use the <string.h> style for C headers. https://codereview.webrtc.org/2286343002/diff/180001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:230: EXPECT_EQ(std::memcmp(participant.fake_frame()->data_, audio_frame.data_, Please change the order, i.e. EXPECT_EQ(0, std::memcmp(...)); https://codereview.webrtc.org/2286343002/diff/180001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:260: EXPECT_EQ(std::memcmp(participant.fake_frame()->data_, audio_frame.data_, Same here.
Updated in response to comments. https://codereview.webrtc.org/2286343002/diff/180001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_mixer_impl.h (right): https://codereview.webrtc.org/2286343002/diff/180001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.h:136: std::unique_ptr<CriticalSectionWrapper> crit_; On 2016/08/31 14:34:24, ivoc wrote: > Please also add GUARDED_BY(crit_) to any members that are protected by this > critical section. Done https://codereview.webrtc.org/2286343002/diff/180001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc (right): https://codereview.webrtc.org/2286343002/diff/180001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:11: #include <cstring> On 2016/08/31 14:34:25, ivoc wrote: > I think we generally use the <string.h> style for C headers. Seems like that. 21 matches for cstring and 243 for string.h in the code base. Also, no matches for std::memcmp, but lots of matches for just memcmp. Done. https://codereview.webrtc.org/2286343002/diff/180001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:230: EXPECT_EQ(std::memcmp(participant.fake_frame()->data_, audio_frame.data_, On 2016/08/31 14:34:24, ivoc wrote: > Please change the order, i.e. EXPECT_EQ(0, std::memcmp(...)); Done. I also changed in a few other tests. https://codereview.webrtc.org/2286343002/diff/180001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:260: EXPECT_EQ(std::memcmp(participant.fake_frame()->data_, audio_frame.data_, On 2016/08/31 14:34:24, ivoc wrote: > Same here. Done.
https://codereview.webrtc.org/2286343002/diff/200001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_mixer_impl.h (right): https://codereview.webrtc.org/2286343002/diff/200001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.h:150: size_t num_mixed_audio_sources_ GUARDED_BY(crit_); Good, but it makes me very suspicious that this class has a large number of member variables that aren't GUARDED_BY(crit_).
https://codereview.webrtc.org/2286343002/diff/200001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_mixer_impl.h (right): https://codereview.webrtc.org/2286343002/diff/200001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.h:150: size_t num_mixed_audio_sources_ GUARDED_BY(crit_); On 2016/09/01 11:19:00, kwiberg-webrtc wrote: > Good, but it makes me very suspicious that this class has a large number of > member variables that aren't GUARDED_BY(crit_). The other ones are only accessed by the mixer functions, which should only run from a single thread. The entry function Mix has a checker for that. The only multithreaded ones are add/remove/query participants, and they only access the GUARDED_BY vars.
https://codereview.webrtc.org/2286343002/diff/200001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_mixer_impl.h (right): https://codereview.webrtc.org/2286343002/diff/200001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.h:150: size_t num_mixed_audio_sources_ GUARDED_BY(crit_); On 2016/09/01 11:24:39, aleloi wrote: > On 2016/09/01 11:19:00, kwiberg-webrtc wrote: > > Good, but it makes me very suspicious that this class has a large number of > > member variables that aren't GUARDED_BY(crit_). > > The other ones are only accessed by the mixer functions, which should only run > from a single thread. The entry function Mix has a checker for that. > > The only multithreaded ones are add/remove/query participants, and they only > access the GUARDED_BY vars. Could you annotate them with the thread checker stuff I pointed to in an earlier comment? (I think thread checker annotations are rather new, so I have no personal experience of using them, but it looks like it'd be the right thing to use here.) Having all the member variables annotated will make it plain what's being protected by what. With this many variables and this much code, avoiding mistakes by hand is too hard.
https://codereview.webrtc.org/2286343002/diff/200001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_mixer_impl.h (right): https://codereview.webrtc.org/2286343002/diff/200001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.h:150: size_t num_mixed_audio_sources_ GUARDED_BY(crit_); On 2016/09/01 11:32:28, kwiberg-webrtc wrote: > On 2016/09/01 11:24:39, aleloi wrote: > > On 2016/09/01 11:19:00, kwiberg-webrtc wrote: > > > Good, but it makes me very suspicious that this class has a large number of > > > member variables that aren't GUARDED_BY(crit_). > > > > The other ones are only accessed by the mixer functions, which should only run > > from a single thread. The entry function Mix has a checker for that. > > > > The only multithreaded ones are add/remove/query participants, and they only > > access the GUARDED_BY vars. > > Could you annotate them with the thread checker stuff I pointed to in an earlier > comment? (I think thread checker annotations are rather new, so I have no > personal experience of using them, but it looks like it'd be the right thing to > use here.) > > Having all the member variables annotated will make it plain what's being > protected by what. With this many variables and this much code, avoiding > mistakes by hand is too hard. Good point! I've followed the example usage instructions in base/thread_checker.h and also how it was implemented in e.g. webrtc/video/rtp_streams_synchronizer.h The id_ field is accessed everywhere, but only read from, so I made it 'const'. The limiter_ is problematic. It is initialized in Init(), which is reasonable, since initialization could fail. But it's then accessed on the mixing thread, which could be different. I don't think one can express 'accessed by a single thread starting at the first call to ::Mix()'. It seems wrong to leave the limiter as is. If there is no better choice, I will add a comment describing its access status.
The CQ bit was checked by aleloi@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: android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_clang_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
On 2016/09/01 12:33:06, aleloi wrote: > https://codereview.webrtc.org/2286343002/diff/200001/webrtc/modules/audio_mix... > File webrtc/modules/audio_mixer/audio_mixer_impl.h (right): > > https://codereview.webrtc.org/2286343002/diff/200001/webrtc/modules/audio_mix... > webrtc/modules/audio_mixer/audio_mixer_impl.h:150: size_t > num_mixed_audio_sources_ GUARDED_BY(crit_); > On 2016/09/01 11:32:28, kwiberg-webrtc wrote: > > On 2016/09/01 11:24:39, aleloi wrote: > > > On 2016/09/01 11:19:00, kwiberg-webrtc wrote: > > > > Good, but it makes me very suspicious that this class has a large number > of > > > > member variables that aren't GUARDED_BY(crit_). > > > > > > The other ones are only accessed by the mixer functions, which should only > run > > > from a single thread. The entry function Mix has a checker for that. > > > > > > The only multithreaded ones are add/remove/query participants, and they only > > > access the GUARDED_BY vars. > > > > Could you annotate them with the thread checker stuff I pointed to in an > earlier > > comment? (I think thread checker annotations are rather new, so I have no > > personal experience of using them, but it looks like it'd be the right thing > to > > use here.) > > > > Having all the member variables annotated will make it plain what's being > > protected by what. With this many variables and this much code, avoiding > > mistakes by hand is too hard. > > Good point! I've followed the example usage instructions in > base/thread_checker.h and also how it was implemented in e.g. > webrtc/video/rtp_streams_synchronizer.h > > The id_ field is accessed everywhere, but only read from, so I made it 'const'. > > The limiter_ is problematic. It is initialized in Init(), which is reasonable, > since initialization could fail. But it's then accessed on the mixing thread, > which could be different. I don't think one can express 'accessed by a single > thread starting at the first call to ::Mix()'. > > It seems wrong to leave the limiter as is. If there is no better choice, I will > add a comment describing its access status. I have added access guards to the limiter as well now. The thread checker is reset after Init has been run by 'DetachFromThread'. I also added a unit test that helped to find and fix a bug: Init accesses the output frequency, which is marked ACCESS_ON(thread_checker_). Then it's illegal to run Init from a thread different from the mixing thread.
https://codereview.webrtc.org/2286343002/diff/200001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_mixer_impl.h (right): https://codereview.webrtc.org/2286343002/diff/200001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.h:150: size_t num_mixed_audio_sources_ GUARDED_BY(crit_); On 2016/09/01 12:33:06, aleloi wrote: > On 2016/09/01 11:32:28, kwiberg-webrtc wrote: > > On 2016/09/01 11:24:39, aleloi wrote: > > > On 2016/09/01 11:19:00, kwiberg-webrtc wrote: > > > > Good, but it makes me very suspicious that this class has a large number > of > > > > member variables that aren't GUARDED_BY(crit_). > > > > > > The other ones are only accessed by the mixer functions, which should only > run > > > from a single thread. The entry function Mix has a checker for that. > > > > > > The only multithreaded ones are add/remove/query participants, and they only > > > access the GUARDED_BY vars. > > > > Could you annotate them with the thread checker stuff I pointed to in an > earlier > > comment? (I think thread checker annotations are rather new, so I have no > > personal experience of using them, but it looks like it'd be the right thing > to > > use here.) > > > > Having all the member variables annotated will make it plain what's being > > protected by what. With this many variables and this much code, avoiding > > mistakes by hand is too hard. > > Good point! I've followed the example usage instructions in > base/thread_checker.h and also how it was implemented in e.g. > webrtc/video/rtp_streams_synchronizer.h Excellent! Did you verify that you got the expected compilation errors when accessing member variables from methods where you hadn't yet added the thread checker call? > The id_ field is accessed everywhere, but only read from, so I made it 'const'. Excellent choice. > The limiter_ is problematic. It is initialized in Init(), which is reasonable, > since initialization could fail. But it's then accessed on the mixing thread, > which could be different. I don't think one can express 'accessed by a single > thread starting at the first call to ::Mix()'. > > It seems wrong to leave the limiter as is. If there is no better choice, I will > add a comment describing its access status. Hmmm. Too bad we can't just throw an exception in the constructor... The problem is that the compiler doesn't know that you promise to call Init() once in the beginning and then never again, so it's doing the right thing when it complains. The root of the evil is the use of an Init() function, which results in a special constructed-but-not-yet-initialized state that's a pain to deal with. Could you make the constructor private, and have a Create() function instead of Init()? Then the Create() function could create all the stuff that might fail, and pass them as arguments to the private constructor. That way, we get rid of the constructed-but-not-yet-initialized state.
On 2016/09/02 07:45:45, kwiberg-webrtc wrote: > https://codereview.webrtc.org/2286343002/diff/200001/webrtc/modules/audio_mix... > File webrtc/modules/audio_mixer/audio_mixer_impl.h (right): > > https://codereview.webrtc.org/2286343002/diff/200001/webrtc/modules/audio_mix... > webrtc/modules/audio_mixer/audio_mixer_impl.h:150: size_t > num_mixed_audio_sources_ GUARDED_BY(crit_); > On 2016/09/01 12:33:06, aleloi wrote: > > On 2016/09/01 11:32:28, kwiberg-webrtc wrote: > > > On 2016/09/01 11:24:39, aleloi wrote: > > > > On 2016/09/01 11:19:00, kwiberg-webrtc wrote: > > > > > Good, but it makes me very suspicious that this class has a large number > > of > > > > > member variables that aren't GUARDED_BY(crit_). > > > > > > > > The other ones are only accessed by the mixer functions, which should only > > run > > > > from a single thread. The entry function Mix has a checker for that. > > > > > > > > The only multithreaded ones are add/remove/query participants, and they > only > > > > access the GUARDED_BY vars. > > > > > > Could you annotate them with the thread checker stuff I pointed to in an > > earlier > > > comment? (I think thread checker annotations are rather new, so I have no > > > personal experience of using them, but it looks like it'd be the right thing > > to > > > use here.) > > > > > > Having all the member variables annotated will make it plain what's being > > > protected by what. With this many variables and this much code, avoiding > > > mistakes by hand is too hard. > > > > Good point! I've followed the example usage instructions in > > base/thread_checker.h and also how it was implemented in e.g. > > webrtc/video/rtp_streams_synchronizer.h > > Excellent! Did you verify that you got the expected compilation errors when > accessing member variables from methods where you hadn't yet added the thread > checker call? > > > The id_ field is accessed everywhere, but only read from, so I made it > 'const'. > > Excellent choice. > > > The limiter_ is problematic. It is initialized in Init(), which is reasonable, > > since initialization could fail. But it's then accessed on the mixing thread, > > which could be different. I don't think one can express 'accessed by a single > > thread starting at the first call to ::Mix()'. > > > > It seems wrong to leave the limiter as is. If there is no better choice, I > will > > add a comment describing its access status. > > Hmmm. Too bad we can't just throw an exception in the constructor... > > The problem is that the compiler doesn't know that you promise to call Init() > once in the beginning and then never again, so it's doing the right thing when > it complains. The root of the evil is the use of an Init() function, which > results in a special constructed-but-not-yet-initialized state that's a pain to > deal with. > > Could you make the constructor private, and have a Create() function instead of > Init()? Then the Create() function could create all the stuff that might fail, > and pass them as arguments to the private constructor. That way, we get rid of > the constructed-but-not-yet-initialized state. Good idea! I've done as you suggested.
Patchset #7 (id:260001) has been deleted
https://codereview.webrtc.org/2286343002/diff/240001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_mixer_impl.cc (right): https://codereview.webrtc.org/2286343002/diff/240001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.cc:150: return false; This can't fail and is set in the constructor now. https://codereview.webrtc.org/2286343002/diff/280001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_mixer_impl.cc (right): https://codereview.webrtc.org/2286343002/diff/280001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.cc:129: : crit_(new CriticalSectionWrapper()), Comments in critical_section_wrapper.h request to use the crit sec constructor.
lgtm, but see nits Good job getting all the members annotated! https://codereview.webrtc.org/2286343002/diff/280001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_mixer_impl.cc (right): https://codereview.webrtc.org/2286343002/diff/280001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.cc:129: : crit_(new CriticalSectionWrapper()), On 2016/09/02 08:51:21, aleloi wrote: > Comments in critical_section_wrapper.h request to use the crit sec constructor. Are you referring to this comment? // Legacy factory method, being deprecated. Please use the constructor. // TODO(tommi): Remove the CriticalSectionWrapper class and move users over // to using rtc::CriticalSection. I think the preferred thing to do is to use rtc::CriticalSection. https://codereview.webrtc.org/2286343002/diff/280001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_mixer_impl.h (right): https://codereview.webrtc.org/2286343002/diff/280001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.h:82: explicit AudioMixerImpl(int id, std::unique_ptr<AudioProcessing> limiter); Since this one now takes a non-1 number of arguments, you can drop the "explicit".
https://codereview.webrtc.org/2286343002/diff/280001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_mixer_impl.cc (right): https://codereview.webrtc.org/2286343002/diff/280001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.cc:129: : crit_(new CriticalSectionWrapper()), On 2016/09/02 09:00:26, kwiberg-webrtc wrote: > On 2016/09/02 08:51:21, aleloi wrote: > > Comments in critical_section_wrapper.h request to use the crit sec > constructor. > > Are you referring to this comment? > > // Legacy factory method, being deprecated. Please use the constructor. > // TODO(tommi): Remove the CriticalSectionWrapper class and move users over > // to using rtc::CriticalSection. > > I think the preferred thing to do is to use rtc::CriticalSection. I did a 'git grep' for CriticalSection. The constructor is not used anywhere at all and everyone still use the Wrapper. Do you think it's safe to use it? Maybe we should ask tommi@? https://codereview.webrtc.org/2286343002/diff/280001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_mixer_impl.h (right): https://codereview.webrtc.org/2286343002/diff/280001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.h:82: explicit AudioMixerImpl(int id, std::unique_ptr<AudioProcessing> limiter); On 2016/09/02 09:00:26, kwiberg-webrtc wrote: > Since this one now takes a non-1 number of arguments, you can drop the > "explicit". Done.
https://codereview.webrtc.org/2286343002/diff/280001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_mixer_impl.cc (right): https://codereview.webrtc.org/2286343002/diff/280001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.cc:129: : crit_(new CriticalSectionWrapper()), On 2016/09/02 09:27:28, aleloi wrote: > On 2016/09/02 09:00:26, kwiberg-webrtc wrote: > > On 2016/09/02 08:51:21, aleloi wrote: > > > Comments in critical_section_wrapper.h request to use the crit sec > > constructor. > > > > Are you referring to this comment? > > > > // Legacy factory method, being deprecated. Please use the constructor. > > // TODO(tommi): Remove the CriticalSectionWrapper class and move users over > > // to using rtc::CriticalSection. > > > > I think the preferred thing to do is to use rtc::CriticalSection. > > I did a 'git grep' for CriticalSection. The constructor is not used anywhere at > all and everyone still use the Wrapper. Do you think it's safe to use it? Maybe > we should ask tommi@? I'm guessing that although no one explicitly calls the constructor, it's still being called. My grep finds a bunch of classes with rtc::CriticalSection members, and a class's constructor will always call the constructors of all member variables one way or another.
Patchset #8 (id:300001) has been deleted
Patchset #5 (id:220001) has been deleted
LGTM.
The CQ bit was checked by aleloi@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kwiberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/2286343002/#ps340001 (title: "Rebase.")
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: android_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/16288)
Description was changed from ========== Several lock acquisitions and one of the two lock members are removed. ENSURE_LOCKS_REQUIRED and CalledOnValidThread annotations are added. ========== to ========== Several lock acquisitions and one of the two lock members are removed. ENSURE_LOCKS_REQUIRED and CalledOnValidThread annotations are added. NOTRY=True ==========
The CQ bit was checked by aleloi@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Several lock acquisitions and one of the two lock members are removed. ENSURE_LOCKS_REQUIRED and CalledOnValidThread annotations are added. NOTRY=True ========== to ========== Several lock acquisitions and one of the two lock members are removed. ENSURE_LOCKS_REQUIRED and CalledOnValidThread annotations are added. NOTRY=True ==========
Message was sent while issue was closed.
Committed patchset #8 (id:340001)
Message was sent while issue was closed.
Description was changed from ========== Several lock acquisitions and one of the two lock members are removed. ENSURE_LOCKS_REQUIRED and CalledOnValidThread annotations are added. NOTRY=True ========== to ========== Several lock acquisitions and one of the two lock members are removed. ENSURE_LOCKS_REQUIRED and CalledOnValidThread annotations are added. NOTRY=True Committed: https://crrev.com/311525e71559227e0cc762236450cd0cbdbde636 Cr-Commit-Position: refs/heads/master@{#14106} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/311525e71559227e0cc762236450cd0cbdbde636 Cr-Commit-Position: refs/heads/master@{#14106} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
