Removed RTPHeader from NetEq's Packet struct.

Removed RTPHeader from NetEq's Packet struct.

Only three items in the (rather large) header were actually used after
InsertPacket: payloadType, timestamp and sequenceNumber. They are now
put directly into Packet. This saves 129 bytes per Packet that no
longer need to be allocated and deallocated.

This also works towards decoupling NetEq from RTP. As part of that,
I've moved the NACK code earlier in InsertPacketInternal, together
with other things that directly reference the RTPHeader.

BUG=webrtc:6549
Committed: https://crrev.com/7a3776102f55bd5927b84a853321672577dd171f
Cr-Commit-Position: refs/heads/master@{#14658}

[ossu, 2016-10-12 20:08:03]

ossu@webrtc.org changed reviewers:
+ henrik.lundin@webrtc.org

[ossu, 2016-10-12 20:08:03]

After having a chat with pthatcher, I got inspired to attempt this change again.
I attempted something similar around the time of
https://codereview.webrtc.org/2289093003 but had more pressing issues to work
on.

Apart from it being slightly more efficient not to have to lug around a bit
RTPHeader for each packet, it also helps clarify exactly what NetEq requires
from the transport layer to do its job. There are still some RTP specific things
in InsertPacketInternal, but once a packet is in the packet buffer, we can rely
only on the fields in Packet.

I'm hoping this change will also make it possible to make Packet a value type
that gets moved around, rather than allocating each packet on the heap. Even
though the payload can be moved, the rest of the fields need copying, and having
a whole RTPHeader in there got in the way of that being efficient.

[ossu, 2016-10-12 20:08:48]

On 2016/10/12 20:08:03, ossu wrote:
> Apart from it being slightly more efficient not to have to lug around a bit
> RTPHeader for each packet, it also helps clarify exactly what NetEq requires

That should of course be "a big RTPHeader". Time for lunch!

[hlundin-webrtc, 2016-10-12 20:39:58]

Veeeery nice! Just a few minor comments.

https://codereview.webrtc.org/2411183003/diff/1/webrtc/modules/audio_coding/n...
File webrtc/modules/audio_coding/neteq/decision_logic.h (right):

https://codereview.webrtc.org/2411183003/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/decision_logic.h:70: // packet header should
be supplied in |packet_header|; otherwise it should
Fix the comment, still mentioning packet_header.

https://codereview.webrtc.org/2411183003/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/decision_logic.h:128: // packet header should
be supplied in |packet_header|; otherwise it should
And here.

https://codereview.webrtc.org/2411183003/diff/1/webrtc/modules/audio_coding/n...
File webrtc/modules/audio_coding/neteq/decision_logic_fax.h (right):

https://codereview.webrtc.org/2411183003/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/decision_logic_fax.h:46: // packet header
should be supplied in |packet_header|; otherwise it should
Fix, or if the comment is identical to the one in the base class (which I
believe it is), delete the comment altogether.

https://codereview.webrtc.org/2411183003/diff/1/webrtc/modules/audio_coding/n...
File webrtc/modules/audio_coding/neteq/decision_logic_normal.h (right):

https://codereview.webrtc.org/2411183003/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/decision_logic_normal.h:50: // packet header
should be supplied in |packet_header|; otherwise it should
Same, same as before.

https://codereview.webrtc.org/2411183003/diff/1/webrtc/modules/audio_coding/n...
File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right):

https://codereview.webrtc.org/2411183003/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:650: // Update the main packet
pointer since the main payload has now changed.
You are not updating the main packet pointer here...

https://codereview.webrtc.org/2411183003/diff/1/webrtc/modules/audio_coding/n...
File webrtc/modules/audio_coding/neteq/packet.h (right):

https://codereview.webrtc.org/2411183003/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/packet.h:20: #include
"webrtc/modules/include/module_common_types.h"
Can you ditch module_common_types.h now?

https://codereview.webrtc.org/2411183003/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/packet.h:70: uint16_t sequenceNumber;
Please, make the member names follow the style guide: sequence_number and
payload_type. I would also argue that payload_type should be an int. The other
two need defined wrap-around to work properly, motivating the uints. (I do
appreciate that not changing the names in the first patch set makes reviewing
easier, so please, change the names in a separate patch set.)

[ossu, 2016-10-12 21:46:03]

https://codereview.webrtc.org/2411183003/diff/1/webrtc/modules/audio_coding/n...
File webrtc/modules/audio_coding/neteq/decision_logic.h (right):

https://codereview.webrtc.org/2411183003/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/decision_logic.h:70: // packet header should
be supplied in |packet_header|; otherwise it should
On 2016/10/12 20:39:57, hlundin-webrtc wrote:
> Fix the comment, still mentioning packet_header.

