Copy payload data when inserting packets into video_coding::PacketBuffer.

Copy payload data when inserting packets into video_coding::PacketBuffer.

The payload pointed to by |dataPtr| is volatile and needs to be copied
to its own buffer.

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

Committed: https://crrev.com/1f39ba1cd916d97e6f203ba9da717ae85aeb4577
Cr-Commit-Position: refs/heads/master@{#14321}

[philipel, 2016-09-01 08:48:11]

Description was changed from

==========
Copy payload data when inserting packets into video_coding::PacketBuffer.

The payload pointed to by |dataPtr| is volatile and needs to be copied
to its own buffer.

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

to

==========
Copy payload data when inserting packets into video_coding::PacketBuffer.

The payload pointed to by |dataPtr| is volatile and needs to be copied
to its own buffer.

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

[philipel, 2016-09-01 08:48:11]

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

[brandtr, 2016-09-01 12:45:14]

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

https://codereview.webrtc.org/2302763002/diff/1/webrtc/modules/video_coding/p...
webrtc/modules/video_coding/packet_buffer.cc:97: memcpy(payload, packet.dataPtr,
packet.sizeBytes);
In this block, the copied VCMPacket essentially takes ownership of the data by
copying it. Would it make sense to make this functionality a member function of
VCMPacket, or to add an explicit copy constructor?

If the data pointed to by dataPtr is volatile, should it be locked during
copying?

[philipel, 2016-09-01 12:49:07]

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

https://codereview.webrtc.org/2302763002/diff/1/webrtc/modules/video_coding/p...
webrtc/modules/video_coding/packet_buffer.cc:97: memcpy(payload, packet.dataPtr,
packet.sizeBytes);
On 2016/09/01 12:45:13, brandtr wrote:
> In this block, the copied VCMPacket essentially takes ownership of the data by
> copying it. Would it make sense to make this functionality a member function
of
> VCMPacket, or to add an explicit copy constructor?
> 
> If the data pointed to by dataPtr is volatile, should it be locked during
> copying?

We don't need to lock the data at this points.

I guess volatile is the wrong word, but If I don't copy the payload here the
data is lost. Maybe non-persistent is a better word to use, WDYT?

[brandtr, 2016-09-01 13:21:53]

lgtm

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

https://codereview.webrtc.org/2302763002/diff/1/webrtc/modules/video_coding/p...
webrtc/modules/video_coding/packet_buffer.cc:97: memcpy(payload, packet.dataPtr,
packet.sizeBytes);
On 2016/09/01 12:49:07, philipel wrote:
> On 2016/09/01 12:45:13, brandtr wrote:
> > In this block, the copied VCMPacket essentially takes ownership of the data
by
> > copying it. Would it make sense to make this functionality a member function
> of
> > VCMPacket, or to add an explicit copy constructor?
> > 
> > If the data pointed to by dataPtr is volatile, should it be locked during
> > copying?
> 
> We don't need to lock the data at this points.
> 
> I guess volatile is the wrong word, but If I don't copy the payload here the
> data is lost. Maybe non-persistent is a better word to use, WDYT?

"Non-persistent" sounds good.

[philipel, 2016-09-01 13:28:00]

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

https://codereview.webrtc.org/2302763002/diff/1/webrtc/modules/video_coding/p...
webrtc/modules/video_coding/packet_buffer.cc:97: memcpy(payload, packet.dataPtr,
packet.sizeBytes);
On 2016/09/01 13:21:53, brandtr wrote:
> On 2016/09/01 12:49:07, philipel wrote:
> > On 2016/09/01 12:45:13, brandtr wrote:
> > > In this block, the copied VCMPacket essentially takes ownership of the
data
> by
> > > copying it. Would it make sense to make this functionality a member
function
> > of
> > > VCMPacket, or to add an explicit copy constructor?
> > > 
> > > If the data pointed to by dataPtr is volatile, should it be locked during
> > > copying?
> > 
> > We don't need to lock the data at this points.
> > 
> > I guess volatile is the wrong word, but If I don't copy the payload here the
> > data is lost. Maybe non-persistent is a better word to use, WDYT?
> 
> "Non-persistent" sounds good.

Done.

[philipel, 2016-09-01 13:49:11]

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

[commit-bot: I haz the power, 2016-09-01 13:49:22]

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-09-01 14:37:03]

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

