Add offset support to BitstreamBuffer.

Add offset support to BitstreamBuffer.

The actual data in the shared memory may start at some offset from the start of shared memory.

TEST=Run vda_unittest and play YT on peach-pit.
BUG=b/25829285

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

[Owen Lin, 2015-12-23 08:37:49]

owenlin@chromium.org changed reviewers:
+ kcwu@chromium.org, posciak@chromium.org

[Owen Lin, 2015-12-23 08:37:50]

PTAL

[kcwu, 2015-12-23 08:59:30]

lgtm

[Pawel Osciak, 2015-12-24 00:58:01]

https://codereview.chromium.org/1541353002/diff/1/content/common/gpu/client/g...
File content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc (right):

https://codereview.chromium.org/1541353002/diff/1/content/common/gpu/client/g...
content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:179:
decode_params.input_buffer_offset = bitstream_buffer.offset();
We must do a base::checked_cast here and in other places like this, since
input_buffer_offset is uint32, and bitstream_buffer.offset() is off_t.

https://codereview.chromium.org/1541353002/diff/1/content/common/gpu/media/an...
File content/common/gpu/media/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1541353002/diff/1/content/common/gpu/media/an...
content/common/gpu/media/android_video_decode_accelerator.cc:210: this,
shm->MapAt(bitstream_buffer.offset(), bitstream_buffer.size()),
MapAt (and underlying mmap()) requires offset to be aligned to PAGE_SIZE (or
base::SysInfo::VMAllocationGranularity()). Are we able to guarantee that?
But perhaps it would be simpler to map as usual, without offset, and just use
pointer+offset below?

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

https://codereview.chromium.org/1541353002/diff/1/media/base/bitstream_buffer...
media/base/bitstream_buffer.h:47: 
Please add a comment what offset is.

[Pawel Osciak, 2015-12-24 01:01:38]

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

https://codereview.chromium.org/1541353002/diff/1/media/base/bitstream_buffer...
media/base/bitstream_buffer.h:47: 
On 2015/12/24 00:58:01, Pawel Osciak wrote:
> Please add a comment what offset is.

Also perhaps please also clarify that size() is for the whole buffer, not from
offset.

[Owen Lin, 2015-12-24 08:01:35]

https://codereview.chromium.org/1541353002/diff/1/content/common/gpu/client/g...
File content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc (right):

https://codereview.chromium.org/1541353002/diff/1/content/common/gpu/client/g...
content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:179:
decode_params.input_buffer_offset = bitstream_buffer.offset();
On 2015/12/24 00:58:01, Pawel Osciak wrote:
> We must do a base::checked_cast here and in other places like this, since
> input_buffer_offset is uint32, and bitstream_buffer.offset() is off_t.

Done.

https://codereview.chromium.org/1541353002/diff/1/content/common/gpu/media/an...
File content/common/gpu/media/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1541353002/diff/1/content/common/gpu/media/an...
content/common/gpu/media/android_video_decode_accelerator.cc:210: this,
shm->MapAt(bitstream_buffer.offset(), bitstream_buffer.size()),
On 2015/12/24 00:58:01, Pawel Osciak wrote:
> MapAt (and underlying mmap()) requires offset to be aligned to PAGE_SIZE (or
> base::SysInfo::VMAllocationGranularity()). Are we able to guarantee that?
> But perhaps it would be simpler to map as usual, without offset, and just use
> pointer+offset below?

I think we can do it in ArcCodec. Align the input buffer size to PAGE_SIZE.

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

https://codereview.chromium.org/1541353002/diff/1/media/base/bitstream_buffer...
media/base/bitstream_buffer.h:47: 
On 2015/12/24 01:01:38, Pawel Osciak wrote:
> On 2015/12/24 00:58:01, Pawel Osciak wrote:
> > Please add a comment what offset is.
> 
> Also perhaps please also clarify that size() is for the whole buffer, not from
> offset.

Done.

https://codereview.chromium.org/1541353002/diff/1/media/base/bitstream_buffer...
media/base/bitstream_buffer.h:47: 
On 2015/12/24 01:01:38, Pawel Osciak wrote:
> On 2015/12/24 00:58:01, Pawel Osciak wrote:
> > Please add a comment what offset is.
> 
> Also perhaps please also clarify that size() is for the whole buffer, not from
> offset.

Done.

[Owen Lin, 2015-12-28 08:27:19]

Patchset #3 (id:40001) has been deleted

[Owen Lin, 2015-12-28 08:28:03]

Please take another look. Thanks.

[Owen Lin, 2015-12-29 07:08:22]

Description was changed from

==========
Add offset to BitstreamBuffer.

The shared buffers passed from ARC will have none-zero offset.
Add it to BitstreamBuffer to handle the case.

TEST=Run vda_unittest and play YT on peach-pit.
BUG=b/25829285
==========

to

==========
Add offset support to BitstreamBuffer.

The actual data in the shared memory may start at some offset from the start of
shared memory.

TEST=Run vda_unittest and play YT on peach-pit.
BUG=b/25829285
==========

[Pawel Osciak, 2015-12-31 02:05:54]

Please also at least test compilation for Intel and veyron, as the bots don't do
this.

https://chromiumcodereview.appspot.com/1541353002/diff/60001/content/common/g...
File content/common/gpu/client/gpu_video_decode_accelerator_host.cc (right):

https://chromiumcodereview.appspot.com/1541353002/diff/60001/content/common/g...
content/common/gpu/client/gpu_video_decode_accelerator_host.cc:132: params.size
= base::checked_cast<uint32>(bitstream_buffer.size());
We've just switched to standard integer types. Please update this CL to use
uint32_t, int32_t, etc. after rebase.

https://chromiumcodereview.appspot.com/1541353002/diff/60001/content/common/g...
File content/common/gpu/media/shared_memory_region.cc (right):

https://chromiumcodereview.appspot.com/1541353002/diff/60001/content/common/g...
content/common/gpu/media/shared_memory_region.cc:25: off_t mask =
base::SysInfo::VMAllocationGranularity() - 1;
This assumes granularity is a power of 2 (which normally it always should be,
but technically we don't guarantee that from the documentation of the method).
Perhaps something like this instead:

https://code.google.com/p/chromium/codesearch#chromium/src/mojo/edk/embedder/...

https://chromiumcodereview.appspot.com/1541353002/diff/60001/content/common/g...
content/common/gpu/media/shared_memory_region.cc:26: return shm_.MapAt((offset_
& ~mask), size_ + (offset_ & mask));
Perhaps we could calculate new offset here (or even in constructor to drop
offset_) and keep only the calculated offset, for use in memory()?

https://chromiumcodereview.appspot.com/1541353002/diff/60001/content/common/g...
File content/common/gpu/media/shared_memory_region.h (right):

https://chromiumcodereview.appspot.com/1541353002/diff/60001/content/common/g...
content/common/gpu/media/shared_memory_region.h:16: SharedMemoryRegion(const
base::SharedMemoryHandle& handle,
We should document arguments, especially offset and size are not obvious, also
the fact that this is taking ownership of shared memory.

https://chromiumcodereview.appspot.com/1541353002/diff/60001/content/common/g...
content/common/gpu/media/shared_memory_region.h:30: void* memory();
We should document how this relates to offset and size passed to constructor.

https://chromiumcodereview.appspot.com/1541353002/diff/60001/content/common/g...
content/common/gpu/media/shared_memory_region.h:34: off_t offset() const {
return offset_; }
Is this used anywhere?

https://chromiumcodereview.appspot.com/1541353002/diff/60001/content/common/g...
File content/common/gpu/media/v4l2_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/1541353002/diff/60001/content/common/g...
content/common/gpu/media/v4l2_video_decode_accelerator.cc:543: if
(decoder_current_bitstream_buffer_->shm == NULL) {
s/ == NULL//

Also, perhaps we could

const auto shm& = decoder_current_bitstream_buffer_->shm;

and use that subsequently in this method?

https://chromiumcodereview.appspot.com/1541353002/diff/60001/content/common/g...
content/common/gpu/media/v4l2_video_decode_accelerator.cc:555: if
(decoder_current_bitstream_buffer_->shm == NULL) {
s/ == NULL//

https://chromiumcodereview.appspot.com/1541353002/diff/60001/content/common/g...
content/common/gpu/media/v4l2_video_decode_accelerator.cc:556: // This is a
buffer of zero size, queued to flush the pipe.  Flush.
s/buffer of zero size,/dummy buffer,/

https://chromiumcodereview.appspot.com/1541353002/diff/60001/content/common/g...
content/common/gpu/media/v4l2_video_decode_accelerator.cc:1281: io_client_,
io_task_runner_, NULL, kFlushBufferId)));
nullptr

https://chromiumcodereview.appspot.com/1541353002/diff/60001/content/common/g...
File content/common/gpu/media/v4l2_video_encode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/1541353002/diff/60001/content/common/g...
content/common/gpu/media/v4l2_video_encode_accelerator.cc:53: : id(id),
shm(shm.Pass()) {}
std::move(shm) should work here.

https://chromiumcodereview.appspot.com/1541353002/diff/60001/content/common/g...
content/common/gpu/media/v4l2_video_encode_accelerator.cc:230: new
BitstreamBufferRef(buffer.id(), shm.Pass()));
std::move(shm)

