From c043afc60534786c88d0cf0f1a96c70a50df87b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Peter=20Bostr=C3=B6m?= Date: Fri, 24 Apr 2015 13:54:27 +0200 Subject: [PATCH] Cleanup inside IncomingVideoStream. Removes logging, thread annotates members (finding 2 bugs where variables were not protected by the correct critsect) and adds consts and scoped_ptrs. BUG= R=mflodman@webrtc.org, stefan@webrtc.org Review URL: https://webrtc-codereview.appspot.com/43219004 Cr-Commit-Position: refs/heads/master@{#9078} --- .../video_render/incoming_video_stream.cc | 172 +++++------------- .../video_render/incoming_video_stream.h | 59 +++--- .../modules/video_render/video_render_impl.cc | 27 +-- .../video_render_internal_impl.cc | 27 +-- 4 files changed, 87 insertions(+), 198 deletions(-) diff --git a/webrtc/modules/video_render/incoming_video_stream.cc b/webrtc/modules/video_render/incoming_video_stream.cc index 04fc6ec713..d5352d61cf 100644 --- a/webrtc/modules/video_render/incoming_video_stream.cc +++ b/webrtc/modules/video_render/incoming_video_stream.cc @@ -31,22 +31,17 @@ namespace webrtc { -IncomingVideoStream::IncomingVideoStream(const int32_t module_id, - const uint32_t stream_id) - : module_id_(module_id), - stream_id_(stream_id), - stream_critsect_(*CriticalSectionWrapper::CreateCriticalSection()), - thread_critsect_(*CriticalSectionWrapper::CreateCriticalSection()), - buffer_critsect_(*CriticalSectionWrapper::CreateCriticalSection()), +IncomingVideoStream::IncomingVideoStream(uint32_t stream_id) + : stream_id_(stream_id), + stream_critsect_(CriticalSectionWrapper::CreateCriticalSection()), + thread_critsect_(CriticalSectionWrapper::CreateCriticalSection()), + buffer_critsect_(CriticalSectionWrapper::CreateCriticalSection()), incoming_render_thread_(), - deliver_buffer_event_(*EventTimerWrapper::Create()), + deliver_buffer_event_(EventTimerWrapper::Create()), running_(false), - external_callback_(NULL), - render_callback_(NULL), - render_buffers_(*(new VideoRenderFrames)), - callbackVideoType_(kVideoI420), - callbackWidth_(0), - callbackHeight_(0), + external_callback_(nullptr), + render_callback_(nullptr), + render_buffers_(new VideoRenderFrames()), incoming_rate_(0), last_rate_calculation_time_ms_(0), num_frames_since_last_calculation_(0), @@ -55,52 +50,29 @@ IncomingVideoStream::IncomingVideoStream(const int32_t module_id, start_image_(), timeout_image_(), timeout_time_() { - WEBRTC_TRACE(kTraceMemory, kTraceVideoRenderer, module_id_, - "%s created for stream %d", __FUNCTION__, stream_id); } IncomingVideoStream::~IncomingVideoStream() { - WEBRTC_TRACE(kTraceMemory, kTraceVideoRenderer, module_id_, - "%s deleted for stream %d", __FUNCTION__, stream_id_); - Stop(); - - // incoming_render_thread_ - Delete in stop - delete &render_buffers_; - delete &stream_critsect_; - delete &buffer_critsect_; - delete &thread_critsect_; - delete &deliver_buffer_event_; -} - -int32_t IncomingVideoStream::ChangeModuleId(const int32_t id) { - CriticalSectionScoped cs(&stream_critsect_); - module_id_ = id; - return 0; } VideoRenderCallback* IncomingVideoStream::ModuleCallback() { - CriticalSectionScoped cs(&stream_critsect_); + CriticalSectionScoped cs(stream_critsect_.get()); return this; } int32_t IncomingVideoStream::RenderFrame(const uint32_t stream_id, const I420VideoFrame& video_frame) { - CriticalSectionScoped csS(&stream_critsect_); - WEBRTC_TRACE(kTraceStream, kTraceVideoRenderer, module_id_, - "%s for stream %d, render time: %u", __FUNCTION__, stream_id_, - video_frame.render_time_ms()); + CriticalSectionScoped csS(stream_critsect_.get()); if (!running_) { - WEBRTC_TRACE(kTraceStream, kTraceVideoRenderer, module_id_, - "%s: Not running", __FUNCTION__); return -1; } // Rate statistics. num_frames_since_last_calculation_++; int64_t now_ms = TickTime::MillisecondTimestamp(); - if (now_ms >= last_rate_calculation_time_ms_ + KFrameRatePeriodMs) { + if (now_ms >= last_rate_calculation_time_ms_ + kFrameRatePeriodMs) { incoming_rate_ = static_cast(1000 * num_frames_since_last_calculation_ / (now_ms - last_rate_calculation_time_ms_)); @@ -109,120 +81,92 @@ int32_t IncomingVideoStream::RenderFrame(const uint32_t stream_id, } // Insert frame. - CriticalSectionScoped csB(&buffer_critsect_); - if (render_buffers_.AddFrame(video_frame) == 1) - deliver_buffer_event_.Set(); + CriticalSectionScoped csB(buffer_critsect_.get()); + if (render_buffers_->AddFrame(video_frame) == 1) + deliver_buffer_event_->Set(); return 0; } int32_t IncomingVideoStream::SetStartImage( const I420VideoFrame& video_frame) { - CriticalSectionScoped csS(&thread_critsect_); + CriticalSectionScoped csS(thread_critsect_.get()); return start_image_.CopyFrame(video_frame); } int32_t IncomingVideoStream::SetTimeoutImage( const I420VideoFrame& video_frame, const uint32_t timeout) { - CriticalSectionScoped csS(&thread_critsect_); + CriticalSectionScoped csS(thread_critsect_.get()); timeout_time_ = timeout; return timeout_image_.CopyFrame(video_frame); } -int32_t IncomingVideoStream::SetRenderCallback( +void IncomingVideoStream::SetRenderCallback( VideoRenderCallback* render_callback) { - CriticalSectionScoped cs(&stream_critsect_); - - WEBRTC_TRACE(kTraceInfo, kTraceVideoRenderer, module_id_, - "%s(%x) for stream %d", __FUNCTION__, render_callback, - stream_id_); + CriticalSectionScoped cs(thread_critsect_.get()); render_callback_ = render_callback; - return 0; } int32_t IncomingVideoStream::SetExpectedRenderDelay( int32_t delay_ms) { - CriticalSectionScoped csS(&stream_critsect_); + CriticalSectionScoped csS(stream_critsect_.get()); if (running_) { - WEBRTC_TRACE(kTraceInfo, kTraceVideoRenderer, module_id_, - "%s(%d) for stream %d", __FUNCTION__, delay_ms, stream_id_); return -1; } - CriticalSectionScoped cs(&buffer_critsect_); - return render_buffers_.SetRenderDelay(delay_ms); + CriticalSectionScoped cs(buffer_critsect_.get()); + return render_buffers_->SetRenderDelay(delay_ms); } -int32_t IncomingVideoStream::SetExternalCallback( +void IncomingVideoStream::SetExternalCallback( VideoRenderCallback* external_callback) { - CriticalSectionScoped cs(&stream_critsect_); - WEBRTC_TRACE(kTraceInfo, kTraceVideoRenderer, module_id_, - "%s(%x) for stream %d", __FUNCTION__, external_callback, - stream_id_); + CriticalSectionScoped cs(thread_critsect_.get()); external_callback_ = external_callback; - callbackVideoType_ = kVideoI420; - callbackWidth_ = 0; - callbackHeight_ = 0; - return 0; } int32_t IncomingVideoStream::Start() { - CriticalSectionScoped csS(&stream_critsect_); - WEBRTC_TRACE(kTraceInfo, kTraceVideoRenderer, module_id_, - "%s for stream %d", __FUNCTION__, stream_id_); + CriticalSectionScoped csS(stream_critsect_.get()); if (running_) { - WEBRTC_TRACE(kTraceWarning, kTraceVideoRenderer, module_id_, - "%s: Already running", __FUNCTION__); return 0; } - CriticalSectionScoped csT(&thread_critsect_); + CriticalSectionScoped csT(thread_critsect_.get()); assert(incoming_render_thread_ == NULL); incoming_render_thread_ = ThreadWrapper::CreateThread( IncomingVideoStreamThreadFun, this, "IncomingVideoStreamThread"); if (!incoming_render_thread_) { - WEBRTC_TRACE(kTraceError, kTraceVideoRenderer, module_id_, - "%s: No thread", __FUNCTION__); return -1; } if (incoming_render_thread_->Start()) { - WEBRTC_TRACE(kTraceInfo, kTraceVideoRenderer, module_id_, - "%s: thread started", __FUNCTION__); } else { - WEBRTC_TRACE(kTraceError, kTraceVideoRenderer, module_id_, - "%s: Could not start send thread", __FUNCTION__); return -1; } incoming_render_thread_->SetPriority(kRealtimePriority); - deliver_buffer_event_.StartTimer(false, KEventStartupTimeMS); + deliver_buffer_event_->StartTimer(false, kEventStartupTimeMs); running_ = true; return 0; } int32_t IncomingVideoStream::Stop() { - CriticalSectionScoped cs_stream(&stream_critsect_); - WEBRTC_TRACE(kTraceInfo, kTraceVideoRenderer, module_id_, - "%s for stream %d", __FUNCTION__, stream_id_); + CriticalSectionScoped cs_stream(stream_critsect_.get()); if (!running_) { - WEBRTC_TRACE(kTraceWarning, kTraceVideoRenderer, module_id_, - "%s: Not running", __FUNCTION__); return 0; } ThreadWrapper* thread = NULL; { - CriticalSectionScoped cs_thread(&thread_critsect_); + CriticalSectionScoped cs_thread(thread_critsect_.get()); if (incoming_render_thread_) { // Setting the incoming render thread to NULL marks that we're performing // a shutdown and will make IncomingVideoStreamProcess abort after wakeup. thread = incoming_render_thread_.release(); - deliver_buffer_event_.StopTimer(); + deliver_buffer_event_->StopTimer(); // Set the event to allow the thread to wake up and shut down without // waiting for a timeout. - deliver_buffer_event_.Set(); + deliver_buffer_event_->Set(); } } if (thread) { @@ -230,8 +174,6 @@ int32_t IncomingVideoStream::Stop() { delete thread; } else { assert(false); - WEBRTC_TRACE(kTraceWarning, kTraceVideoRenderer, module_id_, - "%s: Not able to stop thread, leaking", __FUNCTION__); } } running_ = false; @@ -239,19 +181,17 @@ int32_t IncomingVideoStream::Stop() { } int32_t IncomingVideoStream::Reset() { - CriticalSectionScoped cs_stream(&stream_critsect_); - CriticalSectionScoped cs_buffer(&buffer_critsect_); - render_buffers_.ReleaseAllFrames(); + CriticalSectionScoped cs_buffer(buffer_critsect_.get()); + render_buffers_->ReleaseAllFrames(); return 0; } uint32_t IncomingVideoStream::StreamId() const { - CriticalSectionScoped cs_stream(&stream_critsect_); return stream_id_; } uint32_t IncomingVideoStream::IncomingRate() const { - CriticalSectionScoped cs(&stream_critsect_); + CriticalSectionScoped cs(stream_critsect_.get()); return incoming_rate_; } @@ -260,24 +200,26 @@ bool IncomingVideoStream::IncomingVideoStreamThreadFun(void* obj) { } bool IncomingVideoStream::IncomingVideoStreamProcess() { - if (kEventError != deliver_buffer_event_.Wait(KEventMaxWaitTimeMs)) { - thread_critsect_.Enter(); + if (kEventError != deliver_buffer_event_->Wait(kEventMaxWaitTimeMs)) { + CriticalSectionScoped cs(thread_critsect_.get()); if (incoming_render_thread_ == NULL) { // Terminating - thread_critsect_.Leave(); return false; } // Get a new frame to render and the time for the frame after this one. - buffer_critsect_.Enter(); - I420VideoFrame frame_to_render = render_buffers_.FrameToRender(); - uint32_t wait_time = render_buffers_.TimeToNextFrameRelease(); - buffer_critsect_.Leave(); + I420VideoFrame frame_to_render; + uint32_t wait_time; + { + CriticalSectionScoped cs(buffer_critsect_.get()); + 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; + 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.IsZeroSize()) { if (render_callback_) { @@ -295,33 +237,19 @@ bool IncomingVideoStream::IncomingVideoStreamProcess() { } // No frame. - thread_critsect_.Leave(); return true; } // Send frame for rendering. if (external_callback_) { - WEBRTC_TRACE(kTraceStream, kTraceVideoRenderer, module_id_, - "%s: executing external renderer callback to deliver frame", - __FUNCTION__, frame_to_render.render_time_ms()); external_callback_->RenderFrame(stream_id_, frame_to_render); - } else { - if (render_callback_) { - WEBRTC_TRACE(kTraceStream, kTraceVideoRenderer, module_id_, - "%s: Render frame, time: ", __FUNCTION__, - frame_to_render.render_time_ms()); - render_callback_->RenderFrame(stream_id_, frame_to_render); - } + } else if (render_callback_) { + render_callback_->RenderFrame(stream_id_, frame_to_render); } - // Release critsect before calling the module user. - thread_critsect_.Leave(); - // We're done with this frame. - if (!frame_to_render.IsZeroSize()) { - CriticalSectionScoped cs(&buffer_critsect_); - last_render_time_ms_= frame_to_render.render_time_ms(); - } + if (!frame_to_render.IsZeroSize()) + last_render_time_ms_ = frame_to_render.render_time_ms(); } return true; } diff --git a/webrtc/modules/video_render/incoming_video_stream.h b/webrtc/modules/video_render/incoming_video_stream.h index 833ffe2451..47db31b91f 100644 --- a/webrtc/modules/video_render/incoming_video_stream.h +++ b/webrtc/modules/video_render/incoming_video_stream.h @@ -11,6 +11,7 @@ #ifndef WEBRTC_MODULES_VIDEO_RENDER_MAIN_SOURCE_INCOMING_VIDEO_STREAM_H_ #define WEBRTC_MODULES_VIDEO_RENDER_MAIN_SOURCE_INCOMING_VIDEO_STREAM_H_ +#include "webrtc/base/scoped_ptr.h" #include "webrtc/modules/video_render/include/video_render.h" namespace webrtc { @@ -22,22 +23,19 @@ class VideoRenderFrames; class IncomingVideoStream : public VideoRenderCallback { public: - IncomingVideoStream(const int32_t module_id, - const uint32_t stream_id); + explicit IncomingVideoStream(uint32_t stream_id); ~IncomingVideoStream(); - int32_t ChangeModuleId(const int32_t id); - // Get callback to deliver frames to the module. VideoRenderCallback* ModuleCallback(); virtual int32_t RenderFrame(const uint32_t stream_id, const I420VideoFrame& video_frame); // Set callback to the platform dependent code. - int32_t SetRenderCallback(VideoRenderCallback* render_callback); + void SetRenderCallback(VideoRenderCallback* render_callback); // Callback for file recording, snapshot, ... - int32_t SetExternalCallback(VideoRenderCallback* render_object); + void SetExternalCallback(VideoRenderCallback* render_object); // Start/Stop. int32_t Start(); @@ -62,36 +60,33 @@ class IncomingVideoStream : public VideoRenderCallback { bool IncomingVideoStreamProcess(); private: - enum { KEventStartupTimeMS = 10 }; - enum { KEventMaxWaitTimeMs = 100 }; - enum { KFrameRatePeriodMs = 1000 }; + enum { kEventStartupTimeMs = 10 }; + enum { kEventMaxWaitTimeMs = 100 }; + enum { kFrameRatePeriodMs = 1000 }; - int32_t module_id_; - uint32_t stream_id_; + uint32_t const stream_id_; // Critsects in allowed to enter order. - CriticalSectionWrapper& stream_critsect_; - CriticalSectionWrapper& thread_critsect_; - CriticalSectionWrapper& buffer_critsect_; - rtc::scoped_ptr incoming_render_thread_; - EventTimerWrapper& deliver_buffer_event_; - bool running_; + const rtc::scoped_ptr stream_critsect_; + const rtc::scoped_ptr thread_critsect_; + const rtc::scoped_ptr buffer_critsect_; + rtc::scoped_ptr incoming_render_thread_ + GUARDED_BY(thread_critsect_); + rtc::scoped_ptr deliver_buffer_event_; - VideoRenderCallback* external_callback_; - VideoRenderCallback* render_callback_; - VideoRenderFrames& render_buffers_; + bool running_ GUARDED_BY(stream_critsect_); + VideoRenderCallback* external_callback_ GUARDED_BY(thread_critsect_); + VideoRenderCallback* render_callback_ GUARDED_BY(thread_critsect_); + const rtc::scoped_ptr render_buffers_ + GUARDED_BY(buffer_critsect_); - RawVideoType callbackVideoType_; - uint32_t callbackWidth_; - uint32_t callbackHeight_; - - uint32_t incoming_rate_; - int64_t last_rate_calculation_time_ms_; - uint16_t num_frames_since_last_calculation_; - int64_t last_render_time_ms_; - I420VideoFrame temp_frame_; - I420VideoFrame start_image_; - I420VideoFrame timeout_image_; - uint32_t timeout_time_; + uint32_t incoming_rate_ GUARDED_BY(stream_critsect_); + int64_t last_rate_calculation_time_ms_ GUARDED_BY(stream_critsect_); + uint16_t num_frames_since_last_calculation_ GUARDED_BY(stream_critsect_); + int64_t last_render_time_ms_ GUARDED_BY(thread_critsect_); + I420VideoFrame temp_frame_ GUARDED_BY(thread_critsect_); + I420VideoFrame start_image_ GUARDED_BY(thread_critsect_); + I420VideoFrame timeout_image_ GUARDED_BY(thread_critsect_); + uint32_t timeout_time_ GUARDED_BY(thread_critsect_); }; } // namespace webrtc diff --git a/webrtc/modules/video_render/video_render_impl.cc b/webrtc/modules/video_render/video_render_impl.cc index 283a6d2ae3..96a266c7d6 100644 --- a/webrtc/modules/video_render/video_render_impl.cc +++ b/webrtc/modules/video_render/video_render_impl.cc @@ -197,27 +197,9 @@ ModuleVideoRenderImpl::AddIncomingRenderStream(const uint32_t streamId, } // Create platform independant code - IncomingVideoStream* ptrIncomingStream = new IncomingVideoStream(_id, - streamId); - if (ptrIncomingStream == NULL) - { - WEBRTC_TRACE(kTraceError, kTraceVideoRenderer, _id, - "%s: Can't create incoming stream", __FUNCTION__); - return NULL; - } - - - if (ptrIncomingStream->SetRenderCallback(ptrRenderCallback) == -1) - { - WEBRTC_TRACE(kTraceError, kTraceVideoRenderer, _id, - "%s: Can't set render callback", __FUNCTION__); - delete ptrIncomingStream; - _ptrRenderer->DeleteIncomingRenderStream(streamId); - return NULL; - } - - VideoRenderCallback* moduleCallback = - ptrIncomingStream->ModuleCallback(); + IncomingVideoStream* ptrIncomingStream = new IncomingVideoStream(streamId); + ptrIncomingStream->SetRenderCallback(ptrRenderCallback); + VideoRenderCallback* moduleCallback = ptrIncomingStream->ModuleCallback(); // Store the stream _streamRenderMap[streamId] = ptrIncomingStream; @@ -273,7 +255,8 @@ int32_t ModuleVideoRenderImpl::AddExternalRenderCallback( "%s: could not get stream", __FUNCTION__); return -1; } - return item->second->SetExternalCallback(renderObject); + item->second->SetExternalCallback(renderObject); + return 0; } int32_t ModuleVideoRenderImpl::GetIncomingRenderStreamProperties( diff --git a/webrtc/modules/video_render/video_render_internal_impl.cc b/webrtc/modules/video_render/video_render_internal_impl.cc index 60934b7f6c..42cebf94d2 100644 --- a/webrtc/modules/video_render/video_render_internal_impl.cc +++ b/webrtc/modules/video_render/video_render_internal_impl.cc @@ -420,27 +420,9 @@ ModuleVideoRenderImpl::AddIncomingRenderStream(const uint32_t streamId, } // Create platform independant code - IncomingVideoStream* ptrIncomingStream = new IncomingVideoStream(_id, - streamId); - if (ptrIncomingStream == NULL) - { - WEBRTC_TRACE(kTraceError, kTraceVideoRenderer, _id, - "%s: Can't create incoming stream", __FUNCTION__); - return NULL; - } - - - if (ptrIncomingStream->SetRenderCallback(ptrRenderCallback) == -1) - { - WEBRTC_TRACE(kTraceError, kTraceVideoRenderer, _id, - "%s: Can't set render callback", __FUNCTION__); - delete ptrIncomingStream; - _ptrRenderer->DeleteIncomingRenderStream(streamId); - return NULL; - } - - VideoRenderCallback* moduleCallback = - ptrIncomingStream->ModuleCallback(); + IncomingVideoStream* ptrIncomingStream = new IncomingVideoStream(streamId); + ptrIncomingStream->SetRenderCallback(ptrRenderCallback); + VideoRenderCallback* moduleCallback = ptrIncomingStream->ModuleCallback(); // Store the stream _streamRenderMap[streamId] = ptrIncomingStream; @@ -496,7 +478,8 @@ int32_t ModuleVideoRenderImpl::AddExternalRenderCallback( "%s: could not get stream", __FUNCTION__); return -1; } - return item->second->SetExternalCallback(renderObject); + item->second->SetExternalCallback(renderObject); + return 0; } int32_t ModuleVideoRenderImpl::GetIncomingRenderStreamProperties(