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

webrtc/ortc/rtptransportcontrolleradapter.h

+/*
+ *  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_RTPTRANSPORTCONTROLLERADAPTER_H_
+#define WEBRTC_ORTC_RTPTRANSPORTCONTROLLERADAPTER_H_
+
+#include <memory>
+#include <set>
+#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 {
+
+class RtpTransportAdapter;
+
+// Implementation of RtpTransportControllerInterface. Wraps a MediaController,
+// a VoiceChannel and VideoChannel, and maintains a list of dependent RTP
+// transports.
+//
+// When used along with an RtpSenderAdapter or RtpReceiverAdapter, 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 adapter
+// object can be replaced by a "real" one.
+class RtpTransportControllerAdapter : 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);
+
+  ~RtpTransportControllerAdapter() override;
+
+  // RtpTransportControllerInterface implementation.
+  std::vector<RtpTransportInterface*> GetTransports() const override;
+
+  // Methods used internally by RtpTransportAdapter.
+  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);

[pthatcher1, 2017/02/17 23:10:22]

Does anything call AddTransport other than the Transport itself?  In that case,
why not require an RtpTransportAdapter here?  Then at least it's symmetric with
RemoveTransport.

[Taylor Brandstetter, 2017/02/17 23:48:03]

Actually, it's RtpTransportAdapter::Create that ultimately will call this. Like
I mentioned, this CL isn't finished yet.

+  void RemoveTransport(RtpTransportAdapter* inner_transport);
+  RTCError SetRtcpParameters(const RtcpParameters& parameters,
+                             RtpTransportInterface* inner_transport);
+
+  // Methods used by RtpSenderAdapter/RtpReceiverAdapter.
+  //
+  // AttachSender/AttachReceiver ensures only one sender/receiver adapter 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);
+
+  void DetachAudioSender();
+  void DetachVideoSender();
+  void DetachAudioReceiver();
+  void DetachVideoReceiver();

[pthatcher1, 2017/02/17 23:10:22]

I still find it awkward to have a method AttachX(Y y) where X and Y are
completely different.

It seems what you are really doing is saying "Create a VideoChannel with this
transport if there isn't already one" and "Destroy a VideoChannel if I'm the
last one using it".  

Would it be better to do this?  

VoiceChannel* GetOrCreateVoiceChannel(RtpTransportInterface* transport);
VideoChannel* GetOrCreateVideoChannel(RtpTransportInterface* transport);
void DestroyVoiceChannel(VoiceChannel*);
void DestroyVideoChannel(VideoChannel*);

Then it would be almost exactly like TransportController::CreateDtlsTransport
and DestroyDtlsTransport.  Just refcount the number of times Create/Destroy is
called and create/destroy the channels on demand. 

Then you don't need voice_channel() and video_channel() either. 
 

[Taylor Brandstetter, 2017/02/17 23:48:03]

That doesn't work, because "AttachAudioSender" needs to fail if called twice
before "DetachAudioSender" is called.

There are certainly more elegant ways this could be implemented, with sigslots,
dependency injection, etc. But I went with what was simplest since this is
throwaway code and probably won't need to be changed before it's deleted. If you
think it's not sufficient, let's sit down and talk about it some time.

[pthatcher1, 2017/02/18 00:25:00]

I understand the "this is a temporary hack" part of it.  Perhaps if it were at
least called what it actually does, regardless of how hacky it is.  And what it
actually does is kind of like:

SetVoiceChannelNeededForSending()
SetVoiceChannelNeededForReceiving()
SetVideoChannelNeededForSending()
SetVideoChannelNeededForReceiving() 
ClearVoiceChannelNeededForSending()
ClearVoiceChannelNeededForReceiving()
ClearVideoChannelNeededForSending()
ClearVideoChannelNeededForReceiving()  

Which perhaps would be better just as:

// 0 => 1 causes voice_channel() to be non-null
// 1 => 0 causes voice_channel() to be null
IncrementNeedsForVoiceChannel();
DecrementNeedsForVoiceChannel();
// 0 => 1 causes video_channel() to be non-null
// 1 => 0 causes video_channel() to be null
IncrementNeedsForVideoChannel();
DecrementNeedsForVideoChannel();


The only thing left is the transport thing.  So here's my thinking on that:  can
we just do something hacky like always use the first transport as audio and the
second as video (or use the first as video if there is no second one)?  There
are only ever going to be 1 or 2 transports for a long time, especially since
this code is temporary.  

[Taylor Brandstetter, 2017/02/18 00:55:15]

It wouldn't make sense if I called "SetX" twice in a row and it failed though,
so that naming is more misleading in my opinion.
What's "the transport thing"? What's "the first transport"; the first transport
passed into what method?

Can we please talk about this in person like I asked? We aren't really getting
anywhere.

[Taylor Brandstetter, 2017/02/18 04:05:10]

I went ahead and implemented the sigslot thing (patchset #31) in the interest of
resolving this quickly. I could go a step further and use dependency injection,
but that would likely cause more complexity than it's worth.

+
+  cricket::VoiceChannel* voice_channel() { return voice_channel_; }
+  cricket::VideoChannel* video_channel() { return video_channel_; }
+
+  // |primary_ssrc| out parameter is filled with either
+  // |parameters.encodings[0].ssrc|, or a generated SSRC if that's left unset.
+  RTCError ValidateAndApplyAudioSenderParameters(
+      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:
+  RtpTransportControllerAdapter* GetInternal() override { return this; }
+
+ private:
+  // Only expected to be called by RtpTransportControllerAdapter::CreateProxied.
+  RtpTransportControllerAdapter(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_ssrcs| (which is needed since
+  // multiple SSRCs may be gneerated in one go).
+  uint32_t GenerateUnusedSsrc(std::set<uint32_t>* new_ssrcs) 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, which is more than ToStreamParamsVec does.
+  RTCErrorOr<cricket::StreamParamsVec> MakeStreamParamsVec(
+      std::vector<RtpEncodingParameters> encodings,
+      const std::string& cname,
+      const cricket::MediaContentDescription& description) 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 RtpSenderAdapter/
+  // RtpReceiverAdapter attaches itself.
+  cricket::AudioContentDescription local_audio_description_;
+  cricket::AudioContentDescription remote_audio_description_;
+  cricket::VideoContentDescription local_video_description_;
+  cricket::VideoContentDescription remote_video_description_;
+  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(RtpTransportControllerAdapter);
+};
+
+}  // namespace webrtc
+
+#endif  // WEBRTC_ORTC_RTPTRANSPORTCONTROLLERADAPTER_H_