https://chromiumcodereview.appspot.com/1541353002/diff/60001/content/common/g...
File content/common/gpu/media/vaapi_video_encode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/1541353002/diff/60001/content/common/g...
content/common/gpu/media/vaapi_video_encode_accelerator.cc:100: : id(id),
shm(shm.Pass()) {}
std::move

https://chromiumcodereview.appspot.com/1541353002/diff/60001/content/common/g...
content/common/gpu/media/vaapi_video_encode_accelerator.cc:672: buffer.id(),
shm.Pass(), buffer.offset(), buffer.size()));
I think we don't have this constructor in BitstreamBufferRef?

https://chromiumcodereview.appspot.com/1541353002/diff/60001/media/base/bitst...
File media/base/bitstream_buffer.h (right):

https://chromiumcodereview.appspot.com/1541353002/diff/60001/media/base/bitst...
media/base/bitstream_buffer.h:26: off_t offset);
Please add documentation what offset and size are, as it's not obvious anymore.

https://chromiumcodereview.appspot.com/1541353002/diff/60001/media/base/bitst...
media/base/bitstream_buffer.h:46: // The number of bytes of the bitstream
buffer.
Please clarify if this is the size of the bitstream data inside the shared
memory buffer (data_size) only, or of the whole shared memory (i.e. offset +
data_size).

https://chromiumcodereview.appspot.com/1541353002/diff/60001/media/base/bitst...
media/base/bitstream_buffer.h:49: // The offset of the bistream buffer to the
shared memory.
"Offset to the start of actual bitstream data in shared memory."

[Owen Lin, 2016-01-04 08:54:18]

PTAL. Thanks.

https://codereview.chromium.org/1541353002/diff/60001/content/common/gpu/clie...
File content/common/gpu/client/gpu_video_decode_accelerator_host.cc (right):

https://codereview.chromium.org/1541353002/diff/60001/content/common/gpu/clie...
content/common/gpu/client/gpu_video_decode_accelerator_host.cc:132: params.size
= base::checked_cast<uint32>(bitstream_buffer.size());
On 2015/12/31 02:05:53, Pawel Osciak wrote:
> We've just switched to standard integer types. Please update this CL to use
> uint32_t, int32_t, etc. after rebase.

Acknowledged.

https://codereview.chromium.org/1541353002/diff/60001/content/common/gpu/medi...
File content/common/gpu/media/shared_memory_region.cc (right):

https://codereview.chromium.org/1541353002/diff/60001/content/common/gpu/medi...
content/common/gpu/media/shared_memory_region.cc:25: off_t mask =
base::SysInfo::VMAllocationGranularity() - 1;
On 2015/12/31 02:05:53, Pawel Osciak wrote:
> This assumes granularity is a power of 2 (which normally it always should be,
> but technically we don't guarantee that from the documentation of the method).
> Perhaps something like this instead:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/mojo/edk/embedder/...

Done.

https://codereview.chromium.org/1541353002/diff/60001/content/common/gpu/medi...
content/common/gpu/media/shared_memory_region.cc:26: return shm_.MapAt((offset_
& ~mask), size_ + (offset_ & mask));
On 2015/12/31 02:05:53, Pawel Osciak wrote:
> Perhaps we could calculate new offset here (or even in constructor to drop
> offset_) and keep only the calculated offset, for use in memory()?

Done.

https://codereview.chromium.org/1541353002/diff/60001/content/common/gpu/medi...
File content/common/gpu/media/shared_memory_region.h (right):

https://codereview.chromium.org/1541353002/diff/60001/content/common/gpu/medi...
content/common/gpu/media/shared_memory_region.h:16: SharedMemoryRegion(const
base::SharedMemoryHandle& handle,
On 2015/12/31 02:05:54, Pawel Osciak wrote:
> We should document arguments, especially offset and size are not obvious, also
> the fact that this is taking ownership of shared memory.

Done.

https://codereview.chromium.org/1541353002/diff/60001/content/common/gpu/medi...
content/common/gpu/media/shared_memory_region.h:30: void* memory();
On 2015/12/31 02:05:54, Pawel Osciak wrote:
> We should document how this relates to offset and size passed to constructor.

Done.

https://codereview.chromium.org/1541353002/diff/60001/content/common/gpu/medi...
content/common/gpu/media/shared_memory_region.h:34: off_t offset() const {
return offset_; }
On 2015/12/31 02:05:53, Pawel Osciak wrote:
> Is this used anywhere?

No, let me remove it.

https://codereview.chromium.org/1541353002/diff/60001/content/common/gpu/medi...
File content/common/gpu/media/v4l2_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1541353002/diff/60001/content/common/gpu/medi...
content/common/gpu/media/v4l2_video_decode_accelerator.cc:543: if
(decoder_current_bitstream_buffer_->shm == NULL) {
On 2015/12/31 02:05:54, Pawel Osciak wrote:
> s/ == NULL//
> 
> Also, perhaps we could
> 
> const auto shm& = decoder_current_bitstream_buffer_->shm;
> 
> and use that subsequently in this method?

Done.

https://codereview.chromium.org/1541353002/diff/60001/content/common/gpu/medi...
content/common/gpu/media/v4l2_video_decode_accelerator.cc:555: if
(decoder_current_bitstream_buffer_->shm == NULL) {
On 2015/12/31 02:05:54, Pawel Osciak wrote:
> s/ == NULL//

Done.

https://codereview.chromium.org/1541353002/diff/60001/content/common/gpu/medi...
content/common/gpu/media/v4l2_video_decode_accelerator.cc:556: // This is a
buffer of zero size, queued to flush the pipe.  Flush.
On 2015/12/31 02:05:54, Pawel Osciak wrote:
> s/buffer of zero size,/dummy buffer,/

Done.

https://codereview.chromium.org/1541353002/diff/60001/content/common/gpu/medi...
content/common/gpu/media/v4l2_video_decode_accelerator.cc:1281: io_client_,
io_task_runner_, NULL, kFlushBufferId)));
On 2015/12/31 02:05:54, Pawel Osciak wrote:
> nullptr

Done.

https://codereview.chromium.org/1541353002/diff/60001/content/common/gpu/medi...
File content/common/gpu/media/v4l2_video_encode_accelerator.cc (right):

https://codereview.chromium.org/1541353002/diff/60001/content/common/gpu/medi...
content/common/gpu/media/v4l2_video_encode_accelerator.cc:53: : id(id),
shm(shm.Pass()) {}
On 2015/12/31 02:05:54, Pawel Osciak wrote:
> std::move(shm) should work here.

Done.

https://codereview.chromium.org/1541353002/diff/60001/content/common/gpu/medi...
content/common/gpu/media/v4l2_video_encode_accelerator.cc:230: new
BitstreamBufferRef(buffer.id(), shm.Pass()));
On 2015/12/31 02:05:54, Pawel Osciak wrote:
> std::move(shm)

Done.

https://codereview.chromium.org/1541353002/diff/60001/content/common/gpu/medi...
File content/common/gpu/media/vaapi_video_encode_accelerator.cc (right):

https://codereview.chromium.org/1541353002/diff/60001/content/common/gpu/medi...
content/common/gpu/media/vaapi_video_encode_accelerator.cc:100: : id(id),
shm(shm.Pass()) {}
On 2015/12/31 02:05:54, Pawel Osciak wrote:
> std::move

Done.

https://codereview.chromium.org/1541353002/diff/60001/content/common/gpu/medi...
content/common/gpu/media/vaapi_video_encode_accelerator.cc:672: buffer.id(),
shm.Pass(), buffer.offset(), buffer.size()));
On 2015/12/31 02:05:54, Pawel Osciak wrote:
> I think we don't have this constructor in BitstreamBufferRef?

Done.

https://codereview.chromium.org/1541353002/diff/60001/media/base/bitstream_bu...
File media/base/bitstream_buffer.h (right):

https://codereview.chromium.org/1541353002/diff/60001/media/base/bitstream_bu...
media/base/bitstream_buffer.h:26: off_t offset);
On 2015/12/31 02:05:54, Pawel Osciak wrote:
> Please add documentation what offset and size are, as it's not obvious
anymore.

Done.

