Cast: Implement priority packets in PacedSender

Cast: Implement priority packets in PacedSender

This change implements a high priority packet list in PacedSender. In
practice the order of packet priority is:
1. RTCP
2. Audio
3. Video

The reason for doing this such that audio packets can interleave between
video packets even if the audio packets has a greater value of
timestamp. This characteristic of interleaving packets will facilitate
better bandwidth estimation by looking at audio send and ACK pairs.

cast_simulator shows no ill effect but a slight improvement:
Before
Audio frame count received: 17978
Video frame count received: 5169

After
Audio frame count received: 17989
Video frame count received: 5174

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

[miu, 2014-07-16 22:55:56]

https://codereview.chromium.org/399743002/diff/1/media/cast/net/pacing/paced_...
File media/cast/net/pacing/paced_sender.cc (right):

https://codereview.chromium.org/399743002/diff/1/media/cast/net/pacing/paced_...
media/cast/net/pacing/paced_sender.cc:72: if (IsHighPriority(packets[i].first))
{
Will a SendPacketVector ever contain packets from more than one ssrc?  If not,
you could call IsHighPriority() once before entering the loop.

https://codereview.chromium.org/399743002/diff/1/media/cast/net/pacing/paced_...
media/cast/net/pacing/paced_sender.cc:101: if (IsHighPriority(packets[i].first))
{
ditto re: redundant IsHighPriority() calls.

https://codereview.chromium.org/399743002/diff/1/media/cast/net/pacing/paced_...
media/cast/net/pacing/paced_sender.cc:137: PacketRef
PacedSender::GetNextPacket(PacketType* packet_type,
naming: This method should be called PopNextPacket() or ExtractNextPacket() to
clarify that it mutates the data structures.

https://codereview.chromium.org/399743002/diff/1/media/cast/net/pacing/paced_...
media/cast/net/pacing/paced_sender.cc:139: DCHECK(!priority_packet_list_.empty()
||
Don't need this DCHECK, since the one a few lines down covers this.

https://codereview.chromium.org/399743002/diff/1/media/cast/net/pacing/paced_...
media/cast/net/pacing/paced_sender.cc:145: i = list->begin();
Please merge this with the line above.

https://codereview.chromium.org/399743002/diff/1/media/cast/net/pacing/paced_...
File media/cast/net/pacing/paced_sender.h (right):

https://codereview.chromium.org/399743002/diff/1/media/cast/net/pacing/paced_...
media/cast/net/pacing/paced_sender.h:75: // Because IsHigherPriority() is done
in linear time.
nit: s/done/determined/

https://codereview.chromium.org/399743002/diff/1/media/cast/net/pacing/paced_...
File media/cast/net/pacing/paced_sender_unittest.cc (right):

https://codereview.chromium.org/399743002/diff/1/media/cast/net/pacing/paced_...
media/cast/net/pacing/paced_sender_unittest.cc:361: SendPacketVector
resend_packets =
Can you add comments for the rest of this test code?  It's not clear to me which
packets are for RTCP vs Audio vs Video, and how the packets going in are
expected to be reordered when coming out.

[Alpha Left Google, 2014-07-17 00:12:57]

https://codereview.chromium.org/399743002/diff/1/media/cast/net/pacing/paced_...
File media/cast/net/pacing/paced_sender.cc (right):

https://codereview.chromium.org/399743002/diff/1/media/cast/net/pacing/paced_...
media/cast/net/pacing/paced_sender.cc:72: if (IsHighPriority(packets[i].first))
{
On 2014/07/16 22:55:55, miu wrote:
> Will a SendPacketVector ever contain packets from more than one ssrc?  If not,
> you could call IsHighPriority() once before entering the loop.

Done.

https://codereview.chromium.org/399743002/diff/1/media/cast/net/pacing/paced_...
media/cast/net/pacing/paced_sender.cc:101: if (IsHighPriority(packets[i].first))
{
On 2014/07/16 22:55:55, miu wrote:
> ditto re: redundant IsHighPriority() calls.

Done.

https://codereview.chromium.org/399743002/diff/1/media/cast/net/pacing/paced_...
media/cast/net/pacing/paced_sender.cc:137: PacketRef
PacedSender::GetNextPacket(PacketType* packet_type,
On 2014/07/16 22:55:56, miu wrote:
> naming: This method should be called PopNextPacket() or ExtractNextPacket() to
> clarify that it mutates the data structures.

Done.

https://codereview.chromium.org/399743002/diff/1/media/cast/net/pacing/paced_...
media/cast/net/pacing/paced_sender.cc:139: DCHECK(!priority_packet_list_.empty()
||
On 2014/07/16 22:55:56, miu wrote:
> Don't need this DCHECK, since the one a few lines down covers this.

Done.

https://codereview.chromium.org/399743002/diff/1/media/cast/net/pacing/paced_...
media/cast/net/pacing/paced_sender.cc:145: i = list->begin();
On 2014/07/16 22:55:55, miu wrote:
> Please merge this with the line above.

Done.

https://codereview.chromium.org/399743002/diff/1/media/cast/net/pacing/paced_...
File media/cast/net/pacing/paced_sender.h (right):

https://codereview.chromium.org/399743002/diff/1/media/cast/net/pacing/paced_...
media/cast/net/pacing/paced_sender.h:75: // Because IsHigherPriority() is done
in linear time.
On 2014/07/16 22:55:56, miu wrote:
> nit: s/done/determined/

Done.

https://codereview.chromium.org/399743002/diff/1/media/cast/net/pacing/paced_...
File media/cast/net/pacing/paced_sender_unittest.cc (right):

https://codereview.chromium.org/399743002/diff/1/media/cast/net/pacing/paced_...
media/cast/net/pacing/paced_sender_unittest.cc:361: SendPacketVector
resend_packets =
On 2014/07/16 22:55:56, miu wrote:
> Can you add comments for the rest of this test code?  It's not clear to me
which
> packets are for RTCP vs Audio vs Video, and how the packets going in are
> expected to be reordered when coming out.

Done.

[miu, 2014-07-17 16:12:42]

lgtm

[Alpha Left Google, 2014-07-17 18:16:42]

The CQ bit was checked by hclam@chromium.org

[commit-bot: I haz the power, 2014-07-17 18:18:18]

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

[commit-bot: I haz the power, 2014-07-17 22:46:19]

Change committed as 283911