[commit-bot: I haz the power, 2016-09-01 14:37:03]

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

[philipel, 2016-09-01 14:50:49]

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

[commit-bot: I haz the power, 2016-09-01 14:51:00]

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-09-01 16:51:21]

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

[commit-bot: I haz the power, 2016-09-01 16:51:23]

Dry run: Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_clang_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_clang_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_drmemory_light on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_x64_clang_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_x64_clang_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_x64_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_x64_gyp_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_x64_gyp_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_x64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[philipel, 2016-09-07 08:39:29]

ping

[stefan-webrtc, 2016-09-13 08:16:10]

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

https://codereview.webrtc.org/2302763002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/packet_buffer.cc:94: // data has to be copied to its
own buffer.
Maybe add a TODO mentioning that we'd like to pass the ownership of this data
over from the socket to the packet buffer at some point. That'd avoid the
memcpy, which should be doable since AsyncUDPSocket and friends own the packet
memory at the moment.

https://codereview.webrtc.org/2302763002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/packet_buffer.cc:96: uint8_t* payload = new
uint8_t[packet.sizeBytes];
Not super happy about this as we have to ensure we delete the memory at all
possible places (currently only 2 it seems, but still). Can we do something to
make this better by e.g. using unique_ptr?

[philipel, 2016-09-13 11:12:07]

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

https://codereview.webrtc.org/2302763002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/packet_buffer.cc:94: // data has to be copied to its
own buffer.
On 2016/09/13 08:16:10, stefan-webrtc (holmer) wrote:
> Maybe add a TODO mentioning that we'd like to pass the ownership of this data
> over from the socket to the packet buffer at some point. That'd avoid the
> memcpy, which should be doable since AsyncUDPSocket and friends own the packet
> memory at the moment.

Done.

https://codereview.webrtc.org/2302763002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/packet_buffer.cc:96: uint8_t* payload = new
uint8_t[packet.sizeBytes];
On 2016/09/13 08:16:10, stefan-webrtc (holmer) wrote:
> Not super happy about this as we have to ensure we delete the memory at all
> possible places (currently only 2 it seems, but still). Can we do something to
> make this better by e.g. using unique_ptr?

Since the packet buffer have a fixed vector of packets the lifetime management
is manual. I guess using a unique_ptr could help but I'm not sure how much work
that requires.

Also, the lifetime management is fairly contained so it's not particularly
complex, so I'm pretty certain there is no leak (famous last words).

[philipel, 2016-09-20 09:31:21]

ping

[stefan-webrtc, 2016-09-20 09:45:49]

lgtm

[philipel, 2016-09-20 10:32:18]

The CQ bit was checked by philipel@webrtc.org

[philipel, 2016-09-20 10:32:19]

The patchset sent to the CQ was uploaded after l-g-t-m from brandtr@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2302763002/#ps60001 (title:
"Added TODO")

[commit-bot: I haz the power, 2016-09-20 10:32: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-09-20 12:32:54]

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

[commit-bot: I haz the power, 2016-09-20 12:32:56]

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

[philipel, 2016-09-21 09:27:59]

Description was changed from

==========
Copy payload data when inserting packets into video_coding::PacketBuffer.

The payload pointed to by |dataPtr| is volatile and needs to be copied
to its own buffer.

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

to

==========
Copy payload data when inserting packets into video_coding::PacketBuffer.

The payload pointed to by |dataPtr| is volatile and needs to be copied
to its own buffer.

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

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

[philipel, 2016-09-21 09:28:01]

Committed patchset #4 (id:60001) manually as
1f39ba1cd916d97e6f203ba9da717ae85aeb4577 (presubmit successful).

[commit-bot: I haz the power, 2016-09-21 09:28:01]

Description was changed from

==========
Copy payload data when inserting packets into video_coding::PacketBuffer.

The payload pointed to by |dataPtr| is volatile and needs to be copied
to its own buffer.

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

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

to

==========
Copy payload data when inserting packets into video_coding::PacketBuffer.

The payload pointed to by |dataPtr| is volatile and needs to be copied
to its own buffer.

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

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