https://codereview.chromium.org/1541353002/diff/60001/media/base/bitstream_bu...
media/base/bitstream_buffer.h:46: // The number of bytes of the bitstream
buffer.
On 2015/12/31 02:05:54, Pawel Osciak wrote:
> Please clarify if this is the size of the bitstream data inside the shared
> memory buffer (data_size) only, or of the whole shared memory (i.e. offset +
> data_size).

Done.

https://codereview.chromium.org/1541353002/diff/60001/media/base/bitstream_bu...
media/base/bitstream_buffer.h:49: // The offset of the bistream buffer to the
shared memory.
On 2015/12/31 02:05:54, Pawel Osciak wrote:
> "Offset to the start of actual bitstream data in shared memory."

Done.

[Pawel Osciak, 2016-01-05 06:22:51]

https://chromiumcodereview.appspot.com/1541353002/diff/80001/content/common/g...
File content/common/gpu/media/shared_memory_region.cc (right):

https://chromiumcodereview.appspot.com/1541353002/diff/80001/content/common/g...
content/common/gpu/media/shared_memory_region.cc:18: DCHECK_LT(offset_, 0) <<
"Invalid offset: " << offset_;
DCHECK_GE ?

https://chromiumcodereview.appspot.com/1541353002/diff/80001/content/common/g...
content/common/gpu/media/shared_memory_region.cc:30: return shm_.MapAt(offset_ -
extra_size_, size_ + extra_size_);
I'm wondering, if offset is < 0, then extra_size_ will be negative, but this
could possibly succeed, since we subtract a negative, so we will actually be
adding (it'd also require size < shared memory size, but still possible). Then
in memory() we'd be adding a negative, so subtracting and we may provide an
invalid pointer, to before mapped memory... That would be very unexpected for
Map() to succeed, but memory() to provide an invalid pointer.

Perhaps we should explicitly disallow offset < 0, maybe failing here if
extra_size_ < 0?

https://chromiumcodereview.appspot.com/1541353002/diff/80001/content/common/g...
File content/common/gpu/media/shared_memory_region.h (right):

https://chromiumcodereview.appspot.com/1541353002/diff/80001/content/common/g...
content/common/gpu/media/shared_memory_region.h:20: // Please make sure |handle|
is not |auto_close|.
Could we instead call SharedMemory::DuplicateHandle() in the constructor and use
that to create SharedMemory to avoid this issue?

https://chromiumcodereview.appspot.com/1541353002/diff/80001/content/common/g...
content/common/gpu/media/shared_memory_region.h:36: // |offset| bytes from the
start of the whole shared memory.
The returned pointer points to the memory at the offset previously passed to the
constructor.

https://chromiumcodereview.appspot.com/1541353002/diff/80001/content/common/g...
content/common/gpu/media/shared_memory_region.h:45: size_t extra_size_;
Nit: s/extra_size_/aligned_size_/

https://chromiumcodereview.appspot.com/1541353002/diff/80001/media/base/bitst...
File media/base/bitstream_buffer.h (right):

https://chromiumcodereview.appspot.com/1541353002/diff/80001/media/base/bitst...
media/base/bitstream_buffer.h:25: // is |size| bytes.
s/length/payload/

[Owen Lin, 2016-01-06 03:45:46]

Also tested on peach_pit with a modified vda_unittest with offset.

https://codereview.chromium.org/1541353002/diff/80001/content/common/gpu/medi...
File content/common/gpu/media/shared_memory_region.cc (right):

https://codereview.chromium.org/1541353002/diff/80001/content/common/gpu/medi...
content/common/gpu/media/shared_memory_region.cc:18: DCHECK_LT(offset_, 0) <<
"Invalid offset: " << offset_;
On 2016/01/05 06:22:51, Pawel Osciak wrote:
> DCHECK_GE ?

Done.

https://codereview.chromium.org/1541353002/diff/80001/content/common/gpu/medi...
content/common/gpu/media/shared_memory_region.cc:30: return shm_.MapAt(offset_ -
extra_size_, size_ + extra_size_);
On 2016/01/05 06:22:51, Pawel Osciak wrote:
> I'm wondering, if offset is < 0, then extra_size_ will be negative, but this
> could possibly succeed, since we subtract a negative, so we will actually be
> adding (it'd also require size < shared memory size, but still possible). Then
> in memory() we'd be adding a negative, so subtracting and we may provide an
> invalid pointer, to before mapped memory... That would be very unexpected for
> Map() to succeed, but memory() to provide an invalid pointer.
> 
> Perhaps we should explicitly disallow offset < 0, maybe failing here if
> extra_size_ < 0?

Done.

https://codereview.chromium.org/1541353002/diff/80001/content/common/gpu/medi...
File content/common/gpu/media/shared_memory_region.h (right):

https://codereview.chromium.org/1541353002/diff/80001/content/common/gpu/medi...
content/common/gpu/media/shared_memory_region.h:20: // Please make sure |handle|
is not |auto_close|.
On 2016/01/05 06:22:51, Pawel Osciak wrote:
> Could we instead call SharedMemory::DuplicateHandle() in the constructor and
use
> that to create SharedMemory to avoid this issue?

Oops, I was confused by the document.

https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/shared...

The handle we used is actually FileDescriptor. (On POSIX) 

https://code.google.com/p/chromium/codesearch#chromium/src/base/file_descript...

No matter what's the value of |auto_close| is. FileDescriptor won't close it.
Yes, it sounds different from other SharedMemoryHandle.

I just rephrased the document about this.

https://codereview.chromium.org/1541353002/diff/80001/content/common/gpu/medi...
content/common/gpu/media/shared_memory_region.h:36: // |offset| bytes from the
start of the whole shared memory.
On 2016/01/05 06:22:51, Pawel Osciak wrote:
> The returned pointer points to the memory at the offset previously passed to
the
> constructor.

Done.

https://codereview.chromium.org/1541353002/diff/80001/content/common/gpu/medi...
content/common/gpu/media/shared_memory_region.h:45: size_t extra_size_;
On 2016/01/05 06:22:51, Pawel Osciak wrote:
> Nit: s/extra_size_/aligned_size_/

Done.

https://codereview.chromium.org/1541353002/diff/80001/media/base/bitstream_bu...
File media/base/bitstream_buffer.h (right):

https://codereview.chromium.org/1541353002/diff/80001/media/base/bitstream_bu...
media/base/bitstream_buffer.h:25: // is |size| bytes.
On 2016/01/05 06:22:51, Pawel Osciak wrote:
> s/length/payload/

Done.

[Pawel Osciak, 2016-01-12 07:33:05]

lgtm % tiny nits, sorry

https://chromiumcodereview.appspot.com/1541353002/diff/140001/content/common/...
File content/common/gpu/media/shared_memory_region.h (right):

https://chromiumcodereview.appspot.com/1541353002/diff/140001/content/common/...
content/common/gpu/media/shared_memory_region.h:35: // to the memory at the
offset previous passed to the constructor.
s/previous/previously/

https://chromiumcodereview.appspot.com/1541353002/diff/140001/content/common/...
content/common/gpu/media/shared_memory_region.h:44: size_t aligned_size_;
I'm so sorry, I mistyped my previous comment. I meant "alignment_size_"

[Owen Lin, 2016-01-13 04:09:02]

owenlin@chromium.org changed reviewers:
+ ccameron@chromium.org, dcheng@chromium.org, xhwang@chromium.org

[Owen Lin, 2016-01-13 04:09:02]

Need owners' review. PTAL, thanks.

https://codereview.chromium.org/1541353002/diff/140001/content/common/gpu/med...
File content/common/gpu/media/shared_memory_region.h (right):

https://codereview.chromium.org/1541353002/diff/140001/content/common/gpu/med...
content/common/gpu/media/shared_memory_region.h:35: // to the memory at the
offset previous passed to the constructor.
On 2016/01/12 07:33:04, Pawel Osciak wrote:
> s/previous/previously/

Done.

https://codereview.chromium.org/1541353002/diff/140001/content/common/gpu/med...
content/common/gpu/media/shared_memory_region.h:44: size_t aligned_size_;
On 2016/01/12 07:33:04, Pawel Osciak wrote:
> I'm so sorry, I mistyped my previous comment. I meant "alignment_size_"

Done.

[ccameron, 2016-01-13 10:33:27]

On 2016/01/13 04:09:02, Owen Lin wrote:
> Need owners' review. PTAL, thanks.

content/common/gpu lgtm stamp

[xhwang, 2016-01-13 18:54:54]

https://chromiumcodereview.appspot.com/1541353002/diff/160001/media/base/bits...
File media/base/bitstream_buffer.h (right):

https://chromiumcodereview.appspot.com/1541353002/diff/160001/media/base/bits...
media/base/bitstream_buffer.h:32: off_t offset);
I never used off_t before. After a bit searching around, it seems off_t is not
in standard C and in posix its' for file sizes. Could you please provide more
details about off_t and why it's the best choice here?

https://chromiumcodereview.appspot.com/1541353002/diff/160001/media/base/bits...
media/base/bitstream_buffer.h:32: off_t offset);
What's the default |presentation_timestamp| in this case? I wonder whether the
convenience of multiple ctor's is worth the confusion...

