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

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=dcheng@chromium.org for media_param_traits.cc
(Rationale: the change is small and I'm still going to be
actively working in this area so we can follow up in
other reviews).

Committed: https://crrev.com/957fb245c45052e2c4b2bb0f59e165ba05096904
Cr-Commit-Position: refs/heads/master@{#335872}

[mcasas, 2015-06-12 23:07:27]

Patchset #1 (id:1) has been deleted

[mcasas, 2015-06-13 00:09:50]

Patchset #1 (id:20001) has been deleted

[miu, 2015-06-13 00:41:35]

miu@chromium.org changed reviewers:
+ miu@chromium.org

[miu, 2015-06-13 00:41:37]

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

https://codereview.chromium.org/1179323002/diff/40001/content/browser/media/c...
content/browser/media/capture/content_video_capture_device_core.cc:240: if
(params.requested_format.pixel_format != media::PIXEL_FORMAT_I420 &&
I think you meant for this expression to be:

  if (params.requested_format.pixel_format != media::PIXEL_FORMAT_I420 ||
      (params.requested_format.pixel_storage != media::PIXEL_STORAGE_CPU &&
       params.requested_format.pixel_storage != media::PIXEL_STORAGE_TEXTURE)) {
    ...unsupported format...
  }

https://codereview.chromium.org/1179323002/diff/40001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_buffer_pool.cc (right):

https://codereview.chromium.org/1179323002/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_buffer_pool.cc:48:
{media::PIXEL_STORAGE_GPUMEMORYBUFFER,
nit: Up to you, but this feels more readable:

#if defined(OS_LINUX)
  {media::PIXEL_STORAGE_GPUMEMORYBUFFER, VideoFrame::STORAGE_DMABUFS},
#else
  {media::PIXEL_STORAGE_GPUMEMORYBUFFER, VideoFrame::STORAGE_SHMEM},
#endif

https://codereview.chromium.org/1179323002/diff/40001/media/video/capture/vid...
File media/video/capture/video_capture_device.h (right):

https://codereview.chromium.org/1179323002/diff/40001/media/video/capture/vid...
media/video/capture/video_capture_device.h:249: const VideoCaptureFormat&
frame_format) = 0;
Hmm...We talked about this in a similar change a few months back, where: 1) the
frame_rate field of VideoCaptureFormat is always unused; and 2) you're burdening
all client code with creating a VideoCaptureFormat struct to make a method call.
 Even with this new, 3rd argument I still feel you should just pass 3 args
instead of the struct.  We can revisit this when it becomes 4+ args.  ;-)

[mcasas, 2015-06-16 17:07:14]

Patchset #3 (id:80001) has been deleted

[mcasas, 2015-06-16 17:26:49]

Patchset #3 (id:100001) has been deleted

[mcasas, 2015-06-16 17:33:39]

Patchset #3 (id:100001) has been deleted

[mcasas, 2015-06-16 18:09:39]

Patchset #3 (id:60002) has been deleted

[hubbe, 2015-06-16 21:11:34]

hubbe@chromium.org changed reviewers:
+ hubbe@chromium.org

[hubbe, 2015-06-16 21:11:35]

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

