Reland: Video Capture: extract storage info from pixel format in VideoCaptureFormat.

Reland: Video Capture: extract storage info from pixel format in VideoCaptureFormat.

Relanding https://codereview.chromium.org/1179323002/ with dcheng@
comments addressed.

Original description ---------------------------------------------------
Video Capture: extract storage info from pixel format in VideoCaptureFormat.

Video Capture Devices treat both Texture and GpuMemoryBuffer
as a VideoPixelFormat, but they are storage types. Moreover,
this merging prevents capture devices from indicating a
combination of Storage and Capture formats, i.e.
Texture + ARGB, or GpuMemoryBuffer+YUY2.

This CL separates both concepts and updates necessary
call points. It also extends the translation
VideoCaptureFormat --> VideoFrame in
VideoCaptureDeviceClient and in VideoCaptureBufferPool.
VideoCaptureBufferPool::ReserveOutputBuffer()
also gets a param to specify the StorageType.

This separation also allows for VideoCaptureBufferPool
to operate using VideoPixel{Format, Storage} ISO
VideoFrame types, which spares the constant conversion
between ones and others.

Test: All video captures working exactly as before.
BUG=440843
--------------------------------------------------------------------------

TBR=dalecurtis@chromium.org, miu@chromium.org, hubbe@chromium.org
Rationale: they already LGTM PS1 and the changes
from there onwards are either cosmetic (git cl format,
DCHECK_EQ(expected, actual)) or discussed with dcheng@.

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

[mcasas, 2015-06-24 21:23:22]

mcasas@chromium.org changed reviewers:
+ dcheng@chromium.org

[mcasas, 2015-06-24 21:23:23]

dcheng@ PTAL

https://codereview.chromium.org/1204063005/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/media/video_capture_device_client.cc (right):

https://codereview.chromium.org/1204063005/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_device_client.cc:219:
DCHECK_EQ(frame_format.pixel_storage, media::PIXEL_STORAGE_CPU);
dcheng@: Same comment: this should be DCHECK_EQ(actual, expected)

Done.

https://codereview.chromium.org/1204063005/diff/1/content/common/media/media_...
File content/common/media/media_param_traits.cc (right):

https://codereview.chromium.org/1204063005/diff/1/content/common/media/media_...
content/common/media/media_param_traits.cc:78: !iter->ReadInt(&pixel_storage))
dcheng@ Please change all of these to use ReadParam(). This will simplify your
IsValid()
method (you won't need to check that the enum values are in bounds, for
example), and you won't need the awkward static_cast below.

Done.

https://codereview.chromium.org/1204063005/diff/1/media/base/video_capture_ty...
File media/base/video_capture_types.h (right):

