Late initialize MediaController, for less resource i.e. ProcessThread, usage by PeerConnection.

webrtc/api/peerconnection.cc

Index: webrtc/api/peerconnection.cc
diff --git a/webrtc/api/peerconnection.cc b/webrtc/api/peerconnection.cc
index eacbeaedc3c8bd2262169362276572d95772ce73..14841c04a9a645f654d9578b3ff880f1ac0ea1c0 100644
--- a/webrtc/api/peerconnection.cc
+++ b/webrtc/api/peerconnection.cc
@@ -617,26 +617,19 @@ bool PeerConnection::Initialize(
   port_allocator_->set_step_delay(cricket::kMinimumStepDelay);
 
   // We rely on default values when constraints aren't found.
-  cricket::MediaConfig media_config;
-
-  media_config.disable_prerenderer_smoothing =
+  media_config_.disable_prerenderer_smoothing =
       configuration.disable_prerenderer_smoothing;
 
   // Find DSCP constraint.
   FindConstraint(constraints, MediaConstraintsInterface::kEnableDscp,
-                 &media_config.enable_dscp, NULL);
+                 &media_config_.enable_dscp, NULL);
   // Find constraints for cpu overuse detection.
   FindConstraint(constraints, MediaConstraintsInterface::kCpuOveruseDetection,
-                 &media_config.enable_cpu_overuse_detection, NULL);
-
-  media_controller_.reset(factory_->CreateMediaController(media_config));
-
-  remote_stream_factory_.reset(new RemoteMediaStreamFactory(
-      factory_->signaling_thread(), media_controller_->channel_manager()));
+                 &media_config_.enable_cpu_overuse_detection, NULL);
 
   session_.reset(
-      new WebRtcSession(media_controller_.get(), factory_->signaling_thread(),
-                        factory_->worker_thread(), port_allocator_.get()));
+      new WebRtcSession(factory_->signaling_thread(), factory_->worker_thread(),
+                        port_allocator_.get()));
   stats_.reset(new StatsCollector(this));
 
   // Initialize the WebRtcSession. It creates transport channels etc.
@@ -742,12 +735,14 @@ rtc::scoped_refptr<RtpSenderInterface> PeerConnection::AddTrack(
         << "Adding a track with two streams is not currently supported.";
     return nullptr;
   }
+
   // TODO(deadbeef): Support adding a track to two different senders.
   if (FindSenderForTrack(track) != senders_.end()) {
     LOG(LS_ERROR) << "Sender for track " << track->id() << " already exists.";
     return nullptr;
   }
 
+  LateInitialize();

[pthatcher1, 2016/02/24 23:10:09]

Instead of calling LateInitialize everywhere (which is really error-prone), why
not just have the media_controller created on demand in the media_controller()
method?

[the sun, 2016/02/25 13:15:39]

That's a good suggestion, but unfortunately it doesn't apply here as this
function does not reference media_controller_ directly. LateInitialize() is
called here because of how we use WebRtcSession; in essence it is there to
manage the dual states of WebRtcSession.

