PacketBuffer now can save how many times a packet has been nacked.

PacketBuffer now can save how many times a packet has been nacked.
Also save size/max nack count in the FrameObject/RtpFrameObject.

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

Committed: https://crrev.com/5ceaaae3689cac9ae10dc7caf5802e8d6e0c7a99
Cr-Commit-Position: refs/heads/master@{#12863}

[philipel, 2016-05-17 09:33:28]

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

[stefan-webrtc, 2016-05-20 13:10:23]

https://codereview.webrtc.org/1988653002/diff/1/webrtc/modules/video_coding/f...
File webrtc/modules/video_coding/frame_object.h (right):

https://codereview.webrtc.org/1988653002/diff/1/webrtc/modules/video_coding/f...
webrtc/modules/video_coding/frame_object.h:67: int8_t max_nack_count_;
Why int8_t? int would be nicer

https://codereview.webrtc.org/1988653002/diff/1/webrtc/modules/video_coding/p...
File webrtc/modules/video_coding/packet_buffer.cc (right):

https://codereview.webrtc.org/1988653002/diff/1/webrtc/modules/video_coding/p...
webrtc/modules/video_coding/packet_buffer.cc:142: size_t combined_size = 0;
Should this be frame_size instead?

https://codereview.webrtc.org/1988653002/diff/1/webrtc/modules/video_coding/p...
webrtc/modules/video_coding/packet_buffer.cc:151: start_index = start_index > 0
? start_index - 1 : size_ - 1;
start_index seems like a bad variable name as it doesn't point to the start all
the time.

https://codereview.webrtc.org/1988653002/diff/1/webrtc/modules/video_coding/p...
File webrtc/modules/video_coding/packet_buffer.h (right):

https://codereview.webrtc.org/1988653002/diff/1/webrtc/modules/video_coding/p...
webrtc/modules/video_coding/packet_buffer.h:42: bool InsertPacket(const
VCMPacket& packet, int times_nacked = -1);
Avoid default arguments. Maybe add a nacked member to the VCMPacket class
instead?

https://codereview.webrtc.org/1988653002/diff/1/webrtc/modules/video_coding/p...
File webrtc/modules/video_coding/packet_buffer_unittest.cc (right):

https://codereview.webrtc.org/1988653002/diff/1/webrtc/modules/video_coding/p...
webrtc/modules/video_coding/packet_buffer_unittest.cc:296: RtpFrameObject*
rtpFrame = static_cast<RtpFrameObject*>(frame);
rtp_frame

https://codereview.webrtc.org/1988653002/diff/1/webrtc/modules/video_coding/p...
webrtc/modules/video_coding/packet_buffer_unittest.cc:304: //            seq_num
   , kf, frst, lst, size, data
Not very nicely aligned with the calls below :) Maybe simply remove it?

[philipel, 2016-05-23 09:19:22]

https://codereview.webrtc.org/1988653002/diff/1/webrtc/modules/video_coding/f...
File webrtc/modules/video_coding/frame_object.h (right):

https://codereview.webrtc.org/1988653002/diff/1/webrtc/modules/video_coding/f...
webrtc/modules/video_coding/frame_object.h:67: int8_t max_nack_count_;
On 2016/05/20 13:10:23, stefan-webrtc (holmer) wrote:
> Why int8_t? int would be nicer

Changed to int.

The reason I use int8_t here is because a FrameObject is something we might
potentially have many instances of and therefore size might make a difference.

https://codereview.webrtc.org/1988653002/diff/1/webrtc/modules/video_coding/p...
File webrtc/modules/video_coding/packet_buffer.cc (right):

https://codereview.webrtc.org/1988653002/diff/1/webrtc/modules/video_coding/p...
webrtc/modules/video_coding/packet_buffer.cc:142: size_t combined_size = 0;
On 2016/05/20 13:10:23, stefan-webrtc (holmer) wrote:
> Should this be frame_size instead?

Done.

https://codereview.webrtc.org/1988653002/diff/1/webrtc/modules/video_coding/p...
webrtc/modules/video_coding/packet_buffer.cc:151: start_index = start_index > 0
? start_index - 1 : size_ - 1;
On 2016/05/20 13:10:23, stefan-webrtc (holmer) wrote:
> start_index seems like a bad variable name as it doesn't point to the start
all
> the time.

Hard to find a better name, I added a comment about it.

https://codereview.webrtc.org/1988653002/diff/1/webrtc/modules/video_coding/p...
File webrtc/modules/video_coding/packet_buffer.h (right):

