Support for multiple PeerConnections

This CL has been merged into https://chromiumcodereview.appspot.com/9699069/
---

This updates the PeerConnection glue to support multiple PeerConnections. The functionality depends on JSEP support in libjingle.


TEST=content_unittests (currently disabled)

[Henrik Grunell, 2012-02-29 13:02:56]

Tommi, can you do an initial review? This CL depends on JSEP support in
libjingle, so it hasn't been entirely tested yet.

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

https://chromiumcodereview.appspot.com/9543001/diff/1/content/renderer/media/...
File content/renderer/media/media_stream_impl.cc (right):

https://chromiumcodereview.appspot.com/9543001/diff/1/content/renderer/media/...
content/renderer/media/media_stream_impl.cc:119:
peer_connection_handlers_.push_back(pc_handler);
maybe we should put a CHECK(pc_handler) here first so that we'll crash early in
case allocation fails.

https://chromiumcodereview.appspot.com/9543001/diff/1/content/renderer/media/...
content/renderer/media/media_stream_impl.cc:129:
video_renderers_.erase(vr_it++);
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?

https://chromiumcodereview.appspot.com/9543001/diff/1/content/renderer/media/...
content/renderer/media/media_stream_impl.cc:266: // TODO(grunell): We are not
informed when a renderer should be deleted.
after the for loop, what about adding:
DCHECK_NE(it, peer_connection_handlers_.end());

(assuming that the loop should always find a match)

https://chromiumcodereview.appspot.com/9543001/diff/1/content/renderer/media/...
content/renderer/media/media_stream_impl.cc:280: if (pc_handler)
in what situation will pc_handler be NULL?

https://chromiumcodereview.appspot.com/9543001/diff/1/content/renderer/media/...
content/renderer/media/media_stream_impl.cc:282:
video_renderers_.erase(desc_label);  // Remove old renderer if exists.
is it conceivable that more than one renderer can exist for the same label?

https://chromiumcodereview.appspot.com/9543001/diff/1/content/renderer/media/...
File content/renderer/media/media_stream_impl.h (right):

https://chromiumcodereview.appspot.com/9543001/diff/1/content/renderer/media/...
content/renderer/media/media_stream_impl.h:174: typedef std::map<std::string,
VideoRendererPair> VideoRendererMap;
maybe a comment that documents what the key is?

https://chromiumcodereview.appspot.com/9543001/diff/1/content/renderer/media/...
File content/renderer/media/media_stream_impl_unittest.cc (right):

https://chromiumcodereview.appspot.com/9543001/diff/1/content/renderer/media/...
content/renderer/media/media_stream_impl_unittest.cc:80:
reinterpret_cast<PeerConnectionHandler*>(pc_handler1));
is reinterpret_cast right here?  It will cast the pointer directly without
regard of inheritance etc.  If WebPeerConnectionHandler is in any way related to
PeerConnectionHandler, then this should be static_cast... and if so, is the cast
even necessary?

https://chromiumcodereview.appspot.com/9543001/diff/1/content/renderer/media/...
content/renderer/media/media_stream_impl_unittest.cc:82: delete pc_handler1;
scoped_ptr?

https://chromiumcodereview.appspot.com/9543001/diff/1/content/renderer/media/...
content/renderer/media/media_stream_impl_unittest.cc:84:
reinterpret_cast<PeerConnectionHandler*>(pc_handler2));
same here

https://chromiumcodereview.appspot.com/9543001/diff/1/content/renderer/media/...
content/renderer/media/media_stream_impl_unittest.cc:86: delete pc_handler2;
scoped_ptr?

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

https://chromiumcodereview.appspot.com/9543001/diff/1/content/renderer/media/...
File content/renderer/media/media_stream_impl.cc (right):

https://chromiumcodereview.appspot.com/9543001/diff/1/content/renderer/media/...
content/renderer/media/media_stream_impl.cc:119:
peer_connection_handlers_.push_back(pc_handler);
On 2012/02/29 13:37:00, tommi wrote:
> maybe we should put a CHECK(pc_handler) here first so that we'll crash early
in
> case allocation fails.

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.)

