NetEq now works with packets as values, rather than pointers.

NetEq now works with packets as values, rather than pointers.

PacketList is now list<Packet> instead of list<Packet*>.
Splicing the lists in NetEqImpl::InsertPacketInternal instead of
moving packets. Avoid moving the packet when doing Rfc3389Cng.
Removed PacketBuffer::DeleteFirstPacket and DeleteAllPackets.

BUG=chromium:657300
Committed: https://crrev.com/a73f6c9726322021446696faf47c93f431f6104a
Cr-Commit-Position: refs/heads/master@{#14747}

[ossu, 2016-10-18 12:25:14]

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

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

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, 2016-10-18 12:56:55]

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

[commit-bot: I haz the power, 2016-10-18 12:56:56]

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

[ossu, 2016-10-19 15:28:04]

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

[ossu, 2016-10-19 15:28:05]

This CL changes NetEqs PacketList to be std::list<Packet> rather than
std::list<Packet*>. Packet is a move-only type[1] and since it's now only 56
bytes, it should be quick to copy the bits of data that cannot be moved
(essentially everything but the payload). Additionally, since the packets are
put in a linked list, they should only need moving upon insertion and extraction
- moving the nodes around in the packet buffer should still cost as much as
before.

I've ran the change through NetEqPerformanceTest several times the change seems
minimally for the better in Release mode and slightly slower in Debug mode
(compared to master).

[ossu, 2016-10-19 15:40:42]

Description was changed from

==========
NetEq now works with packets as values, rather than pointers.

PacketList is now list<Packet> instead of list<Packet*>.
Splicing the lists in NetEqImpl::InsertPacketInternal instead of
moving packets. Avoid moving the packet when doing Rfc3389Cng.
Removed PacketBuffer::DeleteFirstPacket and DeleteAllPackets.

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

to

==========
NetEq now works with packets as values, rather than pointers.

PacketList is now list<Packet> instead of list<Packet*>.
Splicing the lists in NetEqImpl::InsertPacketInternal instead of
moving packets. Avoid moving the packet when doing Rfc3389Cng.
Removed PacketBuffer::DeleteFirstPacket and DeleteAllPackets.

BUG=chromium:657300
==========

[kwiberg-webrtc, 2016-10-20 22:39:23]

https://codereview.chromium.org/2425223002/diff/1/webrtc/modules/audio_coding...
File webrtc/modules/audio_coding/neteq/mock/mock_packet_buffer.h (right):

https://codereview.chromium.org/2425223002/diff/1/webrtc/modules/audio_coding...
webrtc/modules/audio_coding/neteq/mock/mock_packet_buffer.h:37: int(Packet&
packet));
It would be more style guide compliant to pass by pointer.

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

https://codereview.chromium.org/2425223002/diff/1/webrtc/modules/audio_coding...
webrtc/modules/audio_coding/neteq/packet.h:78: 
Why the extra blank line?

https://codereview.chromium.org/2425223002/diff/1/webrtc/modules/audio_coding...
File webrtc/modules/audio_coding/neteq/packet_buffer.cc (right):

