Cast:Transport: Dividing A/V Initialization pipeline

media/cast/transport/rtp_sender/rtp_sender.cc

Index: media/cast/transport/rtp_sender/rtp_sender.cc
diff --git a/media/cast/transport/rtp_sender/rtp_sender.cc b/media/cast/transport/rtp_sender/rtp_sender.cc
index 7bcfbca36d282e2ba30d41c8a5ad7a33c4169a1d..c9e642d36713e73f5490437157012f6ac498b633 100644
--- a/media/cast/transport/rtp_sender/rtp_sender.cc
+++ b/media/cast/transport/rtp_sender/rtp_sender.cc
@@ -21,53 +21,55 @@ static const int kStatsCallbackIntervalMs = 33;
 
 RtpSender::RtpSender(
     base::TickClock* clock,
-    const CastTransportConfig& config,
-    bool is_audio,
-    const scoped_refptr<base::TaskRunner>& transport_task_runner,
+    const scoped_refptr<base::SingleThreadTaskRunner>& transport_task_runner,
     PacedSender* const transport)
-    : config_(),
+    : clock_(clock),
       transport_(transport),
       stats_callback_(),
-      transport_task_runner_(transport_task_runner) {
-  // Store generic cast config and create packetizer config.
-  if (is_audio) {
-    storage_.reset(
-        new PacketStorage(clock, config.audio_rtp_config.history_ms));
-    config_.audio = true;
-    config_.ssrc = config.audio_ssrc;
-    config_.payload_type = config.audio_rtp_config.payload_type;
-    config_.frequency = config.audio_frequency;
-    config_.audio_codec = config.audio_codec;
-  } else {
-    storage_.reset(
-        new PacketStorage(clock, config.audio_rtp_config.history_ms));
-    config_.audio = false;
-    config_.ssrc = config.video_ssrc;
-    config_.payload_type = config.video_rtp_config.payload_type;
-    config_.frequency = kVideoFrequency;
-    config_.video_codec = config.video_codec;
-  }
-  // Randomly set start values.
+      transport_task_runner_(transport_task_runner),
+      weak_factory_(this) {
+  // Randomly set sequence number start value.
   config_.sequence_number = base::RandInt(0, 65535);

[Ami GONE FROM CHROMIUM, 2014/02/25 00:22:17]

Does config_ still need to be a member variable?
(if it's important that the sequence_number used be the same for audio & video,
maybe make _that_ the member variable?)

As it is, it's very suspicious that Initialize{Audio,Video} both modify config_
in place, but their call order is arbitrary.

[mikhal1, 2014/02/25 18:07:02]

Audio and video will each have their own rtp_sender, which means their own
seq_number. Put it here, just to avoid the duplication.
On 2014/02/25 00:22:17, Ami Fischman wrote:

-  packetizer_.reset(
-      new RtpPacketizer(transport, storage_.get(), config_));
 }
 
 RtpSender::~RtpSender() {}
 
+void RtpSender::InitializeAudio(const CastTransportAudioConfig& config) {
+  storage_.reset(new PacketStorage(clock_, config.base.rtp_config.history_ms));
+  config_.audio = true;
+  config_.ssrc = config.base.ssrc;
+  config_.payload_type = config.base.rtp_config.payload_type;
+  config_.frequency = config.frequency;
+  config_.audio_codec = config.codec;
+  packetizer_.reset(new RtpPacketizer(transport_, storage_.get(), config_));
+}
+
+void RtpSender::InitializeVideo(const CastTransportVideoConfig& config) {
+  storage_.reset(new PacketStorage(clock_, config.base.rtp_config.history_ms));
+  config_.audio = false;
+  config_.ssrc = config.base.ssrc;
+  config_.payload_type = config.base.rtp_config.payload_type;
+  config_.frequency = kVideoFrequency;
+  config_.video_codec = config.codec;
+  packetizer_.reset(new RtpPacketizer(transport_, storage_.get(), config_));
+}
+
 void RtpSender::IncomingEncodedVideoFrame(const EncodedVideoFrame* video_frame,
                                           const base::TimeTicks& capture_time) {
+  DCHECK(packetizer_);
   packetizer_->IncomingEncodedVideoFrame(video_frame, capture_time);
 }
 
 void RtpSender::IncomingEncodedAudioFrame(
     const EncodedAudioFrame* audio_frame,
     const base::TimeTicks& recorded_time) {
+  DCHECK(packetizer_);
   packetizer_->IncomingEncodedAudioFrame(audio_frame, recorded_time);
 }
 
 void RtpSender::ResendPackets(
     const MissingFramesAndPacketsMap& missing_frames_and_packets) {
+  DCHECK(storage_);
   // Iterate over all frames in the list.
   for (MissingFramesAndPacketsMap::const_iterator it =
            missing_frames_and_packets.begin();
@@ -134,7 +136,7 @@ void RtpSender::SubscribeRtpStatsCallback(
 void RtpSender::ScheduleNextStatsReport() {
   transport_task_runner_->PostDelayedTask(
       FROM_HERE,
-      base::Bind(&RtpSender::RtpStatistics, base::AsWeakPtr(this)),
+      base::Bind(&RtpSender::RtpStatistics, weak_factory_.GetWeakPtr()),
       base::TimeDelta::FromMilliseconds(kStatsCallbackIntervalMs));
 }