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:
@@ skipping 38 matching lines @@
 typedef base::Callback<void(CastTransportStatus status)>
     CastTransportStatusCallback;
 
 typedef base::Callback<void(scoped_ptr<std::vector<FrameEvent>>,
                             scoped_ptr<std::vector<PacketEvent>>)>
     BulkRawEventsCallback;
 
 // 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::TickClock* clock,
-      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,
-      base::TimeDelta raw_events_callback_interval,
-      const PacketReceiverCallback& packet_callback,
-      const scoped_refptr<base::SingleThreadTaskRunner>& transport_task_runner);
+  // Interface to create a cast transport sender / receiver.

[imcheng, 2015/12/17 00:18:10]

I don't think this comment is not accurate. If I understood correctly, this
interface is used for receiving status updates and/or out-of-band data (is this
accurate?) from CastTransportSender.

[xjz, 2015/12/17 22:54:57]

Done.

+  class Client {
+   public:
+    // Audio and Video transport status change is reported on this callback.
+    virtual void OnStatusChange(CastTransportStatus status) = 0;
+
+    // Raw events will be invoked on this callback every

[imcheng, 2015/12/17 00:18:10]

This comment should probably go in CreateParams. Also looks like the code checks
if it is greater than base::TimeDelta() rather than equals?

[xjz, 2015/12/17 22:54:58]

Add comments too in CreateParams. Yes, the code checks if it is greater than
|base::TimeDelta()|. Modified the comment.  

+    // |logging_flush_interval|. If the user is not interested in raw events,
+    // |logging_flush_interval| should be set to |base::TimeDelta|.

[imcheng, 2015/12/17 00:18:10]

s/base::TimeDelta/base::TimeDelta()

[xjz, 2015/12/17 22:54:57]

Done.

+    virtual void OnReceivedLoggingEvents(
+        scoped_ptr<std::vector<FrameEvent>>,

[imcheng, 2015/12/17 00:18:10]

Add names for the input args.

[xjz, 2015/12/17 22:54:57]

Done.

+        scoped_ptr<std::vector<PacketEvent>>) = 0;
+
+    // Incoming packets that do not match the channels created by
+    // Initialize{Audio,Video} will report to this callback.
+    virtual void OnReceivedPackets(scoped_ptr<Packet> packet) = 0;
+
+    virtual ~Client();

[imcheng, 2015/12/17 00:18:10]

nit: Place the destructor before the other pure virtual methods.

[xjz, 2015/12/17 22:54:57]

Done.

+  };
+
+  // Parameters to create a cast transport sender / receiver.
+  // |options| contains optional settings for the transport, possible
+  // keys are:
+  //   "DSCP" (value ignored)
+  //       - Turns DSCP on (higher IP Precedence and Type of Service).
+  //   "disable_non_blocking_io" (value ignored)
+  //       - Windows only.  Turns off non-blocking IO for the socket.
+  //         Note: Non-blocking IO is, by default, enabled on all platforms.
+  //   "pacer_target_burst_size": int
+  //        - Specifies how many packets to send per 10 ms ideally.
+  //   "pacer_max_burst_size": int
+  //        - Specifies how many pakcets to send per 10 ms, maximum.
+  //   "send_buffer_min_size": int
+  //        - Specifies the minimum socket send buffer size.
+  //   "disable_wifi_scan" (value ignored)
+  //        - Disable wifi scans while streaming.
+  //   "media_streaming_mode" (value ignored)
+  //        - Turn media streaming mode on.
+  // Note, these options may be ignored on some platforms.
+  struct CreateParams {
+    CreateParams(

[imcheng, 2015/12/17 00:18:10]

I feel some of these arguments do not belong in this struct. To me, it would
make more sense to have only "pure data" in this class - such as |local_addr|,
|remote_addr|, |transport_logging_flush_interval|, and |options|. The others are
pointers to other data structures that are probably fine to pass into
media::cast::CastTransportSender::Create().

[xjz, 2015/12/17 22:54:57]

There are only three pure data. |options| is also a pointer to other data
structure. I moved the |client| pointer out since it is the interface of all
callbacks. For other parameters, I think it is OK to put them inside this
structure since all of them are parameters used to create the cast transport. If
this structure is not preferred, I will prefer leave other parameters as they
are. It dose not sgtm to just group |local_addr|, |group_addr| and
|logging_flush_interval|.

+        net::NetLog* log,
+        base::TickClock* input_clock,
+        net::IPEndPoint local_addr,
+        net::IPEndPoint remote_addr,
+        scoped_ptr<Client> transport_client,
+        base::TimeDelta transport_logging_flush_interval,
+        scoped_ptr<base::DictionaryValue> options,
+        const scoped_refptr<base::SingleThreadTaskRunner>& task_runner);
+    ~CreateParams();
+
+    net::NetLog* net_log;
+    base::TickClock* clock;
+    net::IPEndPoint local_end_point;
+    net::IPEndPoint remote_end_point;
+    scoped_ptr<Client> client;
+    base::TimeDelta logging_flush_interval;
+    scoped_ptr<base::DictionaryValue> optional_config;

[imcheng, 2015/12/17 00:18:10]

If this is optional, can it be set via a setter() so there's one fewer input
argument in the constructor? It's up to you, because I do see there are several
places downstream where it is assumed to be non-null, and adding null checks may
not end up worth the trouble.

[xjz, 2015/12/17 22:54:57]

It is only used inside the constructor. I prefer leave it as an input parameter.

+    const scoped_refptr<base::SingleThreadTaskRunner>& transport_task_runner;

[imcheng, 2015/12/17 00:18:10]

Did you mean scoped_refptr<base::SingleThreadTaskRunner> transport_task_runner;
? Holding a reference in a member variable is generally a bad idea...

[xjz, 2015/12/17 22:54:57]

Done.

+  };
+
+  static scoped_ptr<CastTransportSender> Create(const CreateParams& params);
 
   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;
   virtual void InitializeVideo(const CastTransportRtpConfig& config,
@@ skipping 41 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_