NetEq: Changed Packet::payload to be an rtc::Buffer

NetEq: Changed Packet::payload to be an rtc::Buffer

That is, rather than keeping a separate pointer and size.
This helps automate memory management in NetEq and will be useful in the
work to minimize the AudioDecoder interface as part of the injectable
audio codec work.

I'm planning a follow-up that will change the current management of Packet* to wrapping them in unique_ptr instead.

Committed: https://crrev.com/dc431ce07e3e7feb46a00886aa5b5c9684761758
Cr-Commit-Position: refs/heads/master@{#14002}

[ossu, 2016-08-30 15:19:05]

Description was changed from

==========
NetEq: Changed Packet::payload to be an rtc::Buffer

That is, rather than keeping a separate pointer and size.
This helps automate memory management in NetEq and will be useful in the
work to minimize the AudioDecoder interface as part of the injectable
audio codec work.
==========

to

==========
NetEq: Changed Packet::payload to be an rtc::Buffer

That is, rather than keeping a separate pointer and size.
This helps automate memory management in NetEq and will be useful in the
work to minimize the AudioDecoder interface as part of the injectable
audio codec work.

I'm planning a follow-up that will change the current management of Packet* to
wrapping them in unique_ptr instead.
==========

[ossu, 2016-08-30 15:19:05]

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

[ossu, 2016-08-30 15:22:16]

Last week we discussed putting a Parse method into AudioDecoder, to help move
the current packet splitting code from NetEq internals. Part of the design of
that Parse method was to have it accept rtc::Buffers of data, so that we can
avoid copying by allowing the decoder to steal the data. This is a step towards
that.

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

https://codereview.webrtc.org/2289093003/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/payload_splitter.cc:97: // The block lengths
in the RED headers do not match the overall packet
Primarily a question to hlundin: I've kept this behavior for now, but how useful
is it really? Is it probable that we get a partial correct packet instead of a
mangled full packet? Won't we risk picking the wrong bits and pump garbage into
our decoders if we keep only parts of a RED packet? Either way, the primary
payload is the one most likely to be discarded.

[kwiberg-webrtc, 2016-08-30 16:04:36]

Extremely nice! Just a few nits though.

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

https://codereview.webrtc.org/2289093003/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:662: delete []
current_packet->payload;
Whatever happens, this CL is going to earn you a heap of karma points. :-)

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

https://codereview.webrtc.org/2289093003/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:564: packet->payload =
rtc::Buffer(payload.data(), payload.size());
You're copying data into an existing buffer, so

  packet->payload.SetData(payload);

I suspect Buffer::SetData may not actually accept an ArrayView, but it should...

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

https://codereview.webrtc.org/2289093003/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/packet_buffer_unittest.cc:63: packet->payload
= rtc::Buffer(payload_size_bytes);
packet->payload.SetSize(payload_size_bytes);

Also, I wonder what the test is doing with all those uninitialized packets...

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

https://codereview.webrtc.org/2289093003/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/payload_splitter.cc:109: new_packet->payload =
rtc::Buffer(payload_ptr, payload_length);
SetData

https://codereview.webrtc.org/2289093003/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/payload_splitter.cc:172:
rtc::Buffer(packet->payload.data(), packet->payload.size());
  new_packet->payload.SetData(packet->payload);

using the ArrayView version of SetData.

https://codereview.webrtc.org/2289093003/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/payload_splitter.cc:430: new_packet->payload =
rtc::Buffer(payload_ptr, bytes_per_frame);
SetData

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

https://codereview.webrtc.org/2289093003/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/payload_splitter_unittest.cc:87: (num_payloads
- 1) * (kPayloadLength + kRedHeaderLength));
SetSize

https://codereview.webrtc.org/2289093003/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/payload_splitter_unittest.cc:130:
packet->payload = rtc::Buffer(payload_length);
SetSize

[ossu, 2016-08-30 16:22:29]

Patchset #2 (id:20001) has been deleted

[ossu, 2016-08-30 16:26:38]

Patchset #2 (id:40001) has been deleted

[ossu, 2016-08-30 16:27:14]

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

https://codereview.webrtc.org/2289093003/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:662: delete []
current_packet->payload;
On 2016/08/30 16:04:36, kwiberg-webrtc wrote:
> Whatever happens, this CL is going to earn you a heap of karma points. :-)

Yay! \o/

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

https://codereview.webrtc.org/2289093003/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:564: packet->payload =
rtc::Buffer(payload.data(), payload.size());
On 2016/08/30 16:04:36, kwiberg-webrtc wrote:
> You're copying data into an existing buffer, so
> 
>   packet->payload.SetData(payload);
> 
> I suspect Buffer::SetData may not actually accept an ArrayView, but it
should...

Good point. Yeah, I was expecting the copy constructor to accept anything with a
data() and a size() but it might be too easy to make unintended copies of data
with that. The SetData call should be an unambiguous copy, so for that I think
it'd be fine!

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

https://codereview.webrtc.org/2289093003/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/packet_buffer_unittest.cc:63: packet->payload
= rtc::Buffer(payload_size_bytes);
On 2016/08/30 16:04:36, kwiberg-webrtc wrote:
> packet->payload.SetSize(payload_size_bytes);
> 
> Also, I wonder what the test is doing with all those uninitialized packets...