https://codereview.chromium.org/1204063005/diff/1/media/base/video_capture_ty...
media/base/video_capture_types.h:40: enum VideoPixelStorage {
dcheng@ enum class.

I don't understand, do you mean I should make a class to wrap 
this enum? It's not customary in media/, I'd be happy to follow
up in another CL if this is interesting.

[mcasas, 2015-06-24 21:35:28]

Patchset #2 (id:20001) has been deleted

[dcheng, 2015-06-24 21:41:48]

https://codereview.chromium.org/1204063005/diff/1/media/base/video_capture_ty...
File media/base/video_capture_types.h (right):

https://codereview.chromium.org/1204063005/diff/1/media/base/video_capture_ty...
media/base/video_capture_types.h:40: enum VideoPixelStorage {
On 2015/06/24 at 21:23:23, mcasas wrote:
> dcheng@ enum class.
> 
> I don't understand, do you mean I should make a class to wrap 
> this enum? It's not customary in media/, I'd be happy to follow
> up in another CL if this is interesting.

enum class VideoPixelStorage {
  CPU,
  TEXTURE,
  GPUMEMORYBUFFER,
};

(Incidentally, what does CPU mean anyway? I assume that means it's in main
memory?)

https://codereview.chromium.org/1204063005/diff/40001/content/browser/media/c...
File content/browser/media/capture/aura_window_capture_machine.cc (right):

https://codereview.chromium.org/1204063005/diff/40001/content/browser/media/c...
content/browser/media/capture/aura_window_capture_machine.cc:311:
DCHECK_EQ(capture_params_.requested_format.pixel_format,
The arguments to this macro should be reversed.

https://codereview.chromium.org/1204063005/diff/40001/content/common/media/me...
File content/common/media/media_param_traits.cc (right):

https://codereview.chromium.org/1204063005/diff/40001/content/common/media/me...
content/common/media/media_param_traits.cc:74: if (!ReadParam(m, iter,
&frame_size_width) ||
ReadParam(m, iter, &r->frame_size)

https://codereview.chromium.org/1204063005/diff/40001/content/common/media/me...
content/common/media/media_param_traits.cc:78: !ReadParam(m, iter,
&r->pixel_storage))
Thanks, this is much better (the original patch didn't check that the enum was
actually in range)

https://codereview.chromium.org/1204063005/diff/40001/media/base/video_captur...
File media/base/video_capture_types.cc (right):

https://codereview.chromium.org/1204063005/diff/40001/media/base/video_captur...
media/base/video_capture_types.cc:128: return "";
Nit: return std::string() instead of return ""

[dcheng, 2015-06-24 21:43:33]

https://codereview.chromium.org/1204063005/diff/40001/content/common/media/me...
File content/common/media/media_param_traits.cc (right):

https://codereview.chromium.org/1204063005/diff/40001/content/common/media/me...
content/common/media/media_param_traits.cc:78: !ReadParam(m, iter,
&r->pixel_storage))
On 2015/06/24 at 21:41:47, dcheng wrote:
> Thanks, this is much better (the original patch didn't check that the enum was
actually in range)

Also, don't you need to IPC_ENUM_TRAITS_MAX_VALUE(media::VideoPixelStorage,
...)? I don't see it added in this patch.

[mcasas, 2015-06-24 22:21:10]

Patchset #3 (id:60001) has been deleted

[mcasas, 2015-06-24 22:45:26]

Patchset #3 (id:80001) has been deleted

[mcasas, 2015-06-24 22:45:42]

Patchset #3 (id:100001) has been deleted

[mcasas, 2015-06-24 22:52:43]

Patchset #3 (id:110024) has been deleted

[mcasas, 2015-06-24 23:05:52]

Patchset #3 (id:130001) has been deleted

[mcasas, 2015-06-24 23:18:16]

Patchset #3 (id:150001) has been deleted

[mcasas, 2015-06-24 23:39:37]

Patchset #3 (id:170001) has been deleted

[mcasas, 2015-06-24 23:58:30]

Patchset #3 (id:190001) has been deleted

[mcasas, 2015-06-24 23:58:46]

Patchset #3 (id:190001) has been deleted

[mcasas, 2015-06-25 01:17:09]

Patchset #3 (id:210001) has been deleted

[mcasas, 2015-06-25 01:17:18]

Patchset #3 (id:230001) has been deleted

[mcasas, 2015-06-25 01:19:51]

(Still have some issues telling IPC how to Read()
VideoPixelFormat and VideoPixelStorage)

https://codereview.chromium.org/1204063005/diff/1/media/base/video_capture_ty...
File media/base/video_capture_types.h (right):

https://codereview.chromium.org/1204063005/diff/1/media/base/video_capture_ty...
media/base/video_capture_types.h:40: enum VideoPixelStorage {
On 2015/06/24 21:41:47, dcheng wrote:
> On 2015/06/24 at 21:23:23, mcasas wrote:
> > dcheng@ enum class.
> > 
> > I don't understand, do you mean I should make a class to wrap 
> > this enum? It's not customary in media/, I'd be happy to follow
> > up in another CL if this is interesting.
> 
> enum class VideoPixelStorage {
>   CPU,
>   TEXTURE,
>   GPUMEMORYBUFFER,
> };
> 
I wrote a bug to do this in the near future, together with 
a similar change for VideoPixelFormat. The reason for not
doing it on the spot is that DCHECK_EQ has troubles to
print out enum classes when the condition does not verify,
and the changes would be too widespread for this CL.

> (Incidentally, what does CPU mean anyway? I assume that means it's in main
> memory?)

Yes, it means precisely that.

https://codereview.chromium.org/1204063005/diff/40001/content/browser/media/c...
File content/browser/media/capture/aura_window_capture_machine.cc (right):

https://codereview.chromium.org/1204063005/diff/40001/content/browser/media/c...
content/browser/media/capture/aura_window_capture_machine.cc:311:
DCHECK_EQ(capture_params_.requested_format.pixel_format,
On 2015/06/24 21:41:47, dcheng wrote:
> The arguments to this macro should be reversed.

Done.

https://codereview.chromium.org/1204063005/diff/40001/content/common/media/me...
File content/common/media/media_param_traits.cc (right):

https://codereview.chromium.org/1204063005/diff/40001/content/common/media/me...
content/common/media/media_param_traits.cc:74: if (!ReadParam(m, iter,
&frame_size_width) ||
On 2015/06/24 21:41:48, dcheng wrote:
> ReadParam(m, iter, &r->frame_size)

no member named 'Read' in 'IPC::ParamTraits<gfx::Size>'

https://codereview.chromium.org/1204063005/diff/40001/content/common/media/me...
content/common/media/media_param_traits.cc:78: !ReadParam(m, iter,
&r->pixel_storage))
On 2015/06/24 21:41:47, dcheng wrote:
> Thanks, this is much better (the original patch didn't check that the enum was
> actually in range)

Acknowledged.

https://codereview.chromium.org/1204063005/diff/40001/content/common/media/me...
content/common/media/media_param_traits.cc:78: !ReadParam(m, iter,
&r->pixel_storage))
On 2015/06/24 21:43:33, dcheng wrote:
> On 2015/06/24 at 21:41:47, dcheng wrote:
> > Thanks, this is much better (the original patch didn't check that the enum
was
> actually in range)
> 
> Also, don't you need to IPC_ENUM_TRAITS_MAX_VALUE(media::VideoPixelStorage,
> ...)? I don't see it added in this patch.

Done,

https://codereview.chromium.org/1204063005/diff/40001/media/base/video_captur...
File media/base/video_capture_types.cc (right):

https://codereview.chromium.org/1204063005/diff/40001/media/base/video_captur...
media/base/video_capture_types.cc:128: return "";
On 2015/06/24 21:41:48, dcheng wrote:
> Nit: return std::string() instead of return ""

Done.

[dcheng, 2015-06-25 02:02:18]

https://codereview.chromium.org/1204063005/diff/1/media/base/video_capture_ty...
File media/base/video_capture_types.h (right):

https://codereview.chromium.org/1204063005/diff/1/media/base/video_capture_ty...
media/base/video_capture_types.h:40: enum VideoPixelStorage {
On 2015/06/25 at 01:19:51, mcasas wrote:
> On 2015/06/24 21:41:47, dcheng wrote:
> > On 2015/06/24 at 21:23:23, mcasas wrote:
> > > dcheng@ enum class.
> > > 
> > > I don't understand, do you mean I should make a class to wrap 
> > > this enum? It's not customary in media/, I'd be happy to follow
> > > up in another CL if this is interesting.
> > 
> > enum class VideoPixelStorage {
> >   CPU,
> >   TEXTURE,
> >   GPUMEMORYBUFFER,
> > };
> > 
> I wrote a bug to do this in the near future, together with 
> a similar change for VideoPixelFormat. The reason for not
> doing it on the spot is that DCHECK_EQ has troubles to
> print out enum classes when the condition does not verify,
> and the changes would be too widespread for this CL.
> 
> > (Incidentally, what does CPU mean anyway? I assume that means it's in main
> > memory?)
> 
> Yes, it means precisely that.

Some comments here would be nice =)

https://codereview.chromium.org/1204063005/diff/250001/content/common/media/m...
File content/common/media/media_param_traits.cc (right):

https://codereview.chromium.org/1204063005/diff/250001/content/common/media/m...
content/common/media/media_param_traits.cc:21:
IPC_ENUM_TRAITS_MAX_VALUE(media::VideoPixelStorage, media::PIXEL_STORAGE_MAX)
See https://codereview.chromium.org/1210743003/ for how to make this work (the
diff between PS1 and PS2 are the changes you'll have to make)

https://codereview.chromium.org/1204063005/diff/250001/content/common/media/m...
content/common/media/media_param_traits.cc:69:
m->WriteInt(static_cast<int>(p.pixel_storage));
Btw, these should probably be WriteParam() as well.

[miu, 2015-06-25 03:05:27]

On 2015/06/25 02:02:18, dcheng wrote:
>
https://codereview.chromium.org/1204063005/diff/1/media/base/video_capture_ty...
> > > enum class VideoPixelStorage {
> > >   CPU,
> > >   TEXTURE,
> > >   GPUMEMORYBUFFER,
> > > };
> > > 
> > > (Incidentally, what does CPU mean anyway? I assume that means it's in main
> > > memory?)
> > 
> > Yes, it means precisely that.
> 
> Some comments here would be nice =)