Done.

https://codereview.webrtc.org/2411183003/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/decision_logic.h:128: // packet header should
be supplied in |packet_header|; otherwise it should
On 2016/10/12 20:39:57, hlundin-webrtc wrote:
> And here.

Done.

https://codereview.webrtc.org/2411183003/diff/1/webrtc/modules/audio_coding/n...
File webrtc/modules/audio_coding/neteq/decision_logic_fax.h (right):

https://codereview.webrtc.org/2411183003/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/decision_logic_fax.h:46: // packet header
should be supplied in |packet_header|; otherwise it should
On 2016/10/12 20:39:57, hlundin-webrtc wrote:
> Fix, or if the comment is identical to the one in the base class (which I
> believe it is), delete the comment altogether.

Done.

https://codereview.webrtc.org/2411183003/diff/1/webrtc/modules/audio_coding/n...
File webrtc/modules/audio_coding/neteq/decision_logic_normal.h (right):

https://codereview.webrtc.org/2411183003/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/decision_logic_normal.h:50: // packet header
should be supplied in |packet_header|; otherwise it should
On 2016/10/12 20:39:57, hlundin-webrtc wrote:
> Same, same as before.

Done.

https://codereview.webrtc.org/2411183003/diff/1/webrtc/modules/audio_coding/n...
File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right):

https://codereview.webrtc.org/2411183003/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:650: // Update the main packet
pointer since the main payload has now changed.
On 2016/10/12 20:39:57, hlundin-webrtc wrote:
> You are not updating the main packet pointer here...

Heh, that's true!

https://codereview.webrtc.org/2411183003/diff/1/webrtc/modules/audio_coding/n...
File webrtc/modules/audio_coding/neteq/packet.h (right):

https://codereview.webrtc.org/2411183003/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/packet.h:20: #include
"webrtc/modules/include/module_common_types.h"
On 2016/10/12 20:39:57, hlundin-webrtc wrote:
> Can you ditch module_common_types.h now?

Yes, but I'll have to put it in packet_buffer.h instead.

https://codereview.webrtc.org/2411183003/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/packet.h:70: uint16_t sequenceNumber;
On 2016/10/12 20:39:58, hlundin-webrtc wrote:
> Please, make the member names follow the style guide: sequence_number and
> payload_type. I would also argue that payload_type should be an int. The other
> two need defined wrap-around to work properly, motivating the uints. (I do
> appreciate that not changing the names in the first patch set makes reviewing
> easier, so please, change the names in a separate patch set.)

Payload types are uint8_t in so many other places, I don't think it makes sense
to change it just here. It could be changed throughout in a separate CL.

Since they have a fixed, defined range of 0-127, I think, if anything, they
should be int8_t. That way we get away from the unsigned type (which the style
guide likes) and can check for invalidity by checking if payload type < 0
whenever we want to do such things.

Finally, eventually we'll probably move away from referencing payload types
directly down here, but leave that to an RTP specific layer above NetEq, to
accommodate other transports. We might as well tackle this issue at that time.

https://codereview.webrtc.org/2411183003/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/packet_buffer.h (right):

https://codereview.webrtc.org/2411183003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/packet_buffer.h:17: #include
"webrtc/modules/include/module_common_types.h"
Needed because IsObsoleteTimestamp is defined inline, below, and it uses
IsNewerTimestamp from module_common_types.h.

[hlundin-webrtc, 2016-10-13 07:54:19]

lgtm

https://codereview.webrtc.org/2411183003/diff/1/webrtc/modules/audio_coding/n...
File webrtc/modules/audio_coding/neteq/packet.h (right):

https://codereview.webrtc.org/2411183003/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/packet.h:20: #include
"webrtc/modules/include/module_common_types.h"
On 2016/10/12 21:46:02, ossu wrote:
> On 2016/10/12 20:39:57, hlundin-webrtc wrote:
> > Can you ditch module_common_types.h now?
> 
> Yes, but I'll have to put it in packet_buffer.h instead.

Acknowledged.

