media: VideoFrame: add offset for shared memory buffers

media: VideoFrame: add offset for shared memory buffers

Frames' data could be located at an offset within a shared memory
buffer. This adds metadata into media::VideoFrame to support this use
case.

BUG=455409
TEST=none

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

[llandwerlin-old, 2015-01-28 13:07:28]

lionel.g.landwerlin@intel.com changed reviewers:
+ bbudge@chromium.org, dalecurtis@chromium.org, piman@chromium.org

[llandwerlin-old, 2015-01-28 13:07:29]

piman@ dalecurtis@: Please review these changes.

To give a bit more background, we would like to reuse
ppapi/shared_impl/media_stream_buffer_manager.h for the  coming PPB_VideoEncoder
api.

The main change is that video frames are all located with one shared memory
buffer.
This CL is about adding metadata to media::VideoFrame to carry this offset.

I have another CL coming to update the gpu message to the VideoEncodeAccelerator
adding the offset as one of the parameter.

Thanks!

[piman, 2015-01-28 18:37:08]

lgtm

[bbudge, 2015-01-28 18:47:07]

Some comments, otherwise LGTM

https://codereview.chromium.org/877353002/diff/1/content/browser/media/captur...
File content/browser/media/capture/content_video_capture_device_core.cc (right):

https://codereview.chromium.org/877353002/diff/1/content/browser/media/captur...
content/browser/media/capture/content_video_capture_device_core.cc:137: 0,
Could you label these to make comprehension easier? Instead of 0, the line would
be:

    0 /* shared_memory_offset*/,

https://codereview.chromium.org/877353002/diff/1/media/base/video_frame.h
File media/base/video_frame.h (right):

https://codereview.chromium.org/877353002/diff/1/media/base/video_frame.h#new...
media/base/video_frame.h:141: size_t data_offset,
Naming this 'data_offset' makes it look odd that it isn't grouped with data and
data_size.
How about naming this to match shared_memory_offset below?

https://codereview.chromium.org/877353002/diff/1/media/base/video_frame.h#new...
media/base/video_frame.h:303: // Return the offset with the shared-memory
handle.
Nit: wording. How about..
// Return the offset into the shared memory where the frame data begins.

[DaleCurtis, 2015-01-28 23:31:48]

I don't follow why this should be part of the VideoFrame. Isn't this handled by
the data pointer provided?

[bbudge, 2015-01-28 23:59:26]

On 2015/01/28 23:31:48, DaleCurtis wrote:
> I don't follow why this should be part of the VideoFrame. Isn't this handled
by
> the data pointer provided?

That's a good point. It seems from this patch that you don't use
shared_memory_offset_ for anything in this class, so you're just associating an
offset with the frame. Couldn't you do the same with a std::map and leave this
class untouched?

[llandwerlin-old, 2015-01-29 11:08:32]

On 2015/01/28 23:59:26, bbudge wrote:
> On 2015/01/28 23:31:48, DaleCurtis wrote:
> > I don't follow why this should be part of the VideoFrame. Isn't this handled
> by
> > the data pointer provided?
> 
> That's a good point. It seems from this patch that you don't use
> shared_memory_offset_ for anything in this class, so you're just associating
an
> offset with the frame. Couldn't you do the same with a std::map and leave this
> class untouched?

We can't really use the data pointer to determine the offset in the shared
memory buffer.
The reason for that is that we need to map the buffer before we can compute that
offset.

media::VideoFrame is only given a base::SharedMemoryHandle (which is a file
descriptor on unix system).
If we create a base::SharedMemory from the given handle and map it, the
operating system will give use a different address in virtual memory and we
won't be able to figure out the offset from that address.

The shared_memory_offset is just metadata, much like end_of_stream_,
mailbox_holder_ or timestamp_.

I mentioned earlier that I have another CL to make use of the
shared_memory_offset_ in the communication with the gpu process. Would merging
those 2 CLs make sense, so you can see how it's going to be used?