[Owen Lin, 2016-01-14 03:56:34]

https://codereview.chromium.org/1541353002/diff/160001/media/base/bitstream_b...
File media/base/bitstream_buffer.h (right):

https://codereview.chromium.org/1541353002/diff/160001/media/base/bitstream_b...
media/base/bitstream_buffer.h:32: off_t offset);
On 2016/01/13 18:54:54, xhwang wrote:
> I never used off_t before. After a bit searching around, it seems off_t is not
> in standard C and in posix its' for file sizes. Could you please provide more
> details about off_t and why it's the best choice here?

This parameter is used to calculate the offset when we call
SharedMemory.MapAt(off_t, size_t);

https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/shared...

It is natural to use the same type of the argument "offset" in MapAt().

https://codereview.chromium.org/1541353002/diff/160001/media/base/bitstream_b...
media/base/bitstream_buffer.h:32: off_t offset);
On 2016/01/13 18:54:54, xhwang wrote:
> What's the default |presentation_timestamp| in this case? I wonder whether the
> convenience of multiple ctor's is worth the confusion...

When the |presentation_timestamp| is not provided it will be
media::kNoTimestamp().

About the multiple ctros, I believe it because there are default values for
|offset| (as 0) and |presentation_timestamp| (as kNoTimestamp). I originally
thought default argument is not allowed for chromium code style. But I checked
it again, and found it is only banned for virtual functions. 

https://google.github.io/styleguide/cppguide.html#Default_Arguments

I am going to merge those four ctors into just one with default arguments.

However, there is one case cannot be covered, i.e., with timestamp but without
offset. There are only two places of such case in the code base. So I modified
the related codes so that only one ctor is required.

[xhwang, 2016-01-14 20:31:50]

lgtm

[Owen Lin, 2016-01-15 06:57:42]

The CQ bit was checked by owenlin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-15 06:58:00]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1541353002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1541353002/180001

[commit-bot: I haz the power, 2016-01-15 07:13:01]

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

[commit-bot: I haz the power, 2016-01-15 07:13:01]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[Owen Lin, 2016-01-21 08:53:45]

Patchset #10 (id:200001) has been deleted

[Owen Lin, 2016-01-21 08:54:21]

Patchset #10 (id:220001) has been deleted

[Owen Lin, 2016-01-22 04:27:12]

Patchset #10 (id:240001) has been deleted

[Owen Lin, 2016-01-22 04:27:24]

Patchset #10 (id:260001) has been deleted

[Owen Lin, 2016-01-22 04:27:49]

The CQ bit was checked by owenlin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-22 04:28:15]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1541353002/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1541353002/280001

[commit-bot: I haz the power, 2016-01-22 05:37:35]

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

[commit-bot: I haz the power, 2016-01-22 05:37:35]

Dry run: This issue passed the CQ dry run.

[dcheng, 2016-01-22 18:27:39]

Personally, I think that offset should be some unsigned type and that the
burdening of doing this checking should be pushed into SharedMemory::MapAt.
WDYT?

https://codereview.chromium.org/1541353002/diff/280001/content/common/gpu/med...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1541353002/diff/280001/content/common/gpu/med...
content/common/gpu/media/gpu_video_decode_accelerator.cc:514: params.offset,
params.presentation_timestamp);
Where do we validate that params.offset can fit in an off_t? What if
params.offset > INT_MAX?

https://codereview.chromium.org/1541353002/diff/280001/content/common/gpu/med...
File content/common/gpu/media/shared_memory_region.h (right):

https://codereview.chromium.org/1541353002/diff/280001/content/common/gpu/med...
content/common/gpu/media/shared_memory_region.h:21: off_t offset,
Does it make sense to use off_t here? I know that posix uses it, but off_t is
signed, and for our purposes, this can only be an offset from the start of the
buffer.

https://codereview.chromium.org/1541353002/diff/280001/media/base/bitstream_b...
File media/base/bitstream_buffer.h (right):

https://codereview.chromium.org/1541353002/diff/280001/media/base/bitstream_b...
media/base/bitstream_buffer.h:48: off_t offset() const { return offset_; }
Any particular to reason to use off_t? I know mmap (and SharedMemory::MapAt
currently) uses it, but since off_t is signed, this means we need to do checks
in various places. Would it make more sense to just use size_t?

[Pawel Osciak, 2016-01-25 08:20:02]

https://codereview.chromium.org/1541353002/diff/280001/content/common/gpu/med...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1541353002/diff/280001/content/common/gpu/med...
content/common/gpu/media/gpu_video_decode_accelerator.cc:514: params.offset,
params.presentation_timestamp);
On 2016/01/22 18:27:39, dcheng wrote:
> Where do we validate that params.offset can fit in an off_t? What if
> params.offset > INT_MAX?

GNU defines off_t as 32 or 64 bit, so there it would always be ok, but from what
I can see, POSIX only defines off_t as "signed integer type"
(http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_types.h.html),
which I'm not sure guarantees 32 bits. I guess we could always checked_cast here
as well to be sure.

https://codereview.chromium.org/1541353002/diff/280001/content/common/gpu/med...
File content/common/gpu/media/shared_memory_region.h (right):

https://codereview.chromium.org/1541353002/diff/280001/content/common/gpu/med...
content/common/gpu/media/shared_memory_region.h:21: off_t offset,
On 2016/01/22 18:27:39, dcheng wrote:
> Does it make sense to use off_t here? I know that posix uses it, but off_t is
> signed, and for our purposes, this can only be an offset from the start of the
> buffer. 

SharedMemory::MapAt(), which we pass this to uses off_t, so this helps avoid
conversions, even if we only use positive values.

https://codereview.chromium.org/1541353002/diff/280001/media/base/bitstream_b...
File media/base/bitstream_buffer.h (right):

https://codereview.chromium.org/1541353002/diff/280001/media/base/bitstream_b...
media/base/bitstream_buffer.h:48: off_t offset() const { return offset_; }
On 2016/01/22 18:27:39, dcheng wrote:
> Any particular to reason to use off_t? I know mmap (and SharedMemory::MapAt
> currently) uses it, but since off_t is signed, this means we need to do checks
> in various places. Would it make more sense to just use size_t?

MapAt() will fail us if the offset is invalid, e.g. if we pass a negative value
for some reason. So I don't think we have to check for <0 explicitly. Also,
technically only in the context of SharedMemoryRegion (not BitstreamBuffer
itself) we need to check and only due to the way we calculate alignment.
Otherwise we can always depend on the system to fail us on invalid offset
values.

[Owen Lin, 2016-01-25 09:30:59]

https://codereview.chromium.org/1541353002/diff/280001/content/common/gpu/med...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1541353002/diff/280001/content/common/gpu/med...
content/common/gpu/media/gpu_video_decode_accelerator.cc:514: params.offset,
params.presentation_timestamp);
On 2016/01/22 18:27:39, dcheng wrote:
> Where do we validate that params.offset can fit in an off_t? What if
> params.offset > INT_MAX?

Good catch, code modified.

[dcheng, 2016-01-25 19:09:18]

https://codereview.chromium.org/1541353002/diff/300001/content/common/gpu/med...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1541353002/diff/300001/content/common/gpu/med...
content/common/gpu/media/gpu_video_decode_accelerator.cc:514:
checked_cast<off_t>(params.offset), params.presentation_timestamp);
Should this really crash the GPU (I think?) process? It seems like it should
just be returning and failing gracefully.

I think it'd be better to have a ParamTraits for BitstreamBuffer, so the
validation code can be in a common logic, rather than interspersed through
various IPC message handlers.

[Owen Lin, 2016-01-29 02:51:01]

The CQ bit was checked by owenlin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-29 02:51:48]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1541353002/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1541353002/320001

[commit-bot: I haz the power, 2016-01-29 04:06:23]

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

[commit-bot: I haz the power, 2016-01-29 04:06:24]

Dry run: This issue passed the CQ dry run.

[Owen Lin, 2016-01-29 05:38:17]

