Refactoring: CastTransport InitializeAudio/InitializeVideo.

Refactoring: CastTransport InitializeAudio/InitializeVideo.
Merge InitializeAudio/InitializeVideo to support multiple
audio/video streams in CastTransport.

BUG=618469
Committed: https://crrev.com/7c66f66ad930015ed019a3872419b5d890462bb0
Cr-Commit-Position: refs/heads/master@{#406655}

[xjz, 2016-06-08 23:07:37]

Patchset #1 (id:1) has been deleted

[xjz, 2016-06-08 23:31:41]

Description was changed from

==========
CastTransport refactoring.
Refactor CastTransport InitializeAudio/InitializeVideo to support
multi audio/video streams, which is required by MSE remoting changes.

BUG=618469
==========

to

==========
CastTransport refactoring.
Refactor CastTransport InitializeAudio/InitializeVideo to support
multi audio/video streams.

BUG=618469
==========

[xjz, 2016-06-08 23:31:41]

xjz@chromium.org changed reviewers:
+ miu@chromium.org

[xjz, 2016-06-08 23:34:32]

Description was changed from

==========
CastTransport refactoring.
Refactor CastTransport InitializeAudio/InitializeVideo to support
multi audio/video streams.

BUG=618469
==========

to

==========
Refactoring: CastTransport InitializeAudio/InitializeVideo.
Refactor InitializeAudio/InitializeVideo to support multiple 
audio/video streams in CastTransport.

BUG=618469
==========

[xjz, 2016-06-08 23:35:22]

PTAL

[miu, 2016-06-20 20:40:12]

The plan is to update this after the RtpPayloadType change, right? (to remove
the need to pass |is_audio| everywhere, iirc)

[xjz, 2016-06-20 20:44:33]

On 2016/06/20 20:40:12, miu wrote:
> The plan is to update this after the RtpPayloadType change, right? (to remove
> the need to pass |is_audio| everywhere, iirc)

Yes. I will rebase this CL after the RtpPayloadType change lands.

[xjz, 2016-07-02 00:35:19]

Rebased. PTAL

[miu, 2016-07-06 21:45:42]

Awesome! So much nicer now! :)

https://codereview.chromium.org/2048033003/diff/40001/media/cast/net/cast_tra...
File media/cast/net/cast_transport_impl.cc (right):

https://codereview.chromium.org/2048033003/diff/40001/media/cast/net/cast_tra...
media/cast/net/cast_transport_impl.cc:112: bool is_audio;
const please  ;)