Thanks.

[DaleCurtis, 2015-01-29 18:54:57]

Yes, I'll need to see the other CL.

[DaleCurtis, 2015-01-29 18:55:51]

(Note, if it's a large CL, you can just upload it, no need to merge it with this
one).

[llandwerlin-old, 2015-01-30 08:03:41]

dalecurtis@, I added the modification to the ipc message with the gpu process,
making use of this shared_memory_offset().
It's fairly small.

https://codereview.chromium.org/877353002/diff/1/media/base/video_frame.h
File media/base/video_frame.h (right):

https://codereview.chromium.org/877353002/diff/1/media/base/video_frame.h#new...
media/base/video_frame.h:141: size_t data_offset,
On 2015/01/28 18:47:07, bbudge wrote:
> Naming this 'data_offset' makes it look odd that it isn't grouped with data
and
> data_size.
> How about naming this to match shared_memory_offset below?

Done.

https://codereview.chromium.org/877353002/diff/1/media/base/video_frame.h#new...
media/base/video_frame.h:303: // Return the offset with the shared-memory
handle.
On 2015/01/28 18:47:07, bbudge wrote:
> Nit: wording. How about..
> // Return the offset into the shared memory where the frame data begins.

Done.

[llandwerlin-old, 2015-01-30 08:04:28]

lionel.g.landwerlin@intel.com changed reviewers:
+ dcheng@chromium.org

[llandwerlin-old, 2015-01-30 08:04:29]

Adding dcheng@ to review the gpu ipc message change.
Thanks!

[DaleCurtis, 2015-01-30 18:44:59]

https://codereview.chromium.org/877353002/diff/20001/content/common/gpu/clien...
File content/common/gpu/client/gpu_video_encode_accelerator_host.cc (right):

https://codereview.chromium.org/877353002/diff/20001/content/common/gpu/clien...
content/common/gpu/client/gpu_video_encode_accelerator_host.cc:187:
encoder_route_id_, next_frame_id_, handle, frame->shared_memory_offset(),
Who is setting this value? It looks like it's always zero.

https://codereview.chromium.org/877353002/diff/20001/content/common/gpu/gpu_m...
File content/common/gpu/gpu_messages.h (right):

https://codereview.chromium.org/877353002/diff/20001/content/common/gpu/gpu_m...
content/common/gpu/gpu_messages.h:715: uint32 /* buffer_offset */,
This seems dangerous...

https://codereview.chromium.org/877353002/diff/20001/content/common/gpu/media...
File content/common/gpu/media/gpu_video_encode_accelerator.cc (right):

https://codereview.chromium.org/877353002/diff/20001/content/common/gpu/media...
content/common/gpu/media/gpu_video_encode_accelerator.cc:288: buffer_offset,
This is unused from now no, no?

[llandwerlin-old, 2015-01-31 22:42:53]

https://codereview.chromium.org/877353002/diff/20001/content/common/gpu/clien...
File content/common/gpu/client/gpu_video_encode_accelerator_host.cc (right):

https://codereview.chromium.org/877353002/diff/20001/content/common/gpu/clien...
content/common/gpu/client/gpu_video_encode_accelerator_host.cc:187:
encoder_route_id_, next_frame_id_, handle, frame->shared_memory_offset(),
On 2015/01/30 18:44:59, DaleCurtis wrote:
> Who is setting this value? It looks like it's always zero.

For now, it's always zero.
The idea is to get this new attribute in for PPB_VideoEncoder : 
https://codereview.chromium.org/859313002/diff/1/content/renderer/pepper/pepp...

https://codereview.chromium.org/877353002/diff/20001/content/common/gpu/gpu_m...
File content/common/gpu/gpu_messages.h (right):

https://codereview.chromium.org/877353002/diff/20001/content/common/gpu/gpu_m...
content/common/gpu/gpu_messages.h:715: uint32 /* buffer_offset */,
On 2015/01/30 18:44:59, DaleCurtis wrote:
> This seems dangerous...

