MediaController/Call instantiation.

talk/app/webrtc/mediacontroller.h

Index: talk/app/webrtc/mediacontroller.h
diff --git a/talk/app/webrtc/audiotrackrenderer.h b/talk/app/webrtc/mediacontroller.h
similarity index 58%
copy from talk/app/webrtc/audiotrackrenderer.h
copy to talk/app/webrtc/mediacontroller.h
index e22805fe19ef918a5983ab2bad9b16a05cd902ff..038e00d2adab9f728499e82265c173ff32b6d51d 100644
--- a/talk/app/webrtc/audiotrackrenderer.h
+++ b/talk/app/webrtc/mediacontroller.h
@@ -1,6 +1,6 @@
 /*
  * libjingle
- * Copyright 2013 Google Inc.
+ * Copyright 2015 Google Inc.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are met:
@@ -25,35 +25,44 @@
  * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
-#ifndef TALK_APP_WEBRTC_AUDIOTRACKRENDERER_H_
-#define TALK_APP_WEBRTC_AUDIOTRACKRENDERER_H_
+#ifndef TALK_APP_WEBRTC_MEDIACONTROLLER_H_
+#define TALK_APP_WEBRTC_MEDIACONTROLLER_H_
 
-#include "talk/media/base/audiorenderer.h"
+#include "webrtc/base/criticalsection.h"
 #include "webrtc/base/thread.h"
+#include "webrtc/call.h"
+
+namespace cricket {
+class VideoChannel;
+}  // namespace cricket
 
 namespace webrtc {
+class VoiceEngine;
 
-// Class used for AudioTrack to get the ID of WebRtc voice channel that
-// the AudioTrack is connecting to.
-// Each AudioTrack owns a AudioTrackRenderer instance.
-// AddChannel() will be called when an AudioTrack is added to a MediaStream.
-// RemoveChannel will be called when the AudioTrack or WebRtc VoE channel is
-// going away.
-// This implementation only supports one channel, and it is only used by
-// Chrome for remote audio tracks."
-class AudioTrackRenderer : public cricket::AudioRenderer {
+// MediaController owns state shared between multiple concurrent media streams.

[pthatcher1, 2015/09/04 04:52:31]

What does "concurrent" mean?  
And what does "media stream" mean?

Perhaps for now we should just say "currently owns shared state between media
channels, but in the future will create and own RtpSenders and RtpReceivers", or
something like that.

[the sun, 2015/09/04 10:18:47]

"media stream" was referring to the actual stream objects that share the state
through the Call instance.
That's slightly better, agreed, although "future" is kind of relative. Doesn't
mean much without a date attached to it. :)

+class MediaController : public webrtc::LoadObserver {
  public:
-  AudioTrackRenderer();
-  ~AudioTrackRenderer();
+  MediaController(rtc::Thread* worker_thread,
+                  webrtc::VoiceEngine* voice_engine);
+  ~MediaController() override;
+
+  webrtc::Call* call() { return call_.get(); }

[pthatcher1, 2015/09/04 04:52:31]

This still needs clear thread safety.

[the sun, 2015/09/04 10:18:47]

Let me first say that thread safety is a very big concern for me. However, I
don't see how this construct is any more dangerous than what is already going on
with e.g. BaseSession* being passed to ChannelManager::CreateNnChannel(). In
this case, the life span of the session, MediaController and Call is close to
identical.

If your objective is that we should fetch the Call* on the worker thread, I can
see a few ways to accomplish that:

1. Forward the MediaController* instead of webrtc::Call* to
ChannelManager::CreateNnChannel(), pass it down to the _w functions and pull out
the Call* there; we could rename this getter call_w() then.

2. Add a "virtual MediaController* BaseSession::media_controller_w() = 0"; now
we can get the Call* just when the mediachannel is created, on the worker
thread.

3. Let ChannelManager keep a map BaseSession* -> Call* so we don't need the MC
at all, right now. Mapping is implicit and lookup/create/destroy Call* all
happens on the worker thread.

I'd pick #2 because that allows the ChannelManager interface to be simpler, or
leave as it is right now.

[pthatcher1, 2015/09/04 22:58:32]

That's exactly why we're refactoring ChannelManager so we don't pass in Session,
and why in the replacement (TranpsortController) we're being very careful about
which methods can be called on which threads.  If all goes well, that CL is
going in today.

Out of those, I think #1 makes a lot of sense.  I'd rather not see anything more
go into BaseSession (#2).  We're trying to get rid of BaseSession.


Another option is to do a thread hop from call() to call_w().  But I think I
like #1 better.  

+  void set_video_channel(cricket::VideoChannel* video_channel);
 
-  // Implements cricket::AudioRenderer.
-  void AddChannel(int channel_id) override;
-  void RemoveChannel(int channel_id) override;
+  // Implements webrtc::LoadObserver.
+  void OnLoadUpdate(Load load) override;
 
  private:
-  int channel_id_;
-};
+  void Construct_w(webrtc::VoiceEngine* voice_engine);
+  void Destruct_w();
 
+  rtc::Thread* worker_thread_;
+  rtc::scoped_ptr<webrtc::Call> call_;
+  rtc::CriticalSection critical_section_;
+  cricket::VideoChannel* video_channel_ GUARDED_BY(critical_section_);
+
+  DISALLOW_IMPLICIT_CONSTRUCTORS(MediaController);
+};
 }  // namespace webrtc
 
-#endif  // TALK_APP_WEBRTC_AUDIOTRACKRENDERER_H_
+#endif  // TALK_APP_WEBRTC_MEDIACONTROLLER_H_