https://codereview.chromium.org/1179323002/diff/130001/media/base/video_captu...
media/base/video_capture_types.cc:77: std::string VideoCaptureFormat::ToString()
const {
What is this function used for?
Who does it affect when we change the format?
If it is only used for debugging, can we file a bug to remove it at some point
in the future?

https://codereview.chromium.org/1179323002/diff/130001/media/base/video_captu...
File media/base/video_capture_types.h (right):

https://codereview.chromium.org/1179323002/diff/130001/media/base/video_captu...
media/base/video_capture_types.h:37: PIXEL_STORAGE_TEXTURE,
Does PIXEL_FORMAT_???? make any sense if the storage is a texture?
Please add some comments to explain the semantics.

[mcasas, 2015-06-16 23:14:02]

hubbe@ PTAL

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

https://codereview.chromium.org/1179323002/diff/40001/content/browser/media/c...
content/browser/media/capture/content_video_capture_device_core.cc:240: if
(params.requested_format.pixel_format != media::PIXEL_FORMAT_I420 &&
On 2015/06/13 00:41:37, miu_NO_REVIEWS_UNTIL_22_June wrote:
> I think you meant for this expression to be:
> 
>   if (params.requested_format.pixel_format != media::PIXEL_FORMAT_I420 ||
>       (params.requested_format.pixel_storage != media::PIXEL_STORAGE_CPU &&
>        params.requested_format.pixel_storage != media::PIXEL_STORAGE_TEXTURE))
{
>     ...unsupported format...
>   }

Actually I think it would be more accurate to say:
If is NOT an I420 + CPU memory NOR an ARGB + texture
  bail out...

I think this reflects more accurately the previous
implied situation, WDYT?

https://codereview.chromium.org/1179323002/diff/40001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_buffer_pool.cc (right):

https://codereview.chromium.org/1179323002/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_buffer_pool.cc:48:
{media::PIXEL_STORAGE_GPUMEMORYBUFFER,
On 2015/06/13 00:41:37, miu_NO_REVIEWS_UNTIL_22_June wrote:
> nit: Up to you, but this feels more readable:
> 
> #if defined(OS_LINUX)
>   {media::PIXEL_STORAGE_GPUMEMORYBUFFER, VideoFrame::STORAGE_DMABUFS},
> #else
>   {media::PIXEL_STORAGE_GPUMEMORYBUFFER, VideoFrame::STORAGE_SHMEM},
> #endif

Yes. And this is actually for USE_OZONE build,
not just for Linux, corrected.

https://codereview.chromium.org/1179323002/diff/40001/media/video/capture/vid...
File media/video/capture/video_capture_device.h (right):

https://codereview.chromium.org/1179323002/diff/40001/media/video/capture/vid...
media/video/capture/video_capture_device.h:249: const VideoCaptureFormat&
frame_format) = 0;
On 2015/06/13 00:41:37, miu_NO_REVIEWS_UNTIL_22_June wrote:
> Hmm...We talked about this in a similar change a few months back, where: 1)
the
> frame_rate field of VideoCaptureFormat is always unused; and 2) you're
burdening
> all client code with creating a VideoCaptureFormat struct to make a method
call.
>  Even with this new, 3rd argument I still feel you should just pass 3 args
> instead of the struct.  We can revisit this when it becomes 4+ args.  ;-)

Done.

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