https://codereview.webrtc.org/1988653002/diff/1/webrtc/modules/video_coding/p...
webrtc/modules/video_coding/packet_buffer.h:42: bool InsertPacket(const
VCMPacket& packet, int times_nacked = -1);
On 2016/05/20 13:10:23, stefan-webrtc (holmer) wrote:
> Avoid default arguments. Maybe add a nacked member to the VCMPacket class
> instead?

Done.

https://codereview.webrtc.org/1988653002/diff/1/webrtc/modules/video_coding/p...
File webrtc/modules/video_coding/packet_buffer_unittest.cc (right):

https://codereview.webrtc.org/1988653002/diff/1/webrtc/modules/video_coding/p...
webrtc/modules/video_coding/packet_buffer_unittest.cc:296: RtpFrameObject*
rtpFrame = static_cast<RtpFrameObject*>(frame);
On 2016/05/20 13:10:23, stefan-webrtc (holmer) wrote:
> rtp_frame

Done.

https://codereview.webrtc.org/1988653002/diff/1/webrtc/modules/video_coding/p...
webrtc/modules/video_coding/packet_buffer_unittest.cc:304: //            seq_num
   , kf, frst, lst, size, data
On 2016/05/20 13:10:23, stefan-webrtc (holmer) wrote:
> Not very nicely aligned with the calls below :) Maybe simply remove it?

Aligned!

[terelius, 2016-05-23 11:49:56]

https://codereview.webrtc.org/1988653002/diff/1/webrtc/modules/video_coding/f...
File webrtc/modules/video_coding/frame_object.h (right):

https://codereview.webrtc.org/1988653002/diff/1/webrtc/modules/video_coding/f...
webrtc/modules/video_coding/frame_object.h:67: int8_t max_nack_count_;
On 2016/05/23 09:19:22, philipel wrote:
> On 2016/05/20 13:10:23, stefan-webrtc (holmer) wrote:
> > Why int8_t? int would be nicer
> 
> Changed to int.
> 
> The reason I use int8_t here is because a FrameObject is something we might
> potentially have many instances of and therefore size might make a difference.

If you keep this as an int8_t and you are going to (semi-implicitly) cast the
ints to int8_t, I think would be better to have int8_t everywhere.

https://codereview.webrtc.org/1988653002/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/frame_object.h (right):

https://codereview.webrtc.org/1988653002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/frame_object.h:68: int max_nack_count_;
Could you document that this is the maximum nack count among the packets. (Based
only on the variable name, I would have guessed that it is a limit on how many
times we are allowed to nack this frame.)

https://codereview.webrtc.org/1988653002/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/packet.cc (right):

https://codereview.webrtc.org/1988653002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/packet.cc:27: timesNacked(-1),
Why does a newly constructed (that has not been nacked yet) have timesNacked =
-1? Wouldn't zero be more natural?

[stefan-webrtc, 2016-05-23 13:01:25]

https://codereview.webrtc.org/1988653002/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/frame_object.h (right):

https://codereview.webrtc.org/1988653002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/frame_object.h:51: int8_t max_nack_count);
int

https://codereview.webrtc.org/1988653002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/frame_object.h:56: int8_t max_nack_count() const;
int

https://codereview.webrtc.org/1988653002/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/packet.h (right):

https://codereview.webrtc.org/1988653002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/packet.h:42: int timesNacked;
Could you align these members so that they are following a single style,
preferably in a follow-up? I'm thinking about line 37. Preferably all would be:
int times_nacked;
etc.

https://codereview.webrtc.org/1988653002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/packet.h:55: 
Remove one empty line

https://codereview.webrtc.org/1988653002/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/packet_buffer.cc (right):

