Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(111)

Issue 2759953003: Interface RTCRtpReceiver and RTCPeerConnection.getReceivers() added. (Closed)

Created:
3 years, 9 months ago by hbos_chromium
Modified:
3 years, 8 months ago
CC:
blink-reviews, blink-reviews-api_chromium.org, chfremer+watch_chromium.org, chromium-reviews, darin-cc_chromium.org, Taylor_Brandstetter, dglazkov+blink, feature-media-reviews_chromium.org, haraken, jam, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, Solis
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Interface RTCRtpReceiver and RTCPeerConnection.getReceivers() added. This CL adds RTCPeerConnection.getReceivers() and a partial implementation of the RTCRtpReceiver interface, only containing the track member. Behind experimental RuntimeEnabled feature "RTCRtpReceiver". getReceivers() spec: https://w3c.github.io/webrtc-pc/#dom-rtcpeerconnection-getreceivers RTCRtpReceiver spec: https://w3c.github.io/webrtc-pc/#rtcrtpreceiver-interface blink layer: - javascript object RTCRtpReceiver.[idl/h/cpp] added. - interface WebRTCRtpReceiver.[h/cpp] added. - interface WebRTCPeerConnectionHandler::getReceivers() added. content layer: - RTCRtpReceiver.[h/cc] added (implementation of WebRTCRtpReceiver). - RTCPeerConnectionHandler::getReceivers() added (implementation of WebRTCPeerConnectionHandler implementation). All remote tracks have receivers. When receivers are surfaced we find which upper-layer track to reference by iterating remote tracks. New receivers are created on getReceivers(), inactive receivers are removed on didRemoveRemoteStream(). We should be able to create receivers at the same time that tracks are created but on didAddRemoteStream() they are not fully initialized which creates problems. I left a TODO, we should use events for when tracks are being added/removed, not streams, but that is a separate issue. (The spec is moving away from streams in favor of addTrack/removeTrack.) Testing: - rtc_peer_connection_handler_unittest.cc: Tests the RTCPeerConnectionHandler::getReceivers implementation. - RTCPeerConnection-getReceivers.html: A LayoutTest testing adding and removing streams and that receivers match. Uses a MockRTCPeerConnectionHandler to add and remove streams (fake call). - webrtc_rtp_browsertest.cc / peerconnection_rtp.js: Sets up a call between two tabs and verifies remote tracks and receivers match, tests the full stack in a real browser. BUG=chromium:703122 Review-Url: https://codereview.chromium.org/2759953003 Cr-Commit-Position: refs/heads/master@{#460793} Committed: https://chromium.googlesource.com/chromium/src/+/cc4863010c8c9b706274a1eeed803d5a3f8250ca

Patch Set 1 #

Patch Set 2 : removeInactiveReceivers in didRemoveRemoteStream #

Total comments: 8

Patch Set 3 : Addressed/added comments #

Total comments: 14

Patch Set 4 : Addressed guidou's comments #

Patch Set 5 : Rebase with master #

Total comments: 27

Patch Set 6 : Addressed foolip's comments #

Patch Set 7 : Rebase with master and addressed nit #

Total comments: 2

