Issue metadata
Sign in to add a comment
|
Creating remote tracks should be the responsibility of RTCPeerConnection |
||||||||||||||||||||||
Issue descriptionWhen 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
,
Sep 28 2017
,
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
,
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
,
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
,
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
,
Oct 17 2017
,
Oct 17 2017
,
Oct 17 2017
,
Nov 27 2017
,
Nov 30 2017
,
Jan 17 2018
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?
,
Jan 17 2018
Perhaps related: https://crbug.com/802938
,
Sep 4
This was fixed by one of the transceiver CLs, don't remember which. |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by hbos@chromium.org
, Sep 28 2017