Cast:Transport: Dividing A/V Initialization pipeline

Cast:Transport: Dividing A/V Initialization pipeline

This cl is part of the effort to split the A/V initialization pipeline,
which started in https://chromiumcodereview.appspot.com/163553006/.

Main changes in this cl:
1. Adding initialization functions to audio and video.
2. Changing the Create API to return a scoped ptr.
3. Using weak pointers in lieu of Unretained (part of BUG=344621
)

BUG=346822
This cl must be committed prior to issue  163553006

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=254615

[mikhal1, 2014-02-20 22:44:39]

Please review,
-Mikhal

[mikhal1, 2014-02-21 21:59:49]

ping

[hubbe, 2014-02-21 23:41:22]

https://codereview.chromium.org/174183003/diff/1/chrome/browser/media/cast_tr...
File chrome/browser/media/cast_transport_host_filter.cc (right):

https://codereview.chromium.org/174183003/diff/1/chrome/browser/media/cast_tr...
chrome/browser/media/cast_transport_host_filter.cc:72:
weak_factory_.GetWeakPtr(),
There should be no need for weak PTRs here as this object destroys all
transports when it is destroyed, and there should be no more callbacks after
that.

If there is something subtle her that I'm missing, we really need to document it
somewhere in some comments.

https://codereview.chromium.org/174183003/diff/1/chrome/renderer/media/cast_s...
File chrome/renderer/media/cast_session_delegate.cc (right):

https://codereview.chromium.org/174183003/diff/1/chrome/renderer/media/cast_s...
chrome/renderer/media/cast_session_delegate.cc:117:
weak_factory_.GetWeakPtr())));
How can we get callbacks if the CastTransportSenderIPC has been destroyed? Why
do we need weak pointers?

https://codereview.chromium.org/174183003/diff/1/media/cast/audio_sender/audi...
File media/cast/audio_sender/audio_sender.cc (right):

https://codereview.chromium.org/174183003/diff/1/media/cast/audio_sender/audi...
media/cast/audio_sender/audio_sender.cc:50: weak_factory_.GetWeakPtr()));
Here I can sort of understand the need for weak ptrs since this class doesn't
seem to own the transport, and thus cannot guarantee that the transport is
destroyed before this class.

https://codereview.chromium.org/174183003/diff/1/media/cast/transport/cast_tr...
File media/cast/transport/cast_transport_defines.h (right):

https://codereview.chromium.org/174183003/diff/1/media/cast/transport/cast_tr...
media/cast/transport/cast_transport_defines.h:21: TRANSPORT_AUDIO_UNINITIALIZED
= 0,
These transport statuses makes no sense.
If I call InitVideo and then InitAudio, but there is an error in the video
config, what will the status be?
Maybe add some comments for how to interpret these things?

https://codereview.chromium.org/174183003/diff/1/media/cast/transport/cast_tr...
File media/cast/transport/cast_transport_sender.h (right):

https://codereview.chromium.org/174183003/diff/1/media/cast/transport/cast_tr...
media/cast/transport/cast_transport_sender.h:60: virtual void
InitializeAudio(const CastTransportAudioConfig& config) = 0;
Can we have these two functions take a CastTransportRtpStatisticsCallback and
get rid of the two Subscribe* functions below?

[mikhal1, 2014-02-24 17:02:50]

+fischman
-Responding to review and rebasing

https://chromiumcodereview.appspot.com/174183003/diff/1/chrome/browser/media/...
File chrome/browser/media/cast_transport_host_filter.cc (right):

https://chromiumcodereview.appspot.com/174183003/diff/1/chrome/browser/media/...
chrome/browser/media/cast_transport_host_filter.cc:72:
weak_factory_.GetWeakPtr(),
On 2014/02/21 23:41:23, hubbe wrote:
> There should be no need for weak PTRs here as this object destroys all
> transports when it is destroyed, and there should be no more callbacks after
> that.
> 
> If there is something subtle her that I'm missing, we really need to document
it
> somewhere in some comments.

Done.

https://chromiumcodereview.appspot.com/174183003/diff/1/chrome/renderer/media...
File chrome/renderer/media/cast_session_delegate.cc (right):

