Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1678813002:
Moved the GainControlForNewAGC class to a separate file. (Closed)
|
Created:
4 years, 10 months ago by peah-webrtc Modified:
4 years, 10 months ago Reviewers:
the sun CC:
webrtc-reviews_webrtc.org, peah-webrtc, Andrew MacDonald, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, kwiberg-webrtc, minyue-webrtc, aluebs-webrtc, bjornv1 Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionMoved the GainControlForNewAGC class to be a separate file.
Apart from being motivated in order to make the source files shorter
this is needed when separating the APM submodules structs into
separate files.
BUG=
Committed: https://crrev.com/be61562bfb7b72a38462c4303c9a205379adeddb
Cr-Commit-Position: refs/heads/master@{#11611}
Patch Set 1 #Patch Set 2 : Added macro call to limit copy and assign access #
Total comments: 21
Patch Set 3 : Changes in response to reviewer comments #Patch Set 4 : Added thread checker and changed to use the GainControl interface solely #Patch Set 5 : Changed the locking test to match how the gain control is used in practice and to comply with the n… #
Total comments: 7
Patch Set 6 : Merge from latest master #Patch Set 7 : Removed threadcheckers that failed on the tests and that are on functions only accessed within APM #Patch Set 8 : Moved threadcheckers to the VolumeCallbacks functions #Patch Set 9 : Testing other threadchecker variant #Patch Set 10 : Added locks #Patch Set 11 : Removed threadchecker and introduced lock #
Messages
Total messages: 51 (22 generated)
Description was changed from ========== Moved the GainControlForNewAGC class to be a separate file. Apart from being motivated in order to make the source files shorter this is needed when separating the APM submodules structs into separate files. BUG= ========== to ========== Moved the GainControlForNewAGC class to be a separate file. Apart from being motivated in order to make the source files shorter this is needed when separating the APM submodules structs into separate files. BUG= ==========
peah@webrtc.org changed reviewers: + solenberg@webrtc.org
Hold on, one more change will come.
Now it is fine again to review it!
Thanks! A few small comments; biggest one is about the name. https://codereview.webrtc.org/1678813002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/gain_control_for_new_agc.cc (right): https://codereview.webrtc.org/1678813002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_for_new_agc.cc:17: GainControlForNewAgc::GainControlForNewAgc(GainControlImpl* gain_control) Do we have any threading issues here at all? https://codereview.webrtc.org/1678813002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/gain_control_for_new_agc.h (right): https://codereview.webrtc.org/1678813002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_for_new_agc.h:16: #include "webrtc/modules/audio_processing/gain_control_impl.h" include not needed? https://codereview.webrtc.org/1678813002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_for_new_agc.h:21: // This class has two main functionalities: functinoalities -> purposes https://codereview.webrtc.org/1678813002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_for_new_agc.h:32: class GainControlForNewAgc : public GainControl, public VolumeCallbacks { Names with "new" and "old" in them will soon be obsolete and confusing. While we're messing about, can we come up with something better? "Agc2"? Something else that is more descriptive of this "new agc"? https://codereview.webrtc.org/1678813002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_for_new_agc.h:54: void SetMicVolume(int volume) override; // VolumeCallbacks implementation. ? https://codereview.webrtc.org/1678813002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_for_new_agc.h:58: GainControl* real_gain_control_; GainControlImpl? and you can forward declare it.
https://codereview.webrtc.org/1678813002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/gain_control_for_new_agc.cc (right): https://codereview.webrtc.org/1678813002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_for_new_agc.cc:17: GainControlForNewAgc::GainControlForNewAgc(GainControlImpl* gain_control) On 2016/02/08 09:21:39, the sun wrote: > Do we have any threading issues here at all? The way it is currently used, threading and lock issues are handled by the code using the class, which I think is what we want long-term. Therefore I'm a bit reluctant to move any lock/thread handling code into this class implementation. Note that the code contained in this class is exactly the same that before this CL was present in audio_processing_impl.cc. Does this make sense? https://codereview.webrtc.org/1678813002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/gain_control_for_new_agc.h (right): https://codereview.webrtc.org/1678813002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_for_new_agc.h:16: #include "webrtc/modules/audio_processing/gain_control_impl.h" On 2016/02/08 09:21:39, the sun wrote: > include not needed? It is needed for GainControlImpl, right? Or do you prefer that a forward should be used instead? https://codereview.webrtc.org/1678813002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_for_new_agc.h:21: // This class has two main functionalities: On 2016/02/08 09:21:39, the sun wrote: > functinoalities -> purposes Done. https://codereview.webrtc.org/1678813002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_for_new_agc.h:32: class GainControlForNewAgc : public GainControl, public VolumeCallbacks { On 2016/02/08 09:21:39, the sun wrote: > Names with "new" and "old" in them will soon be obsolete and confusing. While > we're messing about, can we come up with something better? "Agc2"? Something > else that is more descriptive of this "new agc"? Good point. I exchanged it with "Experimental" AGC as that matches what is written elsewhere in the code. Although I don't think this is either a good name, until I know more about the differences between the AGCs I think it could be good tradeoff. Changing to a more proper name would in my mind be a separate CL. Does that make sense? https://codereview.webrtc.org/1678813002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_for_new_agc.h:54: void SetMicVolume(int volume) override; On 2016/02/08 09:21:39, the sun wrote: > // VolumeCallbacks implementation. > ? Done. https://codereview.webrtc.org/1678813002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_for_new_agc.h:58: GainControl* real_gain_control_; On 2016/02/08 09:21:39, the sun wrote: > GainControlImpl? > and you can forward declare it. This is a bit tricky. As it is currently implemented some GainControl methods referenced in the class (e.g., Enable()) are private in GainControlImpl but public in GainControl. That means that if the code is changed to use GainControlImpl instead of GainControl, it won't be able to access those methods. I don't know the reason for having a more restricted access in GainControlImpl compared to GainControl though. I could change that in this CL if you think it is motivated. What do you think?
https://codereview.webrtc.org/1678813002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/gain_control_for_new_agc.cc (right): https://codereview.webrtc.org/1678813002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_for_new_agc.cc:17: GainControlForNewAgc::GainControlForNewAgc(GainControlImpl* gain_control) On 2016/02/08 13:01:12, peah-webrtc wrote: > On 2016/02/08 09:21:39, the sun wrote: > > Do we have any threading issues here at all? > > The way it is currently used, threading and lock issues are handled by the code > using the class, which I think is what we want long-term. Therefore I'm a bit > reluctant to move any lock/thread handling code into this class implementation. > > Note that the code contained in this class is exactly the same that before this > CL was present in audio_processing_impl.cc. > > Does this make sense? Maybe it's a minor thing, but the volume_ field is manipulated by some of the setters/getters here, and a pointer to one of these objects is returned by gain_control() on the APM. So if we really don't have any race issues, it should be possible to add a thread checker. https://codereview.webrtc.org/1678813002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/gain_control_for_new_agc.h (right): https://codereview.webrtc.org/1678813002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_for_new_agc.h:16: #include "webrtc/modules/audio_processing/gain_control_impl.h" On 2016/02/08 13:01:12, peah-webrtc wrote: > On 2016/02/08 09:21:39, the sun wrote: > > include not needed? > > It is needed for GainControlImpl, right? Or do you prefer that a forward should > be used instead? Yes, forward declare GainControl. To me it looks like you don't need to know about GainControlImpl here (this class only uses the GainControl interface). https://codereview.webrtc.org/1678813002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_for_new_agc.h:32: class GainControlForNewAgc : public GainControl, public VolumeCallbacks { On 2016/02/08 13:01:12, peah-webrtc wrote: > On 2016/02/08 09:21:39, the sun wrote: > > Names with "new" and "old" in them will soon be obsolete and confusing. While > > we're messing about, can we come up with something better? "Agc2"? Something > > else that is more descriptive of this "new agc"? > > Good point. I exchanged it with "Experimental" AGC as that matches what is > written elsewhere in the code. Although I don't think this is either a good > name, until I know more about the differences between the AGCs I think it could > be good tradeoff. Changing to a more proper name would in my mind be a separate > CL. > > Does that make sense? Yes, thanks! https://codereview.webrtc.org/1678813002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_for_new_agc.h:58: GainControl* real_gain_control_; On 2016/02/08 13:01:12, peah-webrtc wrote: > On 2016/02/08 09:21:39, the sun wrote: > > GainControlImpl? > > and you can forward declare it. > > This is a bit tricky. As it is currently implemented some GainControl methods > referenced in the class (e.g., Enable()) are private in GainControlImpl but > public in GainControl. That means that if the code is changed to use > GainControlImpl instead of GainControl, it won't be able to access those > methods. > > I don't know the reason for having a more restricted access in GainControlImpl > compared to GainControl though. I could change that in this CL if you think it > is motivated. > > What do you think? Typically the reason for making a virtual method private in a derived class is to prevent further overriding (i.e. make the class "final" in the Java sense). If you give this class a GainControl*, the private override in GainControlImpl will still be called if you've given GainControlForExperimentalAgc a pointer to a GainControlImpl. It is the access rights declared in GainControl that matters if you use the object as one of those, and I think you can.
https://codereview.webrtc.org/1678813002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/gain_control_for_new_agc.cc (right): https://codereview.webrtc.org/1678813002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_for_new_agc.cc:17: GainControlForNewAgc::GainControlForNewAgc(GainControlImpl* gain_control) On 2016/02/08 14:18:11, the sun wrote: > On 2016/02/08 13:01:12, peah-webrtc wrote: > > On 2016/02/08 09:21:39, the sun wrote: > > > Do we have any threading issues here at all? > > > > The way it is currently used, threading and lock issues are handled by the > code > > using the class, which I think is what we want long-term. Therefore I'm a bit > > reluctant to move any lock/thread handling code into this class > implementation. > > > > Note that the code contained in this class is exactly the same that before > this > > CL was present in audio_processing_impl.cc. > > > > Does this make sense? > > Maybe it's a minor thing, but the volume_ field is manipulated by some of the > setters/getters here, and a pointer to one of these objects is returned by > gain_control() on the APM. So if we really don't have any race issues, it should > be possible to add a thread checker. I think you are right. I added a thread checker. That did, however, require a slight change to a unittest as it did not match the way the gain control is used in practice. Is it ok to bundle the unittest change in this CL? This change is very minor. https://codereview.webrtc.org/1678813002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/gain_control_for_new_agc.h (right): https://codereview.webrtc.org/1678813002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_for_new_agc.h:16: #include "webrtc/modules/audio_processing/gain_control_impl.h" On 2016/02/08 14:18:11, the sun wrote: > On 2016/02/08 13:01:12, peah-webrtc wrote: > > On 2016/02/08 09:21:39, the sun wrote: > > > include not needed? > > > > It is needed for GainControlImpl, right? Or do you prefer that a forward > should > > be used instead? > > Yes, forward declare GainControl. To me it looks like you don't need to know > about GainControlImpl here (this class only uses the GainControl interface). Done. https://codereview.webrtc.org/1678813002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_for_new_agc.h:32: class GainControlForNewAgc : public GainControl, public VolumeCallbacks { On 2016/02/08 14:18:11, the sun wrote: > On 2016/02/08 13:01:12, peah-webrtc wrote: > > On 2016/02/08 09:21:39, the sun wrote: > > > Names with "new" and "old" in them will soon be obsolete and confusing. > While > > > we're messing about, can we come up with something better? "Agc2"? Something > > > else that is more descriptive of this "new agc"? > > > > Good point. I exchanged it with "Experimental" AGC as that matches what is > > written elsewhere in the code. Although I don't think this is either a good > > name, until I know more about the differences between the AGCs I think it > could > > be good tradeoff. Changing to a more proper name would in my mind be a > separate > > CL. > > > > Does that make sense? > > Yes, thanks! Acknowledged. https://codereview.webrtc.org/1678813002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_for_new_agc.h:58: GainControl* real_gain_control_; On 2016/02/08 14:18:11, the sun wrote: > On 2016/02/08 13:01:12, peah-webrtc wrote: > > On 2016/02/08 09:21:39, the sun wrote: > > > GainControlImpl? > > > and you can forward declare it. > > > > This is a bit tricky. As it is currently implemented some GainControl methods > > referenced in the class (e.g., Enable()) are private in GainControlImpl but > > public in GainControl. That means that if the code is changed to use > > GainControlImpl instead of GainControl, it won't be able to access those > > methods. > > > > I don't know the reason for having a more restricted access in GainControlImpl > > compared to GainControl though. I could change that in this CL if you think it > > is motivated. > > > > What do you think? > > Typically the reason for making a virtual method private in a derived class is > to prevent further overriding (i.e. make the class "final" in the Java sense). > > If you give this class a GainControl*, the private override in GainControlImpl > will still be called if you've given GainControlForExperimentalAgc a pointer to > a GainControlImpl. It is the access rights declared in GainControl that matters > if you use the object as one of those, and I think you can. Maybe I got this wrong, but the problem I see is that if I change this to an GainControlImpl*, I cannot use the private methods in GainControlImpl (e.g., Enable) from within the GainControlForNewAgc class. But if I keep it as a GainControl* the methods I need to use are public (e.g., Enable) which means I can use them. One way to circumvent that would of course be to store the pointer as GainControlImpl* and cast to GainControl* each time I want to access these methods. But that seems not like a good solution. I'd rather instead change the constructor receive a GainControl* instead of a GainControlImpl*. I'll do that change. Please let me know what you think about it!
lgtm https://codereview.webrtc.org/1678813002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/gain_control_for_new_agc.h (right): https://codereview.webrtc.org/1678813002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_for_new_agc.h:58: GainControl* real_gain_control_; On 2016/02/09 09:11:15, peah-webrtc wrote: > On 2016/02/08 14:18:11, the sun wrote: > > On 2016/02/08 13:01:12, peah-webrtc wrote: > > > On 2016/02/08 09:21:39, the sun wrote: > > > > GainControlImpl? > > > > and you can forward declare it. > > > > > > This is a bit tricky. As it is currently implemented some GainControl > methods > > > referenced in the class (e.g., Enable()) are private in GainControlImpl but > > > public in GainControl. That means that if the code is changed to use > > > GainControlImpl instead of GainControl, it won't be able to access those > > > methods. > > > > > > I don't know the reason for having a more restricted access in > GainControlImpl > > > compared to GainControl though. I could change that in this CL if you think > it > > > is motivated. > > > > > > What do you think? > > > > Typically the reason for making a virtual method private in a derived class is > > to prevent further overriding (i.e. make the class "final" in the Java sense). > > > > If you give this class a GainControl*, the private override in GainControlImpl > > will still be called if you've given GainControlForExperimentalAgc a pointer > to > > a GainControlImpl. It is the access rights declared in GainControl that > matters > > if you use the object as one of those, and I think you can. > > Maybe I got this wrong, but the problem I see is that if I change this to an > GainControlImpl*, I cannot use the private methods in GainControlImpl (e.g., > Enable) from within the GainControlForNewAgc class. But if I keep it as a > GainControl* the methods I need to use are public (e.g., Enable) which means I > can use them. > > One way to circumvent that would of course be to store the pointer as > GainControlImpl* and cast to GainControl* each time I want to access these > methods. But that seems not like a good solution. > > I'd rather instead change the constructor receive a GainControl* instead of a > GainControlImpl*. I'll do that change. Please let me know what you think about > it! No, sorry, you got it right; I was just bad at expressing myself. https://codereview.webrtc.org/1678813002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc (right): https://codereview.webrtc.org/1678813002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:740: apm_->gain_control()->stream_analog_level(); Just for testing? It's a bit confusing that you say "Retrieve analog level" but never actually do anything with it. EXPECT_EQ(80, apm->...); ? I first thought you were relying on some side effect to be triggered. https://codereview.webrtc.org/1678813002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/gain_control_for_experimental_agc.cc (right): https://codereview.webrtc.org/1678813002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_for_experimental_agc.cc:21: thread_checker.DetachFromThread(); Really needed? https://codereview.webrtc.org/1678813002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_for_experimental_agc.cc:39: RTC_CHECK(thread_checker.CalledOnValidThread()); Thank you! https://codereview.webrtc.org/1678813002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_for_experimental_agc.cc:44: return AudioProcessing::kNoError; (Not possible to add TCs to the other methods?)
https://codereview.webrtc.org/1678813002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc (right): https://codereview.webrtc.org/1678813002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:740: apm_->gain_control()->stream_analog_level(); On 2016/02/09 11:49:55, the sun wrote: > Just for testing? It's a bit confusing that you say "Retrieve analog level" but > never actually do anything with it. EXPECT_EQ(80, apm->...); ? > I first thought you were relying on some side effect to be triggered. The problem with that is that the purpose is not at all to test the functionality but rather to hammer the api in order to find deadlocks and for the thread sanitizers to be able to detect threading problems.. As I have no idea of what the return value should be, I'd rather just not act on it. To test the actual functionality in the AGC I think it is better to test that in another test that tests the AGC functionality. Does that make sense? https://codereview.webrtc.org/1678813002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/gain_control_for_experimental_agc.cc (right): https://codereview.webrtc.org/1678813002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_for_experimental_agc.cc:21: thread_checker.DetachFromThread(); On 2016/02/09 11:49:55, the sun wrote: > Really needed? With the current locking test, it will require quite some work to remove it, and I have no idea whether it will work on the trybots. So I'd rather keep it for now. It does not hurt the module if it is created from another thread. https://codereview.webrtc.org/1678813002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_for_experimental_agc.cc:44: return AudioProcessing::kNoError; On 2016/02/09 11:49:55, the sun wrote: > (Not possible to add TCs to the other methods?) I tried and it is a bit tricky as it requires the locking test to be rewritten. I'd rather keep it as it is for now, as the added thread checkers now protect the volume_ variable. The rest of the methods access the methods of the gain_control object which resources are protected by its own lock.
The CQ bit was checked by peah@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1678813002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1678813002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_asan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/12309)
The CQ bit was checked by peah@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/1678813002/#ps100001 (title: "Merge from latest master")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1678813002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1678813002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_asan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/12340)
The CQ bit was checked by peah@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1678813002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1678813002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_x64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/12444)
The CQ bit was checked by peah@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1678813002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1678813002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Due to the trybots tests failing due to the threadcheckers, I needed to remove two of those accessing the volume variable. That is definitely concerning but I think it makes sense to don't go further with that since. 1) It will require much further work in other areas of the code that will make this CL grow. 2) The problem is something that has been there for a long while, even before the new locking scheme. 3) The problem is most likely not something that ends up in nonthreadsafe memory access as it has never been detected by the memory sanitizers WDYT? Is it ok to leave it as it is now?
The CQ bit was checked by peah@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1678813002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1678813002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/12685)
On 2016/02/12 08:12:42, peah-webrtc wrote: > Due to the trybots tests failing due to the threadcheckers, I needed to remove > two of those accessing the volume variable. That is definitely concerning but I > think it makes sense to don't go further with that since. > > 1) It will require much further work in other areas of the code that will make > this CL grow. > 2) The problem is something that has been there for a long while, even before > the new locking scheme. > 3) The problem is most likely not something that ends up in nonthreadsafe memory > access as it has never been detected by the memory sanitizers > > WDYT? Is it ok to leave it as it is now? A race with that volume int won't cause any crash (worst case complaints by the tsan bots). I don't know how it is used though, so cannot comment on what other decision making/stats may be affected, and how much, by not obtaining the correct value.
The CQ bit was checked by peah@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1678813002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1678813002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios32_sim_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/5111) linux_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_gn_rel/builds/8583)
The CQ bit was checked by peah@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1678813002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1678813002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #12 (id:220001) has been deleted
The CQ bit was checked by peah@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/1678813002/#ps200001 (title: "Removed threadchecker and introduced lock")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1678813002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1678813002/200001
Message was sent while issue was closed.
Description was changed from ========== Moved the GainControlForNewAGC class to be a separate file. Apart from being motivated in order to make the source files shorter this is needed when separating the APM submodules structs into separate files. BUG= ========== to ========== Moved the GainControlForNewAGC class to be a separate file. Apart from being motivated in order to make the source files shorter this is needed when separating the APM submodules structs into separate files. BUG= ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Moved the GainControlForNewAGC class to be a separate file. Apart from being motivated in order to make the source files shorter this is needed when separating the APM submodules structs into separate files. BUG= ========== to ========== Moved the GainControlForNewAGC class to be a separate file. Apart from being motivated in order to make the source files shorter this is needed when separating the APM submodules structs into separate files. BUG= Committed: https://crrev.com/be61562bfb7b72a38462c4303c9a205379adeddb Cr-Commit-Position: refs/heads/master@{#11611} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/be61562bfb7b72a38462c4303c9a205379adeddb Cr-Commit-Position: refs/heads/master@{#11611} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