I guess another option would be to add an internal fully_initialized_session()
helper, which would be slightly better explicit calls to LateInitialize(), but
still very error prone.

   // TODO(deadbeef): Support adding a track to multiple streams.
   rtc::scoped_refptr<RtpSenderInterface> new_sender;
   if (track->kind() == MediaStreamTrackInterface::kAudioKind) {
@@ -816,6 +811,7 @@ rtc::scoped_refptr<DtmfSenderInterface> PeerConnection::CreateDtmfSender(
     return NULL;
   }
 
+  LateInitialize();
   rtc::scoped_refptr<DtmfSenderInterface> sender(
       DtmfSender::Create(track, signaling_thread(), session_.get()));
   if (!sender.get()) {
@@ -829,6 +825,7 @@ rtc::scoped_refptr<RtpSenderInterface> PeerConnection::CreateSender(
     const std::string& kind,
     const std::string& stream_id) {
   TRACE_EVENT0("webrtc", "PeerConnection::CreateSender");
+  LateInitialize();
   rtc::scoped_refptr<RtpSenderInterface> new_sender;
   if (kind == MediaStreamTrackInterface::kAudioKind) {
     new_sender = RtpSenderProxy::Create(
@@ -908,6 +905,7 @@ PeerConnection::CreateDataChannel(
     return nullptr;
   }
 
+  LateInitialize();
   // Trigger the onRenegotiationNeeded event for every new RTP DataChannel, or
   // the first SCTP DataChannel.
   if (session_->data_channel_type() == cricket::DCT_RTP || first_datachannel) {
@@ -977,6 +975,7 @@ void PeerConnection::CreateOffer(CreateSessionDescriptionObserver* observer,
     return;
   }
 
+  LateInitialize();
   cricket::MediaSessionOptions session_options;
   if (!GetOptionsForOffer(options, &session_options)) {
     std::string error = "CreateOffer called with invalid options.";
@@ -997,6 +996,7 @@ void PeerConnection::CreateAnswer(
     return;
   }
 
+  LateInitialize();
   cricket::MediaSessionOptions session_options;
   if (!GetOptionsForAnswer(constraints, &session_options)) {
     std::string error = "CreateAnswer called with invalid constraints.";
@@ -1020,6 +1020,7 @@ void PeerConnection::SetLocalDescription(
     PostSetSessionDescriptionFailure(observer, "SessionDescription is NULL.");
     return;
   }
+  LateInitialize();
   // Update stats here so that we have the most recent stats for tracks and
   // streams that might be removed by updating the session description.
   stats_->UpdateStats(kStatsOutputLevelStandard);
@@ -1098,6 +1099,7 @@ void PeerConnection::SetRemoteDescription(
     PostSetSessionDescriptionFailure(observer, "SessionDescription is NULL.");
     return;
   }
+  LateInitialize();
   // Update stats here so that we have the most recent stats for tracks and
   // streams that might be removed by updating the session description.
   stats_->UpdateStats(kStatsOutputLevelStandard);
@@ -1243,8 +1245,9 @@ void PeerConnection::Close() {
   // Update stats here so that we have the most recent stats for tracks and
   // streams before the channels are closed.
   stats_->UpdateStats(kStatsOutputLevelStandard);
-
   session_->Close();
+  media_controller_.reset(nullptr);
+  remote_stream_factory_.reset(nullptr);
 }
 
 void PeerConnection::OnSessionStateChange(WebRtcSession* /*session*/,
@@ -1320,6 +1323,7 @@ void PeerConnection::OnMessage(rtc::Message* msg) {
 void PeerConnection::CreateAudioReceiver(MediaStreamInterface* stream,
                                          AudioTrackInterface* audio_track,
                                          uint32_t ssrc) {
+  LateInitialize();
   receivers_.push_back(RtpReceiverProxy::Create(
       signaling_thread(),
       new AudioRtpReceiver(audio_track, ssrc, session_.get())));
@@ -1328,6 +1332,7 @@ void PeerConnection::CreateAudioReceiver(MediaStreamInterface* stream,
 void PeerConnection::CreateVideoReceiver(MediaStreamInterface* stream,
                                          VideoTrackInterface* video_track,
                                          uint32_t ssrc) {
+  LateInitialize();
   receivers_.push_back(RtpReceiverProxy::Create(
       signaling_thread(),
       new VideoRtpReceiver(video_track, ssrc, session_.get())));
@@ -1415,6 +1420,7 @@ void PeerConnection::OnAudioTrackAdded(AudioTrackInterface* track,
     return;
   }
 
+  LateInitialize();
   // Normal case; we've never seen this track before.
   rtc::scoped_refptr<RtpSenderInterface> new_sender = RtpSenderProxy::Create(
       signaling_thread(),
@@ -1457,6 +1463,7 @@ void PeerConnection::OnVideoTrackAdded(VideoTrackInterface* track,
     return;
   }
 
+  LateInitialize();
   // Normal case; we've never seen this track before.
   rtc::scoped_refptr<RtpSenderInterface> new_sender = RtpSenderProxy::Create(
       signaling_thread(),
@@ -1497,6 +1504,16 @@ void PeerConnection::PostCreateSessionDescriptionFailure(
   signaling_thread()->Post(this, MSG_CREATE_SESSIONDESCRIPTION_FAILED, msg);
 }
 
+void PeerConnection::LateInitialize() {
+  if (!media_controller_) {
+    RTC_DCHECK(!remote_stream_factory_);
+    media_controller_.reset(factory_->CreateMediaController(media_config_));
+    session_->LateInitialize(media_controller_.get());

[pthatcher1, 2016/02/24 23:10:09]

This would make a lot more sense if WebrtcSession and PeerConnection were one
class.  And it's our intention to merge them, we just haven't done it yet.  If
you'd like to do this "lazy load" thing, I think it would be much better to
merge the two classes first to avoid having to initialize methods in
WebRtcSession.

[the sun, 2016/02/25 13:15:39]

Agree. Like I noted in an initial comment, that is not an option if we are to
land a fix for crbug:582441 for the M50 cut. So the question is whether we want
to land this interim fix or not. Myself, I'm torn.

[pthatcher1, 2016/02/26 21:51:20]

I appreciate that we'd like to fix the bug quickly, but this is would be quite a
nasty temporary fix as-is, and it seems like the bug is a very rare one.  

Maybe if we can fix this CL enough to be not so nasty....

[the sun, 2016/02/29 15:58:22]

Acknowledged.

+    remote_stream_factory_.reset(new RemoteMediaStreamFactory(
+        factory_->signaling_thread(), media_controller_->channel_manager()));

[pthatcher1, 2016/02/24 23:10:09]

I looked into this, and found that the ChannelManager here is only needed for
the SignalVideoCaptureStateChange event, which isn't really needed.  I think we
can remove the signal and remove the need to pass the ChannelManager into the
RemoteMedisStreamFactory.

More info here:
https://docs.google.com/document/d/12LMKcZlcLnw62ijmIiLzjJ6GW_cDhGtOUgKiCVKw9...

[the sun, 2016/02/25 13:15:39]

Wow, thanks for taking such a deep dive!

What about the calls from VideoSource to channel_manager_->AddVideoSink() and
channel_manager_->RemoveVideoSink()? Or the calls from
ChannelManager::StartVideoCapture() onto CaptureManager::StartVideoCapture()
(and similarly for Stop)?

[pthatcher1, 2016/02/26 21:51:20]

ChannelManager::StartVideoCapture() is only called by
VideoSource::Initialize/Create and VideoSource::Restart.

ChannelManager::StopVideoCapture() is only called by VideoSource::Stop() and
VideoSource::~VideoSource.

ChannelManager::AddVideoSink is only called by VideoSource::AddSink.

ChannelManager::RemoveVideoSink is only called by VideoSource::RemoveSink.


In all of these cases, VideoSource knows that it changed something. So I think
it could just do SetState(GetReadyState(capture_state)) at the end of the method
instead of having the change trigger an event which does that.

[the sun, 2016/02/29 15:58:22]

Acknowledged.

+  }
+}
+
 bool PeerConnection::GetOptionsForOffer(
     const PeerConnectionInterface::RTCOfferAnswerOptions& rtc_options,
     cricket::MediaSessionOptions* session_options) {
@@ -1504,6 +1521,7 @@ bool PeerConnection::GetOptionsForOffer(
     return false;
   }
 
+  LateInitialize();
   AddSendStreams(session_options, senders_, rtp_data_channels_);
   // Offer to receive audio/video if the constraint is not set and there are
   // send streams, or we're currently receiving.
@@ -1537,6 +1555,7 @@ bool PeerConnection::GetOptionsForAnswer(
     return false;
   }
 
+  LateInitialize();
   AddSendStreams(session_options, senders_, rtp_data_channels_);
   session_options->bundle_enabled =
       session_options->bundle_enabled &&
@@ -1638,6 +1657,7 @@ void PeerConnection::OnRemoteTrackSeen(const std::string& stream_label,
                                        cricket::MediaType media_type) {
   MediaStreamInterface* stream = remote_streams_->find(stream_label);
 
+  LateInitialize();
   if (media_type == cricket::MEDIA_TYPE_AUDIO) {
     AudioTrackInterface* audio_track = remote_stream_factory_->AddAudioTrack(
         ssrc, session_.get(), stream, track_id);
@@ -1894,6 +1914,7 @@ rtc::scoped_refptr<DataChannel> PeerConnection::InternalCreateDataChannel(
   if (IsClosed()) {
     return nullptr;
   }
+  LateInitialize();
   if (session_->data_channel_type() == cricket::DCT_NONE) {
     LOG(LS_ERROR)
         << "InternalCreateDataChannel: Data is not supported in this call.";