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

Issue 803021 link

Starred by 4 users
Status: Verified
Owner:
hbos@chromium.org
Closed: Mar 2018
Cc:
guidou@chromium.org
niklase@chromium.org
hbos@chromium.org
hta@chromium.org
█ deadbeef@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocked on:
issue webrtc:7815
issue 738918
issue 738929
issue 810415
Blocking:
issue 810708
issue 799030
issue 806875



Sign in to add a comment

Define legacy stream methods on top of RTP Media API

Project Member Reported by hbos@chromium.org, Jan 17 2018 
getLocalStreams() should be "all streams of all (active) senders".
addStream() should be "addTrack() + glue so that stream.addTrack adds the track to the PC"
getRemoteStreams() should be "all streams of all (active) receivers".
These should all be deprecated and removed at some point (to be tracked separately when that happens).

Using this bug ad a blockedon tracker for related bugs

Comment 1 by hbos@chromium.org, Jan 18 2018

Blocking: 799030

Comment 2 by hbos@chromium.org, Jan 29 2018

Cc: deadbeef@chromium.org guidou@chromium.org niklase@chromium.org hbos@chromium.org
 Issue 738918  has been merged into this issue.

Comment 3 by hbos@chromium.org, Jan 29 2018 

 Issue 806769  has been merged into this issue.

Comment 4 by hbos@chromium.org, Jan 31 2018

Blocking: 806875
Project Member

Comment 5 by bugdroid1@chromium.org, Feb 1 2018 

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

commit 55a6f822232bb39ce9296fe9de7baae0df12b244
Author: Henrik Boström <hbos@chromium.org>
Date: Thu Feb 01 14:38:12 2018

Define legacy getLocalStreams() on top of track-based API.

getLocalStreams() is redefined from a list of streams modified by
addStream() and removeStream() to "all streams of all senders".
When using the stream-based APIs, getLocalStreams() behave exactly
like it did before this CL. Because senders are added or removed by
addTrack() and removeTrack(), this CL makes it possible for
spec-compliant APIs to modify the legacy getLocalStreams(), which was
not possible before this CL.

In follow-up CLs addStream() and removeStream() will also be implemented
on top of the track-based APIs and RTCPeerConnectionHandler code will be
removed.

This CL adds LayoutTests for expected behavior of legacy stream-based
APIs when fully implemented on top of track-based APIs. This includes
the first tests for interaction between legacy and non-legacy APIs.

Design doc: https://docs.google.com/document/d/1Obbeg-B4_04twVctHdf7C7vkUQrGlLvOaV0C8J5_-Gs/edit?usp=sharing

Bug:  803021 ,  738929 
Change-Id: Ibab3f8329809437b6058c3350cd11e747054efd2
Reviewed-on: https://chromium-review.googlesource.com/893385
Reviewed-by: Harald Alvestrand <hta@chromium.org>
Commit-Queue: Henrik Boström <hbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#533656}
[modify] https://crrev.com/55a6f822232bb39ce9296fe9de7baae0df12b244/content/renderer/media/rtc_peer_connection_handler.cc
[modify] https://crrev.com/55a6f822232bb39ce9296fe9de7baae0df12b244/third_party/WebKit/LayoutTests/external/wpt/webrtc/RTCPeerConnection-setRemoteDescription-tracks.https-expected.txt
[modify] https://crrev.com/55a6f822232bb39ce9296fe9de7baae0df12b244/third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-AddRemoveTrack.html
[add] https://crrev.com/55a6f822232bb39ce9296fe9de7baae0df12b244/third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-legacy-stream-APIs-expected.txt
[add] https://crrev.com/55a6f822232bb39ce9296fe9de7baae0df12b244/third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-legacy-stream-APIs.html
[modify] https://crrev.com/55a6f822232bb39ce9296fe9de7baae0df12b244/third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp
[modify] https://crrev.com/55a6f822232bb39ce9296fe9de7baae0df12b244/third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.h
[modify] https://crrev.com/55a6f822232bb39ce9296fe9de7baae0df12b244/third_party/WebKit/Source/modules/peerconnection/RTCRtpSender.cpp
[modify] https://crrev.com/55a6f822232bb39ce9296fe9de7baae0df12b244/third_party/WebKit/Source/modules/peerconnection/RTCRtpSender.h

Comment 6 by hbos@chromium.org, Feb 8 2018

Blockedon: 810415

Comment 7 by hbos@chromium.org, Feb 9 2018

Blocking: 810708
Project Member

Comment 8 by bugdroid1@chromium.org, Feb 12 2018 

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

commit 85000e8aea2a9b14b0618cf8351ffbcd5110e123
Author: Henrik Boström <hbos@chromium.org>
Date: Mon Feb 12 10:52:40 2018

Redefine legacy addStream()/removeStream() on top of track-based APIs.

Implementation:
- addStream() is defined in terms of addTrack() and adding a listener
  of tracks being added/removed from the stream. The listener uses
  addTrack()/removeTrack().
- removeStream() is defined in terms of removeTrack() and removing the
  listener of tracks being added/removed from the stream.

This fixes:  https://crbug.com/738929 

Related bug fixes:
- By getting rid of local_streams_, this also fixes the issue where
  createDTMFSender() could only be called for tracks added with the
  legacy APIs.  https://crbug.com/806875 
- By adding code checking if a stream was the first/last stream
  added/removed, the chrome://webrtc-internals events related to
  adding or removing a stream are triggered by addTrack/removeTrack,
  fixing the bug where addTrack-streams did not appear in
  chrome://webrtc-internals.  https://crbug.com/801093 

Old code no longer exercised is removed. MockPeerConnectionImpl is
updated to work with track-based APIs. We should get rid of this class
in future CLs, it's a maintenance burden.

