Cast: reduce the amount of retransmission packets

Cast: reduce the amount of retransmission packets

Upon duplicated ACKs for a frame the previous behavior was to resend
all packets of the first unacked frame. It was shown in reports that it
is excessive. Instead this change sends the first packet of the last
encoded frame. This significantly reduces the amount of packet
re-transmitted.

PacketStorage is also redesigned in this code. It is now a simple deque
instead of a map.

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

[miu, 2014-06-07 00:59:42]

Pretty good, on-the-whole.  Just a bunch of little things...

https://codereview.chromium.org/317243007/diff/1/media/cast/transport/rtp_sen...
File media/cast/transport/rtp_sender/packet_storage/packet_storage.cc (right):

https://codereview.chromium.org/317243007/diff/1/media/cast/transport/rtp_sen...
media/cast/transport/rtp_sender/packet_storage/packet_storage.cc:19:
first_frame_id_in_list_(0) {
last_frame_id_in_list_ needs to be initialized here.

https://codereview.chromium.org/317243007/diff/1/media/cast/transport/rtp_sen...
File media/cast/transport/rtp_sender/packet_storage/packet_storage.h (right):

https://codereview.chromium.org/317243007/diff/1/media/cast/transport/rtp_sen...
media/cast/transport/rtp_sender/packet_storage/packet_storage.h:28: //
StorageIndex contains {frame_id, packet_id}.
Don't need these typedef's anymore.  See comment below about getting rid of the
unused |stored_packets_| member.

https://codereview.chromium.org/317243007/diff/1/media/cast/transport/rtp_sen...
media/cast/transport/rtp_sender/packet_storage/packet_storage.h:33: typedef
std::deque<SendPacketVector> FrameList;
naming nit: FrameQueue or FrameDeque?

https://codereview.chromium.org/317243007/diff/1/media/cast/transport/rtp_sen...
media/cast/transport/rtp_sender/packet_storage/packet_storage.h:38: const size_t
kMaxStoredFrames = 255;
You can kill this.  This constant is defined in cast_defines.h
(kMaxUnackedFrames):
https://code.google.com/p/chromium/codesearch#chromium/src/media/cast/cast_de...

https://codereview.chromium.org/317243007/diff/1/media/cast/transport/rtp_sen...
media/cast/transport/rtp_sender/packet_storage/packet_storage.h:42:
PacketStorage(size_t stored_frames);
Need 'explicit' keyword.

https://codereview.chromium.org/317243007/diff/1/media/cast/transport/rtp_sen...
media/cast/transport/rtp_sender/packet_storage/packet_storage.h:42:
PacketStorage(size_t stored_frames);
This pruning/upper-limit would not be necessary if AudioSender and VideoSender
could call a ReleaseFrame() method once it gets an ACK for a frame.

https://codereview.chromium.org/317243007/diff/1/media/cast/transport/rtp_sen...
media/cast/transport/rtp_sender/packet_storage/packet_storage.h:49: // Store a
list of packets for a frame.
nit: s/a list of/all of the/

https://codereview.chromium.org/317243007/diff/1/media/cast/transport/rtp_sen...
media/cast/transport/rtp_sender/packet_storage/packet_storage.h:52: // Returns a
list of packets for a frame indexed by a 8-bits ID.
s/by a 8-bits ID/by the lowest 8 bits of the ID/

BTW--I understand why you have to use a uint8 here, but it really sucks.  In
future code changes, we need to stop propagating the data types that were used
to optimize the wire format, and start using the appropriate full-width data
types throughout the cast library.

https://codereview.chromium.org/317243007/diff/1/media/cast/transport/rtp_sen...
media/cast/transport/rtp_sender/packet_storage/packet_storage.h:62: static
PacketRef FastCopyPacket(const PacketRef& packet);
There's no need for this to be a member of this class.  The method is only
called in one place, from RtpSender, and the logic is fairly trivial.  It's just
an extra layer of indirection.

https://codereview.chromium.org/317243007/diff/1/media/cast/transport/rtp_sen...
media/cast/transport/rtp_sender/packet_storage/packet_storage.h:65: PacketMap
stored_packets_;
naming nit: Can we call this max_frames_to_keep_ instead?  At least,
s/packet/frame/.

https://codereview.chromium.org/317243007/diff/1/media/cast/transport/rtp_sen...
media/cast/transport/rtp_sender/packet_storage/packet_storage.h:65: PacketMap
stored_packets_;
This member is no longer used.

https://codereview.chromium.org/317243007/diff/1/media/cast/transport/rtp_sen...
media/cast/transport/rtp_sender/packet_storage/packet_storage.h:66: size_t
stored_frames_;
nit: const

https://codereview.chromium.org/317243007/diff/1/media/cast/transport/rtp_sen...
File media/cast/transport/rtp_sender/packet_storage/packet_storage_unittest.cc
(right):

https://codereview.chromium.org/317243007/diff/1/media/cast/transport/rtp_sen...
media/cast/transport/rtp_sender/packet_storage/packet_storage_unittest.cc:74:
uint32 first_frame_id = 0;
nit: These two lines could be:

  uint32 first_frame_id = 0xffffffff - 5;

https://codereview.chromium.org/317243007/diff/1/media/cast/transport/rtp_sen...
media/cast/transport/rtp_sender/packet_storage/packet_storage_unittest.cc:93:
uint32 first_frame_id = 0;
nit: These two lines could be:

  uint32 first_frame_id = 0xffffffff - 5;

https://codereview.chromium.org/317243007/diff/1/media/cast/transport/rtp_sen...
File media/cast/transport/rtp_sender/rtp_packetizer/rtp_packetizer.cc (right):

https://codereview.chromium.org/317243007/diff/1/media/cast/transport/rtp_sen...
media/cast/transport/rtp_sender/rtp_packetizer/rtp_packetizer.cc:110:
packet_storage_->StoreFrame(frame.frame_id, packets);
Nice how the PacketStorage clean-up helped clean up code here too!  :)

