content::RTCRtpSenders/Receivers using track adapter references.

content::RTCRtpSenders/Receivers using track adapter references.

Because senders and receivers can live independently of streams, they
should have their own track adapter references to ensure the glue
between blink and webrtc tracks holds for as long as they are alive.

In this CL, senders and receivers are still bound to tracks that are
part of streams, but with this change there is nothing that prevents
a sender or receiver to have a track that is not bound to any stream.
This will unblock addTrack, removeTrack and replaceTrack work.

Additionally, RTCPeerConnection's maps of senders and receivers is
changed to use WeakMember. This simplifies things, the maps are
cleaned up by themselves and we don't need to concern ourselves with
for what duration a sender or receiver is considered "active".
This will unblock having transceivers and allows the possibility for
senders and receivers to be re-used after becoming "inactive" as they
should according to spec.

BUG=700916, 705901
Review-Url: https://codereview.chromium.org/2946663003
Cr-Commit-Position: refs/heads/master@{#480800}
Committed: https://chromium.googlesource.com/chromium/src/+/ea1bc97d4c563ab5383913096697b679f0a37b63

[hbos_chromium, 2017-06-19 14:19:10]

Patchset #1 (id:1) has been deleted

[hbos_chromium, 2017-06-19 14:23:34]

Description was changed from

==========
RTCRtpSenders/Receivers using track adapter references.

Because senders and receivers can live independently of streams, they
should have their own track adapter references to ensure the glue
between blink and webrtc tracks holds for as long as they are alive.

In this CL, senders and receivers are still bound to tracks that are
part of streams, but with this change there is nothing that prevents
a sender or receiver to have a track that is not bound to any stream.

Additionally, RTCPeerConnection's maps of senders and receivers is
changed to use WeakMember. This simplifies things as it removes the
concept of having inactive senders or receivers and just keeps track of
whichever senders/receivers exist, meaning this code is robust if to if
the usage of senders and receivers change.

BUG=705901, 700916
==========

to

==========
content::RTCRtpSenders/Receivers using track adapter references.

Because senders and receivers can live independently of streams, they
should have their own track adapter references to ensure the glue
between blink and webrtc tracks holds for as long as they are alive.

In this CL, senders and receivers are still bound to tracks that are
part of streams, but with this change there is nothing that prevents
a sender or receiver to have a track that is not bound to any stream.
This will unblock addTrack, removeTrack and replaceTrack work.

Additionally, RTCPeerConnection's maps of senders and receivers is
changed to use WeakMember. This simplifies things, the maps are
cleaned up by themselves and we don't need to concern ourselves with
for what duration a sender or receiver is considered "active".
This will unblock having transceivers and allows the possibility for
senders and receivers to be re-used after becoming "inactive" as they
should according to spec.
==========

[hbos_chromium, 2017-06-19 14:23:38]

The CQ bit was checked by hbos@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-19 14:23:52]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hbos_chromium, 2017-06-19 14:25:11]

hbos@chromium.org changed reviewers:
+ guidou@chromium.org

[hbos_chromium, 2017-06-19 14:25:11]

Please take a look, guidou.

[Guido Urdaneta, 2017-06-19 15:22:56]

Quick comments:
1. Add a BUG= section to the description.
2. Does this CL not affect any tests? Are new tests needed?

[hbos_chromium, 2017-06-19 15:27:45]

Description was changed from

==========
content::RTCRtpSenders/Receivers using track adapter references.

Because senders and receivers can live independently of streams, they
should have their own track adapter references to ensure the glue
between blink and webrtc tracks holds for as long as they are alive.

In this CL, senders and receivers are still bound to tracks that are
part of streams, but with this change there is nothing that prevents
a sender or receiver to have a track that is not bound to any stream.
This will unblock addTrack, removeTrack and replaceTrack work.

Additionally, RTCPeerConnection's maps of senders and receivers is
changed to use WeakMember. This simplifies things, the maps are
cleaned up by themselves and we don't need to concern ourselves with
for what duration a sender or receiver is considered "active".
This will unblock having transceivers and allows the possibility for
senders and receivers to be re-used after becoming "inactive" as they
should according to spec.
==========

