Simplify interface for media/cast: CastTransportSenderImpl

media/cast/net/cast_transport_sender.h

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // This is the main interface for the cast transport sender.  It accepts encoded
 // frames (both audio and video), encrypts their encoded data, packetizes them
 // and feeds them into a transport (e.g., UDP).
 
 // Construction of the Cast Sender and the Cast Transport Sender should be done
 // in the following order:
 // 1. Create CastTransportSender.
 // 2. Create CastSender (accepts CastTransportSender as an input).
 
 // Destruction: The CastTransportSender is assumed to be valid as long as the
 // CastSender is alive. Therefore the CastSender should be destructed before the
 // CastTransportSender.
 
 #ifndef MEDIA_CAST_NET_CAST_TRANSPORT_SENDER_H_
 #define MEDIA_CAST_NET_CAST_TRANSPORT_SENDER_H_
 
 #include "base/basictypes.h"
 #include "base/callback.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
 #include "base/single_thread_task_runner.h"
 #include "base/threading/non_thread_safe.h"
 #include "base/time/tick_clock.h"
 #include "media/cast/logging/logging_defines.h"
 #include "media/cast/net/cast_transport_config.h"
 #include "media/cast/net/cast_transport_defines.h"
 #include "media/cast/net/rtcp/receiver_rtcp_event_subscriber.h"
 #include "media/cast/net/rtcp/rtcp_defines.h"
 #include "net/base/ip_endpoint.h"
 
@@ skipping 12 matching lines @@
 
 // Following the initialization of either audio or video an initialization
 // status will be sent via this callback.
 typedef base::Callback<void(CastTransportStatus status)>
     CastTransportStatusCallback;
 
 typedef base::Callback<void(scoped_ptr<std::vector<FrameEvent>>,
                             scoped_ptr<std::vector<PacketEvent>>)>
     BulkRawEventsCallback;
 
+class CastTransportSenderInfo {
+ public:
+  CastTransportSenderInfo(){};
+
+  virtual void NotifyStatus(CastTransportStatus status) = 0;
+  virtual void SendRawEvents(scoped_ptr<std::vector<FrameEvent>>,
+                             scoped_ptr<std::vector<PacketEvent>>) = 0;
+  // Return true if the callback is provided and false otherwise.
+  virtual bool RecievedPackets(scoped_ptr<Packet> packet) = 0;
+  virtual net::NetLog* GetNetLog() = 0;
+  virtual net::IPEndPoint GetLocalEndPoint() = 0;
+  virtual net::IPEndPoint GetRemoteEndPoint() = 0;
+  // Return base::TimeDelta() if no raw events callback is provided.
+  virtual base::TimeDelta GetRawEventsCallBackInterval() = 0;
+  virtual scoped_ptr<base::DictionaryValue> GetOptions() = 0;
+};
+
 // The application should only trigger this class from the transport thread.
 class CastTransportSender : public base::NonThreadSafe {
  public:
   static scoped_ptr<CastTransportSender> Create(
-      net::NetLog* net_log,
+      base::WeakPtr<CastTransportSenderInfo> cast_transport_sender_info,
       base::TickClock* clock,

[Irfan, 2015/12/11 18:56:04]

Perhaps the UDP transport fields can be combined (local, remote, netlog &
transport options) separately.

Combining unrelated fields shifts the readability issue IMO

[miu, 2015/12/12 01:30:45]

Agreed.  Thinking about this, a structure I'd propose is:

class CastTransportSender {
 public:
  class Client {
   public:
    virtual void OnStatusChange(CastTransportStatus status) = 0;
    virtual void OnReceivedLoggingEvents(scoped_ptr<vector<FrameEvent>>,
                                         scoped_ptr<vector<PacketEvent>>) = 0;
    virtual bool OnReceviedPacket(scoped_ptr<Packet> packet) = 0;
   protected:
    virtual ~Client();
  };

  struct CreateParams {
    CreateParams();

    net::NetLog* net_log;
    base::TickClock* clock;
    net::IPEndPoint local_end_point;
    net::IPEndPoint remote_end_point;
    Client* client;
    base::TimeDelta logging_flush_interval;
    base::DictionaryValue optional_config;
  };

  static scoped_ptr<CastTransportSender> Create(const CreateParams& params);

  ...
};

The "CreateParams inner struct" pattern is used in a lot of other places in
Chromium where tons of arguments are passed-in to construct an instance. 
Example:
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...

Some of the weirdness here is around CastTransportSender being used for the cast
receiver as well.  So, this class is really a CastTransportReceiver too.  IMO,
this class (and its impl, and IPC messages, and so on) should probably be
renamed to just CastTransport to make it clearer that it's just a way of moving
things from one endpoint to the other.  That's probably something you'll want to
do in a separate change.

[xjz, 2015/12/15 18:40:13]

Thanks, Yuri! I'll take your suggestion.
Will do that after.

-      const net::IPEndPoint& local_end_point,
-      const net::IPEndPoint& remote_end_point,
-      scoped_ptr<base::DictionaryValue> options,
-      const CastTransportStatusCallback& status_callback,
-      const BulkRawEventsCallback& raw_events_callback,

[Irfan, 2015/12/11 17:33:25]

my feeling here is that only the callbacks should be combined as an interface
(status, packet reception, raw event logs). Combining everything makes it harder
to read.

[xjz, 2015/12/15 18:40:13]

Yuri suggested this too. Changed.

-      base::TimeDelta raw_events_callback_interval,
-      const PacketReceiverCallback& packet_callback,
       const scoped_refptr<base::SingleThreadTaskRunner>& transport_task_runner);
 
   virtual ~CastTransportSender() {}
 
   // Audio/Video initialization.
   // Encoded frames cannot be transmitted until the relevant initialize method
   // is called.
   virtual void InitializeAudio(const CastTransportRtpConfig& config,
                                const RtcpCastMessageCallback& cast_message_cb,
                                const RtcpRttCallback& rtt_cb) = 0;
@@ skipping 42 matching lines @@
       const RtcpCastMessage* cast_message,
       base::TimeDelta target_delay,
       const ReceiverRtcpEventSubscriber::RtcpEvents* rtcp_events,
       const RtpReceiverStatistics* rtp_receiver_statistics) = 0;
 };
 
 }  // namespace cast
 }  // namespace media
 
 #endif  // MEDIA_CAST_NET_CAST_TRANSPORT_SENDER_H_