Simplify interface for media/cast: CastTransportSenderImpl

chrome/browser/media/cast_transport_host_filter.cc

 // 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.
 
 #include "chrome/browser/media/cast_transport_host_filter.h"
 
 #include "base/thread_task_runner_handle.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/common/cast_messages.h"
 #include "components/net_log/chrome_net_log.h"
 #include "content/public/browser/power_save_blocker.h"
 #include "media/cast/net/cast_transport_sender.h"
 
 namespace {
 
 // How often to send raw events.
 const int kSendRawEventsIntervalSecs = 1;
 
+class TransportClient : public media::cast::CastTransportSender::Client {
+ public:
+  TransportClient(
+      int32 channel_id,
+      base::WeakPtr<cast::CastTransportHostFilter> cast_transport_host_filter)

[Irfan, 2015/12/16 22:37:04]

If you use WeakPtr, I think you will want to validate the pointer is valid
before using in the get() calls below.

OTOH, a raw pointer seems sufficient given that CastTransportHostFilter is
always expected to be valid during the life time of a TransportClient.

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

+1 on raw pointer. Looks like this class is ultimately owned by
CastTransportHostFilter.

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

Done. Use raw pointer.

+      : channel_id_(channel_id),
+        cast_transport_host_filter_(cast_transport_host_filter) {}
+
+  void OnStatusChange(media::cast::CastTransportStatus status) final;

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

nit on naming: I would probably name these OnStatusChanged,
OnLoggingEventsReceived, OnPacketsReceived.

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

Done.

+  void OnReceivedLoggingEvents(
+      scoped_ptr<std::vector<media::cast::FrameEvent>> frame_events,
+      scoped_ptr<std::vector<media::cast::PacketEvent>> packet_events) final;
+  void OnReceivedPackets(scoped_ptr<media::cast::Packet> packet) final;
+
+ private:
+  int32 channel_id_;

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

const int32 channel_id_;

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

Done.

+  base::WeakPtr<cast::CastTransportHostFilter> cast_transport_host_filter_;
+
+  DISALLOW_COPY_AND_ASSIGN(TransportClient);
+};
+
+void TransportClient::OnStatusChange(media::cast::CastTransportStatus status) {
+  cast_transport_host_filter_.get()->Send(
+      new CastMsg_NotifyStatusChange(channel_id_, status));
 }
 
+void TransportClient::OnReceivedLoggingEvents(
+    scoped_ptr<std::vector<media::cast::FrameEvent>> frame_events,
+    scoped_ptr<std::vector<media::cast::PacketEvent>> packet_events) {
+  if (frame_events->empty() && packet_events->empty())
+    return;
+  cast_transport_host_filter_.get()->Send(
+      new CastMsg_RawEvents(channel_id_, *packet_events, *frame_events));
+}
+
+void TransportClient::OnReceivedPackets(
+    scoped_ptr<media::cast::Packet> packet) {
+  cast_transport_host_filter_.get()->Send(
+      new CastMsg_ReceivedPacket(channel_id_, *packet));
+}
+
+}  // namespace
+
 namespace cast {
 
 CastTransportHostFilter::CastTransportHostFilter()
     : BrowserMessageFilter(CastMsgStart),
       weak_factory_(this) {}
 
 CastTransportHostFilter::~CastTransportHostFilter() {}
 
 bool CastTransportHostFilter::OnMessageReceived(const IPC::Message& message) {
   bool handled = true;
@@ skipping 11 matching lines @@
                         OnCancelSendingFrames)
     IPC_MESSAGE_HANDLER(CastHostMsg_AddValidSsrc,
                         OnAddValidSsrc)
     IPC_MESSAGE_HANDLER(CastHostMsg_SendRtcpFromRtpReceiver,
                         OnSendRtcpFromRtpReceiver)
     IPC_MESSAGE_UNHANDLED(handled = false);
   IPC_END_MESSAGE_MAP();
   return handled;
 }
 