to

==========
content::RTCRtpSenders/Receivers using track adapter references.

Because senders and receivers can live independently of streams, they
should have their own track adapter references to ensure the glue
between blink and webrtc tracks holds for as long as they are alive.

In this CL, senders and receivers are still bound to tracks that are
part of streams, but with this change there is nothing that prevents
a sender or receiver to have a track that is not bound to any stream.
This will unblock addTrack, removeTrack and replaceTrack work.

Additionally, RTCPeerConnection's maps of senders and receivers is
changed to use WeakMember. This simplifies things, the maps are
cleaned up by themselves and we don't need to concern ourselves with
for what duration a sender or receiver is considered "active".
This will unblock having transceivers and allows the possibility for
senders and receivers to be re-used after becoming "inactive" as they
should according to spec.

BUG=700916, 705901
==========

[hbos_chromium, 2017-06-19 15:38:53]

On 2017/06/19 15:22:56, Guido Urdaneta wrote:
> Quick comments:
> 1. Add a BUG= section to the description.
> 2. Does this CL not affect any tests? Are new tests needed?

1. Done

2. This changes an "implementation detail". The API and behavior stays the same
(tracks are still part of streams). However, when follow-up CLs add new API it
will be possible to have senders and receivers independent of streams. This will
require new tests, but I'd rather not add tests in this CL for future CL
behavior.

[commit-bot: I haz the power, 2017-06-19 16:04:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-19 16:04:33]

Dry run: This issue passed the CQ dry run.

[Guido Urdaneta, 2017-06-19 16:10:52]

https://codereview.chromium.org/2946663003/diff/20001/content/renderer/media/...
File content/renderer/media/webrtc/rtc_rtp_receiver.h (right):

https://codereview.chromium.org/2946663003/diff/20001/content/renderer/media/...
content/renderer/media/webrtc/rtc_rtp_receiver.h:26:
RTCRtpReceiver(webrtc::RtpReceiverInterface* webrtc_rtp_receiver,
Update raw pointer to scoped_refptr by value.

According to
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/TlL1D-Djt...

the consensus on passing scoped_refptr to a function that takes ownership is to
pass it by value and then call std::move() internally to avoid the additional
AddRef.

https://codereview.chromium.org/2946663003/diff/20001/content/renderer/media/...
File content/renderer/media/webrtc/rtc_rtp_sender.cc (right):

https://codereview.chromium.org/2946663003/diff/20001/content/renderer/media/...
content/renderer/media/webrtc/rtc_rtp_sender.cc:36: DCHECK(track_adapter_ ==
webrtc_rtp_sender_->track());
does DCHECK_EQ work here?

https://codereview.chromium.org/2946663003/diff/20001/content/renderer/media/...
content/renderer/media/webrtc/rtc_rtp_sender.cc:46: DCHECK(track_adapter_ ==
webrtc_rtp_sender_->track());
does DCHECK_EQ work here?

[hbos_chromium, 2017-06-19 16:45:39]

PTAL guidou.

https://codereview.chromium.org/2946663003/diff/20001/content/renderer/media/...
File content/renderer/media/webrtc/rtc_rtp_receiver.h (right):

https://codereview.chromium.org/2946663003/diff/20001/content/renderer/media/...
content/renderer/media/webrtc/rtc_rtp_receiver.h:26:
RTCRtpReceiver(webrtc::RtpReceiverInterface* webrtc_rtp_receiver,
On 2017/06/19 16:10:51, Guido Urdaneta wrote:
> Update raw pointer to scoped_refptr by value.
> 
> According to
>
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/TlL1D-Djt...
> 
> the consensus on passing scoped_refptr to a function that takes ownership is
to
> pass it by value and then call std::move() internally to avoid the additional
> AddRef.

I've never seen std::move be used in combination with scoped_refptr in our
codebase, and I think moving can be misleading since ownership is still shared,
but I've seen const scoped_refptr<>& all over the place. I'll do that?

https://codereview.chromium.org/2946663003/diff/20001/content/renderer/media/...
File content/renderer/media/webrtc/rtc_rtp_sender.cc (right):

https://codereview.chromium.org/2946663003/diff/20001/content/renderer/media/...
content/renderer/media/webrtc/rtc_rtp_sender.cc:36: DCHECK(track_adapter_ ==
webrtc_rtp_sender_->track());
On 2017/06/19 16:10:51, Guido Urdaneta wrote:
> does DCHECK_EQ work here?

No, logging template stuff breaks, the code required to fix is not worth it for
a couple of DCHECKs.

https://codereview.chromium.org/2946663003/diff/20001/content/renderer/media/...
content/renderer/media/webrtc/rtc_rtp_sender.cc:46: DCHECK(track_adapter_ ==
webrtc_rtp_sender_->track());
On 2017/06/19 16:10:51, Guido Urdaneta wrote:
> does DCHECK_EQ work here?

No.

[hbos_chromium, 2017-06-19 16:46:51]

The CQ bit was checked by hbos@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-19 16:47:07]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-19 18:38:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-19 18:38:52]