Patch Set 8 : DISALLOW_COPY_AND_ASSIGN #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1006 lines, -6 lines) Patch
M chrome/browser/media/webrtc/webrtc_browsertest_base.h View 2 chunks +6 lines, -0 lines 0 comments Download
M chrome/browser/media/webrtc/webrtc_browsertest_base.cc View 1 chunk +18 lines, -0 lines 0 comments Download
A chrome/browser/media/webrtc/webrtc_rtp_browsertest.cc View 1 chunk +51 lines, -0 lines 0 comments Download
M chrome/test/BUILD.gn View 1 2 3 4 2 chunks +2 lines, -0 lines 0 comments Download
A chrome/test/data/webrtc/peerconnection_rtp.js View 1 chunk +106 lines, -0 lines 0 comments Download
M chrome/test/data/webrtc/webrtc_jsep01_test.html View 1 chunk +1 line, -0 lines 0 comments Download
M content/renderer/BUILD.gn View 1 2 3 4 5 6 1 chunk +2 lines, -0 lines 0 comments Download
M content/renderer/media/mock_peer_connection_impl.h View 1 chunk +2 lines, -0 lines 0 comments Download
M content/renderer/media/mock_peer_connection_impl.cc View 3 chunks +54 lines, -0 lines 0 comments Download
M content/renderer/media/rtc_peer_connection_handler.h View 2 chunks +9 lines, -0 lines 0 comments Download
M content/renderer/media/rtc_peer_connection_handler.cc View 1 2 3 4 5 6 4 chunks +76 lines, -0 lines 0 comments Download
M content/renderer/media/rtc_peer_connection_handler_unittest.cc View 1 2 3 4 3 chunks +53 lines, -0 lines 0 comments Download
A content/renderer/media/webrtc/rtc_rtp_receiver.h View 1 2 3 4 5 6 7 1 chunk +43 lines, -0 lines 0 comments Download
A content/renderer/media/webrtc/rtc_rtp_receiver.cc View 1 chunk +54 lines, -0 lines 0 comments Download
M content/shell/test_runner/mock_webrtc_peer_connection_handler.h View 2 chunks +3 lines, -0 lines 0 comments Download
M content/shell/test_runner/mock_webrtc_peer_connection_handler.cc View 1 2 3 4 4 chunks +58 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/external/wpt/webrtc/rtcpeerconnection/rtcpeerconnection-idl-expected.txt View 3 chunks +3 lines, -3 lines 0 comments Download
A third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-getReceivers.html View 1 2 3 4 5 1 chunk +216 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/webexposed/global-interface-listing-expected.txt View 1 2 3 4 3 chunks +6 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/modules_idl_files.gni View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/peerconnection/BUILD.gn View 1 2 3 4 1 chunk +2 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.h View 1 2 3 5 chunks +13 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp View 1 2 3 4 5 6 6 chunks +53 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.idl View 1 1 chunk +3 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/modules/peerconnection/RTCRtpReceiver.h View 1 2 3 4 5 1 chunk +37 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/modules/peerconnection/RTCRtpReceiver.cpp View 1 2 3 4 5 1 chunk +27 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/modules/peerconnection/RTCRtpReceiver.idl View 1 chunk +12 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/platform/BUILD.gn View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5 View 1 2 3 4 5 6 1 chunk +4 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/platform/exported/WebMediaStream.cpp View 2 chunks +30 lines, -3 lines 0 comments Download
A third_party/WebKit/Source/platform/exported/WebRTCRtpReceiver.cpp View 1 chunk +11 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/web/tests/ActiveConnectionThrottlingTest.cpp View 2 chunks +5 lines, -0 lines 0 comments Download
M third_party/WebKit/public/BUILD.gn View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/public/platform/WebMediaStream.h View 1 chunk +6 lines, -0 lines 0 comments Download
M third_party/WebKit/public/platform/WebRTCPeerConnectionHandler.h View 1 2 3 chunks +6 lines, -0 lines 0 comments Download
A third_party/WebKit/public/platform/WebRTCRtpReceiver.h View 1 2 1 chunk +31 lines, -0 lines 0 comments Download

Messages

