Remove media::VideoFrame from media::VideoCaptureDevice::Client interface

Remove media::VideoFrame from media::VideoCaptureDevice::Client interface

The VideoCaptureDevice::Client interface will have to return buffers that are not
wrapped in media::VideoFrame objects, for upcoming compatibility with capturing
to texture mailboxes.  Implement this by making the ReserveForProducer() entry
point return a new Client::Buffer object that caches the memory and size of the
reservation.  The lifetime of the reservation is managed by the lifetime of the
Buffer object, not by the media::VideoFrame that we previously returned.

Pursuant to this, VideoCaptureBufferPool is changed to remove entry points that
return pre-constructed media::VideoFrame objects.  The responsibility for
managing the rotation/letterboxing state of buffers is also moved out of
VideoCaptureBufferPool into its user, VideoCaptureController.

Also: media::VideoFrame::WrapExternalSharedMemory() is renamed to
media::VideoFrame::WrapExternalPackedMemory(), as not all users will expect to
wrap shared memory.  media::VideoFrame::PlaneAllocationSize() is added to
facilitate querying the planar size of just one plane of a packed allocation.

BUG=269312
TEST=local build, run apprtc on CrOS snow, unittests and apprtc on desktop Linux

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=236279

[sheu, 2013-10-29 00:22:22]

I'm gonna add on wjia@ when we know this is the direction we want to go in.

nick@: added you on CC.  This is what we discussed a few weeks ago w.r.t. buffer
reservations.
jiayl@: I'm told that you're now the right person to bug about this stack. 
PTAL.

[ncarter (slow), 2013-10-29 18:42:16]

From a high level this looks like the right approach.

The two points I'm not sure of have to do with the Buffer type. In particular,
do we really need to use inheritance in the VCD::Client's Buffer? And secondly,
does the Buffer abstraction belong in VideoCaptureBufferPool instead of its
clients? The fact that VCBP unittest and VCC both kind of roll the same kind of
Buffer class is a code smell to me. I guess the advantage to having an abstract
buffer type at the VCD::Client layer is in the ability to test that interface
without a real VCBP ... but do the tests actually do this?

The answer here may come down to a question of which is a greater burden on the
tests: is it easier to mock out VCD::Client::Buffer, than to use a real VCBP to
provide real Buffers?

https://codereview.chromium.org/48113011/diff/70001/content/browser/renderer_...
File content/browser/renderer_host/media/video_capture_buffer_pool_unittest.cc
(right):