https://chromiumcodereview.appspot.com/174183003/diff/1/chrome/renderer/media...
chrome/renderer/media/cast_session_delegate.cc:117:
weak_factory_.GetWeakPtr())));
Specifically here, probably not. But we already have the weak_ptr, so how is
this harmful?
On 2014/02/21 23:41:23, hubbe wrote:
> How can we get callbacks if the CastTransportSenderIPC has been destroyed? Why
> do we need weak pointers?
>

https://chromiumcodereview.appspot.com/174183003/diff/1/media/cast/transport/...
File media/cast/transport/cast_transport_defines.h (right):

https://chromiumcodereview.appspot.com/174183003/diff/1/media/cast/transport/...
media/cast/transport/cast_transport_defines.h:21: TRANSPORT_AUDIO_UNINITIALIZED
= 0,
Each initialization should trigger the callback with the relevant status. I
divided it to allow the application to distinguish between audio and video
(assuming they are using the callback for both).
Added a comment in cast_transport_sender.h
On 2014/02/21 23:41:23, hubbe wrote:
> These transport statuses makes no sense.
> If I call InitVideo and then InitAudio, but there is an error in the video
> config, what will the status be?
> Maybe add some comments for how to interpret these things?

https://chromiumcodereview.appspot.com/174183003/diff/1/media/cast/transport/...
File media/cast/transport/cast_transport_sender.h (right):

https://chromiumcodereview.appspot.com/174183003/diff/1/media/cast/transport/...
media/cast/transport/cast_transport_sender.h:60: virtual void
InitializeAudio(const CastTransportAudioConfig& config) = 0;
Sorry, but no.
These callbacks are called from the cast audio and video senders, which get the
initialized transport as an input. Perhaps this can be revisited, but not part
of this cl.
On 2014/02/21 23:41:23, hubbe wrote:
> Can we have these two functions take a CastTransportRtpStatisticsCallback and
> get rid of the two Subscribe* functions below?

[Ami GONE FROM CHROMIUM, 2014-02-25 00:22:16]

I don't know this code; comments are general-purpose.

https://chromiumcodereview.appspot.com/174183003/diff/120001/chrome/renderer/...
File chrome/renderer/media/cast_session_delegate.cc (right):

https://chromiumcodereview.appspot.com/174183003/diff/120001/chrome/renderer/...
chrome/renderer/media/cast_session_delegate.cc:110:
media::cast::transport::CastTransportVideoConfig video_config;
move decl's into respective code blocks where they are used below

https://chromiumcodereview.appspot.com/174183003/diff/120001/chrome/renderer/...
chrome/renderer/media/cast_session_delegate.cc:119: audio_config.base.ssrc =
audio_config_->sender_ssrc;
would it make more sense to have CastTransport{Audio,Video}Config ctors that
accept an {Audio,Video}SenderConfig?
Or simply have Initialize{Audio,Video} accept {Audio,Video}SenderConfig ?

https://chromiumcodereview.appspot.com/174183003/diff/120001/media/cast/trans...
File media/cast/transport/cast_transport_config.cc (right):

https://chromiumcodereview.appspot.com/174183003/diff/120001/media/cast/trans...
media/cast/transport/cast_transport_config.cc:22: : ssrc(0), aes_key(""),
aes_iv_mask("") {}
std::string's default-initialize to empty strings; you don't need to explicitly
construct them here.

https://chromiumcodereview.appspot.com/174183003/diff/120001/media/cast/trans...
File media/cast/transport/cast_transport_config.h (right):

https://chromiumcodereview.appspot.com/174183003/diff/120001/media/cast/trans...
media/cast/transport/cast_transport_config.h:50: uint32 ssrc;
is there any reason to support mutable config objects?  
It would be much cleaner to make all the members const and set them in the ctor,
which would also handily avoid the need to initialize members to arbitrary
defaults.  E.g. why does it make sense to set an audio codec to kOpus by
default?

https://chromiumcodereview.appspot.com/174183003/diff/120001/media/cast/trans...
File media/cast/transport/rtp_sender/rtp_sender.cc (right):

https://chromiumcodereview.appspot.com/174183003/diff/120001/media/cast/trans...
media/cast/transport/rtp_sender/rtp_sender.cc:32: config_.sequence_number =
base::RandInt(0, 65535);
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.

[hubbe, 2014-02-25 00:37:04]

https://chromiumcodereview.appspot.com/174183003/diff/1/chrome/renderer/media...
File chrome/renderer/media/cast_session_delegate.cc (right):