https://codereview.chromium.org/2425223002/diff/1/webrtc/modules/audio_coding...
webrtc/modules/audio_coding/neteq/packet_buffer.cc:33: bool operator()(Packet&
packet) {
const?

https://codereview.chromium.org/2425223002/diff/1/webrtc/modules/audio_coding...
webrtc/modules/audio_coding/neteq/packet_buffer.cc:100: if (rit !=
buffer_.rend() && packet.timestamp == (*rit).timestamp) {
(*x).foo can be written as x->foo...

https://codereview.chromium.org/2425223002/diff/1/webrtc/modules/audio_coding...
webrtc/modules/audio_coding/neteq/packet_buffer.cc:108: if (it != buffer_.end()
&& packet.timestamp == (*it).timestamp) {
Same here.

https://codereview.chromium.org/2425223002/diff/1/webrtc/modules/audio_coding...
webrtc/modules/audio_coding/neteq/packet_buffer.cc:182: if ((*it).timestamp >=
timestamp) {
->

https://codereview.chromium.org/2425223002/diff/1/webrtc/modules/audio_coding...
webrtc/modules/audio_coding/neteq/packet_buffer.cc:184: *next_timestamp =
(*it).timestamp;
->

https://codereview.chromium.org/2425223002/diff/1/webrtc/modules/audio_coding...
webrtc/modules/audio_coding/neteq/packet_buffer.cc:237: Packet& packet = *it;
const? And can you use cbegin/cend so that the iterator is a const_iterator?

https://codereview.chromium.org/2425223002/diff/1/webrtc/modules/audio_coding...
File webrtc/modules/audio_coding/neteq/packet_buffer_unittest.cc (right):

https://codereview.chromium.org/2425223002/diff/1/webrtc/modules/audio_coding...
webrtc/modules/audio_coding/neteq/packet_buffer_unittest.cc:91: Packet packet =
gen.NextPacket(payload_len);
const?

https://codereview.chromium.org/2425223002/diff/1/webrtc/modules/audio_coding...
webrtc/modules/audio_coding/neteq/packet_buffer_unittest.cc:144: Packet packet =
gen.NextPacket(payload_len);
const?

https://codereview.chromium.org/2425223002/diff/1/webrtc/modules/audio_coding...
webrtc/modules/audio_coding/neteq/packet_buffer_unittest.cc:287:
rtc::Optional<Packet> packet = buffer.GetNextPacket();
const?

https://codereview.chromium.org/2425223002/diff/1/webrtc/modules/audio_coding...
webrtc/modules/audio_coding/neteq/packet_buffer_unittest.cc:364:
rtc::Optional<Packet> packet = buffer.GetNextPacket();
const

https://codereview.chromium.org/2425223002/diff/1/webrtc/modules/audio_coding...
File webrtc/modules/audio_coding/neteq/red_payload_splitter.cc (right):

https://codereview.chromium.org/2425223002/diff/1/webrtc/modules/audio_coding...
webrtc/modules/audio_coding/neteq/red_payload_splitter.cc:37: const Packet&
red_packet = (*it);
Redundant parentheses.

https://codereview.chromium.org/2425223002/diff/1/webrtc/modules/audio_coding...
webrtc/modules/audio_coding/neteq/red_payload_splitter.cc:139: uint8_t
this_payload_type = (*it).payload_type;
->

[kwiberg-webrtc, 2016-10-20 22:39:52]

Excellent improvement!

[ossu, 2016-10-21 12:54:42]

Thanks! :D

https://codereview.chromium.org/2425223002/diff/1/webrtc/modules/audio_coding...
File webrtc/modules/audio_coding/neteq/mock/mock_packet_buffer.h (right):

https://codereview.chromium.org/2425223002/diff/1/webrtc/modules/audio_coding...
webrtc/modules/audio_coding/neteq/mock/mock_packet_buffer.h:37: int(Packet&
packet));
On 2016/10/20 22:39:22, kwiberg-webrtc wrote:
> It would be more style guide compliant to pass by pointer.

Done... I miss references :(

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

https://codereview.chromium.org/2425223002/diff/1/webrtc/modules/audio_coding...
webrtc/modules/audio_coding/neteq/packet.h:78: 
On 2016/10/20 22:39:22, kwiberg-webrtc wrote:
> Why the extra blank line?

To give the reader some time to breathe! (fixed)

https://codereview.chromium.org/2425223002/diff/1/webrtc/modules/audio_coding...
File webrtc/modules/audio_coding/neteq/packet_buffer.cc (right):

https://codereview.chromium.org/2425223002/diff/1/webrtc/modules/audio_coding...
webrtc/modules/audio_coding/neteq/packet_buffer.cc:33: bool operator()(Packet&
packet) {
On 2016/10/20 22:39:22, kwiberg-webrtc wrote:
> const?

yas!

https://codereview.chromium.org/2425223002/diff/1/webrtc/modules/audio_coding...
webrtc/modules/audio_coding/neteq/packet_buffer.cc:100: if (rit !=
buffer_.rend() && packet.timestamp == (*rit).timestamp) {
On 2016/10/20 22:39:23, kwiberg-webrtc wrote:
> (*x).foo can be written as x->foo...

Done.

https://codereview.chromium.org/2425223002/diff/1/webrtc/modules/audio_coding...
webrtc/modules/audio_coding/neteq/packet_buffer.cc:108: if (it != buffer_.end()
&& packet.timestamp == (*it).timestamp) {
On 2016/10/20 22:39:23, kwiberg-webrtc wrote:
> Same here.

Done.

https://codereview.chromium.org/2425223002/diff/1/webrtc/modules/audio_coding...
webrtc/modules/audio_coding/neteq/packet_buffer.cc:182: if ((*it).timestamp >=
timestamp) {
On 2016/10/20 22:39:23, kwiberg-webrtc wrote:
> ->

Done.

https://codereview.chromium.org/2425223002/diff/1/webrtc/modules/audio_coding...
webrtc/modules/audio_coding/neteq/packet_buffer.cc:184: *next_timestamp =
(*it).timestamp;
On 2016/10/20 22:39:22, kwiberg-webrtc wrote:
> ->

Done.

https://codereview.chromium.org/2425223002/diff/1/webrtc/modules/audio_coding...
webrtc/modules/audio_coding/neteq/packet_buffer.cc:237: Packet& packet = *it;
On 2016/10/20 22:39:22, kwiberg-webrtc wrote:
> const? And can you use cbegin/cend so that the iterator is a const_iterator?

Nope! Can't erase from a const_iterator (which makes sense, imo). Made the
Packet& const, tho.

https://codereview.chromium.org/2425223002/diff/1/webrtc/modules/audio_coding...
File webrtc/modules/audio_coding/neteq/packet_buffer_unittest.cc (right):

https://codereview.chromium.org/2425223002/diff/1/webrtc/modules/audio_coding...
webrtc/modules/audio_coding/neteq/packet_buffer_unittest.cc:91: Packet packet =
gen.NextPacket(payload_len);
On 2016/10/20 22:39:23, kwiberg-webrtc wrote:
> const?

Done.

https://codereview.chromium.org/2425223002/diff/1/webrtc/modules/audio_coding...
webrtc/modules/audio_coding/neteq/packet_buffer_unittest.cc:144: Packet packet =
gen.NextPacket(payload_len);
On 2016/10/20 22:39:23, kwiberg-webrtc wrote:
> const?

Done.

https://codereview.chromium.org/2425223002/diff/1/webrtc/modules/audio_coding...
webrtc/modules/audio_coding/neteq/packet_buffer_unittest.cc:287:
rtc::Optional<Packet> packet = buffer.GetNextPacket();
On 2016/10/20 22:39:23, kwiberg-webrtc wrote:
> const?

Done.

https://codereview.chromium.org/2425223002/diff/1/webrtc/modules/audio_coding...
webrtc/modules/audio_coding/neteq/packet_buffer_unittest.cc:364:
rtc::Optional<Packet> packet = buffer.GetNextPacket();
On 2016/10/20 22:39:23, kwiberg-webrtc wrote:
> const

Done.

https://codereview.chromium.org/2425223002/diff/1/webrtc/modules/audio_coding...
File webrtc/modules/audio_coding/neteq/red_payload_splitter.cc (right):

https://codereview.chromium.org/2425223002/diff/1/webrtc/modules/audio_coding...
webrtc/modules/audio_coding/neteq/red_payload_splitter.cc:37: const Packet&
red_packet = (*it);
On 2016/10/20 22:39:23, kwiberg-webrtc wrote:
> Redundant parentheses.

Done.

https://codereview.chromium.org/2425223002/diff/1/webrtc/modules/audio_coding...
webrtc/modules/audio_coding/neteq/red_payload_splitter.cc:139: uint8_t
this_payload_type = (*it).payload_type;
On 2016/10/20 22:39:23, kwiberg-webrtc wrote:
> ->

Done.

[hlundin-webrtc, 2016-10-21 13:53:33]

Solid gold! LGTM with two minor comments.

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

https://codereview.chromium.org/2425223002/diff/1/webrtc/modules/audio_coding...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:668: Packet& current_packet =
(*it);
const Packet&

https://codereview.chromium.org/2425223002/diff/20001/webrtc/modules/audio_co...
File webrtc/modules/audio_coding/neteq/packet_buffer_unittest.cc (right):

https://codereview.chromium.org/2425223002/diff/20001/webrtc/modules/audio_co...
webrtc/modules/audio_coding/neteq/packet_buffer_unittest.cc:288:
EXPECT_TRUE(packet);
Can't you just do EXPECT_EQ(packet, rtc::Optional<Packet>(expect_order[i])) ?
(Btw, I think the expected and actual values are mixed up in the EXPECT_EQ.)

If that does not work, then I'd prefer
ASSERT_TRUE(packet);
and then skip the if(packet.

[kwiberg-webrtc, 2016-10-24 11:42:25]

lgtm

https://codereview.chromium.org/2425223002/diff/1/webrtc/modules/audio_coding...
File webrtc/modules/audio_coding/neteq/packet_buffer.cc (right):

https://codereview.chromium.org/2425223002/diff/1/webrtc/modules/audio_coding...
webrtc/modules/audio_coding/neteq/packet_buffer.cc:237: Packet& packet = *it;
On 2016/10/21 12:54:41, ossu wrote:
> On 2016/10/20 22:39:22, kwiberg-webrtc wrote:
> > const? And can you use cbegin/cend so that the iterator is a const_iterator?
> 
> Nope! Can't erase from a const_iterator (which makes sense, imo). Made the
> Packet& const, tho.

Acknowledged.

https://codereview.chromium.org/2425223002/diff/20001/webrtc/modules/audio_co...
File webrtc/modules/audio_coding/neteq/packet_buffer_unittest.cc (right):

https://codereview.chromium.org/2425223002/diff/20001/webrtc/modules/audio_co...
webrtc/modules/audio_coding/neteq/packet_buffer_unittest.cc:288:
EXPECT_TRUE(packet);
On 2016/10/21 13:53:33, hlundin-webrtc wrote:
> Can't you just do EXPECT_EQ(packet, rtc::Optional<Packet>(expect_order[i])) ?
> (Btw, I think the expected and actual values are mixed up in the EXPECT_EQ.)

+1.

[ossu, 2016-10-24 12:05:51]

Patchset #3 (id:40001) has been deleted

[ossu, 2016-10-24 12:18:28]

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

https://codereview.chromium.org/2425223002/diff/1/webrtc/modules/audio_coding...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:668: Packet& current_packet =
(*it);
On 2016/10/21 13:53:33, hlundin-webrtc wrote:
> const Packet&

Done.

https://codereview.chromium.org/2425223002/diff/20001/webrtc/modules/audio_co...
File webrtc/modules/audio_coding/neteq/packet_buffer_unittest.cc (right):

https://codereview.chromium.org/2425223002/diff/20001/webrtc/modules/audio_co...
webrtc/modules/audio_coding/neteq/packet_buffer_unittest.cc:288:
EXPECT_TRUE(packet);
On 2016/10/24 11:42:25, kwiberg-webrtc wrote:
> On 2016/10/21 13:53:33, hlundin-webrtc wrote:
> > Can't you just do EXPECT_EQ(packet, rtc::Optional<Packet>(expect_order[i]))
?
> > (Btw, I think the expected and actual values are mixed up in the EXPECT_EQ.)
> 
> +1.

Yes and no. I'd have to move the packet into the new temporary Optional to do
the comparison. However, with a recent change by danilchap@, we can now do a
comparison between the object directly.

[ossu, 2016-10-24 12:18:48]

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

[commit-bot: I haz the power, 2016-10-24 12:18:50]

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, 2016-10-24 14:19:20]

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

[commit-bot: I haz the power, 2016-10-24 14:19:21]

Dry run: Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_clang_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_compile_mips_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build
URL)
  android_compile_x64_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build
URL)
  android_compile_x86_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build
URL)
  android_compile_x86_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build
URL)
  android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_gyp_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_arm on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_asan on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_compile_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_gyp_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_libfuzzer_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_msan on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_tsan2 on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_ubsan on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_ubsan_vptr on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[ossu, 2016-10-24 15:22:42]

The CQ bit was checked by ossu@webrtc.org

[ossu, 2016-10-24 15:22:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
henrik.lundin@webrtc.org, kwiberg@webrtc.org
Link to the patchset: https://codereview.chromium.org/2425223002/#ps80001
(title: "Compare packets better in test. One more const.")

[commit-bot: I haz the power, 2016-10-24 15:22:48]

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-24 15:25:30]

Description was changed from

==========
NetEq now works with packets as values, rather than pointers.

PacketList is now list<Packet> instead of list<Packet*>.
Splicing the lists in NetEqImpl::InsertPacketInternal instead of
moving packets. Avoid moving the packet when doing Rfc3389Cng.
Removed PacketBuffer::DeleteFirstPacket and DeleteAllPackets.

BUG=chromium:657300
==========

to

==========
NetEq now works with packets as values, rather than pointers.

PacketList is now list<Packet> instead of list<Packet*>.
Splicing the lists in NetEqImpl::InsertPacketInternal instead of
moving packets. Avoid moving the packet when doing Rfc3389Cng.
Removed PacketBuffer::DeleteFirstPacket and DeleteAllPackets.

BUG=chromium:657300
==========

[commit-bot: I haz the power, 2016-10-24 15:25:32]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-10-24 15:25:36]

Description was changed from

==========
NetEq now works with packets as values, rather than pointers.

PacketList is now list<Packet> instead of list<Packet*>.
Splicing the lists in NetEqImpl::InsertPacketInternal instead of
moving packets. Avoid moving the packet when doing Rfc3389Cng.
Removed PacketBuffer::DeleteFirstPacket and DeleteAllPackets.

BUG=chromium:657300
==========

to

==========
NetEq now works with packets as values, rather than pointers.

PacketList is now list<Packet> instead of list<Packet*>.
Splicing the lists in NetEqImpl::InsertPacketInternal instead of
moving packets. Avoid moving the packet when doing Rfc3389Cng.
Removed PacketBuffer::DeleteFirstPacket and DeleteAllPackets.

BUG=chromium:657300

Committed: https://crrev.com/a73f6c9726322021446696faf47c93f431f6104a
Cr-Commit-Position: refs/heads/master@{#14747}
==========

[commit-bot: I haz the power, 2016-10-24 15:25:37]

Patchset 4 (id:??) landed as
https://crrev.com/a73f6c9726322021446696faf47c93f431f6104a
Cr-Commit-Position: refs/heads/master@{#14747}