Nack count returned on OnReceivedPacket.

Nack count returned on OnReceivedPacket.

OnReceivedPacket now return the number of times the packet has been nacked. Also some minor refactoring.

BUG=webrtc:5514
R=stefan@webrtc.org

Committed: https://chromium.googlesource.com/external/webrtc/+/1a830c2c66a5f9e25e081e07694090468dcf4743

[philipel, 2016-05-12 11:22:55]

Description was changed from

==========
Nack count returned on OnReceivedPacket.

OnReceivedPacket now return the number of times the packet has been nacked.

BUG=webrtc:5514
==========

to

==========
Nack count returned on OnReceivedPacket.

OnReceivedPacket now return the number of times the packet has been nacked. Also
some minor refactoring.

BUG=webrtc:5514
==========

[philipel, 2016-05-12 11:24:16]

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

[philipel, 2016-05-12 11:25:33]

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

[commit-bot: I haz the power, 2016-05-12 11:25:42]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1972123002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1972123002/1

[commit-bot: I haz the power, 2016-05-12 11:40:50]

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

[commit-bot: I haz the power, 2016-05-12 11:40:51]

Dry run: Try jobs failed on following builders:
  android_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/13242)

[stefan-webrtc, 2016-05-12 12:16:53]

https://codereview.webrtc.org/1972123002/diff/1/webrtc/modules/video_coding/n...
File webrtc/modules/video_coding/nack_module.cc (right):

https://codereview.webrtc.org/1972123002/diff/1/webrtc/modules/video_coding/n...
webrtc/modules/video_coding/nack_module.cc:84: if (AheadOf(last_seq_num_,
seq_num)) {
Should last_seq_num_ be called newest_seq_num_ instead? Clarify that it's not
the most recently received packet, but the "highest" sequence number

https://codereview.webrtc.org/1972123002/diff/1/webrtc/modules/video_coding/n...
webrtc/modules/video_coding/nack_module.cc:93:
UpdateReorderingStatistics(seq_num);
Should we not update reordering statistics if the packet wasn't in the nack
list?

[philipel, 2016-05-12 12:55:17]

https://codereview.webrtc.org/1972123002/diff/1/webrtc/modules/video_coding/n...
File webrtc/modules/video_coding/nack_module.cc (right):

https://codereview.webrtc.org/1972123002/diff/1/webrtc/modules/video_coding/n...
webrtc/modules/video_coding/nack_module.cc:84: if (AheadOf(last_seq_num_,
seq_num)) {
On 2016/05/12 12:16:53, stefan-webrtc (holmer) wrote:
> Should last_seq_num_ be called newest_seq_num_ instead? Clarify that it's not
> the most recently received packet, but the "highest" sequence number

Done.

https://codereview.webrtc.org/1972123002/diff/1/webrtc/modules/video_coding/n...
webrtc/modules/video_coding/nack_module.cc:93:
UpdateReorderingStatistics(seq_num);
On 2016/05/12 12:16:53, stefan-webrtc (holmer) wrote:
> Should we not update reordering statistics if the packet wasn't in the nack
> list?

If the packet isn't in the nack list then the packet was received in order.

To clarify, the reordering statistics doesn't tell us how often reordering
occurs, it only tells us how severe the reordering is when it occurs. But now
that you mention it, I realize I have to include how often it occurs as well...

[stefan-webrtc, 2016-05-12 14:27:44]

lgtm, but consider adding the comment I requested.

https://codereview.webrtc.org/1972123002/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/nack_module.cc (right):

https://codereview.webrtc.org/1972123002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/nack_module.cc:88: int nacks_sent_for_packet =
nack_list_it->second.retries;
Comment on this special case. Something like "Packet was out of order due to
being nacked, don't update reordering statistics"?

[philipel, 2016-05-12 14:42:14]

Yeah... I now realize I completely misunderstood your earlier comment. If you
look at the code regarding updating the reordering statistics again I think you
will find it now makes sense... :)

[philipel, 2016-05-12 14:42:20]

The CQ bit was checked by philipel@webrtc.org

[philipel, 2016-05-12 14:42:20]

The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1972123002/#ps40001 (title:
"Feedback fix.")

[commit-bot: I haz the power, 2016-05-12 14:42:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1972123002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1972123002/40001

[commit-bot: I haz the power, 2016-05-12 16:42:59]

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

[commit-bot: I haz the power, 2016-05-12 16:43:00]

Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[philipel, 2016-05-13 09:12:16]

Description was changed from

==========
Nack count returned on OnReceivedPacket.

OnReceivedPacket now return the number of times the packet has been nacked. Also
some minor refactoring.

BUG=webrtc:5514
==========

to

==========
Nack count returned on OnReceivedPacket.

OnReceivedPacket now return the number of times the packet has been nacked. Also
some minor refactoring.

BUG=webrtc:5514
R=stefan@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/1a830c2c66a5f9e25e081e076...
==========

[commit-bot: I haz the power, 2016-05-13 09:12:17]

Description was changed from

==========
Nack count returned on OnReceivedPacket.

OnReceivedPacket now return the number of times the packet has been nacked. Also
some minor refactoring.

BUG=webrtc:5514
R=stefan@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/1a830c2c66a5f9e25e081e076...
==========

to

==========
Nack count returned on OnReceivedPacket.

OnReceivedPacket now return the number of times the packet has been nacked. Also
some minor refactoring.

BUG=webrtc:5514
R=stefan@webrtc.org

Committed: https://crrev.com/1a830c2c66a5f9e25e081e07694090468dcf4743
Cr-Commit-Position: refs/heads/master@{#12717}
==========

[philipel, 2016-05-13 09:12:17]

Committed patchset #3 (id:40001) manually as
1a830c2c66a5f9e25e081e07694090468dcf4743 (presubmit successful).