https://codereview.chromium.org/2048033003/diff/40001/media/cast/net/cast_tra...
media/cast/net/cast_transport_impl.cc:323: for (auto it = sessions_.begin(); it
!= sessions_.end(); ++it) {
nit: for (const auto& e : sessions_) { if (e.second->rtcp_session->...

https://codereview.chromium.org/2048033003/diff/40001/media/cast/net/cast_tra...
media/cast/net/cast_transport_impl.cc:324: if (it->second->rtcp_session &&
IIUC, you don't need to null-check the rtcp_session member since it should
always be non-null if the entry exists in the sessions_ map.

https://codereview.chromium.org/2048033003/diff/40001/media/cast/net/cast_tra...
File media/cast/net/cast_transport_impl.h (right):

https://codereview.chromium.org/2048033003/diff/40001/media/cast/net/cast_tra...
media/cast/net/cast_transport_impl.h:183: // Records the initialized stream
sessions on RTP sender.
Looks like the key is the SSRC. Can you mention this in the comment?

https://codereview.chromium.org/2048033003/diff/40001/media/cast/net/pacing/p...
File media/cast/net/pacing/paced_sender.cc (right):

https://codereview.chromium.org/2048033003/diff/40001/media/cast/net/pacing/p...
media/cast/net/pacing/paced_sender.cc:56: int64_t last_byte_sent_for_audio;  //
Number of bytes sent to network from
Looks like this field is not being used anymore.

https://codereview.chromium.org/2048033003/diff/40001/media/cast/sender/audio...
File media/cast/sender/audio_sender.cc (right):

https://codereview.chromium.org/2048033003/diff/40001/media/cast/sender/audio...
media/cast/sender/audio_sender.cc:64: media::cast::CastTransportRtpConfig
transport_config;
Lines 64-73 could be moved into the FrameSender constructor. (They are identical
to lines 135-144 in video_sender.cc.)

https://codereview.chromium.org/2048033003/diff/40001/media/cast/test/cast_be...
File media/cast/test/cast_benchmarks.cc (right):

https://codereview.chromium.org/2048033003/diff/40001/media/cast/test/cast_be...
media/cast/test/cast_benchmarks.cc:102: audio_ssrc_ = config.ssrc;
Need to set video_ssrc_ instead, if this is video.

[xjz, 2016-07-08 23:09:34]

Patchset #3 (id:60001) has been deleted

[xjz, 2016-07-13 17:56:31]

The CQ bit was checked by xjz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-13 17:57:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xjz, 2016-07-13 18:23:57]

Description was changed from

==========
Refactoring: CastTransport InitializeAudio/InitializeVideo.
Refactor InitializeAudio/InitializeVideo to support multiple 
audio/video streams in CastTransport.

BUG=618469
==========

to

==========
Refactoring: CastTransport InitializeAudio/InitializeVideo.
Merge InitializeAudio/InitializeVideo to support multiple 
audio/video streams in CastTransport.

BUG=618469
==========

[xjz, 2016-07-13 18:23:57]

xjz@chromium.org changed reviewers:
+ dcheng@chromium.org

[commit-bot: I haz the power, 2016-07-13 18:40:54]

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

[commit-bot: I haz the power, 2016-07-13 18:40:55]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[xjz, 2016-07-13 19:00:26]

The CQ bit was checked by xjz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-13 19:02:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xjz, 2016-07-13 19:12:11]

Patchset #3 (id:80001) has been deleted

[commit-bot: I haz the power, 2016-07-13 20:25:49]

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

[commit-bot: I haz the power, 2016-07-13 20:25:51]

Dry run: This issue passed the CQ dry run.

[xjz, 2016-07-13 20:31:17]

Rebased and addressed comments. PTAL.

dcheng@: need RS on chrome/common/cast_messages.h. Thanks!

https://codereview.chromium.org/2048033003/diff/40001/media/cast/net/cast_tra...
File media/cast/net/cast_transport_impl.cc (right):

https://codereview.chromium.org/2048033003/diff/40001/media/cast/net/cast_tra...
media/cast/net/cast_transport_impl.cc:112: bool is_audio;
On 2016/07/06 21:45:42, miu wrote:
> const please  ;)

Done.

https://codereview.chromium.org/2048033003/diff/40001/media/cast/net/cast_tra...
media/cast/net/cast_transport_impl.cc:323: for (auto it = sessions_.begin(); it
!= sessions_.end(); ++it) {
On 2016/07/06 21:45:42, miu wrote:
> nit: for (const auto& e : sessions_) { if (e.second->rtcp_session->...

Done.

https://codereview.chromium.org/2048033003/diff/40001/media/cast/net/cast_tra...
media/cast/net/cast_transport_impl.cc:324: if (it->second->rtcp_session &&
On 2016/07/06 21:45:42, miu wrote:
> IIUC, you don't need to null-check the rtcp_session member since it should
> always be non-null if the entry exists in the sessions_ map.

Done.

https://codereview.chromium.org/2048033003/diff/40001/media/cast/net/cast_tra...
File media/cast/net/cast_transport_impl.h (right):

https://codereview.chromium.org/2048033003/diff/40001/media/cast/net/cast_tra...
media/cast/net/cast_transport_impl.h:183: // Records the initialized stream
sessions on RTP sender.
On 2016/07/06 21:45:42, miu wrote:
> Looks like the key is the SSRC. Can you mention this in the comment?

Done.

https://codereview.chromium.org/2048033003/diff/40001/media/cast/net/pacing/p...
File media/cast/net/pacing/paced_sender.cc (right):

https://codereview.chromium.org/2048033003/diff/40001/media/cast/net/pacing/p...
media/cast/net/pacing/paced_sender.cc:56: int64_t last_byte_sent_for_audio;  //
Number of bytes sent to network from
On 2016/07/06 21:45:42, miu wrote:
> Looks like this field is not being used anymore.

This is currently used for rejecting resending video packets if sender doen't
get the ACK of the sent audio packets.

https://codereview.chromium.org/2048033003/diff/40001/media/cast/sender/audio...
File media/cast/sender/audio_sender.cc (right):

https://codereview.chromium.org/2048033003/diff/40001/media/cast/sender/audio...
media/cast/sender/audio_sender.cc:64: media::cast::CastTransportRtpConfig
transport_config;
On 2016/07/06 21:45:42, miu wrote:
> Lines 64-73 could be moved into the FrameSender constructor. (They are
identical
> to lines 135-144 in video_sender.cc.)

Done.

https://codereview.chromium.org/2048033003/diff/40001/media/cast/test/cast_be...
File media/cast/test/cast_benchmarks.cc (right):

https://codereview.chromium.org/2048033003/diff/40001/media/cast/test/cast_be...
media/cast/test/cast_benchmarks.cc:102: audio_ssrc_ = config.ssrc;
On 2016/07/06 21:45:42, miu wrote:
> Need to set video_ssrc_ instead, if this is video.

Done.

[dcheng, 2016-07-14 03:40:27]

https://codereview.chromium.org/2048033003/diff/100001/media/cast/net/pacing/...
File media/cast/net/pacing/paced_sender.cc (right):

https://codereview.chromium.org/2048033003/diff/100001/media/cast/net/pacing/...
media/cast/net/pacing/paced_sender.cc:173: DCHECK(session_it !=
sessions_.end());
The inconsistent handling of ssrc here makes me nervous. Some things early
return and some DCHECK. It looks like this case is /probably/ OK, since
CastTransportImpl consults its own sessions_ map first... but I'm not really
sure. Are PacedSender::sessions_ and CastTransportImpl::sessions_ guaranteed to
be in sync?

https://codereview.chromium.org/2048033003/diff/100001/media/cast/sender/fram...
File media/cast/sender/frame_sender.cc (right):

https://codereview.chromium.org/2048033003/diff/100001/media/cast/sender/fram...
media/cast/sender/frame_sender.cc:99: transport_config, base::WrapUnique(new
FrameSender::RtcpClient(
Nit: base::MakeUnique<FrameSender::RtcpClient>(...)

[xjz, 2016-07-14 18:26:57]

Patchset #4 (id:120001) has been deleted

[xjz, 2016-07-14 18:29:57]

Thanks for the review. Addressed comments. PTAL.

https://codereview.chromium.org/2048033003/diff/100001/media/cast/net/pacing/...
File media/cast/net/pacing/paced_sender.cc (right):

https://codereview.chromium.org/2048033003/diff/100001/media/cast/net/pacing/...
media/cast/net/pacing/paced_sender.cc:173: DCHECK(session_it !=
sessions_.end());
On 2016/07/14 03:40:27, dcheng wrote:
> The inconsistent handling of ssrc here makes me nervous. Some things early
> return and some DCHECK. It looks like this case is /probably/ OK, since
> CastTransportImpl consults its own sessions_ map first... but I'm not really
> sure. Are PacedSender::sessions_ and CastTransportImpl::sessions_ guaranteed
to
> be in sync?

Yes, whenever a session for a RTP stream (with unique ssrc) is inserted in
CastTransportImpl::sessions_, it will be registered in PacedSender::sessions_ as
well. A session should be always registered in both sessions_ through
CastTransportImpl::InitializeStream() before any transmission starts.

https://codereview.chromium.org/2048033003/diff/100001/media/cast/sender/fram...
File media/cast/sender/frame_sender.cc (right):

https://codereview.chromium.org/2048033003/diff/100001/media/cast/sender/fram...
media/cast/sender/frame_sender.cc:99: transport_config, base::WrapUnique(new
FrameSender::RtcpClient(
On 2016/07/14 03:40:27, dcheng wrote:
> Nit: base::MakeUnique<FrameSender::RtcpClient>(...)

Done.

[miu, 2016-07-14 22:00:41]

https://codereview.chromium.org/2048033003/diff/140001/media/cast/net/cast_tr...
File media/cast/net/cast_transport.h (right):

https://codereview.chromium.org/2048033003/diff/140001/media/cast/net/cast_tr...
media/cast/net/cast_transport.h:88: virtual void
OnStatusChanged(CastTransportStatus status) = 0;
Just noticed this: I think you need to add a "ssrc" argument here. The impl of
this interface should be able to tell which stream is reporting on its status.
For example, if both audio and video streams are added, one of them could call
back with an INITIALIZED result while the other could call back with some other
error code.

https://codereview.chromium.org/2048033003/diff/140001/media/cast/net/cast_tr...
File media/cast/net/cast_transport_impl.cc (right):

https://codereview.chromium.org/2048033003/diff/140001/media/cast/net/cast_tr...
media/cast/net/cast_transport_impl.cc:191: sessions_[config.ssrc] =
std::move(session);
You've changed the order-of-operations here: Please move this statement one line
up (so it happens before the call to OnStatusChanged(INITIALIZED)).

[xjz, 2016-07-14 22:43:40]

https://codereview.chromium.org/2048033003/diff/140001/media/cast/net/cast_tr...
File media/cast/net/cast_transport.h (right):

https://codereview.chromium.org/2048033003/diff/140001/media/cast/net/cast_tr...
media/cast/net/cast_transport.h:88: virtual void
OnStatusChanged(CastTransportStatus status) = 0;
On 2016/07/14 22:00:41, miu wrote:
> Just noticed this: I think you need to add a "ssrc" argument here. The impl of
> this interface should be able to tell which stream is reporting on its status.
> For example, if both audio and video streams are added, one of them could call
> back with an INITIALIZED result while the other could call back with some
other
> error code.

It makes sense to add a "ssrc" argument for
TRANSPORT_STREAM_INITIALIZED/UNINITIALIZED status, but not for
TRANSPORT_SOCKET_ERROR status. Do you think it is good to still using the
original TRANSPORT_AUDIO/VIDEO_INITIALIZED/UNINITIALIZED status? In that way, we
can separate the audio/video status, but not for multiple audio (or video)
streams.

[dcheng, 2016-07-15 01:21:19]

https://codereview.chromium.org/2048033003/diff/100001/media/cast/net/pacing/...
File media/cast/net/pacing/paced_sender.cc (right):

https://codereview.chromium.org/2048033003/diff/100001/media/cast/net/pacing/...
media/cast/net/pacing/paced_sender.cc:173: DCHECK(session_it !=
sessions_.end());
On 2016/07/14 18:29:57, xjz wrote:
> On 2016/07/14 03:40:27, dcheng wrote:
> > The inconsistent handling of ssrc here makes me nervous. Some things early
> > return and some DCHECK. It looks like this case is /probably/ OK, since
> > CastTransportImpl consults its own sessions_ map first... but I'm not really
> > sure. Are PacedSender::sessions_ and CastTransportImpl::sessions_ guaranteed
> to
> > be in sync?
> 
> Yes, whenever a session for a RTP stream (with unique ssrc) is inserted in
> CastTransportImpl::sessions_, it will be registered in PacedSender::sessions_
as
> well. A session should be always registered in both sessions_ through
> CastTransportImpl::InitializeStream() before any transmission starts. 

Is it possible to be consistent about DCHECKing() through this file? Or are some
of these function not guarded by a sessions_ check in CastTransportImpl?

[miu, 2016-07-18 18:56:13]

On 2016/07/14 22:43:40, xjz wrote:
> It makes sense to add a "ssrc" argument for
> TRANSPORT_STREAM_INITIALIZED/UNINITIALIZED status, but not for
> TRANSPORT_SOCKET_ERROR status. Do you think it is good to still using the
> original TRANSPORT_AUDIO/VIDEO_INITIALIZED/UNINITIALIZED status? In that way,
we
> can separate the audio/video status, but not for multiple audio (or video)
> streams.

As discussed face-to-face, since the CastTransportStatus is either
transport-wide or per-stream, and the per-stream stuff is not used (except in
unit tests), then there's nothing you need to do here.

[xjz, 2016-07-18 20:21:58]

Addressed comments. PTAL.

https://codereview.chromium.org/2048033003/diff/100001/media/cast/net/pacing/...
File media/cast/net/pacing/paced_sender.cc (right):

https://codereview.chromium.org/2048033003/diff/100001/media/cast/net/pacing/...
media/cast/net/pacing/paced_sender.cc:173: DCHECK(session_it !=
sessions_.end());
On 2016/07/15 01:21:19, dcheng wrote:
> On 2016/07/14 18:29:57, xjz wrote:
> > On 2016/07/14 03:40:27, dcheng wrote:
> > > The inconsistent handling of ssrc here makes me nervous. Some things early
> > > return and some DCHECK. It looks like this case is /probably/ OK, since
> > > CastTransportImpl consults its own sessions_ map first... but I'm not
really
> > > sure. Are PacedSender::sessions_ and CastTransportImpl::sessions_
guaranteed
> > to
> > > be in sync?
> > 
> > Yes, whenever a session for a RTP stream (with unique ssrc) is inserted in
> > CastTransportImpl::sessions_, it will be registered in
PacedSender::sessions_
> as
> > well. A session should be always registered in both sessions_ through
> > CastTransportImpl::InitializeStream() before any transmission starts. 
> 
> Is it possible to be consistent about DCHECKing() through this file? Or are
some
> of these function not guarded by a sessions_ check in CastTransportImpl?

Done. Removed the DCHECK() since it should be guarded by the check in
CastTransportImpl.

https://codereview.chromium.org/2048033003/diff/140001/media/cast/net/cast_tr...
File media/cast/net/cast_transport_impl.cc (right):

https://codereview.chromium.org/2048033003/diff/140001/media/cast/net/cast_tr...
media/cast/net/cast_transport_impl.cc:191: sessions_[config.ssrc] =
std::move(session);
On 2016/07/14 22:00:41, miu wrote:
> You've changed the order-of-operations here: Please move this statement one
line
> up (so it happens before the call to OnStatusChanged(INITIALIZED)).

Done.

[miu, 2016-07-19 01:12:50]

lgtm

[dcheng, 2016-07-19 05:56:10]

https://codereview.chromium.org/2048033003/diff/100001/media/cast/net/pacing/...
File media/cast/net/pacing/paced_sender.cc (right):

https://codereview.chromium.org/2048033003/diff/100001/media/cast/net/pacing/...
media/cast/net/pacing/paced_sender.cc:173: DCHECK(session_it !=
sessions_.end());
On 2016/07/18 20:21:58, xjz wrote:
> On 2016/07/15 01:21:19, dcheng wrote:
> > On 2016/07/14 18:29:57, xjz wrote:
> > > On 2016/07/14 03:40:27, dcheng wrote:
> > > > The inconsistent handling of ssrc here makes me nervous. Some things
early
> > > > return and some DCHECK. It looks like this case is /probably/ OK, since
> > > > CastTransportImpl consults its own sessions_ map first... but I'm not
> really
> > > > sure. Are PacedSender::sessions_ and CastTransportImpl::sessions_
> guaranteed
> > > to
> > > > be in sync?
> > > 
> > > Yes, whenever a session for a RTP stream (with unique ssrc) is inserted in
> > > CastTransportImpl::sessions_, it will be registered in
> PacedSender::sessions_
> > as
> > > well. A session should be always registered in both sessions_ through
> > > CastTransportImpl::InitializeStream() before any transmission starts. 
> > 
> > Is it possible to be consistent about DCHECKing() through this file? Or are
> some
> > of these function not guarded by a sessions_ check in CastTransportImpl?
> 
> Done. Removed the DCHECK() since it should be guarded by the check in
> CastTransportImpl.

To be clear, I think it's better to have the DCHECK: it's a useful assert to
have to trigger if this precondition is ever violated (and it's useful for
documenting said precondition). I was just trying to better understand how this
is all handled, since some sessions_.find() instances are guarded with an if ()
check and some are not.

For example, in LogPacketEvent, we have this check. But it's not clear why. For
example, I think this function can get called indirectly via SendFrameAsPackets.
But the ssrc in there appears to be configured in
CastTransportImpl::Initialize... so how can it be out of sync? Maybe it can, and
I don't understand how, but this is an example of what I mean.

[xjz, 2016-07-19 18:18:33]

dcheng@: Thanks for the review. Addressed your comments. PTAL.

https://codereview.chromium.org/2048033003/diff/100001/media/cast/net/pacing/...
File media/cast/net/pacing/paced_sender.cc (right):

https://codereview.chromium.org/2048033003/diff/100001/media/cast/net/pacing/...
media/cast/net/pacing/paced_sender.cc:173: DCHECK(session_it !=
sessions_.end());
> To be clear, I think it's better to have the DCHECK: it's a useful assert to
> have to trigger if this precondition is ever violated (and it's useful for
> documenting said precondition). I was just trying to better understand how
this
> is all handled, since some sessions_.find() instances are guarded with an if
()
> check and some are not.
> 
> For example, in LogPacketEvent, we have this check. But it's not clear why.
For
> example, I think this function can get called indirectly via
SendFrameAsPackets.
> But the ssrc in there appears to be configured in
> CastTransportImpl::Initialize... so how can it be out of sync? Maybe it can,
and
> I don't understand how, but this is an example of what I mean.

Thanks for the clarification. Yes, the sessions should be already registered
through CastTransportImpl::InitializeStream(). Either DCHECK() or if() check is
only a guard for any possible future violation. I now use NOTREACHED() to
indicate that this should not happen.

[xjz, 2016-07-19 21:10:31]

The CQ bit was checked by xjz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-19 21:11:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xjz, 2016-07-19 21:41:56]

Patchset #6 (id:180001) has been deleted

[xjz, 2016-07-19 21:43:20]

dcheng@: Changed the guards back to DCHECK(). PTAL.

https://codereview.chromium.org/2048033003/diff/100001/media/cast/net/pacing/...
File media/cast/net/pacing/paced_sender.cc (right):

https://codereview.chromium.org/2048033003/diff/100001/media/cast/net/pacing/...
media/cast/net/pacing/paced_sender.cc:173: DCHECK(session_it !=
sessions_.end());
On 2016/07/19 18:18:33, xjz wrote:
> > To be clear, I think it's better to have the DCHECK: it's a useful assert to
> > have to trigger if this precondition is ever violated (and it's useful for
> > documenting said precondition). I was just trying to better understand how
> this
> > is all handled, since some sessions_.find() instances are guarded with an if
> ()
> > check and some are not.
> > 
> > For example, in LogPacketEvent, we have this check. But it's not clear why.
> For
> > example, I think this function can get called indirectly via
> SendFrameAsPackets.
> > But the ssrc in there appears to be configured in
> > CastTransportImpl::Initialize... so how can it be out of sync? Maybe it can,
> and
> > I don't understand how, but this is an example of what I mean.
> 
> Thanks for the clarification. Yes, the sessions should be already registered
> through CastTransportImpl::InitializeStream(). Either DCHECK() or if() check
is
> only a guard for any possible future violation. I now use NOTREACHED() to
> indicate that this should not happen. 

Changed it back to DCHECK() so that the code will crash if the condition is
violated (which should never happen).

[xjz, 2016-07-19 21:43:27]

The CQ bit was checked by xjz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-19 21:44:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-19 23:01:07]

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

[commit-bot: I haz the power, 2016-07-19 23:01:08]

Dry run: This issue passed the CQ dry run.

[dcheng, 2016-07-20 10:43:32]

My immediate comments have been addressed, but my larger point still stands:
it's very hard to follow and audit that every use of ssrc won't end up pointing
to an invalid iterator. It doesn't help that PacedSender has if () guards in
some places and DCHECKs() in others.

What is the relationship between these two classes? Is it 1:1? Are there other
entry points to PacedSender that come potentially come over IPC?

Basically, how is a reader suppose to figure out what's correct without trying
to trace through all the potential code paths?

[dcheng, 2016-07-20 17:43:40]

Per our discussion, LGTM. Some more comments might be good.

[xjz, 2016-07-20 18:04:45]

On 2016/07/20 10:43:32, dcheng wrote:
> My immediate comments have been addressed, but my larger point still stands:
> it's very hard to follow and audit that every use of ssrc won't end up
pointing
> to an invalid iterator. It doesn't help that PacedSender has if () guards in
> some places and DCHECKs() in others.
> 
> What is the relationship between these two classes? Is it 1:1? Are there other
> entry points to PacedSender that come potentially come over IPC?
> 
> Basically, how is a reader suppose to figure out what's correct without trying
> to trace through all the potential code paths?

The |sessions_| in PacedSender is synced with that in CastTransportImpl. 
Added some comments about this.

[xjz, 2016-07-20 18:05:39]

On 2016/07/20 17:43:40, dcheng wrote:
> Per our discussion, LGTM. Some more comments might be good.

Thanks! Added some comments.

[xjz, 2016-07-20 19:03:59]

The CQ bit was checked by xjz@chromium.org

[xjz, 2016-07-20 19:04:00]

The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2048033003/#ps220001
(title: "Add comments.")

[commit-bot: I haz the power, 2016-07-20 19:04:58]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-20 20:29:25]

Description was changed from

==========
Refactoring: CastTransport InitializeAudio/InitializeVideo.
Merge InitializeAudio/InitializeVideo to support multiple 
audio/video streams in CastTransport.

BUG=618469
==========

to

==========
Refactoring: CastTransport InitializeAudio/InitializeVideo.
Merge InitializeAudio/InitializeVideo to support multiple 
audio/video streams in CastTransport.

BUG=618469
==========

[commit-bot: I haz the power, 2016-07-20 20:29:27]

Committed patchset #7 (id:220001)

[commit-bot: I haz the power, 2016-07-20 20:31:23]

Description was changed from

==========
Refactoring: CastTransport InitializeAudio/InitializeVideo.
Merge InitializeAudio/InitializeVideo to support multiple 
audio/video streams in CastTransport.

BUG=618469
==========

to

==========
Refactoring: CastTransport InitializeAudio/InitializeVideo.
Merge InitializeAudio/InitializeVideo to support multiple
audio/video streams in CastTransport.

BUG=618469

Committed: https://crrev.com/7c66f66ad930015ed019a3872419b5d890462bb0
Cr-Commit-Position: refs/heads/master@{#406655}
==========

[commit-bot: I haz the power, 2016-07-20 20:31:25]

Patchset 7 (id:??) landed as
https://crrev.com/7c66f66ad930015ed019a3872419b5d890462bb0
Cr-Commit-Position: refs/heads/master@{#406655}