Cast: Move retransmission to the transport

Cast: Move retransmission to the transport

This change is to move the logic of retransmission from AudioSender/
VideoSender to CastTransportSender.

With this change AudioSender and VideoSender will not handle packets any
more. They will simply see frames. Retransmission is now handled by
CastTransportSender.

Added two more public methods for CastTransportSender:
* CancelSendingFrames
  Cancel sending certain frames.
* SendFrameForKickstart
  Sends the last packet of a frame to kick start the session.

cast_simulator shows no regression.
BUG=393042
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=290094

[Alpha Left Google, 2014-08-11 18:53:32]

On 2014/08/08 06:11:54, Alpha wrote:

ping.

[hubbe, 2014-08-11 19:11:50]

A bunch of comments/questions.
This means the RTCP parser needs a security review, right?

https://codereview.chromium.org/445933002/diff/20001/media/cast/net/cast_tran...
File media/cast/net/cast_transport_sender_impl.cc (right):

https://codereview.chromium.org/445933002/diff/20001/media/cast/net/cast_tran...
media/cast/net/cast_transport_sender_impl.cc:148: new
Rtcp(base::Bind(&CastTransportSenderImpl::OnReceivedCastMessage,
Do we really want one rtcp object for audio and one for video?
Or should we have one rtcp object for all channels?

https://codereview.chromium.org/445933002/diff/20001/media/cast/net/cast_tran...
media/cast/net/cast_transport_sender_impl.cc:221: ResendPackets(ssrc,
missing_frames_and_packets, true, base::TimeDelta());
Why not call audio/video_sender->CancelSendingFrames() instead?

https://codereview.chromium.org/445933002/diff/20001/media/cast/net/cast_tran...
media/cast/net/cast_transport_sender_impl.cc:236: ResendPackets(ssrc,
missing_frames_and_packets, false, most_recent_rtt_);
Why not call audio/video_sender_->ResendFrameForKickStart() instead?

https://codereview.chromium.org/445933002/diff/20001/media/cast/net/cast_tran...
media/cast/net/cast_transport_sender_impl.cc:338: most_recent_rtt_ = rtt;
Seems like it would be cleaner to just have the rtcp object store this.

https://codereview.chromium.org/445933002/diff/20001/media/cast/net/cast_tran...
File media/cast/net/cast_transport_sender_impl.h (right):

https://codereview.chromium.org/445933002/diff/20001/media/cast/net/cast_tran...
media/cast/net/cast_transport_sender_impl.h:162: base::TimeDelta
most_recent_rtt_;
Given that audio packets are usually smaller than video packets, do we really
want to mix up their RTTs?

https://codereview.chromium.org/445933002/diff/20001/media/cast/sender/video_...
File media/cast/sender/video_sender.cc (right):

https://codereview.chromium.org/445933002/diff/20001/media/cast/sender/video_...
media/cast/sender/video_sender.cc:305: std::set<uint32> cancel_sending_frames;
Seems like a vector would be sufficient here.

[Alpha Left Google, 2014-08-11 21:03:18]

https://codereview.chromium.org/445933002/diff/20001/media/cast/net/cast_tran...
File media/cast/net/cast_transport_sender_impl.cc (right):

https://codereview.chromium.org/445933002/diff/20001/media/cast/net/cast_tran...
media/cast/net/cast_transport_sender_impl.cc:148: new
Rtcp(base::Bind(&CastTransportSenderImpl::OnReceivedCastMessage,
On 2014/08/11 19:11:49, hubbe wrote:
> Do we really want one rtcp object for audio and one for video?
> Or should we have one rtcp object for all channels?

Yes we do. RTCP object maintain some states internally. We could eventually
merge them but that's no the scope of this change.

https://codereview.chromium.org/445933002/diff/20001/media/cast/net/cast_tran...
media/cast/net/cast_transport_sender_impl.cc:221: ResendPackets(ssrc,
missing_frames_and_packets, true, base::TimeDelta());
On 2014/08/11 19:11:50, hubbe wrote:
> Why not call audio/video_sender->CancelSendingFrames() instead?

Done.

https://codereview.chromium.org/445933002/diff/20001/media/cast/net/cast_tran...
media/cast/net/cast_transport_sender_impl.cc:236: ResendPackets(ssrc,
missing_frames_and_packets, false, most_recent_rtt_);
On 2014/08/11 19:11:49, hubbe wrote:
> Why not call audio/video_sender_->ResendFrameForKickStart() instead?

Done.

https://codereview.chromium.org/445933002/diff/20001/media/cast/net/cast_tran...
media/cast/net/cast_transport_sender_impl.cc:338: most_recent_rtt_ = rtt;
On 2014/08/11 19:11:49, hubbe wrote:
> Seems like it would be cleaner to just have the rtcp object store this.

Done.

https://codereview.chromium.org/445933002/diff/20001/media/cast/net/cast_tran...
File media/cast/net/cast_transport_sender_impl.h (right):

https://codereview.chromium.org/445933002/diff/20001/media/cast/net/cast_tran...
media/cast/net/cast_transport_sender_impl.h:162: base::TimeDelta
most_recent_rtt_;
On 2014/08/11 19:11:50, hubbe wrote:
> Given that audio packets are usually smaller than video packets, do we really
> want to mix up their RTTs?

RTT is gathered by RTCP packets so it doesn't matter how large the RTP payload
packets are. And they are muxed to the same socket anyway.

In the latest patchset I'm using RTT from each RTCP estimate.

[Alpha Left Google, 2014-08-12 18:19:16]

Ping.

[hubbe, 2014-08-12 18:24:11]

https://codereview.chromium.org/445933002/diff/40001/media/cast/net/rtp/rtp_s...
File media/cast/net/rtp/rtp_sender.cc (right):

https://codereview.chromium.org/445933002/diff/40001/media/cast/net/rtp/rtp_s...
media/cast/net/rtp/rtp_sender.cc:128: ResendPackets(missing_frames_and_packets,
true, base::TimeDelta());
Why go through ResendPackets()?
Seems like you just need to call transport_->CancelSendingPacket() a few times
here...

[Alpha Left Google, 2014-08-12 23:57:39]

https://codereview.chromium.org/445933002/diff/40001/media/cast/net/rtp/rtp_s...
File media/cast/net/rtp/rtp_sender.cc (right):

https://codereview.chromium.org/445933002/diff/40001/media/cast/net/rtp/rtp_s...
media/cast/net/rtp/rtp_sender.cc:128: ResendPackets(missing_frames_and_packets,
true, base::TimeDelta());
On 2014/08/12 18:24:11, hubbe wrote:
> Why go through ResendPackets()?
> Seems like you just need to call transport_->CancelSendingPacket() a few times
> here...

Done.

[Alpha Left Google, 2014-08-14 18:36:11]

Ping.

[hubbe, 2014-08-14 20:17:48]

lgtm

[Alpha Left Google, 2014-08-14 20:31:56]

The CQ bit was checked by hclam@chromium.org

[commit-bot: I haz the power, 2014-08-14 20:33:16]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hclam@chromium.org/445933002/60001

[Alpha Left Google, 2014-08-14 21:34:29]

The CQ bit was checked by hclam@chromium.org

[commit-bot: I haz the power, 2014-08-14 21:35:57]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hclam@chromium.org/445933002/80001

[commit-bot: I haz the power, 2014-08-15 03:39:46]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-15 03:45:54]

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

[commit-bot: I haz the power, 2014-08-15 03:45:55]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Alpha Left Google, 2014-08-15 21:08:56]

+tsepez for IPC messages.

[Tom Sepez, 2014-08-15 22:45:08]

Messages LGTM.

[Alpha Left Google, 2014-08-15 23:15:21]

The CQ bit was checked by hclam@chromium.org

[commit-bot: I haz the power, 2014-08-15 23:18:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hclam@chromium.org/445933002/100001

[commit-bot: I haz the power, 2014-08-16 05:42:29]

Committed patchset #6 (100001) as 290094