-void CastTransportHostFilter::ReceivedPacket(
-    int32 channel_id,
-    scoped_ptr<media::cast::Packet> packet) {
-  Send(new CastMsg_ReceivedPacket(channel_id, *packet));
-}
-
-void CastTransportHostFilter::NotifyStatusChange(
-    int32 channel_id,
-    media::cast::CastTransportStatus status) {
-  Send(new CastMsg_NotifyStatusChange(channel_id, status));
-}
-
-void CastTransportHostFilter::SendRawEvents(
-    int32 channel_id,
-    scoped_ptr<std::vector<media::cast::FrameEvent>> frame_events,
-    scoped_ptr<std::vector<media::cast::PacketEvent>> packet_events) {
-  if (frame_events->empty() && packet_events->empty())
-    return;
-  Send(new CastMsg_RawEvents(channel_id, *packet_events, *frame_events));
-}
-
 void CastTransportHostFilter::SendRtt(int32 channel_id,
                                       uint32 ssrc,
                                       base::TimeDelta rtt) {
   Send(new CastMsg_Rtt(channel_id, ssrc, rtt));
 }
 
 void CastTransportHostFilter::SendCastMessage(
     int32 channel_id,
     uint32 ssrc,
     const media::cast::RtcpCastMessage& cast_message) {
@@ skipping 12 matching lines @@
         content::PowerSaveBlocker::Create(
             content::PowerSaveBlocker::kPowerSaveBlockPreventAppSuspension,
             content::PowerSaveBlocker::kReasonOther,
             "Cast is streaming content to a remote receiver").Pass();
   }
 
   if (id_map_.Lookup(channel_id)) {
     id_map_.Remove(channel_id);
   }
 
+  scoped_ptr<media::cast::CastTransportSender::Client> client(
+      new TransportClient(channel_id, weak_factory_.GetWeakPtr()));

[Irfan, 2015/12/16 22:37:04]

I think what we want to do is pass a raw pointer to CastTransportSender after
allocating on heap and have scoped_ptr be only created in CastTransportSender to
own the lifetime of that pointer.

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

This is correct but might not be good as commented somewhere else. I prefer pass
it separately as Derek suggested.

+  media::cast::CastTransportSender::CreateParams transport_params(
+      g_browser_process->net_log(), &clock_, local_end_point, remote_end_point,
+      client.Pass(), base::TimeDelta::FromSeconds(kSendRawEventsIntervalSecs),
+      make_scoped_ptr(options.DeepCopy()), base::ThreadTaskRunnerHandle::Get());
   scoped_ptr<media::cast::CastTransportSender> sender =
-      media::cast::CastTransportSender::Create(
-          g_browser_process->net_log(),
-          &clock_,
-          local_end_point,
-          remote_end_point,
-          make_scoped_ptr(options.DeepCopy()),
-          base::Bind(&CastTransportHostFilter::NotifyStatusChange,
-                     weak_factory_.GetWeakPtr(),
-                     channel_id),
-          base::Bind(&CastTransportHostFilter::SendRawEvents,
-                     weak_factory_.GetWeakPtr(),
-                     channel_id),
-          base::TimeDelta::FromSeconds(kSendRawEventsIntervalSecs),
-          base::Bind(&CastTransportHostFilter::ReceivedPacket,
-                     weak_factory_.GetWeakPtr(),
-                     channel_id),
-          base::ThreadTaskRunnerHandle::Get());
+      media::cast::CastTransportSender::Create(transport_params);
   id_map_.AddWithID(sender.release(), channel_id);
 }
 
 void CastTransportHostFilter::OnDelete(int32 channel_id) {
   media::cast::CastTransportSender* sender = id_map_.Lookup(channel_id);
   if (sender) {
     id_map_.Remove(channel_id);
   } else {
     DVLOG(1) << "CastTransportHostFilter::Delete called "
              << "on non-existing channel";
@@ skipping 125 matching lines @@
                                     params.rtp_receiver_statistics.get());
   } else {
     DVLOG(1)
         << "CastTransportHostFilter::OnSendRtcpFromRtpReceiver "
         << "on non-existing channel";
   }
 }
 
 
 }  // namespace cast