Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(7)

Issue 1393563002: Moving MediaStreamSignaling logic into PeerConnection. (Closed)

Created:
3 years, 8 months ago by Taylor Brandstetter
Modified:
3 years, 8 months ago
Reviewers:
pthatcher1
CC:
webrtc-reviews_webrtc.org, interface-changes_webrtc.org, rwolff_gocast.it, yujie_mao (webrtc), Andrew MacDonald, tterriberry_mozilla.com, qiang.lu, niklas.enbom, peah-webrtc
Base URL:
https://chromium.googlesource.com/external/webrtc.git@master
Target Ref:
refs/pending/heads/master
Project:
webrtc
Visibility:
Public.

Description

Moving MediaStreamSignaling logic into PeerConnection. This needs to happen because in the future, m-lines will be offered based on the set of RtpSenders/RtpReceivers, rather than the set of tracks that MediaStreamSignaling knows about. Besides that, MediaStreamSignaling was a "glue class" without a clearly defined role, so it going away is good for other reasons as well. Committed: https://crrev.com/97c392935411398b506861601c82e31d95c591f0 Cr-Commit-Position: refs/heads/master@{#10268}

Patch Set 1 #

Patch Set 2 : Cleaning up comments, fixing naming, etc. #

Total comments: 35

Patch Set 3 : Fixing patch conflicts. #

Patch Set 4 : Fixing Mac compile error. #

Patch Set 5 : Fixing ASan warnings. #

Total comments: 2

Patch Set 6 : Fixing naming and adding comments. #

Total comments: 1

Patch Set 7 : Adding another comment and doing git cl format. #

Patch Set 8 : Fixing patch conflicts (uint32 going to uint32_t) #

Patch Set 9 : Temporarily disabling one of the new tests. Will fix in future CL. #

Patch Set 10 : Fixing copyright header #

Unified diffs Side-by-side diffs Delta from patch set Stats (+2996 lines, -3324 lines) Patch
M talk/app/webrtc/datachannel.h View 1 2 3 4 5 6 7 7 chunks +32 lines, -13 lines 0 comments Download
M talk/app/webrtc/datachannel.cc View 1 2 3 4 5 6 7 7 chunks +57 lines, -9 lines 0 comments Download
M talk/app/webrtc/datachannel_unittest.cc View 1 2 3 4 5 6 7 2 chunks +73 lines, -0 lines 0 comments Download
M talk/app/webrtc/mediastreamsignaling.h View 1 2 3 4 5 6 7 1 chunk +1 line, -376 lines 0 comments Download
M talk/app/webrtc/mediastreamsignaling.cc View 1 2 3 4 5 6 7 1 chunk +1 line, -997 lines 0 comments Download
D talk/app/webrtc/mediastreamsignaling_unittest.cc View 1 2 3 4 5 6 7 1 chunk +0 lines, -1341 lines 0 comments Download
M talk/app/webrtc/peerconnection.h View 1 2 3 4 5 6 7 7 chunks +221 lines, -34 lines 0 comments Download
M talk/app/webrtc/peerconnection.cc View 1 2 3 4 5 6 7 26 chunks +1044 lines, -104 lines 0 comments Download
M talk/app/webrtc/peerconnectionendtoend_unittest.cc View 1 2 3 4 5 6 7 8 1 chunk +32 lines, -0 lines 0 comments Download
M talk/app/webrtc/peerconnectionfactory.cc View 1 chunk +1 line, -0 lines 0 comments Download
M talk/app/webrtc/peerconnectioninterface_unittest.cc View 1 2 3 4 5 6 7 23 chunks +1002 lines, -20 lines 0 comments Download
M talk/app/webrtc/sctputils.h View 1 chunk +3 lines, -0 lines 0 comments Download
M talk/app/webrtc/sctputils.cc View 1 2 3 4 5 6 7 1 chunk +13 lines, -0 lines 0 comments Download
M talk/app/webrtc/sctputils_unittest.cc View 1 2 3 4 5 6 7 8 9 2 chunks +18 lines, -1 line 0 comments Download
M talk/app/webrtc/statscollector.h View 1 2 3 4 5 6 7 3 chunks +6 lines, -4 lines 0 comments Download
M talk/app/webrtc/statscollector.cc View 1 2 3 4 5 6 7 22 chunks +48 lines, -48 lines 0 comments Download
M talk/app/webrtc/statscollector_unittest.cc View 1 2 3 4 5 6 7 32 chunks +67 lines, -58 lines 0 comments Download
M talk/app/webrtc/webrtcsession.h View 1 2 3 4 5 6 7 8 chunks +19 lines, -19 lines 0 comments Download
M talk/app/webrtc/webrtcsession.cc View 1 2 3 4 5 6 7 20 chunks +42 lines, -86 lines 0 comments Download
M talk/app/webrtc/webrtcsession_unittest.cc View 1 2 3 4 5 6 7 115 chunks +285 lines, -135 lines 0 comments Download
M talk/app/webrtc/webrtcsessiondescriptionfactory.h View 1 2 3 4 5 6 7 6 chunks +8 lines, -18 lines 0 comments Download
M talk/app/webrtc/webrtcsessiondescriptionfactory.cc View 1 2 3 4 5 6 7 11 chunks +23 lines, -60 lines 0 comments Download
M talk/libjingle_tests.gyp View 1 chunk +0 lines, -1 line 0 comments Download