Acknowledged.

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

https://codereview.webrtc.org/2289093003/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/payload_splitter.cc:109: new_packet->payload =
rtc::Buffer(payload_ptr, payload_length);
On 2016/08/30 16:04:36, kwiberg-webrtc wrote:
> SetData

Acknowledged.

https://codereview.webrtc.org/2289093003/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/payload_splitter.cc:172:
rtc::Buffer(packet->payload.data(), packet->payload.size());
On 2016/08/30 16:04:36, kwiberg-webrtc wrote:
>   new_packet->payload.SetData(packet->payload);
> 
> using the ArrayView version of SetData.

Acknowledged.

https://codereview.webrtc.org/2289093003/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/payload_splitter.cc:430: new_packet->payload =
rtc::Buffer(payload_ptr, bytes_per_frame);
On 2016/08/30 16:04:36, kwiberg-webrtc wrote:
> SetData

Acknowledged.

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

https://codereview.webrtc.org/2289093003/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/payload_splitter_unittest.cc:87: (num_payloads
- 1) * (kPayloadLength + kRedHeaderLength));
On 2016/08/30 16:04:36, kwiberg-webrtc wrote:
> SetSize

Acknowledged.

https://codereview.webrtc.org/2289093003/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/payload_splitter_unittest.cc:130:
packet->payload = rtc::Buffer(payload_length);
On 2016/08/30 16:04:36, kwiberg-webrtc wrote:
> SetSize

Acknowledged.

[ossu, 2016-08-30 16:34:11]

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

[commit-bot: I haz the power, 2016-08-30 16:34:18]

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

[kwiberg-webrtc, 2016-08-30 16:37:05]

lgtm

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

https://codereview.webrtc.org/2289093003/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:564: packet->payload =
rtc::Buffer(payload.data(), payload.size());
On 2016/08/30 16:27:13, ossu wrote:
> On 2016/08/30 16:04:36, kwiberg-webrtc wrote:
> > You're copying data into an existing buffer, so
> > 
> >   packet->payload.SetData(payload);
> > 
> > I suspect Buffer::SetData may not actually accept an ArrayView, but it
> should...
> 
> Good point. Yeah, I was expecting the copy constructor to accept anything with
a
> data() and a size() but it might be too easy to make unintended copies of data
> with that. The SetData call should be an unambiguous copy, so for that I think
> it'd be fine!

Yes, we explicitly don't want a copy constructor. It's too easy to make copies
without noticing it that way...

[commit-bot: I haz the power, 2016-08-30 18:34:48]

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

[commit-bot: I haz the power, 2016-08-30 18:34:49]

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

[ossu, 2016-08-31 15:40:55]

The CQ bit was checked by ossu@webrtc.org

[commit-bot: I haz the power, 2016-08-31 15:41:02]

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-08-31 15:51:15]

Description was changed from

==========
NetEq: Changed Packet::payload to be an rtc::Buffer

That is, rather than keeping a separate pointer and size.
This helps automate memory management in NetEq and will be useful in the
work to minimize the AudioDecoder interface as part of the injectable
audio codec work.

I'm planning a follow-up that will change the current management of Packet* to
wrapping them in unique_ptr instead.
==========

to

==========
NetEq: Changed Packet::payload to be an rtc::Buffer

That is, rather than keeping a separate pointer and size.
This helps automate memory management in NetEq and will be useful in the
work to minimize the AudioDecoder interface as part of the injectable
audio codec work.

I'm planning a follow-up that will change the current management of Packet* to
wrapping them in unique_ptr instead.
==========

[commit-bot: I haz the power, 2016-08-31 15:51:16]

Committed patchset #2 (id:60001)

[commit-bot: I haz the power, 2016-08-31 15:51:22]

Description was changed from

==========
NetEq: Changed Packet::payload to be an rtc::Buffer

That is, rather than keeping a separate pointer and size.
This helps automate memory management in NetEq and will be useful in the
work to minimize the AudioDecoder interface as part of the injectable
audio codec work.

I'm planning a follow-up that will change the current management of Packet* to
wrapping them in unique_ptr instead.
==========

to

==========
NetEq: Changed Packet::payload to be an rtc::Buffer

That is, rather than keeping a separate pointer and size.
This helps automate memory management in NetEq and will be useful in the
work to minimize the AudioDecoder interface as part of the injectable
audio codec work.

I'm planning a follow-up that will change the current management of Packet* to
wrapping them in unique_ptr instead.

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

[commit-bot: I haz the power, 2016-08-31 15:51:23]

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

[hlundin-webrtc, 2016-09-01 14:08:04]

LGTM

https://codereview.webrtc.org/2289093003/diff/60001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/payload_splitter_unittest.cc (right):

https://codereview.webrtc.org/2289093003/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/payload_splitter_unittest.cc:135:
memset(packet->payload.data(), payload_value, packet->payload.size());
I was thinking you could use std::fill, to avoid the pitfall with the element
sizes. But that makes the line just as long as the current one. I for one
wouldn't mind an rtc::BufferT::fill(T value) method... :)