New issue
Advanced search Search tips

Issue 741619 link

Starred by 2 users

Issue metadata

Status: Verified
Owner:
Closed: Oct 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug


Sign in to add a comment

RTCPeerConnection should not rely on remote stream added/remove events

Project Member Reported by hbos@chromium.org, Jul 12 2017

Issue description

addTrack supports stream-less streams, these should be signaled ( https://crbug.com/webrtc/7933 ) as such, meaning we can no longer rely on all remote tracks belonging to a remote stream.

If we move over to only listening to receivers being added or removed, we'll be able to define getRemoteStreams() in terms of receivers' streams ( https://crbug.com/741618 ).
 

Comment 1 by hbos@chromium.org, Aug 23 2017

Blocking: 700916

Comment 2 by hbos@chromium.org, Aug 29 2017

Labels: -M-62 M-63

Comment 3 by hbos@webrtc.org, Aug 30 2017

Blocking: webrtc:7933

Comment 4 by hbos@webrtc.org, Aug 30 2017

Blockedon: -webrtc:7933
Project Member

Comment 5 by bugdroid1@chromium.org, Sep 8 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/13fd38073fc397678c4076dd4e0893db705f544d

commit 13fd38073fc397678c4076dd4e0893db705f544d
Author: Henrik Boström <hbos@chromium.org>
Date: Fri Sep 08 10:56:06 2017

Remove RemoteMediaStreamImpl in favor of WebRtcMediaStreamAdapter.

RTCPeerConnectionHandler keeps track of remote streams using
AdapterRefs from a WebRtcMediaStreamAdapterMap. Follow-up CLs will
allow receivers to hold AdapterRefs, enabling remote streams to
exist independently of the remote_streams_ vector. Ultimately we
want to get rid of the remote_streams_ vector, listen to receivers
being added and removed (allowing streamless remote tracks) and
define getRemoteStreams() as the set "for each receiver, for each
stream of receiver".

Bug:  705901 ,  741619 ,  741618 ,  738929 
Change-Id: I7179626ee20090528a63d0b5a0655fc07e243bd0
Reviewed-on: https://chromium-review.googlesource.com/628524
Commit-Queue: Henrik Boström <hbos@chromium.org>
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: Guido Urdaneta <guidou@chromium.org>
Cr-Commit-Position: refs/heads/master@{#500562}
[modify] https://crrev.com/13fd38073fc397678c4076dd4e0893db705f544d/content/renderer/BUILD.gn
[delete] https://crrev.com/482ec872f8974245c8649ac2fecf60782f907b0e/content/renderer/media/remote_media_stream_impl.cc
[delete] https://crrev.com/482ec872f8974245c8649ac2fecf60782f907b0e/content/renderer/media/remote_media_stream_impl.h
[modify] https://crrev.com/13fd38073fc397678c4076dd4e0893db705f544d/content/renderer/media/rtc_peer_connection_handler.cc
[modify] https://crrev.com/13fd38073fc397678c4076dd4e0893db705f544d/content/renderer/media/rtc_peer_connection_handler.h
[modify] https://crrev.com/13fd38073fc397678c4076dd4e0893db705f544d/content/renderer/media/webrtc/webrtc_media_stream_adapter_map.h

Comment 6 by hbos@chromium.org, Sep 8 2017

Status: Started (was: Assigned)

Comment 7 by hbos@chromium.org, Sep 28 2017

Blocking: 760107

Comment 8 by hbos@chromium.org, Sep 28 2017

Blockedon: 769743

Comment 9 by hbos@chromium.org, Sep 28 2017

Cc: hbos@chromium.org
 Issue 755166  has been merged into this issue.
Project Member

Comment 10 by bugdroid1@chromium.org, Oct 13 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/09343dfc9e9911662d68a400d339f11c7f4854a4

commit 09343dfc9e9911662d68a400d339f11c7f4854a4
Author: Henrik Boström <hbos@chromium.org>
Date: Fri Oct 13 19:53:05 2017

Decoupling remote MediaStream and MediaStreamTrack creation.

The MediaStream::Create taking MediaStreamDescriptor* as an argument
used to unconditionally create MediaStreamTracks from the associated
MediaStreamComponents. This CL makes it possible to create a
MediaStream from a MediaStreamDescriptor and MediaStreamTrackVectors.
DCHECKs ensure the descriptor and tracks match.

This makes it possible to create MediaStream containing pre-existing
tracks by adding the tracks to the stream yourself. This is required for
the track-based APIs.

I exercise creating streams and tracks separately this way in
DidAddRemoteStream even though tracks are always created, yielding
exactly the same result as calling the previous Create function.

Bug:  741619 ,  769743 
Change-Id: Ia0d361546a95b23c9a5275b7657460c7ab4ba73e
Reviewed-on: https://chromium-review.googlesource.com/684297
Commit-Queue: Henrik Boström <hbos@chromium.org>
Reviewed-by: Guido Urdaneta <guidou@chromium.org>
Cr-Commit-Position: refs/heads/master@{#508788}
[modify] https://crrev.com/09343dfc9e9911662d68a400d339f11c7f4854a4/third_party/WebKit/Source/modules/mediastream/MediaStream.cpp
[modify] https://crrev.com/09343dfc9e9911662d68a400d339f11c7f4854a4/third_party/WebKit/Source/modules/mediastream/MediaStream.h
[modify] https://crrev.com/09343dfc9e9911662d68a400d339f11c7f4854a4/third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp

Project Member

Comment 11 by bugdroid1@chromium.org, Oct 13 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/101ae86567ba231c612af756e2bb5925a127ea30

commit 101ae86567ba231c612af756e2bb5925a127ea30
Author: Henrik Boström <hbos@chromium.org>
Date: Fri Oct 13 20:38:59 2017

PC.setRemoteDescription: Use track-based callbacks over stream-based.

In RTP Media API, the processing of remote MediaStreamTracks works with
the "addition" or "removal" of a remote track, i.e. of a receiver. The
receiver has a track and an associated set of streams.
https://w3c.github.io/webrtc-pc/#processing-remote-mediastreamtracks

I refer to the new set of APIs as the track-based APIs, because they're
centered around tracks, tracks being added to streams is a side-effect
to the processing of tracks.

This is different from the stream-based APIs, centered around remote
streams being added or removed. While tracks could be added or removed
from streams in this API there was always the assumption that a stream
owned its track. As such, when a stream was added or removed, all of
its tracks would be created or destroyed.

Switching over to track-based callbacks, the ownership of tracks
changes. A track can be added or removed to zero, one or multiple
streams. On DidAddRemoteTrack we check if a stream or track exist before
we create it and add the track to the stream.

In this CL:
- The receiver's set of associated streams is exposed (C++ layers).
- Stream added/removed callbacks replaced by track added/removed,
  creating or getting streams/tracks based on the receivers.
- Updating mocks and unittests.

All usage cases that worked with addStream/removeStream() should
continue to work with the new callbacks, and the new callbacks will
enable new RTP Media API behavior not previously supported.

Additionally, this fixes  https://crbug.com/773613  because it makes ontrack fire
for new tracks, even if the new tracks are surfaced as belonging to an existing
stream.

Follow-up CLs:
- Add unittests for moving existing tracks between streams and make
  sure we don't end up with multiple tracks.  https://crbug.com/769743 
- With receivers added/removed based on callbacks, define
  getReceivers() as "return rtp_receivers_".  http://crbug.com/755166 

Future CLs:
- All associated events are fired according to spec and in the right
  order.  https://crbug.com/760107 
- Define getRemoteStreams() in terms of "all streams of all receivers".
   https://crbug.com/741618 
- Define addStream/removeStream() in terms of addTrack()/removeTrack().
   https://crbug.com/738929 
- Add usage metrics for add/remove tracks.  https://crbug.com/765170 

Resolving all these bugs will reduce the technical dept.

Bug:  773613 ,  741619 ,  705901 ,  769743 ,  760107 ,  741618 ,  738929 ,  765170 ,  webrtc:7933 
Change-Id: I75c18b889e1d4d5827c53bf6dd627f495ca22b51
Reviewed-on: https://chromium-review.googlesource.com/657639
Commit-Queue: Henrik Boström <hbos@chromium.org>
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: Guido Urdaneta <guidou@chromium.org>
Cr-Commit-Position: refs/heads/master@{#508805}
[modify] https://crrev.com/101ae86567ba231c612af756e2bb5925a127ea30/content/renderer/media/mock_peer_connection_impl.cc
[modify] https://crrev.com/101ae86567ba231c612af756e2bb5925a127ea30/content/renderer/media/mock_peer_connection_impl.h
[modify] https://crrev.com/101ae86567ba231c612af756e2bb5925a127ea30/content/renderer/media/mock_web_rtc_peer_connection_handler_client.cc
[modify] https://crrev.com/101ae86567ba231c612af756e2bb5925a127ea30/content/renderer/media/mock_web_rtc_peer_connection_handler_client.h
[modify] https://crrev.com/101ae86567ba231c612af756e2bb5925a127ea30/content/renderer/media/rtc_peer_connection_handler.cc
[modify] https://crrev.com/101ae86567ba231c612af756e2bb5925a127ea30/content/renderer/media/rtc_peer_connection_handler.h
[modify] https://crrev.com/101ae86567ba231c612af756e2bb5925a127ea30/content/renderer/media/rtc_peer_connection_handler_unittest.cc
[modify] https://crrev.com/101ae86567ba231c612af756e2bb5925a127ea30/content/renderer/media/webrtc/rtc_rtp_receiver.cc
[modify] https://crrev.com/101ae86567ba231c612af756e2bb5925a127ea30/content/renderer/media/webrtc/rtc_rtp_receiver.h
[modify] https://crrev.com/101ae86567ba231c612af756e2bb5925a127ea30/third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp
[modify] https://crrev.com/101ae86567ba231c612af756e2bb5925a127ea30/third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.h
[modify] https://crrev.com/101ae86567ba231c612af756e2bb5925a127ea30/third_party/WebKit/Source/modules/peerconnection/RTCRtpReceiver.cpp
[modify] https://crrev.com/101ae86567ba231c612af756e2bb5925a127ea30/third_party/WebKit/Source/modules/peerconnection/RTCRtpReceiver.h
[modify] https://crrev.com/101ae86567ba231c612af756e2bb5925a127ea30/third_party/WebKit/public/platform/WebRTCPeerConnectionHandlerClient.h
[modify] https://crrev.com/101ae86567ba231c612af756e2bb5925a127ea30/third_party/WebKit/public/platform/WebRTCRtpReceiver.h

Comment 12 by hbos@chromium.org, Oct 13 2017

Status: Verified (was: Started)
The above CL resolves this issue.

Comment 13 by hbos@chromium.org, Oct 17 2017

Blockedon: -769743
Blocking: 769743
Project Member

Comment 14 by bugdroid1@chromium.org, Oct 17 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d9da69b5c188c2bc1996bdf09d93e49fc23febe3

commit d9da69b5c188c2bc1996bdf09d93e49fc23febe3
Author: Henrik Boström <hbos@chromium.org>
Date: Tue Oct 17 18:46:20 2017

Simplify getReceivers().

Now that RTCPeerConnection and RTCPeerConnectionHandler listens to
tracks being added/removed, not streams, we know when receivers are
added or removed and keep track of them.

This simplifies getReceivers() to "return rtp_receivers_;" instead of
querying the lower layers.

Bug:  741619 
Change-Id: I84c43df31259bce8e31b9c34945ed8d972e45ca7
Reviewed-on: https://chromium-review.googlesource.com/707798
Reviewed-by: Guido Urdaneta <guidou@chromium.org>
Reviewed-by: Philip Jägenstedt <foolip@chromium.org>
Reviewed-by: Taylor Brandstetter <deadbeef@chromium.org>
Commit-Queue: Henrik Boström <hbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#509453}
[modify] https://crrev.com/d9da69b5c188c2bc1996bdf09d93e49fc23febe3/content/renderer/media/rtc_peer_connection_handler.cc
[modify] https://crrev.com/d9da69b5c188c2bc1996bdf09d93e49fc23febe3/content/renderer/media/rtc_peer_connection_handler.h
[modify] https://crrev.com/d9da69b5c188c2bc1996bdf09d93e49fc23febe3/content/renderer/media/rtc_peer_connection_handler_unittest.cc
[modify] https://crrev.com/d9da69b5c188c2bc1996bdf09d93e49fc23febe3/third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp
[modify] https://crrev.com/d9da69b5c188c2bc1996bdf09d93e49fc23febe3/third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.h
[modify] https://crrev.com/d9da69b5c188c2bc1996bdf09d93e49fc23febe3/third_party/WebKit/Source/platform/testing/TestingPlatformSupportWithWebRTC.cpp
[modify] https://crrev.com/d9da69b5c188c2bc1996bdf09d93e49fc23febe3/third_party/WebKit/Source/platform/testing/TestingPlatformSupportWithWebRTC.h
[modify] https://crrev.com/d9da69b5c188c2bc1996bdf09d93e49fc23febe3/third_party/WebKit/public/platform/WebRTCPeerConnectionHandler.h

Sign in to add a comment