Initial implementation of RtpTransportControllerReceive and related interfaces.

webrtc/call/rtp_transport_controller_receive.cc

Index: webrtc/call/rtp_transport_controller_receive.cc
diff --git a/webrtc/call/rtp_transport_controller_receive.cc b/webrtc/call/rtp_transport_controller_receive.cc
new file mode 100644
index 0000000000000000000000000000000000000000..8c556569495cd0d25ce95817d723762163a6cd0f
--- /dev/null
+++ b/webrtc/call/rtp_transport_controller_receive.cc
@@ -0,0 +1,181 @@
+/*
+ *  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/congestion_controller/include/receive_side_congestion_controller.h"
+#include "webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h"
+#include "webrtc/modules/rtp_rtcp/source/rtp_packet_received.h"
+
+namespace webrtc {
+
+class RtpTransportControllerReceive

[danilchap, 2017/04/19 15:45:25]

may be hide the class inside unnamed namespace to be safer

[nisse-webrtc, 2017/04/20 10:53:38]

Done.

+    : public RtpTransportControllerReceiveInterface {
+ public:
+  explicit RtpTransportControllerReceive(

[danilchap, 2017/04/19 15:45:25]

remove explicit: there are 2 parameters

[nisse-webrtc, 2017/04/20 10:53:38]

Done.

+      ReceiveSideCongestionController* receive_side_cc,
+      bool enable_receive_side_bwe);
+
+  // ImplementRtpTransportControllerReceiveInterface
+  void AddReceiver(uint32_t ssrc,
+                   const Config& config,
+                   RtpPacketReceiverInterface* receiver) override;
+  void RemoveReceiver(const RtpPacketReceiverInterface* receiver) override;
+
+  void AddSink(uint32_t ssrc, RtpPacketSinkInterface* sink) override;
+  void RemoveSink(const RtpPacketSinkInterface* sink) override;
+
+#if 0
+  void RegisterPayload(uint8_t payload_type, MediaType media_type,
+                       RtpPacketReceiverInterface *receiver) override;
+#endif
+  PacketReceiver::DeliveryStatus OnRtpPacket(
+      int64_t arrival_time_ms,
+      rtc::ArrayView<const uint8_t> packet) override;
+
+  ~RtpTransportControllerReceive() override;

[danilchap, 2017/04/19 15:45:25]

declare destructor before other methods, just after constructor

[nisse-webrtc, 2017/04/20 10:53:37]

Done.

+
+ private:
+  struct Stream {
+    Config config;
+    RtpPacketReceiverInterface* receiver;
+    std::vector<RtpPacketSinkInterface*> auxillary_sinks;
+
+    Stream(Config config, RtpPacketReceiverInterface* receiver)
+        : config(config), receiver(receiver) {}
+  };
+
+  Stream* LookupStream(uint32_t ssrc);
+
+  // Indexed by ssrc.
+  std::map<uint32_t, Stream> streams_;
+  ReceiveSideCongestionController* const receive_side_cc_;
+  const bool enable_receive_side_bwe_;
+};
+
+RtpTransportControllerReceive::RtpTransportControllerReceive(
+    ReceiveSideCongestionController* receive_side_cc,
+    bool enable_receive_side_bwe)
+    : receive_side_cc_(receive_side_cc),
+      enable_receive_side_bwe_(enable_receive_side_bwe) {}
+
+RtpTransportControllerReceive::~RtpTransportControllerReceive() {
+  RTC_DCHECK(streams_.empty());
+}
+
+RtpTransportControllerReceive::Stream*
+RtpTransportControllerReceive::LookupStream(uint32_t ssrc) {
+  const auto& it = streams_.find(ssrc);

[danilchap, 2017/04/19 15:45:25]

iterators usually ok to copy, i.e.
auto it = streams_.find(ssrc);

[nisse-webrtc, 2017/04/20 10:53:38]

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

+  return (it != streams_.end()) ? &it->second : nullptr;
+}
+
+void RtpTransportControllerReceive::AddReceiver(
+    uint32_t ssrc,
+    const Config& config,
+    RtpPacketReceiverInterface* receiver) {
+  RTC_DCHECK(!LookupStream(ssrc));
+
+  streams_.insert(std::pair<uint32_t, Stream>(ssrc, Stream(config, receiver)));

[danilchap, 2017/04/19 15:45:25]

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);

[nisse-webrtc, 2017/04/20 10:53:38]

Done.

+}
+
+void RtpTransportControllerReceive::RemoveReceiver(
+    const RtpPacketReceiverInterface* receiver) {
+  for (auto it = streams_.begin(); it != streams_.end();) {
+    if (it->second.receiver == receiver) {
+      receive_side_cc_
+          ->GetRemoteBitrateEstimator(it->second.config.use_send_side_bwe)
+          ->RemoveStream(it->first);
+      it = streams_.erase(it);
+    } else {
+      ++it;
+    }
+  }
+}
+
+void RtpTransportControllerReceive::AddSink(uint32_t ssrc,
+                                            RtpPacketSinkInterface* sink) {
+  Stream* stream = LookupStream(ssrc);
+  // Can't DCHECK this, since flexfec tests create flexfec streams
+  // without creating the streams they are protecting.
+  if (!stream)
+    return;
+
+  stream->auxillary_sinks.push_back(sink);
+}
+
+void RtpTransportControllerReceive::RemoveSink(
+    const RtpPacketSinkInterface* sink) {
+  for (auto it : streams_) {

[danilchap, 2017/04/19 15:45:25]

auto&

[nisse-webrtc, 2017/04/20 10:53:37]

Done.

+    auto sinks_end = it.second.auxillary_sinks.end();
+    auto sinks_it =
+        std::remove(it.second.auxillary_sinks.begin(), sinks_end, sink);
+    it.second.auxillary_sinks.erase(sinks_it, sinks_end);

[danilchap, 2017/04/19 15:45:25]

what does this line do?

[nisse-webrtc, 2017/04/20 10:53:37]

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).

+  }
+}
+
+PacketReceiver::DeliveryStatus RtpTransportControllerReceive::OnRtpPacket(
+    int64_t arrival_time_ms,
+    rtc::ArrayView<const uint8_t> raw_packet) {
+  RtpPacketReceived parsed_packet;
+  if (!parsed_packet.Parse(raw_packet.data(), raw_packet.size()))

[danilchap, 2017/04/19 15:45:25]

Parse(raw_packet)
(there is Parse version that takes exactly ArrayView<const uint8_t>)

[nisse-webrtc, 2017/04/20 10:53:37]

Nice, I wasn't aware of that.

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

[danilchap, 2017/04/19 15:45:25]

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.

[nisse-webrtc, 2017/04/20 10:53:38]

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

+    return PacketReceiver::DELIVERY_UNKNOWN_SSRC;
+  }
+  if (!stream->receiver->OnRtpPacketReceive(&parsed_packet))
+    return PacketReceiver::DELIVERY_PACKET_ERROR;
+  for (auto it : stream->auxillary_sinks) {

[danilchap, 2017/04/19 15:45:25]

may be auto* to show values are pointers and thus copy is cheap

[nisse-webrtc, 2017/04/20 10:53:37]

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

+    it->OnRtpPacket(parsed_packet);
+  }
+  if (receive_side_cc_) {
+    if (!stream->config.use_send_side_bwe &&
+        parsed_packet.HasExtension<TransportSequenceNumber>()) {
+      // Inconsistent configuration of send side BWE. Do nothing.
+      // TODO(nisse): Without this check, we may produce RTCP feedback
+      // packets even when not negotiated. But it would be cleaner to
+      // move the check down to RTCPSender::SendFeedbackPacket, which
+      // would also help the PacketRouter to select an appropriate rtp
+      // module in the case that some, but not all, have RTCP feedback
+      // enabled.
+      return PacketReceiver::DELIVERY_OK;
+    }
+    // Receive side bwe is not used for audio.
+    if (enable_receive_side_bwe_ ||
+        (stream->config.use_send_side_bwe &&
+         parsed_packet.HasExtension<TransportSequenceNumber>())) {
+      RTPHeader header;
+      parsed_packet.GetHeader(&header);
+
+      receive_side_cc_->OnReceivedPacket(
+          parsed_packet.arrival_time_ms(),
+          parsed_packet.payload_size() + parsed_packet.padding_size(), header);
+    }
+  }
+  return PacketReceiver::DELIVERY_OK;
+}
+
+//  static

[danilchap, 2017/04/19 15:45:25]

remove  this comment

[nisse-webrtc, 2017/04/20 10:53:38]

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.

+std::unique_ptr<RtpTransportControllerReceiveInterface>
+RtpTransportControllerReceiveInterface::Create(
+    ReceiveSideCongestionController* receive_side_cc,
+    bool enable_receive_side_bwe) {
+  return std::unique_ptr<RtpTransportControllerReceiveInterface>(
+      new RtpTransportControllerReceive(receive_side_cc,
+                                        enable_receive_side_bwe));
+}
+
+}  // namespace webrtc