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

Issue 2794943002: Delete MediaController class, move Call ownership to PeerConnection. (Closed)

Created:
3 years, 8 months ago by nisse-webrtc
Modified:
3 years, 7 months ago
CC:
webrtc-reviews_webrtc.org, yujie_mao (webrtc), tterriberry_mozilla.com, qiang.lu, niklas.enbom, peah-webrtc
Target Ref:
refs/heads/master
Project:
webrtc
Visibility:
Public.

Description

Delete MediaController class, move Call ownership to PeerConnection. BUG=webrtc:7135 Review-Url: https://codereview.webrtc.org/2794943002 Cr-Commit-Position: refs/heads/master@{#18026} Committed: https://chromium.googlesource.com/external/webrtc/+/eaabdf6259ccd9f93f754c5ffcce9a7d189f5b93

Patch Set 1 #

Patch Set 2 : Rebased. #

Patch Set 3 : Enable and fix webrtcsession and peerconnection unittests. #

Patch Set 4 : Update ortc code. #

Patch Set 5 : Re-enable and update rtcstatscollector_unittest.cc. #

Patch Set 6 : Re-enable and update ChannelManager tests. #

Patch Set 7 : Re-enable and update RtpSenderReceiverTest. #

Patch Set 8 : Hack for injecting a FakeCall, and re-enable TestPacketOptionsAndOnPacketSent test. #

Total comments: 2

Patch Set 9 : Move Call ownership to PeerConnection, and pass pointer to WebRtcSession constructor. #

Total comments: 2

Patch Set 10 : Added thread affinity DCHECK. #

Patch Set 11 : Revert DCHECK addition. #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+181 lines, -351 lines) Patch
M webrtc/api/BUILD.gn View 1 1 chunk +0 lines, -1 line 0 comments Download
D webrtc/api/mediacontroller.h View 1 chunk +0 lines, -18 lines 0 comments Download
M webrtc/ortc/rtptransportcontrolleradapter.h View 1 2 3 3 chunks +6 lines, -2 lines 0 comments Download
M webrtc/ortc/rtptransportcontrolleradapter.cc View 1 2 3 4 5 6 7 8 6 chunks +41 lines, -13 lines 4 comments Download
M webrtc/pc/BUILD.gn View 1 2 3 4 2 chunks +0 lines, -3 lines 0 comments Download
M webrtc/pc/channelmanager.h View 1 9 chunks +14 lines, -11 lines 0 comments Download
M webrtc/pc/channelmanager.cc View 1 12 chunks +25 lines, -23 lines 0 comments Download
M webrtc/pc/channelmanager_unittest.cc View 1 2 3 4 5 5 chunks +10 lines, -9 lines 0 comments Download
D webrtc/pc/fakemediacontroller.h View 1 chunk +0 lines, -42 lines 0 comments Download
D webrtc/pc/mediacontroller.h View 1 chunk +0 lines, -46 lines 0 comments Download
D webrtc/pc/mediacontroller.cc View 1 chunk +0 lines, -99 lines 0 comments Download
M webrtc/pc/peerconnection.h View 1 2 3 4 5 6 7 8 3 chunks +5 lines, -2 lines 0 comments Download
M webrtc/pc/peerconnection.cc View 1 2 3 4 5 6 7 8 10 4 chunks +32 lines, -4 lines 0 comments Download
M webrtc/pc/peerconnectionfactory.h View 2 chunks +1 line, -4 lines 0 comments Download
M webrtc/pc/peerconnectionfactory.cc View 1 2 chunks +4 lines, -8 lines 0 comments Download
M webrtc/pc/peerconnectioninterface_unittest.cc View 1 2 3 chunks +2 lines, -19 lines 0 comments Download
M webrtc/pc/rtcstatscollector_unittest.cc View 1 2 3 4 2 chunks +1 line, -7 lines 0 comments Download
M webrtc/pc/rtpsenderreceiver_unittest.cc View 1 2 3 4 5 6 5 chunks +4 lines, -6 lines 0 comments Download
M webrtc/pc/statscollector_unittest.cc View 2 chunks +2 lines, -7 lines 0 comments Download
M webrtc/pc/test/mock_webrtcsession.h View 1 2 3 4 5 6 7 8 1 chunk +6 lines, -3 lines 0 comments Download
M webrtc/pc/webrtcsession.h View 1 2 3 4 5 6 7 8 4 chunks +8 lines, -3 lines 0 comments Download
M webrtc/pc/webrtcsession.cc View 1 2 3 4 5 6 7 8 7 chunks +12 lines, -8 lines 0 comments Download
M webrtc/pc/webrtcsession_unittest.cc View 1 2 3 4 5 6 7 8 7 chunks +8 lines, -13 lines 0 comments Download

