Support for multiple PeerConnections

content/renderer/media/media_stream_impl.cc

Index: content/renderer/media/media_stream_impl.cc
diff --git a/content/renderer/media/media_stream_impl.cc b/content/renderer/media/media_stream_impl.cc
index 76e3384a307e546c67d2066634b4f4c2a9765862..39d6bc8e3574a3c134ab99d613481e42bf6a748b 100644
--- a/content/renderer/media/media_stream_impl.cc
+++ b/content/renderer/media/media_stream_impl.cc
@@ -80,15 +80,13 @@ MediaStreamImpl::MediaStreamImpl(
       p2p_socket_dispatcher_(p2p_socket_dispatcher),
       network_manager_(NULL),
       vc_manager_(vc_manager),
-      peer_connection_handler_(NULL),
-      message_loop_proxy_(base::MessageLoopProxy::current()),
       signaling_thread_(NULL),
       worker_thread_(NULL),
       chrome_worker_thread_("Chrome_libJingle_WorkerThread") {
 }
 
 MediaStreamImpl::~MediaStreamImpl() {
-  DCHECK(!peer_connection_handler_);
+  DCHECK(peer_connection_handlers_.empty());
   if (dependency_factory_.get())
     dependency_factory_->ReleasePeerConnectionFactory();
   if (network_manager_) {
@@ -111,30 +109,39 @@ MediaStreamImpl::~MediaStreamImpl() {
 WebKit::WebPeerConnectionHandler* MediaStreamImpl::CreatePeerConnectionHandler(
     WebKit::WebPeerConnectionHandlerClient* client) {
   DCHECK(CalledOnValidThread());
-  if (peer_connection_handler_) {
-    DVLOG(1) << "A PeerConnection already exists";
-    return NULL;
-  }
   if (!EnsurePeerConnectionFactory())
     return NULL;
 
-  peer_connection_handler_ = new PeerConnectionHandler(
+  PeerConnectionHandler* pc_handler = new PeerConnectionHandler(
       client,
       this,
       dependency_factory_.get());
+  peer_connection_handlers_.push_back(pc_handler);

[tommi (sloooow) - chröme, 2012/02/29 13:37:00]

maybe we should put a CHECK(pc_handler) here first so that we'll crash early in
case allocation fails.

[Henrik Grunell, 2012/03/23 12:51:56]

Crashing is not necessary at all actually. We won't crash later if we just skip
adding it to list. I added an if-else instead, with a log if we fail creating.
(Side note: if WebKit gets a null ptr back it will handle it appropriately.)

 
-  return peer_connection_handler_;
+  return pc_handler;
 }
 
-void MediaStreamImpl::ClosePeerConnection() {
+void MediaStreamImpl::ClosePeerConnection(PeerConnectionHandler* pc_handler) {
   DCHECK(CalledOnValidThread());
-  video_renderer_ = NULL;
-  peer_connection_handler_ = NULL;
+  VideoRendererMap::iterator vr_it = video_renderers_.begin();
+  while (vr_it != video_renderers_.end()) {
+    if (vr_it->second.second == pc_handler) {
+      video_renderers_.erase(vr_it++);

[tommi (sloooow) - chröme, 2012/02/29 13:37:00]

hmm... is it safe to continue using the iterator after you've erased from the
list?  I don't think it's safe for vector, not sure about list.

also, what about using a for loop instead of incrementing in two places? 
alternatively:

if (vr_it->second.second == pc_handler)
  video_renderers_.erase(vr_it);
++vr_it;

or is that somehow different than what you're doing?

[Henrik Grunell, 2012/03/23 12:51:56]

It is safe; post increment will be evaluated before calling the function, but
the original value will be passed to the function.
This will crash, when incrementing it's already erased.
 

+    } else {
+      ++vr_it;
+    }
+  }
+  peer_connection_handlers_.remove(pc_handler);
   // TODO(grunell): This is a temporary workaround for an error in native
   // PeerConnection where added live tracks are not seen on the remote side.
-  MediaStreamTrackPtrMap::const_iterator it = local_tracks_.begin();
-  for (; it != local_tracks_.end(); ++it)
+  // If several PeerConnections are used, the tracks will be ended when the
+  // first PeerConnection is closed, which is an issue. The alternative (skip
+  // this workaround) would mean that only one call could be made properly.
+  // When JSEP support is added this will be fixed in native PeerConnection.
+  for (MediaStreamTrackPtrMap::const_iterator it = local_tracks_.begin();
+       it != local_tracks_.end(); ++it) {
     it->second->set_state(webrtc::MediaStreamTrackInterface::kEnded);
+  }
 }
 
 webrtc::MediaStreamTrackInterface* MediaStreamImpl::GetLocalMediaStreamTrack(
@@ -246,27 +253,35 @@ scoped_refptr<media::VideoDecoder> MediaStreamImpl::GetVideoDecoder(
         capability);
   } else {
     // It's a remote stream.
-    if (!video_renderer_.get())
-      video_renderer_ = new talk_base::RefCountedObject<VideoRendererWrapper>();
-    if (video_renderer_->renderer()) {
-      // The renderer is used by PeerConnection, release it first.
-      if (peer_connection_handler_) {
-        peer_connection_handler_->SetVideoRenderer(
-            UTF16ToUTF8(descriptor.label()),
-            NULL);
+    std::string desc_label = UTF16ToUTF8(descriptor.label());
+    PeerConnectionHandler* pc_handler = NULL;
+    std::list<PeerConnectionHandler*>::iterator it;
+    for (it = peer_connection_handlers_.begin();
+         it != peer_connection_handlers_.end(); ++it) {
+      if ((*it)->HasStream(desc_label)) {
+        pc_handler = *it;
+        break;
       }
-      video_renderer_->SetVideoDecoder(NULL);
     }
+    // TODO(grunell): We are not informed when a renderer should be deleted.

[tommi (sloooow) - chröme, 2012/02/29 13:37:00]

after the for loop, what about adding:
DCHECK_NE(it, peer_connection_handlers_.end());

(assuming that the loop should always find a match)

[Henrik Grunell, 2012/03/23 12:51:56]

We can't assume that. See comment below.

+    // When this has been fixed, ensure we delete it. For now, we hold on
+    // to all renderers until a PeerConnectionHandler is closed or we are
+    // deleted (then all renderers are deleted), so it sort of leaks.
+    // TODO(grunell): There is no support for multiple renderers per stream in
+    // native PeerConnection, this code may need to be updated when that is
+    // supported.
+    talk_base::scoped_refptr<VideoRendererWrapper> video_renderer =
+        new talk_base::RefCountedObject<VideoRendererWrapper>();
     RTCVideoDecoder* rtc_video_decoder = new RTCVideoDecoder(
         message_loop_factory->GetMessageLoop("RtcVideoDecoderThread"),
         url.spec());
     decoder = rtc_video_decoder;
-    video_renderer_->SetVideoDecoder(rtc_video_decoder);
-    if (peer_connection_handler_) {
-      peer_connection_handler_->SetVideoRenderer(
-          UTF16ToUTF8(descriptor.label()),
-          video_renderer_);
-    }
+    video_renderer->SetVideoDecoder(rtc_video_decoder);
+    if (pc_handler)

[tommi (sloooow) - chröme, 2012/02/29 13:37:00]

in what situation will pc_handler be NULL?

[Henrik Grunell, 2012/03/23 12:51:56]

It can be if a PeerConnection is closed and the js app sets the url belonging to
a remote stream in that PC to a video tag. The URL registry in WebKit isn't
cleaned up right  away, so it may give us a stream descriptor, but no PC will be
found for it. I think we should return a null ptr as decoder instead, I'll
verify it's ok but I'll upload a new patch with other changes first.

[Henrik Grunell, 2012/03/26 05:04:26]

Verified OK to return null ptr.

+      pc_handler->SetVideoRenderer(desc_label, video_renderer);
+    video_renderers_.erase(desc_label);  // Remove old renderer if exists.

[tommi (sloooow) - chröme, 2012/02/29 13:37:00]

is it conceivable that more than one renderer can exist for the same label?

[Henrik Grunell, 2012/03/23 12:51:56]

At the moment no, there shall be zero or one renderer wrapper per stream
(desc_label is the stream label). Discussion is ongoing for adding support for
several decoders per stream. It will probably be one renderer wrapper per
stream, several decoders per renderer wrapper. But it's a later change.

+    video_renderers_.insert(
+        std::make_pair(desc_label, std::make_pair(video_renderer, pc_handler)));
   }
   return decoder;
 }