https://codereview.chromium.org/1179323002/diff/130001/media/base/video_captu...
media/base/video_capture_types.cc:77: std::string VideoCaptureFormat::ToString()
const {
On 2015/06/16 21:11:35, hubbe wrote:
> What is this function used for?
> Who does it affect when we change the format?
> If it is only used for debugging, can we file a bug to remove it at some point
> in the future?
> 

Is for Debug/dev purposes. Filed a bug.

https://codereview.chromium.org/1179323002/diff/130001/media/base/video_captu...
File media/base/video_capture_types.h (right):

https://codereview.chromium.org/1179323002/diff/130001/media/base/video_captu...
media/base/video_capture_types.h:37: PIXEL_STORAGE_TEXTURE,
On 2015/06/16 21:11:35, hubbe wrote:
> Does PIXEL_FORMAT_???? make any sense if the storage is a texture?
> Please add some comments to explain the semantics.

Done.

[hubbe, 2015-06-18 19:23:11]

https://codereview.chromium.org/1179323002/diff/150001/content/browser/media/...
File content/browser/media/capture/aura_window_capture_machine.cc (right):

https://codereview.chromium.org/1179323002/diff/150001/content/browser/media/...
content/browser/media/capture/aura_window_capture_machine.cc:282:
media::PIXEL_STORAGE_TEXTURE) {
Dcheck that pixel_format is ARGB?

https://codereview.chromium.org/1179323002/diff/150001/content/browser/media/...
File content/browser/media/capture/desktop_capture_device_aura_unittest.cc
(right):

https://codereview.chromium.org/1179323002/diff/150001/content/browser/media/...
content/browser/media/capture/desktop_capture_device_aura_unittest.cc:64:
DoReserveOutputBuffer();
CHECK/EXPECT input values?

https://codereview.chromium.org/1179323002/diff/150001/content/browser/media/...
File content/browser/media/capture/desktop_capture_device_unittest.cc (right):

https://codereview.chromium.org/1179323002/diff/150001/content/browser/media/...
content/browser/media/capture/desktop_capture_device_unittest.cc:85:
DoReserveOutputBuffer();
CHECK/EXPECT input values?

https://codereview.chromium.org/1179323002/diff/150001/content/browser/render...
File content/browser/renderer_host/media/video_capture_buffer_pool.cc (left):

https://codereview.chromium.org/1179323002/diff/150001/content/browser/render...
content/browser/renderer_host/media/video_capture_buffer_pool.cc:191: const
size_t byte_count = VideoFrame::AllocationSize(format, dimensions);
Being able to re-use VideoFrame code seems like a good thing...

https://codereview.chromium.org/1179323002/diff/150001/content/browser/render...
File content/browser/renderer_host/media/video_capture_buffer_pool.cc (right):

https://codereview.chromium.org/1179323002/diff/150001/content/browser/render...
content/browser/renderer_host/media/video_capture_buffer_pool.cc:355: const bool
use_gmb_tracker =
Why not inline this?

https://codereview.chromium.org/1179323002/diff/150001/content/browser/render...
File content/browser/renderer_host/media/video_capture_controller_unittest.cc
(right):

https://codereview.chromium.org/1179323002/diff/150001/content/browser/render...
content/browser/renderer_host/media/video_capture_controller_unittest.cc:663:
for (int format = 0; format < media::PIXEL_FORMAT_UNKNOWN; ++format) {
Seems brittle.
How about iterate to media::PIXEL_FORMAT_MAX and explicitly skip UNKNOWN with an
if statement?

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

https://codereview.chromium.org/1179323002/diff/150001/media/base/video_captu...
media/base/video_capture_types.cc:34: pixel_storage(pixel_storage) {}
Maybe DCHECK that TEXTURE is only used with ARGB here?

https://codereview.chromium.org/1179323002/diff/150001/media/base/video_captu...
media/base/video_capture_types.cc:44: (pixel_format < PIXEL_FORMAT_MAX);
Maybe return false if storage is TEXTURE and pixel format is not ARGB?

https://codereview.chromium.org/1179323002/diff/150001/media/base/video_captu...
File media/base/video_capture_types.h (right):

https://codereview.chromium.org/1179323002/diff/150001/media/base/video_captu...
media/base/video_capture_types.h:21: enum VideoPixelFormat {
Would it be possible to unify VideoPixelFormat and VideoFrame::Format?

https://codereview.chromium.org/1179323002/diff/150001/media/base/video_captu...
media/base/video_capture_types.h:37: // Format are possible, though some are
very typical, such as texture + ARGB,
I don't think this is clear enough / struct.
What does texture + MJPEG mean? Is it allowed? Is the format ignored for
textures?

https://codereview.chromium.org/1179323002/diff/150001/media/base/video_captu...
media/base/video_capture_types.h:39: enum VideoPixelStorage {
Shouldn't this really be media::VideoFrame::StorageType?

https://codereview.chromium.org/1179323002/diff/150001/media/video/capture/fa...
File media/video/capture/fake_video_capture_device_unittest.cc (right):

https://codereview.chromium.org/1179323002/diff/150001/media/video/capture/fa...
media/video/capture/fake_video_capture_device_unittest.cc:87: EXPECT_TRUE(format
== PIXEL_FORMAT_I420 || format == PIXEL_FORMAT_ARGB);
EXPECT for storage?

https://codereview.chromium.org/1179323002/diff/150001/media/video/capture/vi...
File media/video/capture/video_capture_device_unittest.cc (right):

https://codereview.chromium.org/1179323002/diff/150001/media/video/capture/vi...
media/video/capture/video_capture_device_unittest.cc:106:
DoReserveOutputBuffer();
EXPECTs for format/storage?

[mcasas, 2015-06-19 02:05:56]

Patchset #6 (id:190001) has been deleted

[mcasas, 2015-06-19 02:08:31]

Patchset #5 (id:170001) has been deleted

[mcasas, 2015-06-19 02:58:36]

hubbe@ PTAL

https://codereview.chromium.org/1179323002/diff/150001/content/browser/media/...
File content/browser/media/capture/aura_window_capture_machine.cc (right):

https://codereview.chromium.org/1179323002/diff/150001/content/browser/media/...
content/browser/media/capture/aura_window_capture_machine.cc:282:
media::PIXEL_STORAGE_TEXTURE) {
On 2015/06/18 19:23:10, hubbe wrote:
> Dcheck that pixel_format is ARGB?

Done.

https://codereview.chromium.org/1179323002/diff/150001/content/browser/media/...
File content/browser/media/capture/desktop_capture_device_aura_unittest.cc
(right):

https://codereview.chromium.org/1179323002/diff/150001/content/browser/media/...
content/browser/media/capture/desktop_capture_device_aura_unittest.cc:64:
DoReserveOutputBuffer();
On 2015/06/18 19:23:10, hubbe wrote:
> CHECK/EXPECT input values?

Done.

https://codereview.chromium.org/1179323002/diff/150001/content/browser/media/...
File content/browser/media/capture/desktop_capture_device_unittest.cc (right):

https://codereview.chromium.org/1179323002/diff/150001/content/browser/media/...
content/browser/media/capture/desktop_capture_device_unittest.cc:85:
DoReserveOutputBuffer();
On 2015/06/18 19:23:10, hubbe wrote:
> CHECK/EXPECT input values?

Done.

https://codereview.chromium.org/1179323002/diff/150001/content/browser/render...
File content/browser/renderer_host/media/video_capture_buffer_pool.cc (left):

https://codereview.chromium.org/1179323002/diff/150001/content/browser/render...
content/browser/renderer_host/media/video_capture_buffer_pool.cc:191: const
size_t byte_count = VideoFrame::AllocationSize(format, dimensions);
On 2015/06/18 19:23:10, hubbe wrote:
> Being able to re-use VideoFrame code seems like a good thing...

Yes but VideoCaptureBufferPool actually operates
with capture data types, and they are not (yet) 
the same thing. Hence the change.

https://codereview.chromium.org/1179323002/diff/150001/content/browser/render...
File content/browser/renderer_host/media/video_capture_buffer_pool.cc (right):

https://codereview.chromium.org/1179323002/diff/150001/content/browser/render...
content/browser/renderer_host/media/video_capture_buffer_pool.cc:355: const bool
use_gmb_tracker =
On 2015/06/18 19:23:10, hubbe wrote:
> Why not inline this?

Done.

https://codereview.chromium.org/1179323002/diff/150001/content/browser/render...
File content/browser/renderer_host/media/video_capture_controller_unittest.cc
(right):

https://codereview.chromium.org/1179323002/diff/150001/content/browser/render...
content/browser/renderer_host/media/video_capture_controller_unittest.cc:663:
for (int format = 0; format < media::PIXEL_FORMAT_UNKNOWN; ++format) {
On 2015/06/18 19:23:11, hubbe wrote:
> Seems brittle.
> How about iterate to media::PIXEL_FORMAT_MAX and explicitly skip UNKNOWN with
an
> if statement?

Done.

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

https://codereview.chromium.org/1179323002/diff/150001/media/base/video_captu...
media/base/video_capture_types.cc:34: pixel_storage(pixel_storage) {}
On 2015/06/18 19:23:11, hubbe wrote:
> Maybe DCHECK that TEXTURE is only used with ARGB here?

Well right now constructor does not check anything
so I'd rather leave it like that, e.g. frame_size==(0,0)
would be OK.
Usually IsValid() is used below to check for 
semantic correctness.

https://codereview.chromium.org/1179323002/diff/150001/media/base/video_captu...
media/base/video_capture_types.cc:44: (pixel_format < PIXEL_FORMAT_MAX);
On 2015/06/18 19:23:11, hubbe wrote:
> Maybe return false if storage is TEXTURE and pixel format is not ARGB?

Done.

https://codereview.chromium.org/1179323002/diff/150001/media/base/video_captu...
File media/base/video_capture_types.h (right):

https://codereview.chromium.org/1179323002/diff/150001/media/base/video_captu...
media/base/video_capture_types.h:21: enum VideoPixelFormat {
On 2015/06/18 19:23:11, hubbe wrote:
> Would it be possible to unify VideoPixelFormat and VideoFrame::Format?

Added a TODO(). It would not be trivial because
they are not fully coincidental (e.g. YV12A is only 
in VideoFrame::Format, YUY2 is only here, etc).

https://codereview.chromium.org/1179323002/diff/150001/media/base/video_captu...
media/base/video_capture_types.h:37: // Format are possible, though some are
very typical, such as texture + ARGB,
On 2015/06/18 19:23:11, hubbe wrote:
> I don't think this is clear enough / struct.
> What does texture + MJPEG mean? Is it allowed? 

Same as with VideoFrame, the specification of 
storage and pixel format are independent, so all
combinations are meaningful although perhaps some
are just not implemented. A texture MJPEG means
exactly what is says: a formatless buffer allocated
in the GPU (==texture) with data representing a
video frame in MJPEG pixel format.

> Is the format ignored for
> textures?
> 
No it isn't. But the only one used so far is ARGB.

https://codereview.chromium.org/1179323002/diff/150001/media/base/video_captu...
media/base/video_capture_types.h:39: enum VideoPixelStorage {
On 2015/06/18 19:23:11, hubbe wrote:
> Shouldn't this really be media::VideoFrame::StorageType?

Again is only partially coincidental with
media::VideoFrame::StorageType, so although
it would be good, this is more reduced.

https://codereview.chromium.org/1179323002/diff/150001/media/video/capture/fa...
File media/video/capture/fake_video_capture_device_unittest.cc (right):

https://codereview.chromium.org/1179323002/diff/150001/media/video/capture/fa...
media/video/capture/fake_video_capture_device_unittest.cc:87: EXPECT_TRUE(format
== PIXEL_FORMAT_I420 || format == PIXEL_FORMAT_ARGB);
On 2015/06/18 19:23:11, hubbe wrote:
> EXPECT for storage?

Done.

https://codereview.chromium.org/1179323002/diff/150001/media/video/capture/vi...
File media/video/capture/video_capture_device_unittest.cc (right):

https://codereview.chromium.org/1179323002/diff/150001/media/video/capture/vi...
media/video/capture/video_capture_device_unittest.cc:106:
DoReserveOutputBuffer();
On 2015/06/18 19:23:11, hubbe wrote:
> EXPECTs for format/storage?

Nope, normal VCDevices do not call this method,
so is used with a .Times(0). 
I'll add a NOTREACHED().

https://codereview.chromium.org/1179323002/diff/210001/content/browser/media/...
File content/browser/media/capture/aura_window_capture_machine.cc (right):

https://codereview.chromium.org/1179323002/diff/210001/content/browser/media/...
content/browser/media/capture/aura_window_capture_machine.cc:310:
media::PIXEL_STORAGE_TEXTURE) {
DCHECK(video_frame.get() || 
              capture_params_.requested_format.pixel_storage ==
media::PIXEL_STORAGE_TEXTURE) ?

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

https://codereview.chromium.org/1179323002/diff/210001/media/base/video_captu...
media/base/video_capture_types.cc:43: (pixel_format >= 0) && (pixel_format <
PIXEL_FORMAT_MAX) &&
move second comparison to a new line for less diffs?

[mcasas, 2015-06-23 22:33:54]

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

[mcasas, 2015-06-23 22:33:55]

dalecurtis@ PTAL/Owners RS

[miu, 2015-06-23 22:58:15]

https://codereview.chromium.org/1179323002/diff/210001/media/base/video_captu...
File media/base/video_capture_types.h (right):

https://codereview.chromium.org/1179323002/diff/210001/media/base/video_captu...
media/base/video_capture_types.h:108: VideoPixelStorage pixel_storage;
Should this extra field be added in
content/common/media/video_capture_messages.h (where the IPC struct is
declared)?

https://codereview.chromium.org/1179323002/diff/210001/media/capture/screen_c...
File media/capture/screen_capture_device_core.cc (right):

https://codereview.chromium.org/1179323002/diff/210001/media/capture/screen_c...
media/capture/screen_capture_device_core.cc:45:
params.requested_format.pixel_storage != PIXEL_STORAGE_TEXTURE) {
I think the logical-AND should be a logical-OR now, right?

https://codereview.chromium.org/1179323002/diff/210001/media/capture/screen_c...
media/capture/screen_capture_device_core.cc:47: "unsupported format: %d",
params.requested_format.pixel_format);
Please update error message to include storage texture, as well.

[mcasas, 2015-06-23 23:20:03]

Patchset #7 (id:250001) has been deleted

[mcasas, 2015-06-23 23:20:12]

Patchset #6 (id:230001) has been deleted

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

miu@ PTAL
(just addressed your comments and added a 
DCHECK, sorry for the rebase)

https://codereview.chromium.org/1179323002/diff/210001/media/base/video_captu...
File media/base/video_capture_types.h (right):

https://codereview.chromium.org/1179323002/diff/210001/media/base/video_captu...
media/base/video_capture_types.h:108: VideoPixelStorage pixel_storage;
On 2015/06/23 22:58:15, miu wrote:
> Should this extra field be added in
> content/common/media/video_capture_messages.h (where the IPC struct is
> declared)?

I don't understand, VideoPixelStorage is not
sent to Render process, stays local to Browser.

https://codereview.chromium.org/1179323002/diff/210001/media/capture/screen_c...
File media/capture/screen_capture_device_core.cc (right):

https://codereview.chromium.org/1179323002/diff/210001/media/capture/screen_c...
media/capture/screen_capture_device_core.cc:45:
params.requested_format.pixel_storage != PIXEL_STORAGE_TEXTURE) {
On 2015/06/23 22:58:15, miu wrote:
> I think the logical-AND should be a logical-OR now, right?

Done.

https://codereview.chromium.org/1179323002/diff/210001/media/capture/screen_c...
media/capture/screen_capture_device_core.cc:47: "unsupported format: %d",
params.requested_format.pixel_format);
On 2015/06/23 22:58:15, miu wrote:
> Please update error message to include storage texture, as well.

Done.

[hubbe, 2015-06-23 23:35:41]

lgtm

https://codereview.chromium.org/1179323002/diff/150001/media/base/video_captu...
File media/base/video_capture_types.h (right):

https://codereview.chromium.org/1179323002/diff/150001/media/base/video_captu...
media/base/video_capture_types.h:37: // Format are possible, though some are
very typical, such as texture + ARGB,
On 2015/06/19 02:58:36, mcasas wrote:
> On 2015/06/18 19:23:11, hubbe wrote:
> > I don't think this is clear enough / struct.
> > What does texture + MJPEG mean? Is it allowed? 
> 
> Same as with VideoFrame, the specification of 
> storage and pixel format are independent, so all
> combinations are meaningful although perhaps some
> are just not implemented. A texture MJPEG means
> exactly what is says: a formatless buffer allocated
> in the GPU (==texture) with data representing a
> video frame in MJPEG pixel format.
> 
> > Is the format ignored for
> > textures?
> > 
> No it isn't. But the only one used so far is ARGB.
> 

A texture is not a formatless buffer.
OpenGL doesn't give you direct access to the bytes of a texture, so it's not
possible to store arbitrary data in it.

[miu, 2015-06-23 23:35:55]

https://codereview.chromium.org/1179323002/diff/210001/media/base/video_captu...
File media/base/video_capture_types.h (right):

https://codereview.chromium.org/1179323002/diff/210001/media/base/video_captu...
media/base/video_capture_types.h:108: VideoPixelStorage pixel_storage;
On 2015/06/23 23:24:43, mcasas wrote:
> On 2015/06/23 22:58:15, miu wrote:
> > Should this extra field be added in
> > content/common/media/video_capture_messages.h (where the IPC struct is
> > declared)?
> 
> I don't understand, VideoPixelStorage is not
> sent to Render process, stays local to Browser.

I meant: There's a declaration of the VideoCaptureFormat struct.  I believe it's
used in the IPC messages sent from renderer to browser to start/resume the video
capture.

https://codereview.chromium.org/1179323002/diff/210001/media/capture/screen_c...
File media/capture/screen_capture_device_core.cc (right):

https://codereview.chromium.org/1179323002/diff/210001/media/capture/screen_c...
media/capture/screen_capture_device_core.cc:45:
params.requested_format.pixel_storage != PIXEL_STORAGE_TEXTURE) {
On 2015/06/23 23:24:43, mcasas wrote:
> On 2015/06/23 22:58:15, miu wrote:
> > I think the logical-AND should be a logical-OR now, right?
> 
> Done.

Oh wait.  I might have misled you here.  I think there are only two supported
format+storage combos: I420+STORAGE_CPU and ARGB+STORAGE_TEXTURE.  The logic
should probably be:

  if (!(params.requested_format.pixel_format == PIXEL_FORMAT_I420 &&
        params.requested_format.pixel_storage == PIXEL_STORAGE_CPU) &&
      !(params.requested_format.pixel_format == PIXEL_FORMAT_ARGB &&
        params.requested_format.pixel_storage == PIXEL_STORAGE_TEXTURE)) {
    ...do unsupported error...
    return;
  }

[mcasas, 2015-06-23 23:58:44]

On 2015/06/23 23:35:41, hubbe wrote:
> lgtm
> 
>
https://codereview.chromium.org/1179323002/diff/150001/media/base/video_captu...
> File media/base/video_capture_types.h (right):
> 
>
https://codereview.chromium.org/1179323002/diff/150001/media/base/video_captu...
> media/base/video_capture_types.h:37: // Format are possible, though some are
> very typical, such as texture + ARGB,
> On 2015/06/19 02:58:36, mcasas wrote:
> > On 2015/06/18 19:23:11, hubbe wrote:
> > > I don't think this is clear enough / struct.
> > > What does texture + MJPEG mean? Is it allowed? 
> > 
> > Same as with VideoFrame, the specification of 
> > storage and pixel format are independent, so all
> > combinations are meaningful although perhaps some
> > are just not implemented. A texture MJPEG means
> > exactly what is says: a formatless buffer allocated
> > in the GPU (==texture) with data representing a
> > video frame in MJPEG pixel format.
> > 
> > > Is the format ignored for
> > > textures?
> > > 
> > No it isn't. But the only one used so far is ARGB.
> > 
> 
> A texture is not a formatless buffer.
> OpenGL doesn't give you direct access to the bytes of a texture, so it's not
> possible to store arbitrary data in it.

That is true. The idea with VideoFrame is to support
pixel formats beyond the usual ARGB (or variations thereof)
of Textures. For instance, a YUY2 VideoFrame-texture, could 
be glUpload()Ed and transformed into a more classical 
openGL-ARGB texture via shaders. Notwithstanding the 
fact that some platforms support non-ARGB textures
as well.

[mcasas, 2015-06-24 00:16:39]

miu@ PTAL

https://codereview.chromium.org/1179323002/diff/210001/media/base/video_captu...
File media/base/video_capture_types.h (right):

https://codereview.chromium.org/1179323002/diff/210001/media/base/video_captu...
media/base/video_capture_types.h:108: VideoPixelStorage pixel_storage;
On 2015/06/23 23:35:55, miu wrote:
> On 2015/06/23 23:24:43, mcasas wrote:
> > On 2015/06/23 22:58:15, miu wrote:
> > > Should this extra field be added in
> > > content/common/media/video_capture_messages.h (where the IPC struct is
> > > declared)?
> > 
> > I don't understand, VideoPixelStorage is not
> > sent to Render process, stays local to Browser.
> 
> I meant: There's a declaration of the VideoCaptureFormat struct.  I believe
it's
> used in the IPC messages sent from renderer to browser to start/resume the
video
> capture.

Oh yes, media_param_traits.cc, good catch, thanks.

https://codereview.chromium.org/1179323002/diff/210001/media/capture/screen_c...
File media/capture/screen_capture_device_core.cc (right):

https://codereview.chromium.org/1179323002/diff/210001/media/capture/screen_c...
media/capture/screen_capture_device_core.cc:45:
params.requested_format.pixel_storage != PIXEL_STORAGE_TEXTURE) {
On 2015/06/23 23:35:55, miu wrote:
> On 2015/06/23 23:24:43, mcasas wrote:
> > On 2015/06/23 22:58:15, miu wrote:
> > > I think the logical-AND should be a logical-OR now, right?
> > 
> > Done.
> 
> Oh wait.  I might have misled you here.  I think there are only two supported
> format+storage combos: I420+STORAGE_CPU and ARGB+STORAGE_TEXTURE.  The logic
> should probably be:
> 
>   if (!(params.requested_format.pixel_format == PIXEL_FORMAT_I420 &&
>         params.requested_format.pixel_storage == PIXEL_STORAGE_CPU) &&
>       !(params.requested_format.pixel_format == PIXEL_FORMAT_ARGB &&
>         params.requested_format.pixel_storage == PIXEL_STORAGE_TEXTURE)) {
>     ...do unsupported error...
>     return;
>   }
> 

Done.

[miu, 2015-06-24 00:20:39]

lgtm

[DaleCurtis, 2015-06-24 00:25:28]

rs lgtm

[mcasas, 2015-06-24 00:34:17]

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

[mcasas, 2015-06-24 00:34:18]

inferno@ content/common/media/media_param_traits.cc PTAL

[mcasas, 2015-06-24 01:22:35]

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

[mcasas, 2015-06-24 01:22:36]

dcheng@ PTAL at content/common/media/media_param_traits.cc

[mcasas, 2015-06-24 03:07:43]

The CQ bit was checked by mcasas@chromium.org

[mcasas, 2015-06-24 03:07:44]

The patchset sent to the CQ was uploaded after l-g-t-m from hubbe@chromium.org
Link to the patchset: https://codereview.chromium.org/1179323002/#ps290001
(title: "miu@s comments")

[commit-bot: I haz the power, 2015-06-24 03:08:06]

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

[commit-bot: I haz the power, 2015-06-24 04:11:53]

Committed patchset #7 (id:290001)

[commit-bot: I haz the power, 2015-06-24 04:12:39]

Patchset 7 (id:??) landed as
https://crrev.com/957fb245c45052e2c4b2bb0f59e165ba05096904
Cr-Commit-Position: refs/heads/master@{#335872}

[dcheng, 2015-06-24 18:40:12]

Umm... is there any particular reason this had to be TBRed? I received this code
review at the end of the day yesterday, and was going to look at it today.

Please do not TBR IPC changes unless they are actually trivial. Trivial =
removing an IPC message or renaming it.

Adding fields may look trivial, but it often is not.

[dcheng, 2015-06-24 19:00:56]

A revert of this CL (patchset #7 id:290001) has been created in
https://codereview.chromium.org/1204843004/ by dcheng@chromium.org.

The reason for reverting is: The IPC changes in this patch were not reviewed..

[dcheng, 2015-06-24 19:01:14]

https://codereview.chromium.org/1179323002/diff/290001/content/browser/media/...
File content/browser/media/capture/aura_window_capture_machine.cc (right):

https://codereview.chromium.org/1179323002/diff/290001/content/browser/media/...
content/browser/media/capture/aura_window_capture_machine.cc:311:
DCHECK_EQ(capture_params_.requested_format.pixel_format,
DCHECK_EQ(expected, actual) is the convention.

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

https://codereview.chromium.org/1179323002/diff/290001/content/browser/render...
content/browser/renderer_host/media/video_capture_device_client.cc:219:
DCHECK_EQ(frame_format.pixel_storage, media::PIXEL_STORAGE_CPU);
Same comment: this should be DCHECK_EQ(actual, expected)

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

https://codereview.chromium.org/1179323002/diff/290001/content/common/media/m...
content/common/media/media_param_traits.cc:78: !iter->ReadInt(&pixel_storage))
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.

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

https://codereview.chromium.org/1179323002/diff/290001/media/base/video_captu...
media/base/video_capture_types.h:40: enum VideoPixelStorage {
enum class.