Messages

Total messages: 37 (7 generated)
nisse-webrtc
Request for comments. This is a first crude attempt to delete the MediaController class, moving ...
3 years, 8 months ago (2017-04-03 14:51:01 UTC) #2
the sun
On 2017/04/03 14:51:01, nisse-webrtc wrote: > Request for comments. > > This is a first ...
3 years, 8 months ago (2017-04-03 14:59:46 UTC) #3
pthatcher1
On 2017/04/03 14:59:46, the sun wrote: > On 2017/04/03 14:51:01, nisse-webrtc wrote: > > Request ...
3 years, 8 months ago (2017-04-04 00:56:50 UTC) #4
nisse-webrtc
On 2017/04/03 14:59:46, the sun wrote: > I think the idea with MediaController was that ...
3 years, 8 months ago (2017-04-04 08:40:28 UTC) #5
nisse-webrtc
On 2017/04/04 00:56:50, pthatcher1 wrote: > One key thing is that I remember that we ...
3 years, 8 months ago (2017-04-04 09:01:26 UTC) #6
nisse-webrtc
Taylor, do you have any advice? I think I have to put this cl on ...
3 years, 8 months ago (2017-04-04 09:03:44 UTC) #8
the sun
On 2017/04/04 09:01:26, nisse-webrtc wrote: > On 2017/04/04 00:56:50, pthatcher1 wrote: > > One key ...
3 years, 8 months ago (2017-04-04 21:18:01 UTC) #9
pthatcher1
On 2017/04/04 08:40:28, nisse-webrtc wrote: > On 2017/04/03 14:59:46, the sun wrote: > > I ...
3 years, 8 months ago (2017-04-04 22:58:04 UTC) #10
Taylor Brandstetter
Sorry I'm late to the party. But basically, I'd echo what Peter is saying. PeerConnection ...
3 years, 8 months ago (2017-04-05 03:34:35 UTC) #11
stefan-webrtc
On 2017/04/04 22:58:04, pthatcher1 wrote: > On 2017/04/04 08:40:28, nisse-webrtc wrote: > > On 2017/04/03 ...
3 years, 8 months ago (2017-04-05 07:26:01 UTC) #12
nisse-webrtc
Maybe I'm still missing some parts of the big picture. One of the things I'd ...
3 years, 7 months ago (2017-04-25 08:41:50 UTC) #13
Taylor Brandstetter
If it would make things easier for you, I don't have an objection to deleting ...
3 years, 7 months ago (2017-04-26 21:40:26 UTC) #14
nisse-webrtc
On 2017/04/26 21:40:26, Taylor Brandstetter wrote: > If it would make things easier for you, ...
3 years, 7 months ago (2017-04-27 06:56:36 UTC) #15
nisse-webrtc
This now seems to pass the tests. Can we move forward? https://codereview.webrtc.org/2794943002/diff/140001/webrtc/pc/webrtcsession.h File webrtc/pc/webrtcsession.h (right): ...
3 years, 7 months ago (2017-04-27 13:46:34 UTC) #16
pthatcher1
On 2017/04/26 21:40:26, Taylor Brandstetter wrote: > If it would make things easier for you, ...
3 years, 7 months ago (2017-04-29 00:27:23 UTC) #17
pthatcher1
On 2017/04/05 07:26:01, stefan-webrtc wrote: > On 2017/04/04 22:58:04, pthatcher1 wrote: > > On 2017/04/04 ...
3 years, 7 months ago (2017-04-29 00:31:51 UTC) #18
Taylor Brandstetter
lgtm https://codereview.webrtc.org/2794943002/diff/140001/webrtc/pc/webrtcsession.h File webrtc/pc/webrtcsession.h (right): https://codereview.webrtc.org/2794943002/diff/140001/webrtc/pc/webrtcsession.h#newcode369 webrtc/pc/webrtcsession.h:369: virtual Call* call() { return call_.get(); }; On ...
3 years, 7 months ago (2017-04-30 07:21:29 UTC) #19
nisse-webrtc
On 2017/04/29 00:31:51, pthatcher1 wrote: > If RTP code is removed from it, that RTP ...
3 years, 7 months ago (2017-05-02 06:44:51 UTC) #20
nisse-webrtc
On 2017/04/30 07:21:29, Taylor Brandstetter wrote: > I think it would make more sense to ...
3 years, 7 months ago (2017-05-02 13:37:38 UTC) #21
nisse-webrtc
On 2017/05/02 13:37:38, nisse-webrtc wrote: > Ok, I'm trying this, moving ownership up to PeerConnection. ...
3 years, 7 months ago (2017-05-02 14:11:33 UTC) #22
the sun
On 2017/05/02 14:11:33, nisse-webrtc wrote: > On 2017/05/02 13:37:38, nisse-webrtc wrote: > > > Ok, ...
3 years, 7 months ago (2017-05-02 20:30:07 UTC) #23
the sun
https://codereview.webrtc.org/2794943002/diff/160001/webrtc/pc/peerconnection.cc File webrtc/pc/peerconnection.cc (right): https://codereview.webrtc.org/2794943002/diff/160001/webrtc/pc/peerconnection.cc#newcode2328 webrtc/pc/peerconnection.cc:2328: void PeerConnection::CreateCall_w() { DCHECK thread affinity?
3 years, 7 months ago (2017-05-02 20:30:20 UTC) #24
nisse-webrtc
Added DCHECK, updated description, and linked to the Pluggable RTP Transport tracking bug. https://codereview.webrtc.org/2794943002/diff/160001/webrtc/pc/peerconnection.cc File ...
3 years, 7 months ago (2017-05-04 09:24:01 UTC) #26
nisse-webrtc
On 2017/05/04 09:24:01, nisse-webrtc wrote: > On 2017/05/02 20:30:20, the sun wrote: > > DCHECK ...
3 years, 7 months ago (2017-05-04 09:39:47 UTC) #27
stefan-webrtc
https://codereview.webrtc.org/2794943002/diff/200001/webrtc/ortc/rtptransportcontrolleradapter.cc File webrtc/ortc/rtptransportcontrolleradapter.cc (right): https://codereview.webrtc.org/2794943002/diff/200001/webrtc/ortc/rtptransportcontrolleradapter.cc#newcode614 webrtc/ortc/rtptransportcontrolleradapter.cc:614: // to be in MediaController). I think this TODO ...
3 years, 7 months ago (2017-05-05 08:47:05 UTC) #28
stefan-webrtc
lgtm https://codereview.webrtc.org/2794943002/diff/200001/webrtc/ortc/rtptransportcontrolleradapter.cc File webrtc/ortc/rtptransportcontrolleradapter.cc (right): https://codereview.webrtc.org/2794943002/diff/200001/webrtc/ortc/rtptransportcontrolleradapter.cc#newcode614 webrtc/ortc/rtptransportcontrolleradapter.cc:614: // to be in MediaController). On 2017/05/05 08:47:05, ...
3 years, 7 months ago (2017-05-05 08:53:50 UTC) #29
nisse-webrtc
https://codereview.webrtc.org/2794943002/diff/200001/webrtc/ortc/rtptransportcontrolleradapter.cc File webrtc/ortc/rtptransportcontrolleradapter.cc (right): https://codereview.webrtc.org/2794943002/diff/200001/webrtc/ortc/rtptransportcontrolleradapter.cc#newcode614 webrtc/ortc/rtptransportcontrolleradapter.cc:614: // to be in MediaController). On 2017/05/05 08:53:50, stefan-webrtc ...
3 years, 7 months ago (2017-05-05 09:02:37 UTC) #30
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2794943002/200001
3 years, 7 months ago (2017-05-05 09:03:29 UTC) #33
commit-bot: I haz the power
Committed patchset #11 (id:200001) as https://chromium.googlesource.com/external/webrtc/+/eaabdf6259ccd9f93f754c5ffcce9a7d189f5b93
3 years, 7 months ago (2017-05-05 09:23:08 UTC) #36
Taylor Brandstetter
3 years, 7 months ago (2017-05-05 16:32:01 UTC) #37
Message was sent while issue was closed.
https://codereview.webrtc.org/2794943002/diff/200001/webrtc/ortc/rtptransport...
File webrtc/ortc/rtptransportcontrolleradapter.cc (right):

https://codereview.webrtc.org/2794943002/diff/200001/webrtc/ortc/rtptransport...
webrtc/ortc/rtptransportcontrolleradapter.cc:614: // to be in MediaController).
On 2017/05/05 09:02:37, nisse-webrtc wrote:
> On 2017/05/05 08:53:50, stefan-webrtc wrote:
> > On 2017/05/05 08:47:05, stefan-webrtc wrote:
> > > I think this TODO should say when these duplicates can be removed, and
> perhaps
> > > why they're needed?
> > 
> > Talked offline, and duplicates are needed since this is an ORTC object and
> isn't
> > used by PC. 
> 
> I'm afraid I'm not at all familiar with the new ortc-related code. Taylor, do
> you see any good way to reduce duplication?
> 
> One possibility might be make these bandwidth values the default values in the
> definition of the config struct? (For a followup, I'll now try to get this cl
> landed).

I wouldn't worry about reducing this duplication. Eventually this whole class
will be deleted and it will naturally go away; see the comment in the header
file. Speaking of which, that comment should be updated to remove "wraps a
MediaController".

Powered by Google App Engine
This is Rietveld 408576698