diff --git a/talk/media/webrtc/webrtcvideocapturer.cc b/talk/media/webrtc/webrtcvideocapturer.cc index 259f53ade0..aaa6f1e480 100644 --- a/talk/media/webrtc/webrtcvideocapturer.cc +++ b/talk/media/webrtc/webrtcvideocapturer.cc @@ -34,6 +34,8 @@ #ifdef HAVE_WEBRTC_VIDEO #include "talk/media/webrtc/webrtcvideoframe.h" #include "talk/media/webrtc/webrtcvideoframefactory.h" +#include "webrtc/base/bind.h" +#include "webrtc/base/checks.h" #include "webrtc/base/criticalsection.h" #include "webrtc/base/logging.h" #include "webrtc/base/safe_conversions.h" @@ -127,14 +129,16 @@ static bool FormatToCapability(const VideoFormat& format, WebRtcVideoCapturer::WebRtcVideoCapturer() : factory_(new WebRtcVcmFactory), module_(NULL), - captured_frames_(0) { + captured_frames_(0), + start_thread_(nullptr) { set_frame_factory(new WebRtcVideoFrameFactory()); } WebRtcVideoCapturer::WebRtcVideoCapturer(WebRtcVcmFactoryInterface* factory) : factory_(factory), module_(NULL), - captured_frames_(0) { + captured_frames_(0), + start_thread_(nullptr) { set_frame_factory(new WebRtcVideoFrameFactory()); } @@ -145,6 +149,7 @@ WebRtcVideoCapturer::~WebRtcVideoCapturer() { } bool WebRtcVideoCapturer::Init(const Device& device) { + DCHECK(!start_thread_); if (module_) { LOG(LS_ERROR) << "The capturer is already initialized"; return false; @@ -221,6 +226,7 @@ bool WebRtcVideoCapturer::Init(const Device& device) { } bool WebRtcVideoCapturer::Init(webrtc::VideoCaptureModule* module) { + DCHECK(!start_thread_); if (module_) { LOG(LS_ERROR) << "The capturer is already initialized"; return false; @@ -271,12 +277,15 @@ CaptureState WebRtcVideoCapturer::Start(const VideoFormat& capture_format) { } rtc::CritScope cs(&critical_section_stopping_); - // TODO(hellner): weird to return failure when it is in fact actually running. if (IsRunning()) { LOG(LS_ERROR) << "The capturer is already running"; return CS_FAILED; } + DCHECK(!start_thread_); + + start_thread_ = rtc::Thread::Current(); + SetCaptureFormat(&capture_format); webrtc::VideoCaptureCapability cap; @@ -290,6 +299,7 @@ CaptureState WebRtcVideoCapturer::Start(const VideoFormat& capture_format) { module_->RegisterCaptureDataCallback(*this); if (module_->StartCapture(cap) != 0) { LOG(LS_ERROR) << "Camera '" << camera_id << "' failed to start"; + start_thread_ = nullptr; return CS_FAILED; } @@ -310,6 +320,7 @@ CaptureState WebRtcVideoCapturer::Start(const VideoFormat& capture_format) { void WebRtcVideoCapturer::Stop() { rtc::CritScope cs(&critical_section_stopping_); if (IsRunning()) { + DCHECK(start_thread_); rtc::Thread::Current()->Clear(this); module_->StopCapture(); module_->DeRegisterCaptureDataCallback(); @@ -322,6 +333,7 @@ void WebRtcVideoCapturer::Stop() { << drop_ratio << "%"; } SetCaptureFormat(NULL); + start_thread_ = nullptr; } bool WebRtcVideoCapturer::IsRunning() { @@ -363,16 +375,18 @@ void WebRtcVideoCapturer::OnIncomingCapturedFrame(const int32_t id, << ". Expected format " << GetCaptureFormat()->ToString(); } - // Signal down stream components on captured frame. - // The CapturedFrame class doesn't support planes. We have to ExtractBuffer - // to one block for it. - size_t length = - webrtc::CalcBufferSize(webrtc::kI420, sample.width(), sample.height()); - capture_buffer_.resize(length); - // TODO(ronghuawu): Refactor the WebRtcCapturedFrame to avoid memory copy. - webrtc::ExtractBuffer(sample, length, &capture_buffer_[0]); - WebRtcCapturedFrame frame(sample, &capture_buffer_[0], length); - SignalFrameCaptured(this, &frame); + if (start_thread_->IsCurrent()) { + SignalFrameCapturedOnStartThread(&sample); + } else { + // This currently happens on with at least VideoCaptureModuleV4L2 and + // possibly other implementations of WebRTC's VideoCaptureModule. + // In order to maintain the threading contract with the upper layers and + // consistency with other capturers such as in Chrome, we need to do a + // thread hop. + start_thread_->Invoke( + rtc::Bind(&WebRtcVideoCapturer::SignalFrameCapturedOnStartThread, + this, &sample)); + } } void WebRtcVideoCapturer::OnCaptureDelayChanged(const int32_t id, @@ -380,6 +394,22 @@ void WebRtcVideoCapturer::OnCaptureDelayChanged(const int32_t id, LOG(LS_INFO) << "Capture delay changed to " << delay << " ms"; } +void WebRtcVideoCapturer::SignalFrameCapturedOnStartThread( + webrtc::I420VideoFrame* frame) { + DCHECK(start_thread_->IsCurrent()); + // Signal down stream components on captured frame. + // The CapturedFrame class doesn't support planes. We have to ExtractBuffer + // to one block for it. + size_t length = + webrtc::CalcBufferSize(webrtc::kI420, frame->width(), frame->height()); + capture_buffer_.resize(length); + // TODO(magjed): Refactor the WebRtcCapturedFrame to avoid memory copy or + // take over ownership of the buffer held by |frame| if that's possible. + webrtc::ExtractBuffer(*frame, length, &capture_buffer_[0]); + WebRtcCapturedFrame webrtc_frame(*frame, &capture_buffer_[0], length); + SignalFrameCaptured(this, &webrtc_frame); +} + // WebRtcCapturedFrame WebRtcCapturedFrame::WebRtcCapturedFrame(const webrtc::I420VideoFrame& sample, void* buffer, diff --git a/talk/media/webrtc/webrtcvideocapturer.h b/talk/media/webrtc/webrtcvideocapturer.h index a529ddd3ee..c0f7807e6f 100644 --- a/talk/media/webrtc/webrtcvideocapturer.h +++ b/talk/media/webrtc/webrtcvideocapturer.h @@ -85,10 +85,19 @@ class WebRtcVideoCapturer : public VideoCapturer, virtual void OnCaptureDelayChanged(const int32_t id, const int32_t delay); + // Used to signal captured frames on the same thread as invoked Start(). + // With WebRTC's current VideoCapturer implementations, this will mean a + // thread hop, but in other implementations (e.g. Chrome) it will be called + // directly from OnIncomingCapturedFrame. + // TODO(tommi): Remove this workaround when we've updated the WebRTC capturers + // to follow the same contract. + void SignalFrameCapturedOnStartThread(webrtc::I420VideoFrame* frame); + rtc::scoped_ptr factory_; webrtc::VideoCaptureModule* module_; int captured_frames_; std::vector capture_buffer_; + rtc::Thread* start_thread_; // Set in Start(), unset in Stop(); // Critical section to avoid Stop during an OnIncomingCapturedFrame callback. rtc::CriticalSection critical_section_stopping_; diff --git a/talk/media/webrtc/webrtcvideoengine.cc b/talk/media/webrtc/webrtcvideoengine.cc index f9508e1cc9..d4db7b7e27 100644 --- a/talk/media/webrtc/webrtcvideoengine.cc +++ b/talk/media/webrtc/webrtcvideoengine.cc @@ -3904,6 +3904,7 @@ int WebRtcVideoMediaChannel::GetRecvChannelId(uint32 ssrc) { bool WebRtcVideoMediaChannel::SetSendParams( WebRtcVideoChannelSendInfo* send_channel, const VideoSendParams& send_params) { + ASSERT(engine()->worker_thread()->IsCurrent()); const int channel_id = send_channel->channel_id(); MaybeRegisterExternalEncoder(send_channel, send_params.codec); diff --git a/webrtc/modules/video_coding/main/interface/video_coding.h b/webrtc/modules/video_coding/main/interface/video_coding.h index 02b36b14bb..bba68c56f0 100644 --- a/webrtc/modules/video_coding/main/interface/video_coding.h +++ b/webrtc/modules/video_coding/main/interface/video_coding.h @@ -11,6 +11,16 @@ #ifndef WEBRTC_MODULES_INTERFACE_VIDEO_CODING_H_ #define WEBRTC_MODULES_INTERFACE_VIDEO_CODING_H_ +#if defined(WEBRTC_WIN) +// This is a workaround on Windows due to the fact that some Windows +// headers define CreateEvent as a macro to either CreateEventW or CreateEventA. +// This can cause problems since we use that name as well and could +// declare them as one thing here whereas in another place a windows header +// may have been included and then implementing CreateEvent() causes compilation +// errors. So for consistency, we include the main windows header here. +#include +#endif + #include "webrtc/common_video/interface/i420_video_frame.h" #include "webrtc/modules/interface/module.h" #include "webrtc/modules/interface/module_common_types.h" @@ -112,6 +122,8 @@ public: // encoding-related settings by calling this function. // For instance, a send codec has to be registered again. // + // NOTE: Must be called on the thread that constructed the VCM instance. + // // Return value : VCM_OK, on success. // < 0, on error. virtual int32_t InitializeSender() = 0; @@ -119,6 +131,8 @@ public: // Registers a codec to be used for encoding. Calling this // API multiple times overwrites any previously registered codecs. // + // NOTE: Must be called on the thread that constructed the VCM instance. + // // Input: // - sendCodec : Settings for the codec to be registered. // - numberOfCores : The number of cores the codec is allowed @@ -132,6 +146,17 @@ public: uint32_t numberOfCores, uint32_t maxPayloadSize) = 0; + // Get the current send codec in use. + // + // If a codec has not been set yet, the |id| property of the return value + // will be 0 and |name| empty. + // + // NOTE: This method intentionally does not hold locks and minimizes data + // copying. It must be called on the thread where the VCM was constructed. + virtual const VideoCodec& GetSendCodec() const = 0; + + // DEPRECATED: Use GetSendCodec() instead. + // // API to get the current send codec in use. // // Input: @@ -139,12 +164,20 @@ public: // // Return value : VCM_OK, on success. // < 0, on error. + // + // NOTE: The returned codec information is not guaranteed to be current when + // the call returns. This method acquires a lock that is aligned with + // video encoding, so it should be assumed to be allowed to block for + // several milliseconds. virtual int32_t SendCodec(VideoCodec* currentSendCodec) const = 0; + // DEPRECATED: Use GetSendCodec() instead. + // // API to get the current send codec type // // Return value : Codec type, on success. // kVideoCodecUnknown, on error or if no send codec is set + // NOTE: Same notes apply as for SendCodec() above. virtual VideoCodecType SendCodec() const = 0; // Register an external encoder object. This can not be used together with diff --git a/webrtc/modules/video_coding/main/source/video_coding_impl.cc b/webrtc/modules/video_coding/main/source/video_coding_impl.cc index a90ab0709e..89eb29bf5b 100644 --- a/webrtc/modules/video_coding/main/source/video_coding_impl.cc +++ b/webrtc/modules/video_coding/main/source/video_coding_impl.cc @@ -111,12 +111,18 @@ class VideoCodingModuleImpl : public VideoCodingModule { return sender_->RegisterSendCodec(sendCodec, numberOfCores, maxPayloadSize); } - virtual int32_t SendCodec(VideoCodec* currentSendCodec) const OVERRIDE { - return sender_->SendCodec(currentSendCodec); + virtual const VideoCodec& GetSendCodec() const OVERRIDE { + return sender_->GetSendCodec(); } + // DEPRECATED. + virtual int32_t SendCodec(VideoCodec* currentSendCodec) const OVERRIDE { + return sender_->SendCodecBlocking(currentSendCodec); + } + + // DEPRECATED. virtual VideoCodecType SendCodec() const OVERRIDE { - return sender_->SendCodec(); + return sender_->SendCodecBlocking(); } virtual int32_t RegisterExternalEncoder(VideoEncoder* externalEncoder, @@ -351,6 +357,8 @@ class VideoCodingModuleImpl : public VideoCodingModule { private: EncodedImageCallbackWrapper post_encode_callback_; + // TODO(tommi): Change sender_ and receiver_ to be non pointers + // (construction is 1 alloc instead of 3). scoped_ptr sender_; scoped_ptr receiver_; scoped_ptr own_event_factory_; diff --git a/webrtc/modules/video_coding/main/source/video_coding_impl.h b/webrtc/modules/video_coding/main/source/video_coding_impl.h index 39e763fc07..ed38bd5e8e 100644 --- a/webrtc/modules/video_coding/main/source/video_coding_impl.h +++ b/webrtc/modules/video_coding/main/source/video_coding_impl.h @@ -16,6 +16,7 @@ #include #include "webrtc/base/thread_annotations.h" +#include "webrtc/base/thread_checker.h" #include "webrtc/modules/video_coding/main/source/codec_database.h" #include "webrtc/modules/video_coding/main/source/frame_buffer.h" #include "webrtc/modules/video_coding/main/source/generic_decoder.h" @@ -62,12 +63,25 @@ class VideoSender { int32_t InitializeSender(); // Register the send codec to be used. + // This method must be called on the construction thread. int32_t RegisterSendCodec(const VideoCodec* sendCodec, uint32_t numberOfCores, uint32_t maxPayloadSize); + // Non-blocking access to the currently active send codec configuration. + // Must be called from the same thread as the VideoSender instance was + // created on. + const VideoCodec& GetSendCodec() const; + + // Get a copy of the currently configured send codec. + // This method acquires a lock to copy the current configuration out, + // so it can block and the returned information is not guaranteed to be + // accurate upon return. Consider using GetSendCodec() instead and make + // decisions on that thread with regards to the current codec. + int32_t SendCodecBlocking(VideoCodec* currentSendCodec) const; + + // Same as SendCodecBlocking. Try to use GetSendCodec() instead. + VideoCodecType SendCodecBlocking() const; - int32_t SendCodec(VideoCodec* currentSendCodec) const; - VideoCodecType SendCodec() const; int32_t RegisterExternalEncoder(VideoEncoder* externalEncoder, uint8_t payloadType, bool internalSource); @@ -124,6 +138,10 @@ class VideoSender { bool frame_dropper_enabled_; VCMProcessTimer _sendStatsTimer; + // Must be accessed on the construction thread of VideoSender. + VideoCodec current_codec_; + rtc::ThreadChecker main_thread_; + VCMQMSettingsCallback* qm_settings_callback_; VCMProtectionCallback* protection_callback_; }; diff --git a/webrtc/modules/video_coding/main/source/video_sender.cc b/webrtc/modules/video_coding/main/source/video_sender.cc index b2dd23e9d8..24240e2ed5 100644 --- a/webrtc/modules/video_coding/main/source/video_sender.cc +++ b/webrtc/modules/video_coding/main/source/video_sender.cc @@ -12,6 +12,7 @@ #include // std::max +#include "webrtc/base/checks.h" #include "webrtc/common_video/libyuv/include/webrtc_libyuv.h" #include "webrtc/modules/video_coding/codecs/interface/video_codec_interface.h" #include "webrtc/modules/video_coding/main/source/encoded_frame.h" @@ -72,8 +73,10 @@ VideoSender::VideoSender(Clock* clock, _codecDataBase(), frame_dropper_enabled_(true), _sendStatsTimer(1000, clock_), + current_codec_(), qm_settings_callback_(NULL), - protection_callback_(NULL) {} + protection_callback_(NULL) { +} VideoSender::~VideoSender() { delete _sendCritSect; @@ -86,13 +89,8 @@ int32_t VideoSender::Process() { _sendStatsTimer.Processed(); CriticalSectionScoped cs(process_crit_sect_.get()); if (_sendStatsCallback != NULL) { - uint32_t bitRate; - uint32_t frameRate; - { - CriticalSectionScoped cs(_sendCritSect); - bitRate = _mediaOpt.SentBitRate(); - frameRate = _mediaOpt.SentFrameRate(); - } + uint32_t bitRate = _mediaOpt.SentBitRate(); + uint32_t frameRate = _mediaOpt.SentFrameRate(); _sendStatsCallback->SendStatistics(bitRate, frameRate); } } @@ -102,6 +100,7 @@ int32_t VideoSender::Process() { // Reset send side to initial state - all components int32_t VideoSender::InitializeSender() { + DCHECK(main_thread_.CalledOnValidThread()); CriticalSectionScoped cs(_sendCritSect); _codecDataBase.ResetSender(); _encoder = NULL; @@ -118,6 +117,7 @@ int64_t VideoSender::TimeUntilNextProcess() { int32_t VideoSender::RegisterSendCodec(const VideoCodec* sendCodec, uint32_t numberOfCores, uint32_t maxPayloadSize) { + DCHECK(main_thread_.CalledOnValidThread()); CriticalSectionScoped cs(_sendCritSect); if (sendCodec == NULL) { return VCM_PARAMETER_ERROR; @@ -129,6 +129,9 @@ int32_t VideoSender::RegisterSendCodec(const VideoCodec* sendCodec, // Update encoder regardless of result to make sure that we're not holding on // to a deleted instance. _encoder = _codecDataBase.GetEncoder(); + // Cache the current codec here so they can be fetched from this thread + // without requiring the _sendCritSect lock. + current_codec_ = *sendCodec; if (!ret) { LOG(LS_ERROR) << "Failed to initialize the encoder with payload name " @@ -162,20 +165,21 @@ int32_t VideoSender::RegisterSendCodec(const VideoCodec* sendCodec, return VCM_OK; } -// Get current send codec -int32_t VideoSender::SendCodec(VideoCodec* currentSendCodec) const { - CriticalSectionScoped cs(_sendCritSect); +const VideoCodec& VideoSender::GetSendCodec() const { + DCHECK(main_thread_.CalledOnValidThread()); + return current_codec_; +} +int32_t VideoSender::SendCodecBlocking(VideoCodec* currentSendCodec) const { + CriticalSectionScoped cs(_sendCritSect); if (currentSendCodec == NULL) { return VCM_PARAMETER_ERROR; } return _codecDataBase.SendCodec(currentSendCodec) ? 0 : -1; } -// Get the current send codec type -VideoCodecType VideoSender::SendCodec() const { +VideoCodecType VideoSender::SendCodecBlocking() const { CriticalSectionScoped cs(_sendCritSect); - return _codecDataBase.SendCodec(); } @@ -214,7 +218,6 @@ int32_t VideoSender::CodecConfigParameters(uint8_t* buffer, // TODO(andresp): Make const once media_opt is thread-safe and this has a // pointer to it. int32_t VideoSender::SentFrameCount(VCMFrameCount* frameCount) { - CriticalSectionScoped cs(_sendCritSect); *frameCount = _mediaOpt.SentFrameCount(); return VCM_OK; } @@ -400,8 +403,6 @@ int32_t VideoSender::EnableFrameDropper(bool enable) { } int VideoSender::SetSenderNackMode(SenderNackMode mode) { - CriticalSectionScoped cs(_sendCritSect); - switch (mode) { case VideoCodingModule::kNackNone: _mediaOpt.EnableProtectionMethod(false, media_optimization::kNack); @@ -411,7 +412,6 @@ int VideoSender::SetSenderNackMode(SenderNackMode mode) { break; case VideoCodingModule::kNackSelective: return VCM_NOT_IMPLEMENTED; - break; } return VCM_OK; } @@ -421,7 +421,6 @@ int VideoSender::SetSenderReferenceSelection(bool enable) { } int VideoSender::SetSenderFEC(bool enable) { - CriticalSectionScoped cs(_sendCritSect); _mediaOpt.EnableProtectionMethod(enable, media_optimization::kFec); return VCM_OK; } @@ -439,17 +438,12 @@ void VideoSender::StopDebugRecording() { } void VideoSender::SuspendBelowMinBitrate() { - CriticalSectionScoped cs(_sendCritSect); - VideoCodec current_send_codec; - if (SendCodec(¤t_send_codec) != 0) { - assert(false); // Must set a send codec before SuspendBelowMinBitrate. - return; - } + DCHECK(main_thread_.CalledOnValidThread()); int threshold_bps; - if (current_send_codec.numberOfSimulcastStreams == 0) { - threshold_bps = current_send_codec.minBitrate * 1000; + if (current_codec_.numberOfSimulcastStreams == 0) { + threshold_bps = current_codec_.minBitrate * 1000; } else { - threshold_bps = current_send_codec.simulcastStream[0].minBitrate * 1000; + threshold_bps = current_codec_.simulcastStream[0].minBitrate * 1000; } // Set the hysteresis window to be at 10% of the threshold, but at least // 10 kbps. diff --git a/webrtc/video_engine/vie_encoder.cc b/webrtc/video_engine/vie_encoder.cc index dae03f05da..9d4fdb2071 100644 --- a/webrtc/video_engine/vie_encoder.cc +++ b/webrtc/video_engine/vie_encoder.cc @@ -414,9 +414,7 @@ int32_t ViEEncoder::SetEncoder(const webrtc::VideoCodec& video_codec) { } int32_t ViEEncoder::GetEncoder(VideoCodec* video_codec) { - if (vcm_.SendCodec(video_codec) != 0) { - return -1; - } + *video_codec = vcm_.GetSendCodec(); return 0; }