https://codereview.chromium.org/1541353002/diff/300001/content/common/gpu/med...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1541353002/diff/300001/content/common/gpu/med...
content/common/gpu/media/gpu_video_decode_accelerator.cc:514:
checked_cast<off_t>(params.offset), params.presentation_timestamp);
On 2016/01/25 19:09:18, dcheng wrote:
> Should this really crash the GPU (I think?) process? It seems like it should
> just be returning and failing gracefully.
> 
> I think it'd be better to have a ParamTraits for BitstreamBuffer, so the
> validation code can be in a common logic, rather than interspersed through
> various IPC message handlers.

Change to returning error.

Besides, I also tried the ParamTrais
(https://codereview.chromium.org/1645873002/) 

However, the validation rules for a bitstream buffer are:

1. the handle is valid
2. the id is positive.
3. the offset is in the range of off_t.

However a BitstreamBuffer may not satisfy 1 and 2. That's means we may Write() a
BitstreamBuffer but fail to Read() it back. This sounds weired. So, I still
prefer leaving the validation job in gpu_video|jpeg_decode_accelerator.cc, or
even defer it to the real accelerators.

How do you think?

[dcheng, 2016-01-29 22:34:19]

On 2016/01/29 at 05:38:17, owenlin wrote:
>
https://codereview.chromium.org/1541353002/diff/300001/content/common/gpu/med...
> File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):
> 
>
https://codereview.chromium.org/1541353002/diff/300001/content/common/gpu/med...
> content/common/gpu/media/gpu_video_decode_accelerator.cc:514:
checked_cast<off_t>(params.offset), params.presentation_timestamp);
> On 2016/01/25 19:09:18, dcheng wrote:
> > Should this really crash the GPU (I think?) process? It seems like it should
> > just be returning and failing gracefully.
> > 
> > I think it'd be better to have a ParamTraits for BitstreamBuffer, so the
> > validation code can be in a common logic, rather than interspersed through
> > various IPC message handlers.
> 
> Change to returning error.
> 
> Besides, I also tried the ParamTrais
(https://codereview.chromium.org/1645873002/) 
> 
> However, the validation rules for a bitstream buffer are:
> 
> 1. the handle is valid
> 2. the id is positive.
> 3. the offset is in the range of off_t.
> 
> However a BitstreamBuffer may not satisfy 1 and 2. That's means we may Write()
a BitstreamBuffer but fail to Read() it back. This sounds weired. So, I still
prefer leaving the validation job in gpu_video|jpeg_decode_accelerator.cc, or
even defer it to the real accelerators.
> 
> How do you think?

Why would we need to write an invalid bitstream buffer?

[Owen Lin, 2016-02-01 02:39:30]

On 2016/01/29 22:34:19, dcheng wrote:
> On 2016/01/29 at 05:38:17, owenlin wrote:
> >
>
https://codereview.chromium.org/1541353002/diff/300001/content/common/gpu/med...
> > File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):
> > 
> >
>
https://codereview.chromium.org/1541353002/diff/300001/content/common/gpu/med...
> > content/common/gpu/media/gpu_video_decode_accelerator.cc:514:
> checked_cast<off_t>(params.offset), params.presentation_timestamp);
> > On 2016/01/25 19:09:18, dcheng wrote:
> > > Should this really crash the GPU (I think?) process? It seems like it
should
> > > just be returning and failing gracefully.
> > > 
> > > I think it'd be better to have a ParamTraits for BitstreamBuffer, so the
> > > validation code can be in a common logic, rather than interspersed through
> > > various IPC message handlers.
> > 
> > Change to returning error.
> > 
> > Besides, I also tried the ParamTrais
> (https://codereview.chromium.org/1645873002/) 
> > 
> > However, the validation rules for a bitstream buffer are:
> > 
> > 1. the handle is valid
> > 2. the id is positive.
> > 3. the offset is in the range of off_t.
> > 
> > However a BitstreamBuffer may not satisfy 1 and 2. That's means we may
Write()
> a BitstreamBuffer but fail to Read() it back. This sounds weired. So, I still
> prefer leaving the validation job in gpu_video|jpeg_decode_accelerator.cc, or
> even defer it to the real accelerators.
> > 
> > How do you think?
> 
> Why would we need to write an invalid bitstream buffer?

No, we don't need to. But even it happens (due to a bug?) it's not ParamTraits's
job to do the check. The job for ParamTraits should just be serializing and
deserilizing the object. For now, one could create an invalid BitstreamBuffer
and then call VDA->Decode() WITH or WITHOUT passing the argument via IPC. The
point is VDA need to do the check, instead of any one in the IPC pipeline.

[wuchengli1, 2016-02-02 06:49:15]

wuchengli@google.com changed reviewers:
+ wuchengli@google.com

[wuchengli1, 2016-02-02 06:49:16]

dcheng@ PTAL. Thanks.

[dcheng, 2016-02-02 07:25:29]

On 2016/02/01 at 02:39:30, owenlin wrote:
> On 2016/01/29 22:34:19, dcheng wrote:
> > On 2016/01/29 at 05:38:17, owenlin wrote:
> > >
> >
https://codereview.chromium.org/1541353002/diff/300001/content/common/gpu/med...
> > > File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):
> > > 
> > >
> >
https://codereview.chromium.org/1541353002/diff/300001/content/common/gpu/med...
> > > content/common/gpu/media/gpu_video_decode_accelerator.cc:514:
> > checked_cast<off_t>(params.offset), params.presentation_timestamp);
> > > On 2016/01/25 19:09:18, dcheng wrote:
> > > > Should this really crash the GPU (I think?) process? It seems like it
should
> > > > just be returning and failing gracefully.
> > > > 
> > > > I think it'd be better to have a ParamTraits for BitstreamBuffer, so the
> > > > validation code can be in a common logic, rather than interspersed
through
> > > > various IPC message handlers.
> > > 
> > > Change to returning error.
> > > 
> > > Besides, I also tried the ParamTrais
> > (https://codereview.chromium.org/1645873002/) 
> > > 
> > > However, the validation rules for a bitstream buffer are:
> > > 
> > > 1. the handle is valid
> > > 2. the id is positive.
> > > 3. the offset is in the range of off_t.
> > > 
> > > However a BitstreamBuffer may not satisfy 1 and 2. That's means we may
Write()
> > a BitstreamBuffer but fail to Read() it back. This sounds weired. So, I
still
> > prefer leaving the validation job in gpu_video|jpeg_decode_accelerator.cc,
or
> > even defer it to the real accelerators.
> > > 
> > > How do you think?
> > 
> > Why would we need to write an invalid bitstream buffer?
> 
> No, we don't need to. But even it happens (due to a bug?) it's not
ParamTraits's job to do the check. The job for ParamTraits should just be
serializing and deserilizing the object. For now, one could create an invalid
BitstreamBuffer and then call VDA->Decode() WITH or WITHOUT passing the argument
via IPC. The point is VDA need to do the check, instead of any one in the IPC
pipeline.

ParamTraits is not just for serialization / deserialization: it verifies what it
can as well. Doing it in ParamTraits is preferable to doing it in every message
handler, to avoid duplicating code.

A examples where ParamTraits performs validation:
- ParamTraits<gfx::Size>:
https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/ipc/gfx_par...
- ParamTraits<std::map<K, V>>:
https://code.google.com/p/chromium/codesearch#chromium/src/ipc/ipc_message_ut...
- ParamTraits<gpu::SyncToken>:
https://code.google.com/p/chromium/codesearch#chromium/src/gpu/ipc/gpu_comman...

[Pawel Osciak, 2016-02-02 08:09:00]

On 2016/02/02 07:25:29, dcheng wrote:
>
> ParamTraits is not just for serialization / deserialization: it verifies what
it
> can as well. Doing it in ParamTraits is preferable to doing it in every
message
> handler, to avoid duplicating code.
> 
> A examples where ParamTraits performs validation:
> - ParamTraits<gfx::Size>:
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/ipc/gfx_par...
> - ParamTraits<std::map<K, V>>:
>
https://code.google.com/p/chromium/codesearch#chromium/src/ipc/ipc_message_ut...
> - ParamTraits<gpu::SyncToken>:
>
https://code.google.com/p/chromium/codesearch#chromium/src/gpu/ipc/gpu_comman...

In general, we can't 100% tell if values are valid without attempting to map, so
we can't really fully verify in message handlers either. We can technically
check against SharedMemoryHandle::IsValid(), or size < 0, but that still does
not mean the handle is valid and can be used. However, we can't do anything too
meaningful (or break anything) with the passed values for shared memory unless
we actually try to map and access either. So there is no real way to misuse
them.

A full check (including size < 0, etc.) will always happen when we try to
actually do something with the buffer. This only can (and will be) verified by
base::SharedMemory when we try to map/access the buffer via the handle with the
given offset, size, etc.

So I agree we should probably either do it in traits if the preference is to do
at least some (partial) verification there first. Or not do this at all in
handlers and let SharedMemory do this, which it will anyway.

[Owen Lin, 2016-02-02 08:47:25]

On 2016/02/02 08:09:00, Pawel Osciak wrote:
> On 2016/02/02 07:25:29, dcheng wrote:
> >
> > ParamTraits is not just for serialization / deserialization: it verifies
what
> it
> > can as well. Doing it in ParamTraits is preferable to doing it in every
> message
> > handler, to avoid duplicating code.
> > 
> > A examples where ParamTraits performs validation:
> > - ParamTraits<gfx::Size>:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/ipc/gfx_par...
> > - ParamTraits<std::map<K, V>>:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/ipc/ipc_message_ut...
> > - ParamTraits<gpu::SyncToken>:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/gpu/ipc/gpu_comman...
> 
> In general, we can't 100% tell if values are valid without attempting to map,
so
> we can't really fully verify in message handlers either. We can technically
> check against SharedMemoryHandle::IsValid(), or size < 0, but that still does
> not mean the handle is valid and can be used. However, we can't do anything
too
> meaningful (or break anything) with the passed values for shared memory unless
> we actually try to map and access either. So there is no real way to misuse
> them.
> 
> A full check (including size < 0, etc.) will always happen when we try to
> actually do something with the buffer. This only can (and will be) verified by
> base::SharedMemory when we try to map/access the buffer via the handle with
the
> given offset, size, etc.
> 
> So I agree we should probably either do it in traits if the preference is to
do
> at least some (partial) verification there first. Or not do this at all in
> handlers and let SharedMemory do this, which it will anyway.

The examples actually support my points.

The gfx::Size is designed to NOT allow negative width or height. 
https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/geometry/si...

So if that happens, it must something wrong during the
serializing/deserializing. 
Same for the map<> case, we cannot have a map with negative size. If that
happens, it must serializing/descrializing issue.

However, it is not the case BitstreamBuffer(). One can easy create a bitstream
buffer which is not valid:
     BitstreamBuffer bsb(-1, SharedMemryHandle(), 0);

And then call 
     VideoDecodeAccelerator->Decode(bsb);

If that is allowed, we should also allow such object to be send through the IPC
channel.

For now, there are two choices.

1.) Add those enforcement to BitstreamBuffer(), and remove the checks in GVDA
and GJDA. We can do the same check in ParamTraits::Read().

