New issue
Advanced search Search tips

Issue 769743 link

Starred by 3 users

Issue metadata

Status: Duplicate
Merged: issue 777617
Owner:
Closed: Sep 4
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocked on:
issue 741619

Blocking:
issue 700916



Sign in to add a comment

Creating remote tracks should be the responsibility of RTCPeerConnection

Project Member Reported by hbos@chromium.org, Sep 28 2017

Issue description

When a remote web track is added to a remote web stream[1], MediaStreamDescriptor::AddRemoteTrack[2] calls MediaStream::AddTrackByComponent[3], which creates a new blink track and adds it to the stream.

This was fine when streams owned tracks, but with the track-based APIs tracks can exist independently of streams, and be added and removed from streams without being destroyed. As such, when a web track is added to a web stream there may already exist a blink track for it. This may result in duplicate blink tracks for the same underlying component.

We should make track creation the responsibility of the RTCPeerConnection, for example at |DidAddRemoteTrack| if that's not too late, to allow us to check if the blink track already exists.

[1] https://cs.chromium.org/chromium/src/content/renderer/media/webrtc/webrtc_media_stream_adapter.cc?l=333

[2] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/mediastream/MediaStreamDescriptor.cpp?l=110

[3] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/mediastream/MediaStream.cpp?l=344


 

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

TODO: Add unittests for tracks jumping between streams. This can be done with replaceTrack or SDP munging. We need to cover two cases:

1. DidAddRemoteTrack with existing stream(s):
   The receiver track must be added to the stream. The track may already exist
   due to wiring explained in this bug or due to 2).

2. DidAddRemoteTrack with new stream(s):
   The stream needs to be created with all of its tracks to reflect the web
   stream. Typically this means creating new tracks, but it's possible for the
   stream to contain tracks that already exists on some other receiver or some
   other stream already.

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

Description: Show this description
Project Member

Comment 3 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 4 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 5 by hbos@chromium.org, Oct 13 2017

Left to do: When tracks are added to a remote stream, instead of the stream creating the blink layer tracks for them, it should ask the PC if the PC has already created them, a "GetOrCreate" call
Project Member

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

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

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

Add test coverage for remote tracks switching streams.

trackSwitchingStream:
1. SRD: "stream1 with track1 created."
2. SRD: "track1 is moved to stream2."

trackAddedToSecondStream:
1. SRD: "stream1 with track1 created."
2. SRD: "stream2 with track1 created, track1 still belonging to stream1."

Bug:  769743 
Change-Id: I64f5c0790c17c2c396f38be39580da495844c749
Reviewed-on: https://chromium-review.googlesource.com/711135
Reviewed-by: Taylor Brandstetter <deadbeef@chromium.org>
Commit-Queue: Henrik Boström <hbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#509300}
[modify] https://crrev.com/a889f268ab3eae9dcbdc77e733d7d8b1505a3e76/chrome/browser/media/webrtc/webrtc_rtp_browsertest.cc
[modify] https://crrev.com/a889f268ab3eae9dcbdc77e733d7d8b1505a3e76/chrome/test/data/webrtc/peerconnection_rtp.js

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

Blocking: -741619

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

Blockedon: 741619

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

Blocking: 760107

Comment 10 by hbos@chromium.org, Nov 27 2017

Blocking: -760107

Comment 11 by hbos@chromium.org, Nov 30 2017

Blocking: 700916

Comment 12 by hbos@chromium.org, Jan 17 2018

Status: Assigned (was: Started)
Have other higher priorities. Before I start this again I need to look at what code is talked about to see if this is still applicable. Remote tracks belong 1:1 to receivers. It should be possible to add/remove streams to a receiver, and as such which stream a track belongs to, but the track is always one m-line, it should be created/removed based on the "ontrack"-like callbacks from third_party/webrtc.

Is there any code that still relies on stream creation for creating tracks?

Comment 13 by hbos@chromium.org, Jan 17 2018

Perhaps related:  https://crbug.com/802938 
Mergedinto: 777617
Status: Duplicate (was: Assigned)
This was fixed by one of the transceiver CLs, don't remember which.

Sign in to add a comment