Design doc: https://docs.google.com/document/d/1Obbeg-B4_04twVctHdf7C7vkUQrGlLvOaV0C8J5_-Gs/edit?usp=sharing

Bug:  738929 ,  803021 ,  806875 ,  801093 
Change-Id: I77541a3dce180e1cf8b311cb7803047215b0af7b
Reviewed-on: https://chromium-review.googlesource.com/899346
Commit-Queue: Henrik Boström <hbos@chromium.org>
Reviewed-by: Mike West <mkwst@chromium.org>
Reviewed-by: Harald Alvestrand <hta@chromium.org>
Cr-Commit-Position: refs/heads/master@{#536052}
[modify] https://crrev.com/85000e8aea2a9b14b0618cf8351ffbcd5110e123/content/renderer/media/webrtc/mock_peer_connection_impl.cc
[modify] https://crrev.com/85000e8aea2a9b14b0618cf8351ffbcd5110e123/content/renderer/media/webrtc/mock_peer_connection_impl.h
[modify] https://crrev.com/85000e8aea2a9b14b0618cf8351ffbcd5110e123/content/renderer/media/webrtc/rtc_peer_connection_handler.cc
[modify] https://crrev.com/85000e8aea2a9b14b0618cf8351ffbcd5110e123/content/renderer/media/webrtc/rtc_peer_connection_handler.h
[modify] https://crrev.com/85000e8aea2a9b14b0618cf8351ffbcd5110e123/content/renderer/media/webrtc/rtc_peer_connection_handler_unittest.cc
[modify] https://crrev.com/85000e8aea2a9b14b0618cf8351ffbcd5110e123/content/renderer/media/webrtc/rtc_rtp_sender.cc
[modify] https://crrev.com/85000e8aea2a9b14b0618cf8351ffbcd5110e123/content/renderer/media/webrtc/rtc_rtp_sender.h
[modify] https://crrev.com/85000e8aea2a9b14b0618cf8351ffbcd5110e123/third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-AddRemoveTrack.html
[delete] https://crrev.com/b7e3a91cb94a9f47a8ebbef7a887d31069c18aac/third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-legacy-stream-APIs-expected.txt
[modify] https://crrev.com/85000e8aea2a9b14b0618cf8351ffbcd5110e123/third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp
[modify] https://crrev.com/85000e8aea2a9b14b0618cf8351ffbcd5110e123/third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.h
[modify] https://crrev.com/85000e8aea2a9b14b0618cf8351ffbcd5110e123/third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.idl
[modify] https://crrev.com/85000e8aea2a9b14b0618cf8351ffbcd5110e123/third_party/WebKit/Source/platform/testing/TestingPlatformSupportWithWebRTC.cpp
[modify] https://crrev.com/85000e8aea2a9b14b0618cf8351ffbcd5110e123/third_party/WebKit/Source/platform/testing/TestingPlatformSupportWithWebRTC.h
[modify] https://crrev.com/85000e8aea2a9b14b0618cf8351ffbcd5110e123/third_party/WebKit/public/platform/WebRTCPeerConnectionHandler.h

Comment 9 by hbos@chromium.org, Feb 12 2018

Description: Show this description
Project Member

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

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

commit dc9e81d247022350573a814c4e032c6637ca28a6
Author: Henrik Boström <hbos@chromium.org>
Date: Thu Feb 15 16:08:30 2018

Predictable rtp_senders_ order with std::vector and removal of old code.

RTCPeerConnection::rtp_senders_ is turned into a vector, which
simplifies getSenders() to "return rtp_senders_;" and removes the need
query lower layers.

RTCPeerConnectionHandler::GetSenders() is removed and its rtp_senders_
is also changed from a map to a vector.

Bug:  803021 
Change-Id: I87316042436d6644552030e67827b55fab046b58
Reviewed-on: https://chromium-review.googlesource.com/909110
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Harald Alvestrand <hta@chromium.org>
Commit-Queue: Henrik Boström <hbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537032}
[modify] https://crrev.com/dc9e81d247022350573a814c4e032c6637ca28a6/content/renderer/media/webrtc/rtc_peer_connection_handler.cc
[modify] https://crrev.com/dc9e81d247022350573a814c4e032c6637ca28a6/content/renderer/media/webrtc/rtc_peer_connection_handler.h
[modify] https://crrev.com/dc9e81d247022350573a814c4e032c6637ca28a6/content/renderer/media/webrtc/rtc_peer_connection_handler_unittest.cc
[modify] https://crrev.com/dc9e81d247022350573a814c4e032c6637ca28a6/third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp
[modify] https://crrev.com/dc9e81d247022350573a814c4e032c6637ca28a6/third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.h
[modify] https://crrev.com/dc9e81d247022350573a814c4e032c6637ca28a6/third_party/WebKit/Source/platform/testing/TestingPlatformSupportWithWebRTC.cpp
[modify] https://crrev.com/dc9e81d247022350573a814c4e032c6637ca28a6/third_party/WebKit/Source/platform/testing/TestingPlatformSupportWithWebRTC.h
[modify] https://crrev.com/dc9e81d247022350573a814c4e032c6637ca28a6/third_party/WebKit/public/platform/WebRTCPeerConnectionHandler.h

Comment 11 by hbos@chromium.org, Feb 15 2018

Labels: -Pri-3 Pri-2
With the above CL we have getLocalStreams(), addStream(), removeStream() and removed old code.

Remaining: getRemoteStreams() on top of getReceivers().

Comment 12 by hbos@chromium.org, Mar 14 2018

Status: Verified (was: Assigned)
getRemoteStreams() was already fixed quite some time ago, I forgot about that.
►

Sign in to add a comment