From 0f8b403eb536ebc721201de5a272ffb66eb5ffe1 Mon Sep 17 00:00:00 2001 From: tommi Date: Wed, 22 Feb 2017 11:22:05 -0800 Subject: [PATCH] Introduce a new constructor to PlatformThread. The new constructor introduces two new changes: * Support specifying thread priority at construction time. - Moving forward, the SetPriority() method will be removed. * New thread function type. - The new type has 'void' as a return type and a polling loop inside PlatformThread, is not used. The old function type is still supported until all places have been moved over. In this CL, the first steps towards deprecating the old mechanism are taken by moving parts of the code that were simple to move, over to the new callback type. BUG=webrtc:7187 Review-Url: https://codereview.webrtc.org/2708723003 Cr-Commit-Position: refs/heads/master@{#16779} --- webrtc/base/event_tracer.cc | 13 ++-- webrtc/base/platform_thread.cc | 53 ++++++++++++--- webrtc/base/platform_thread.h | 17 ++++- webrtc/base/platform_thread_unittest.cc | 50 +++++++++++++- webrtc/base/rate_limiter_unittest.cc | 3 +- .../base/sequenced_task_checker_unittest.cc | 3 +- webrtc/base/task_queue.h | 4 +- webrtc/base/task_queue_libevent.cc | 4 +- webrtc/base/task_queue_win.cc | 4 +- webrtc/call/rampup_tests.cc | 64 ++++++++--------- webrtc/call/rampup_tests.h | 8 +-- .../include/incoming_video_stream.h | 8 +-- webrtc/common_video/incoming_video_stream.cc | 59 +++++++++------- .../rtc_event_log_helper_thread.cc | 3 +- .../rtc_event_log_helper_thread.h | 2 +- .../video_coding/frame_buffer2_unittest.cc | 4 +- .../source/critical_section_unittest.cc | 3 +- webrtc/video/end_to_end_tests.cc | 68 +++++++++---------- webrtc/video/video_quality_test.cc | 49 +++++++------ webrtc/voice_engine/channel_proxy.cc | 8 ++- 20 files changed, 258 insertions(+), 169 deletions(-) diff --git a/webrtc/base/event_tracer.cc b/webrtc/base/event_tracer.cc index 4393bff298..cb7554a891 100644 --- a/webrtc/base/event_tracer.cc +++ b/webrtc/base/event_tracer.cc @@ -80,7 +80,7 @@ namespace rtc { namespace tracing { namespace { -static bool EventTracingThreadFunc(void* params); +static void EventTracingThreadFunc(void* params); // Atomic-int fast path for avoiding logging when disabled. static volatile int g_event_logging_active = 0; @@ -89,7 +89,10 @@ static volatile int g_event_logging_active = 0; class EventLogger final { public: EventLogger() - : logging_thread_(EventTracingThreadFunc, this, "EventTracingThread"), + : logging_thread_(EventTracingThreadFunc, + this, + "EventTracingThread", + kLowPriority), shutdown_event_(false, false) {} ~EventLogger() { RTC_DCHECK(thread_checker_.CalledOnValidThread()); } @@ -210,7 +213,6 @@ class EventLogger final { // Finally start, everything should be set up now. logging_thread_.Start(); TRACE_EVENT_INSTANT0("webrtc", "EventLogger::Start"); - logging_thread_.SetPriority(kLowPriority); } void Stop() { @@ -326,11 +328,8 @@ class EventLogger final { bool output_file_owned_ = false; }; -static bool EventTracingThreadFunc(void* params) { +static void EventTracingThreadFunc(void* params) { static_cast(params)->Log(); - // False indicates that the thread function has done its job and doesn't need - // to be restarted again. Log() runs its own internal loop. - return false; } static EventLogger* volatile g_event_logger = nullptr; diff --git a/webrtc/base/platform_thread.cc b/webrtc/base/platform_thread.cc index d1bd509bf1..525f0ddf08 100644 --- a/webrtc/base/platform_thread.cc +++ b/webrtc/base/platform_thread.cc @@ -92,14 +92,27 @@ struct ThreadAttributes { #endif // defined(WEBRTC_WIN) } -PlatformThread::PlatformThread(ThreadRunFunction func, +PlatformThread::PlatformThread(ThreadRunFunctionDeprecated func, void* obj, const char* thread_name) - : run_function_(func), + : run_function_deprecated_(func), obj_(obj), name_(thread_name ? thread_name : "webrtc") { RTC_DCHECK(func); RTC_DCHECK(name_.length() < 64); + spawned_thread_checker_.DetachFromThread(); +} + +PlatformThread::PlatformThread(ThreadRunFunction func, + void* obj, + const char* thread_name, + ThreadPriority priority /*= kNormalPriority*/) + : run_function_(func), priority_(priority), obj_(obj), name_(thread_name) { + RTC_DCHECK(func); + RTC_DCHECK(!name_.empty()); + // TODO(tommi): Consider lowering the limit to 15 (limit on Linux). + RTC_DCHECK(name_.length() < 64); + spawned_thread_checker_.DetachFromThread(); } PlatformThread::~PlatformThread() { @@ -180,11 +193,14 @@ void PlatformThread::Stop() { thread_ = nullptr; thread_id_ = 0; #else - RTC_CHECK_EQ(1, AtomicOps::Increment(&stop_flag_)); + if (!run_function_) + RTC_CHECK_EQ(1, AtomicOps::Increment(&stop_flag_)); RTC_CHECK_EQ(0, pthread_join(thread_, nullptr)); - AtomicOps::ReleaseStore(&stop_flag_, 0); + if (!run_function_) + AtomicOps::ReleaseStore(&stop_flag_, 0); thread_ = 0; #endif // defined(WEBRTC_WIN) + spawned_thread_checker_.DetachFromThread(); } // TODO(tommi): Deprecate the loop behavior in PlatformThread. @@ -195,8 +211,16 @@ void PlatformThread::Stop() { // All implementations will need to be aware of how the thread should be stopped // and encouraging a busy polling loop, can be costly in terms of power and cpu. void PlatformThread::Run() { - if (!name_.empty()) - rtc::SetCurrentThreadName(name_.c_str()); + // Attach the worker thread checker to this thread. + RTC_DCHECK(spawned_thread_checker_.CalledOnValidThread()); + rtc::SetCurrentThreadName(name_.c_str()); + + if (run_function_) { + SetPriority(priority_); + run_function_(obj_); + return; + } +// TODO(tommi): Delete the below. #if !defined(WEBRTC_MAC) && !defined(WEBRTC_WIN) const struct timespec ts_null = {0}; #endif @@ -204,7 +228,7 @@ void PlatformThread::Run() { // The interface contract of Start/Stop is that for a successful call to // Start, there should be at least one call to the run function. So we // call the function before checking |stop_|. - if (!run_function_(obj_)) + if (!run_function_deprecated_(obj_)) break; #if defined(WEBRTC_WIN) // Alertable sleep to permit RaiseFlag to run and update |stop_|. @@ -221,8 +245,19 @@ void PlatformThread::Run() { } bool PlatformThread::SetPriority(ThreadPriority priority) { - RTC_DCHECK(thread_checker_.CalledOnValidThread()); - RTC_DCHECK(IsRunning()); +#if RTC_DCHECK_IS_ON + if (run_function_) { + // The non-deprecated way of how this function gets called, is that it must + // be called on the worker thread itself. + RTC_DCHECK(spawned_thread_checker_.CalledOnValidThread()); + } else { + // In the case of deprecated use of this method, it must be called on the + // same thread as the PlatformThread object is constructed on. + RTC_DCHECK(thread_checker_.CalledOnValidThread()); + RTC_DCHECK(IsRunning()); + } +#endif + #if defined(WEBRTC_WIN) return SetThreadPriority(thread_, priority) != FALSE; #elif defined(__native_client__) diff --git a/webrtc/base/platform_thread.h b/webrtc/base/platform_thread.h index 667407fabc..ed26ca03cc 100644 --- a/webrtc/base/platform_thread.h +++ b/webrtc/base/platform_thread.h @@ -32,7 +32,8 @@ void SetCurrentThreadName(const char* name); // Callback function that the spawned thread will enter once spawned. // A return value of false is interpreted as that the function has no // more work to do and that the thread can be released. -typedef bool (*ThreadRunFunction)(void*); +typedef bool (*ThreadRunFunctionDeprecated)(void*); +typedef void (*ThreadRunFunction)(void*); enum ThreadPriority { #ifdef WEBRTC_WIN @@ -55,7 +56,13 @@ enum ThreadPriority { // called from the same thread, including instantiation. class PlatformThread { public: - PlatformThread(ThreadRunFunction func, void* obj, const char* thread_name); + PlatformThread(ThreadRunFunctionDeprecated func, + void* obj, + const char* thread_name); + PlatformThread(ThreadRunFunction func, + void* obj, + const char* thread_name, + ThreadPriority priority = kNormalPriority); virtual ~PlatformThread(); const std::string& name() const { return name_; } @@ -74,6 +81,7 @@ class PlatformThread { void Stop(); // Set the priority of the thread. Must be called when thread is running. + // TODO(tommi): Make private and only allow public support via ctor. bool SetPriority(ThreadPriority priority); protected: @@ -85,12 +93,15 @@ class PlatformThread { private: void Run(); - ThreadRunFunction const run_function_; + ThreadRunFunctionDeprecated const run_function_deprecated_ = nullptr; + ThreadRunFunction const run_function_ = nullptr; + const ThreadPriority priority_ = kNormalPriority; void* const obj_; // TODO(pbos): Make sure call sites use string literals and update to a const // char* instead of a std::string. const std::string name_; rtc::ThreadChecker thread_checker_; + rtc::ThreadChecker spawned_thread_checker_; #if defined(WEBRTC_WIN) static DWORD WINAPI StartThread(void* param); diff --git a/webrtc/base/platform_thread_unittest.cc b/webrtc/base/platform_thread_unittest.cc index 322eb35ffc..d6d35e40e4 100644 --- a/webrtc/base/platform_thread_unittest.cc +++ b/webrtc/base/platform_thread_unittest.cc @@ -16,20 +16,65 @@ namespace rtc { namespace { // Function that does nothing, and reports success. -bool NullRunFunction(void* obj) { +bool NullRunFunctionDeprecated(void* obj) { webrtc::SleepMs(0); // Hand over timeslice, prevents busy looping. return true; } +void NullRunFunction(void* obj) {} + // Function that sets a boolean. -bool SetFlagRunFunction(void* obj) { +bool SetFlagRunFunctionDeprecated(void* obj) { bool* obj_as_bool = static_cast(obj); *obj_as_bool = true; webrtc::SleepMs(0); // Hand over timeslice, prevents busy looping. return true; } + +void SetFlagRunFunction(void* obj) { + bool* obj_as_bool = static_cast(obj); + *obj_as_bool = true; +} + } // namespace +TEST(PlatformThreadTest, StartStopDeprecated) { + PlatformThread thread(&NullRunFunctionDeprecated, nullptr, + "PlatformThreadTest"); + EXPECT_TRUE(thread.name() == "PlatformThreadTest"); + EXPECT_TRUE(thread.GetThreadRef() == 0); + thread.Start(); + EXPECT_TRUE(thread.GetThreadRef() != 0); + thread.Stop(); + EXPECT_TRUE(thread.GetThreadRef() == 0); +} + +TEST(PlatformThreadTest, StartStop2Deprecated) { + PlatformThread thread1(&NullRunFunctionDeprecated, nullptr, + "PlatformThreadTest1"); + PlatformThread thread2(&NullRunFunctionDeprecated, nullptr, + "PlatformThreadTest2"); + EXPECT_TRUE(thread1.GetThreadRef() == thread2.GetThreadRef()); + thread1.Start(); + thread2.Start(); + EXPECT_TRUE(thread1.GetThreadRef() != thread2.GetThreadRef()); + thread2.Stop(); + thread1.Stop(); +} + +TEST(PlatformThreadTest, RunFunctionIsCalledDeprecated) { + bool flag = false; + PlatformThread thread(&SetFlagRunFunctionDeprecated, &flag, + "RunFunctionIsCalled"); + thread.Start(); + + // At this point, the flag may be either true or false. + thread.Stop(); + + // We expect the thread to have run at least once. + EXPECT_TRUE(flag); +} + TEST(PlatformThreadTest, StartStop) { PlatformThread thread(&NullRunFunction, nullptr, "PlatformThreadTest"); EXPECT_TRUE(thread.name() == "PlatformThreadTest"); @@ -62,4 +107,5 @@ TEST(PlatformThreadTest, RunFunctionIsCalled) { // We expect the thread to have run at least once. EXPECT_TRUE(flag); } + } // rtc diff --git a/webrtc/base/rate_limiter_unittest.cc b/webrtc/base/rate_limiter_unittest.cc index 3c1b541f5e..6d925670bf 100644 --- a/webrtc/base/rate_limiter_unittest.cc +++ b/webrtc/base/rate_limiter_unittest.cc @@ -130,9 +130,8 @@ class ThreadTask { rtc::Event end_signal_; }; -bool RunTask(void* thread_task) { +void RunTask(void* thread_task) { reinterpret_cast(thread_task)->Run(); - return false; } TEST_F(RateLimitTest, MultiThreadedUsage) { diff --git a/webrtc/base/sequenced_task_checker_unittest.cc b/webrtc/base/sequenced_task_checker_unittest.cc index 8588648316..ae6e09dff7 100644 --- a/webrtc/base/sequenced_task_checker_unittest.cc +++ b/webrtc/base/sequenced_task_checker_unittest.cc @@ -35,14 +35,13 @@ class CallCalledSequentiallyOnThread { } private: - static bool Run(void* obj) { + static void Run(void* obj) { CallCalledSequentiallyOnThread* call_stuff_on_thread = static_cast(obj); EXPECT_EQ( call_stuff_on_thread->expect_true_, call_stuff_on_thread->sequenced_task_checker_->CalledSequentially()); call_stuff_on_thread->thread_has_run_event_.Set(); - return false; } const bool expect_true_; diff --git a/webrtc/base/task_queue.h b/webrtc/base/task_queue.h index a6a70364ac..5fcf00fcd2 100644 --- a/webrtc/base/task_queue.h +++ b/webrtc/base/task_queue.h @@ -232,7 +232,7 @@ class LOCKABLE TaskQueue { private: #if defined(WEBRTC_BUILD_LIBEVENT) - static bool ThreadMain(void* context); + static void ThreadMain(void* context); static void OnWakeup(int socket, short flags, void* context); // NOLINT static void RunTask(int fd, short flags, void* context); // NOLINT static void RunTimer(int fd, short flags, void* context); // NOLINT @@ -263,7 +263,7 @@ class LOCKABLE TaskQueue { class MultimediaTimer; typedef std::unordered_map> DelayedTasks; - static bool ThreadMain(void* context); + static void ThreadMain(void* context); static bool ProcessQueuedMessages(DelayedTasks* delayed_tasks, std::vector* timers); diff --git a/webrtc/base/task_queue_libevent.cc b/webrtc/base/task_queue_libevent.cc index f3ab89be80..c80258811f 100644 --- a/webrtc/base/task_queue_libevent.cc +++ b/webrtc/base/task_queue_libevent.cc @@ -256,7 +256,7 @@ void TaskQueue::PostTaskAndReply(std::unique_ptr task, } // static -bool TaskQueue::ThreadMain(void* context) { +void TaskQueue::ThreadMain(void* context) { TaskQueue* me = static_cast(context); QueueContext queue_context(me); @@ -269,8 +269,6 @@ bool TaskQueue::ThreadMain(void* context) { for (TimerEvent* timer : queue_context.pending_timers_) delete timer; - - return false; } // static diff --git a/webrtc/base/task_queue_win.cc b/webrtc/base/task_queue_win.cc index 11aa81de03..c8ef7211e8 100644 --- a/webrtc/base/task_queue_win.cc +++ b/webrtc/base/task_queue_win.cc @@ -228,7 +228,7 @@ void TaskQueue::PostTaskAndReply(std::unique_ptr task, } // static -bool TaskQueue::ThreadMain(void* context) { +void TaskQueue::ThreadMain(void* context) { HANDLE timer_handles[MultimediaTimer::kMaxTimers]; // Active multimedia timers. std::vector mm_timers; @@ -283,8 +283,6 @@ bool TaskQueue::ThreadMain(void* context) { RTC_DCHECK_EQ(WAIT_IO_COMPLETION, result); } } - - return false; } // static diff --git a/webrtc/call/rampup_tests.cc b/webrtc/call/rampup_tests.cc index 94153b08ae..6ec3d836ab 100644 --- a/webrtc/call/rampup_tests.cc +++ b/webrtc/call/rampup_tests.cc @@ -44,7 +44,7 @@ RampUpTester::RampUpTester(size_t num_video_streams, bool red, bool report_perf_stats) : EndToEndTest(test::CallTest::kLongTimeoutMs), - event_(false, false), + stop_event_(false, false), clock_(Clock::GetRealTimeClock()), num_video_streams_(num_video_streams), num_audio_streams_(num_audio_streams), @@ -72,7 +72,6 @@ RampUpTester::RampUpTester(size_t num_video_streams, } RampUpTester::~RampUpTester() { - event_.Set(); } Call::Config RampUpTester::GetSenderCallConfig() { @@ -282,25 +281,25 @@ void RampUpTester::OnCallsCreated(Call* sender_call, Call* receiver_call) { sender_call_ = sender_call; } -bool RampUpTester::BitrateStatsPollingThread(void* obj) { - return static_cast(obj)->PollStats(); +void RampUpTester::BitrateStatsPollingThread(void* obj) { + static_cast(obj)->PollStats(); } -bool RampUpTester::PollStats() { - if (sender_call_) { - Call::Stats stats = sender_call_->GetStats(); +void RampUpTester::PollStats() { + do { + if (sender_call_) { + Call::Stats stats = sender_call_->GetStats(); - EXPECT_GE(stats.send_bandwidth_bps, start_bitrate_bps_); - EXPECT_GE(expected_bitrate_bps_, 0); - if (stats.send_bandwidth_bps >= expected_bitrate_bps_ && - (min_run_time_ms_ == -1 || - clock_->TimeInMilliseconds() - test_start_ms_ >= min_run_time_ms_)) { - ramp_up_finished_ms_ = clock_->TimeInMilliseconds(); - observation_complete_.Set(); + EXPECT_GE(stats.send_bandwidth_bps, start_bitrate_bps_); + EXPECT_GE(expected_bitrate_bps_, 0); + if (stats.send_bandwidth_bps >= expected_bitrate_bps_ && + (min_run_time_ms_ == -1 || + clock_->TimeInMilliseconds() - test_start_ms_ >= min_run_time_ms_)) { + ramp_up_finished_ms_ = clock_->TimeInMilliseconds(); + observation_complete_.Set(); + } } - } - - return !event_.Wait(kPollIntervalMs); + } while (!stop_event_.Wait(kPollIntervalMs)); } void RampUpTester::ReportResult(const std::string& measurement, @@ -380,6 +379,7 @@ void RampUpTester::PerformTest() { poller_thread_.Start(); EXPECT_TRUE(Wait()) << "Timed out while waiting for ramp-up to complete."; TriggerTestDone(); + stop_event_.Set(); poller_thread_.Stop(); } @@ -415,22 +415,22 @@ RampUpDownUpTester::RampUpDownUpTester(size_t num_video_streams, RampUpDownUpTester::~RampUpDownUpTester() {} -bool RampUpDownUpTester::PollStats() { - if (send_stream_) { - webrtc::VideoSendStream::Stats stats = send_stream_->GetStats(); - int transmit_bitrate_bps = 0; - for (auto it : stats.substreams) { - transmit_bitrate_bps += it.second.total_bitrate_bps; +void RampUpDownUpTester::PollStats() { + do { + if (send_stream_) { + webrtc::VideoSendStream::Stats stats = send_stream_->GetStats(); + int transmit_bitrate_bps = 0; + for (auto it : stats.substreams) { + transmit_bitrate_bps += it.second.total_bitrate_bps; + } + EvolveTestState(transmit_bitrate_bps, stats.suspended); + } else if (num_audio_streams_ > 0 && sender_call_ != nullptr) { + // An audio send stream doesn't have bitrate stats, so the call send BW is + // currently used instead. + int transmit_bitrate_bps = sender_call_->GetStats().send_bandwidth_bps; + EvolveTestState(transmit_bitrate_bps, false); } - EvolveTestState(transmit_bitrate_bps, stats.suspended); - } else if (num_audio_streams_ > 0 && sender_call_ != nullptr) { - // An audio send stream doesn't have bitrate stats, so the call send BW is - // currently used instead. - int transmit_bitrate_bps = sender_call_->GetStats().send_bandwidth_bps; - EvolveTestState(transmit_bitrate_bps, false); - } - - return !event_.Wait(kPollIntervalMs); + } while (!stop_event_.Wait(kPollIntervalMs)); } Call::Config RampUpDownUpTester::GetReceiverCallConfig() { diff --git a/webrtc/call/rampup_tests.h b/webrtc/call/rampup_tests.h index 65276ea265..b9fd5974ee 100644 --- a/webrtc/call/rampup_tests.h +++ b/webrtc/call/rampup_tests.h @@ -49,7 +49,7 @@ class RampUpTester : public test::EndToEndTest { void PerformTest() override; protected: - virtual bool PollStats(); + virtual void PollStats(); void AccumulateStats(const VideoSendStream::StreamStats& stream, size_t* total_packets_sent, @@ -63,7 +63,7 @@ class RampUpTester : public test::EndToEndTest { void TriggerTestDone(); webrtc::RtcEventLogNullImpl event_log_; - rtc::Event event_; + rtc::Event stop_event_; Clock* const clock_; FakeNetworkPipe::Config forward_transport_config_; const size_t num_video_streams_; @@ -95,7 +95,7 @@ class RampUpTester : public test::EndToEndTest { std::vector* receive_configs) override; void OnCallsCreated(Call* sender_call, Call* receiver_call) override; - static bool BitrateStatsPollingThread(void* obj); + static void BitrateStatsPollingThread(void* obj); const int start_bitrate_bps_; const int64_t min_run_time_ms_; @@ -125,7 +125,7 @@ class RampUpDownUpTester : public RampUpTester { ~RampUpDownUpTester() override; protected: - bool PollStats() override; + void PollStats() override; private: enum TestStates { diff --git a/webrtc/common_video/include/incoming_video_stream.h b/webrtc/common_video/include/incoming_video_stream.h index 250bed8dbf..2ea80eace7 100644 --- a/webrtc/common_video/include/incoming_video_stream.h +++ b/webrtc/common_video/include/incoming_video_stream.h @@ -31,14 +31,10 @@ class IncomingVideoStream : public rtc::VideoSinkInterface { ~IncomingVideoStream() override; protected: - static bool IncomingVideoStreamThreadFun(void* obj); - bool IncomingVideoStreamProcess(); + static void IncomingVideoStreamThreadFun(void* obj); + void IncomingVideoStreamProcess(); private: - enum { kEventStartupTimeMs = 10 }; - enum { kEventMaxWaitTimeMs = 100 }; - enum { kFrameRatePeriodMs = 1000 }; - void OnFrame(const VideoFrame& video_frame) override; rtc::ThreadChecker main_thread_checker_; diff --git a/webrtc/common_video/incoming_video_stream.cc b/webrtc/common_video/incoming_video_stream.cc index ed7b9ea29d..c1f61d1cfb 100644 --- a/webrtc/common_video/incoming_video_stream.cc +++ b/webrtc/common_video/incoming_video_stream.cc @@ -16,13 +16,18 @@ #include "webrtc/system_wrappers/include/event_wrapper.h" namespace webrtc { +namespace { +const int kEventStartupTimeMs = 10; +const int kEventMaxWaitTimeMs = 100; +} // namespace IncomingVideoStream::IncomingVideoStream( int32_t delay_ms, rtc::VideoSinkInterface* callback) : incoming_render_thread_(&IncomingVideoStreamThreadFun, this, - "IncomingVideoStreamThread"), + "IncomingVideoStreamThread", + rtc::kRealtimePriority), deliver_buffer_event_(EventTimerWrapper::Create()), external_callback_(callback), render_buffers_(new VideoRenderFrames(delay_ms)) { @@ -32,7 +37,6 @@ IncomingVideoStream::IncomingVideoStream( deliver_buffer_event_->StartTimer(false, kEventStartupTimeMs); incoming_render_thread_.Start(); - incoming_render_thread_.SetPriority(rtc::kRealtimePriority); } IncomingVideoStream::~IncomingVideoStream() { @@ -57,39 +61,44 @@ void IncomingVideoStream::OnFrame(const VideoFrame& video_frame) { } } -bool IncomingVideoStream::IncomingVideoStreamThreadFun(void* obj) { - return static_cast(obj)->IncomingVideoStreamProcess(); +// static +void IncomingVideoStream::IncomingVideoStreamThreadFun(void* obj) { + static_cast(obj)->IncomingVideoStreamProcess(); } -bool IncomingVideoStream::IncomingVideoStreamProcess() { +void IncomingVideoStream::IncomingVideoStreamProcess() { RTC_DCHECK_RUN_ON(&render_thread_checker_); - if (kEventError != deliver_buffer_event_->Wait(kEventMaxWaitTimeMs)) { - // Get a new frame to render and the time for the frame after this one. - rtc::Optional frame_to_render; - uint32_t wait_time; - { - rtc::CritScope cs(&buffer_critsect_); - if (!render_buffers_.get()) { - // Terminating - return false; + while (true) { + if (kEventError != deliver_buffer_event_->Wait(kEventMaxWaitTimeMs)) { + // Get a new frame to render and the time for the frame after this one. + rtc::Optional frame_to_render; + uint32_t wait_time; + { + rtc::CritScope cs(&buffer_critsect_); + if (!render_buffers_.get()) { + // Terminating + return; + } + + frame_to_render = render_buffers_->FrameToRender(); + wait_time = render_buffers_->TimeToNextFrameRelease(); } - frame_to_render = render_buffers_->FrameToRender(); - wait_time = render_buffers_->TimeToNextFrameRelease(); - } - // Set timer for next frame to render. - if (wait_time > kEventMaxWaitTimeMs) { - wait_time = kEventMaxWaitTimeMs; - } + // Set timer for next frame to render. + if (wait_time > kEventMaxWaitTimeMs) { + wait_time = kEventMaxWaitTimeMs; + } - deliver_buffer_event_->StartTimer(false, wait_time); + deliver_buffer_event_->StartTimer(false, wait_time); - if (frame_to_render) { - external_callback_->OnFrame(*frame_to_render); + if (frame_to_render) { + external_callback_->OnFrame(*frame_to_render); + } + } else { + RTC_NOTREACHED(); } } - return true; } } // namespace webrtc diff --git a/webrtc/logging/rtc_event_log/rtc_event_log_helper_thread.cc b/webrtc/logging/rtc_event_log/rtc_event_log_helper_thread.cc index e7edd9a93c..f8fda49223 100644 --- a/webrtc/logging/rtc_event_log/rtc_event_log_helper_thread.cc +++ b/webrtc/logging/rtc_event_log/rtc_event_log_helper_thread.cc @@ -307,10 +307,9 @@ void RtcEventLogHelperThread::ProcessEvents() { } } -bool RtcEventLogHelperThread::ThreadOutputFunction(void* obj) { +void RtcEventLogHelperThread::ThreadOutputFunction(void* obj) { RtcEventLogHelperThread* helper = static_cast(obj); helper->ProcessEvents(); - return false; } } // namespace webrtc diff --git a/webrtc/logging/rtc_event_log/rtc_event_log_helper_thread.h b/webrtc/logging/rtc_event_log/rtc_event_log_helper_thread.h index 7d41f60db2..420e5c529f 100644 --- a/webrtc/logging/rtc_event_log/rtc_event_log_helper_thread.h +++ b/webrtc/logging/rtc_event_log/rtc_event_log_helper_thread.h @@ -81,7 +81,7 @@ class RtcEventLogHelperThread final { void SignalNewEvent(); private: - static bool ThreadOutputFunction(void* obj); + static void ThreadOutputFunction(void* obj); bool AppendEventToString(rtclog::Event* event); bool LogToMemory(); diff --git a/webrtc/modules/video_coding/frame_buffer2_unittest.cc b/webrtc/modules/video_coding/frame_buffer2_unittest.cc index c3d8ce7fab..b67d99376f 100644 --- a/webrtc/modules/video_coding/frame_buffer2_unittest.cc +++ b/webrtc/modules/video_coding/frame_buffer2_unittest.cc @@ -197,7 +197,7 @@ class TestFrameBuffer2 : public ::testing::Test { ASSERT_FALSE(frames_[index]); } - static bool ExtractLoop(void* obj) { + static void ExtractLoop(void* obj) { TestFrameBuffer2* tfb = static_cast(obj); while (true) { tfb->trigger_extract_event_.Wait(rtc::Event::kForever); @@ -205,7 +205,7 @@ class TestFrameBuffer2 : public ::testing::Test { rtc::CritScope lock(&tfb->crit_); tfb->crit_acquired_event_.Set(); if (tfb->tear_down_) - return false; + return; std::unique_ptr frame; FrameBuffer::ReturnReason res = diff --git a/webrtc/system_wrappers/source/critical_section_unittest.cc b/webrtc/system_wrappers/source/critical_section_unittest.cc index 6d73242f6a..f6ceff7c2f 100644 --- a/webrtc/system_wrappers/source/critical_section_unittest.cc +++ b/webrtc/system_wrappers/source/critical_section_unittest.cc @@ -68,10 +68,9 @@ public: } }; -bool LockUnlockThenStopRunFunction(void* obj) { +void LockUnlockThenStopRunFunction(void* obj) { ProtectedCount* the_count = static_cast(obj); the_count->Increment(); - return false; } TEST_F(CritSectTest, ThreadWakesOnce) NO_THREAD_SAFETY_ANALYSIS { diff --git a/webrtc/video/end_to_end_tests.cc b/webrtc/video/end_to_end_tests.cc index 6137308f6c..f427471193 100644 --- a/webrtc/video/end_to_end_tests.cc +++ b/webrtc/video/end_to_end_tests.cc @@ -2142,45 +2142,45 @@ TEST_P(EndToEndTest, RembWithSendSideBwe) { sender_call_ = sender_call; } - static bool BitrateStatsPollingThread(void* obj) { - return static_cast(obj)->PollStats(); + static void BitrateStatsPollingThread(void* obj) { + static_cast(obj)->PollStats(); } - bool PollStats() { - if (sender_call_) { - Call::Stats stats = sender_call_->GetStats(); - switch (state_) { - case kWaitForFirstRampUp: - if (stats.send_bandwidth_bps >= remb_bitrate_bps_) { - state_ = kWaitForRemb; - remb_bitrate_bps_ /= 2; - rtp_rtcp_->SetREMBData( - remb_bitrate_bps_, - std::vector(&sender_ssrc_, &sender_ssrc_ + 1)); - rtp_rtcp_->SendRTCP(kRtcpRr); - } - break; + void PollStats() { + do { + if (sender_call_) { + Call::Stats stats = sender_call_->GetStats(); + switch (state_) { + case kWaitForFirstRampUp: + if (stats.send_bandwidth_bps >= remb_bitrate_bps_) { + state_ = kWaitForRemb; + remb_bitrate_bps_ /= 2; + rtp_rtcp_->SetREMBData( + remb_bitrate_bps_, + std::vector(&sender_ssrc_, &sender_ssrc_ + 1)); + rtp_rtcp_->SendRTCP(kRtcpRr); + } + break; - case kWaitForRemb: - if (stats.send_bandwidth_bps == remb_bitrate_bps_) { - state_ = kWaitForSecondRampUp; - remb_bitrate_bps_ *= 2; - rtp_rtcp_->SetREMBData( - remb_bitrate_bps_, - std::vector(&sender_ssrc_, &sender_ssrc_ + 1)); - rtp_rtcp_->SendRTCP(kRtcpRr); - } - break; + case kWaitForRemb: + if (stats.send_bandwidth_bps == remb_bitrate_bps_) { + state_ = kWaitForSecondRampUp; + remb_bitrate_bps_ *= 2; + rtp_rtcp_->SetREMBData( + remb_bitrate_bps_, + std::vector(&sender_ssrc_, &sender_ssrc_ + 1)); + rtp_rtcp_->SendRTCP(kRtcpRr); + } + break; - case kWaitForSecondRampUp: - if (stats.send_bandwidth_bps == remb_bitrate_bps_) { - observation_complete_.Set(); - } - break; + case kWaitForSecondRampUp: + if (stats.send_bandwidth_bps == remb_bitrate_bps_) { + observation_complete_.Set(); + } + break; + } } - } - - return !stop_event_.Wait(1000); + } while (!stop_event_.Wait(1000)); } void PerformTest() override { diff --git a/webrtc/video/video_quality_test.cc b/webrtc/video/video_quality_test.cc index b754d29548..c8e379e4e9 100644 --- a/webrtc/video/video_quality_test.cc +++ b/webrtc/video/video_quality_test.cc @@ -555,37 +555,34 @@ class VideoAnalyzer : public PacketReceiver, comparison_available_event_.Set(); } - static bool PollStatsThread(void* obj) { - return static_cast(obj)->PollStats(); + static void PollStatsThread(void* obj) { + static_cast(obj)->PollStats(); } - bool PollStats() { - if (done_.Wait(kSendStatsPollingIntervalMs)) - return false; + void PollStats() { + while (!done_.Wait(kSendStatsPollingIntervalMs)) { + rtc::CritScope crit(&comparison_lock_); - rtc::CritScope crit(&comparison_lock_); + VideoSendStream::Stats send_stats = send_stream_->GetStats(); + // It's not certain that we yet have estimates for any of these stats. + // Check that they are positive before mixing them in. + if (send_stats.encode_frame_rate > 0) + encode_frame_rate_.AddSample(send_stats.encode_frame_rate); + if (send_stats.avg_encode_time_ms > 0) + encode_time_ms_.AddSample(send_stats.avg_encode_time_ms); + if (send_stats.encode_usage_percent > 0) + encode_usage_percent_.AddSample(send_stats.encode_usage_percent); + if (send_stats.media_bitrate_bps > 0) + media_bitrate_bps_.AddSample(send_stats.media_bitrate_bps); - VideoSendStream::Stats send_stats = send_stream_->GetStats(); - // It's not certain that we yet have estimates for any of these stats. Check - // that they are positive before mixing them in. - if (send_stats.encode_frame_rate > 0) - encode_frame_rate_.AddSample(send_stats.encode_frame_rate); - if (send_stats.avg_encode_time_ms > 0) - encode_time_ms_.AddSample(send_stats.avg_encode_time_ms); - if (send_stats.encode_usage_percent > 0) - encode_usage_percent_.AddSample(send_stats.encode_usage_percent); - if (send_stats.media_bitrate_bps > 0) - media_bitrate_bps_.AddSample(send_stats.media_bitrate_bps); - - if (receive_stream_ != nullptr) { - VideoReceiveStream::Stats receive_stats = receive_stream_->GetStats(); - if (receive_stats.decode_ms > 0) - decode_time_ms_.AddSample(receive_stats.decode_ms); - if (receive_stats.max_decode_ms > 0) - decode_time_max_ms_.AddSample(receive_stats.max_decode_ms); + if (receive_stream_ != nullptr) { + VideoReceiveStream::Stats receive_stats = receive_stream_->GetStats(); + if (receive_stats.decode_ms > 0) + decode_time_ms_.AddSample(receive_stats.decode_ms); + if (receive_stats.max_decode_ms > 0) + decode_time_max_ms_.AddSample(receive_stats.max_decode_ms); + } } - - return true; } static bool FrameComparisonThread(void* obj) { diff --git a/webrtc/voice_engine/channel_proxy.cc b/webrtc/voice_engine/channel_proxy.cc index 86501409e2..5538d9ee9e 100644 --- a/webrtc/voice_engine/channel_proxy.cc +++ b/webrtc/voice_engine/channel_proxy.cc @@ -155,8 +155,12 @@ bool ChannelProxy::SendTelephoneEventOutband(int event, int duration_ms) { } void ChannelProxy::SetBitrate(int bitrate_bps, int64_t probing_interval_ms) { - RTC_DCHECK(worker_thread_checker_.CalledOnValidThread() || - module_process_thread_checker_.CalledOnValidThread()); + // This method can be called on the worker thread, module process thread + // or on a TaskQueue via VideoSendStreamImpl::OnEncoderConfigurationChanged. + // TODO(solenberg): Figure out a good way to check this or enforce calling + // rules. + // RTC_DCHECK(worker_thread_checker_.CalledOnValidThread() || + // module_process_thread_checker_.CalledOnValidThread()); channel()->SetBitRate(bitrate_bps, probing_interval_ms); }