New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 741618 link

Starred by 2 users

Issue metadata

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

Blocked on:
issue 741619

Blocking:
issue 700916



Sign in to add a comment

RTCPeerConnection.getRemoteStreams() should return all streams of all receivers

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

Issue description

Comment 1 by hbos@chromium.org, Jul 12 2017

Blockedon: 741619

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

Blocking: 700916

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

Labels: -M-62 M-63
Project Member

Comment 4 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 5 by hbos@chromium.org, Sep 8 2017

Status: Started (was: Assigned)
Project Member

Comment 6 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

Project Member

Comment 7 by bugdroid1@chromium.org, Oct 19 2017

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

commit cd258a5bc5f6f9fe8b6fb830b9069236632f2aec
Author: Henrik Boström <hbos@chromium.org>
Date: Thu Oct 19 00:30:50 2017

Redefine getRemoteStreams() as "get all streams of all receivers".

getRemoteStreams() does not exist in the spec. This redefines the method
on top of the RTP Media API and removes the need to keep track of a
remote_streams_ list and some plumbing.

This also makes the order of getReceivers() well-defined (receivers are
iterated in the same order that they were added) so that the order of
getRemoteStreams() remains well-defined and the same as before this CL.

Bug:  741618 
Change-Id: I78d25abf7e01ce8efd9dd085d304e778c9789bc4
Reviewed-on: https://chromium-review.googlesource.com/707693
Commit-Queue: Henrik Boström <hbos@chromium.org>
Reviewed-by: Guido Urdaneta <guidou@chromium.org>
Reviewed-by: Taylor Brandstetter <deadbeef@chromium.org>
Cr-Commit-Position: refs/heads/master@{#509944}
[add] https://crrev.com/cd258a5bc5f6f9fe8b6fb830b9069236632f2aec/third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-getRemoteStreams.html
[modify] https://crrev.com/cd258a5bc5f6f9fe8b6fb830b9069236632f2aec/third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp
[modify] https://crrev.com/cd258a5bc5f6f9fe8b6fb830b9069236632f2aec/third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.h
[modify] https://crrev.com/cd258a5bc5f6f9fe8b6fb830b9069236632f2aec/third_party/WebKit/Source/modules/peerconnection/RTCRtpReceiver.cpp
[modify] https://crrev.com/cd258a5bc5f6f9fe8b6fb830b9069236632f2aec/third_party/WebKit/Source/modules/peerconnection/RTCRtpReceiver.h

Comment 8 by hbos@chromium.org, Oct 19 2017

Cc: deadbeef@chromium.org
Status: Fixed (was: Started)
With the above CL this is fixed.

When we have transceivers, the set of receivers will not decrease as streams are removed. If we want to retain the same behavior of getRemoteStreams()-streams disappearing I suppose we'll have to look at the state of the transceiver to see if it's active, and ignore inactive ones. WDYT deadbeef?
That makes sense. You'd be enumerating transceivers and checking their direction instead of enumerating receivers.
Project Member

Comment 10 by bugdroid1@chromium.org, Feb 21 2018

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

commit dd3c1dab5b9c8e92b9cb0ae3054f8513333e76a3
Author: Henrik Boström <hbos@chromium.org>
Date: Wed Feb 21 14:48:01 2018

Remove RTCPeerConnectionHandler::remote_streams_.

This is no longer needed because we already keep track of remote streams
as "streams of receivers".
Forgotten clean-up relating to the linked bugs (issues already closed).

Bug:  741618 ,  705901 
Change-Id: I062d66d85aded03d013796cfe5e8c237b5d65832
Reviewed-on: https://chromium-review.googlesource.com/928725
Reviewed-by: Guido Urdaneta <guidou@chromium.org>
Commit-Queue: Henrik Boström <hbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#538099}
[modify] https://crrev.com/dd3c1dab5b9c8e92b9cb0ae3054f8513333e76a3/content/renderer/media/webrtc/rtc_peer_connection_handler.cc
[modify] https://crrev.com/dd3c1dab5b9c8e92b9cb0ae3054f8513333e76a3/content/renderer/media/webrtc/rtc_peer_connection_handler.h

Project Member

Comment 11 by bugdroid1@chromium.org, Apr 20 2018

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

commit 37e317d0a7865fb5a31da742384743f2ef5ffc0f
Author: Henrik Boström <hbos@chromium.org>
Date: Fri Apr 20 10:42:26 2018

Remove RTCPeerConnectionHandler::remote_streams_ for reals.

The previous CL that attempted to do this,
https://chromium-review.googlesource.com/928725, removed all usages of
it but forgot to remove the actual member. So doing that here.

Bug:  741618 
Change-Id: Ibc26479b5cf10b45b206d544fa68cb9eff625d31
Reviewed-on: https://chromium-review.googlesource.com/1021472
Reviewed-by: Guido Urdaneta <guidou@chromium.org>
Commit-Queue: Henrik Boström <hbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#552300}
[modify] https://crrev.com/37e317d0a7865fb5a31da742384743f2ef5ffc0f/content/renderer/media/webrtc/rtc_peer_connection_handler.h

Sign in to add a comment