https://codereview.webrtc.org/1988653002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/packet_buffer.cc:147: while
(!sequence_buffer_[start_index].frame_begin) {
Can this be a do ... while instead so that you don't have to repeat lines
148-151 at lines 155 - 158?

https://codereview.webrtc.org/1988653002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/packet_buffer.cc:149: max_nack_count =
std::max<int8_t>(
int

https://codereview.webrtc.org/1988653002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/packet_buffer.cc:156: max_nack_count =
std::max<int8_t>(
int

[philipel, 2016-05-23 13:34:18]

https://codereview.webrtc.org/1988653002/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/frame_object.h (right):

https://codereview.webrtc.org/1988653002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/frame_object.h:51: int8_t max_nack_count);
On 2016/05/23 13:01:24, stefan-webrtc (holmer) wrote:
> int

Done.

https://codereview.webrtc.org/1988653002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/frame_object.h:56: int8_t max_nack_count() const;
On 2016/05/23 13:01:24, stefan-webrtc (holmer) wrote:
> int

Done.

https://codereview.webrtc.org/1988653002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/frame_object.h:68: int max_nack_count_;
On 2016/05/23 11:49:55, terelius wrote:
> Could you document that this is the maximum nack count among the packets.
(Based
> only on the variable name, I would have guessed that it is a limit on how many
> times we are allowed to nack this frame.)

Done.

https://codereview.webrtc.org/1988653002/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/packet.cc (right):

https://codereview.webrtc.org/1988653002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/packet.cc:27: timesNacked(-1),
On 2016/05/23 11:49:55, terelius wrote:
> Why does a newly constructed (that has not been nacked yet) have timesNacked =
> -1? Wouldn't zero be more natural?

-1 mean we don't know, so 0 can mean 0.

https://codereview.webrtc.org/1988653002/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/packet.h (right):

https://codereview.webrtc.org/1988653002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/packet.h:42: int timesNacked;
On 2016/05/23 13:01:25, stefan-webrtc (holmer) wrote:
> Could you align these members so that they are following a single style,
> preferably in a follow-up? I'm thinking about line 37. Preferably all would
be:
> int times_nacked;
> etc.

You mean change the format of the variable names? Sure, I can do that in some
follow-up CL...

https://codereview.webrtc.org/1988653002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/packet.h:55: 
On 2016/05/23 13:01:24, stefan-webrtc (holmer) wrote:
> Remove one empty line

Done.

https://codereview.webrtc.org/1988653002/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/packet_buffer.cc (right):

https://codereview.webrtc.org/1988653002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/packet_buffer.cc:147: while
(!sequence_buffer_[start_index].frame_begin) {
On 2016/05/23 13:01:25, stefan-webrtc (holmer) wrote:
> Can this be a do ... while instead so that you don't have to repeat lines
> 148-151 at lines 155 - 158?

Cleaned it up a bit, can't really use do-while in this case.

https://codereview.webrtc.org/1988653002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/packet_buffer.cc:149: max_nack_count =
std::max<int8_t>(
On 2016/05/23 13:01:25, stefan-webrtc (holmer) wrote:
> int

Done.

[stefan-webrtc, 2016-05-23 13:42:07]

lgtm

https://codereview.webrtc.org/1988653002/diff/60001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/frame_object.cc (right):

https://codereview.webrtc.org/1988653002/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/frame_object.cc:34: size = frame_size;
Maybe pass this in to the FrameObject via constructor instead?

[philipel, 2016-05-23 13:52:01]

The CQ bit was checked by philipel@webrtc.org

[commit-bot: I haz the power, 2016-05-23 13:52:17]

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

[commit-bot: I haz the power, 2016-05-23 15:52:41]

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

[commit-bot: I haz the power, 2016-05-23 15:52:42]

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

[philipel, 2016-05-24 08:21:03]

Description was changed from

==========
PacketBuffer now can save how many times a packet has been nacked.
Also save size/max nack count in the FrameObject/RtpFrameObject.

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

to

==========
PacketBuffer now can save how many times a packet has been nacked.
Also save size/max nack count in the FrameObject/RtpFrameObject.

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

Committed:
https://chromium.googlesource.com/external/webrtc/+/5ceaaae3689cac9ae10dc7caf...
==========

[philipel, 2016-05-24 08:21:05]

Committed patchset #4 (id:60001) manually as
5ceaaae3689cac9ae10dc7caf5802e8d6e0c7a99 (presubmit successful).

[commit-bot: I haz the power, 2016-05-24 08:21:07]

Description was changed from

==========
PacketBuffer now can save how many times a packet has been nacked.
Also save size/max nack count in the FrameObject/RtpFrameObject.

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

Committed:
https://chromium.googlesource.com/external/webrtc/+/5ceaaae3689cac9ae10dc7caf...
==========

to

==========
PacketBuffer now can save how many times a packet has been nacked.
Also save size/max nack count in the FrameObject/RtpFrameObject.

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

Committed: https://crrev.com/5ceaaae3689cac9ae10dc7caf5802e8d6e0c7a99
Cr-Commit-Position: refs/heads/master@{#12863}
==========