Total messages: 70 (53 generated)
hbos_chromium
Please take a look, guidou, foolip and pfeldman.
3 years, 9 months ago (2017-03-24 16:35:02 UTC) #34
Taylor_Brandstetter
https://codereview.chromium.org/2759953003/diff/160001/content/renderer/media/rtc_peer_connection_handler.cc File content/renderer/media/rtc_peer_connection_handler.cc (right): https://codereview.chromium.org/2759953003/diff/160001/content/renderer/media/rtc_peer_connection_handler.cc#newcode1652 content/renderer/media/rtc_peer_connection_handler.cc:1652: web_receivers[i].reset(new RTCRtpReceiver( If I understand correctly: this method will ...
3 years, 9 months ago (2017-03-24 18:13:07 UTC) #38
hbos_chromium
PTAL deadbeef and guidou. https://codereview.chromium.org/2759953003/diff/160001/content/renderer/media/rtc_peer_connection_handler.cc File content/renderer/media/rtc_peer_connection_handler.cc (right): https://codereview.chromium.org/2759953003/diff/160001/content/renderer/media/rtc_peer_connection_handler.cc#newcode1652 content/renderer/media/rtc_peer_connection_handler.cc:1652: web_receivers[i].reset(new RTCRtpReceiver( On 2017/03/24 18:13:07, ...
3 years, 9 months ago (2017-03-27 14:55:19 UTC) #39
Guido Urdaneta
https://codereview.chromium.org/2759953003/diff/180001/content/renderer/media/rtc_peer_connection_handler.cc File content/renderer/media/rtc_peer_connection_handler.cc (right): https://codereview.chromium.org/2759953003/diff/180001/content/renderer/media/rtc_peer_connection_handler.cc#newcode1661 content/renderer/media/rtc_peer_connection_handler.cc:1661: // When that is addressed, DCHECK that the track ...
3 years, 9 months ago (2017-03-27 17:12:24 UTC) #40
Taylor_Brandstetter
https://codereview.chromium.org/2759953003/diff/160001/content/renderer/media/rtc_peer_connection_handler.cc File content/renderer/media/rtc_peer_connection_handler.cc (right): https://codereview.chromium.org/2759953003/diff/160001/content/renderer/media/rtc_peer_connection_handler.cc#newcode2026 content/renderer/media/rtc_peer_connection_handler.cc:2026: for (const auto& remote_stream_pair : remote_streams_) { On 2017/03/27 ...
3 years, 9 months ago (2017-03-27 21:39:02 UTC) #41
hbos_chromium
PTAL guidou https://codereview.chromium.org/2759953003/diff/180001/content/renderer/media/rtc_peer_connection_handler.cc File content/renderer/media/rtc_peer_connection_handler.cc (right): https://codereview.chromium.org/2759953003/diff/180001/content/renderer/media/rtc_peer_connection_handler.cc#newcode1661 content/renderer/media/rtc_peer_connection_handler.cc:1661: // When that is addressed, DCHECK that ...
3 years, 8 months ago (2017-03-28 10:12:55 UTC) #44
Guido Urdaneta
lgtm % nit, but wait for foolip's review of Layout tests and blink-specific details. https://codereview.chromium.org/2759953003/diff/180001/content/renderer/media/rtc_peer_connection_handler.cc ...
3 years, 8 months ago (2017-03-28 14:27:03 UTC) #51
hbos_chromium
PTAL foolip and optionally deadbeef.
3 years, 8 months ago (2017-03-28 14:36:49 UTC) #52
foolip
https://codereview.chromium.org/2759953003/diff/220001/third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-getReceivers.html File third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-getReceivers.html (right): https://codereview.chromium.org/2759953003/diff/220001/third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-getReceivers.html#newcode4 third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-getReceivers.html:4: <title>RTCPeerConnection.getReceivers</title> Ideally a test like this would go directly ...
3 years, 8 months ago (2017-03-29 10:07:59 UTC) #53
hbos_chromium
PTAL pfeldman for 3 files: content/renderer/BUILD.gn content/shell/test_runner/mock_webrtc_peer_connection_handler.cc content/shell/test_runner/mock_webrtc_peer_connection_handler.h And PTAL foolip. https://codereview.chromium.org/2759953003/diff/220001/third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-getReceivers.html File third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-getReceivers.html (right): ...
3 years, 8 months ago (2017-03-29 13:50:13 UTC) #54
foolip
lgtm https://codereview.chromium.org/2759953003/diff/220001/third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-getReceivers.html File third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-getReceivers.html (right): https://codereview.chromium.org/2759953003/diff/220001/third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-getReceivers.html#newcode4 third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-getReceivers.html:4: <title>RTCPeerConnection.getReceivers</title> On 2017/03/29 13:50:13, hbos_chromium wrote: > On ...
3 years, 8 months ago (2017-03-29 14:17:48 UTC) #55
hbos_chromium
PTAL pfeldman for 3 files: content/renderer/BUILD.gn content/shell/test_runner/mock_webrtc_peer_connection_handler.cc content/shell/test_runner/mock_webrtc_peer_connection_handler.h https://codereview.chromium.org/2759953003/diff/180001/content/renderer/media/rtc_peer_connection_handler.cc File content/renderer/media/rtc_peer_connection_handler.cc (right): https://codereview.chromium.org/2759953003/diff/180001/content/renderer/media/rtc_peer_connection_handler.cc#newcode1663 content/renderer/media/rtc_peer_connection_handler.cc:1663: web_receivers.push_back(std::unique_ptr<blink::WebRTCRtpReceiver>( ...
3 years, 8 months ago (2017-03-29 14:36:42 UTC) #58
hbos_chromium
No response from pfeldman, I'll try jochen. Can you take a look at these? content/renderer/BUILD.gn ...
3 years, 8 months ago (2017-03-30 08:19:33 UTC) #62
jochen (gone - plz use gerrit)
lgtm https://codereview.chromium.org/2759953003/diff/260001/content/renderer/media/webrtc/rtc_rtp_receiver.h File content/renderer/media/webrtc/rtc_rtp_receiver.h (right): https://codereview.chromium.org/2759953003/diff/260001/content/renderer/media/webrtc/rtc_rtp_receiver.h#newcode36 content/renderer/media/webrtc/rtc_rtp_receiver.h:36: }; disallow copy/assign
3 years, 8 months ago (2017-03-30 11:53:38 UTC) #63
hbos_chromium
Thanks! :) https://codereview.chromium.org/2759953003/diff/260001/content/renderer/media/webrtc/rtc_rtp_receiver.h File content/renderer/media/webrtc/rtc_rtp_receiver.h (right): https://codereview.chromium.org/2759953003/diff/260001/content/renderer/media/webrtc/rtc_rtp_receiver.h#newcode36 content/renderer/media/webrtc/rtc_rtp_receiver.h:36: }; On 2017/03/30 11:53:38, jochen (slow until ...
3 years, 8 months ago (2017-03-30 12:04:32 UTC) #64
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2759953003/280001
3 years, 8 months ago (2017-03-30 12:05:02 UTC) #67
commit-bot: I haz the power
3 years, 8 months ago (2017-03-30 16:48:34 UTC) #70
Message was sent while issue was closed.
Committed patchset #8 (id:280001) as
https://chromium.googlesource.com/chromium/src/+/cc4863010c8c9b706274a1eeed80...

Powered by Google App Engine
This is Rietveld 408576698