This is going to be used in many files.  Rather than CPU, can we rename it to
MAIN_MEMORY to avoid confusion?

[mcasas, 2015-06-25 13:45:14]

Patchset #4 (id:270001) has been deleted

[mcasas, 2015-06-25 13:51:03]

dcheng@ took your changes in, thanks! PTAL

miu@ after your comment the code was moved back 
to its original place and the change to using enum class
is postponed to another CL. I propose we discuss the
change s/CPU/what/ in a subsequent review. Thanks!

https://codereview.chromium.org/1204063005/diff/250001/content/common/media/m...
File content/common/media/media_param_traits.cc (right):

https://codereview.chromium.org/1204063005/diff/250001/content/common/media/m...
content/common/media/media_param_traits.cc:21:
IPC_ENUM_TRAITS_MAX_VALUE(media::VideoPixelStorage, media::PIXEL_STORAGE_MAX)
On 2015/06/25 02:02:18, dcheng wrote:
> See https://codereview.chromium.org/1210743003/ for how to make this work (the
> diff between PS1 and PS2 are the changes you'll have to make)

Done.

https://codereview.chromium.org/1204063005/diff/250001/content/common/media/m...
content/common/media/media_param_traits.cc:69:
m->WriteInt(static_cast<int>(p.pixel_storage));
On 2015/06/25 02:02:18, dcheng wrote:
> Btw, these should probably be WriteParam() as well.

