New issue
Advanced search Search tips

Issue 760107 link

Starred by 6 users

Issue metadata

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


Sign in to add a comment

Unflag addTrack, removeTrack, ontrack and RTCRtpSender with track

Project Member Reported by hbos@chromium.org, Aug 29 2017

Issue description

Implement enough of the RTCRtpSender API that unflagging makes sense and is safe to do.

This should include...
- RTCRtpSender with track attribute
- getSenders()
- addTrack()
- removeTrack()
- Firing the associated events

Also make sure relevant adapter.js expectations pass so that we don't end up with something broken and un-shimmable when unflagging.

Spec:
- RTCRtpSender https://w3c.github.io/webrtc-pc/#rtcrtpsender-interface
- RTCPeerConnection extensions https://w3c.github.io/webrtc-pc/#rtcpeerconnection-interface-extensions
 
with the adapter.js covering the grey (brown?) area of the interactions between the track API and the streams API that everyone is avoiding to specify ;-)

At least this means reverting https://github.com/webrtc/adapter/pull/654 and passing all relevant tests

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

Description: Show this description

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

Blockedon: webrtc:7933

Comment 4 by hbos@chromium.org, Sep 25 2017

Blockedon: 739104

Comment 5 by hbos@chromium.org, Sep 25 2017

Blockedon: 705901

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

Blockedon: 755166

Comment 7 by hbos@webrtc.org, Sep 27 2017

Blockedon: webrtc:7429

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

Blockedon: 741619

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

Blockedon: 765170

Comment 10 by hbos@webrtc.org, Oct 10 2017

Blockedon: webrtc:7932

Comment 11 by hbos@webrtc.org, Oct 10 2017

Blockedon: webrtc:8377

Comment 12 by hbos@webrtc.org, Oct 10 2017

Blockedon: webrtc:8315

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

Blockedon: 773523
Project Member

Comment 14 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 15 by hbos@chromium.org, Oct 17 2017

Blockedon: 769743

Comment 16 by hbos@chromium.org, Nov 1 2017

Cc: hbos@chromium.org
 Issue 779535  has been merged into this issue.

Comment 17 by hbos@chromium.org, Nov 1 2017

Labels: -M-63 M-64

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

Blockedon: -webrtc:8315 -webrtc:7932 -webrtc:7933 -webrtc:8377
Updating bug relationships, it makes sense to fix a lot of this stuff after unflagging the core behavior.

Removing "blocked on":
- webrtc:7932: "Multiple streams per sender"
  It's OK to throw an exception for now, this was not supported with the legacy
  APIs or adapter.js.
- webrtc:7933: "addTrack with no stream should not result in announcing a
  stream in the SDP"
  This is OK. If  https://crbug.com/webrtc/7429  can be triggered we can throw an
  exception in the no stream case for now.
- chromium:769743: "Creating remote tracks should be the responsibility of RTCPeerConnection"
  This is cleanup work.
- webrtc:8315: "Support updating a receiver's associated set of streams"
  Merged into the next one:
- webrtc:8377: "Support a track moving between streams/receivers"
  Removing and adding a receiver is OK for now and what old APIs do, this is a
  non-blocker. Spec will probably add a new event for this case.

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

Blockedon: -769743

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

Description: Show this description

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

Blockedon: 777999 777777
Project Member

Comment 22 by bugdroid1@chromium.org, Nov 29 2017

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

commit fb265faa85701894a930a1024fba9c03c8f6a859
Author: Henrik Boström <hbos@chromium.org>
Date: Wed Nov 29 16:32:08 2017

Unflag RTCRtpSender.

This ships a significant portion of the RTP Media API
https://w3c.github.io/webrtc-pc/#rtp-media-api

This exposes:
- RTCPeerConnection (existing interface)
  - getSenders()
  - addTrack()
  - removeTrack()
  - ontrack
- RTCRtpSender (new interface)
  - track
  - (not shipping: replaceTrack, transport, rtcpTransport, get/setParameters)
- RTCTrackEvent (new interface)
  - receiver
  - track
  - streams
  - (not shipping: transceiver)

Intent to Implement & Ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/GnlJt54O_EY

Bug:  760107 
Change-Id: I281463ced5716fc305cc119c7da1e34a7be07b96
Reviewed-on: https://chromium-review.googlesource.com/790911
Commit-Queue: Henrik Boström <hbos@chromium.org>
Reviewed-by: Guido Urdaneta <guidou@chromium.org>
Reviewed-by: Philip Jägenstedt <foolip@chromium.org>
Cr-Commit-Position: refs/heads/master@{#520136}
[modify] https://crrev.com/fb265faa85701894a930a1024fba9c03c8f6a859/third_party/WebKit/LayoutTests/virtual/stable/webexposed/global-interface-listing-expected.txt
[modify] https://crrev.com/fb265faa85701894a930a1024fba9c03c8f6a859/third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp
[modify] https://crrev.com/fb265faa85701894a930a1024fba9c03c8f6a859/third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.idl
[modify] https://crrev.com/fb265faa85701894a930a1024fba9c03c8f6a859/third_party/WebKit/Source/modules/peerconnection/RTCRtpSender.idl
[modify] https://crrev.com/fb265faa85701894a930a1024fba9c03c8f6a859/third_party/WebKit/Source/modules/peerconnection/RTCTrackEvent.idl
[modify] https://crrev.com/fb265faa85701894a930a1024fba9c03c8f6a859/third_party/WebKit/Source/platform/runtime_enabled_features.json5

Comment 23 by hbos@chromium.org, Nov 29 2017

Status: Verified (was: Started)

Comment 24 by hbos@chromium.org, Dec 18 2017

Components: Blink>WebRTC>PeerConnection

Comment 25 by hbos@chromium.org, Dec 18 2017

Summary: Unflag addTrack, removeTrack, ontrack and RTCRtpSender with track (was: Unflag RTCRtpSender)

Sign in to add a comment