Dry run: This issue passed the CQ dry run.

[hbos_chromium, 2017-06-20 11:23:24]

The CQ bit was checked by hbos@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-20 11:23:40]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Guido Urdaneta, 2017-06-20 11:26:16]

https://codereview.chromium.org/2946663003/diff/20001/content/renderer/media/...
File content/renderer/media/webrtc/rtc_rtp_receiver.h (right):

https://codereview.chromium.org/2946663003/diff/20001/content/renderer/media/...
content/renderer/media/webrtc/rtc_rtp_receiver.h:26:
RTCRtpReceiver(webrtc::RtpReceiverInterface* webrtc_rtp_receiver,
On 2017/06/19 16:45:39, hbos_chromium wrote:
> On 2017/06/19 16:10:51, Guido Urdaneta wrote:
> > Update raw pointer to scoped_refptr by value.
> > 
> > According to
> >
>
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/TlL1D-Djt...
> > 
> > the consensus on passing scoped_refptr to a function that takes ownership is
> to
> > pass it by value and then call std::move() internally to avoid the
additional
> > AddRef.
> 
> I've never seen std::move be used in combination with scoped_refptr in our
> codebase, and I think moving can be misleading since ownership is still
shared,
> but I've seen const scoped_refptr<>& all over the place. I'll do that?

Pass by value, not const-ref

https://codereview.chromium.org/2946663003/diff/20001/content/renderer/media/...
File content/renderer/media/webrtc/rtc_rtp_sender.cc (right):

https://codereview.chromium.org/2946663003/diff/20001/content/renderer/media/...
content/renderer/media/webrtc/rtc_rtp_sender.cc:36: DCHECK(track_adapter_ ==
webrtc_rtp_sender_->track());
On 2017/06/19 16:45:39, hbos_chromium wrote:
> On 2017/06/19 16:10:51, Guido Urdaneta wrote:
> > does DCHECK_EQ work here?
> 
> No, logging template stuff breaks, the code required to fix is not worth it
for
> a couple of DCHECKs.

Acknowledged.

https://codereview.chromium.org/2946663003/diff/20001/content/renderer/media/...
content/renderer/media/webrtc/rtc_rtp_sender.cc:46: DCHECK(track_adapter_ ==
webrtc_rtp_sender_->track());
On 2017/06/19 16:45:39, hbos_chromium wrote:
> On 2017/06/19 16:10:51, Guido Urdaneta wrote:
> > does DCHECK_EQ work here?
> 
> No.

Acknowledged.

[hbos_chromium, 2017-06-20 11:26:54]

PTAL guidou - changed to scoped_refptr and std::move.

[Guido Urdaneta, 2017-06-20 11:29:35]

lgtm

[Guido Urdaneta, 2017-06-20 11:32:16]

https://codereview.chromium.org/2946663003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.h
(right):

https://codereview.chromium.org/2946663003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.h:214: friend
class RTCPeerConnectionTest;
Maybe I missed this on another review, but can you change this friend to
FRIEND_TEST_ALL_PREFIXES?

[Guido Urdaneta, 2017-06-20 11:47:42]

https://codereview.chromium.org/2946663003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.h
(right):

https://codereview.chromium.org/2946663003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.h:214: friend
class RTCPeerConnectionTest;
On 2017/06/20 11:32:16, Guido Urdaneta wrote:
> Maybe I missed this on another review, but can you change this friend to
> FRIEND_TEST_ALL_PREFIXES?

Note that you don't have to fix this (if there is anything to fix, that is)  on
this CL

[hbos_chromium, 2017-06-20 11:51:01]

The CQ bit was unchecked by hbos@chromium.org

[hbos_chromium, 2017-06-20 11:51:09]

The CQ bit was checked by hbos@chromium.org

[commit-bot: I haz the power, 2017-06-20 11:51:20]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hbos_chromium, 2017-06-20 12:05:49]

