Initial implementation of RtpTransportControllerReceive and related interfaces.

webrtc/call/rtp_transport_controller_receive.cc

+/*
+ *  Copyright (c) 2017 The WebRTC project authors. All Rights Reserved.
+ *
+ *  Use of this source code is governed by a BSD-style license
+ *  that can be found in the LICENSE file in the root of the source
+ *  tree. An additional intellectual property rights grant can be found
+ *  in the file PATENTS.  All contributing project authors may
+ *  be found in the AUTHORS file in the root of the source tree.
+ */
+
+#include <map>
+#include <utility>
+#include <vector>
+
+#include "webrtc/call/rtp_transport_controller_receive.h"
+
+#include "webrtc/modules/rtp_rtcp/source/rtp_packet_received.h"
+
+namespace webrtc {
+
+class RtpTransportControllerReceive
+    : public RtpTransportControllerReceiveInterface {
+ public:
+  // ImplementRtpTransportControllerReceiveInterface
+  bool AddReceiver(uint32_t ssrc,
+                   MediaType media_type,
+                   const Config& config,
+                   RtpPacketReceiverInterface* receiver) override;
+  bool RemoveReceiver(const RtpPacketReceiverInterface* receiver) override;
+
+  bool AddSink(uint32_t ssrc,
+               MediaType media_type,
+               RtpPacketSinkInterface* sink) override;
+  bool RemoveSink(const RtpPacketSinkInterface* sink) override;
+
+#if 0
+  bool RegisterPayload(uint8_t payload_type, MediaType media_type,
+                       RtpPacketReceiverInterface *receiver) override;
+#endif
+  PacketReceiver::DeliveryStatus OnRtpPacket(
+      MediaType media_type,
+      rtc::ArrayView<const uint8_t> packet) override;
+
+ private:
+  struct Stream {
+    MediaType media_type = MediaType::ANY;
+    Config config;
+    RtpPacketReceiverInterface* receiver = nullptr;
+    std::vector<RtpPacketSinkInterface*> auxillary_sinks;
+
+    Stream(MediaType media_type,
+           Config config,
+           RtpPacketReceiverInterface* receiver)
+        : media_type(media_type), config(config), receiver(receiver) {}
+  };
+
+  Stream* LookupStream(uint32_t ssrc, MediaType media_type);
+
+  // Indexed by ssrc. We could use a map indexed by pairs (media_type, ssrc),
+  // except that we want special handling of MediaType:ANY specially.
+  std::multimap<uint32_t, Stream> streams_;
+  RtpPacketObserverInterface* observer_;
+};
+
+RtpTransportControllerReceive::Stream*
+RtpTransportControllerReceive::LookupStream(uint32_t ssrc,
+                                            MediaType media_type) {
+  auto range = streams_.equal_range(ssrc);
+  for (auto it = range.first; it != range.second; ++it) {
+    if (media_type == MediaType::ANY || media_type == it->second.media_type)
+      return &it->second;
+  }
+  return nullptr;
+}
+
+bool RtpTransportControllerReceive::AddReceiver(
+    uint32_t ssrc,
+    MediaType media_type,
+    const Config& config,

[pthatcher1, 2017/02/23 00:26:08]

If the config is not used when passing the packet to the receiver, why is it
needed here?  Just for the observer?

[nisse-webrtc, 2017/02/23 09:45:19]

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.

[pthatcher1, 2017/02/23 22:27:33]

So basically you just want to know if the RtpPacketReceiverInterface is using
send-side BWE or not?  

+    RtpPacketReceiverInterface* receiver) {
+  if (LookupStream(ssrc, media_type)) {
+    return false;
+  }
+  streams_.insert(
+      std::pair<uint32_t, Stream>(ssrc, Stream(media_type, config, receiver)));
+  return true;
+}
+
+bool RtpTransportControllerReceive::RemoveReceiver(
+    const RtpPacketReceiverInterface* receiver) {
+  for (auto it = streams_.begin(); it != streams_.end(); ++it) {
+    if (it->second.receiver == receiver) {
+      streams_.erase(it);
+      return true;
+    }
+  }
+  return false;
+}
+
+bool RtpTransportControllerReceive::AddSink(uint32_t ssrc,
+                                            MediaType media_type,
+                                            RtpPacketSinkInterface* sink) {
+  Stream* stream = LookupStream(ssrc, media_type);
+  if (stream) {
+    stream->auxillary_sinks.push_back(sink);
+    return true;
+  }
+  return false;
+}
+
+bool RtpTransportControllerReceive::RemoveSink(
+    const RtpPacketSinkInterface* sink) {
+  bool found = false;
+  for (auto it : streams_) {
+    auto sinks_end = it.second.auxillary_sinks.end();
+    auto sinks_it =
+        std::remove(it.second.auxillary_sinks.begin(), sinks_end, sink);
+    if (sinks_it != sinks_end) {
+      it.second.auxillary_sinks.erase(sinks_it, sinks_end);
+      found = true;
+    }
+  }
+  return found;
+}
+
+PacketReceiver::DeliveryStatus RtpTransportControllerReceive::OnRtpPacket(
+    MediaType media_type,
+    rtc::ArrayView<const uint8_t> raw_packet) {
+  RtpPacketReceived parsed_packet;
+  if (!parsed_packet.Parse(raw_packet.data(), raw_packet.size()))
+    return PacketReceiver::DELIVERY_PACKET_ERROR;

[pthatcher1, 2017/02/23 00:26:08]

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.

[nisse-webrtc, 2017/02/23 09:45:19]

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?

[pthatcher1, 2017/02/23 22:27:33]

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.
Then use an RtpPacketParser for the parsing part.  
That's what we call an RtpTransport.  Taylor is working on one of those.  

+  Stream* stream = LookupStream(parsed_packet.Ssrc(), media_type);
+  if (!stream) {
+    // TODO(nisse): Lookup payload, for unsignalled streams.
+    return PacketReceiver::DELIVERY_UNKNOWN_SSRC;

[pthatcher1, 2017/02/23 00:26:08]

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.

+  }
+  if (!stream->receiver->OnRtpPacketReceive(&parsed_packet))
+    return PacketReceiver::DELIVERY_PACKET_ERROR;

[pthatcher1, 2017/02/23 00:26:08]

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.

[nisse-webrtc, 2017/02/23 09:45:19]

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.

+  for (auto it : stream->auxillary_sinks) {
+    it->OnRtpPacket(parsed_packet);
+  }

[pthatcher1, 2017/02/23 00:26:08]

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?

[nisse-webrtc, 2017/02/23 09:45:19]

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.

[brandtr, 2017/02/23 12:19:32]

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.

+  if (observer_)
+    observer_->OnRtpPacket(media_type, stream->config, parsed_packet);

[pthatcher1, 2017/02/23 00:26:08]

Again, this seems like something that could be handled by the receiver.

[nisse-webrtc, 2017/02/23 09:45:19]

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.

+  return PacketReceiver::DELIVERY_OK;
+}
+
+}  // namespace webrtc