https://codereview.chromium.org/317243007/diff/1/media/cast/transport/rtp_sen...
File media/cast/transport/rtp_sender/rtp_sender.cc (right):

https://codereview.chromium.org/317243007/diff/1/media/cast/transport/rtp_sen...
media/cast/transport/rtp_sender/rtp_sender.cc:96: PacketRef packet_copy =
PacketStorage::FastCopyPacket(it->second);
Dumb question: Why would we ever want a deep copy of a packet?

https://codereview.chromium.org/317243007/diff/1/media/cast/video_sender/vide...
File media/cast/video_sender/video_sender.cc (right):

https://codereview.chromium.org/317243007/diff/1/media/cast/video_sender/vide...
media/cast/video_sender/video_sender.cc:318: duplicate_ack_counter_ = 0;
Good catch!  :)

https://codereview.chromium.org/317243007/diff/1/media/cast/video_sender/vide...
media/cast/video_sender/video_sender.cc:359: // Send the first packet of the
last encoded frame to kick start
Could you add:

  VLOG(1) << "Resending first packet of frame " << last_sent_frame_id_ << " to
kick-start.";

(It's been useful to have this kind of logging.)

[Alpha Left Google, 2014-06-07 01:15:10]

https://codereview.chromium.org/317243007/diff/1/media/cast/transport/rtp_sen...
File media/cast/transport/rtp_sender/packet_storage/packet_storage.cc (right):

https://codereview.chromium.org/317243007/diff/1/media/cast/transport/rtp_sen...
media/cast/transport/rtp_sender/packet_storage/packet_storage.cc:19:
first_frame_id_in_list_(0) {
On 2014/06/07 00:59:41, miu wrote:
> last_frame_id_in_list_ needs to be initialized here.

Done.

https://codereview.chromium.org/317243007/diff/1/media/cast/transport/rtp_sen...
File media/cast/transport/rtp_sender/packet_storage/packet_storage.h (right):

https://codereview.chromium.org/317243007/diff/1/media/cast/transport/rtp_sen...
media/cast/transport/rtp_sender/packet_storage/packet_storage.h:28: //
StorageIndex contains {frame_id, packet_id}.
On 2014/06/07 00:59:41, miu wrote:
> Don't need these typedef's anymore.  See comment below about getting rid of
the
> unused |stored_packets_| member.

Done.

https://codereview.chromium.org/317243007/diff/1/media/cast/transport/rtp_sen...
media/cast/transport/rtp_sender/packet_storage/packet_storage.h:33: typedef
std::deque<SendPacketVector> FrameList;
On 2014/06/07 00:59:42, miu wrote:
> naming nit: FrameQueue or FrameDeque?

Done.

https://codereview.chromium.org/317243007/diff/1/media/cast/transport/rtp_sen...
media/cast/transport/rtp_sender/packet_storage/packet_storage.h:38: const size_t
kMaxStoredFrames = 255;
On 2014/06/07 00:59:41, miu wrote:
> You can kill this.  This constant is defined in cast_defines.h
> (kMaxUnackedFrames):
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/cast/cast_de...

Done.

https://codereview.chromium.org/317243007/diff/1/media/cast/transport/rtp_sen...
media/cast/transport/rtp_sender/packet_storage/packet_storage.h:42:
PacketStorage(size_t stored_frames);
On 2014/06/07 00:59:41, miu wrote:
> This pruning/upper-limit would not be necessary if AudioSender and VideoSender
> could call a ReleaseFrame() method once it gets an ACK for a frame.

I think I'll keep the current design. There is extra plumbing to do what you
suggested and the benefit is unclear to me.

https://codereview.chromium.org/317243007/diff/1/media/cast/transport/rtp_sen...
media/cast/transport/rtp_sender/packet_storage/packet_storage.h:49: // Store a
list of packets for a frame.
On 2014/06/07 00:59:41, miu wrote:
> nit: s/a list of/all of the/
>  

Done.

https://codereview.chromium.org/317243007/diff/1/media/cast/transport/rtp_sen...
media/cast/transport/rtp_sender/packet_storage/packet_storage.h:52: // Returns a
list of packets for a frame indexed by a 8-bits ID.
On 2014/06/07 00:59:41, miu wrote:
> s/by a 8-bits ID/by the lowest 8 bits of the ID/
> 
> BTW--I understand why you have to use a uint8 here, but it really sucks.  In
> future code changes, we need to stop propagating the data types that were used
> to optimize the wire format, and start using the appropriate full-width data
> types throughout the cast library.

This one unfortunately needs to be 8 bits because it comes from NACK. :(

The method is specifically called GetFrame8 to clarify so we know what happens
when passing in int32 or something else.

https://codereview.chromium.org/317243007/diff/1/media/cast/transport/rtp_sen...
media/cast/transport/rtp_sender/packet_storage/packet_storage.h:62: static
PacketRef FastCopyPacket(const PacketRef& packet);
On 2014/06/07 00:59:41, miu wrote:
> There's no need for this to be a member of this class.  The method is only
> called in one place, from RtpSender, and the logic is fairly trivial.  It's
just
> an extra layer of indirection.

There might be other place that needs this. And it was originally in
PacketStorage. In this particular case it is because PacketStoarge keeps one ref
that this fast copy can be done.

https://codereview.chromium.org/317243007/diff/1/media/cast/transport/rtp_sen...
media/cast/transport/rtp_sender/packet_storage/packet_storage.h:65: PacketMap
stored_packets_;
On 2014/06/07 00:59:41, miu wrote:
> This member is no longer used.

Done.

https://codereview.chromium.org/317243007/diff/1/media/cast/transport/rtp_sen...
media/cast/transport/rtp_sender/packet_storage/packet_storage.h:65: PacketMap
stored_packets_;
On 2014/06/07 00:59:41, miu wrote:
> naming nit: Can we call this max_frames_to_keep_ instead?  At least,
> s/packet/frame/.

Changed it to max_stored_frames_.

https://codereview.chromium.org/317243007/diff/1/media/cast/transport/rtp_sen...
media/cast/transport/rtp_sender/packet_storage/packet_storage.h:66: size_t
stored_frames_;
On 2014/06/07 00:59:42, miu wrote:
> nit: const

Done.

https://codereview.chromium.org/317243007/diff/1/media/cast/transport/rtp_sen...
File media/cast/transport/rtp_sender/packet_storage/packet_storage_unittest.cc
(right):

https://codereview.chromium.org/317243007/diff/1/media/cast/transport/rtp_sen...
media/cast/transport/rtp_sender/packet_storage/packet_storage_unittest.cc:74:
uint32 first_frame_id = 0;
On 2014/06/07 00:59:42, miu wrote:
> nit: These two lines could be:
> 
>   uint32 first_frame_id = 0xffffffff - 5;

Done.

https://codereview.chromium.org/317243007/diff/1/media/cast/transport/rtp_sen...
media/cast/transport/rtp_sender/packet_storage/packet_storage_unittest.cc:93:
uint32 first_frame_id = 0;
On 2014/06/07 00:59:42, miu wrote:
> nit: These two lines could be:
> 
>   uint32 first_frame_id = 0xffffffff - 5;

Done.

https://codereview.chromium.org/317243007/diff/1/media/cast/transport/rtp_sen...
File media/cast/transport/rtp_sender/rtp_packetizer/rtp_packetizer.cc (right):

https://codereview.chromium.org/317243007/diff/1/media/cast/transport/rtp_sen...
media/cast/transport/rtp_sender/rtp_packetizer/rtp_packetizer.cc:110:
packet_storage_->StoreFrame(frame.frame_id, packets);
On 2014/06/07 00:59:42, miu wrote:
> Nice how the PacketStorage clean-up helped clean up code here too!  :)

Done.

https://codereview.chromium.org/317243007/diff/1/media/cast/transport/rtp_sen...
File media/cast/transport/rtp_sender/rtp_sender.cc (right):

https://codereview.chromium.org/317243007/diff/1/media/cast/transport/rtp_sen...
media/cast/transport/rtp_sender/rtp_sender.cc:96: PacketRef packet_copy =
PacketStorage::FastCopyPacket(it->second);
On 2014/06/07 00:59:42, miu wrote:
> Dumb question: Why would we ever want a deep copy of a packet?

Because the pacer could have one shallow in the queue. The second copy has a
different sequence number in the RTP header although packet content is the same.

https://codereview.chromium.org/317243007/diff/1/media/cast/video_sender/vide...
File media/cast/video_sender/video_sender.cc (right):

https://codereview.chromium.org/317243007/diff/1/media/cast/video_sender/vide...
media/cast/video_sender/video_sender.cc:318: duplicate_ack_counter_ = 0;
On 2014/06/07 00:59:42, miu wrote:
> Good catch!  :)

Done.

https://codereview.chromium.org/317243007/diff/1/media/cast/video_sender/vide...
media/cast/video_sender/video_sender.cc:359: // Send the first packet of the
last encoded frame to kick start
On 2014/06/07 00:59:42, miu wrote:
> Could you add:
> 
>   VLOG(1) << "Resending first packet of frame " << last_sent_frame_id_ << " to
> kick-start.";
> 
> (It's been useful to have this kind of logging.)

Done.

[miu, 2014-06-07 02:00:10]

https://codereview.chromium.org/317243007/diff/1/media/cast/transport/rtp_sen...
File media/cast/transport/rtp_sender/packet_storage/packet_storage.h (right):

https://codereview.chromium.org/317243007/diff/1/media/cast/transport/rtp_sen...
media/cast/transport/rtp_sender/packet_storage/packet_storage.h:42:
PacketStorage(size_t stored_frames);
On 2014/06/07 01:15:09, Alpha wrote:
> On 2014/06/07 00:59:41, miu wrote:
> > This pruning/upper-limit would not be necessary if AudioSender and
VideoSender
> > could call a ReleaseFrame() method once it gets an ACK for a frame.
> 
> I think I'll keep the current design. There is extra plumbing to do what you
> suggested and the benefit is unclear to me.

SGTM for this change.  The benefits I see are:

1. Reduced memory usage: PacketStorage would only have to keep those frames in
memory that the receiver does not yet have.
2. PacketStorage could store any number of frames, without a need to configure a
"max limit" and also not need any pruning logic.

https://codereview.chromium.org/317243007/diff/1/media/cast/transport/rtp_sen...
media/cast/transport/rtp_sender/packet_storage/packet_storage.h:62: static
PacketRef FastCopyPacket(const PacketRef& packet);
On 2014/06/07 01:15:09, Alpha wrote:
> On 2014/06/07 00:59:41, miu wrote:
> > There's no need for this to be a member of this class.  The method is only
> > called in one place, from RtpSender, and the logic is fairly trivial.  It's
> just
> > an extra layer of indirection.
> 
> There might be other place that needs this. And it was originally in
> PacketStorage. In this particular case it is because PacketStoarge keeps one
ref
> that this fast copy can be done.

However, the function is static.  It has no way of knowing what object holds the
"one ref."  The holder might not be the PacketStorage instance the caller is
using.  On the other hand, RtpSender *does* know this the case.

Thinking about this "encapsulation of functionality problem" further, I think
you should just get rid of the PacketStorage class, and move all its code into
RtpSender.  WDYT?

https://codereview.chromium.org/317243007/diff/1/media/cast/transport/rtp_sen...
File media/cast/transport/rtp_sender/rtp_sender.cc (right):

https://codereview.chromium.org/317243007/diff/1/media/cast/transport/rtp_sen...
media/cast/transport/rtp_sender/rtp_sender.cc:96: PacketRef packet_copy =
PacketStorage::FastCopyPacket(it->second);
On 2014/06/07 01:15:09, Alpha wrote:
> On 2014/06/07 00:59:42, miu wrote:
> > Dumb question: Why would we ever want a deep copy of a packet?
> 
> Because the pacer could have one shallow in the queue. The second copy has a
> different sequence number in the RTP header although packet content is the
same.

Okay.  We can clean this up in a later change.  It doesn't feel right to have to
copy packet data just to change the sequence number.

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

https://codereview.chromium.org/317243007/diff/50009/media/cast/transport/rtp...
media/cast/transport/rtp_sender/packet_storage/packet_storage.h:52: static
PacketRef FastCopyPacket(const PacketRef& packet);
IMHO, the naming could be clearer.  How about: GetUnsharedCopyOfPacket()

[miu, 2014-06-07 02:20:09]

lgtm

[Alpha Left Google, 2014-06-07 02:20:21]

The CQ bit was checked by hclam@chromium.org

[commit-bot: I haz the power, 2014-06-07 02:21:35]

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

[commit-bot: I haz the power, 2014-06-07 09:04:19]

Change committed as 275663