https://chromiumcodereview.appspot.com/9543001/diff/1/content/renderer/media/...
content/renderer/media/media_stream_impl.cc:129:
video_renderers_.erase(vr_it++);
On 2012/02/29 13:37:00, tommi wrote:
> 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.
It is safe; post increment will be evaluated before calling the function, but
the original value will be passed to the function.

> 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;

This will crash, when incrementing it's already erased.
 
> or is that somehow different than what you're doing?

https://chromiumcodereview.appspot.com/9543001/diff/1/content/renderer/media/...
content/renderer/media/media_stream_impl.cc:266: // TODO(grunell): We are not
informed when a renderer should be deleted.
On 2012/02/29 13:37:00, tommi wrote:
> after the for loop, what about adding:
> DCHECK_NE(it, peer_connection_handlers_.end());
> 
> (assuming that the loop should always find a match)

We can't assume that. See comment below.

https://chromiumcodereview.appspot.com/9543001/diff/1/content/renderer/media/...
content/renderer/media/media_stream_impl.cc:280: if (pc_handler)
On 2012/02/29 13:37:00, tommi wrote:
> in what situation will pc_handler be NULL?

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.

https://chromiumcodereview.appspot.com/9543001/diff/1/content/renderer/media/...
content/renderer/media/media_stream_impl.cc:282:
video_renderers_.erase(desc_label);  // Remove old renderer if exists.
On 2012/02/29 13:37:00, tommi wrote:
> is it conceivable that more than one renderer can exist for the same label?

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.

https://chromiumcodereview.appspot.com/9543001/diff/1/content/renderer/media/...
File content/renderer/media/media_stream_impl.h (right):

https://chromiumcodereview.appspot.com/9543001/diff/1/content/renderer/media/...
content/renderer/media/media_stream_impl.h:174: typedef std::map<std::string,
VideoRendererPair> VideoRendererMap;
On 2012/02/29 13:37:00, tommi wrote:
> maybe a comment that documents what the key is?

Done.

https://chromiumcodereview.appspot.com/9543001/diff/1/content/renderer/media/...
File content/renderer/media/media_stream_impl_unittest.cc (right):

https://chromiumcodereview.appspot.com/9543001/diff/1/content/renderer/media/...
content/renderer/media/media_stream_impl_unittest.cc:80:
reinterpret_cast<PeerConnectionHandler*>(pc_handler1));
On 2012/02/29 13:37:00, tommi wrote:
> is reinterpret_cast right here?  It will cast the pointer directly without
> regard of inheritance etc.  If WebPeerConnectionHandler is in any way related
to
> PeerConnectionHandler, then this should be static_cast... and if so, is the
cast
> even necessary?

Interesting, don't know why I used reinterpret here. Must have forgotten it when
testing something during dev. Changed to static_cast; it's a base->derived cast.

https://chromiumcodereview.appspot.com/9543001/diff/1/content/renderer/media/...
content/renderer/media/media_stream_impl_unittest.cc:82: delete pc_handler1;
On 2012/02/29 13:37:00, tommi wrote:
> scoped_ptr?

I wanted to mimic WebKit here by deleting explicitly directly after
ClosePeerConnection call. It actually doesn't matter for how the test looks
right now, but I prefer it if it's expanded.

https://chromiumcodereview.appspot.com/9543001/diff/1/content/renderer/media/...
content/renderer/media/media_stream_impl_unittest.cc:84:
reinterpret_cast<PeerConnectionHandler*>(pc_handler2));
On 2012/02/29 13:37:00, tommi wrote:
> same here

Done.

https://chromiumcodereview.appspot.com/9543001/diff/1/content/renderer/media/...
content/renderer/media/media_stream_impl_unittest.cc:86: delete pc_handler2;
On 2012/02/29 13:37:00, tommi wrote:
> scoped_ptr?

Same as above.

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

https://chromiumcodereview.appspot.com/9543001/diff/1/content/renderer/media/...
File content/renderer/media/media_stream_impl.cc (right):

https://chromiumcodereview.appspot.com/9543001/diff/1/content/renderer/media/...
content/renderer/media/media_stream_impl.cc:280: if (pc_handler)
On 2012/03/23 12:51:56, Henrik Grunell wrote:
> On 2012/02/29 13:37:00, tommi wrote:
> > in what situation will pc_handler be NULL?
> 
> 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.

Verified OK to return null ptr.