Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1436553004:
Created a test that reports the statistics for the duration of APM stream processing API calls. (Closed)
|
Created:
5 years, 1 month ago by peah-webrtc Modified:
5 years ago CC:
webrtc-reviews_webrtc.org, Andrew MacDonald, tterriberry_mozilla.com, hlundin-webrtc, kwiberg-webrtc, aluebs-webrtc, bjornv1 Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionA unittest that reports the statistics for the duration of an APM stream processing API call.
BUG=webrtc:5099
Committed: https://crrev.com/880896ab0976bbf86a6753d0c900c70e51f421cb
Cr-Commit-Position: refs/heads/master@{#10786}
Committed: https://crrev.com/9fca7e18c3e74b9e24bd23d5f1ea29d989b9698f
Cr-Commit-Position: refs/heads/master@{#11098}
Patch Set 1 #Patch Set 2 : Various changes making the test test what is desired. #
Total comments: 58
Patch Set 3 : Changes in response to reviewer comments. As those required quite extensive refactoring, some more … #
Total comments: 40
Patch Set 4 : Changes according to latest reviewer comments #
Total comments: 4
Patch Set 5 : Renamed test file and moved to new build target #
Total comments: 2
Patch Set 6 : Moved to the correct build target #Patch Set 7 : Fixed the condition for whether correct statistics can be produced #Patch Set 8 : Cleaned up statistics computation according to reviewer comments #Patch Set 9 : Added missing (redundant) return statement #Patch Set 10 : Removed redundant const specifier that caused copy of the struct to fail #Patch Set 11 : Added setting of the stream level in order to fix bot error #Patch Set 12 : Reverted the two latest CL and replaced one of them by a RTC_NOTREACHED call #Patch Set 13 : Changed AGC mode to the AdaptiveDigital that should work on all platforms without any additional AP… #Patch Set 14 : Changed name for the ThreadWrapper to the new name PlatformThread and updated its include #Patch Set 15 : Deactivated test not relevant for Android when running on Android #Patch Set 16 : Changed the code to ensure that the threads are successfully stopped after the test is finished #Patch Set 17 : Corrected the number of frames to process in the test #Patch Set 18 : Updates to handle changes in external code used in this test #
Messages
Total messages: 66 (28 generated)
Description was changed from ========== Created a test that reports the statistics for the duration of an APM stream processing API call. BUG=webrtc:5099 ========== to ========== Created a test that reports the statistics for the duration of an APM stream processing API call. BUG=webrtc:5099 ==========
peah@webrtc.org changed reviewers: + henrik.lundin@webrtc.org, kwiberg@webrtc.org, solenberg@webrtc.org
Description was changed from ========== Created a test that reports the statistics for the duration of an APM stream processing API call. BUG=webrtc:5099 ========== to ========== A unittest that reports the statistics for the duration of an APM stream processing API call. BUG=webrtc:5099 ==========
Created a unittest that reports the statistics for the duration of an APM stream processing API call.
Please hold on with reviewing. Some minor changes that significantly will affect the results will be implemented.
Now it is fully fine to review this again whenever you have the time.
Nice. But I do have a few comments... :) https://codereview.webrtc.org/1436553004/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc (right): https://codereview.webrtc.org/1436553004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:2: * Copyright (c) 2014 The WebRTC project authors. All Rights Reserved. 2015 https://codereview.webrtc.org/1436553004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:36: // Populates a float audio frame with random data. It seems to populate more than one frame. Make the comment reflect that. https://codereview.webrtc.org/1436553004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:37: void PopulateAudioFrame(float** frame, frame is an output parameter, it should go last in the parameter list. https://codereview.webrtc.org/1436553004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:44: // Store random 16 bit quantized float number between +-amplitude. Where does the 16-bit quantization happen? Is it implicit in Rand? https://codereview.webrtc.org/1436553004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:55: explicit AudioFrameData(int max_frame_size) { size_t https://codereview.webrtc.org/1436553004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:89: bool use_realtime_simulation; All members but this one has default values. https://codereview.webrtc.org/1436553004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:122: int RenderMinusCaptureCounters() { Do you need both "minus" methods? And if you do, you could implement this one as int RenderMinusCaptureCounters() { return -CaptureMinusRenderCounters(); } https://codereview.webrtc.org/1436553004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:134: int render_count GUARDED_BY(crit_) = 0; Trailing underscore on both counter variables. https://codereview.webrtc.org/1436553004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:139: class CaptureSideCalledChecker { Generalize the name to make it shorter. Nothing within the class implementation limits its use to the capture side. It is essentially a locked bool; maybe call it LockedBool? https://codereview.webrtc.org/1436553004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:141: bool CaptureSideCalled() { This is a simple "getter" function, albeit with a lock. I think you should name it as such: bool capture_side_called() const { ... } Getters are normally const methods, but since you have a lock you cannot do that unless you declare the lock mutable. Up to you. https://codereview.webrtc.org/1436553004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:146: void FlagCaptureSideCalled() { This is a "setter" with a lock. Name it such: void set_capture_side_called(bool value) { ... } https://codereview.webrtc.org/1436553004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:199: class CaptureProcessor : public TimedThreadApiProcessor { Mark this class final. https://codereview.webrtc.org/1436553004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:220: class RenderProcessor : public TimedThreadApiProcessor { Mark this class final. https://codereview.webrtc.org/1436553004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:241: class CallSimulator { I think this class should be made a gtest fixture class. That is, let it inherit from ::testing::Test, and change the TEST()s below to TEST_F(CallSimulator, ...). That way, the gtest framework will invoke SetUp() and TearDown() for you. You won't be able to pass arguments to the fixture's constructor. Instead, you can create a new method in this class that allows the TEST_F to set the config. Or create a value-parameterized test. https://codereview.webrtc.org/1436553004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:291: mutable test::Random rand_gen_; Why is this mutable? https://codereview.webrtc.org/1436553004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:313: apm_(AudioProcessingImpl::Create()), Create() may return null in case of failure. You can place an ASSERT in SetUp(). https://codereview.webrtc.org/1436553004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:337: StartThreads(); You should make sure that none of the ASSERT_TRUEs in StartThreads() failed. Either ASSERT_NO_FATAL_FAILURE(StartThreads()); - or - StartThreads(); if (::testing::Test::HasFatalFailure()) { FAIL(); } Note that you can drop the ::testing::Test:: prefix if you make CallSimulator inherit from ::testing::Test. https://codereview.webrtc.org/1436553004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:353: Config config; Move the declaration down to within the else-clause. https://codereview.webrtc.org/1436553004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:375: TimedThreadApiProcessor::TimedThreadApiProcessor( I think you can define all the member methods within the class declaration above. https://codereview.webrtc.org/1436553004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:391: frame_data_(max_frame_size), No need to pass max_frame_size as a parameter; simply use kMaxFrameSize. YAGNI... https://codereview.webrtc.org/1436553004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:408: const int duration = static_cast<int>(end_time - start_time); use rtc::checked_cast<int> from base/safe_conversions.h. https://codereview.webrtc.org/1436553004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:408: const int duration = static_cast<int>(end_time - start_time); duration_us, to convey the unit. https://codereview.webrtc.org/1436553004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:428: return static_cast<int64_t>(sqrt(variance / api_call_durations_.size())); rtc::checked_cast https://codereview.webrtc.org/1436553004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:466: const int64_t start_time = clock_->TimeInMicroseconds(); Please, comment on why you cannot just sleep. https://codereview.webrtc.org/1436553004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:481: CaptureProcessor::CaptureProcessor( There is a non-negligible overhead associated with the class structure of the Processor classes. I don't know if the code will be easier/shorter, but you might want to consider having only the base class, and then instantiate each object with function pointers to file-scope functions for the two specializations you need. I mean, the Process() method could just as well be two different stand-alone functions taking the few (four?) member variables they need as pointers/references. Similar for ReadyToProcess(). https://codereview.webrtc.org/1436553004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:515: api_call_durations_[new_capture_counter - 1] = end_time - start_time; Why not just api_call_durations_.push_back(...)? You can still allocate enough memory to start with, if you are worried about performance hits while resizing (I wouldn't be). Also, if you want to stop storing more durations after a certain size, simply checking size() will do. You don't need frame_counters_->IncreaseCaptureCounter() to return a value. https://codereview.webrtc.org/1436553004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:624: ASSERT_EQ(EventTypeWrapper::kEventSignaled, simulator.Run(timeout)); Maybe EXPECT_EQ instead. It might still be interesting to see the printouts of a timed-out run. https://codereview.webrtc.org/1436553004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:626: print_simulation_statistics(simulator.GetRender(), "render", print_durations); To me it looks like the print_simulation_statistics lambda is not using any of the local variables in RunSimulations. But it is using TimedThreadApiProcessor pointers. Why not make the print functionality a part of TimedThreadApiProcessor instead? https://codereview.webrtc.org/1436553004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:633: const bool kUseTimedSimulation = false; I don't think it makes much sense to declare this constant. In the tests below, the constant is either used (meaning false), or true is explicitly given. Also, the name is confusing. When I read the invocation lines below, I read "RunSimulation, blah, blah, and use timed simulation", which is the opposite of what is going on. I prefer code like RunSimulation(SimulationConfig(..., false /*use_realtime_simulation*/)); for the constants that change between different runs. The kPrintAllDurations is fine, even though I think it could be made a file constant that won't have to clutter the test parameter list.
Changes in response to previous reviewer comments. As those anyway required quite extensive refactoring, some more tests were added in the process. https://codereview.webrtc.org/1436553004/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc (right): https://codereview.webrtc.org/1436553004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:2: * Copyright (c) 2014 The WebRTC project authors. All Rights Reserved. On 2015/11/13 12:18:32, hlundin-webrtc wrote: > 2015 Done. https://codereview.webrtc.org/1436553004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:36: // Populates a float audio frame with random data. On 2015/11/13 12:18:31, hlundin-webrtc wrote: > It seems to populate more than one frame. Make the comment reflect that. It supports multiple channel frames, and I think it would make sense to extend this test in the (very near) future to use it to benchmark the complexity of the APM submodules such as the beamformer, which requires stereo input. I extended the comment a bit to reflect that it generates a multi-channel frame. Please let me know whether it is fine! Done. https://codereview.webrtc.org/1436553004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:37: void PopulateAudioFrame(float** frame, On 2015/11/13 12:18:31, hlundin-webrtc wrote: > frame is an output parameter, it should go last in the parameter list. Done. https://codereview.webrtc.org/1436553004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:44: // Store random 16 bit quantized float number between +-amplitude. On 2015/11/13 12:18:32, hlundin-webrtc wrote: > Where does the 16-bit quantization happen? Is it implicit in Rand? Sorry, that comment is completely wrong! Removed it! Done. https://codereview.webrtc.org/1436553004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:55: explicit AudioFrameData(int max_frame_size) { On 2015/11/13 12:18:32, hlundin-webrtc wrote: > size_t Done. https://codereview.webrtc.org/1436553004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:89: bool use_realtime_simulation; On 2015/11/13 12:18:32, hlundin-webrtc wrote: > All members but this one has default values. Done. https://codereview.webrtc.org/1436553004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:122: int RenderMinusCaptureCounters() { On 2015/11/13 12:18:32, hlundin-webrtc wrote: > Do you need both "minus" methods? And if you do, you could implement this one as > int RenderMinusCaptureCounters() { > return -CaptureMinusRenderCounters(); > } It is mainly for convenience and to (tiny) increase the code readability in the actual test code. I will add the change in the function. Please let me know if you also want me to instead move the -CaptureMinusRenderCounters() to the test code. Done. https://codereview.webrtc.org/1436553004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:134: int render_count GUARDED_BY(crit_) = 0; On 2015/11/13 12:18:32, hlundin-webrtc wrote: > Trailing underscore on both counter variables. Done. https://codereview.webrtc.org/1436553004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:139: class CaptureSideCalledChecker { On 2015/11/13 12:18:32, hlundin-webrtc wrote: > Generalize the name to make it shorter. Nothing within the class implementation > limits its use to the capture side. It is essentially a locked bool; maybe call > it LockedBool? Good point! I called it LockedFlag since it is does not provide all the functionality that a bool does. Done. https://codereview.webrtc.org/1436553004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:141: bool CaptureSideCalled() { On 2015/11/13 12:18:32, hlundin-webrtc wrote: > This is a simple "getter" function, albeit with a lock. I think you should name > it as such: > > bool capture_side_called() const { > ... > } > > Getters are normally const methods, but since you have a lock you cannot do that > unless you declare the lock mutable. Up to you. Done. https://codereview.webrtc.org/1436553004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:146: void FlagCaptureSideCalled() { On 2015/11/13 12:18:32, hlundin-webrtc wrote: > This is a "setter" with a lock. Name it such: > void set_capture_side_called(bool value) { > ... > } Done. https://codereview.webrtc.org/1436553004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:199: class CaptureProcessor : public TimedThreadApiProcessor { On 2015/11/13 12:18:31, hlundin-webrtc wrote: > Mark this class final. Done. https://codereview.webrtc.org/1436553004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:220: class RenderProcessor : public TimedThreadApiProcessor { On 2015/11/13 12:18:31, hlundin-webrtc wrote: > Mark this class final. Done. https://codereview.webrtc.org/1436553004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:241: class CallSimulator { On 2015/11/13 12:18:32, hlundin-webrtc wrote: > I think this class should be made a gtest fixture class. That is, let it inherit > from ::testing::Test, and change the TEST()s below to TEST_F(CallSimulator, > ...). That way, the gtest framework will invoke SetUp() and TearDown() for you. > > You won't be able to pass arguments to the fixture's constructor. Instead, you > can create a new method in this class that allows the TEST_F to set the config. > Or create a value-parameterized test. Done. https://codereview.webrtc.org/1436553004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:291: mutable test::Random rand_gen_; On 2015/11/13 12:18:31, hlundin-webrtc wrote: > Why is this mutable? Done. https://codereview.webrtc.org/1436553004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:313: apm_(AudioProcessingImpl::Create()), On 2015/11/13 12:18:31, hlundin-webrtc wrote: > Create() may return null in case of failure. You can place an ASSERT in SetUp(). Done. https://codereview.webrtc.org/1436553004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:337: StartThreads(); On 2015/11/13 12:18:31, hlundin-webrtc wrote: > You should make sure that none of the ASSERT_TRUEs in StartThreads() failed. > Either > ASSERT_NO_FATAL_FAILURE(StartThreads()); > - or - > StartThreads(); > if (::testing::Test::HasFatalFailure()) { > FAIL(); > } > > Note that you can drop the ::testing::Test:: prefix if you make CallSimulator > inherit from ::testing::Test. Done. https://codereview.webrtc.org/1436553004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:353: Config config; On 2015/11/13 12:18:31, hlundin-webrtc wrote: > Move the declaration down to within the else-clause. Done. https://codereview.webrtc.org/1436553004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:375: TimedThreadApiProcessor::TimedThreadApiProcessor( On 2015/11/13 12:18:32, hlundin-webrtc wrote: > I think you can define all the member methods within the class declaration > above. Did this on all methods apart from the Process() method that requires the CallSimulator implementation to be known in order to properly build. Done. https://codereview.webrtc.org/1436553004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:391: frame_data_(max_frame_size), On 2015/11/13 12:18:31, hlundin-webrtc wrote: > No need to pass max_frame_size as a parameter; simply use kMaxFrameSize. > YAGNI... Done. https://codereview.webrtc.org/1436553004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:408: const int duration = static_cast<int>(end_time - start_time); On 2015/11/13 12:18:31, hlundin-webrtc wrote: > duration_us, to convey the unit. Done. https://codereview.webrtc.org/1436553004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:408: const int duration = static_cast<int>(end_time - start_time); On 2015/11/13 12:18:32, hlundin-webrtc wrote: > use rtc::checked_cast<int> from base/safe_conversions.h. Done. https://codereview.webrtc.org/1436553004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:428: return static_cast<int64_t>(sqrt(variance / api_call_durations_.size())); On 2015/11/13 12:18:32, hlundin-webrtc wrote: > rtc::checked_cast Done. https://codereview.webrtc.org/1436553004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:466: const int64_t start_time = clock_->TimeInMicroseconds(); On 2015/11/13 12:18:32, hlundin-webrtc wrote: > Please, comment on why you cannot just sleep. Very good point. As I never use this in the test any more, and as I think it will not be used, I removed this and replaced it with a trivial spinlock functionality in the one place where it made sense to use it. Done. https://codereview.webrtc.org/1436553004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:481: CaptureProcessor::CaptureProcessor( On 2015/11/13 12:18:32, hlundin-webrtc wrote: > There is a non-negligible overhead associated with the class structure of the > Processor classes. I don't know if the code will be easier/shorter, but you > might want to consider having only the base class, and then instantiate each > object with function pointers to file-scope functions for the two > specializations you need. I mean, the Process() method could just as well be two > different stand-alone functions taking the few (four?) member variables they > need as pointers/references. Similar for ReadyToProcess(). Good point!!! Will implement it like that. Done. https://codereview.webrtc.org/1436553004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:515: api_call_durations_[new_capture_counter - 1] = end_time - start_time; On 2015/11/13 12:18:32, hlundin-webrtc wrote: > Why not just api_call_durations_.push_back(...)? You can still allocate enough > memory to start with, if you are worried about performance hits while resizing > (I wouldn't be). Also, if you want to stop storing more durations after a > certain size, simply checking size() will do. You don't need > frame_counters_->IncreaseCaptureCounter() to return a value. Good and very valid point! Done. https://codereview.webrtc.org/1436553004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:624: ASSERT_EQ(EventTypeWrapper::kEventSignaled, simulator.Run(timeout)); On 2015/11/13 12:18:32, hlundin-webrtc wrote: > Maybe EXPECT_EQ instead. It might still be interesting to see the printouts of a > timed-out run. Done. https://codereview.webrtc.org/1436553004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:626: print_simulation_statistics(simulator.GetRender(), "render", print_durations); On 2015/11/13 12:18:32, hlundin-webrtc wrote: > To me it looks like the print_simulation_statistics lambda is not using any of > the local variables in RunSimulations. But it is using TimedThreadApiProcessor > pointers. Why not make the print functionality a part of TimedThreadApiProcessor > instead? Done. https://codereview.webrtc.org/1436553004/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:633: const bool kUseTimedSimulation = false; On 2015/11/13 12:18:32, hlundin-webrtc wrote: > I don't think it makes much sense to declare this constant. In the tests below, > the constant is either used (meaning false), or true is explicitly given. > > Also, the name is confusing. When I read the invocation lines below, I read > "RunSimulation, blah, blah, and use timed simulation", which is the opposite of > what is going on. > > I prefer code like > RunSimulation(SimulationConfig(..., false /*use_realtime_simulation*/)); > for the constants that change between different runs. The kPrintAllDurations is > fine, even though I think it could be made a file constant that won't have to > clutter the test parameter list. Good points and I fully agree. This has been partly changed in the refactoring. In that I also made kPrintAllDurations a file constant. Done.
https://codereview.webrtc.org/1436553004/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc (right): https://codereview.webrtc.org/1436553004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:42: // Variant of echo canceller settings to use in the test. "echo canceller" -> something like "processing type". https://codereview.webrtc.org/1436553004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:158: default: Try to remove the default. https://codereview.webrtc.org/1436553004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:258: ~TimedThreadApiProcessor() {} Delete the dtor. https://codereview.webrtc.org/1436553004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:276: webrtc::test::PrintResult( We should maybe reformat this print-out. Stay tuned. https://codereview.webrtc.org/1436553004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:280: "ms", false); ms -> us https://codereview.webrtc.org/1436553004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:309: return rtc::checked_cast<int64_t>( Guard against divide-by-0. https://codereview.webrtc.org/1436553004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:319: return average_duration / api_call_durations_.size(); return api_call_durations_.size() > 0 ? average_duration / api_call_durations_.size() : -1; https://codereview.webrtc.org/1436553004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:325: // Set the stream delay . https://codereview.webrtc.org/1436553004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:418: if (processor_type_ == ProcessorType::kRender) { switch(processor_type_) https://codereview.webrtc.org/1436553004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:436: ProcessorType processor_type_; This (and more?) can and should be const. https://codereview.webrtc.org/1436553004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:470: void CheckIfSimulationDone() { MaybeEndTest() https://codereview.webrtc.org/1436553004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:482: // ::testing::TestWithParam<> implementation . https://codereview.webrtc.org/1436553004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:556: case SettingsType::kDefaultApmMobile: Be consistent and use {} on all cases. https://codereview.webrtc.org/1436553004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:558: ASSERT_FALSE(apm_ == nullptr); Try ASSERT_TRUE(apm_), and see if that works, or maybe ASSERT_TRUE(!!apm_). Otherwise, I prefer ASSERT_TRUE(apm_ != nullptr). https://codereview.webrtc.org/1436553004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:568: } break; ... break; } All of them. https://codereview.webrtc.org/1436553004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:611: default: Try to remove the default. https://codereview.webrtc.org/1436553004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:654: SimulationConfig simulation_config_; const https://codereview.webrtc.org/1436553004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:672: if (processor_type_ == ProcessorType::kRender) { switch
cc:kjellander@ to weigh in on my suggestion regarding gyp target for this performance test. https://codereview.webrtc.org/1436553004/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc (right): https://codereview.webrtc.org/1436553004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:276: webrtc::test::PrintResult( On 2015/11/18 12:43:30, hlundin-webrtc wrote: > We should maybe reformat this print-out. Stay tuned. I suggest the following: webrtc::test::PrintResultMeanAndError("apm_timing", <sample_rate>, <settings_type_name>, "<mean>, <std>", "us", false); https://codereview.webrtc.org/1436553004/diff/40001/webrtc/modules/modules.gyp File webrtc/modules/modules.gyp (right): https://codereview.webrtc.org/1436553004/diff/40001/webrtc/modules/modules.gy... webrtc/modules/modules.gyp:344: 'audio_processing/audio_processing_lock_performance_unittest.cc', I suggest you put this in the webrtc_perf_tests target in webrtc_tests.gypi.
https://codereview.webrtc.org/1436553004/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc (right): https://codereview.webrtc.org/1436553004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:42: // Variant of echo canceller settings to use in the test. On 2015/11/18 12:43:31, hlundin-webrtc wrote: > "echo canceller" -> something like "processing type". Done. https://codereview.webrtc.org/1436553004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:158: default: On 2015/11/18 12:43:30, hlundin-webrtc wrote: > Try to remove the default. Done. https://codereview.webrtc.org/1436553004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:258: ~TimedThreadApiProcessor() {} On 2015/11/18 12:43:30, hlundin-webrtc wrote: > Delete the dtor. Done. https://codereview.webrtc.org/1436553004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:276: webrtc::test::PrintResult( On 2015/11/18 12:43:30, hlundin-webrtc wrote: > We should maybe reformat this print-out. Stay tuned. Acknowledged. https://codereview.webrtc.org/1436553004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:276: webrtc::test::PrintResult( On 2015/11/18 13:14:06, hlundin-webrtc wrote: > On 2015/11/18 12:43:30, hlundin-webrtc wrote: > > We should maybe reformat this print-out. Stay tuned. > > I suggest the following: > webrtc::test::PrintResultMeanAndError("apm_timing", > <sample_rate>, > <settings_type_name>, > "<mean>, <std>", > "us", > false); Done. https://codereview.webrtc.org/1436553004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:280: "ms", false); On 2015/11/18 12:43:30, hlundin-webrtc wrote: > ms -> us Done. https://codereview.webrtc.org/1436553004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:309: return rtc::checked_cast<int64_t>( On 2015/11/18 12:43:30, hlundin-webrtc wrote: > Guard against divide-by-0. Done. https://codereview.webrtc.org/1436553004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:319: return average_duration / api_call_durations_.size(); On 2015/11/18 12:43:31, hlundin-webrtc wrote: > return api_call_durations_.size() > 0 ? average_duration / > api_call_durations_.size() : -1; Done. https://codereview.webrtc.org/1436553004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:325: // Set the stream delay On 2015/11/18 12:43:30, hlundin-webrtc wrote: > . Done. https://codereview.webrtc.org/1436553004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:418: if (processor_type_ == ProcessorType::kRender) { On 2015/11/18 12:43:31, hlundin-webrtc wrote: > switch(processor_type_) Done. https://codereview.webrtc.org/1436553004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:436: ProcessorType processor_type_; On 2015/11/18 12:43:31, hlundin-webrtc wrote: > This (and more?) can and should be const. Done. https://codereview.webrtc.org/1436553004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:470: void CheckIfSimulationDone() { On 2015/11/18 12:43:31, hlundin-webrtc wrote: > MaybeEndTest() Done. https://codereview.webrtc.org/1436553004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:482: // ::testing::TestWithParam<> implementation On 2015/11/18 12:43:31, hlundin-webrtc wrote: > . Done. https://codereview.webrtc.org/1436553004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:556: case SettingsType::kDefaultApmMobile: On 2015/11/18 12:43:31, hlundin-webrtc wrote: > Be consistent and use {} on all cases. Done. https://codereview.webrtc.org/1436553004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:558: ASSERT_FALSE(apm_ == nullptr); On 2015/11/18 12:43:31, hlundin-webrtc wrote: > Try ASSERT_TRUE(apm_), and see if that works, or maybe ASSERT_TRUE(!!apm_). > > Otherwise, I prefer ASSERT_TRUE(apm_ != nullptr). Done. https://codereview.webrtc.org/1436553004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:568: } break; On 2015/11/18 12:43:30, hlundin-webrtc wrote: > ... > break; > } > > All of them. Done. https://codereview.webrtc.org/1436553004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:611: default: On 2015/11/18 12:43:31, hlundin-webrtc wrote: > Try to remove the default. Done. https://codereview.webrtc.org/1436553004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:654: SimulationConfig simulation_config_; On 2015/11/18 12:43:31, hlundin-webrtc wrote: > const Done. https://codereview.webrtc.org/1436553004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:672: if (processor_type_ == ProcessorType::kRender) { On 2015/11/18 12:43:31, hlundin-webrtc wrote: > switch Done. https://codereview.webrtc.org/1436553004/diff/40001/webrtc/modules/modules.gyp File webrtc/modules/modules.gyp (right): https://codereview.webrtc.org/1436553004/diff/40001/webrtc/modules/modules.gy... webrtc/modules/modules.gyp:344: 'audio_processing/audio_processing_lock_performance_unittest.cc', On 2015/11/18 13:14:06, hlundin-webrtc wrote: > I suggest you put this in the webrtc_perf_tests target in webrtc_tests.gypi. I added a new target for this. Could you please verify that it makes sense as it is?
I think you introduced a bug in the code. And I'd like you to move the test into another target. But other than that, you have my lgtm. https://codereview.webrtc.org/1436553004/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc (right): https://codereview.webrtc.org/1436553004/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:309: return (api_call_durations_.size() > (kNumInitializationFrames - 1) The condition is not the same as the denominator you are in fact using. Make the code slightly more readable and less error-prone by adding const int denominator = rtc::checked_cast<int>(api_call_durations_.size()) - kNumInitializationFrames; https://codereview.webrtc.org/1436553004/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:322: return (api_call_durations_.size() > (kNumInitializationFrames - 1) Same as above. https://codereview.webrtc.org/1436553004/diff/80001/webrtc/webrtc_tests.gypi File webrtc/webrtc_tests.gypi (right): https://codereview.webrtc.org/1436553004/diff/80001/webrtc/webrtc_tests.gypi#... webrtc/webrtc_tests.gypi:290: 'target_name': 'audio_processing_performance_tests', I think you should add this to webrtc_perf_tests above.
kjellander@webrtc.org changed reviewers: + kjellander@webrtc.org
webrtc/webrtc_tests.gypi: lgtm (adding to the existing perf test is the way to go).
https://codereview.webrtc.org/1436553004/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc (right): https://codereview.webrtc.org/1436553004/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:309: return (api_call_durations_.size() > (kNumInitializationFrames - 1) On 2015/11/18 15:05:31, hlundin-webrtc wrote: > The condition is not the same as the denominator you are in fact using. > > Make the code slightly more readable and less error-prone by adding > const int denominator = rtc::checked_cast<int>(api_call_durations_.size()) - > kNumInitializationFrames; Fully true! Done. https://codereview.webrtc.org/1436553004/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_lock_performance_unittest.cc:322: return (api_call_durations_.size() > (kNumInitializationFrames - 1) On 2015/11/18 15:05:31, hlundin-webrtc wrote: > Same as above. Done. https://codereview.webrtc.org/1436553004/diff/80001/webrtc/webrtc_tests.gypi File webrtc/webrtc_tests.gypi (right): https://codereview.webrtc.org/1436553004/diff/80001/webrtc/webrtc_tests.gypi#... webrtc/webrtc_tests.gypi:290: 'target_name': 'audio_processing_performance_tests', On 2015/11/18 15:05:31, hlundin-webrtc wrote: > I think you should add this to webrtc_perf_tests above. Done.
Nice! lgtm (again).
lgtm
peah@webrtc.org changed reviewers: - kwiberg@webrtc.org
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/1436553004/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1436553004/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_compile_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_compile_dbg/build...)
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, henrik.lundin@webrtc.org, kjellander@webrtc.org Link to the patchset: https://codereview.webrtc.org/1436553004/#ps180001 (title: "Removed redundant const specifier that caused copy of the struct to fail")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1436553004/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1436553004/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/9249)
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, henrik.lundin@webrtc.org, kjellander@webrtc.org Link to the patchset: https://codereview.webrtc.org/1436553004/#ps220001 (title: "Reverted the two latest CL and replaced one of them by a RTC_NOTREACHED call")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1436553004/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1436553004/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/9238)
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, henrik.lundin@webrtc.org, kjellander@webrtc.org Link to the patchset: https://codereview.webrtc.org/1436553004/#ps240001 (title: "Changed AGC mode to the AdaptiveDigital that should work on all platforms without any additional AP…")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1436553004/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1436553004/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_arm64_rel/build...)
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/1436553004/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1436553004/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_arm64_rel/build...)
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, henrik.lundin@webrtc.org, kjellander@webrtc.org Link to the patchset: https://codereview.webrtc.org/1436553004/#ps260001 (title: "Changed name for the ThreadWrapper to the new name PlatformThread and updated its include")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1436553004/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1436553004/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_arm64_rel/build...)
On 2015/11/24 13:43:56, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > android_arm64_rel on tryserver.webrtc (JOB_FAILED, > http://build.chromium.org/p/tryserver.webrtc/builders/android_arm64_rel/build...) I'm not sure if you've been really unlucky with this trybot (one device of three was dead so I've filed a ticket for that). but it seems consistent that the test takes an enormous amount of time to execute on ARM64. Any possibility you can try running it on a local Nexus 9 device? Let me know if you need help with that (I have one you can borrow).
On 2015/11/24 21:16:10, kjellander (webrtc) wrote: > On 2015/11/24 13:43:56, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > android_arm64_rel on tryserver.webrtc (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.webrtc/builders/android_arm64_rel/build...) > > I'm not sure if you've been really unlucky with this trybot (one device of three > was dead so I've filed a ticket for that). but it seems consistent that the test > takes an enormous amount of time to execute on ARM64. Any possibility you can > try running it on a local Nexus 9 device? Let me know if you need help with that > (I have one you can borrow). Thanks! I will! I've reviewed the code and I cannot see anything that should cause it to run that long. The test is, however, a bit unrealistic since it runs stuff on Android that we don't typically run on Android. Therefore I just tried to deactivate that on Android and try one more round on the bots.
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, henrik.lundin@webrtc.org, kjellander@webrtc.org Link to the patchset: https://codereview.webrtc.org/1436553004/#ps280001 (title: "Deactivated test not relevant for Android when running on Android")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1436553004/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1436553004/280001
Message was sent while issue was closed.
Committed patchset #15 (id:280001)
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/880896ab0976bbf86a6753d0c900c70e51f421cb Cr-Commit-Position: refs/heads/master@{#10786}
Message was sent while issue was closed.
A revert of this CL (patchset #15 id:280001) has been created in https://codereview.webrtc.org/1473733004/ by kjellander@webrtc.org. The reason for reverting is: This breaks the Win32 Release [large tests] bot (webrtc_perf_tests times out after 1h23m): https://build.chromium.org/p/client.webrtc/builders/Win32%20Release%20%5Blarg... The Mac64 Release [large tests] bot's runtime also increased with +20 minutes. These bot configs are not a part of the default trybot set, so please run them manually or add this to the CL description: CQ_EXTRA_TRYBOTS=tryserver.webrtc:win_baremetal,mac_baremetal,linux_baremetal .
Message was sent while issue was closed.
Description was changed from ========== A unittest that reports the statistics for the duration of an APM stream processing API call. BUG=webrtc:5099 Committed: https://crrev.com/880896ab0976bbf86a6753d0c900c70e51f421cb Cr-Commit-Position: refs/heads/master@{#10786} ========== to ========== A unittest that reports the statistics for the duration of an APM stream processing API call. BUG=webrtc:5099 Committed: https://crrev.com/880896ab0976bbf86a6753d0c900c70e51f421cb Cr-Commit-Position: refs/heads/master@{#10786} ==========
Hi, I (think) I found the culprit for why the test took too long to run on some platform. I think it was that the threads were not really interrupted and stopped after the test was done, perhaps due to the code having no Sleep statements. The test was hence reverted but with this change I'd like to try to reland it. Would be great if you could please have a look to see that it makes sense!
I checked up the the correctness of returning false from the thread main function when there is no more work to be done in the thread and it can stop running, and it works as intended to return false from this function in that case.
LGTM on the diff between #15 and #17.
On 2015/12/14 09:28:21, hlundin-webrtc wrote: > LGTM on the diff between #15 and #17. No further comments.
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, kjellander@webrtc.org, henrik.lundin@webrtc.org Link to the patchset: https://codereview.webrtc.org/1436553004/#ps340001 (title: "Updates to handle changes in external code used in this test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1436553004/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1436553004/340001
Message was sent while issue was closed.
Description was changed from ========== A unittest that reports the statistics for the duration of an APM stream processing API call. BUG=webrtc:5099 Committed: https://crrev.com/880896ab0976bbf86a6753d0c900c70e51f421cb Cr-Commit-Position: refs/heads/master@{#10786} ========== to ========== A unittest that reports the statistics for the duration of an APM stream processing API call. BUG=webrtc:5099 Committed: https://crrev.com/880896ab0976bbf86a6753d0c900c70e51f421cb Cr-Commit-Position: refs/heads/master@{#10786} ==========
Message was sent while issue was closed.
Committed patchset #18 (id:340001)
Message was sent while issue was closed.
Description was changed from ========== A unittest that reports the statistics for the duration of an APM stream processing API call. BUG=webrtc:5099 Committed: https://crrev.com/880896ab0976bbf86a6753d0c900c70e51f421cb Cr-Commit-Position: refs/heads/master@{#10786} ========== to ========== A unittest that reports the statistics for the duration of an APM stream processing API call. BUG=webrtc:5099 Committed: https://crrev.com/880896ab0976bbf86a6753d0c900c70e51f421cb Cr-Commit-Position: refs/heads/master@{#10786} Committed: https://crrev.com/9fca7e18c3e74b9e24bd23d5f1ea29d989b9698f Cr-Commit-Position: refs/heads/master@{#11098} ==========
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/9fca7e18c3e74b9e24bd23d5f1ea29d989b9698f Cr-Commit-Position: refs/heads/master@{#11098} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
