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

Issue 2709723003: Initial implementation of RtpTransportControllerReceive and related interfaces.

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 months ago by nisse-webrtc
Modified:
3 weeks, 2 days 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: 37 (7 generated)
nisse-webrtc
This is an early cl adding a RtpTransportControllerReceive class which is responsible for RTP demuxing ...
3 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. ...
3 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 ...
3 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, ...
3 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 ...
3 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 ...
3 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: > ...
3 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 > ...
2 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 ...
2 months, 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 ...
2 months, 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 ...
2 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 ...
2 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 ...
2 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 ...
2 months, 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 ...
2 months, 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 ...
2 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 ...
2 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 ...
2 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 ...
1 month, 1 week 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 month, 1 week 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 month, 1 week 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 month, 1 week 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 month, 1 week 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 + ...
1 month 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 ...
1 month 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 ...
1 month 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 month 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 ...
4 weeks, 1 day 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 weeks, 5 days ago (2017-05-02 09:34:47 UTC) #36
pthatcher1
3 weeks, 2 days ago (2017-05-05 20:33:42 UTC) #37
On 2017/05/02 09:34:47, nisse-webrtc wrote:
> It seems the problem with this cl is to divide the needed changes into pieces
> that make sense. And it seems difficult to make progress. 
> 
> Would it make more sense to start with only the RtpPacketSinkInterface
(matching
> current methods like
> 
> void AudioReceiveStream::OnRtpPacket(const RtpPacketReceived& packet)
> 
> and a demuxer object which is essentially is multimap of ssrc to sinks (later
to
> be extended with mid-support and the like)?

Several smaller CLs would be easier to get through, I think, then one big one.. 
Something that just starts with one sink/receiver interface and demuxing onto
those without the extra complications of flexfec might make sense to understand.
 

On the other hand, I'm not an owner in this area of the code, so I'm ultimately
not the one you need approval from.  I'm more picky than most about names and
interfaces, but if you convince the owners of this area that this is the right
way to go, then don't wait for me, especially since I am often so slow to get
back to you (sorry... this one was like 6 days!).

If you want to convince me that this is the right way to go, than maybe we
should have a VC to help me understand rather than going back and forth via
email.  Mostly at this point I'm just picky about the name of the send/receive
split of the RtpTransportController and the fact that we have two sink/receiver
interfaces with different names and different purposes and the names don't make
it clear how they differ.  


> 
>
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),
> On 2017/04/29 00:58:46, pthatcher1 wrote:
> > Why not just use the config_.rtp.extensions?  Why make a separate copy of
> them?
> 
> This is also a type conversion, from a vector to the type
RtpHeaderExtensionsMap
> used by IdentifyExtensions.
> 
> 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>
> On 2017/04/29 00:58:46, pthatcher1 wrote:
> > RtpTransportControllerReceiveInterface is a confusing name.
> > 
> > Why not just RtpPacketReceiverInterface or something like that?
> 
> It's analogous to RtpTransportControllerSendInterface. The idea is that this
> doesn't represent a receiver (or sender), it represents the functionality of
the
> RtpTransportController which a receiver (and sender) needs.
> 
>
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_);
> On 2017/04/29 00:58:46, pthatcher1 wrote:
> > 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.
> 
> Ok, I'll try to keep that in mind, and rename these members.
> 
>
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);
> On 2017/04/29 00:58:46, pthatcher1 wrote:
> > 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")?
> 
> I've tried to explain a couple of times, and it seems you're still unhappy
about
> the distinction. I'll try to clean it up.
> 
>
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:51: };
> On 2017/04/29 00:58:46, pthatcher1 wrote:
> > Could we, instead, have an RtpHeaderExtensionIdentifier that has
> > IdentifyHeaderExtensions?
> 
> I think we could. It adds one more method call on the receive path, but that's
> probably no big deal. Or we could keep parsing and identification in Call for
> the time being, like it's currently done with Call::receive_rtp_config.
> 
> > 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.  
> 
> Maybe. It's tricky to find a sane order of the steps...
> 
>
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
> On 2017/04/29 00:58:46, pthatcher1 wrote:
> > "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.  
> 
> The intent is that this interface is passed to ReceiveStream constructors.
Then
> what it needs to do will change over time. Initially, it will be a demuxer.
> Further on, it should act like a factory for RtpTransportReceiver objects.
Sign in to reply to this message.

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