Initial implementation of RtpTransportControllerReceive and related interfaces.

webrtc/call/rtp_transport_controller_receive.h

+/*
+ *  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.
+ */
+#ifndef WEBRTC_CALL_RTP_TRANSPORT_CONTROLLER_RECEIVE_H_
+#define WEBRTC_CALL_RTP_TRANSPORT_CONTROLLER_RECEIVE_H_
+
+#include "webrtc/base/array_view.h"
+
+// For MediaType and DeliveryStatus.
+// TODO(nisse): This file ought to not depend on call. We could move
+// MediaType definition here, and perhaps rename it to transport_id in
+// the process, since its main purpose is to disambiguate ssrc
+// collisions between transports.
+#include "webrtc/call/call.h"
+
+namespace webrtc {
+
+class RtpPacketReceived;
+
+// This class represents a receiver of already parsed RTP packets.
+class RtpPacketSinkInterface {
+ public:
+  virtual void OnRtpPacket(const RtpPacketReceived& packet) = 0;
+  virtual ~RtpPacketSinkInterface() {}
+};
+
+// This class represents a receiver of RTP packets, which has the
+// additional responsibility of identifying extension headers. I.e.,
+// the caller of OnRtpPacketReceive is expected to have called
+// packet->Parse, but not packet->IdentifyExtensions.
+class RtpPacketReceiverInterface {
+ public:
+  virtual bool OnRtpPacketReceive(RtpPacketReceived* packet) = 0;
+  virtual ~RtpPacketReceiverInterface() {}
+};

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

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.

+
+class RtpPacketObserverInterface;
+
+// This class represents the RTP receive demuxing. It isn't thread
+// aware, leaving responsibility of multithreading issues to the user
+// of this class.
+// TODO(nisse): Add RTCP processing, we should aim to terminate RTCP
+// and not leave any RTCP processing to individual receive streams.
+class RtpTransportControllerReceiveInterface {

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

Why not just call it RtpDemuxerInterface?

+ public:
+  // Configuration needed for media-independent processing of received
+  // packets, in particular, feeding information to the congestion
+  // controller.
+
+  // TODO(nisse): It turns out this data is only used for the
+  // RtpPacketObserverInterface callback. Since we have no interest in
+  // the details, make it a void* pointer or use a template parameter
+  // for the type?
+  struct Config {
+    // See draft-holmer-rmcat-transport-wide-cc-extensions for details.
+    bool use_send_side_bwe = false;
+  };
+
+  // The Register* functions return true if registration succeeds,
+  // false if the ssrc is already taken.
+
+  // Registers the receiver responsible for an ssrc. The |media_type|
+  // identifies the incoming transport, since we may use separate
+  // transport for audio and video, with independent ssrc spaces.

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

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.

+  virtual bool AddReceiver(uint32_t ssrc,
+                           MediaType media_type,
+                           const Config& config,
+                           RtpPacketReceiverInterface* receiver) = 0;
+  // TODO(nisse): It's unclear what info is conveniently available at
+  // remove time. For now, we take only the |receiver| pointer and
+  // iterate over the mapping.

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

The name here should match that for PayloadType below.  And should leave room
for MID.  

+  virtual bool RemoveReceiver(const RtpPacketReceiverInterface* receiver);
+
+  // Used to represent auxillary sinks, currently used for FlexFec.
+  // The responsible receiver must be registered first.
+  virtual bool AddSink(uint32_t ssrc,
+                       MediaType media_type,
+                       RtpPacketSinkInterface* sink) = 0;
+  // TODO(nisse): It's unclear what info is conveniently available at
+  // remove time. For now, we take only the |receiver| pointer and
+  // iterate over the mapping.
+  virtual bool RemoveSink(const RtpPacketSinkInterface* sink);
+
+#if 0
+  // TODO(nisse): Not yet implemented.
+  // Incoming packets with unknown ssrcs represent unsignalled
+  // streams. We dispatch on payload type and media type. The receiver
+  // will typically create a new RtpReceiver to pass the packet to. A
+  // true return value from the callback means that we should retry
+  // lookup.
+  virtual bool AddPayload(uint8_t payload_type, MediaType media_type,
+                          RtpPacketReceiverInterface *receiver) = 0;

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

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.

+#endif
+  // Process raw incoming RTP packets.
+  // TODO(nisse): DeliveryStatus is needed for the current handling of
+  // unsignalled ssrcs. Change return type to bool or void, once we do
+  // that via AddPayload instead.
+  virtual PacketReceiver::DeliveryStatus OnRtpPacket(
+      MediaType media_type,

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

How do you know the media type of an unparsed, undemuxed rtp packet?  and why do
you even need it to do demux?

+      rtc::ArrayView<const uint8_t> packet) = 0;

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

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.

+
+  virtual ~RtpTransportControllerReceiveInterface();
+
+  // Creates the default implementation.
+  static RtpTransportControllerReceiveInterface* Create(
+      RtpPacketObserverInterface* observer);
+};
+
+// Callback invoked for all processed RTP packets, used for feeding
+// the congestion controller (which is more tightly coupled to the
+// send side).

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

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)

+class RtpPacketObserverInterface {
+ public:
+  virtual void OnRtpPacket(
+      // TODO(nisse): Add media type / transport id to the RtpParsedPacket.
+      MediaType media_type,
+      const RtpTransportControllerReceiveInterface::Config,
+      const RtpPacketReceived& packet) = 0;
+  virtual ~RtpPacketObserverInterface();
+};
+
+}  // namespace webrtc
+
+#endif  // WEBRTC_CALL_RTP_TRANSPORT_CONTROLLER_RECEIVE_H_