https://codereview.webrtc.org/2411183003/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/packet.h:70: uint16_t sequenceNumber;
On 2016/10/12 21:46:02, ossu wrote:
> On 2016/10/12 20:39:58, hlundin-webrtc wrote:
> > Please, make the member names follow the style guide: sequence_number and
> > payload_type. I would also argue that payload_type should be an int. The
other
> > two need defined wrap-around to work properly, motivating the uints. (I do
> > appreciate that not changing the names in the first patch set makes
reviewing
> > easier, so please, change the names in a separate patch set.)
> 
> Payload types are uint8_t in so many other places, I don't think it makes
sense
> to change it just here. It could be changed throughout in a separate CL.
> 
> Since they have a fixed, defined range of 0-127, I think, if anything, they
> should be int8_t. That way we get away from the unsigned type (which the style
> guide likes) and can check for invalidity by checking if payload type < 0
> whenever we want to do such things.
> 
> Finally, eventually we'll probably move away from referencing payload types
> directly down here, but leave that to an RTP specific layer above NetEq, to
> accommodate other transports. We might as well tackle this issue at that time.

Acknowledged.

[ossu, 2016-10-18 10:23:20]

The CQ bit was checked by ossu@webrtc.org

[commit-bot: I haz the power, 2016-10-18 10:23:25]

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, 2016-10-18 10:31:46]

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

[commit-bot: I haz the power, 2016-10-18 10:31:48]

Try jobs failed on following builders:
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/9261)

[ossu, 2016-10-18 11:03:50]

Description was changed from

==========
Removed RTPHeader from NetEq's Packet struct.

Only three items in the (rather large) header were actually used after
InsertPacket: payloadType, timestamp and sequenceNumber. They are now
put directly into Packet. This saves 129 bytes per Packet that no
longer need to be allocated and deallocated.

This also works towards decoupling NetEq from RTP. As part of that,
I've moved the NACK code earlier in InsertPacketInternal, together
with other things that directly reference the RTPHeader.

BUG=
==========

to

==========
Removed RTPHeader from NetEq's Packet struct.

Only three items in the (rather large) header were actually used after
InsertPacket: payloadType, timestamp and sequenceNumber. They are now
put directly into Packet. This saves 129 bytes per Packet that no
longer need to be allocated and deallocated.

This also works towards decoupling NetEq from RTP. As part of that,
I've moved the NACK code earlier in InsertPacketInternal, together
with other things that directly reference the RTPHeader.

BUG=webrtc:6549
==========

[ossu, 2016-10-18 11:04:29]

The CQ bit was checked by ossu@webrtc.org

[commit-bot: I haz the power, 2016-10-18 11:04:39]

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, 2016-10-18 11:06:16]

Description was changed from

==========
Removed RTPHeader from NetEq's Packet struct.

Only three items in the (rather large) header were actually used after
InsertPacket: payloadType, timestamp and sequenceNumber. They are now
put directly into Packet. This saves 129 bytes per Packet that no
longer need to be allocated and deallocated.

This also works towards decoupling NetEq from RTP. As part of that,
I've moved the NACK code earlier in InsertPacketInternal, together
with other things that directly reference the RTPHeader.

BUG=webrtc:6549
==========

to

==========
Removed RTPHeader from NetEq's Packet struct.

Only three items in the (rather large) header were actually used after
InsertPacket: payloadType, timestamp and sequenceNumber. They are now
put directly into Packet. This saves 129 bytes per Packet that no
longer need to be allocated and deallocated.

This also works towards decoupling NetEq from RTP. As part of that,
I've moved the NACK code earlier in InsertPacketInternal, together
with other things that directly reference the RTPHeader.

BUG=webrtc:6549
==========

[commit-bot: I haz the power, 2016-10-18 11:06:17]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-10-18 11:06:24]

Description was changed from

==========
Removed RTPHeader from NetEq's Packet struct.

Only three items in the (rather large) header were actually used after
InsertPacket: payloadType, timestamp and sequenceNumber. They are now
put directly into Packet. This saves 129 bytes per Packet that no
longer need to be allocated and deallocated.

This also works towards decoupling NetEq from RTP. As part of that,
I've moved the NACK code earlier in InsertPacketInternal, together
with other things that directly reference the RTPHeader.

BUG=webrtc:6549
==========

to

==========
Removed RTPHeader from NetEq's Packet struct.

Only three items in the (rather large) header were actually used after
InsertPacket: payloadType, timestamp and sequenceNumber. They are now
put directly into Packet. This saves 129 bytes per Packet that no
longer need to be allocated and deallocated.

This also works towards decoupling NetEq from RTP. As part of that,
I've moved the NACK code earlier in InsertPacketInternal, together
with other things that directly reference the RTPHeader.

BUG=webrtc:6549

Committed: https://crrev.com/7a3776102f55bd5927b84a853321672577dd171f
Cr-Commit-Position: refs/heads/master@{#14658}
==========

[commit-bot: I haz the power, 2016-10-18 11:06:26]

Patchset 2 (id:??) landed as
https://crrev.com/7a3776102f55bd5927b84a853321672577dd171f
Cr-Commit-Position: refs/heads/master@{#14658}