Adding "adapter" ORTC objects on top of ChannelManager/BaseChannel/etc.

webrtc/ortc/rtptransportcontrollershim.h

Index: webrtc/ortc/rtptransportcontrollershim.h
diff --git a/webrtc/ortc/rtptransportcontrollershim.h b/webrtc/ortc/rtptransportcontrollershim.h
new file mode 100644
index 0000000000000000000000000000000000000000..6cec153159a23ce31ef281636bf838a1eb61cdbe
--- /dev/null
+++ b/webrtc/ortc/rtptransportcontrollershim.h
@@ -0,0 +1,187 @@
+/*
+ *  Copyright 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_ORTC_RTPTRANSPORTCONTROLLERSHIM_H_
+#define WEBRTC_ORTC_RTPTRANSPORTCONTROLLERSHIM_H_
+
+#include <memory>
+#include <string>
+#include <vector>
+
+#include "webrtc/base/constructormagic.h"
+#include "webrtc/base/thread.h"
+#include "webrtc/call/call.h"
+#include "webrtc/logging/rtc_event_log/rtc_event_log.h"
+#include "webrtc/api/ortc/rtptransportcontrollerinterface.h"
+#include "webrtc/pc/channelmanager.h"
+#include "webrtc/pc/mediacontroller.h"
+#include "webrtc/media/base/mediachannel.h"  // For MediaConfig.
+
+namespace webrtc {
+
+// Implementation of RtpTransportControllerInterface. Wraps a MediaController,
+// a VoiceChannel and VideoChannel, and maintains a list of dependent RTP
+// transports.
+//
+// When used along with an RtpSenderShim or RtpReceiverShim, the
+// sender/receiver passes its parameters along to this class, which turns them
+// into cricket:: media descriptions (the interface used by BaseChannel).
+//
+// Due to the fact that BaseChannel has different subclasses for audio/video,
+// the actual BaseChannel object is not created until an RtpSender/RtpReceiver
+// needs them.
+//
+// All methods should be called on the signaling thread.
+//
+// TODO(deadbeef): When BaseChannel is split apart into separate
+// "RtpSender"/"RtpTransceiver"/"RtpSender"/"RtpReceiver" objects, this shim
+// object can be replaced by a "real" one.
+class RtpTransportControllerShim : public RtpTransportControllerInterface {
+ public:
+  // Creates a proxy that will call "public interface" methods on the correct
+  // thread.
+  //
+  // Doesn't take ownership of any objects passed in.
+  //
+  // |channel_manager| must not be null.
+  static std::unique_ptr<RtpTransportControllerInterface> CreateProxied(
+      const cricket::MediaConfig& config,
+      cricket::ChannelManager* channel_manager,
+      webrtc::RtcEventLog* event_log,
+      rtc::Thread* signaling_thread,
+      rtc::Thread* worker_thread);
+
+  ~RtpTransportControllerShim() override;
+
+  // RtpTransportControllerInterface implementation.
+  std::vector<RtpTransportInterface*> GetTransports() const override;
+
+  // Methods used internally by RtpTransportShim.
+  MediaControllerInterface* media_controller() const {
+    return media_controller_.get();
+  }
+
+  rtc::Thread* signaling_thread() const { return signaling_thread_; }
+  rtc::Thread* worker_thread() const { return worker_thread_; }
+
+  // Doesn't take ownership.
+  //
+  // NOTE: "AddTransport" takes a proxy class, such that "GetTransports()" can
+  // return proxies, but the other methods take a pointer to the inner object,
+  // since these methods are called by the inner object which is unaware of the
+  // proxy.
+  void AddTransport(RtpTransportInterface* transport_proxy);
+  void RemoveTransport(RtpTransportInterface* inner_transport);

[pthatcher1, 2017/02/10 22:36:53]

If this has to be an "inner" and not a proxy, should we make the type make that
clear?

[Taylor Brandstetter, 2017/02/14 06:55:05]

Done.

+  RTCError SetRtcpParameters(const RtcpParameters& parameters,

[pthatcher1, 2017/02/10 22:36:53]

Why is it necessary to first AddTransport and then Attach it below, and then
Detach it and also Remove it?   Why not just have one Attach/Add/Set and one
Detach/Remove/Clear for each combination of (audio, video) and (sender,
receiver)?

[Taylor Brandstetter, 2017/02/14 06:55:05]

AddTransport occurs when the transport is created. Attach/Detach are when the
senders/receivers are created/destroyed.

They occur at different times so they can't be bunched together.

+                             RtpTransportInterface* inner_transport);
+
+  // Methods used by RtpSenderShim/RtpReceiverShim.
+  //
+  // AttachSender/AttachReceiver ensures only one sender/receiver shim per
+  // media type is trying to use this object simultaneously, and the
+  // sender/receiver for the same media type are using the same transport.
+  // That's all this class currently supports, due to limits of BaseChannel.
+  //
+  // The "Detach" methods will cause the corresponding parameters to be
+  // cleared, and will allow a different sender or receiver to be connected.
+  RTCError AttachAudioSender(RtpTransportInterface* inner_transport);
+  RTCError AttachVideoSender(RtpTransportInterface* inner_transport);
+  RTCError AttachAudioReceiver(RtpTransportInterface* inner_transport);
+  RTCError AttachVideoReceiver(RtpTransportInterface* inner_transport);

[pthatcher1, 2017/02/10 22:36:53]

It's weird that it's "Attach$Type$Actioner" when the type passed in is an
RtpTransport.  Should it be, instead "Set$Type$ActionTransport", such as
"SetAudioSendTransport"?  Similarly, below, should it be "Clear" instead of
"Detatch"?  

[Taylor Brandstetter, 2017/02/14 06:55:05]

I think you misunderstand how these are used; see rtpsenderadapter.cc for
example. Or we can talk when we're both in the office.

+
+  void DetachAudioSender();
+  void DetachVideoSender();
+  void DetachAudioReceiver();
+  void DetachVideoReceiver();
+
+  cricket::VoiceChannel* voice_channel() { return voice_channel_; }
+  cricket::VideoChannel* video_channel() { return video_channel_; }

[pthatcher1, 2017/02/10 22:36:53]

Why are these public?

[Taylor Brandstetter, 2017/02/14 06:55:06]

Because they're needed by RtpSenderAdapter/RtpReceiverAdapter.

+
+  // |primary_ssrc| out parameter is filled with either
+  // |parameters.encodings[0].ssrc|, or a generated SSRC if that's left unset.
+  RTCError ValidateAndApplyAudioSenderParameters(

[pthatcher1, 2017/02/10 22:36:53]

Why not just call it ApplyXParameters?  I think the facts that it validates
doesn't need to go in the name.

[Taylor Brandstetter, 2017/02/14 06:55:06]

I disagree. These parameters are passed through so many layers I found it useful
to clarify which one is validating them.

+      const RtpParameters& parameters,
+      uint32_t* primary_ssrc);
+  RTCError ValidateAndApplyVideoSenderParameters(
+      const RtpParameters& parameters,
+      uint32_t* primary_ssrc);
+  RTCError ValidateAndApplyAudioReceiverParameters(
+      const RtpParameters& parameters);
+  RTCError ValidateAndApplyVideoReceiverParameters(
+      const RtpParameters& parameters);
+
+ protected:
+  RtpTransportControllerShim* GetInternal() override { return this; }
+
+ private:
+  // Only expected to be called by RtpTransportControllerShim::CreateProxied.
+  RtpTransportControllerShim(const cricket::MediaConfig& config,
+                             cricket::ChannelManager* channel_manager,
+                             webrtc::RtcEventLog* event_log,
+                             rtc::Thread* signaling_thread,
+                             rtc::Thread* worker_thread);
+
+  void CreateVoiceChannel();
+  void CreateVideoChannel();
+  void DestroyVoiceChannel();
+  void DestroyVideoChannel();
+
+  void CopyRtcpParametersToDescriptions(
+      const RtcpParameters& params,
+      cricket::MediaContentDescription* local,
+      cricket::MediaContentDescription* remote);
+
+  // Helper function to generate an SSRC that doesn't match one in any of the
+  // "content description" structs, or in |new_params| (which is needed since
+  // multiple SSRCs may be gneerated in one go).
+  uint32_t GenerateUnusedSsrc(const cricket::StreamParams& new_params) const;
+
+  // |description| is the matching description where existing SSRCs can be
+  // found.
+  // This is a member function because it may need to generate SSRCs
+  // that don't match existing ones.
+  RTCError ValidateAndConvertSenderEncodings(
+      const std::vector<RtpEncodingParameters> encodings,
+      const std::string& cname,
+      const cricket::MediaContentDescription& description,
+      cricket::StreamParamsVec* cricket_streams,
+      bool* sending,
+      int* bandwidth) const;
+
+  rtc::Thread* signaling_thread_;
+  rtc::Thread* worker_thread_;
+  // |transport_proxies_| and |inner_audio_transport_|/|inner_audio_transport_|
+  // are somewhat redundant, but the latter are only set when
+  // RtpSenders/RtpReceivers are attached to the transport.
+  std::vector<RtpTransportInterface*> transport_proxies_;
+  RtpTransportInterface* inner_audio_transport_ = nullptr;
+  RtpTransportInterface* inner_video_transport_ = nullptr;
+  std::unique_ptr<MediaControllerInterface> media_controller_;
+
+  // BaseChannel takes content descriptions as input, so we store them here
+  // such that they can be updated when a new RtpSenderShim/RtpReceiverShim
+  // attaches itself.
+  cricket::AudioContentDescription local_audio_description_;
+  cricket::AudioContentDescription remote_audio_description_;
+  cricket::VideoContentDescription local_video_description_;
+  cricket::VideoContentDescription remote_video_description_;

[pthatcher1, 2017/02/10 22:36:53]

Aren't these store on the VoiceChannel and VideoChannel?  Can't we just use the
ones already in there?

[Taylor Brandstetter, 2017/02/14 06:55:05]

No and no.

+  cricket::VoiceChannel* voice_channel_ = nullptr;
+  cricket::VideoChannel* video_channel_ = nullptr;
+  bool have_audio_sender_ = false;
+  bool have_video_sender_ = false;
+  bool have_audio_receiver_ = false;
+  bool have_video_receiver_ = false;
+
+  RTC_DISALLOW_IMPLICIT_CONSTRUCTORS(RtpTransportControllerShim);
+};
+
+}  // namespace webrtc
+
+#endif  // WEBRTC_ORTC_RTPTRANSPORTCONTROLLERSHIM_H_