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

Issue 2709723003: Initial implementation of RtpTransportControllerReceive and related interfaces.

Can't Edit
Can't Publish+Mail
Start Review
Created:
7 months, 3 weeks ago by nisse-webrtc
Modified:
1 month, 1 week ago
CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, mflodman
Target Ref:
refs/heads/master
Project:
webrtc
Visibility:
Public.

Description

Initial implementation of RtpTransportControllerReceive and related interfaces. BUG=webrtc:7135

Patch Set 1 #

Total comments: 22

Patch Set 2 : Rebase. #

Total comments: 8

Patch Set 3 : Adapt Call to use the new RtpTransportReceive class. #

Total comments: 5

Patch Set 4 : Rebased. #

Patch Set 5 : Fix presubmit warnings. #

Patch Set 6 : Use ReceiveSideCongestionController, delete RtpPacketObserverInterface. #

Patch Set 7 : Add back update to the video_receive_ssrcs_ map. #

Patch Set 8 : Fix book-keeping for RTX streams. #

Patch Set 9 : Delete SetReceiveAudioLevelIndicationStatus and EnableReceiveTransportSequenceNumber from tests. #

Patch Set 10 : Rebased. #

Patch Set 11 : Rebased. #

Patch Set 12 : Drop check for MediaType::ANY. Fix receive counters. #

Patch Set 13 : Rebase. #

Patch Set 14 : Make demuxer unaware of media type. Use separate instances for audio and video. #

Patch Set 15 : Rebased. #

Patch Set 16 : git cl format. #

Patch Set 17 : Fix audio. #

Total comments: 15

Patch Set 18 : Address easy comments. #

Patch Set 19 : Rebase. #

Patch Set 20 : Simplify error handling. Fix flexfec test, streams must be created in the right order. #

Patch Set 21 : Delete DCHECK which was triggered by CallTest.CreateDestroy_FlexfecReceiveStream. #

Patch Set 22 : Add return statement, to please windows compiler. #

Total comments: 29

Patch Set 23 : Address comments. #

Patch Set 24 : Rebased. #

Total comments: 10

Patch Set 25 : Address nits. #

Patch Set 26 : Rebase. #

Total comments: 19

Patch Set 27 : Rename foo_audio --> audio_foo. #

Patch Set 28 : Merge remote-tracking branch 'origin/master' into design-RtpTransportReceiveController #