https://chromiumcodereview.appspot.com/174183003/diff/1/chrome/renderer/media...
chrome/renderer/media/cast_session_delegate.cc:117:
weak_factory_.GetWeakPtr())));
On 2014/02/24 17:02:50, mikhal1 wrote:
> Specifically here, probably not. But we already have the weak_ptr, so how is
> this harmful?
> On 2014/02/21 23:41:23, hubbe wrote:
> > How can we get callbacks if the CastTransportSenderIPC has been destroyed?
Why
> > do we need weak pointers?
> > 
> 

It is harmful in two ways:
1: There is extra overhead for the CPU when running the callback
2: Whenever you do something redundant, people who read the code will go: Why? 
(Extra overhead for code readers...)

https://chromiumcodereview.appspot.com/174183003/diff/1/media/cast/transport/...
File media/cast/transport/cast_transport_defines.h (right):

https://chromiumcodereview.appspot.com/174183003/diff/1/media/cast/transport/...
media/cast/transport/cast_transport_defines.h:21: TRANSPORT_AUDIO_UNINITIALIZED
= 0,
On 2014/02/24 17:02:50, mikhal1 wrote:
> Each initialization should trigger the callback with the relevant status. I
> divided it to allow the application to distinguish between audio and video
> (assuming they are using the callback for both).
> Added a comment in cast_transport_sender.h
> On 2014/02/21 23:41:23, hubbe wrote:
> > These transport statuses makes no sense.
> > If I call InitVideo and then InitAudio, but there is an error in the video
> > config, what will the status be?
> > Maybe add some comments for how to interpret these things?
> 

In that case, let's change the status callback to take two parameters, one which
specifies audio/video/other, and one which indicates the status.

https://chromiumcodereview.appspot.com/174183003/diff/1/media/cast/transport/...
File media/cast/transport/cast_transport_sender.h (right):

https://chromiumcodereview.appspot.com/174183003/diff/1/media/cast/transport/...
media/cast/transport/cast_transport_sender.h:60: virtual void
InitializeAudio(const CastTransportAudioConfig& config) = 0;
On 2014/02/24 17:02:50, mikhal1 wrote:
> Sorry, but no.
> These callbacks are called from the cast audio and video senders, which get
the
> initialized transport as an input. Perhaps this can be revisited, but not part
> of this cl.
> On 2014/02/21 23:41:23, hubbe wrote:
> > Can we have these two functions take a CastTransportRtpStatisticsCallback
and
> > get rid of the two Subscribe* functions below?
> 

Ugh, this is soooo messy, but I don't have a really good answer for how to make
it better either. :(

[Ami GONE FROM CHROMIUM, 2014-02-25 00:57:24]

https://chromiumcodereview.appspot.com/174183003/diff/1/chrome/renderer/media...
File chrome/renderer/media/cast_session_delegate.cc (right):

https://chromiumcodereview.appspot.com/174183003/diff/1/chrome/renderer/media...
chrome/renderer/media/cast_session_delegate.cc:117:
weak_factory_.GetWeakPtr())));
On 2014/02/25 00:37:04, hubbe wrote:
> On 2014/02/24 17:02:50, mikhal1 wrote:
> > Specifically here, probably not. But we already have the weak_ptr, so how is
> > this harmful?
> > On 2014/02/21 23:41:23, hubbe wrote:
> > > How can we get callbacks if the CastTransportSenderIPC has been destroyed?
> Why
> > > do we need weak pointers?
> > > 
> > 
> 
> It is harmful in two ways:
> 1: There is extra overhead for the CPU when running the callback
> 2: Whenever you do something redundant, people who read the code will go: Why?

> (Extra overhead for code readers...)
> 