2.) Remove the checks from GVDA and ParamTraits. Let each VDA to report the
errors.

I am fine with both options, the only concern is there maybe some special case
where we need to create an invalid bitstream buffer. (Hope not.) How do you
think?

[dcheng, 2016-02-02 23:04:40]

On 2016/02/02 at 08:47:25, owenlin wrote:
> On 2016/02/02 08:09:00, Pawel Osciak wrote:
> > On 2016/02/02 07:25:29, dcheng wrote:
> > >
> > > ParamTraits is not just for serialization / deserialization: it verifies
what
> > it
> > > can as well. Doing it in ParamTraits is preferable to doing it in every
> > message
> > > handler, to avoid duplicating code.
> > > 
> > > A examples where ParamTraits performs validation:
> > > - ParamTraits<gfx::Size>:
> > >
> >
https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/ipc/gfx_par...
> > > - ParamTraits<std::map<K, V>>:
> > >
> >
https://code.google.com/p/chromium/codesearch#chromium/src/ipc/ipc_message_ut...
> > > - ParamTraits<gpu::SyncToken>:
> > >
> >
https://code.google.com/p/chromium/codesearch#chromium/src/gpu/ipc/gpu_comman...
> > 
> > In general, we can't 100% tell if values are valid without attempting to
map, so
> > we can't really fully verify in message handlers either. We can technically
> > check against SharedMemoryHandle::IsValid(), or size < 0, but that still
does
> > not mean the handle is valid and can be used. However, we can't do anything
too
> > meaningful (or break anything) with the passed values for shared memory
unless
> > we actually try to map and access either. So there is no real way to misuse
> > them.
> > 
> > A full check (including size < 0, etc.) will always happen when we try to
> > actually do something with the buffer. This only can (and will be) verified
by
> > base::SharedMemory when we try to map/access the buffer via the handle with
the
> > given offset, size, etc.
> > 
> > So I agree we should probably either do it in traits if the preference is to
do
> > at least some (partial) verification there first. Or not do this at all in
> > handlers and let SharedMemory do this, which it will anyway.
> 
> The examples actually support my points.
> 
> The gfx::Size is designed to NOT allow negative width or height. 
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/geometry/si...
> 
> So if that happens, it must something wrong during the
serializing/deserializing. 
> Same for the map<> case, we cannot have a map with negative size. If that
happens, it must serializing/descrializing issue.
> 

Sorry I will try to write a longer reply, but I am dealing with some urgent
security bugs atm. The point here is that there is precedent for ParamTraits
sanitizing things: it is not just a serializing/deserializing issue. There are
plenty of other examples in the codebase (ParamTraits<GURL> is another notable
one).

> However, it is not the case BitstreamBuffer(). One can easy create a bitstream
buffer which is not valid:
>      BitstreamBuffer bsb(-1, SharedMemryHandle(), 0);

Maybe. But that doesn't happen today. If this situation does come up, it's easy
enough to guard against: we can define that an invalid BitstreamBuffer has a
fixed ID, null SharedMemoryHandle(), etc.

> 
> And then call 
>      VideoDecodeAccelerator->Decode(bsb);
> 
> If that is allowed, we should also allow such object to be send through the
IPC channel.
> 
> For now, there are two choices.
> 
> 1.) Add those enforcement to BitstreamBuffer(), and remove the checks in GVDA
and GJDA. We can do the same check in ParamTraits::Read().
> 
> 2.) Remove the checks from GVDA and ParamTraits. Let each VDA to report the
errors.
> 
> I am fine with both options, the only concern is there maybe some special case
where we need to create an invalid bitstream buffer. (Hope not.) How do you
think?

In this case, validating a SharedMemoryHandle is tricky, since you cannot do it
without mapping. But we can still do some sanitization that the value for offset
is in range for off_t, etc. That's why I think it makes more sense to directly
embed the BitstreamBuffer into the params: we can do the easy validation in a
common place, rather than hoping everything remembers to call
VerifyDecodeParams().

[Owen Lin, 2016-02-24 07:37:11]

Patchset #14 (id:360001) has been deleted

[Owen Lin, 2016-02-24 07:41:01]

dcheng@, PTAL. Patch #13 is the rebase of the original code over the TOT. You
can review the diff between #13 and #14 to see what are modified.

[dcheng, 2016-03-01 01:48:43]

Overall, seems reasonable.

