Remove dead code paths around PIXEL_STORAGE_TEXTURE in capture pipeline.

Remove dead code paths around PIXEL_STORAGE_TEXTURE in capture pipeline.

Removes code to capture a result as a GPU texture (i.e., tab/desktop
capture without read-back into main memory).  This code has no
activation path and is being removed to make future refactoring/clean-up
efforts go more smoothly.

This change also has a few minor clean-ups/comment changes in downstream
code, based on revealations during the code removal process.

BUG=552570
Committed: https://crrev.com/d3ec5867d26f67b821489476179ff1a9b9996d2a
Cr-Commit-Position: refs/heads/master@{#360021}

[miu, 2015-11-12 01:22:48]

Patchset #1 (id:1) has been deleted

[miu, 2015-11-12 01:22:59]

Patchset #1 (id:20001) has been deleted

[miu, 2015-11-12 01:24:23]

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

[miu, 2015-11-12 01:24:24]

dalecurtis: OWNERS stamp for media/base/* changes.

mcasas: PTAL at all changes.

[mcasas, 2015-11-13 19:13:57]

looks good, just a few minor comments, in 
general all in the line of explicitly DCHECK()ing
the expected combination pixel_format/storage: 
with this CL all VideoFrames coming from 'a' capture 
are I420 + ShMem/GMB after the 
VideoCaptureDeviceClient adaptation step 
(or earlier, in cases like screen/tab capture).

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

https://codereview.chromium.org/1439533004/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_buffer_pool.cc:264: storage ==
media::PIXEL_STORAGE_CPU);
Actually, these are all the cases now, remove this DCHECK()?

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

https://codereview.chromium.org/1439533004/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_buffer_pool_unittest.cc:14:
#include "cc/test/test_web_graphics_context_3d.h"
Probably remove these two includes.

https://codereview.chromium.org/1439533004/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_buffer_pool_unittest.cc:19:
#include "media/base/video_util.h"
Probably unused too.

https://codereview.chromium.org/1439533004/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_buffer_pool_unittest.cc:32:
{media::PIXEL_FORMAT_ARGB, media::PIXEL_STORAGE_CPU},
At the level of VCBP,  CPU storage should support all formats,
whereas GMB only is intended for I420. ARGB was explicited
here for Texture. I wonder if we could reflect that in these test
parameters?  Either that, or add provisions in 
VCBP::SharedMemTracker::Init() to limit it to I420.

Unclear to me, since now we support more than what we use
(VideoCaptureDeviceClient should only use I420 now).

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

https://codereview.chromium.org/1439533004/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.cc:347:
(frame->HasTextures() && frame->format() == media::PIXEL_FORMAT_ARGB))
Same here, nobody should produce HasTextures() anymore
(except perhaps FakeVCD and it should be adapted too in 
this CL).

https://codereview.chromium.org/1439533004/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.cc:430: if
(frame->HasTextures()) {
I think this if() is also unused if noone is going to
produce textures anymore (we can leave a
DCHECK(!frame->HasTextures()) if you want).

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

https://codereview.chromium.org/1439533004/diff/40001/media/base/video_captur...
media/base/video_capture_types.cc:60: (pixel_format > PIXEL_FORMAT_UNKNOWN &&
Actually there's at least one case of valid VideoFrame with
PIXEL_FORMAT_UNKNOWN [1], in CreateEOSFrame(). Suggest
here s/>/>=/.


[1]
https://code.google.com/p/chromium/codesearch#chromium/src/media/base/video_f...

https://codereview.chromium.org/1439533004/diff/40001/media/base/video_captur...
File media/base/video_capture_types.h (left):

https://codereview.chromium.org/1439533004/diff/40001/media/base/video_captur...
media/base/video_capture_types.h:27: PIXEL_STORAGE_TEXTURE,
Yay!

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

https://codereview.chromium.org/1439533004/diff/40001/media/base/video_frame....
media/base/video_frame.cc:833: // TODO(miu): This function should support any
frame format.
nit: bug for this? (It should be accompanied by unittest etc).

https://codereview.chromium.org/1439533004/diff/40001/media/capture/content/t...
File media/capture/content/thread_safe_capture_oracle.cc (right):

https://codereview.chromium.org/1439533004/diff/40001/media/capture/content/t...
media/capture/content/thread_safe_capture_oracle.cc:44: }
What about 

  DCHECK_EQ(PIXEL_FORMAT_I420, params_.requested_format.pixel_format);

I know this is enforced at least in screen_capture_device_core.cc
but well...

https://codereview.chromium.org/1439533004/diff/40001/media/capture/content/t...
media/capture/content/thread_safe_capture_oracle.cc:69: (visible_size.height() +
15) & ~15);
Suggest replacing these interesting bit operations with
#include "base/bits.h"
inline size_t Align(size_t size, size_t alignment);

so the line would read:

  const gfx::Size coded_size(base::Align(visible_size.width(), 16),
                             base::Align(visible_size.height(), 16));

[miu, 2015-11-14 03:43:48]

mcasas: PTAL.  Addressed your comments and also uncovered more dead code to
remove.  Also, you mentioned FakeVCD, but I didn't see any "texture" code in
there.  Are there actually any changes that need to be made there?

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

https://codereview.chromium.org/1439533004/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_buffer_pool.cc:264: storage ==
media::PIXEL_STORAGE_CPU);
On 2015/11/13 19:13:57, mcasas wrote:
> Actually, these are all the cases now, remove this DCHECK()?

Done.  Replaced with switch statement to ensure compiles will error out if the
enum is ever changed.

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

https://codereview.chromium.org/1439533004/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_buffer_pool_unittest.cc:14:
#include "cc/test/test_web_graphics_context_3d.h"
On 2015/11/13 19:13:57, mcasas wrote:
> Probably remove these two includes.

They seem to be used for testing GMBs (see lines 145-152).

https://codereview.chromium.org/1439533004/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_buffer_pool_unittest.cc:19:
#include "media/base/video_util.h"
On 2015/11/13 19:13:57, mcasas wrote:
> Probably unused too.

Done.

https://codereview.chromium.org/1439533004/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_buffer_pool_unittest.cc:32:
{media::PIXEL_FORMAT_ARGB, media::PIXEL_STORAGE_CPU},
On 2015/11/13 19:13:57, mcasas wrote:
> At the level of VCBP,  CPU storage should support all formats,
> whereas GMB only is intended for I420. ARGB was explicited
> here for Texture. I wonder if we could reflect that in these test
> parameters?  Either that, or add provisions in 
> VCBP::SharedMemTracker::Init() to limit it to I420.
> 
> Unclear to me, since now we support more than what we use
> (VideoCaptureDeviceClient should only use I420 now).

Good questions.  A few points:

1. There's nothing extra needed for the buffer pool to support ANY pixel format
(for the SHMEM case).  So, it's probably too pedantic to purposely exclude any.

2. Code that uses the buffer pool already stringently checks for I420 format, so
there's no risk of unintended use.

3. We should consider whether I420 should be the only supported format.  I know
of efforts where people would, for example, love to have I444 support to
eliminate color bleeding problems for certain applications.

So, I'll leave the ARGB+STORAGE_CPU case in-place for these tests for now.

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

https://codereview.chromium.org/1439533004/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.cc:347:
(frame->HasTextures() && frame->format() == media::PIXEL_FORMAT_ARGB))
On 2015/11/13 19:13:57, mcasas wrote:
> Same here, nobody should produce HasTextures() anymore
> (except perhaps FakeVCD and it should be adapted too in 
> this CL).

Done.  And, I made these checks a bit stricter to ensure client code doesn't
provide VideoFrames: 1) with an incorrect pixel format or storage type, and 2)
that are not actually backed by the buffer.

https://codereview.chromium.org/1439533004/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.cc:430: if
(frame->HasTextures()) {
On 2015/11/13 19:13:57, mcasas wrote:
> I think this if() is also unused if noone is going to
> produce textures anymore (we can leave a
> DCHECK(!frame->HasTextures()) if you want).

Deleted.

Also, I removed all the DCHECKs() here since this is a private method called
only by DoIncomingCapturedVideoFrameOnIOThread(), which itself does these
checks.

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

https://codereview.chromium.org/1439533004/diff/40001/media/base/video_captur...
media/base/video_capture_types.cc:60: (pixel_format > PIXEL_FORMAT_UNKNOWN &&
On 2015/11/13 19:13:57, mcasas wrote:
> Actually there's at least one case of valid VideoFrame with
> PIXEL_FORMAT_UNKNOWN [1], in CreateEOSFrame(). Suggest
> here s/>/>=/.
> 
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/base/video_f...

Done.

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

https://codereview.chromium.org/1439533004/diff/40001/media/base/video_frame....
media/base/video_frame.cc:833: // TODO(miu): This function should support any
frame format.
On 2015/11/13 19:13:57, mcasas wrote:
> nit: bug for this? (It should be accompanied by unittest etc).

Done.

https://codereview.chromium.org/1439533004/diff/40001/media/capture/content/t...
File media/capture/content/thread_safe_capture_oracle.cc (right):

https://codereview.chromium.org/1439533004/diff/40001/media/capture/content/t...
media/capture/content/thread_safe_capture_oracle.cc:44: }
On 2015/11/13 19:13:57, mcasas wrote:
> What about 
> 
>   DCHECK_EQ(PIXEL_FORMAT_I420, params_.requested_format.pixel_format);
> 
> I know this is enforced at least in screen_capture_device_core.cc
> but well...

Nothing operates on the pixels here, so I'd prefer to just plumb-through the
configured params_.requested_format.pixel_format.  In the future, both upstream
or downstream code may include more than just I420 support.  See my other
comment for details.

https://codereview.chromium.org/1439533004/diff/40001/media/capture/content/t...
media/capture/content/thread_safe_capture_oracle.cc:69: (visible_size.height() +
15) & ~15);
On 2015/11/13 19:13:57, mcasas wrote:
> Suggest replacing these interesting bit operations with
> #include "base/bits.h"
> inline size_t Align(size_t size, size_t alignment);
> 
> so the line would read:
> 
>   const gfx::Size coded_size(base::Align(visible_size.width(), 16),
>                              base::Align(visible_size.height(), 16));

Done.  Also, re-opened a new bug since the original one was fixed and verified
(but the solution was a hack that should have been revisited).

[DaleCurtis, 2015-11-16 17:09:33]

media/base lgtm

[mcasas, 2015-11-17 02:23:47]

LGTM with some minor comments around.

Big thanks!

https://codereview.chromium.org/1439533004/diff/80001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://codereview.chromium.org/1439533004/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.cc:156: << ", " <<
params.requested_format.frame_rate
Nit: Same as below, consider
media::VideoCaptureFormat::ToString(params.requested_format)
to just print it all.

https://codereview.chromium.org/1439533004/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.cc:175: << ",
pixel_storage=" << params.requested_format.pixel_storage;
Nit: Consider using [1] 
media::VideoCaptureFormat::ToString(params.requested_format) 

[1] 
https://code.google.com/p/chromium/codesearch#chromium/src/media/base/video_c...

https://codereview.chromium.org/1439533004/diff/80001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_device_client.cc (right):

https://codereview.chromium.org/1439533004/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_device_client.cc:316:
DCHECK(pixel_format == media::PIXEL_FORMAT_I420);
DCHECK_EQ(media::PIXEL_FORMAT_I420, pixel_format);

https://codereview.chromium.org/1439533004/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_device_client.cc:344:
DCHECK(frame_format.pixel_format == media::PIXEL_FORMAT_I420);
DCHECK_EQ(media::PIXEL_FORMAT_I420, frame_format.pixel_format);

https://codereview.chromium.org/1439533004/diff/80001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_host.cc (right):

https://codereview.chromium.org/1439533004/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_host.cc:95:
DCHECK_CURRENTLY_ON(BrowserThread::IO);
I understand that new pixel formats might come in 
the (near) future, but what about a

DCHECK_EQ(media::PIXEL_FORMAT_I420, video_frame->format());
here?

https://codereview.chromium.org/1439533004/diff/80001/media/capture/content/t...
File media/capture/content/thread_safe_capture_oracle.cc (right):

https://codereview.chromium.org/1439533004/diff/80001/media/capture/content/t...
media/capture/content/thread_safe_capture_oracle.cc:117:
DCHECK_EQ(params_.requested_format.pixel_storage, media::PIXEL_STORAGE_CPU);
nit: put first the expected value.

[miu, 2015-11-17 04:28:03]

https://codereview.chromium.org/1439533004/diff/80001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://codereview.chromium.org/1439533004/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.cc:156: << ", " <<
params.requested_format.frame_rate
On 2015/11/17 02:23:47, mcasas wrote:
> Nit: Same as below, consider
> media::VideoCaptureFormat::ToString(params.requested_format)
> to just print it all.

Done.

https://codereview.chromium.org/1439533004/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.cc:175: << ",
pixel_storage=" << params.requested_format.pixel_storage;
On 2015/11/17 02:23:47, mcasas wrote:
> Nit: Consider using [1] 
> media::VideoCaptureFormat::ToString(params.requested_format) 
> 
> [1] 
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/base/video_c...

Done.

https://codereview.chromium.org/1439533004/diff/80001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_device_client.cc (right):

https://codereview.chromium.org/1439533004/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_device_client.cc:316:
DCHECK(pixel_format == media::PIXEL_FORMAT_I420);
On 2015/11/17 02:23:47, mcasas wrote:
> DCHECK_EQ(media::PIXEL_FORMAT_I420, pixel_format);

Done.

https://codereview.chromium.org/1439533004/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_device_client.cc:344:
DCHECK(frame_format.pixel_format == media::PIXEL_FORMAT_I420);
On 2015/11/17 02:23:47, mcasas wrote:
> DCHECK_EQ(media::PIXEL_FORMAT_I420, frame_format.pixel_format);

Done.

https://codereview.chromium.org/1439533004/diff/80001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_host.cc (right):

https://codereview.chromium.org/1439533004/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_host.cc:95:
DCHECK_CURRENTLY_ON(BrowserThread::IO);
On 2015/11/17 02:23:47, mcasas wrote:
> I understand that new pixel formats might come in 
> the (near) future, but what about a
> 
> DCHECK_EQ(media::PIXEL_FORMAT_I420, video_frame->format());
> here?

Acknowledged.

I thought about this, but putting in the DCHECK would imply we should take
pixel_format out of the VideoCaptureMsg_BufferReady_Params struct.  I'd rather
not push any more code in the direction of "requires I420" than is absolutely
necessary.  ;-)

https://codereview.chromium.org/1439533004/diff/80001/media/capture/content/t...
File media/capture/content/thread_safe_capture_oracle.cc (right):

https://codereview.chromium.org/1439533004/diff/80001/media/capture/content/t...
media/capture/content/thread_safe_capture_oracle.cc:117:
DCHECK_EQ(params_.requested_format.pixel_storage, media::PIXEL_STORAGE_CPU);
On 2015/11/17 02:23:47, mcasas wrote:
> nit: put first the expected value.

Done.

[miu, 2015-11-17 04:28:57]

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

[miu, 2015-11-17 04:28:57]

wfh: Need OWNERS stamp for trivial change in
content/common/media/video_capture_messages.h.

[Will Harris, 2015-11-17 04:32:35]

On 2015/11/17 04:28:57, miu wrote:
> wfh: Need OWNERS stamp for trivial change in
> content/common/media/video_capture_messages.h.

Rubber lgtm for messages file.

[miu, 2015-11-17 05:06:04]

The CQ bit was checked by miu@chromium.org

[miu, 2015-11-17 05:06:04]

The patchset sent to the CQ was uploaded after l-g-t-m from mcasas@chromium.org,
dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/1439533004/#ps100001
(title: "mcasas's second round comments   REBASE")

[commit-bot: I haz the power, 2015-11-17 05:06:24]

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

[commit-bot: I haz the power, 2015-11-17 06:18:26]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2015-11-17 06:19:22]

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