Cast: Fix two video freezing problems

Cast: Fix two video freezing problems

1) The first one only happens when running UDP over localhost: The connect() call
   call fails and the sender stops listening to incoming packets. I fixed this by
   re-starting the packet listening if needed after each successfully sent packet.

2) The second problem is that the packet storage times out packets based on time,
   with the assumption that no packet should have to be re-sent if it's older than
   the max history time. However, if packets are not ACKed in a timely fashion, the
   video_sender may need to re-send arbitrarily old packets. To fix this, I change
   the packet storage to remember "max outanding frames" regardless of timing.

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

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

[Alpha Left Google, 2014-04-29 00:55:05]

https://codereview.chromium.org/252923007/diff/20001/media/cast/transport/cas...
File media/cast/transport/cast_transport_config.cc (right):

https://codereview.chromium.org/252923007/diff/20001/media/cast/transport/cas...
media/cast/transport/cast_transport_config.cc:24: : max_outstanding_frames(-1)
{}
Why not just 0?

https://codereview.chromium.org/252923007/diff/20001/media/cast/transport/cas...
File media/cast/transport/cast_transport_config.h (right):

https://codereview.chromium.org/252923007/diff/20001/media/cast/transport/cas...
media/cast/transport/cast_transport_config.h:41: int max_delay_ms;
Can we move this to VideoSenderConfig and AudioSenderConfig? CastTransport has
no use of this value at all.

