Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2709723003:
Initial implementation of RtpTransportControllerReceive and related interfaces.
|
Created:
3 years, 10 months ago by nisse-webrtc Modified:
3 years, 3 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, mflodman Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionInitial 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 #Messages
Total messages: 40 (8 generated)
nisse@webrtc.org changed reviewers: + brandtr@webrtc.org, danilchap@webrtc.org, pthatcher@webrtc.org, solenberg@webrtc.org, stefan@webrtc.org
This is an early cl adding a RtpTransportControllerReceive class which is responsible for RTP demuxing (and nothing more, although at some point I want to extend it do do RTCP receive processing too). My plan is to switch all receive streams to implement the new interfaces, and refactor Call to delegate receive processing to the new class. I'll update the cl when I get that to work. If any of you would like to have a look at this initial step, feedback is appreciated.
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. It should focus on just that one responsibility. We can build up further things from there. But I think it's a mistake to mix too many things together. It gets confusing fast, and more so when we add other forms of demux. https://codereview.webrtc.org/2709723003/diff/1/webrtc/call/rtp_transport_con... File webrtc/call/rtp_transport_controller_receive.cc (right): https://codereview.webrtc.org/2709723003/diff/1/webrtc/call/rtp_transport_con... webrtc/call/rtp_transport_controller_receive.cc:79: const Config& config, If the config is not used when passing the packet to the receiver, why is it needed here? Just for the observer? https://codereview.webrtc.org/2709723003/diff/1/webrtc/call/rtp_transport_con... webrtc/call/rtp_transport_controller_receive.cc:131: return PacketReceiver::DELIVERY_PACKET_ERROR; I think it would make sense to have the RtpDemuxer take an already parsed RtpPacket. We should parse RtpPackets as soon as possible in the pipeline. https://codereview.webrtc.org/2709723003/diff/1/webrtc/call/rtp_transport_con... webrtc/call/rtp_transport_controller_receive.cc:135: return PacketReceiver::DELIVERY_UNKNOWN_SSRC; Please take a look at https://tools.ietf.org/html/draft-ietf-rtcweb-jsep-18#appendix-B. We should prepare to handle MID-based demux as well. https://codereview.webrtc.org/2709723003/diff/1/webrtc/call/rtp_transport_con... webrtc/call/rtp_transport_controller_receive.cc:138: return PacketReceiver::DELIVERY_PACKET_ERROR; We should have different errors for "couldn't demux" and "demuxed but the receiver rejected it". Having this error be the same as "couldn't parse" mixes up the two. https://codereview.webrtc.org/2709723003/diff/1/webrtc/call/rtp_transport_con... webrtc/call/rtp_transport_controller_receive.cc:141: } I do not understand why this is not just the responsibility of the receiver (that just received the packet above) to do. Why does the demuxer care about this? https://codereview.webrtc.org/2709723003/diff/1/webrtc/call/rtp_transport_con... webrtc/call/rtp_transport_controller_receive.cc:143: observer_->OnRtpPacket(media_type, stream->config, parsed_packet); Again, this seems like something that could be handled by the receiver. https://codereview.webrtc.org/2709723003/diff/1/webrtc/call/rtp_transport_con... File webrtc/call/rtp_transport_controller_receive.h (right): https://codereview.webrtc.org/2709723003/diff/1/webrtc/call/rtp_transport_con... webrtc/call/rtp_transport_controller_receive.h:41: }; I find the distinction between these two sinks confusing. It seems like it would be better to have different types for ParsedRtpPacket // Parsed but not identified IdentifiedRtpPacket // Parsed and identified And then have two sinks: ParsedRtpPacketSinkInterface virtual void OnParseRtpPacket(ParsedRtpPacket) IdentifiedRtpPacketSinkInterface virtual void OnIdentifiedRtpPacket(IdentifiedRtpPacket) It would be nicer still to not have this separation, but at least make the distinction more clear and consistent in naming. https://codereview.webrtc.org/2709723003/diff/1/webrtc/call/rtp_transport_con... webrtc/call/rtp_transport_controller_receive.h:50: class RtpTransportControllerReceiveInterface { Why not just call it RtpDemuxerInterface? https://codereview.webrtc.org/2709723003/diff/1/webrtc/call/rtp_transport_con... webrtc/call/rtp_transport_controller_receive.h:70: // transport for audio and video, with independent ssrc spaces. I think it would be a mistake to have an RtpDemuxer handle demuxing from more than one incoming transport. Then every time you call any method you have to specify the transport. It would be much more clear to have one Demuxer per transport. If you want to demux from more than one transport, create more than one demuxer. https://codereview.webrtc.org/2709723003/diff/1/webrtc/call/rtp_transport_con... webrtc/call/rtp_transport_controller_receive.h:77: // iterate over the mapping. The name here should match that for PayloadType below. And should leave room for MID. https://codereview.webrtc.org/2709723003/diff/1/webrtc/call/rtp_transport_con... webrtc/call/rtp_transport_controller_receive.h:98: RtpPacketReceiverInterface *receiver) = 0; The name here should match the AddReceiver (or AddSink) by ssrc above. In fact, I would suggest that we have one AddReceiver (or AddSink) that takes a set of SSRCs, a set of payload types, and a single MID, rather than 2-3 separate methods. https://codereview.webrtc.org/2709723003/diff/1/webrtc/call/rtp_transport_con... webrtc/call/rtp_transport_controller_receive.h:105: MediaType media_type, How do you know the media type of an unparsed, undemuxed rtp packet? and why do you even need it to do demux? https://codereview.webrtc.org/2709723003/diff/1/webrtc/call/rtp_transport_con... webrtc/call/rtp_transport_controller_receive.h:106: rtc::ArrayView<const uint8_t> packet) = 0; Yes another RtpPacket sink! What is this one? An UnparsedRtpPacket? Why not have it demux an already parsed packet? I would call this DemuxRtpPacket to go along with RtpPacketDemuxer. https://codereview.webrtc.org/2709723003/diff/1/webrtc/call/rtp_transport_con... webrtc/call/rtp_transport_controller_receive.h:117: // send side). Yet another sink! One is called a sink, one's called a receiver, and one's called an observer. I find this confusing. In this case (the observer), it seems like you have a further processed RtpPacket (has a media type and transport id). Perhaps: DemuxedRtpPacket DemuxedRtpPacketSinkInterface virtual void OnDemuxedRtpPacket(DemuxedRtpPacket)
On 2017/02/23 00:26:09, pthatcher1 wrote: > 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. Sounds like a good idea. I'm a little confused about the details, though. We'll see if I can make sense if it. I've had a quick look at that draft, which refers to https://tools.ietf.org/html/draft-ietf-mmusic-sdp-bundle-negotiation-04 for a corresponding RTP header extension. But the latter seems focused on SDP negotiation, details on RTP wire format are well hidden. I'll discuss with local RTP experts to see if I can understand this. And then the current interface to Call, using media_type, which is almost but not quite a transport id. In particular handling of MediaType::ANY (used by lots of test code) complicates things. Fitting this with the rest of the code, which is mostly unaware of m-lines, is going to be a bit tricky. It's not obvious what's the first step to make progress in the right direction. But if I guess a little, this is what you're suggesting RtpDemuxer should do: 1. Handle only a single transport (so RtpTransportController or Call would have multiple RtpDemuxers). 2. Accept RTP packets as input. Either raw, unparsed, (i.e., ArrayView<const uint8_t>) or with the base RTP header already parsed (i.e., RtpPacketReceived*, like the RtpPacketReceiverInterface)? 3. Get the mid ("m= line identifer") out of the packet (exactly how is still unclear to me, is it a header extension with a fixed numerical id?), look up m= line data, including the map of negotiated extension headers, and a handler for unsignalled ssrcs. In the unbundled case, there would be a single configured m= line which applies to the transport, and mids in the packets would be optional (or forbidden?). 4. Identify extension headers. 5. Lookup receiver based on ssrc. Could be a per-transport map or a map associated with the m-line, whatever is most convenient. Invoke either the right receiver, or a callback for unsignalled ssrcs. Payload types are never part of the demux. While in the current code, the bundled case is supported by mapping payload type to media type, if I've understood it correctly, and correct routing of unsignalled ssrcs depend on this. And for ORTC, we would still have the "m= line data" and mids on the wire in the RTP packets, but we wouldn't ever use the actual m= syntax, which is SDP only?
Thanks for the feedback. I appreciate advice both on where we want to end up, and what the initial steps should be. https://codereview.webrtc.org/2709723003/diff/1/webrtc/call/rtp_transport_con... File webrtc/call/rtp_transport_controller_receive.cc (right): https://codereview.webrtc.org/2709723003/diff/1/webrtc/call/rtp_transport_con... webrtc/call/rtp_transport_controller_receive.cc:79: const Config& config, On 2017/02/23 00:26:08, pthatcher1 wrote: > If the config is not used when passing the packet to the receiver, why is it > needed here? Just for the observer? Just for the observer, which corresponds to the current method Call::NotifyBweOfReceivedPacket, https://codesearch.chromium.org/chromium/src/third_party/webrtc/call/call.cc?... It needs a fully parsed packet, to examine extension headers, and it needs to know if the stream uses send-side BWE or not. https://codereview.webrtc.org/2709723003/diff/1/webrtc/call/rtp_transport_con... webrtc/call/rtp_transport_controller_receive.cc:131: return PacketReceiver::DELIVERY_PACKET_ERROR; On 2017/02/23 00:26:08, pthatcher1 wrote: > I think it would make sense to have the RtpDemuxer take an already parsed > RtpPacket. We should parse RtpPackets as soon as possible in the pipeline. What do you think of the RtpPacketReceived class? If we parse base header first, and edentify extensions later, then we either need to pass a non-const pointer to RtpPacketReceived, or make unnecessary copies of this object (but not of the underlying data buffer, I hope). I could try to push parsing earlier in the pipeline. One complication is that RTP packets are inserted not only from the network, but also from code processing red/rtx, ulpfec and FlexFEC. I would really like to have the responsibility for identifying extensions in a single place. FlexFEC packets need demux, and are currently routed via Call. Other recovery mechanisms bypass Call and send packets directly to the corresponding receiver, and it seems a bit odd to send them via a demuxer just to identify extensions. Maybe we should have an "m= line" object, which will own the RtpExtensionsMap, and have references to this object both from the demuxer (essential for handling unsignalled ssrcs) and from the stream itself (to support ulpfec and rtx). What's the right name for such an object? https://codereview.webrtc.org/2709723003/diff/1/webrtc/call/rtp_transport_con... webrtc/call/rtp_transport_controller_receive.cc:138: return PacketReceiver::DELIVERY_PACKET_ERROR; On 2017/02/23 00:26:08, pthatcher1 wrote: > We should have different errors for "couldn't demux" and "demuxed but the > receiver rejected it". Having this error be the same as "couldn't parse" mixes > up the two. I'm thinking maybe we don't need any return code at all, it's unclear what useful error handling one could do. We could have counters for stats, or propagate some callback up to the application. But the application can't do much more than display an error message to aid support and debugging. https://codereview.webrtc.org/2709723003/diff/1/webrtc/call/rtp_transport_con... webrtc/call/rtp_transport_controller_receive.cc:141: } On 2017/02/23 00:26:08, pthatcher1 wrote: > I do not understand why this is not just the responsibility of the receiver > (that just received the packet above) to do. Why does the demuxer care about > this? I'm thinking that a video receiver shouldn't need to know if its packets are protected by a FlexFEC stream. It seems cleaner to let the code setting up the FlexFEC stream tell the demuxer (or the RtpTransportController if we want to split responsibility) which packets it wants to see. Now this separation is just almost in place; the video receive stream currently needs to know if it's protected by flexfec, and this requirement is somewhere down in the jitterbuffer logic. I don't know why, but I'd really prefer if we could eliminate that dependency. https://codereview.webrtc.org/2709723003/diff/1/webrtc/call/rtp_transport_con... webrtc/call/rtp_transport_controller_receive.cc:143: observer_->OnRtpPacket(media_type, stream->config, parsed_packet); On 2017/02/23 00:26:08, pthatcher1 wrote: > Again, this seems like something that could be handled by the receiver. I'm aiming to decouple receivers from the congestion controller. I really want all interaction with the congestion controller to happen in the media-independent RTP code. The way I see it is that we have two pretty complex layers, RTP implementation, including congestion control, but unaware of media details. Which is what we're trying to refactor as RtpTransportController or "Pluggable RTP Transport". Media pipelines, which should be purged from all RTP dependencies. And then in between, we need some code to implement video-over-rtp, audio-over-quartc (product of media types and transports, with codec types possibly entering as well). We really need to cut down on the responsibilities of the in-between pieces. Everything which can be moved to the RTP-independent media pipeline, or to the media-independent RTP transport code, should be moved. And callbacks to flexfec and congestioncontroller are very reasonable to moved out. If they don't belong in the demuxer, they should be in RtpTransportController, not in the receive stream.
https://codereview.webrtc.org/2709723003/diff/1/webrtc/call/rtp_transport_con... File webrtc/call/rtp_transport_controller_receive.cc (right): https://codereview.webrtc.org/2709723003/diff/1/webrtc/call/rtp_transport_con... webrtc/call/rtp_transport_controller_receive.cc:141: } On 2017/02/23 09:45:19, nisse-webrtc wrote: > On 2017/02/23 00:26:08, pthatcher1 wrote: > > I do not understand why this is not just the responsibility of the receiver > > (that just received the packet above) to do. Why does the demuxer care about > > this? > > I'm thinking that a video receiver shouldn't need to know if its packets are > protected by a FlexFEC stream. It seems cleaner to let the code setting up the > FlexFEC stream tell the demuxer (or the RtpTransportController if we want to > split responsibility) which packets it wants to see. > > Now this separation is just almost in place; the video receive stream currently > needs to know if it's protected by flexfec, and this requirement is somewhere > down in the jitterbuffer logic. I don't know why, but I'd really prefer if we > could eliminate that dependency. I believe that philipel@ is planning on removing this dependency, if/when a new jitter buffer timing module is implemented. Currently, the "protected_by_fec" flag is used for the buffer length adaptation, but it's not clear to me how big of an impact it has in practice.
On 2017/02/23 08:17:17, nisse-webrtc wrote: > On 2017/02/23 00:26:09, pthatcher1 wrote: > > 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. > > Sounds like a good idea. I'm a little confused about the details, though. We'll > see if I can make sense if it. > > I've had a quick look at that draft, which refers to > https://tools.ietf.org/html/draft-ietf-mmusic-sdp-bundle-negotiation-04 for a > corresponding RTP header extension. But the latter seems focused on SDP > negotiation, details on RTP wire format are well hidden. I'll discuss with local > RTP experts to see if I can understand this. You can ignore the SDP part. And you can ignore MID for now other than to put a "TODO: Demux by MID header extension" in the right place. If you want to learn about the header extension, it's here: https://tools.ietf.org/html/draft-ietf-mmusic-sdp-bundle-negotiation-36#secti... > > And then the current interface to Call, using media_type, which is almost but > not quite a transport id. In particular handling of MediaType::ANY (used by lots > of test code) complicates things. > > Fitting this with the rest of the code, which is mostly unaware of m-lines, is > going to be a bit tricky. It's not obvious what's the first step to make > progress in the right direction. > This code should know nothing about m-lines. The whole mapping of m-line to transport and RtpSender/RtpReceiver is going to change soon as we implement "unified plan SDP". > But if I guess a little, this is what you're suggesting RtpDemuxer should do: > > 1. Handle only a single transport (so RtpTransportController or Call would have > multiple RtpDemuxers). Yes > > 2. Accept RTP packets as input. Either raw, unparsed, (i.e., ArrayView<const > uint8_t>) or with the base RTP header already parsed (i.e., RtpPacketReceived*, > like the RtpPacketReceiverInterface)? I think parsing it as much as possible before going in makes sense. For example, mapping header extension IDs to semantics before this point would be a good thing because it will make it really easy to get the MID. > > 3. Get the mid ("m= line identifer") out of the packet (exactly how is still > unclear to me, is it a header extension with a fixed numerical id?), Header extension, yes. Fixed id, no. You need to know the negotiated id => semantics mapping. > look up m= > line data, including the map of negotiated extension headers, The mapping is per-transport, not per m=line. In other words, whatever code does the parsing should know the transport context that it received the packet on and be able to do that as part of the parsing. > and a handler for > unsignalled ssrcs. If the packet has no MID and no known SSRC, then it falls back to payload type demux. If no payload type matches, then the packet should be dropped. In other words, the "unsignalled ssrc" case is actually the payload type case. > In the unbundled case, there would be a single configured m= > line which applies to the transport, Yes. Demux is trivial. But it's not that there's on m=line, it's that there is one RtpReceiver/track (with our current SDP, which is non-standard, there could be many RtpReceivers in one m=line). > and mids in the packets would be optional > (or forbidden?). > > 4. Identify extension headers. Should be done by the packet parser. > > 5. Lookup receiver based on ssrc. Could be a per-transport map or a map > associated with the m-line, whatever is most convenient. Invoke either the right > receiver, or a callback for unsignalled ssrcs. This should be a per-transport map of ssrc => receiver. And there should be a pt => receiver and mid => receiver map. > > Payload types are never part of the demux. While in the current code, the > bundled case is supported by mapping payload type to media type, if I've > understood it correctly, and correct routing of unsignalled ssrcs depend on > this. > The payload type => media type and then media type => demux is a hacky way of doing it that is going to fail in the full unbundled case when we move to standardized SDP. We should instead just move straight to payload type in the RtpDemuxer. > And for ORTC, we would still have the "m= line data" and mids on the wire in the > RTP packets, but we wouldn't ever use the actual m= syntax, which is SDP only? Correct. ORTC calls it a "receiver ID", but it's the same header extension.
https://codereview.webrtc.org/2709723003/diff/1/webrtc/call/rtp_transport_con... File webrtc/call/rtp_transport_controller_receive.cc (right): https://codereview.webrtc.org/2709723003/diff/1/webrtc/call/rtp_transport_con... webrtc/call/rtp_transport_controller_receive.cc:79: const Config& config, On 2017/02/23 09:45:19, nisse-webrtc wrote: > On 2017/02/23 00:26:08, pthatcher1 wrote: > > If the config is not used when passing the packet to the receiver, why is it > > needed here? Just for the observer? > > Just for the observer, which corresponds to the current method > Call::NotifyBweOfReceivedPacket, > https://codesearch.chromium.org/chromium/src/third_party/webrtc/call/call.cc?... > > It needs a fully parsed packet, to examine extension headers, and it needs to > know if the stream uses send-side BWE or not. So basically you just want to know if the RtpPacketReceiverInterface is using send-side BWE or not? https://codereview.webrtc.org/2709723003/diff/1/webrtc/call/rtp_transport_con... webrtc/call/rtp_transport_controller_receive.cc:131: return PacketReceiver::DELIVERY_PACKET_ERROR; On 2017/02/23 09:45:19, nisse-webrtc wrote: > On 2017/02/23 00:26:08, pthatcher1 wrote: > > I think it would make sense to have the RtpDemuxer take an already parsed > > RtpPacket. We should parse RtpPackets as soon as possible in the pipeline. > > What do you think of the RtpPacketReceived class? If we parse base header first, > and edentify extensions later, then we either need to pass a non-const pointer > to RtpPacketReceived, or make unnecessary copies of this object (but not of the > underlying data buffer, I hope). > > I could try to push parsing earlier in the pipeline. > > One complication is that RTP packets are inserted not only from the network, but > also from code processing red/rtx, ulpfec and FlexFEC. > > I would really like to have the responsibility for identifying extensions in a > single place. Perhaps a good solution would be to have an RtpPacketParser, and an RtpPacketDemuxer. And if you need to give an unparsed RTP packet to an RtpPacketDemuxer, first pass it to the RtpPacketParser and then to the RtpPacketDemuxer. Then at least the responsibilities are clearly defined. > > FlexFEC packets need demux, and are currently routed via Call. Other recovery > mechanisms bypass Call and send packets directly to the corresponding receiver, > and it seems a bit odd to send them via a demuxer just to identify extensions. Then use an RtpPacketParser for the parsing part. > > Maybe we should have an "m= line" object, which will own the RtpExtensionsMap, > and have references to this object both from the demuxer (essential for handling > unsignalled ssrcs) and from the stream itself (to support ulpfec and rtx). > What's the right name for such an object? That's what we call an RtpTransport. Taylor is working on one of those.
On 2017/02/23 22:27:33, pthatcher1 wrote: > https://codereview.webrtc.org/2709723003/diff/1/webrtc/call/rtp_transport_con... > File webrtc/call/rtp_transport_controller_receive.cc (right): > > https://codereview.webrtc.org/2709723003/diff/1/webrtc/call/rtp_transport_con... > webrtc/call/rtp_transport_controller_receive.cc:79: const Config& config, > On 2017/02/23 09:45:19, nisse-webrtc wrote: > > On 2017/02/23 00:26:08, pthatcher1 wrote: > > > If the config is not used when passing the packet to the receiver, why is it > > > needed here? Just for the observer? > > > > Just for the observer, which corresponds to the current method > > Call::NotifyBweOfReceivedPacket, > > > https://codesearch.chromium.org/chromium/src/third_party/webrtc/call/call.cc?... > > > > It needs a fully parsed packet, to examine extension headers, and it needs to > > know if the stream uses send-side BWE or not. > > So basically you just want to know if the RtpPacketReceiverInterface is using > send-side BWE or not? > > https://codereview.webrtc.org/2709723003/diff/1/webrtc/call/rtp_transport_con... > webrtc/call/rtp_transport_controller_receive.cc:131: return > PacketReceiver::DELIVERY_PACKET_ERROR; > On 2017/02/23 09:45:19, nisse-webrtc wrote: > > On 2017/02/23 00:26:08, pthatcher1 wrote: > > > I think it would make sense to have the RtpDemuxer take an already parsed > > > RtpPacket. We should parse RtpPackets as soon as possible in the pipeline. > > > > What do you think of the RtpPacketReceived class? If we parse base header > first, > > and edentify extensions later, then we either need to pass a non-const pointer > > to RtpPacketReceived, or make unnecessary copies of this object (but not of > the > > underlying data buffer, I hope). > > > > I could try to push parsing earlier in the pipeline. > > > > One complication is that RTP packets are inserted not only from the network, > but > > also from code processing red/rtx, ulpfec and FlexFEC. > > > > I would really like to have the responsibility for identifying extensions in a > > single place. > > Perhaps a good solution would be to have an RtpPacketParser, and an > RtpPacketDemuxer. And if you need to give an unparsed RTP packet to an > RtpPacketDemuxer, first pass it to the RtpPacketParser and then to the > RtpPacketDemuxer. Then at least the responsibilities are clearly defined. > > > > > FlexFEC packets need demux, and are currently routed via Call. Other recovery > > mechanisms bypass Call and send packets directly to the corresponding > receiver, > > and it seems a bit odd to send them via a demuxer just to identify extensions. > > Then use an RtpPacketParser for the parsing part. > > > > > Maybe we should have an "m= line" object, which will own the RtpExtensionsMap, > > and have references to this object both from the demuxer (essential for > handling > > unsignalled ssrcs) and from the stream itself (to support ulpfec and rtx). > > What's the right name for such an object? > > That's what we call an RtpTransport. Taylor is working on one of those. Actually, to be clear, it's not an "m= line" object. It's more like a "bundle group" object, since many m= lines can share the same RtpTransport, header extensions, etc. The per m= line thing is the RtpTransceiver, which Taylor's team will also be adding. But there are many RtpTransceiver to one RtpTransport.
https://codereview.webrtc.org/2709723003/diff/20001/webrtc/call/rtp_transport... File webrtc/call/rtp_transport_controller_receive.h (right): https://codereview.webrtc.org/2709723003/diff/20001/webrtc/call/rtp_transport... webrtc/call/rtp_transport_controller_receive.h:12: I find it confusing that we have 3 interfaces for receiving (in some sense of the word) packets: RtpPacketSinkInterface RtpPacketReceiverInterface RtpPacketObserverInterface A way to make the relationship clear, would be to use the type system, e.g. the RtpPacketReceived class, could be split in two, one with the unparsed payload, and a decorator class which references the former and adds fields for the parsed info. https://codereview.webrtc.org/2709723003/diff/20001/webrtc/call/rtp_transport... webrtc/call/rtp_transport_controller_receive.h:36: // packet->Parse, but not packet->IdentifyExtensions. Why? That seems like a hard-to-enforce contract. https://codereview.webrtc.org/2709723003/diff/20001/webrtc/call/rtp_transport... webrtc/call/rtp_transport_controller_receive.h:82: virtual bool AddSink(uint32_t ssrc, If the "RtpTransportControllerReceiveInterface"s purpose is to demux and route packets, couldn't this "side channel" be implemented with an RtpPacketReceiverInterface ? I find it confusing to attach both Receivers and Sinks to the same object. https://codereview.webrtc.org/2709723003/diff/20001/webrtc/call/rtp_transport... webrtc/call/rtp_transport_controller_receive.h:88: virtual bool RemoveSink(const RtpPacketSinkInterface* sink); = 0 Note: https://google.github.io/styleguide/cppguide.html#Interfaces
https://codereview.webrtc.org/2709723003/diff/20001/webrtc/call/rtp_transport... File webrtc/call/rtp_transport_controller_receive.h (right): https://codereview.webrtc.org/2709723003/diff/20001/webrtc/call/rtp_transport... webrtc/call/rtp_transport_controller_receive.h:12: On 2017/03/15 14:03:04, the sun wrote: > I find it confusing that we have 3 interfaces for receiving (in some sense of > the word) packets: > RtpPacketSinkInterface > RtpPacketReceiverInterface > RtpPacketObserverInterface > > A way to make the relationship clear, would be to use the type system, e.g. the > RtpPacketReceived class, could be split in two, one with the unparsed payload, > and a decorator class which references the former and adds fields for the parsed > info. > I added a TODO comment explaining how to get rid of the second. As for the ObserverInterface, I think the right way is to extract the piece of the congestion controller which is used on the receive side into its own class, and let RtpTransportReceive own that. https://codereview.webrtc.org/2709723003/diff/20001/webrtc/call/rtp_transport... webrtc/call/rtp_transport_controller_receive.h:36: // packet->Parse, but not packet->IdentifyExtensions. On 2017/03/15 14:03:04, the sun wrote: > Why? That seems like a hard-to-enforce contract. It's kind-of enforced via const; an implementation of RtpPacketSinkInterface can't call packet.IdentifyExtensions(...). https://codereview.webrtc.org/2709723003/diff/20001/webrtc/call/rtp_transport... webrtc/call/rtp_transport_controller_receive.h:82: virtual bool AddSink(uint32_t ssrc, On 2017/03/15 14:03:04, the sun wrote: > If the "RtpTransportControllerReceiveInterface"s purpose is to demux and route > packets, couldn't this "side channel" be implemented with an > RtpPacketReceiverInterface ? > > I find it confusing to attach both Receivers and Sinks to the same object. Agree it's a bit confusing, but they have different responsibilities. https://codereview.webrtc.org/2709723003/diff/20001/webrtc/call/rtp_transport... webrtc/call/rtp_transport_controller_receive.h:88: virtual bool RemoveSink(const RtpPacketSinkInterface* sink); On 2017/03/15 14:03:04, the sun wrote: > = 0 > > Note: https://google.github.io/styleguide/cppguide.html#Interfaces Done.
Description was changed from ========== Initial implementation of RtpTransportControllerReceive and related interfaces. BUG=webrtc:7135 ========== to ========== Initial implementation of RtpTransportControllerReceive and related interfaces. BUG=webrtc:7135 ==========
pthatcher@webrtc.org changed reviewers: + deadbeef@webrtc.org
https://codereview.webrtc.org/2709723003/diff/40001/webrtc/call/rtp_transport... File webrtc/call/rtp_transport_controller_receive.h (right): https://codereview.webrtc.org/2709723003/diff/40001/webrtc/call/rtp_transport... webrtc/call/rtp_transport_controller_receive.h:33: // TODO(nisse): This additional interface may be a bit confusing, it's there are use cases (mid & rid) where extensions have to be identified before receiver can be chosen. Likely they will need to be identified twice: 1st with transport-wide extensions map, then, if individual receiver has it's own map, with receiver's extensions map. https://codereview.webrtc.org/2709723003/diff/40001/webrtc/call/rtp_transport... webrtc/call/rtp_transport_controller_receive.h:53: class RtpPacketObserverInterface; May be more concrete examples of receivers and observers give better explanation why there are several interfaces. Personally I think 3 is 1 too many, 2 should be enough: Receiver/Sink reserve single ssrc and process all packets addressed to it. That packet doesn't have to come from a network, it might be restored rtx, red or fec packet. audio/video streams, rtx, flexfec will implement this interface. packet goes to 1 receiver. Observers watch all packets from the network, regardless of ssrc. But they are not interested in recreated packets. Congestion controller and flexfec (with own ssrc filtering) should implement this interface. packet goes through all observers. (flexfec will likely implement both interfaces: it need to watch all protected streams/ssrcs and it is responsible for own stream/ssrc.)
It's a little hard for me to review without seeing how this fits into the big picture, and which aspects of the design are intentional vs. exist only because of current limitations in other areas (like BaseChannel...). Is there a design doc describing how we plan for things to work in end? Something that defines the scope of each object and how it interacts with other objects, at a high level? https://codereview.webrtc.org/2709723003/diff/40001/webrtc/call/rtp_transport... File webrtc/call/rtp_transport_controller_receive.h (right): https://codereview.webrtc.org/2709723003/diff/40001/webrtc/call/rtp_transport... webrtc/call/rtp_transport_controller_receive.h:49: virtual bool OnRtpPacketReceive(RtpPacketReceived* packet) = 0; OnRtpPacketReceived instead of OnRtpPacketReceive? https://codereview.webrtc.org/2709723003/diff/40001/webrtc/call/rtp_transport... webrtc/call/rtp_transport_controller_receive.h:126: // Creates the default implementation. Add comment for what |observer| is meant for? https://codereview.webrtc.org/2709723003/diff/40001/webrtc/call/rtp_transport... webrtc/call/rtp_transport_controller_receive.h:134: class RtpPacketObserverInterface { The distinction between "RtpPacketObserver", "RtpPacketReceiver" and "RtpPacketSink" seems likely to confuse people. Could they be named more descriptively so the differences are more obvious?
On 2017/03/17 19:45:22, Taylor Brandstetter wrote: > It's a little hard for me to review without seeing how this fits into the big > picture, and which aspects of the design are intentional vs. exist only because > of current limitations in other areas (like BaseChannel...). > > Is there a design doc describing how we plan for things to work in end? > Something that defines the scope of each object and how it interacts with other > objects, at a high level? I think we should expand nisses design doc to contain this. Nisse, let's take a look on Monday and get some more details in there around these questions. > > https://codereview.webrtc.org/2709723003/diff/40001/webrtc/call/rtp_transport... > File webrtc/call/rtp_transport_controller_receive.h (right): > > https://codereview.webrtc.org/2709723003/diff/40001/webrtc/call/rtp_transport... > webrtc/call/rtp_transport_controller_receive.h:49: virtual bool > OnRtpPacketReceive(RtpPacketReceived* packet) = 0; > OnRtpPacketReceived instead of OnRtpPacketReceive? > > https://codereview.webrtc.org/2709723003/diff/40001/webrtc/call/rtp_transport... > webrtc/call/rtp_transport_controller_receive.h:126: // Creates the default > implementation. > Add comment for what |observer| is meant for? > > https://codereview.webrtc.org/2709723003/diff/40001/webrtc/call/rtp_transport... > webrtc/call/rtp_transport_controller_receive.h:134: class > RtpPacketObserverInterface { > The distinction between "RtpPacketObserver", "RtpPacketReceiver" and > "RtpPacketSink" seems likely to confuse people. Could they be named more > descriptively so the differences are more obvious?
On 2017/03/18 10:35:52, stefan-webrtc wrote: > On 2017/03/17 19:45:22, Taylor Brandstetter wrote: > > It's a little hard for me to review without seeing how this fits into the big > > picture, and which aspects of the design are intentional vs. exist only > because > > of current limitations in other areas (like BaseChannel...). > > > > Is there a design doc describing how we plan for things to work in end? > > Something that defines the scope of each object and how it interacts with > other > > objects, at a high level? > > I think we should expand nisses design doc to contain this. > > Nisse, let's take a look on Monday and get some more details in there around > these questions. Thank you, that'd be great! > > > > > > https://codereview.webrtc.org/2709723003/diff/40001/webrtc/call/rtp_transport... > > File webrtc/call/rtp_transport_controller_receive.h (right): > > > > > https://codereview.webrtc.org/2709723003/diff/40001/webrtc/call/rtp_transport... > > webrtc/call/rtp_transport_controller_receive.h:49: virtual bool > > OnRtpPacketReceive(RtpPacketReceived* packet) = 0; > > OnRtpPacketReceived instead of OnRtpPacketReceive? > > > > > https://codereview.webrtc.org/2709723003/diff/40001/webrtc/call/rtp_transport... > > webrtc/call/rtp_transport_controller_receive.h:126: // Creates the default > > implementation. > > Add comment for what |observer| is meant for? > > > > > https://codereview.webrtc.org/2709723003/diff/40001/webrtc/call/rtp_transport... > > webrtc/call/rtp_transport_controller_receive.h:134: class > > RtpPacketObserverInterface { > > The distinction between "RtpPacketObserver", "RtpPacketReceiver" and > > "RtpPacketSink" seems likely to confuse people. Could they be named more > > descriptively so the differences are more obvious?
On 2017/03/15 14:55:01, nisse-webrtc wrote: > As for the > ObserverInterface, I think the right way is to extract the piece of the > congestion controller which is used on the receive side into its own class, and > let RtpTransportReceive own that. This part now done, interface deleted. (Except that ownership of ReceiveSideCongestionController stays with Call).
On 2017/03/21 14:32:46, nisse-webrtc wrote: > On 2017/03/15 14:55:01, nisse-webrtc wrote: > > As for the > > ObserverInterface, I think the right way is to extract the piece of the > > congestion controller which is used on the receive side into its own class, > and > > let RtpTransportReceive own that. > > This part now done, interface deleted. (Except that ownership of > ReceiveSideCongestionController stays with Call). Current red bots seem to be because I've broken stats. I wonder how to do that properly. We have the following counters in Call: RateCounter received_bytes_per_second_counter_; RateCounter received_audio_bytes_per_second_counter_; RateCounter received_video_bytes_per_second_counter_; They are updated depending on the media type, and which mapping the ssrc is found in. Not clear how to do this in a media-independent fashion. In the unbundled case, it would be natural to have one counter per transport, and use media_type to distinguish. But what about the bundled case? Should we still set media_type (based on payload type or mid extension), similarly to how we do it today? And use separate ssrc demuxers and ssrc mappings and everything? And then MediaType::ANY makes things even more complicated. Maybe make an effort and delete that first. In tests, I think it originates in FakeNetworkPipe.
On 2017/03/23 12:24:22, nisse-webrtc wrote: > On 2017/03/21 14:32:46, nisse-webrtc wrote: > > On 2017/03/15 14:55:01, nisse-webrtc wrote: > > > As for the > > > ObserverInterface, I think the right way is to extract the piece of the > > > congestion controller which is used on the receive side into its own class, > > and > > > let RtpTransportReceive own that. > > > > This part now done, interface deleted. (Except that ownership of > > ReceiveSideCongestionController stays with Call). > > Current red bots seem to be because I've broken stats. I wonder how to do that > properly. We have the following counters in Call: > > RateCounter received_bytes_per_second_counter_; > RateCounter received_audio_bytes_per_second_counter_; > RateCounter received_video_bytes_per_second_counter_; > > They are updated depending on the media type, and which mapping the ssrc is > found in. Not clear how to do this in a media-independent fashion. > > In the unbundled case, it would be natural to have one counter per transport, > and use media_type to distinguish. > > But what about the bundled case? Should we still set media_type (based on > payload type or mid extension), similarly to how we do it today? And use > separate ssrc demuxers and ssrc mappings and everything? > > And then MediaType::ANY makes things even more complicated. Maybe make an effort > and delete that first. In tests, I think it originates in FakeNetworkPipe. Call::DeliverRtp already has media_type; can't it just use that and increment the correct counter? Maybe I don't understand the issue.
On 2017/03/23 21:59:25, Taylor Brandstetter wrote: > Call::DeliverRtp already has media_type; can't it just use that and increment > the correct counter? Maybe I don't understand the issue. Should work, except, that when media_type == MediaType::ANY, it's not obvious which counter to update. If I manage to eliminate that, things get easier. See https://codereview.webrtc.org/2774463003/
https://codereview.webrtc.org/2709723003/diff/320001/webrtc/audio/audio_recei... File webrtc/audio/audio_receive_stream.h (right): https://codereview.webrtc.org/2709723003/diff/320001/webrtc/audio/audio_recei... 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_recei... File webrtc/audio/audio_receive_stream_unittest.cc (right): https://codereview.webrtc.org/2709723003/diff/320001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream_unittest.cc:257: recv_stream.OnRtpPacketReceive(&parsed_packet); EXPECT_TRUE( https://codereview.webrtc.org/2709723003/diff/320001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2709723003/diff/320001/webrtc/call/call.cc#newc... webrtc/call/call.cc:567: // reason is ssrc collisions due to misconfiguration. Should that really be checked at this level? Call has so far been considered an implementation detail. I see the same pattern repeated below and it makes me worried. In general, internal code should DCHECK failure conditions rather than returning errors, and the input should be verified at API boundaries. We currently have heaps of code which returns errors that are never handled, but silently ignored. Check early, fail fast! https://codereview.webrtc.org/2709723003/diff/320001/webrtc/call/flexfec_rece... File webrtc/call/flexfec_receive_stream_impl.h (right): https://codereview.webrtc.org/2709723003/diff/320001/webrtc/call/flexfec_rece... webrtc/call/flexfec_receive_stream_impl.h:43: // webrtc::RtpPacketReceiverInterface implementation, for the What's the difference between an RtpPacketReceiver and an RtpPacketSink? https://codereview.webrtc.org/2709723003/diff/320001/webrtc/call/rtp_transpor... File webrtc/call/rtp_transport_controller_receive.h (right): https://codereview.webrtc.org/2709723003/diff/320001/webrtc/call/rtp_transpor... webrtc/call/rtp_transport_controller_receive.h:26: // This class represents a receiver of already parsed RTP packets. It would be nice to use the type system to document this. For instance, the parsed state of the received RTP packet, could be implemented as a decorator for the packet data container. https://codereview.webrtc.org/2709723003/diff/320001/webrtc/call/rtp_transpor... webrtc/call/rtp_transport_controller_receive.h:87: // Used to represent auxillary sinks, currently used for FlexFec. I get the feeling that what you're implementing here could be accomplished with a filter graph? We have at least 3 different varieties of OnRtpPacket(), in separate interfaces. Can we have less code by having a single abstraction for an RTP packet consumer? https://codereview.webrtc.org/2709723003/diff/320001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel_proxy.h (right): https://codereview.webrtc.org/2709723003/diff/320001/webrtc/voice_engine/chan... webrtc/voice_engine/channel_proxy.h:53: class ChannelProxy : public RtpPacketSinkInterface { No, I don't think you should register the ChannelProxy directly as a packet sink. This should just be a proxy class. You can register the voe::Channel directly. Oh, this doesn't have an impl. Leftover?
https://codereview.webrtc.org/2709723003/diff/320001/webrtc/audio/audio_recei... File webrtc/audio/audio_receive_stream.cc (right): https://codereview.webrtc.org/2709723003/diff/320001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:301: channel_proxy_->OnRtpPacket(*packet); This is where we need channel_proxy_ to implement OnRtpPacket. Registering the voe::Channel directly doesn't seem practical at the moment, when registration is done by Call. Next step on the receive side is to pass an |RtpTransportControllerReceiveInterface*| from Call to the receive stream constructors, which would give a bit more flexibility. https://codereview.webrtc.org/2709723003/diff/320001/webrtc/audio/audio_recei... File webrtc/audio/audio_receive_stream.h (right): https://codereview.webrtc.org/2709723003/diff/320001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.h:24: #include "webrtc/modules/rtp_rtcp/source/rtp_packet_received.h" On 2017/04/18 09:59:00, the sun wrote: > We only need forward decl here. Done. https://codereview.webrtc.org/2709723003/diff/320001/webrtc/audio/audio_recei... File webrtc/audio/audio_receive_stream_unittest.cc (right): https://codereview.webrtc.org/2709723003/diff/320001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream_unittest.cc:257: recv_stream.OnRtpPacketReceive(&parsed_packet); On 2017/04/18 09:59:00, the sun wrote: > EXPECT_TRUE( Done. https://codereview.webrtc.org/2709723003/diff/320001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2709723003/diff/320001/webrtc/call/call.cc#newc... webrtc/call/call.cc:567: // reason is ssrc collisions due to misconfiguration. On 2017/04/18 09:59:00, the sun wrote: > Should that really be checked at this level? Call has so far been considered an > implementation detail. I see the same pattern repeated below and it makes me > worried. > > In general, internal code should DCHECK failure conditions rather than returning > errors, and the input should be verified at API boundaries. We currently have > heaps of code which returns errors that are never handled, but silently ignored. > > Check early, fail fast! I would make things simpler to drop the DCHECK, drop the return value of AddReceiver, and add DCHECK further down. But it's not clear to me who's responsibility it is to sanity check the SDP negotiation. We don't want to crash because the peer sends us some bogus SDP using the same ssrc for two separate audio streams. https://codereview.webrtc.org/2709723003/diff/320001/webrtc/call/flexfec_rece... File webrtc/call/flexfec_receive_stream_impl.h (right): https://codereview.webrtc.org/2709723003/diff/320001/webrtc/call/flexfec_rece... webrtc/call/flexfec_receive_stream_impl.h:43: // webrtc::RtpPacketReceiverInterface implementation, for the On 2017/04/18 09:59:00, the sun wrote: > What's the difference between an RtpPacketReceiver and an RtpPacketSink? A PacketReceiver knows the extensions mapping, and is responsible for calling IdentifyExtensions. And hence needs a mutable packet object. I'll investigate moving the responsibility for the extensions map up (or, alternatively, eliminating IdentifyExtensions altogether). If that works out, we only need the RtpPacketSink interface. https://codereview.webrtc.org/2709723003/diff/320001/webrtc/call/rtp_transpor... File webrtc/call/rtp_transport_controller_receive.h (right): https://codereview.webrtc.org/2709723003/diff/320001/webrtc/call/rtp_transpor... webrtc/call/rtp_transport_controller_receive.h:26: // This class represents a receiver of already parsed RTP packets. On 2017/04/18 09:59:00, the sun wrote: > It would be nice to use the type system to document this. For instance, the > parsed state of the received RTP packet, could be implemented as a decorator for > the packet data container. I think I replied to this earlier... I use const for that purpose, even if it might be a bit crude. "Decorators" goes beyond my current C++ knowledge... Eventually, I hope we will need only the "sink" interface with a const packet. https://codereview.webrtc.org/2709723003/diff/320001/webrtc/call/rtp_transpor... webrtc/call/rtp_transport_controller_receive.h:87: // Used to represent auxillary sinks, currently used for FlexFec. On 2017/04/18 09:59:00, the sun wrote: > I get the feeling that what you're implementing here could be accomplished with > a filter graph? We have at least 3 different varieties of OnRtpPacket(), in > separate interfaces. Can we have less code by having a single abstraction for an > RTP packet consumer? Probably. We just need to move complete parsing further up. pthatcher also suggested doing parsing and demuxing in separate classes, which might also help make this clearer. https://codereview.webrtc.org/2709723003/diff/320001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel_proxy.h (right): https://codereview.webrtc.org/2709723003/diff/320001/webrtc/voice_engine/chan... webrtc/voice_engine/channel_proxy.h:53: class ChannelProxy : public RtpPacketSinkInterface { On 2017/04/18 09:59:00, the sun wrote: > No, I don't think you should register the ChannelProxy directly as a packet > sink. This should just be a proxy class. You can register the voe::Channel > directly. I'll give it a try. > Oh, this doesn't have an impl. Leftover? There is a OnRtpPacket method below. Which just passes the packet on to channel()->OnRtpPacket(...).
The CQ bit was checked by nisse@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Seems to pass tests now. I think this is a step in the right direction, even if it's a bit ugly with the multiple OnRtpPacket-like interfaces. Can we land it, or what's the next step? The closest alternative, if we don't want to make the cl even larger, might be to try to move knowledge of the rtp extensions map over from receive streams to Call first. Not sure how hard that would be.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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 may be hide the class inside unnamed namespace to be safer https://codereview.webrtc.org/2709723003/diff/410001/webrtc/call/rtp_transpor... webrtc/call/rtp_transport_controller_receive.cc:25: explicit RtpTransportControllerReceive( remove explicit: there are 2 parameters https://codereview.webrtc.org/2709723003/diff/410001/webrtc/call/rtp_transpor... webrtc/call/rtp_transport_controller_receive.cc:46: ~RtpTransportControllerReceive() override; declare destructor before other methods, just after constructor 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); iterators usually ok to copy, i.e. auto it = streams_.find(ssrc); 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))); 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); https://codereview.webrtc.org/2709723003/diff/410001/webrtc/call/rtp_transpor... webrtc/call/rtp_transport_controller_receive.cc:118: for (auto it : streams_) { auto& 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); what does this line do? 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())) Parse(raw_packet) (there is Parse version that takes exactly ArrayView<const uint8_t>) 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. 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. 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) { may be auto* to show values are pointers and thus copy is cheap https://codereview.webrtc.org/2709723003/diff/410001/webrtc/call/rtp_transpor... webrtc/call/rtp_transport_controller_receive.cc:171: // static remove this comment 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() {} 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) https://codereview.webrtc.org/2709723003/diff/410001/webrtc/call/rtp_transpor... webrtc/call/rtp_transport_controller_receive.h:68: struct Config { is there plan to make it TransportController config? (then Create factory should take the parameter) or just Receiver's config (then likely rename to something like StreamConfig/ChannelConfig/SinkConfig) https://codereview.webrtc.org/2709723003/diff/410001/webrtc/call/rtp_transpor... webrtc/call/rtp_transport_controller_receive.h:90: #if 0 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. 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); add override
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.
I think this is getting close to ready. RtpTransportControllerReceive is now essentially a demuxer + ReceiveSideCongestionController.
few more nits https://codereview.webrtc.org/2709723003/diff/450001/webrtc/call/rtp_transpor... File webrtc/call/rtp_transport_controller_receive.cc (right): https://codereview.webrtc.org/2709723003/diff/450001/webrtc/call/rtp_transpor... webrtc/call/rtp_transport_controller_receive.cc:41: #if 0 remove https://codereview.webrtc.org/2709723003/diff/450001/webrtc/call/rtp_transpor... webrtc/call/rtp_transport_controller_receive.cc:138: if (!stream->receiver->OnRtpPacketReceive(&parsed_packet)) here you do not have {} around one-line if, previous statement - you do. Choose one style in the same file. https://codereview.webrtc.org/2709723003/diff/450001/webrtc/call/rtp_transpor... webrtc/call/rtp_transport_controller_receive.cc:177: return std::unique_ptr<RtpTransportControllerReceiveInterface>( may be start using MakeUnique helper: #include "webrtc/base/ptr_util.h" return rtc::MakeUnique<RtpTransportControllerReceive>(receive_side_cc, enable_receive_side_bwe); https://codereview.webrtc.org/2709723003/diff/450001/webrtc/call/rtp_transpor... File webrtc/call/rtp_transport_controller_receive.h (right): https://codereview.webrtc.org/2709723003/diff/450001/webrtc/call/rtp_transpor... webrtc/call/rtp_transport_controller_receive.h:66: // TODO(nisse): It turns out this data is only used for the callback what (and when) should be done about it? https://codereview.webrtc.org/2709723003/diff/450001/webrtc/call/rtp_transpor... webrtc/call/rtp_transport_controller_receive.h:70: bool use_send_side_bwe = false; why controller itself configured with 'enable_receive_side_bwe' and each individual receiver configured with 'use_send_side_bwe' Can you put comments what each of those two parameters mean?
After some offline discussion, my plan is to first investigate deletion of |enable_receive_side_bwe|, and then split into a first cl adding the RtpTransportControllerReceiv class, and a followup to take it into use. https://codereview.webrtc.org/2709723003/diff/450001/webrtc/call/rtp_transpor... File webrtc/call/rtp_transport_controller_receive.cc (right): https://codereview.webrtc.org/2709723003/diff/450001/webrtc/call/rtp_transpor... webrtc/call/rtp_transport_controller_receive.cc:41: #if 0 On 2017/04/25 09:33:40, danilchap wrote: > remove Done. https://codereview.webrtc.org/2709723003/diff/450001/webrtc/call/rtp_transpor... webrtc/call/rtp_transport_controller_receive.cc:138: if (!stream->receiver->OnRtpPacketReceive(&parsed_packet)) On 2017/04/25 09:33:40, danilchap wrote: > here you do not have {} around one-line if, previous statement - you do. > Choose one style in the same file. Added braces for all the ifs. https://codereview.webrtc.org/2709723003/diff/450001/webrtc/call/rtp_transpor... webrtc/call/rtp_transport_controller_receive.cc:177: return std::unique_ptr<RtpTransportControllerReceiveInterface>( On 2017/04/25 09:33:40, danilchap wrote: > may be start using MakeUnique helper: > > #include "webrtc/base/ptr_util.h" > > return rtc::MakeUnique<RtpTransportControllerReceive>(receive_side_cc, > enable_receive_side_bwe); Done. https://codereview.webrtc.org/2709723003/diff/450001/webrtc/call/rtp_transpor... File webrtc/call/rtp_transport_controller_receive.h (right): https://codereview.webrtc.org/2709723003/diff/450001/webrtc/call/rtp_transpor... webrtc/call/rtp_transport_controller_receive.h:66: // TODO(nisse): It turns out this data is only used for the callback On 2017/04/25 09:33:40, danilchap wrote: > what (and when) should be done about it? I'm rephrasing the comment. There's nothing obvious to do about it, beside some general simplification of how the various bitrate estimators are used. https://codereview.webrtc.org/2709723003/diff/450001/webrtc/call/rtp_transpor... webrtc/call/rtp_transport_controller_receive.h:70: bool use_send_side_bwe = false; On 2017/04/25 09:33:40, danilchap wrote: > why controller itself configured with 'enable_receive_side_bwe' and each > individual receiver configured with 'use_send_side_bwe' > Can you put comments what each of those two parameters mean? |enable_receive_side_bwe| used to be a check of the MediaType, needed to make some audio tests pass. Without it, we generated unexpected REMB packets, iirc. This isn't pretty. I could try deleting |enable_receive_side_bwe| and run the bots, to see if it's still needed and how to fix it.
On 2017/04/25 11:22:50, nisse-webrtc wrote: > |enable_receive_side_bwe| used to be a check of the MediaType, needed to make > some audio tests pass. Without it, we generated unexpected REMB packets, iirc. > This isn't pretty. I could try deleting |enable_receive_side_bwe| and run the > bots, to see if it's still needed and how to fix it. I've made a cl to try deleting the check, see https://codereview.webrtc.org/2840833002/. If that goes well, we don't need it here either. Unfortunately, that cl got a bit blocked on a voe::Channel tsan race.
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?
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)? 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.
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.
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).
brandtr@webrtc.org changed reviewers: - brandtr@webrtc.org
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. |
