Chromium Code Reviews
Help | Chromium Project | Sign in
(16)

Issue 2709723003: Initial implementation of RtpTransportControllerReceive and related interfaces.

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 months ago by nisse-webrtc
Modified:
6 hours, 26 minutes 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: 13
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 1 comment 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 1 comment 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 19 chunks +71 lines, -196 lines 4 comments Download
M webrtc/call/flexfec_receive_stream_impl.h View 1 2 4 chunks +14 lines, -3 lines 1 comment Download
M webrtc/call/flexfec_receive_stream_impl.cc View 1 2 3 2 chunks +7 lines, -0 lines 1 comment 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 5 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: 35 (7 generated)
nisse-webrtc
This is an early cl adding a RtpTransportControllerReceive class which is responsible for RTP demuxing ...
2 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. ...
2 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 ...
2 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, ...
2 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 ...
2 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 ...
2 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: > ...
2 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 > ...
1 month, 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 ...
1 month, 2 weeks 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 ...
1 month, 2 weeks 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 ...
1 month, 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 ...
1 month, 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 ...
1 month, 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 ...
1 month, 1 week 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 ...
1 month, 1 week 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 ...
1 month 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 ...
1 month 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 ...
1 month 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 ...
1 week, 3 days 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 ...
1 week, 2 days 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, ...
1 week, 2 days 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 ...
1 week, 2 days 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 ...
1 week, 1 day 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 days, 22 hours 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 days, 21 hours 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 days, 20 hours 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, ...
1 day, 17 hours ago (2017-04-27 13:49:50 UTC) #34
pthatcher1
6 hours, 26 minutes ago (2017-04-29 00:58:46 UTC) #35
https://codereview.webrtc.org/2709723003/diff/490001/webrtc/audio/audio_recei...
File webrtc/audio/audio_receive_stream.cc (right):

https://codereview.webrtc.org/2709723003/diff/490001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream.cc:71:
rtp_header_extensions_(config.rtp.extensions),
Why not just use the config_.rtp.extensions?  Why make a separate copy of them?

https://codereview.webrtc.org/2709723003/diff/490001/webrtc/audio/audio_recei...
File webrtc/audio/audio_receive_stream.h (right):

https://codereview.webrtc.org/2709723003/diff/490001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream.h:59: bool
OnRtpPacketReceive(RtpPacketReceived* packet) override;
This is a weird name.  Shouldn't it be "OnRtpPacketRecieved"?  With a "d" on the
end?

https://codereview.webrtc.org/2709723003/diff/490001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/2709723003/diff/490001/webrtc/call/call.cc#newc...
webrtc/call/call.cc:250: std::unique_ptr<RtpTransportControllerReceiveInterface>
RtpTransportControllerReceiveInterface is a confusing name.

Why not just RtpPacketReceiverInterface or something like that?

https://codereview.webrtc.org/2709723003/diff/490001/webrtc/call/call.cc#newc...
webrtc/call/call.cc:251: rtp_transport_receive_video_ GUARDED_BY(receive_crit_);
I prefer "audio_foo" and "video_foo" over "foo_audio" and "foo_video".  It's
more consistent with all the code I've seen "like
received_video_bytes_per_second_counter_" in this very file.

https://codereview.webrtc.org/2709723003/diff/490001/webrtc/call/call.cc#newc...
webrtc/call/call.cc:527: config.rtp.remote_ssrc, receive_config,
receive_stream);
A "receive" adds a "receiver" using a "receive config"?  I find these names very
confusing.

https://codereview.webrtc.org/2709723003/diff/490001/webrtc/call/call.cc#newc...
webrtc/call/call.cc:750:
rtp_transport_receive_video_->RemoveReceiver(receive_stream_impl);
They have receivers and sinks?  And they are the same object?  This is even more
confusing to me.  What's the difference between a receiver and a sink (and a
"receive")?

https://codereview.webrtc.org/2709723003/diff/490001/webrtc/call/flexfec_rece...
File webrtc/call/flexfec_receive_stream_impl.cc (right):

https://codereview.webrtc.org/2709723003/diff/490001/webrtc/call/flexfec_rece...
webrtc/call/flexfec_receive_stream_impl.cc:156: return true;
Why does FlexfecReceiveStreamImpl have to know about header extensions?  Why is
that not done before being passed in?

https://codereview.webrtc.org/2709723003/diff/490001/webrtc/call/flexfec_rece...
File webrtc/call/flexfec_receive_stream_impl.h (right):

https://codereview.webrtc.org/2709723003/diff/490001/webrtc/call/flexfec_rece...
webrtc/call/flexfec_receive_stream_impl.h:49: void OnRtpPacket(const
RtpPacketReceived& packet) override;
Why do we have two methods that look almost exactly the same with almost the
same names, but not quiet, and then one is for some stuff and one is for other
stuff, but it's not at all clear why or what the distinction is.  One is
magically FlexFec and the other is not, but why?

https://codereview.webrtc.org/2709723003/diff/490001/webrtc/call/rtp_transpor...
File webrtc/call/rtp_transport_controller_receive.h (right):

https://codereview.webrtc.org/2709723003/diff/490001/webrtc/call/rtp_transpor...
webrtc/call/rtp_transport_controller_receive.h:30: virtual void
OnRtpPacket(const RtpPacketReceived& packet) = 0;
Why the name "RtpPacketReceived"?  Why not just "RtpPacket" or perhaps, since
it's a "parsed RTP packet", must "ParsedRtpPacket"?  "RtpPacketReceived" is a
confusing name.

https://codereview.webrtc.org/2709723003/diff/490001/webrtc/call/rtp_transpor...
webrtc/call/rtp_transport_controller_receive.h:51: };
Could we, instead, have an RtpHeaderExtensionIdentifier that has
IdentifyHeaderExtensions?  Then instead of calling OnRtpPacketReceive with an
RtpPacket that doesn't know it's header extensions, it first calls
IdentifyHeaderExtensions and then calls RtpPacketSinceInterface::OnRtpPacket?   
That would be a lot more clear.

But even then, I think it would be better to just get rid the need for this in
the first place before doing this work.

https://codereview.webrtc.org/2709723003/diff/490001/webrtc/call/rtp_transpor...
webrtc/call/rtp_transport_controller_receive.h:53: // This class represents the
RTP receive parsing and demuxing, for a
"RTP receive parsing and demuxing".  What does "receive" mean here?  And why
does the demuxer need to parse first?  Why not have an RtpParserInterface?  And
an RtpDemuxerInterface?  Even if the demuxer also parses, it should at least be
called RtpDemuxer.  That would be a *much* more clear name.

https://codereview.webrtc.org/2709723003/diff/490001/webrtc/call/rtp_transpor...
webrtc/call/rtp_transport_controller_receive.h:81: const Config& config,
Is this going to do full demux using MID, SSRCs, and PTs?  If so, the config
should contain the ssrc.  Later, it needs to expand to an MID, a list of SSRCs,
and a list of PTs.  So it's probably best to just stick it in the config now so
you don't have to keep changing the interface.

I can see this doing just SSRC demux and a higher-level construct will do full
demux, and that's probably fine.  But it should be made clear here.

https://codereview.webrtc.org/2709723003/diff/490001/webrtc/call/rtp_transpor...
webrtc/call/rtp_transport_controller_receive.h:90: virtual void AddSink(uint32_t
ssrc, RtpPacketSinkInterface* sink) = 0;
Why do we need two separate interfaces for getting an RTP packet?  Why doesn't
this just use AddReceiver?
Sign in to reply to this message.

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