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

Issue 2709723003: Initial implementation of RtpTransportControllerReceive and related interfaces.

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 months ago by nisse-webrtc
Modified:
1 day, 15 hours 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. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+449 lines, -307 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 4 chunks +8 lines, -3 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 4 chunks +6 lines, -11 lines 0 comments Download
M webrtc/audio/audio_receive_stream_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 2 chunks +1 line, -7 lines 0 comments Download
M webrtc/call/BUILD.gn 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 2 chunks +2 lines, -0 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 19 chunks +71 lines, -196 lines 0 comments Download
M webrtc/call/flexfec_receive_stream_impl.h View 1 2 4 chunks +14 lines, -3 lines 0 comments Download
M webrtc/call/flexfec_receive_stream_impl.cc View 1 2 3 2 chunks +7 lines, -0 lines 0 comments Download
A webrtc/call/rtp_transport_controller_receive.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 1 chunk +107 lines, -0 lines 0 comments Download
A webrtc/call/rtp_transport_controller_receive.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 1 chunk +180 lines, -0 lines 0 comments Download
M webrtc/modules/rtp_rtcp/include/flexfec_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 2 chunks +4 lines, -2 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 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 1 chunk +0 lines, -2 lines 0 comments Download
M webrtc/video/rtp_stream_receiver.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 5 chunks +5 lines, -4 lines 0 comments Download
M webrtc/video/rtp_stream_receiver.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 6 chunks +24 lines, -34 lines 0 comments Download
M webrtc/video/video_receive_stream.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 3 chunks +4 lines, -2 lines 0 comments Download
M webrtc/video/video_receive_stream.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +2 lines, -2 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 4 chunks +5 lines, -3 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 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 4 chunks +4 lines, -4 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 1 chunk +0 lines, -11 lines 0 comments Download
Trybot results: Sign in to try more bots
Commit queue not available (can’t edit this change).

Messages

Total messages: 39 (8 generated)
nisse-webrtc
This is an early cl adding a RtpTransportControllerReceive class which is responsible for RTP demuxing ...
6 months 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. ...
6 months 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 ...
6 months 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, ...
6 months 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 ...
6 months 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 ...
6 months 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: > ...
6 months 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 > ...
5 months, 3 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 ...
5 months, 1 week 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 ...
5 months, 1 week 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 ...
5 months, 1 week 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 ...
5 months, 1 week 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 ...
5 months, 1 week 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 ...
5 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 ...
5 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 ...
5 months 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 ...
5 months 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 ...
5 months 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 ...
4 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 ...
4 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, ...
4 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 ...
4 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 ...
4 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 + ...
3 months, 4 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 ...
3 months, 4 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 ...
3 months, 4 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, ...
3 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 ...
3 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 ...
3 months, 3 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 ...
3 months, 2 weeks ago (2017-05-05 20:33:42 UTC) #37
nisse-webrtc
1 month, 3 weeks ago (2017-06-29 10:34:40 UTC) #38
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.
Feel free to prune the reviewers list; there's no need to review in the current
shape (which is ps#27).
Sign in to reply to this message.

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