FWIW I consider #1 above to be a non-issue, and #2 to be a huge issue.
If cast_transport_ is owner by |this| for its entire lifetime (which I believe
it is) and if cast_transport_ doesn't hand off its status_callback_ to anyone
else (which I don't think it can) and if all the dtors & callbacks in question
run on the same thread (which I think they do) using a base::Unretained(this) is
safe.  Best to include a comment explaining why that is.

[mikhal1, 2014-02-25 18:07:00]

PTAL

https://codereview.chromium.org/174183003/diff/1/chrome/renderer/media/cast_s...
File chrome/renderer/media/cast_session_delegate.cc (right):

https://codereview.chromium.org/174183003/diff/1/chrome/renderer/media/cast_s...
chrome/renderer/media/cast_session_delegate.cc:117:
weak_factory_.GetWeakPtr())));
Back to unretained, added a comment,
On 2014/02/25 00:57:25, Ami Fischman wrote:
> On 2014/02/25 00:37:04, hubbe wrote:
> > On 2014/02/24 17:02:50, mikhal1 wrote:
> > > Specifically here, probably not. But we already have the weak_ptr, so how
is
> > > this harmful?
> > > On 2014/02/21 23:41:23, hubbe wrote:
> > > > How can we get callbacks if the CastTransportSenderIPC has been
destroyed?
> > Why
> > > > do we need weak pointers?
> > > > 
> > > 
> > 
> > It is harmful in two ways:
> > 1: There is extra overhead for the CPU when running the callback
> > 2: Whenever you do something redundant, people who read the code will go:
Why?
> 
> > (Extra overhead for code readers...)
> > 
> 
> FWIW I consider #1 above to be a non-issue, and #2 to be a huge issue.
> If cast_transport_ is owner by |this| for its entire lifetime (which I believe
> it is) and if cast_transport_ doesn't hand off its status_callback_ to anyone
> else (which I don't think it can) and if all the dtors & callbacks in question
> run on the same thread (which I think they do) using a base::Unretained(this)
is
> safe.  Best to include a comment explaining why that is.

https://codereview.chromium.org/174183003/diff/1/media/cast/transport/cast_tr...
File media/cast/transport/cast_transport_defines.h (right):

https://codereview.chromium.org/174183003/diff/1/media/cast/transport/cast_tr...
media/cast/transport/cast_transport_defines.h:21: TRANSPORT_AUDIO_UNINITIALIZED
= 0,
On 2014/02/25 00:37:04, hubbe wrote:
> On 2014/02/24 17:02:50, mikhal1 wrote:
> > Each initialization should trigger the callback with the relevant status. I
> > divided it to allow the application to distinguish between audio and video
> > (assuming they are using the callback for both).
> > Added a comment in cast_transport_sender.h
> > On 2014/02/21 23:41:23, hubbe wrote:
> > > These transport statuses makes no sense.
> > > If I call InitVideo and then InitAudio, but there is an error in the video
> > > config, what will the status be?
> > > Maybe add some comments for how to interpret these things?
> > 
> 
> In that case, let's change the status callback to take two parameters, one
which
> specifies audio/video/other, and one which indicates the status.
> 
> 
I'm not sure that's much better, as there's only one transport.

https://codereview.chromium.org/174183003/diff/120001/chrome/renderer/media/c...
File chrome/renderer/media/cast_session_delegate.cc (right):

https://codereview.chromium.org/174183003/diff/120001/chrome/renderer/media/c...
chrome/renderer/media/cast_session_delegate.cc:110:
media::cast::transport::CastTransportVideoConfig video_config;
irrelevant
On 2014/02/25 00:22:17, Ami Fischman wrote:
> move decl's into respective code blocks where they are used below

https://codereview.chromium.org/174183003/diff/120001/chrome/renderer/media/c...
chrome/renderer/media/cast_session_delegate.cc:110:
media::cast::transport::CastTransportVideoConfig video_config;
On 2014/02/25 00:22:17, Ami Fischman wrote:
> move decl's into respective code blocks where they are used below

Done.

https://codereview.chromium.org/174183003/diff/120001/chrome/renderer/media/c...
chrome/renderer/media/cast_session_delegate.cc:119: audio_config.base.ssrc =
audio_config_->sender_ssrc;
Initially I thought it would, but in second thought, I'm not sure about it.
There are values like the encryption that aren't in the cast_sender config, and
the regular config has a lot of settings that aren't included here. Added a todo
for now.

On 2014/02/25 00:22:17, Ami Fischman wrote:
> would it make more sense to have CastTransport{Audio,Video}Config ctors that
> accept an {Audio,Video}SenderConfig?
> Or simply have Initialize{Audio,Video} accept {Audio,Video}SenderConfig ?

https://codereview.chromium.org/174183003/diff/120001/media/cast/transport/ca...
File media/cast/transport/cast_transport_config.cc (right):

https://codereview.chromium.org/174183003/diff/120001/media/cast/transport/ca...
media/cast/transport/cast_transport_config.cc:22: : ssrc(0), aes_key(""),
aes_iv_mask("") {}
On 2014/02/25 00:22:17, Ami Fischman wrote:
> std::string's default-initialize to empty strings; you don't need to
explicitly
> construct them here.

Done.

https://codereview.chromium.org/174183003/diff/120001/media/cast/transport/ca...
File media/cast/transport/cast_transport_config.h (right):

https://codereview.chromium.org/174183003/diff/120001/media/cast/transport/ca...
media/cast/transport/cast_transport_config.h:50: uint32 ssrc;
The rational is that most of the use cases are the same. It also makes it easier
for testing (but that might be not a good enough reason).
On 2014/02/25 00:22:17, Ami Fischman wrote:
> is there any reason to support mutable config objects?  
> It would be much cleaner to make all the members const and set them in the
ctor,
> which would also handily avoid the need to initialize members to arbitrary
> defaults.  E.g. why does it make sense to set an audio codec to kOpus by
> default?

https://codereview.chromium.org/174183003/diff/120001/media/cast/transport/rt...
File media/cast/transport/rtp_sender/rtp_sender.cc (right):

https://codereview.chromium.org/174183003/diff/120001/media/cast/transport/rt...
media/cast/transport/rtp_sender/rtp_sender.cc:32: config_.sequence_number =
base::RandInt(0, 65535);
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:
> 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.

[Ami GONE FROM CHROMIUM, 2014-02-25 18:35:08]

Once https://codereview.chromium.org/177113007/ lands you won't need my review,
so backing off.

https://codereview.chromium.org/174183003/diff/220001/chrome/renderer/media/c...
File chrome/renderer/media/cast_session_delegate.h (right):

https://codereview.chromium.org/174183003/diff/220001/chrome/renderer/media/c...
chrome/renderer/media/cast_session_delegate.h:28: class EncodingEventSubscriber;
I think you committed a classic blunder.
Please don't rebase while there are outstanding comments as it makes
reviewing inter-patchset diffs unnecessarily difficult.
(see the _Important_ note in
http://dev.chromium.org/developers/contributing-code#TOC-The-review-process)

https://codereview.chromium.org/174183003/diff/220001/media/cast/transport/ca...
File media/cast/transport/cast_transport_config.h (right):

https://codereview.chromium.org/174183003/diff/220001/media/cast/transport/ca...
media/cast/transport/cast_transport_config.h:51: uint32 ssrc;
On 2014/02/25 18:07:02, mikhal1 wrote:
> The rational is that most of the use cases are the same.

I don't understand this sentence.  Are you saying that most callers want the
same (default/zero) values?  If anything that's a reason to have an explicit
CreateDefaultConfig() factory method, not a reason to make state mutable.

> It also makes it easier
> for testing (but that might be not a good enough reason).

Ironically, it makes testing less relevant as well, b/c it makes it less likely
that you have good test coverage.

[hubbe, 2014-02-25 22:10:50]

LGTM
Please make sure you got all of Ami's comments addressed though.

[mikhal1, 2014-02-25 22:33:32]

https://codereview.chromium.org/174183003/diff/220001/chrome/renderer/media/c...
File chrome/renderer/media/cast_session_delegate.h (right):

https://codereview.chromium.org/174183003/diff/220001/chrome/renderer/media/c...
chrome/renderer/media/cast_session_delegate.h:28: class EncodingEventSubscriber;
You're right, sorry.
On 2014/02/25 18:35:08, Ami Fischman wrote:
> I think you committed a classic blunder.
> Please don't rebase while there are outstanding comments as it makes
> reviewing inter-patchset diffs unnecessarily difficult.
> (see the _Important_ note in
> http://dev.chromium.org/developers/contributing-code#TOC-The-review-process)

https://codereview.chromium.org/174183003/diff/220001/media/cast/transport/ca...
File media/cast/transport/cast_transport_config.h (right):

https://codereview.chromium.org/174183003/diff/220001/media/cast/transport/ca...
media/cast/transport/cast_transport_config.h:51: uint32 ssrc;
True. Will update that in a separate cl, as this has nothing to do with this cl.
On 2014/02/25 18:35:08, Ami Fischman wrote:
> On 2014/02/25 18:07:02, mikhal1 wrote:
> > The rational is that most of the use cases are the same.
> 
> I don't understand this sentence.  Are you saying that most callers want the
> same (default/zero) values?  If anything that's a reason to have an explicit
> CreateDefaultConfig() factory method, not a reason to make state mutable.
> 
> > It also makes it easier
> > for testing (but that might be not a good enough reason).
> 
> Ironically, it makes testing less relevant as well, b/c it makes it less
likely
> that you have good test coverage.

[mikhal1, 2014-02-25 22:33:46]

The CQ bit was checked by mikhal@chromium.org

[commit-bot: I haz the power, 2014-02-25 22:50:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mikhal@chromium.org/174183003/220001

[mikhal1, 2014-02-25 23:11:19]

The CQ bit was unchecked by mikhal@chromium.org

[mikhal1, 2014-02-25 23:19:37]

+palmer for chrome/common/cast_messages.h

[mikhal1, 2014-02-26 17:34:14]

Actually adding Palmer (cast_messages)

[palmer, 2014-02-26 23:02:07]

chrome/common/cast_messages.h LGTM.

[mikhal1, 2014-02-26 23:19:02]

The CQ bit was checked by mikhal@chromium.org

[commit-bot: I haz the power, 2014-02-26 23:19:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mikhal@chromium.org/174183003/190002

[commit-bot: I haz the power, 2014-02-26 23:19:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-26 23:19:43]

Failed to apply patch for chrome/browser/media/cast_transport_host_filter.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/media/cast_transport_host_filter.cc
  Hunk #1 FAILED at 7.
  Hunk #2 FAILED at 50.
  Hunk #3 succeeded at 108 (offset 5 lines).
  2 out of 3 hunks FAILED -- saving rejects to file
chrome/browser/media/cast_transport_host_filter.cc.rej

Patch:       chrome/browser/media/cast_transport_host_filter.cc
Index: chrome/browser/media/cast_transport_host_filter.cc
diff --git a/chrome/browser/media/cast_transport_host_filter.cc
b/chrome/browser/media/cast_transport_host_filter.cc
index
fe5ccacd41a32078a547185cc1627122ad3ff3ae..d39c51f89c4b01a2238c7cae7e7315982978ced1
100644
--- a/chrome/browser/media/cast_transport_host_filter.cc
+++ b/chrome/browser/media/cast_transport_host_filter.cc
@@ -7,11 +7,9 @@
 
 namespace cast {
 
-CastTransportHostFilter::CastTransportHostFilter() {
-}
+CastTransportHostFilter::CastTransportHostFilter() {}
 
-CastTransportHostFilter::~CastTransportHostFilter() {
-}
+CastTransportHostFilter::~CastTransportHostFilter() {}
 
 bool CastTransportHostFilter::OnMessageReceived(const IPC::Message& message,
                                                 bool* message_was_ok) {
@@ -50,46 +48,32 @@ void CastTransportHostFilter::ReceivedRtpStatistics(
     const media::cast::transport::RtcpSenderInfo& sender_info,
     base::TimeTicks time_sent,
     uint32 rtp_timestamp) {
-  Send(new CastMsg_RtpStatistics(channel_id,
-                                 audio,
-                                 sender_info,
-                                 time_sent,
-                                 rtp_timestamp));
+  Send(new CastMsg_RtpStatistics(
+      channel_id, audio, sender_info, time_sent, rtp_timestamp));
 }
 
-void CastTransportHostFilter::OnNew(
-    int32 channel_id,
-    const media::cast::transport::CastTransportConfig& config) {
-  media::cast::transport::CastTransportSender* sender =
-      id_map_.Lookup(channel_id);
-  if (sender) {
+void CastTransportHostFilter::OnNew(int32 channel_id,
+                                    const net::IPEndPoint& local_end_point,
+                                    const net::IPEndPoint& remote_end_point) {
+  if (id_map_.Lookup(channel_id)) {
     id_map_.Remove(channel_id);
   }
 
-  sender = media::cast::transport::CastTransportSender::
-      CreateCastTransportSender(
+  scoped_ptr<media::cast::transport::CastTransportSender> sender =
+      media::cast::transport::CastTransportSender::Create(
           &clock_,
-          config,
+          local_end_point,
+          remote_end_point,
           base::Bind(&CastTransportHostFilter::NotifyStatusChange,
                      base::Unretained(this),
                      channel_id),
           base::MessageLoopProxy::current());
-  sender->SetPacketReceiver(
-          base::Bind(&CastTransportHostFilter::ReceivedPacket,
-                     base::Unretained(this),
-                     channel_id));
-  sender->SubscribeAudioRtpStatsCallback(
-          base::Bind(&CastTransportHostFilter::ReceivedRtpStatistics,
-                     base::Unretained(this),
-                     channel_id,
-                     true  /* audio */));
-  sender->SubscribeVideoRtpStatsCallback(
-          base::Bind(&CastTransportHostFilter::ReceivedRtpStatistics,
-                     base::Unretained(this),
-                     channel_id,
-                     false /* not audio */));
 
-  id_map_.AddWithID(sender, channel_id);
+ 
sender->SetPacketReceiver(base::Bind(&CastTransportHostFilter::ReceivedPacket,
+                                       base::Unretained(this),
+                                       channel_id));
+
+  id_map_.AddWithID(sender.release(), channel_id);
 }
 
 void CastTransportHostFilter::OnDelete(int32 channel_id) {
@@ -103,6 +87,42 @@ void CastTransportHostFilter::OnDelete(int32 channel_id) {
   }
 }
 
+void CastTransportHostFilter::OnInitializeAudio(
+    int32 channel_id,
+    const media::cast::transport::CastTransportAudioConfig& config) {
+  media::cast::transport::CastTransportSender* sender =
+      id_map_.Lookup(channel_id);
+  if (sender) {
+    sender->InitializeAudio(config);
+    sender->SubscribeAudioRtpStatsCallback(
+        base::Bind(&CastTransportHostFilter::ReceivedRtpStatistics,
+                   base::Unretained(this),
+                   channel_id,
+                   true /* audio */));
+  } else {
+    DVLOG(1)
+        << "CastTransportHostFilter::OnInitializeAudio on non-existing
channel";
+  }
+}
+
+void CastTransportHostFilter::OnInitializeVideo(
+    int32 channel_id,
+    const media::cast::transport::CastTransportVideoConfig& config) {
+  media::cast::transport::CastTransportSender* sender =
+      id_map_.Lookup(channel_id);
+  if (sender) {
+    sender->InitializeVideo(config);
+    sender->SubscribeVideoRtpStatsCallback(
+        base::Bind(&CastTransportHostFilter::ReceivedRtpStatistics,
+                   base::Unretained(this),
+                   channel_id,
+                   false /* not audio */));
+  } else {
+    DVLOG(1)
+        << "CastTransportHostFilter::OnInitializeVideo on non-existing
channel";
+  }
+}
+
 void CastTransportHostFilter::OnInsertCodedAudioFrame(
     int32 channel_id,
     const media::cast::transport::EncodedAudioFrame& audio_frame,

[mikhal1, 2014-02-26 23:53:54]

The CQ bit was checked by mikhal@chromium.org

[commit-bot: I haz the power, 2014-02-26 23:56:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mikhal@chromium.org/174183003/250001

[commit-bot: I haz the power, 2014-02-27 01:31:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-27 01:31:40]

Retried try job too often on linux_chromeos for step(s) app_list_unittests,
ash_unittests, aura_unittests, base_unittests, browser_tests,
cacheinvalidation_unittests, check_deps, check_deps2git, chromeos_unittests,
components_unittests, content_browsertests, content_unittests, crypto_unittests,
dbus_unittests, device_unittests, events_unittests, google_apis_unittests,
gpu_unittests, interactive_ui_tests, ipc_tests, jingle_unittests,
keyboard_unittests, media_unittests, net_unittests, ppapi_unittests,
printing_unittests, sandbox_linux_unittests, sql_unittests, sync_unit_tests,
unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[mikhal1, 2014-02-27 17:07:53]

The CQ bit was checked by mikhal@chromium.org

[commit-bot: I haz the power, 2014-02-27 17:08:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mikhal@chromium.org/174183003/270001

[commit-bot: I haz the power, 2014-02-27 18:26:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-27 18:26:48]

Try jobs failed on following builders: linux_chromium_rel

[mikhal1, 2014-03-03 18:04:57]

The CQ bit was checked by mikhal@chromium.org

[commit-bot: I haz the power, 2014-03-03 18:05:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mikhal@chromium.org/174183003/290001

[commit-bot: I haz the power, 2014-03-03 18:31:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mikhal@chromium.org/174183003/290001

[commit-bot: I haz the power, 2014-03-03 19:59:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mikhal@chromium.org/174183003/290001

[commit-bot: I haz the power, 2014-03-04 00:26:14]

Change committed as 254615