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

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:
3 days, 1 hour 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. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+452 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 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 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 1 chunk +108 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 1 chunk +182 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 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 1 chunk +0 lines, -11 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 30 (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 ...
1 month, 4 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: > ...
1 month, 4 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 > ...
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, 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 ...
1 month, 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 ...
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 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 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 ...
6 days, 7 hours 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 ...
5 days, 8 hours 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, ...
5 days, 2 hours 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 ...
5 days, 1 hour ago (2017-04-19 15:45:26 UTC) #29
nisse-webrtc
4 days, 6 hours ago (2017-04-20 10:53:38 UTC) #30
https://codereview.webrtc.org/2709723003/diff/410001/webrtc/call/rtp_transpor...
File webrtc/call/rtp_transport_controller_receive.cc (right):

https://codereview.webrtc.org/2709723003/diff/410001/webrtc/call/rtp_transpor...
webrtc/call/rtp_transport_controller_receive.cc:22: class
RtpTransportControllerReceive
On 2017/04/19 15:45:25, danilchap wrote:
> may be hide the class inside unnamed namespace to be safer

Done.

https://codereview.webrtc.org/2709723003/diff/410001/webrtc/call/rtp_transpor...
webrtc/call/rtp_transport_controller_receive.cc:25: explicit
RtpTransportControllerReceive(
On 2017/04/19 15:45:25, danilchap wrote:
> remove explicit: there are 2 parameters

Done.

https://codereview.webrtc.org/2709723003/diff/410001/webrtc/call/rtp_transpor...
webrtc/call/rtp_transport_controller_receive.cc:46:
~RtpTransportControllerReceive() override;
On 2017/04/19 15:45:25, danilchap wrote:
> declare destructor before other methods, just after constructor

Done.

https://codereview.webrtc.org/2709723003/diff/410001/webrtc/call/rtp_transpor...
webrtc/call/rtp_transport_controller_receive.cc:78: const auto& it =
streams_.find(ssrc);
On 2017/04/19 15:45:25, danilchap wrote:
> iterators usually ok to copy, i.e.
> auto it = streams_.find(ssrc);

Ok. Keeping const, though, to make it clearer that we're not using it to iterate
anything.

https://codereview.webrtc.org/2709723003/diff/410001/webrtc/call/rtp_transpor...
webrtc/call/rtp_transport_controller_receive.cc:88:
streams_.insert(std::pair<uint32_t, Stream>(ssrc, Stream(config, receiver)));
On 2017/04/19 15:45:25, danilchap wrote:
> may be
> streams_.emplace(ssrc, Stream(config, receiver));
> to avoid double (with DCHECK) lookup:
> bool inserted = streams_.emplace(ssrc, Stream(config, receiver)).second;
> RTC_DCHECK(inserted);

Done.

https://codereview.webrtc.org/2709723003/diff/410001/webrtc/call/rtp_transpor...
webrtc/call/rtp_transport_controller_receive.cc:118: for (auto it : streams_) {
On 2017/04/19 15:45:25, danilchap wrote:
> auto&

Done.

https://codereview.webrtc.org/2709723003/diff/410001/webrtc/call/rtp_transpor...
webrtc/call/rtp_transport_controller_receive.cc:122:
it.second.auxillary_sinks.erase(sinks_it, sinks_end);
On 2017/04/19 15:45:25, danilchap wrote:
> what does this line do?

It's one half of the somewhat strange erase-remove idiom. See
https://en.wikipedia.org/wiki/Erase%E2%80%93remove_idiom

(If you know of a nicer way to do this, I'd like to hear).

https://codereview.webrtc.org/2709723003/diff/410001/webrtc/call/rtp_transpor...
webrtc/call/rtp_transport_controller_receive.cc:130: if
(!parsed_packet.Parse(raw_packet.data(), raw_packet.size()))
On 2017/04/19 15:45:25, danilchap wrote:
> Parse(raw_packet)
> (there is Parse version that takes exactly ArrayView<const uint8_t>)

Nice, I wasn't aware of that.

https://codereview.webrtc.org/2709723003/diff/410001/webrtc/call/rtp_transpor...
webrtc/call/rtp_transport_controller_receive.cc:136: // TODO(nisse): Lookup
payload, for unsignalled streams.
On 2017/04/19 15:45:25, danilchap wrote:
> this todo about adding a brand new feature. May be no need to write TODO in
that
> case.
> Specially that it is more complicated than demuxing by payload.

I'm removing this TODO and related #if:ed out code.

https://codereview.webrtc.org/2709723003/diff/410001/webrtc/call/rtp_transpor...
webrtc/call/rtp_transport_controller_receive.cc:141: for (auto it :
stream->auxillary_sinks) {
On 2017/04/19 15:45:25, danilchap wrote:
> may be auto* to show values are pointers and thus copy is cheap

Done. I'm not that familiar with all the conventions regarding auto.

https://codereview.webrtc.org/2709723003/diff/410001/webrtc/call/rtp_transpor...
webrtc/call/rtp_transport_controller_receive.cc:171: //  static
On 2017/04/19 15:45:25, danilchap wrote:
> remove  this comment

I think it's a common convention in the rest of the code to mark implementation
of static methods like this (but with a single space). Including in recent code,
like the TaskQueue implementations. There's no other syntactic hint that it's
not a usual method.

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

https://codereview.webrtc.org/2709723003/diff/410001/webrtc/call/rtp_transpor...
webrtc/call/rtp_transport_controller_receive.h:30: virtual
~RtpPacketSinkInterface() {}
On 2017/04/19 15:45:25, danilchap wrote:
> in all 3 classes in this file prefer default order: Nested type, factory
method,
> destructor, other methods.
> (https://google.github.io/styleguide/cppguide.html#Declaration_Order)

Done.

https://codereview.webrtc.org/2709723003/diff/410001/webrtc/call/rtp_transpor...
webrtc/call/rtp_transport_controller_receive.h:90: #if 0
On 2017/04/19 15:45:25, danilchap wrote:
> better to remove for now and add when it will be used.
> The interface might be different than current plan and it would be easier to
> review which one is better when it will be in use.

Done.

https://codereview.webrtc.org/2709723003/diff/410001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/include/flexfec_receiver.h (right):

https://codereview.webrtc.org/2709723003/diff/410001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/include/flexfec_receiver.h:48: void OnRtpPacket(const
RtpPacketReceived& packet);
On 2017/04/19 15:45:25, danilchap wrote:
> add override

Done.
Sign in to reply to this message.

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