One random thought (I don't expect this to be implemented in this CL, and I'm
not sure it makes sense at all, since there's no precedent for it)… but it would
be kind of nice if we could wrap all the SharedMemory handling inside
BitstreamBuffer's ParamTraits.

So BitstreamBuffer would become non-copyable, embedding a base::SharedMemory
object directly. ParamTraits<BitstreamBuffer>::Write() knows to serialize this
as a SharedMemoryHandle + size. On the other end,
ParamTraits<BitstreamBuffer>::Read() knows to unserialize the handle + size pair
into a SharedMemory object.

Again, to be clear, the way the code is currently structured seems to be safe,
but this seems like it might be an improvement worth pursuing. It's also
possible that Mojo will obsolete all this, so I wouldn't put too much effort
into this route.

https://codereview.chromium.org/1541353002/diff/380001/content/common/gpu/gpu...
File content/common/gpu/gpu_messages.cc (right):

https://codereview.chromium.org/1541353002/diff/380001/content/common/gpu/gpu...
content/common/gpu/gpu_messages.cc:19: WriteParam(m,
static_cast<int64_t>(p.offset()));
Does it make sense to DCHECK() that this is non-negative?

https://codereview.chromium.org/1541353002/diff/380001/content/common/gpu/gpu...
content/common/gpu/gpu_messages.cc:49: if (!checked_offset.IsValid()) {
Similarly, does it make sense to assert that the value is non-negative?

https://codereview.chromium.org/1541353002/diff/380001/content/common/gpu/med...
File content/common/gpu/media/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1541353002/diff/380001/content/common/gpu/med...
content/common/gpu/media/android_video_decode_accelerator.cc:388: shm.reset(new
SharedMemoryRegion(base::SharedMemoryHandle(), 0, 0, false));
Maybe just shm.reset() and null check when initializing |memory| below? Then I
think you won't need the overloaded constructor.

const uint8_t* memory = shm ? static_cast<const uint8_t*>(shm->memory()) :
nullptr;

https://codereview.chromium.org/1541353002/diff/380001/content/common/gpu/med...
File content/common/gpu/media/shared_memory_region.h (right):

https://codereview.chromium.org/1541353002/diff/380001/content/common/gpu/med...
content/common/gpu/media/shared_memory_region.h:13: // Helper class to access a
region of a SharedMemory.
This class needs a better comment: specifically, base::SharedMemory requires
that offset be aligned, so this class wraps it with some pointer arithmetic to
hide that fact.

https://codereview.chromium.org/1541353002/diff/380001/content/common/gpu/med...
File content/common/gpu/media/v4l2_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1541353002/diff/380001/content/common/gpu/med...
content/common/gpu/media/v4l2_video_decode_accelerator.cc:68:
SharedMemoryRegion* shm,
scoped_ptr<SharedMemoryRegion> to have the compiler document that ownership is
transferred (similar for the other BitstreamBufferRef helpers)

https://codereview.chromium.org/1541353002/diff/380001/media/base/bitstream_b...
File media/base/bitstream_buffer.cc (right):

https://codereview.chromium.org/1541353002/diff/380001/media/base/bitstream_b...
media/base/bitstream_buffer.cc:10: : BitstreamBuffer(-1,
base::SharedMemoryHandle(), 0, 0, kNoTimestamp()) {}
Since you have default args on the constructor... does it work if you omit the
last two arguments for the delegated ctor call here?

[Owen Lin, 2016-03-02 02:07:00]

Patchset #15 (id:400001) has been deleted

[Owen Lin, 2016-03-02 02:50:53]

https://codereview.chromium.org/1541353002/diff/380001/content/common/gpu/gpu...
File content/common/gpu/gpu_messages.cc (right):

https://codereview.chromium.org/1541353002/diff/380001/content/common/gpu/gpu...
content/common/gpu/gpu_messages.cc:19: WriteParam(m,
static_cast<int64_t>(p.offset()));
On 2016/03/01 01:48:43, dcheng wrote:
> Does it make sense to DCHECK() that this is non-negative?
I agree with Pawel that
"MapAt() will fail us if the offset is invalid, e.g. if we pass a negative value
for some reason. So I don't think we have to check for <0 explicitly."

https://codereview.chromium.org/1541353002/diff/380001/content/common/gpu/gpu...
content/common/gpu/gpu_messages.cc:49: if (!checked_offset.IsValid()) {
On 2016/03/01 01:48:43, dcheng wrote:
> Similarly, does it make sense to assert that the value is non-negative?

See above.

https://codereview.chromium.org/1541353002/diff/380001/content/common/gpu/med...
File content/common/gpu/media/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1541353002/diff/380001/content/common/gpu/med...
content/common/gpu/media/android_video_decode_accelerator.cc:388: shm.reset(new
SharedMemoryRegion(base::SharedMemoryHandle(), 0, 0, false));
On 2016/03/01 01:48:43, dcheng wrote:
> Maybe just shm.reset() and null check when initializing |memory| below? Then I
> think you won't need the overloaded constructor.
> 
> const uint8_t* memory = shm ? static_cast<const uint8_t*>(shm->memory()) :
> nullptr;

Thanks.

https://codereview.chromium.org/1541353002/diff/380001/content/common/gpu/med...
File content/common/gpu/media/shared_memory_region.h (right):

https://codereview.chromium.org/1541353002/diff/380001/content/common/gpu/med...
content/common/gpu/media/shared_memory_region.h:13: // Helper class to access a
region of a SharedMemory.
On 2016/03/01 01:48:43, dcheng wrote:
> This class needs a better comment: specifically, base::SharedMemory requires
> that offset be aligned, so this class wraps it with some pointer arithmetic to
> hide that fact.

Done.

https://codereview.chromium.org/1541353002/diff/380001/content/common/gpu/med...
File content/common/gpu/media/v4l2_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1541353002/diff/380001/content/common/gpu/med...
content/common/gpu/media/v4l2_video_decode_accelerator.cc:68:
SharedMemoryRegion* shm,
On 2016/03/01 01:48:43, dcheng wrote:
> scoped_ptr<SharedMemoryRegion> to have the compiler document that ownership is
> transferred (similar for the other BitstreamBufferRef helpers)

Done.

https://codereview.chromium.org/1541353002/diff/380001/media/base/bitstream_b...
File media/base/bitstream_buffer.cc (right):

https://codereview.chromium.org/1541353002/diff/380001/media/base/bitstream_b...
media/base/bitstream_buffer.cc:10: : BitstreamBuffer(-1,
base::SharedMemoryHandle(), 0, 0, kNoTimestamp()) {}
On 2016/03/01 01:48:43, dcheng wrote:
> Since you have default args on the constructor... does it work if you omit the
> last two arguments for the delegated ctor call here?

Done.

[dcheng, 2016-03-02 05:58:20]

https://codereview.chromium.org/1541353002/diff/380001/content/common/gpu/gpu...
File content/common/gpu/gpu_messages.cc (right):

https://codereview.chromium.org/1541353002/diff/380001/content/common/gpu/gpu...
content/common/gpu/gpu_messages.cc:19: WriteParam(m,
static_cast<int64_t>(p.offset()));
On 2016/03/02 at 02:50:52, Owen Lin wrote:
> On 2016/03/01 01:48:43, dcheng wrote:
> > Does it make sense to DCHECK() that this is non-negative?
> I agree with Pawel that
> "MapAt() will fail us if the offset is invalid, e.g. if we pass a negative
value for some reason. So I don't think we have to check for <0 explicitly."

It's not in the contract of SharedMemory::MapAt(), and it's not actually obvious
that it will fail. In fact, here's the Windows implementation:

bool SharedMemory::MapAt(off_t offset, size_t bytes) {
  ...
  memory_ = MapViewOfFile(
      mapped_file_, read_only_ ? FILE_MAP_READ : FILE_MAP_READ | FILE_MAP_WRITE,
      static_cast<uint64_t>(offset) >> 32, static_cast<DWORD>(offset), bytes);
  ...
}

Will this fail or succeed with off_t = -1? We cast it to unsigned before we
shift, so we won't get sign extension...

[Owen Lin, 2016-03-02 06:32:12]

On 2016/03/02 05:58:20, dcheng wrote:
>
https://codereview.chromium.org/1541353002/diff/380001/content/common/gpu/gpu...
> File content/common/gpu/gpu_messages.cc (right):
> 
>
https://codereview.chromium.org/1541353002/diff/380001/content/common/gpu/gpu...
> content/common/gpu/gpu_messages.cc:19: WriteParam(m,
> static_cast<int64_t>(p.offset()));
> On 2016/03/02 at 02:50:52, Owen Lin wrote:
> > On 2016/03/01 01:48:43, dcheng wrote:
> > > Does it make sense to DCHECK() that this is non-negative?
> > I agree with Pawel that
> > "MapAt() will fail us if the offset is invalid, e.g. if we pass a negative
> value for some reason. So I don't think we have to check for <0 explicitly."
> 
> It's not in the contract of SharedMemory::MapAt(), and it's not actually
obvious
> that it will fail. In fact, here's the Windows implementation:
> 
> bool SharedMemory::MapAt(off_t offset, size_t bytes) {
>   ...
>   memory_ = MapViewOfFile(
>       mapped_file_, read_only_ ? FILE_MAP_READ : FILE_MAP_READ |
FILE_MAP_WRITE,
>       static_cast<uint64_t>(offset) >> 32, static_cast<DWORD>(offset), bytes);
>   ...
> }
> 
> Will this fail or succeed with off_t = -1? We cast it to unsigned before we
> shift, so we won't get sign extension...

I see your point. But here is the document of SharedMemory::Map()

// Maps the shared memory into the caller's address space.
// Returns true on success, false otherwise.  The memory address
// is accessed via the memory() accessor.  The mapped address is guaranteed to
// have an alignment of at least MAP_MINIMUM_ALIGNMENT. This method will fail
// if this object is currently mapped.

So the question is: Should MapAt(-1) success on windows? If not, it should
return false.
If you're OK with that, I can make a separate CL to address this issue on
Windows.

[dcheng, 2016-03-02 07:44:33]

On 2016/03/02 at 06:32:12, owenlin wrote:
> On 2016/03/02 05:58:20, dcheng wrote:
> >
https://codereview.chromium.org/1541353002/diff/380001/content/common/gpu/gpu...
> > File content/common/gpu/gpu_messages.cc (right):
> > 
> >
https://codereview.chromium.org/1541353002/diff/380001/content/common/gpu/gpu...
> > content/common/gpu/gpu_messages.cc:19: WriteParam(m,
> > static_cast<int64_t>(p.offset()));
> > On 2016/03/02 at 02:50:52, Owen Lin wrote:
> > > On 2016/03/01 01:48:43, dcheng wrote:
> > > > Does it make sense to DCHECK() that this is non-negative?
> > > I agree with Pawel that
> > > "MapAt() will fail us if the offset is invalid, e.g. if we pass a negative
> > value for some reason. So I don't think we have to check for <0 explicitly."
> > 
> > It's not in the contract of SharedMemory::MapAt(), and it's not actually
obvious
> > that it will fail. In fact, here's the Windows implementation:
> > 
> > bool SharedMemory::MapAt(off_t offset, size_t bytes) {
> >   ...
> >   memory_ = MapViewOfFile(
> >       mapped_file_, read_only_ ? FILE_MAP_READ : FILE_MAP_READ |
FILE_MAP_WRITE,
> >       static_cast<uint64_t>(offset) >> 32, static_cast<DWORD>(offset),
bytes);
> >   ...
> > }
> > 
> > Will this fail or succeed with off_t = -1? We cast it to unsigned before we
> > shift, so we won't get sign extension...
> 
> I see your point. But here is the document of SharedMemory::Map()


At the end of the day, it just doesn't make sense for these to be signed. Just
send serialize it as unsigned for the IPC, with a DCHECK() that we don't
static_cast a negative value in Read().

> 
> // Maps the shared memory into the caller's address space.
> // Returns true on success, false otherwise.  The memory address
> // is accessed via the memory() accessor.  The mapped address is guaranteed to
> // have an alignment of at least MAP_MINIMUM_ALIGNMENT. This method will fail
> // if this object is currently mapped.
> 
> So the question is: Should MapAt(-1) success on windows? If not, it should
return false.
> If you're OK with that, I can make a separate CL to address this issue on
Windows.

MapAt()'s offset argument should just be unsigned.

[Owen Lin, 2016-03-02 09:15:57]

Patchset #17 (id:460001) has been deleted

[Owen Lin, 2016-03-02 09:23:46]

On 2016/03/02 07:44:33, dcheng wrote:
> On 2016/03/02 at 06:32:12, owenlin wrote:
> > On 2016/03/02 05:58:20, dcheng wrote:
> > >
>
https://codereview.chromium.org/1541353002/diff/380001/content/common/gpu/gpu...
> > > File content/common/gpu/gpu_messages.cc (right):
> > > 
> > >
>
https://codereview.chromium.org/1541353002/diff/380001/content/common/gpu/gpu...
> > > content/common/gpu/gpu_messages.cc:19: WriteParam(m,
> > > static_cast<int64_t>(p.offset()));
> > > On 2016/03/02 at 02:50:52, Owen Lin wrote:
> > > > On 2016/03/01 01:48:43, dcheng wrote:
> > > > > Does it make sense to DCHECK() that this is non-negative?
> > > > I agree with Pawel that
> > > > "MapAt() will fail us if the offset is invalid, e.g. if we pass a
negative
> > > value for some reason. So I don't think we have to check for <0
explicitly."
> > > 
> > > It's not in the contract of SharedMemory::MapAt(), and it's not actually
> obvious
> > > that it will fail. In fact, here's the Windows implementation:
> > > 
> > > bool SharedMemory::MapAt(off_t offset, size_t bytes) {
> > >   ...
> > >   memory_ = MapViewOfFile(
> > >       mapped_file_, read_only_ ? FILE_MAP_READ : FILE_MAP_READ |
> FILE_MAP_WRITE,
> > >       static_cast<uint64_t>(offset) >> 32, static_cast<DWORD>(offset),
> bytes);
> > >   ...
> > > }
> > > 
> > > Will this fail or succeed with off_t = -1? We cast it to unsigned before
we
> > > shift, so we won't get sign extension...
> > 
> > I see your point. But here is the document of SharedMemory::Map()
> 
> 
> At the end of the day, it just doesn't make sense for these to be signed. Just
> send serialize it as unsigned for the IPC, with a DCHECK() that we don't
> static_cast a negative value in Read().

If you insist. But we shall not DCHECK in Read(). We don't want to crash the GPU
process when a renderer process sends invalid arguments. 

> 
> > 
> > // Maps the shared memory into the caller's address space.
> > // Returns true on success, false otherwise.  The memory address
> > // is accessed via the memory() accessor.  The mapped address is guaranteed
to
> > // have an alignment of at least MAP_MINIMUM_ALIGNMENT. This method will
fail
> > // if this object is currently mapped.
> > 
> > So the question is: Should MapAt(-1) success on windows? If not, it should
> return false.
> > If you're OK with that, I can make a separate CL to address this issue on
> Windows.
> 
> MapAt()'s offset argument should just be unsigned.

Maybe. That's the point. The function is added in the CL:
https://chromiumcodereview.appspot.com/11876037
But I can see a good reason why it chose off_t:
http://man7.org/linux/man-pages/man2/mmap.2.html

The fact is the type of |offset| of MapAt() is now signed. The function could be
called with or without IPC. It needs to handle the case of MapAt(-1) correctly,
i.e., return false. Maybe |offset| should just be unsigned. But in both cases,
we need not (and should not) validate |offset| in ParamTraits.

[Owen Lin, 2016-03-04 07:11:50]

Hi Daniel, would you like to take another look. Thanks.

[dcheng, 2016-03-05 01:30:57]

ipc lgtm with comment addressed

https://codereview.chromium.org/1541353002/diff/480001/content/common/gpu/med...
File content/common/gpu/media_messages.cc (right):

https://codereview.chromium.org/1541353002/diff/480001/content/common/gpu/med...
content/common/gpu/media_messages.cc:20: WriteParam(m,
static_cast<int64_t>(p.offset()));
static_cast<uint64_t> (and corresponding change on the other side)

[Owen Lin, 2016-03-08 03:02:51]

The CQ bit was checked by owenlin@chromium.org

[Owen Lin, 2016-03-08 03:02:51]

The patchset sent to the CQ was uploaded after l-g-t-m from kcwu@chromium.org,
posciak@chromium.org, ccameron@chromium.org, xhwang@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1541353002/#ps500001
(title: "address review comments and rebase")

[commit-bot: I haz the power, 2016-03-08 03:03:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1541353002/500001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1541353002/500001

[commit-bot: I haz the power, 2016-03-08 04:53:19]

Description was changed from

==========
Add offset support to BitstreamBuffer.

The actual data in the shared memory may start at some offset from the start of
shared memory.

TEST=Run vda_unittest and play YT on peach-pit.
BUG=b/25829285
==========

to

==========
Add offset support to BitstreamBuffer.

The actual data in the shared memory may start at some offset from the start of
shared memory.

TEST=Run vda_unittest and play YT on peach-pit.
BUG=b/25829285
==========

[commit-bot: I haz the power, 2016-03-08 04:53:21]

Committed patchset #18 (id:500001)

[commit-bot: I haz the power, 2016-03-08 04:54:45]

Description was changed from

==========
Add offset support to BitstreamBuffer.

The actual data in the shared memory may start at some offset from the start of
shared memory.

TEST=Run vda_unittest and play YT on peach-pit.
BUG=b/25829285
==========

to

==========
Add offset support to BitstreamBuffer.

The actual data in the shared memory may start at some offset from the start of
shared memory.

TEST=Run vda_unittest and play YT on peach-pit.
BUG=b/25829285

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

[commit-bot: I haz the power, 2016-03-08 04:54:47]

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

[kcwu, 2016-03-16 06:20:53]

Ah, just found I have some comments forgot to publish.

https://codereview.chromium.org/1541353002/diff/380001/content/common/gpu/med...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1541353002/diff/380001/content/common/gpu/med...
content/common/gpu/media/gpu_video_decode_accelerator.cc:492: void
GpuVideoDecodeAccelerator::CallOrPostNotifyError(
unused?

https://codereview.chromium.org/1541353002/diff/380001/content/common/gpu/med...
File content/common/gpu/media/shared_memory_region.cc (right):

https://codereview.chromium.org/1541353002/diff/380001/content/common/gpu/med...
content/common/gpu/media/shared_memory_region.cc:1: // Copyright (c) 2015 The
Chromium Authors. All rights reserved.
s/(c) 2015/2016/

https://codereview.chromium.org/1541353002/diff/380001/content/common/gpu/med...
content/common/gpu/media/shared_memory_region.cc:17: alignment_size_(offset %
base::SysInfo::VMAllocationGranularity()) {
How about name it |offset_to_alignment| or |padding_size|? My first impression
for "size" of |alignment_size_| is something like 4096, so I feel it a little
confusing.

https://codereview.chromium.org/1541353002/diff/380001/content/common/gpu/med...
File content/common/gpu/media/shared_memory_region.h (right):

https://codereview.chromium.org/1541353002/diff/380001/content/common/gpu/med...
content/common/gpu/media/shared_memory_region.h:1: // Copyright (c) 2015 The
Chromium Authors. All rights reserved.
s/(c) 2015/2016/