Unified diffs Side-by-side diffs Delta from patch set Stats (+22 lines, -69 lines) Patch
M webrtc/audio/audio_receive_stream.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 2 chunks +3 lines, -0 lines 0 comments Download
M webrtc/audio/audio_receive_stream.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 1 chunk +0 lines, -9 lines 0 comments Download
M webrtc/call/call.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 1 chunk +3 lines, -0 lines 0 comments Download
M webrtc/test/call_test.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 2 chunks +4 lines, -4 lines 0 comments Download
M webrtc/test/mock_voe_channel_proxy.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 1 chunk +2 lines, -0 lines 0 comments Download
M webrtc/video/rtp_video_stream_receiver.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 2 chunks +1 line, -2 lines 0 comments Download
M webrtc/video/rtp_video_stream_receiver.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 4 chunks +8 lines, -20 lines 0 comments Download
M webrtc/voice_engine/channel.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 1 chunk +0 lines, -2 lines 0 comments Download
M webrtc/voice_engine/channel.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 2 chunks +1 line, -19 lines 0 comments Download
M webrtc/voice_engine/channel_proxy.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 1 chunk +0 lines, -2 lines 0 comments Download
M webrtc/voice_engine/channel_proxy.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 1 chunk +0 lines, -11 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 40 (8 generated)
nisse-webrtc
This is an early cl adding a RtpTransportControllerReceive class which is responsible for RTP demuxing ...
7 months, 3 weeks ago (2017-02-22 10:08:57 UTC) #2
pthatcher1
I think it would be much better to write an RtpDemuxer class which implements https://tools.ietf.org/html/draft-ietf-rtcweb-jsep-18#appendix-B. ...
7 months, 3 weeks ago (2017-02-23 00:26:09 UTC) #3
nisse-webrtc
On 2017/02/23 00:26:09, pthatcher1 wrote: > I think it would be much better to write ...
7 months, 3 weeks ago (2017-02-23 08:17:17 UTC) #4
nisse-webrtc
Thanks for the feedback. I appreciate advice both on where we want to end up, ...
7 months, 3 weeks ago (2017-02-23 09:45:19 UTC) #5
brandtr
https://codereview.webrtc.org/2709723003/diff/1/webrtc/call/rtp_transport_controller_receive.cc File webrtc/call/rtp_transport_controller_receive.cc (right): https://codereview.webrtc.org/2709723003/diff/1/webrtc/call/rtp_transport_controller_receive.cc#newcode141 webrtc/call/rtp_transport_controller_receive.cc:141: } On 2017/02/23 09:45:19, nisse-webrtc wrote: > On 2017/02/23 ...
7 months, 3 weeks ago (2017-02-23 12:19:32 UTC) #6
pthatcher1
On 2017/02/23 08:17:17, nisse-webrtc wrote: > On 2017/02/23 00:26:09, pthatcher1 wrote: > > I think ...
7 months, 3 weeks ago (2017-02-23 22:21:48 UTC) #7
pthatcher1
https://codereview.webrtc.org/2709723003/diff/1/webrtc/call/rtp_transport_controller_receive.cc File webrtc/call/rtp_transport_controller_receive.cc (right): https://codereview.webrtc.org/2709723003/diff/1/webrtc/call/rtp_transport_controller_receive.cc#newcode79 webrtc/call/rtp_transport_controller_receive.cc:79: const Config& config, On 2017/02/23 09:45:19, nisse-webrtc wrote: > ...
7 months, 3 weeks ago (2017-02-23 22:27:33 UTC) #8
pthatcher1
On 2017/02/23 22:27:33, pthatcher1 wrote: > https://codereview.webrtc.org/2709723003/diff/1/webrtc/call/rtp_transport_controller_receive.cc > File webrtc/call/rtp_transport_controller_receive.cc (right): > > https://codereview.webrtc.org/2709723003/diff/1/webrtc/call/rtp_transport_controller_receive.cc#newcode79 > ...
7 months, 2 weeks ago (2017-03-03 06:56:49 UTC) #9
the sun
https://codereview.webrtc.org/2709723003/diff/20001/webrtc/call/rtp_transport_controller_receive.h File webrtc/call/rtp_transport_controller_receive.h (right): https://codereview.webrtc.org/2709723003/diff/20001/webrtc/call/rtp_transport_controller_receive.h#newcode12 webrtc/call/rtp_transport_controller_receive.h:12: I find it confusing that we have 3 interfaces ...
7 months ago (2017-03-15 14:03:04 UTC) #10
nisse-webrtc
https://codereview.webrtc.org/2709723003/diff/20001/webrtc/call/rtp_transport_controller_receive.h File webrtc/call/rtp_transport_controller_receive.h (right): https://codereview.webrtc.org/2709723003/diff/20001/webrtc/call/rtp_transport_controller_receive.h#newcode12 webrtc/call/rtp_transport_controller_receive.h:12: On 2017/03/15 14:03:04, the sun wrote: > I find ...
7 months ago (2017-03-15 14:55:01 UTC) #11
danilchap
https://codereview.webrtc.org/2709723003/diff/40001/webrtc/call/rtp_transport_controller_receive.h File webrtc/call/rtp_transport_controller_receive.h (right): https://codereview.webrtc.org/2709723003/diff/40001/webrtc/call/rtp_transport_controller_receive.h#newcode33 webrtc/call/rtp_transport_controller_receive.h:33: // TODO(nisse): This additional interface may be a bit ...
7 months ago (2017-03-16 11:25:18 UTC) #14
Taylor Brandstetter
It's a little hard for me to review without seeing how this fits into the ...
7 months ago (2017-03-17 19:45:22 UTC) #15
stefan-webrtc
On 2017/03/17 19:45:22, Taylor Brandstetter wrote: > It's a little hard for me to review ...
7 months ago (2017-03-18 10:35:52 UTC) #16
the sun
On 2017/03/18 10:35:52, stefan-webrtc wrote: > On 2017/03/17 19:45:22, Taylor Brandstetter wrote: > > It's ...
7 months ago (2017-03-19 19:48:29 UTC) #17
nisse-webrtc
On 2017/03/15 14:55:01, nisse-webrtc wrote: > As for the > ObserverInterface, I think the right ...
7 months ago (2017-03-21 14:32:46 UTC) #18
nisse-webrtc
On 2017/03/21 14:32:46, nisse-webrtc wrote: > On 2017/03/15 14:55:01, nisse-webrtc wrote: > > As for ...
6 months, 4 weeks ago (2017-03-23 12:24:22 UTC) #19
Taylor Brandstetter
On 2017/03/23 12:24:22, nisse-webrtc wrote: > On 2017/03/21 14:32:46, nisse-webrtc wrote: > > On 2017/03/15 ...
6 months, 3 weeks ago (2017-03-23 21:59:25 UTC) #20
nisse-webrtc
On 2017/03/23 21:59:25, Taylor Brandstetter wrote: > Call::DeliverRtp already has media_type; can't it just use ...
6 months, 3 weeks ago (2017-03-24 07:49:25 UTC) #21
the sun
https://codereview.webrtc.org/2709723003/diff/320001/webrtc/audio/audio_receive_stream.h File webrtc/audio/audio_receive_stream.h (right): https://codereview.webrtc.org/2709723003/diff/320001/webrtc/audio/audio_receive_stream.h#newcode24 webrtc/audio/audio_receive_stream.h:24: #include "webrtc/modules/rtp_rtcp/source/rtp_packet_received.h" We only need forward decl here. https://codereview.webrtc.org/2709723003/diff/320001/webrtc/audio/audio_receive_stream_unittest.cc ...
6 months ago (2017-04-18 09:59:01 UTC) #22
nisse-webrtc
https://codereview.webrtc.org/2709723003/diff/320001/webrtc/audio/audio_receive_stream.cc File webrtc/audio/audio_receive_stream.cc (right): https://codereview.webrtc.org/2709723003/diff/320001/webrtc/audio/audio_receive_stream.cc#newcode301 webrtc/audio/audio_receive_stream.cc:301: channel_proxy_->OnRtpPacket(*packet); This is where we need channel_proxy_ to implement ...
6 months ago (2017-04-19 08:35:44 UTC) #23
nisse-webrtc
Seems to pass tests now. I think this is a step in the right direction, ...
6 months ago (2017-04-19 14:47:59 UTC) #26
danilchap
https://codereview.webrtc.org/2709723003/diff/410001/webrtc/call/rtp_transport_controller_receive.cc File webrtc/call/rtp_transport_controller_receive.cc (right): https://codereview.webrtc.org/2709723003/diff/410001/webrtc/call/rtp_transport_controller_receive.cc#newcode22 webrtc/call/rtp_transport_controller_receive.cc:22: class RtpTransportControllerReceive may be hide the class inside unnamed ...
6 months ago (2017-04-19 15:45:26 UTC) #29
nisse-webrtc
https://codereview.webrtc.org/2709723003/diff/410001/webrtc/call/rtp_transport_controller_receive.cc File webrtc/call/rtp_transport_controller_receive.cc (right): https://codereview.webrtc.org/2709723003/diff/410001/webrtc/call/rtp_transport_controller_receive.cc#newcode22 webrtc/call/rtp_transport_controller_receive.cc:22: class RtpTransportControllerReceive On 2017/04/19 15:45:25, danilchap wrote: > may ...
6 months ago (2017-04-20 10:53:38 UTC) #30
nisse-webrtc
I think this is getting close to ready. RtpTransportControllerReceive is now essentially a demuxer + ...
5 months, 3 weeks ago (2017-04-25 08:51:58 UTC) #31
danilchap
few more nits https://codereview.webrtc.org/2709723003/diff/450001/webrtc/call/rtp_transport_controller_receive.cc File webrtc/call/rtp_transport_controller_receive.cc (right): https://codereview.webrtc.org/2709723003/diff/450001/webrtc/call/rtp_transport_controller_receive.cc#newcode41 webrtc/call/rtp_transport_controller_receive.cc:41: #if 0 remove https://codereview.webrtc.org/2709723003/diff/450001/webrtc/call/rtp_transport_controller_receive.cc#newcode138 webrtc/call/rtp_transport_controller_receive.cc:138: if ...
5 months, 3 weeks ago (2017-04-25 09:33:40 UTC) #32
nisse-webrtc
After some offline discussion, my plan is to first investigate deletion of |enable_receive_side_bwe|, and then ...
5 months, 3 weeks ago (2017-04-25 11:22:50 UTC) #33
nisse-webrtc
On 2017/04/25 11:22:50, nisse-webrtc wrote: > |enable_receive_side_bwe| used to be a check of the MediaType, ...
5 months, 3 weeks ago (2017-04-27 13:49:50 UTC) #34
pthatcher1
https://codereview.webrtc.org/2709723003/diff/490001/webrtc/audio/audio_receive_stream.cc File webrtc/audio/audio_receive_stream.cc (right): https://codereview.webrtc.org/2709723003/diff/490001/webrtc/audio/audio_receive_stream.cc#newcode71 webrtc/audio/audio_receive_stream.cc:71: rtp_header_extensions_(config.rtp.extensions), Why not just use the config_.rtp.extensions? Why make ...
5 months, 3 weeks ago (2017-04-29 00:58:46 UTC) #35
nisse-webrtc
It seems the problem with this cl is to divide the needed changes into pieces ...
5 months, 2 weeks ago (2017-05-02 09:34:47 UTC) #36
pthatcher1
On 2017/05/02 09:34:47, nisse-webrtc wrote: > It seems the problem with this cl is to ...
5 months, 2 weeks ago (2017-05-05 20:33:42 UTC) #37
nisse-webrtc
This cl is partly superseded by https://codereview.webrtc.org/2886993005. I'm keeping it open for now, to see ...
3 months, 2 weeks ago (2017-06-29 10:34:40 UTC) #38
nisse-webrtc
1 month, 1 week ago (2017-09-07 07:54:00 UTC) #40
On 2017/06/29 10:34:40, nisse-webrtc wrote:
> This cl is partly superseded by  https://codereview.webrtc.org/2886993005. I'm
> keeping it open for now, to see what remains of interest after a later rebase.

Merged the changes from master. What remains is mainly reworked extensions
handling and TODO items. Which might be addressed in later separate cls.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 81bcdb8aa