Done.

[dcheng, 2015-06-25 17:11:27]

https://codereview.chromium.org/1204063005/diff/290001/content/browser/render...
File content/browser/renderer_host/media/video_capture_device_client.cc (right):

https://codereview.chromium.org/1204063005/diff/290001/content/browser/render...
content/browser/renderer_host/media/video_capture_device_client.cc:548:
DCHECK_EQ(frame_format.pixel_storage, media::PIXEL_STORAGE_GPUMEMORYBUFFER);
Nit: DCHECK_EQ(expected, actual)

https://codereview.chromium.org/1204063005/diff/290001/media/base/video_captu...
File media/base/video_capture_types.cc (right):

https://codereview.chromium.org/1204063005/diff/290001/media/base/video_captu...
media/base/video_capture_types.cc:45: (pixel_format >= 0) && (pixel_format <
PIXEL_FORMAT_MAX) &&
Nit: This check is redundant since the IPC deserialization uses ReadParam now.

[dcheng, 2015-06-25 17:11:45]

I meant to say:
IPC changes LGTM with nits addressed.

[mcasas, 2015-06-25 18:23:03]

https://codereview.chromium.org/1204063005/diff/290001/content/browser/render...
File content/browser/renderer_host/media/video_capture_device_client.cc (right):

https://codereview.chromium.org/1204063005/diff/290001/content/browser/render...
content/browser/renderer_host/media/video_capture_device_client.cc:548:
DCHECK_EQ(frame_format.pixel_storage, media::PIXEL_STORAGE_GPUMEMORYBUFFER);
On 2015/06/25 17:11:27, dcheng wrote:
> Nit: DCHECK_EQ(expected, actual)

Done. This and a few others.

https://codereview.chromium.org/1204063005/diff/290001/media/base/video_captu...
File media/base/video_capture_types.cc (right):

https://codereview.chromium.org/1204063005/diff/290001/media/base/video_captu...
media/base/video_capture_types.cc:45: (pixel_format >= 0) && (pixel_format <
PIXEL_FORMAT_MAX) &&
On 2015/06/25 17:11:27, dcheng wrote:
> Nit: This check is redundant since the IPC deserialization uses ReadParam now.

I know but it's still used in a few other
places, perhaps wrongly so.

[mcasas, 2015-06-25 18:23:09]

The CQ bit was checked by mcasas@chromium.org

[mcasas, 2015-06-25 18:23:10]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1204063005/#ps310001
(title: "dcheng@ nit on DCHECK_EQ(expected, actual)")

[commit-bot: I haz the power, 2015-06-25 18:24:30]

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

[dcheng, 2015-06-25 18:24:38]

https://codereview.chromium.org/1204063005/diff/290001/media/base/video_captu...
File media/base/video_capture_types.cc (right):

https://codereview.chromium.org/1204063005/diff/290001/media/base/video_captu...
media/base/video_capture_types.cc:45: (pixel_format >= 0) && (pixel_format <
PIXEL_FORMAT_MAX) &&
On 2015/06/25 at 18:23:02, mcasas wrote:
> On 2015/06/25 17:11:27, dcheng wrote:
> > Nit: This check is redundant since the IPC deserialization uses ReadParam
now.
> 
> I know but it's still used in a few other
> places, perhaps wrongly so.

This check is meaningless outside a security boundary like IPC.

If the renderer is compromised and setting invalid enum values in
VideoCaptureFormat, it doesn't matter that we have the IsValid() check: the
renderer is compromised.

[commit-bot: I haz the power, 2015-06-25 18:32:02]

Committed patchset #5 (id:310001)

[commit-bot: I haz the power, 2015-06-25 18:33:08]

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