https://codereview.chromium.org/2946663003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.h
(right):

https://codereview.chromium.org/2946663003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.h:214: friend
class RTCPeerConnectionTest;
On 2017/06/20 11:47:42, Guido Urdaneta wrote:
> On 2017/06/20 11:32:16, Guido Urdaneta wrote:
> > Maybe I missed this on another review, but can you change this friend to
> > FRIEND_TEST_ALL_PREFIXES?
> 
> Note that you don't have to fix this (if there is anything to fix, that is) 
on
> this CL

Done: https://codereview.chromium.org/2944323002/

[commit-bot: I haz the power, 2017-06-20 13:04:50]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1497959469723170,
"parent_rev": "a76034b7173c0ac0ef978b694a8d41f4fd663780", "commit_rev":
"ea1bc97d4c563ab5383913096697b679f0a37b63"}

[commit-bot: I haz the power, 2017-06-20 13:05:02]

Description was changed from

==========
content::RTCRtpSenders/Receivers using track adapter references.

Because senders and receivers can live independently of streams, they
should have their own track adapter references to ensure the glue
between blink and webrtc tracks holds for as long as they are alive.

In this CL, senders and receivers are still bound to tracks that are
part of streams, but with this change there is nothing that prevents
a sender or receiver to have a track that is not bound to any stream.
This will unblock addTrack, removeTrack and replaceTrack work.

Additionally, RTCPeerConnection's maps of senders and receivers is
changed to use WeakMember. This simplifies things, the maps are
cleaned up by themselves and we don't need to concern ourselves with
for what duration a sender or receiver is considered "active".
This will unblock having transceivers and allows the possibility for
senders and receivers to be re-used after becoming "inactive" as they
should according to spec.

BUG=700916, 705901
==========

to

==========
content::RTCRtpSenders/Receivers using track adapter references.

Because senders and receivers can live independently of streams, they
should have their own track adapter references to ensure the glue
between blink and webrtc tracks holds for as long as they are alive.

In this CL, senders and receivers are still bound to tracks that are
part of streams, but with this change there is nothing that prevents
a sender or receiver to have a track that is not bound to any stream.
This will unblock addTrack, removeTrack and replaceTrack work.

Additionally, RTCPeerConnection's maps of senders and receivers is
changed to use WeakMember. This simplifies things, the maps are
cleaned up by themselves and we don't need to concern ourselves with
for what duration a sender or receiver is considered "active".
This will unblock having transceivers and allows the possibility for
senders and receivers to be re-used after becoming "inactive" as they
should according to spec.

BUG=700916, 705901

Review-Url: https://codereview.chromium.org/2946663003
Cr-Commit-Position: refs/heads/master@{#480800}
Committed:
https://chromium.googlesource.com/chromium/src/+/ea1bc97d4c563ab5383913096697...
==========

[commit-bot: I haz the power, 2017-06-20 13:05:03]

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/ea1bc97d4c563ab5383913096697...