Messages

Total messages: 18 (6 generated)
Taylor Brandstetter
This may seem large, but 90% of it is just code being moved around. There ...
3 years, 8 months ago (2015-10-07 00:26:19 UTC) #2
pthatcher1
I need to look at peerconnection.cc and .h (the big ones :), but I reviewed ...
3 years, 8 months ago (2015-10-07 02:50:52 UTC) #3
pthatcher1
OK, I've read through everything now. https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/peerconnection.cc File talk/app/webrtc/peerconnection.cc (right): https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/peerconnection.cc#newcode1542 talk/app/webrtc/peerconnection.cc:1542: void PeerConnection::RejectRemoteTracks(cricket::MediaType media_type) ...
3 years, 8 months ago (2015-10-08 05:23:54 UTC) #4
pthatcher1
https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/statscollector.h File talk/app/webrtc/statscollector.h (right): https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/statscollector.h#newcode64 talk/app/webrtc/statscollector.h:64: explicit StatsCollector(PeerConnection* pc, WebRtcSession* session); On 2015/10/07 02:50:51, pthatcher1 ...
3 years, 8 months ago (2015-10-08 22:05:36 UTC) #5
Taylor Brandstetter
https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/datachannel.cc File talk/app/webrtc/datachannel.cc (right): https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/datachannel.cc#newcode47 talk/app/webrtc/datachannel.cc:47: bool SctpSidAllocator::AllocateSctpSid(rtc::SSLRole role, int* sid) { On 2015/10/07 02:50:51, ...
3 years, 8 months ago (2015-10-09 19:54:09 UTC) #6
pthatcher1
lgtm https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/peerconnection.cc File talk/app/webrtc/peerconnection.cc (right): https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/peerconnection.cc#newcode1857 talk/app/webrtc/peerconnection.cc:1857: kv.second->OnDataEngineClose(); On 2015/10/09 19:54:09, Taylor Brandstetter wrote: > ...
3 years, 8 months ago (2015-10-09 21:12:26 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1393563002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1393563002/160001
3 years, 8 months ago (2015-10-13 17:20:46 UTC) #10
commit-bot: I haz the power
Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/1209)
3 years, 8 months ago (2015-10-13 17:23:09 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1393563002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1393563002/180001
3 years, 8 months ago (2015-10-13 20:05:18 UTC) #15
commit-bot: I haz the power
Committed patchset #10 (id:180001)
3 years, 8 months ago (2015-10-13 20:23:46 UTC) #16
commit-bot: I haz the power
Patchset 10 (id:??) landed as https://crrev.com/97c392935411398b506861601c82e31d95c591f0 Cr-Commit-Position: refs/heads/master@{#10268}
3 years, 8 months ago (2015-10-13 20:23:55 UTC) #17
Taylor Brandstetter
3 years, 8 months ago (2015-10-13 23:41:50 UTC) #18
Message was sent while issue was closed.
A revert of this CL (patchset #10 id:180001) has been created in
https://codereview.webrtc.org/1403633005/ by deadbeef@webrtc.org.

The reason for reverting is: Broke browser_tests on Mac. Still need to
investigate the cause..

Powered by Google App Engine
This is Rietveld 408576698