https://codereview.chromium.org/252923007/diff/20001/media/cast/transport/cas...
media/cast/transport/cast_transport_config.h:47: struct TransportRtpConfig {
nit: This should be CastTransportRtpConfig to be consistent.

https://codereview.chromium.org/252923007/diff/20001/media/cast/transport/rtp...
File media/cast/transport/rtp_sender/packet_storage/packet_storage.cc (right):

https://codereview.chromium.org/252923007/diff/20001/media/cast/transport/rtp...
media/cast/transport/rtp_sender/packet_storage/packet_storage.cc:20:
DCHECK_LT(stored_frames, 250);
Please add comments to explain why. Particularly with the algorithm in
GetPacket().

https://codereview.chromium.org/252923007/diff/20001/media/cast/transport/rtp...
media/cast/transport/rtp_sender/packet_storage/packet_storage.cc:27: uint32
frame_to_remove = current_frame_id - stored_frames_;
Do we care about wrap around? For audio maybe?

https://codereview.chromium.org/252923007/diff/20001/media/cast/transport/rtp...
media/cast/transport/rtp_sender/packet_storage/packet_storage.cc:119: uint32
frame_id_32bit = (last_stored & ~0xFF) | frame_id_8bit;
Do you know if we have a utility method somewhere to do this conversion?

https://codereview.chromium.org/252923007/diff/20001/media/cast/transport/rtp...
File media/cast/transport/rtp_sender/packet_storage/packet_storage.h (right):

https://codereview.chromium.org/252923007/diff/20001/media/cast/transport/rtp...
media/cast/transport/rtp_sender/packet_storage/packet_storage.h:27: typedef
std::pair<uint32, uint16> StorageIndex;
Please document what these integers mean.

https://codereview.chromium.org/252923007/diff/20001/media/cast/transport/rtp...
media/cast/transport/rtp_sender/packet_storage/packet_storage.h:32:
PacketStorage(int stored_frames);
uint32 to be consistent with |stored_frames_|.

https://codereview.chromium.org/252923007/diff/20001/media/cast/transport/rtp...
media/cast/transport/rtp_sender/packet_storage/packet_storage.h:46: bool
GetPacket32(uint32 frame_id,
Please document the difference between GetPacket32 and GetPacket.

https://codereview.chromium.org/252923007/diff/20001/media/cast/transport/rtp...
media/cast/transport/rtp_sender/packet_storage/packet_storage.h:46: bool
GetPacket32(uint32 frame_id,
Also it seems like this should be a private method.

https://codereview.chromium.org/252923007/diff/20001/media/cast/transport/tra...
File media/cast/transport/transport/udp_transport.cc (right):

https://codereview.chromium.org/252923007/diff/20001/media/cast/transport/tra...
media/cast/transport/transport/udp_transport.cc:129: << length_or_status << ". 
Stop receiving packets.";
"Stop receiving packets." is not true. Please fix the comments.

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

https://codereview.chromium.org/252923007/diff/20001/media/cast/video_sender/...
media/cast/video_sender/video_sender.cc:312: // Last frame acked, no point in
doing anything
Please have a test for this case. This is a third bug fixed in this change. This
means that all frames are acked and sender is idle, make sure we don't resend
the last frame over and over.

[hubbe, 2014-04-29 17:19:58]

https://codereview.chromium.org/252923007/diff/20001/media/cast/transport/cas...
File media/cast/transport/cast_transport_config.cc (right):

https://codereview.chromium.org/252923007/diff/20001/media/cast/transport/cas...
media/cast/transport/cast_transport_config.cc:24: : max_outstanding_frames(-1)
{}
On 2014/04/29 00:55:05, Alpha wrote:
> Why not just 0?

Because I want it to be an error to not set this.

https://codereview.chromium.org/252923007/diff/20001/media/cast/transport/cas...
File media/cast/transport/cast_transport_config.h (right):

https://codereview.chromium.org/252923007/diff/20001/media/cast/transport/cas...
media/cast/transport/cast_transport_config.h:41: int max_delay_ms;
On 2014/04/29 00:55:05, Alpha wrote:
> Can we move this to VideoSenderConfig and AudioSenderConfig? CastTransport has
> no use of this value at all.

I'd prefer to do that in a separate CL.
I can add a TODO if you like.

https://codereview.chromium.org/252923007/diff/20001/media/cast/transport/cas...
media/cast/transport/cast_transport_config.h:47: struct TransportRtpConfig {
On 2014/04/29 00:55:05, Alpha wrote:
> nit: This should be CastTransportRtpConfig to be consistent.

Done.

https://codereview.chromium.org/252923007/diff/20001/media/cast/transport/rtp...
File media/cast/transport/rtp_sender/packet_storage/packet_storage.cc (right):

https://codereview.chromium.org/252923007/diff/20001/media/cast/transport/rtp...
media/cast/transport/rtp_sender/packet_storage/packet_storage.cc:20:
DCHECK_LT(stored_frames, 250);
On 2014/04/29 00:55:05, Alpha wrote:
> Please add comments to explain why. Particularly with the algorithm in
> GetPacket().

Done.

https://codereview.chromium.org/252923007/diff/20001/media/cast/transport/rtp...
media/cast/transport/rtp_sender/packet_storage/packet_storage.cc:27: uint32
frame_to_remove = current_frame_id - stored_frames_;
On 2014/04/29 00:55:05, Alpha wrote:
> Do we care about wrap around? For audio maybe?

We do. That's why I use IsOlderFrameId()

https://codereview.chromium.org/252923007/diff/20001/media/cast/transport/rtp...
media/cast/transport/rtp_sender/packet_storage/packet_storage.cc:119: uint32
frame_id_32bit = (last_stored & ~0xFF) | frame_id_8bit;
On 2014/04/29 00:55:05, Alpha wrote:
> Do you know if we have a utility method somewhere to do this conversion?

No idea.
The obvious place for it (cast_defines.h) didn't contain one.

https://codereview.chromium.org/252923007/diff/20001/media/cast/transport/rtp...
File media/cast/transport/rtp_sender/packet_storage/packet_storage.h (right):

https://codereview.chromium.org/252923007/diff/20001/media/cast/transport/rtp...
media/cast/transport/rtp_sender/packet_storage/packet_storage.h:27: typedef
std::pair<uint32, uint16> StorageIndex;
On 2014/04/29 00:55:05, Alpha wrote:
> Please document what these integers mean.

Done.

https://codereview.chromium.org/252923007/diff/20001/media/cast/transport/rtp...
media/cast/transport/rtp_sender/packet_storage/packet_storage.h:32:
PacketStorage(int stored_frames);
On 2014/04/29 00:55:05, Alpha wrote:
> uint32 to be consistent with |stored_frames_|.

It's an int in the rtp config.

https://codereview.chromium.org/252923007/diff/20001/media/cast/transport/rtp...
media/cast/transport/rtp_sender/packet_storage/packet_storage.h:46: bool
GetPacket32(uint32 frame_id,
On 2014/04/29 00:55:05, Alpha wrote:
> Also it seems like this should be a private method.

Done.

https://codereview.chromium.org/252923007/diff/20001/media/cast/transport/rtp...
media/cast/transport/rtp_sender/packet_storage/packet_storage.h:46: bool
GetPacket32(uint32 frame_id,
On 2014/04/29 00:55:05, Alpha wrote:
> Also it seems like this should be a private method.

Done.

https://codereview.chromium.org/252923007/diff/20001/media/cast/transport/tra...
File media/cast/transport/transport/udp_transport.cc (right):

https://codereview.chromium.org/252923007/diff/20001/media/cast/transport/tra...
media/cast/transport/transport/udp_transport.cc:129: << length_or_status << ". 
Stop receiving packets.";
On 2014/04/29 00:55:05, Alpha wrote:
> "Stop receiving packets." is not true. Please fix the comments.
> 

Done.

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

https://codereview.chromium.org/252923007/diff/20001/media/cast/video_sender/...
media/cast/video_sender/video_sender.cc:312: // Last frame acked, no point in
doing anything
On 2014/04/29 00:55:05, Alpha wrote:
> Please have a test for this case. This is a third bug fixed in this change.
This
> means that all frames are acked and sender is idle, make sure we don't resend
> the last frame over and over.

Actually, the problem is that without this if-statement, it tries to resend the
last_acked_frame + 1 (a frame which doesn't exist yet). When that happens new
DCHECKS are triggered in packet_storage.cc, so no test is actually required.

[hubbe, 2014-04-29 17:21:27]

Adding palmer for cast_messages.h

[palmer, 2014-04-29 17:33:43]

https://codereview.chromium.org/252923007/diff/40001/media/cast/cast_config.h
File media/cast/cast_config.h (right):

https://codereview.chromium.org/252923007/diff/40001/media/cast/cast_config.h...
media/cast/cast_config.h:33: // uint32 sender_ssrc;  use rtp_config.ssrc instead
Probably better to delete this now-dead code than to comment it out?

https://codereview.chromium.org/252923007/diff/40001/media/cast/cast_config.h...
media/cast/cast_config.h:52: // uint32 sender_ssrc;  use rtp_config.ssrc instead
Same thing here.

https://codereview.chromium.org/252923007/diff/40001/media/cast/transport/rtp...
File media/cast/transport/rtp_sender/packet_storage/packet_storage.cc (right):

https://codereview.chromium.org/252923007/diff/40001/media/cast/transport/rtp...
media/cast/transport/rtp_sender/packet_storage/packet_storage.cc:21: // no
sense. Let's limit it to 250, to avoid accidental wrap-arounds.
Why 250?

Wouldn't it be better to declare |stored_frames| as a uint8? Then it would hold
all and only the correct values, with no need for DCHECKs.

https://codereview.chromium.org/252923007/diff/40001/media/cast/transport/rtp...
media/cast/transport/rtp_sender/packet_storage/packet_storage.cc:115: bool
PacketStorage::GetPacket(uint8 frame_id_8bit,
Like here. (Nit: I'd argue that adding "_8bit" is redundant, since it's declared
as a uint8.)

https://codereview.chromium.org/252923007/diff/40001/media/cast/transport/rtp...
File media/cast/transport/rtp_sender/packet_storage/packet_storage_unittest.cc
(left):

https://codereview.chromium.org/252923007/diff/40001/media/cast/transport/rtp...
media/cast/transport/rtp_sender/packet_storage/packet_storage_unittest.cc:70:
TEST_F(PacketStorageTest, MaxNumberOfPackets) {
Isn't some test along these lines still a good idea?

[hubbe, 2014-04-29 18:32:38]

https://codereview.chromium.org/252923007/diff/40001/media/cast/cast_config.h
File media/cast/cast_config.h (right):

https://codereview.chromium.org/252923007/diff/40001/media/cast/cast_config.h...
media/cast/cast_config.h:33: // uint32 sender_ssrc;  use rtp_config.ssrc instead
On 2014/04/29 17:33:44, Chromium Palmer wrote:
> Probably better to delete this now-dead code than to comment it out?

It was meant as documentation. Better comment written.

https://codereview.chromium.org/252923007/diff/40001/media/cast/cast_config.h...
media/cast/cast_config.h:52: // uint32 sender_ssrc;  use rtp_config.ssrc instead
On 2014/04/29 17:33:44, Chromium Palmer wrote:
> Same thing here.

Done.

https://codereview.chromium.org/252923007/diff/40001/media/cast/transport/rtp...
File media/cast/transport/rtp_sender/packet_storage/packet_storage.cc (right):

https://codereview.chromium.org/252923007/diff/40001/media/cast/transport/rtp...
media/cast/transport/rtp_sender/packet_storage/packet_storage.cc:21: // no
sense. Let's limit it to 250, to avoid accidental wrap-arounds.
On 2014/04/29 17:33:44, Chromium Palmer wrote:
> Why 250?
> 
> Wouldn't it be better to declare |stored_frames| as a uint8? Then it would
hold
> all and only the correct values, with no need for DCHECKs.

I can do that, but I really want at least one illegal value so that I can
initialize the configuration struct to that illegal value and  generate an error
if it's not set correctly.  -1 seems like a better illegal value than 255.

https://codereview.chromium.org/252923007/diff/40001/media/cast/transport/rtp...
media/cast/transport/rtp_sender/packet_storage/packet_storage.cc:115: bool
PacketStorage::GetPacket(uint8 frame_id_8bit,
On 2014/04/29 17:33:44, Chromium Palmer wrote:
> Like here. (Nit: I'd argue that adding "_8bit" is redundant, since it's
declared
> as a uint8.)

I did that to distinguish it from "frame_id_32bit" which is a local variable.

https://codereview.chromium.org/252923007/diff/40001/media/cast/transport/rtp...
File media/cast/transport/rtp_sender/packet_storage/packet_storage_unittest.cc
(left):

https://codereview.chromium.org/252923007/diff/40001/media/cast/transport/rtp...
media/cast/transport/rtp_sender/packet_storage/packet_storage_unittest.cc:70:
TEST_F(PacketStorageTest, MaxNumberOfPackets) {
On 2014/04/29 17:33:44, Chromium Palmer wrote:
> Isn't some test along these lines still a good idea?

I improved the PacketContent test to verify that older frames are no longer
stored.

[Alpha Left Google, 2014-04-29 19:46:48]

https://codereview.chromium.org/252923007/diff/50001/media/cast/cast_config.h
File media/cast/cast_config.h (right):

https://codereview.chromium.org/252923007/diff/50001/media/cast/cast_config.h...
media/cast/cast_config.h:33: // The sender ssrc is i rtp_config.ssrc.
nit: sender ssrc is

https://codereview.chromium.org/252923007/diff/50001/media/cast/cast_config.h...
media/cast/cast_config.h:52: // The sender ssrc is i rtp_config.ssrc.
nit: sender ssrc is

https://codereview.chromium.org/252923007/diff/50001/media/cast/transport/rtp...
File media/cast/transport/rtp_sender/packet_storage/packet_storage.h (right):

https://codereview.chromium.org/252923007/diff/50001/media/cast/transport/rtp...
media/cast/transport/rtp_sender/packet_storage/packet_storage.h:50: // Same as
GetPacket, but takes a 32-bit frame id.
Make this private and use FRIEND_TEST_ALL_PREFIXES.

[hubbe, 2014-04-29 20:03:18]

https://codereview.chromium.org/252923007/diff/50001/media/cast/cast_config.h
File media/cast/cast_config.h (right):

https://codereview.chromium.org/252923007/diff/50001/media/cast/cast_config.h...
media/cast/cast_config.h:33: // The sender ssrc is i rtp_config.ssrc.
On 2014/04/29 19:46:48, Alpha wrote:
> nit: sender ssrc is

Done.

https://codereview.chromium.org/252923007/diff/50001/media/cast/cast_config.h...
media/cast/cast_config.h:52: // The sender ssrc is i rtp_config.ssrc.
On 2014/04/29 19:46:48, Alpha wrote:
> nit: sender ssrc is

Done.

https://codereview.chromium.org/252923007/diff/50001/media/cast/transport/rtp...
File media/cast/transport/rtp_sender/packet_storage/packet_storage.h (right):

https://codereview.chromium.org/252923007/diff/50001/media/cast/transport/rtp...
media/cast/transport/rtp_sender/packet_storage/packet_storage.h:50: // Same as
GetPacket, but takes a 32-bit frame id.
On 2014/04/29 19:46:48, Alpha wrote:
> Make this private and use FRIEND_TEST_ALL_PREFIXES.

Done.

[Alpha Left Google, 2014-04-29 20:11:39]

Okay. LGTM.

[palmer, 2014-04-29 22:53:29]

IPC security LGTM.

[hubbe, 2014-04-29 23:11:31]

The CQ bit was checked by hubbe@chromium.org

[commit-bot: I haz the power, 2014-04-29 23:14:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hubbe@chromium.org/252923007/150001

[commit-bot: I haz the power, 2014-04-30 08:42:41]

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

[commit-bot: I haz the power, 2014-04-30 08:42:41]

Failed to apply patch for
media/cast/transport/rtp_sender/rtp_packetizer/rtp_packetizer_unittest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file
media/cast/transport/rtp_sender/rtp_packetizer/rtp_packetizer_unittest.cc
  Hunk #1 FAILED at 102.
  1 out of 1 hunk FAILED -- saving rejects to file
media/cast/transport/rtp_sender/rtp_packetizer/rtp_packetizer_unittest.cc.rej

Patch:      
media/cast/transport/rtp_sender/rtp_packetizer/rtp_packetizer_unittest.cc
Index: media/cast/transport/rtp_sender/rtp_packetizer/rtp_packetizer_unittest.cc
diff --git
a/media/cast/transport/rtp_sender/rtp_packetizer/rtp_packetizer_unittest.cc
b/media/cast/transport/rtp_sender/rtp_packetizer/rtp_packetizer_unittest.cc
index
40dddde06a2c78d745e723c853f39541b601999e..62b854d0f83c7c9f59125100b1974e3d60bb25c4
100644
--- a/media/cast/transport/rtp_sender/rtp_packetizer/rtp_packetizer_unittest.cc
+++ b/media/cast/transport/rtp_sender/rtp_packetizer/rtp_packetizer_unittest.cc
@@ -102,7 +102,7 @@ class RtpPacketizerTest : public ::testing::Test {
   RtpPacketizerTest()
       : task_runner_(new test::FakeSingleThreadTaskRunner(&testing_clock_)),
         video_frame_(),
-        packet_storage_(&testing_clock_, kMaxPacketStorageTimeMs) {
+        packet_storage_(200) {
     logging_.AddRawEventSubscriber(&subscriber_);
     config_.sequence_number = kSeqNum;
     config_.ssrc = kSsrc;

[hubbe, 2014-05-05 05:27:36]

The CQ bit was checked by hubbe@chromium.org

[commit-bot: I haz the power, 2014-05-05 05:27:44]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hubbe@chromium.org/252923007/170001

[commit-bot: I haz the power, 2014-05-05 06:17:56]

Change committed as 268142

[hubbe, 2014-05-05 06:24:25]

A revert of this CL has been created in
https://codereview.chromium.org/268983002/ by hubbe@chromium.org.

The reason for reverting is: Unused variable:
http://build.chromium.org/p/chromium.mac/builders/Mac%20Builder%20%28dbg%29/b....

[hubbe, 2014-05-05 06:36:47]

The CQ bit was checked by hubbe@chromium.org

[commit-bot: I haz the power, 2014-05-05 06:36:50]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hubbe@chromium.org/252923007/190001

[commit-bot: I haz the power, 2014-05-05 14:16:47]

Change committed as 268176