https://codereview.chromium.org/48113011/diff/70001/content/browser/renderer_...
content/browser/renderer_host/media/video_capture_buffer_pool_unittest.cc:21:
class Buffer {
I almost wonder if this class ought to be the return type from
ReserveForProducer ...

https://codereview.chromium.org/48113011/diff/70001/content/browser/renderer_...
content/browser/renderer_host/media/video_capture_buffer_pool_unittest.cc:49: //
To verify that ReserveI420VideoFrame always sets |buffer_id_to_drop|,
s/VideoFrame/Buffer/

https://codereview.chromium.org/48113011/diff/70001/content/browser/renderer_...
File content/browser/renderer_host/media/web_contents_video_capture_device.cc
(right):

https://codereview.chromium.org/48113011/diff/70001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_video_capture_device.cc:476:
base::Passed(&output_buffer),
I think this ownership scheme will work today, but it's also possibly fragile,
because we're relying on a fairly indirect invariant: |callback| must outlive
all uses of the memory in |storage|. I don't think that's violated today, but I
wonder if it might be safer to do something like this:
 - scoped_ptr<output_buffer> is passed into a cleanup closure of the VideoFrame.
 - the delivery callback (DidCaptureFrame) captures a reference to the
VideoFrame object (guaranteeing its lifetime), and also takes a
base::Unretained() output_buffer?

https://codereview.chromium.org/48113011/diff/70001/media/video/capture/video...
File media/video/capture/video_capture_device.h (right):

https://codereview.chromium.org/48113011/diff/70001/media/video/capture/video...
media/video/capture/video_capture_device.h:126: Buffer(void* data, size_t size)
: data_(data), size_(size) {}
Do you expect Buffer to be created directly? If this type is designed for
inheritance, should the ctor be protected?

https://codereview.chromium.org/48113011/diff/70001/media/video/capture/video...
media/video/capture/video_capture_device.h:169: // Captured a new video frame,
in a raw buffer backed by |buffer|.
"in a raw buffer backed by |buffer|" -> "held in |buffer|" or some such. ('raw'
not seeming to provide much distinguishing power here)

https://codereview.chromium.org/48113011/diff/70001/media/video/capture/video...
File media/video/capture/video_capture_device_unittest.cc (right):

https://codereview.chromium.org/48113011/diff/70001/media/video/capture/video...
media/video/capture/video_capture_device_unittest.cc:94: virtual void
OnIncomingCapturedI420Buffer(scoped_ptr<Buffer> buffer,
This method doesn't seem to match anything in VideoCaptureDevice::Client
interface. Does this compile?

[sheu, 2013-11-05 20:02:09]

Updated, PTAL.

I had to refcount stuff :-(

https://chromiumcodereview.appspot.com/48113011/diff/70001/content/browser/re...
File content/browser/renderer_host/media/video_capture_buffer_pool_unittest.cc
(right):

https://chromiumcodereview.appspot.com/48113011/diff/70001/content/browser/re...
content/browser/renderer_host/media/video_capture_buffer_pool_unittest.cc:21:
class Buffer {
On 2013/10/29 18:42:17, ncarter wrote:
> I almost wonder if this class ought to be the return type from
> ReserveForProducer ...

I thought about it, and I think the answer is "no".

The idea I think would be to return the
media::VideoCaptureDevice::Client::Buffer type (the return type of the
VCD::ReserveOutputBuffer call) directly from the VCBP.  Unfortunately, the
VideoCaptureHost operates on buffer indices (and the VideoCaptureController is
aware of this), so for that purpose, the Buffer type returned from the VCBP
would have to have a visible buffer_id as part of its interface.  But the
buffer_id is really internal to VCC, and it makes no sense to leak it
externally.

Having the buffer_id be a part of the interface would incidentally have the nice
property of making RecognizeBuffer a lot simpler.

https://chromiumcodereview.appspot.com/48113011/diff/70001/content/browser/re...
content/browser/renderer_host/media/video_capture_buffer_pool_unittest.cc:49: //
To verify that ReserveI420VideoFrame always sets |buffer_id_to_drop|,
On 2013/10/29 18:42:17, ncarter wrote:
> s/VideoFrame/Buffer/

Done.

https://chromiumcodereview.appspot.com/48113011/diff/70001/content/browser/re...
File content/browser/renderer_host/media/web_contents_video_capture_device.cc
(right):

https://chromiumcodereview.appspot.com/48113011/diff/70001/content/browser/re...
content/browser/renderer_host/media/web_contents_video_capture_device.cc:476:
base::Passed(&output_buffer),
On 2013/10/29 18:42:17, ncarter wrote:
> I think this ownership scheme will work today, but it's also possibly fragile,
> because we're relying on a fairly indirect invariant: |callback| must outlive
> all uses of the memory in |storage|. I don't think that's violated today, but
I
> wonder if it might be safer to do something like this:
>  - scoped_ptr<output_buffer> is passed into a cleanup closure of the
VideoFrame.
>  - the delivery callback (DidCaptureFrame) captures a reference to the
> VideoFrame object (guaranteeing its lifetime), and also takes a
> base::Unretained() output_buffer?

That actually blew this whole thing wide open.  Long story short, I've made the
Buffer refcounted.  :-(

Problem is, the external user of the Buffer (in our case here, a
media::VideoFrame) expects to hold onto the Buffer past when it returns the
buffer back to the VideoCaptureController::Client.  So it needs ownership.

At the same time, passing it back to the VCC::Client, that also expects to be
able to maintain lifetime for the Buffer object while it's passing the Buffer
around to send to the renderer process.  So it needs ownership too.

https://chromiumcodereview.appspot.com/48113011/diff/70001/media/video/captur...
File media/video/capture/video_capture_device.h (right):

https://chromiumcodereview.appspot.com/48113011/diff/70001/media/video/captur...
media/video/capture/video_capture_device.h:126: Buffer(void* data, size_t size)
: data_(data), size_(size) {}
On 2013/10/29 18:42:17, ncarter wrote:
> Do you expect Buffer to be created directly? If this type is designed for
> inheritance, should the ctor be protected?

Done.

https://chromiumcodereview.appspot.com/48113011/diff/70001/media/video/captur...
media/video/capture/video_capture_device.h:169: // Captured a new video frame,
in a raw buffer backed by |buffer|.
On 2013/10/29 18:42:17, ncarter wrote:
> "in a raw buffer backed by |buffer|" -> "held in |buffer|" or some such.
('raw'
> not seeming to provide much distinguishing power here)

Done.

https://chromiumcodereview.appspot.com/48113011/diff/70001/media/video/captur...
File media/video/capture/video_capture_device_unittest.cc (right):

https://chromiumcodereview.appspot.com/48113011/diff/70001/media/video/captur...
media/video/capture/video_capture_device_unittest.cc:94: virtual void
OnIncomingCapturedI420Buffer(scoped_ptr<Buffer> buffer,
On 2013/10/29 18:42:17, ncarter wrote:
> This method doesn't seem to match anything in VideoCaptureDevice::Client
> interface. Does this compile?

Apparently not :-P

I didn't realize I had to build/run media_unittest as well.  All I'd been
running were browser_tests and content_unittest.

[ncarter (slow), 2013-11-07 02:25:28]

Early comment, not quite done reviewing yet ...

https://codereview.chromium.org/48113011/diff/170001/content/browser/renderer...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://codereview.chromium.org/48113011/diff/170001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_controller.cc:505: buffer_id =
VideoCaptureBufferPool::kInvalidId;
If you want to protect against misuse of buffer_id, it might be better to just
use scope for it (e.g. a helper function that does the wrapping) ... [continued]

https://codereview.chromium.org/48113011/diff/170001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_controller.cc:518:
rotated_buffers_.erase(buffer_id);
... [continued] to avoid the bugs like this (buffer_id is always kInvalidId
here).

https://codereview.chromium.org/48113011/diff/170001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_controller.cc:520: if
(!rotated_buffers_.count(buffer_id)) {
You can make this an .insert(); the return value will tell you whether or not it
was already present.

[sheu, 2013-11-08 19:26:32]

jiayl@, nick@: PTAL.  I've got a rebase on this on top of the latest change from
jiayl@, but I'll hold off on the upload if y'all still want to comment on the
current revision.

https://chromiumcodereview.appspot.com/48113011/diff/170001/content/browser/r...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://chromiumcodereview.appspot.com/48113011/diff/170001/content/browser/r...
content/browser/renderer_host/media/video_capture_controller.cc:505: buffer_id =
VideoCaptureBufferPool::kInvalidId;
On 2013/11/07 02:25:28, ncarter wrote:
> If you want to protect against misuse of buffer_id, it might be better to just
> use scope for it (e.g. a helper function that does the wrapping) ...
[continued]

I think I just went a little overboard with the ghetto scoping.  Removed.

https://chromiumcodereview.appspot.com/48113011/diff/170001/content/browser/r...
content/browser/renderer_host/media/video_capture_controller.cc:518:
rotated_buffers_.erase(buffer_id);
On 2013/11/07 02:25:28, ncarter wrote:
> ... [continued] to avoid the bugs like this (buffer_id is always kInvalidId
> here).

/le sigh

Done.

https://chromiumcodereview.appspot.com/48113011/diff/170001/content/browser/r...
content/browser/renderer_host/media/video_capture_controller.cc:520: if
(!rotated_buffers_.count(buffer_id)) {
On 2013/11/07 02:25:28, ncarter wrote:
> You can make this an .insert(); the return value will tell you whether or not
it
> was already present.

Done.

[jiayl, 2013-11-08 22:16:45]

https://chromiumcodereview.appspot.com/48113011/diff/170001/content/browser/r...
File content/browser/renderer_host/media/video_capture_buffer_pool.cc (right):

https://chromiumcodereview.appspot.com/48113011/diff/170001/content/browser/r...
content/browser/renderer_host/media/video_capture_buffer_pool.cc:134: // largest
one that's not big enough, in case we have to reallocate a buffer.
Why is the largest one preferred for reallocation? It seems to me reallocating
the smallest one could potentially minimize reallocations in the future.

https://chromiumcodereview.appspot.com/48113011/diff/170001/content/browser/r...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://chromiumcodereview.appspot.com/48113011/diff/170001/content/browser/r...
content/browser/renderer_host/media/video_capture_controller.cc:136:
std::set<int> rotated_buffers_;
I don't find where this is used. Plus don't we need to store the degree of
rotation?

https://chromiumcodereview.appspot.com/48113011/diff/170001/content/browser/r...
content/browser/renderer_host/media/video_capture_controller.cc:517: if
((rotation % 180) == 0) {
why 180 not 360? Could you add comments to explain?

https://chromiumcodereview.appspot.com/48113011/diff/170001/content/browser/r...
content/browser/renderer_host/media/video_capture_controller.cc:521: //
TODO(jiayl): Generalize the |rotation| mechanism.
What does it mean?

[sheu, 2013-11-09 00:59:29]

Comments addressed.  I'll upload a version rebased against jiayl@'s CL once that
lands.

PTAL: jiayl@, ncarter@.

https://chromiumcodereview.appspot.com/48113011/diff/170001/content/browser/r...
File content/browser/renderer_host/media/video_capture_buffer_pool.cc (right):

https://chromiumcodereview.appspot.com/48113011/diff/170001/content/browser/r...
content/browser/renderer_host/media/video_capture_buffer_pool.cc:134: // largest
one that's not big enough, in case we have to reallocate a buffer.
On 2013/11/08 22:16:46, jiayl wrote:
> Why is the largest one preferred for reallocation? It seems to me reallocating
> the smallest one could potentially minimize reallocations in the future.

I'd like reallocations of large buffer first so we can reduce memory footprint,
as soon as possible after switching to a smaller buffer size.

https://chromiumcodereview.appspot.com/48113011/diff/170001/content/browser/r...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://chromiumcodereview.appspot.com/48113011/diff/170001/content/browser/r...
content/browser/renderer_host/media/video_capture_controller.cc:136:
std::set<int> rotated_buffers_;
On 2013/11/08 22:16:46, jiayl wrote:
> I don't find where this is used. Plus don't we need to store the degree of
> rotation?

It's used in DoReserveOutputBuffers.  See the comment there.

https://chromiumcodereview.appspot.com/48113011/diff/170001/content/browser/r...
content/browser/renderer_host/media/video_capture_controller.cc:517: if
((rotation % 180) == 0) {
On 2013/11/08 22:16:46, jiayl wrote:
> why 180 not 360? Could you add comments to explain?

Updated comment.  Basically: a 180 degree rotation would not require
letterboxing (since the aspect ratio remains the same), and no clearing is
required.

https://chromiumcodereview.appspot.com/48113011/diff/170001/content/browser/r...
content/browser/renderer_host/media/video_capture_controller.cc:521: //
TODO(jiayl): Generalize the |rotation| mechanism.
On 2013/11/08 22:16:46, jiayl wrote:
> What does it mean?

This block is copied out from the VideoCaptureBufferPool, so it means what it
meant in that context.  I didn't add it :-)

Do you have the context for this comment in the old location?

[ncarter (slow), 2013-11-09 01:10:34]

https://chromiumcodereview.appspot.com/48113011/diff/170001/content/browser/r...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://chromiumcodereview.appspot.com/48113011/diff/170001/content/browser/r...
content/browser/renderer_host/media/video_capture_controller.cc:521: //
TODO(jiayl): Generalize the |rotation| mechanism.
On 2013/11/09 00:59:30, sheu wrote:
> On 2013/11/08 22:16:46, jiayl wrote:
> > What does it mean?
> 
> This block is copied out from the VideoCaptureBufferPool, so it means what it
> meant in that context.  I didn't add it :-)
> 
> Do you have the context for this comment in the old location?

Just remove the TODO. You've mostly accomplished it here, though the context was
really about moving this same rotation logic up into the capture device impl
itself.

[ncarter (slow), 2013-11-09 01:26:32]

Only one comment now; I'm good with this otherwise. Let me know when the merged
patch is up, as I expect there will be some conflicts, and I'd be interested in
doing a review for the sake of correctness.

https://chromiumcodereview.appspot.com/48113011/diff/170001/content/browser/r...
File content/browser/renderer_host/media/video_capture_buffer_pool.cc (right):

https://chromiumcodereview.appspot.com/48113011/diff/170001/content/browser/r...
content/browser/renderer_host/media/video_capture_buffer_pool.cc:129: //
RecognizeReservedBuffer() can have some pointer to recognize.
Is there an alternative to creating a dummy shared memory allocation? You're not
allocating a byte here, you're almost certainly allocating full 4KB page (as
this is shareable memory). And having the pointer around seems like it has the
potential to create some confusion.

Now that we're handing out int buffer_ids, it seems like we don't need
RecognizeReservedBuffer at all?

[sheu, 2013-11-11 22:37:43]

Updated and rebased.  PTAL

https://chromiumcodereview.appspot.com/48113011/diff/170001/content/browser/r...
File content/browser/renderer_host/media/video_capture_buffer_pool.cc (right):

https://chromiumcodereview.appspot.com/48113011/diff/170001/content/browser/r...
content/browser/renderer_host/media/video_capture_buffer_pool.cc:129: //
RecognizeReservedBuffer() can have some pointer to recognize.
On 2013/11/09 01:26:38, ncarter wrote:
> Is there an alternative to creating a dummy shared memory allocation? You're
not
> allocating a byte here, you're almost certainly allocating full 4KB page (as
> this is shareable memory). And having the pointer around seems like it has the
> potential to create some confusion.
> 
> Now that we're handing out int buffer_ids, it seems like we don't need
> RecognizeReservedBuffer at all?

I was hesitant about exporting buffer_id, but I've decided to go ahead and do
that.

RecongizeReservedBuffer removed.

https://chromiumcodereview.appspot.com/48113011/diff/170001/content/browser/r...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://chromiumcodereview.appspot.com/48113011/diff/170001/content/browser/r...
content/browser/renderer_host/media/video_capture_controller.cc:521: //
TODO(jiayl): Generalize the |rotation| mechanism.
On 2013/11/09 01:10:34, ncarter wrote:
> On 2013/11/09 00:59:30, sheu wrote:
> > On 2013/11/08 22:16:46, jiayl wrote:
> > > What does it mean?
> > 
> > This block is copied out from the VideoCaptureBufferPool, so it means what
it
> > meant in that context.  I didn't add it :-)
> > 
> > Do you have the context for this comment in the old location?
> 
> Just remove the TODO. You've mostly accomplished it here, though the context
was
> really about moving this same rotation logic up into the capture device impl
> itself.

Done.

[jiayl, 2013-11-12 19:23:48]

LGTM. You will need a media owner's approval, e.g. fischman

[ncarter (slow), 2013-11-12 21:14:21]

LGTM. Really nice work.

https://codereview.chromium.org/48113011/diff/360001/content/browser/renderer...
File content/browser/renderer_host/media/video_capture_buffer_pool.cc (right):

https://codereview.chromium.org/48113011/diff/360001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_buffer_pool.cc:54: }
Should probably add a DCHECK here that the buffer is in one of the reserved
states, as it would indicate a programming mistake if this function were called
on a buffer were free to be reserved.

[sheu, 2013-11-18 22:42:10]

wjia@: PTAL at... pretty much everything.  Thanks!

https://codereview.chromium.org/48113011/diff/360001/content/browser/renderer...
File content/browser/renderer_host/media/video_capture_buffer_pool.cc (right):

https://codereview.chromium.org/48113011/diff/360001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_buffer_pool.cc:54: }
On 2013/11/12 21:14:22, ncarter wrote:
> Should probably add a DCHECK here that the buffer is in one of the reserved
> states, as it would indicate a programming mistake if this function were
called
> on a buffer were free to be reserved.

Done.

[wjia(left Chromium), 2013-11-19 01:47:35]

Nice!

https://codereview.chromium.org/48113011/diff/470001/content/browser/renderer...
File content/browser/renderer_host/media/video_capture_buffer_pool.cc (right):

https://codereview.chromium.org/48113011/diff/470001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_buffer_pool.cc:122: for
(BufferMap::iterator it = buffers_.begin(); it != buffers_.end(); it++) {
nit: ++it

https://codereview.chromium.org/48113011/diff/470001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_buffer_pool.cc:132: realloc =
it;
OOC, what's the reason to reallocate the largest one? 

It seems VideoCaptureBufferPool doesn't downsize buffer when the requested size
is smaller than the existing buffer size. Any concern about holding unnecessary
memory?

https://codereview.chromium.org/48113011/diff/470001/content/browser/renderer...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://codereview.chromium.org/48113011/diff/470001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_controller.cc:37: :
Buffer(buffer_id, data, size), pool_(pool) {}
The |pool| shouldn't be NULL pointer here. It's used in dtor without checking.
Add a ChECK here.

https://codereview.chromium.org/48113011/diff/470001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_controller.cc:506:
rotated_buffers_.insert(buffer_id);
nit: no need to call insert again.

[sheu, 2013-11-19 20:28:16]

Updated.  PTAL

fischman@: PTAL at media/base/*.

https://codereview.chromium.org/48113011/diff/470001/content/browser/renderer...
File content/browser/renderer_host/media/video_capture_buffer_pool.cc (right):

https://codereview.chromium.org/48113011/diff/470001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_buffer_pool.cc:122: for
(BufferMap::iterator it = buffers_.begin(); it != buffers_.end(); it++) {
On 2013/11/19 01:47:35, wjia wrote:
> nit: ++it

Done.

https://codereview.chromium.org/48113011/diff/470001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_buffer_pool.cc:132: realloc =
it;
On 2013/11/19 01:47:35, wjia wrote:
> OOC, what's the reason to reallocate the largest one? 
> 
> It seems VideoCaptureBufferPool doesn't downsize buffer when the requested
size
> is smaller than the existing buffer size. Any concern about holding
unnecessary
> memory?

VideoCaptureBufferPool doesn't downsize until it has run out of buffers.  At
that point I think it makes sense to downsize, starting with the largest one so
we get the biggest reduction in memory usage.

https://codereview.chromium.org/48113011/diff/470001/content/browser/renderer...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://codereview.chromium.org/48113011/diff/470001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_controller.cc:37: :
Buffer(buffer_id, data, size), pool_(pool) {}
On 2013/11/19 01:47:35, wjia wrote:
> The |pool| shouldn't be NULL pointer here. It's used in dtor without checking.
> Add a ChECK here.

Done.

https://codereview.chromium.org/48113011/diff/470001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_controller.cc:506:
rotated_buffers_.insert(buffer_id);
On 2013/11/19 01:47:35, wjia wrote:
> nit: no need to call insert again.

Done.

[sheu, 2013-11-19 20:29:03]

fischman@: PTAL at media/base/*

[Ami GONE FROM CHROMIUM, 2013-11-19 21:01:51]

LGTM % nits for media/base.

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

https://codereview.chromium.org/48113011/diff/560001/media/base/video_frame.c...
media/base/video_frame.cc:272: RoundUp(coded_size.width(), 2) *
RoundUp(coded_size.height(), 2);
Any reason for this not to be outside the outer switch?

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

https://codereview.chromium.org/48113011/diff/560001/media/base/video_frame.h...
media/base/video_frame.h:201: size_t plane,
size_t is a weird type to use for this.  Please name the k[ARGB]Plane enum type
at the top of this file and use that type here.

[sheu, 2013-11-19 21:30:26]

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

https://codereview.chromium.org/48113011/diff/560001/media/base/video_frame.h...
media/base/video_frame.h:201: size_t plane,
On 2013/11/19 21:01:52, Ami Fischman wrote:
> size_t is a weird type to use for this.  Please name the k[ARGB]Plane enum
type
> at the top of this file and use that type here.

Everything else uses size_t for plane index.  Should I go through and convert
all similar definitions/callers?

[ncarter (slow), 2013-11-19 21:32:22]

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

https://codereview.chromium.org/48113011/diff/560001/media/base/video_frame.c...
media/base/video_frame.cc:272: RoundUp(coded_size.width(), 2) *
RoundUp(coded_size.height(), 2);
On 2013/11/19 21:01:52, Ami Fischman wrote:
> Any reason for this not to be outside the outer switch?

Making the switch statement RoundUp-ready would no doubt encourage a lawsuit
from Monsanto.

[sheu, 2013-11-19 21:35:15]

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

https://codereview.chromium.org/48113011/diff/560001/media/base/video_frame.c...
media/base/video_frame.cc:272: RoundUp(coded_size.width(), 2) *
RoundUp(coded_size.height(), 2);
On 2013/11/19 21:32:22, ncarter wrote:
> On 2013/11/19 21:01:52, Ami Fischman wrote:
> > Any reason for this not to be outside the outer switch?
> 
> Making the switch statement RoundUp-ready would no doubt encourage a lawsuit
> from Monsanto.

communicate with care

[ncarter (slow), 2013-11-19 21:36:41]

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

https://codereview.chromium.org/48113011/diff/560001/media/base/video_frame.h...
media/base/video_frame.h:201: size_t plane,
On 2013/11/19 21:30:27, sheu wrote:
> On 2013/11/19 21:01:52, Ami Fischman wrote:
> > size_t is a weird type to use for this.  Please name the k[ARGB]Plane enum
> type
> > at the top of this file and use that type here.
> 
> Everything else uses size_t for plane index.  Should I go through and convert
> all similar definitions/callers?

I concur with sheu@; we should leave this as size_t. A common usage is to have a
for loop doing plane_index++; requiring an enum here makes that usage more
cumbersome. I'm not sure why size_t was used instead of int, but if we change
one spot we should change all of them, and that would be best done as a separate
change.

[wjia(left Chromium), 2013-11-19 22:30:37]

lgtm

[Ami GONE FROM CHROMIUM, 2013-11-19 22:33:26]

SGTM


On Tue, Nov 19, 2013 at 1:36 PM, <nick@chromium.org> wrote:

>
> https://codereview.chromium.org/48113011/diff/560001/
> media/base/video_frame.h
> File media/base/video_frame.h (right):
>
> https://codereview.chromium.org/48113011/diff/560001/
> media/base/video_frame.h#newcode201
> media/base/video_frame.h:201: size_t plane,
> On 2013/11/19 21:30:27, sheu wrote:
>
>> On 2013/11/19 21:01:52, Ami Fischman wrote:
>> > size_t is a weird type to use for this.  Please name the
>>
> k[ARGB]Plane enum
>
>> type
>> > at the top of this file and use that type here.
>>
>
>  Everything else uses size_t for plane index.  Should I go through and
>>
> convert
>
>> all similar definitions/callers?
>>
>
> I concur with sheu@; we should leave this as size_t. A common usage is
> to have a for loop doing plane_index++; requiring an enum here makes
> that usage more cumbersome. I'm not sure why size_t was used instead of
> int, but if we change one spot we should change all of them, and that
> would be best done as a separate change.
>
> https://codereview.chromium.org/48113011/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2013-11-19 23:44:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sheu@chromium.org/48113011/690001

[commit-bot: I haz the power, 2013-11-20 02:39:54]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-11-20 17:33:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sheu@chromium.org/48113011/820001

[commit-bot: I haz the power, 2013-11-20 20:47:19]

Change committed as 236279