Can you explain why you think this could be dangerous?
I believe that wrong memory mapping parameters would just lead the mmap
operation to fail (for example with a buffer offset outside the buffer's limit).

https://codereview.chromium.org/877353002/diff/20001/content/common/gpu/media...
File content/common/gpu/media/gpu_video_encode_accelerator.cc (right):

https://codereview.chromium.org/877353002/diff/20001/content/common/gpu/media...
content/common/gpu/media/gpu_video_encode_accelerator.cc:288: buffer_offset,
On 2015/01/30 18:44:59, DaleCurtis wrote:
> This is unused from now no, no? 

Yes, once in the gpu process this is unused.
I just put it for consistency.

[DaleCurtis, 2015-02-02 19:24:18]

I defer to dcheng@ on whether this is really problem, but it does seem risky to
me.

Bundling the offset with the VideoFrame also seems wonky since it seems only one
place cares about it. Everywhere else just cares about the data pointers. How
hard is it to ship this value external to the frame?

That said, we already bundle the shared memory handle which is also seemingly
unrelated, so maybe this is a battle that was lost long ago.

https://codereview.chromium.org/877353002/diff/20001/content/common/gpu/gpu_m...
File content/common/gpu/gpu_messages.h (right):

https://codereview.chromium.org/877353002/diff/20001/content/common/gpu/gpu_m...
content/common/gpu/gpu_messages.h:715: uint32 /* buffer_offset */,
On 2015/01/31 22:42:53, llandwerlin wrote:
> On 2015/01/30 18:44:59, DaleCurtis wrote:
> > This seems dangerous...
> 
> Can you explain why you think this could be dangerous?
> I believe that wrong memory mapping parameters would just lead the mmap
> operation to fail (for example with a buffer offset outside the buffer's
limit).

An attacker controls both the buffer_offset and buffer_size in this case, which
without careful consideration on the endpoint may lead to issues.

[dcheng, 2015-02-02 20:56:23]

https://codereview.chromium.org/877353002/diff/20001/content/common/gpu/media...
File content/common/gpu/media/gpu_video_encode_accelerator.cc (right):

https://codereview.chromium.org/877353002/diff/20001/content/common/gpu/media...
content/common/gpu/media/gpu_video_encode_accelerator.cc:266: size_t map_size = 
buffer_size + (buffer_offset - map_offset);
This calculation can trivially overflow. buffer_size = 4294963201, buffer_offset
= 4095, assuming 4096 byte pages.

Chrome's IPC tips recommend base/numerics
(http://www.chromium.org/Home/chromium-security/education/security-tips-for-ip...)
to prevent this error from going unnoticed.

https://codereview.chromium.org/877353002/diff/20001/media/base/video_frame.cc
File media/base/video_frame.cc (right):

https://codereview.chromium.org/877353002/diff/20001/media/base/video_frame.c...
media/base/video_frame.cc:794: size_t VideoFrame::shared_memory_offset() const {
Simple accessors like this should be inlined.

[llandwerlin-old, 2015-02-03 16:35:06]

Sorry for the delay, PTAL.

https://codereview.chromium.org/877353002/diff/20001/content/common/gpu/media...
File content/common/gpu/media/gpu_video_encode_accelerator.cc (right):

https://codereview.chromium.org/877353002/diff/20001/content/common/gpu/media...
content/common/gpu/media/gpu_video_encode_accelerator.cc:266: size_t map_size = 
buffer_size + (buffer_offset - map_offset);
On 2015/02/02 20:56:23, dcheng wrote:
> This calculation can trivially overflow. buffer_size = 4294963201,
buffer_offset
> = 4095, assuming 4096 byte pages.
> 
> Chrome's IPC tips recommend base/numerics
>
(http://www.chromium.org/Home/chromium-security/education/security-tips-for-ip...)
> to prevent this error from going unnoticed.

Done.

https://codereview.chromium.org/877353002/diff/20001/media/base/video_frame.cc
File media/base/video_frame.cc (right):

https://codereview.chromium.org/877353002/diff/20001/media/base/video_frame.c...
media/base/video_frame.cc:794: size_t VideoFrame::shared_memory_offset() const {
On 2015/02/02 20:56:23, dcheng wrote:
> Simple accessors like this should be inlined.

Done.

[dcheng, 2015-02-06 21:39:47]

https://codereview.chromium.org/877353002/diff/40001/content/common/gpu/media...
File content/common/gpu/media/gpu_video_encode_accelerator.cc (right):

https://codereview.chromium.org/877353002/diff/40001/content/common/gpu/media...
content/common/gpu/media/gpu_video_encode_accelerator.cc:275:
static_cast<off_t>(map_offset.ValueOrDie()));
Let's avoid casting if we don't have to. It makes it harder to be sure if
something is going to overflow at some point.

How about this?

uint32 aligned_offset = buffer_offset %
base::SysInfo::VMAllocationGranularity();
base::CheckedNumeric<off_t> map_offset = buffer_offset;
map_offset -= aligned_offset;
base::CheckedNumeric<size_t> map_size = buffer_size;
map_size += aligned_offset;

if (!map_offset.IsValid() || !map_size.IsValid()) {
  // error handling here...
}

[llandwerlin-old, 2015-02-09 14:48:47]

PTAL, thanks for the snippet of code.

https://codereview.chromium.org/877353002/diff/40001/content/common/gpu/media...
File content/common/gpu/media/gpu_video_encode_accelerator.cc (right):

https://codereview.chromium.org/877353002/diff/40001/content/common/gpu/media...
content/common/gpu/media/gpu_video_encode_accelerator.cc:275:
static_cast<off_t>(map_offset.ValueOrDie()));
On 2015/02/06 21:39:47, dcheng wrote:
> Let's avoid casting if we don't have to. It makes it harder to be sure if
> something is going to overflow at some point.
> 
> How about this?
> 
> uint32 aligned_offset = buffer_offset %
> base::SysInfo::VMAllocationGranularity();
> base::CheckedNumeric<off_t> map_offset = buffer_offset;
> map_offset -= aligned_offset;
> base::CheckedNumeric<size_t> map_size = buffer_size;
> map_size += aligned_offset;
> 
> if (!map_offset.IsValid() || !map_size.IsValid()) {
>   // error handling here...
> }

Done.

[dcheng, 2015-02-09 18:57:09]

https://codereview.chromium.org/877353002/diff/60001/content/common/gpu/media...
File content/common/gpu/media/gpu_video_encode_accelerator.cc (right):

https://codereview.chromium.org/877353002/diff/60001/content/common/gpu/media...
content/common/gpu/media/gpu_video_encode_accelerator.cc:287: (buffer_offset -
map_offset.ValueOrDie());
Use aligned_offset here too.

[llandwerlin-old, 2015-02-09 19:34:34]

Thanks, PTAL.

https://codereview.chromium.org/877353002/diff/60001/content/common/gpu/media...
File content/common/gpu/media/gpu_video_encode_accelerator.cc (right):

https://codereview.chromium.org/877353002/diff/60001/content/common/gpu/media...
content/common/gpu/media/gpu_video_encode_accelerator.cc:287: (buffer_offset -
map_offset.ValueOrDie());
On 2015/02/09 18:57:09, dcheng wrote:
> Use aligned_offset here too.

Done.

[dcheng, 2015-02-09 20:23:19]

IPC changes lgtm

[DaleCurtis, 2015-02-09 20:30:36]

lgtm

[llandwerlin-old, 2015-02-09 21:07:17]

The CQ bit was checked by lionel.g.landwerlin@intel.com

[commit-bot: I haz the power, 2015-02-09 21:07:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/877353002/80001

[commit-bot: I haz the power, 2015-02-09 23:27:41]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-02-09 23:28:39]

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