Prioritize video packets when sending padding or preemptive retransmits.

Prioritize video packets when sending padding or preemptive retransmits.

Video modules are added in reverse order to ensure that the padding order is the same as before, prioritizing high resolution streams.

BUG=webrtc:7043
Review-Url: https://codereview.webrtc.org/2655033002
Cr-Commit-Position: refs/heads/master@{#16329}
Committed: https://chromium.googlesource.com/external/webrtc/+/16b02211a938c2320ef458425bcd4f657328ba52

[stefan-webrtc, 2017-01-25 16:12:33]

stefan@webrtc.org changed reviewers:
+ mflodman@webrtc.org, philipel@webrtc.org

[stefan-webrtc, 2017-01-25 16:12:47]

The CQ bit was checked by stefan@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-25 16:12:52]

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

[commit-bot: I haz the power, 2017-01-25 17:20:24]

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

[commit-bot: I haz the power, 2017-01-25 17:20:25]

Dry run: This issue passed the CQ dry run.

[philipel, 2017-01-26 12:14:03]

lgtm

[mflodman, 2017-01-26 13:01:40]

https://codereview.webrtc.org/2655033002/diff/1/webrtc/modules/pacing/packet_...
File webrtc/modules/pacing/packet_router.cc (right):

https://codereview.webrtc.org/2655033002/diff/1/webrtc/modules/pacing/packet_...
webrtc/modules/pacing/packet_router.cc:73: for (RtpRtcp* module : rtp_modules_)
{
I think it would be good with a comment about the module/padding order here too.
It's a risk otherwise we'll refactor this code and change the behavior, without
noticing the comment you added above.

https://codereview.webrtc.org/2655033002/diff/1/webrtc/video/video_send_strea...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/2655033002/diff/1/webrtc/video/video_send_strea...
webrtc/video/video_send_stream.cc:802: // when sending padding, with the hope
that the packet rate will be smaller,
Isn't this the opposite? AddRtpModule does a push_front -> the module added in
the end will be in the front and do all the padding.

[stefan-webrtc, 2017-01-27 11:52:38]

https://codereview.webrtc.org/2655033002/diff/1/webrtc/modules/pacing/packet_...
File webrtc/modules/pacing/packet_router.cc (right):

https://codereview.webrtc.org/2655033002/diff/1/webrtc/modules/pacing/packet_...
webrtc/modules/pacing/packet_router.cc:73: for (RtpRtcp* module : rtp_modules_)
{
On 2017/01/26 13:01:40, mflodman wrote:
> I think it would be good with a comment about the module/padding order here
too.
> It's a risk otherwise we'll refactor this code and change the behavior,
without
> noticing the comment you added above.

Done.

https://codereview.webrtc.org/2655033002/diff/1/webrtc/video/video_send_strea...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/2655033002/diff/1/webrtc/video/video_send_strea...
webrtc/video/video_send_stream.cc:802: // when sending padding, with the hope
that the packet rate will be smaller,
On 2017/01/26 13:01:40, mflodman wrote:
> Isn't this the opposite? AddRtpModule does a push_front -> the module added in
> the end will be in the front and do all the padding.

Yes, you are right. Thanks :)

[mflodman, 2017-01-27 12:57:59]

One nit, then LGTM.

https://codereview.webrtc.org/2655033002/diff/20001/webrtc/video/video_send_s...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/2655033002/diff/20001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:804: for (RtpRtcp* rtp_rtcp :
rtp_rtcp_modules_) {
Remove {}, to be consistent with the rest of the file.

[stefan-webrtc, 2017-01-27 13:04:28]

.

[stefan-webrtc, 2017-01-27 13:05:12]

.

[stefan-webrtc, 2017-01-27 13:05:19]

https://codereview.webrtc.org/2655033002/diff/20001/webrtc/video/video_send_s...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/2655033002/diff/20001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:804: for (RtpRtcp* rtp_rtcp :
rtp_rtcp_modules_) {
On 2017/01/27 12:57:59, mflodman wrote:
> Remove {}, to be consistent with the rest of the file.

Done.

[stefan-webrtc, 2017-01-27 13:05:37]

The CQ bit was checked by stefan@webrtc.org

[stefan-webrtc, 2017-01-27 13:05:38]

The patchset sent to the CQ was uploaded after l-g-t-m from philipel@webrtc.org,
mflodman@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2655033002/#ps60001 (title:
".")

[commit-bot: I haz the power, 2017-01-27 13:05:45]

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

[commit-bot: I haz the power, 2017-01-27 15:12:17]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1485522337476900,
"parent_rev": "fb45c6c103eb5fe594a54f4291726e8242f0ec38", "commit_rev":
"16b02211a938c2320ef458425bcd4f657328ba52"}

[commit-bot: I haz the power, 2017-01-27 15:12:20]

Description was changed from

==========
Prioritize video packets when sending padding or preemptive retransmits.

Video modules are added in reverse order to ensure that the padding order is the
same as before, prioritizing high resolution streams.

BUG=webrtc:7043
==========

to

==========
Prioritize video packets when sending padding or preemptive retransmits.

Video modules are added in reverse order to ensure that the padding order is the
same as before, prioritizing high resolution streams.

BUG=webrtc:7043

Review-Url: https://codereview.webrtc.org/2655033002
Cr-Commit-Position: refs/heads/master@{#16329}
Committed:
https://chromium.googlesource.com/external/webrtc/+/16b02211a938c2320ef458425...
==========

[commit-bot: I haz the power, 2017-01-27 15:12:21]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/external/webrtc/+/16b02211a938c2320ef458425...