Cleanup of video capture into GpuMemoryBuffer

Cleanup of video capture into GpuMemoryBuffer

This is CL #1 of 2 for cleaning up GpuMemoryBuffer usage for video
capture. This project is superseded by capture using mojo.

This CL cleans up:
- VideoPixelStorage type PIXEL_STORAGE_GPUMEMORYBUFFER
- GpuMemoryBufferTracker as a VideoCaptureBufferTracker
- GpuMemoryBufferBufferHandle as a VideoCaptureBufferHandle
- kUseGpuMemoryBuffersForCapture flag

Note that this leads to having enums and factories that only produce
shared memory. chfremer@ is going to take over that refactor.

BUG=440843, 653579
Committed: https://crrev.com/ba3ab95c91d96790a4d6276225e786d570260410
Cr-Commit-Position: refs/heads/master@{#423994}

[emircan, 2016-10-05 22:37:17]

Description was changed from

==========
cleanup storage

BUG=
==========

to

==========
Cleanup of VideoPixelStorage type PIXEL_STORAGE_GPUMEMORYBUFFER

BUG=
==========

[emircan, 2016-10-05 23:46:39]

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

[commit-bot: I haz the power, 2016-10-05 23:47:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[emircan, 2016-10-05 23:50:23]

Description was changed from

==========
Cleanup of VideoPixelStorage type PIXEL_STORAGE_GPUMEMORYBUFFER

BUG=
==========

to

==========
Cleanup of video capture into GpuMemoryBuffer

This is CL #1 of 2 for cleaning up GpuMemoryBuffer usage for video
capture. This project is superseded by capture using mojo.

This CL cleans up VideoPixelStorage type PIXEL_STORAGE_GPUMEMORYBUFFER
and 

BUG=440843,651897
==========

[emircan, 2016-10-05 23:50:23]

emircan@chromium.org changed reviewers:
+ chfremer@chromium.org, mcasas@chromium.org

[emircan, 2016-10-05 23:50:34]

Patchset #1 (id:1) has been deleted

[emircan, 2016-10-05 23:52:35]

Description was changed from

==========
Cleanup of video capture into GpuMemoryBuffer

This is CL #1 of 2 for cleaning up GpuMemoryBuffer usage for video
capture. This project is superseded by capture using mojo.

This CL cleans up VideoPixelStorage type PIXEL_STORAGE_GPUMEMORYBUFFER
and 

BUG=440843,651897
==========

to

==========
Cleanup of video capture into GpuMemoryBuffer

This is CL #1 of 2 for cleaning up GpuMemoryBuffer usage for video
capture. This project is superseded by capture using mojo.

This CL cleans up:
- VideoPixelStorage type PIXEL_STORAGE_GPUMEMORYBUFFER.
- GpuMemoryBufferTracker as a VideoCaptureBufferTracker
- GpuMemoryBufferBufferHandle as a VideoCaptureBufferHandle

Note that this leads to having enums and factories that only produce
shared memory. chfremer@ is going to take over that refactor.

BUG=440843,651897
==========

[emircan, 2016-10-06 00:07:51]

Description was changed from

==========
Cleanup of video capture into GpuMemoryBuffer

This is CL #1 of 2 for cleaning up GpuMemoryBuffer usage for video
capture. This project is superseded by capture using mojo.

This CL cleans up:
- VideoPixelStorage type PIXEL_STORAGE_GPUMEMORYBUFFER.
- GpuMemoryBufferTracker as a VideoCaptureBufferTracker
- GpuMemoryBufferBufferHandle as a VideoCaptureBufferHandle

Note that this leads to having enums and factories that only produce
shared memory. chfremer@ is going to take over that refactor.

BUG=440843,651897
==========

to

==========
Cleanup of video capture into GpuMemoryBuffer

This is CL #1 of 3 for cleaning up GpuMemoryBuffer usage for video
capture. This project is superseded by capture using mojo.

This CL cleans up:
- VideoPixelStorage type PIXEL_STORAGE_GPUMEMORYBUFFER.
- GpuMemoryBufferTracker as a VideoCaptureBufferTracker
- GpuMemoryBufferBufferHandle as a VideoCaptureBufferHandle

Note that this leads to having enums and factories that only produce
shared memory. chfremer@ is going to take over that refactor.

BUG=440843,651897
==========

[emircan, 2016-10-06 00:09:13]

Description was changed from

==========
Cleanup of video capture into GpuMemoryBuffer

This is CL #1 of 3 for cleaning up GpuMemoryBuffer usage for video
capture. This project is superseded by capture using mojo.

This CL cleans up:
- VideoPixelStorage type PIXEL_STORAGE_GPUMEMORYBUFFER.
- GpuMemoryBufferTracker as a VideoCaptureBufferTracker
- GpuMemoryBufferBufferHandle as a VideoCaptureBufferHandle

Note that this leads to having enums and factories that only produce
shared memory. chfremer@ is going to take over that refactor.

BUG=440843,651897
==========

to

==========
Cleanup of video capture into GpuMemoryBuffer

This is CL #1 of 2 for cleaning up GpuMemoryBuffer usage for video
capture. This project is superseded by capture using mojo.

This CL cleans up:
- VideoPixelStorage type PIXEL_STORAGE_GPUMEMORYBUFFER.
- GpuMemoryBufferTracker as a VideoCaptureBufferTracker
- GpuMemoryBufferBufferHandle as a VideoCaptureBufferHandle

Note that this leads to having enums and factories that only produce
shared memory. chfremer@ is going to take over that refactor.

BUG=440843,651897
==========

[commit-bot: I haz the power, 2016-10-06 01:23:55]

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

[commit-bot: I haz the power, 2016-10-06 01:23:57]

Dry run: This issue passed the CQ dry run.

[emircan, 2016-10-06 03:55:37]

PTAL.

[chfremer, 2016-10-06 16:45:01]

lgtm % comment

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

https://codereview.chromium.org/2398813002/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.cc:516: case
media::VideoFrame::STORAGE_GPU_MEMORY_BUFFERS: {
I think it is okay to keep the switch, since we are still keeping the enum. But
any reason why we cannot remove the complete case block for the GPU case?

[mcasas, 2016-10-06 18:31:15]

https://codereview.chromium.org/2398813002/diff/20001/content/browser/BUILD.gn
File content/browser/BUILD.gn (right):

https://codereview.chromium.org/2398813002/diff/20001/content/browser/BUILD.g...
content/browser/BUILD.gn:1: # Copyright 2014 The Chromium Authors. All rights
reserved.
Don't use https://crbug.com/651897 to track this
work, it would confuse it :)

Made a new bug for you: https://crbug.com/653579.

https://codereview.chromium.org/2398813002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/media/shared_memory_buffer_tracker.h (right):

https://codereview.chromium.org/2398813002/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/shared_memory_buffer_tracker.h:14: : public
media::VideoCaptureBufferTracker {
Should we fold SharedMemoryBufferTracker into
media::VideoCaptureBufferTracker, or keep that
one as base class for this guy and hypothetical 
imminent derived classes in //service/video_capture ?

Similar discussion applies to  SharedMemoryBufferHandle
and its parent media::VideoCaptureBufferHandle.

I'm OK with either but think it has to be made
explicit in this CL or, by preference, in the 
associated bug https://crbug.com/653579

https://codereview.chromium.org/2398813002/diff/20001/media/base/video_captur...
File media/base/video_capture_types.h (right):

https://codereview.chromium.org/2398813002/diff/20001/media/base/video_captur...
media/base/video_capture_types.h:24: // TODO(mcasas): http://crbug.com/504160
Consider making this an enum class.
Yeah, depending on the plans, add a TODO()
to extend or remove this enum.

[chfremer, 2016-10-06 19:25:07]

https://codereview.chromium.org/2398813002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/media/shared_memory_buffer_tracker.h (right):

https://codereview.chromium.org/2398813002/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/shared_memory_buffer_tracker.h:14: : public
media::VideoCaptureBufferTracker {
On 2016/10/06 18:31:15, mcasas wrote:
> Should we fold SharedMemoryBufferTracker into
> media::VideoCaptureBufferTracker, or keep that
> one as base class for this guy and hypothetical 
> imminent derived classes in //service/video_capture ?
> 
> Similar discussion applies to  SharedMemoryBufferHandle
> and its parent media::VideoCaptureBufferHandle.
> 
> I'm OK with either but think it has to be made
> explicit in this CL or, by preference, in the 
> associated bug https://crbug.com/653579

I would prefer to keep it as the base class, because the derived class in
//service_video_capture is not as hypothetical as it seems (it already exists in
my local branch).

[mcasas, 2016-10-07 00:56:16]

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

https://codereview.chromium.org/2398813002/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.cc:352: const
gpu::SyncToken& sync_token,
We can also remove |sync_token| and associated
logic from this file IIUC.

[emircan, 2016-10-07 01:05:44]

On 2016/10/07 00:56:16, mcasas wrote:
>
https://codereview.chromium.org/2398813002/diff/20001/content/browser/rendere...
> File content/browser/renderer_host/media/video_capture_controller.cc (right):
> 
>
https://codereview.chromium.org/2398813002/diff/20001/content/browser/rendere...
> content/browser/renderer_host/media/video_capture_controller.cc:352: const
> gpu::SyncToken& sync_token,
> We can also remove |sync_token| and associated
> logic from this file IIUC.

I am taking care of it in CL#2. I haven't finished yet, and I will ping you once
it is ready and I adress your commands here.

[mcasas, 2016-10-07 15:08:04]

On 2016/10/07 01:05:44, emircan wrote:
> On 2016/10/07 00:56:16, mcasas wrote:
> >
>
https://codereview.chromium.org/2398813002/diff/20001/content/browser/rendere...
> > File content/browser/renderer_host/media/video_capture_controller.cc
(right):
> > 
> >
>
https://codereview.chromium.org/2398813002/diff/20001/content/browser/rendere...
> > content/browser/renderer_host/media/video_capture_controller.cc:352: const
> > gpu::SyncToken& sync_token,
> > We can also remove |sync_token| and associated
> > logic from this file IIUC.
> 
> I am taking care of it in CL#2. I haven't finished yet, and I will ping you
once
> it is ready and I adress your commands here.

lgtm seeing CL #2, but please leave comments or
explain the bug when things are left half done.

[emircan, 2016-10-07 17:30:01]

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

[commit-bot: I haz the power, 2016-10-07 17:30:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[emircan, 2016-10-07 17:33:00]

Description was changed from

==========
Cleanup of video capture into GpuMemoryBuffer

This is CL #1 of 2 for cleaning up GpuMemoryBuffer usage for video
capture. This project is superseded by capture using mojo.

This CL cleans up:
- VideoPixelStorage type PIXEL_STORAGE_GPUMEMORYBUFFER.
- GpuMemoryBufferTracker as a VideoCaptureBufferTracker
- GpuMemoryBufferBufferHandle as a VideoCaptureBufferHandle

Note that this leads to having enums and factories that only produce
shared memory. chfremer@ is going to take over that refactor.

BUG=440843,651897
==========

to

==========
Cleanup of video capture into GpuMemoryBuffer

This is CL #1 of 2 for cleaning up GpuMemoryBuffer usage for video
capture. This project is superseded by capture using mojo.

This CL cleans up:
- VideoPixelStorage type PIXEL_STORAGE_GPUMEMORYBUFFER.
- GpuMemoryBufferTracker as a VideoCaptureBufferTracker
- GpuMemoryBufferBufferHandle as a VideoCaptureBufferHandle

Note that this leads to having enums and factories that only produce
shared memory. chfremer@ is going to take over that refactor.

BUG=440843,653579
==========

[emircan, 2016-10-07 17:39:30]

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

[commit-bot: I haz the power, 2016-10-07 17:39:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[emircan, 2016-10-07 17:40:12]

PTAL. Additionally, I removed kUseGpuMemoryBuffersForCapture flag and addressed
your comments.

https://codereview.chromium.org/2398813002/diff/20001/content/browser/BUILD.gn
File content/browser/BUILD.gn (right):

https://codereview.chromium.org/2398813002/diff/20001/content/browser/BUILD.g...
content/browser/BUILD.gn:1: # Copyright 2014 The Chromium Authors. All rights
reserved.
On 2016/10/06 18:31:15, mcasas wrote:
> Don't use https://crbug.com/651897 to track this
> work, it would confuse it :)
> 
> Made a new bug for you: https://crbug.com/653579.

Ok moving there. Thanks.

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

https://codereview.chromium.org/2398813002/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.cc:352: const
gpu::SyncToken& sync_token,
On 2016/10/07 00:56:16, mcasas wrote:
> We can also remove |sync_token| and associated
> logic from this file IIUC.

As I noted earlier I am cleaning up IPC in CL#2, and sync tokens come from
there, see https://codereview.chromium.org/2395913003/.

https://codereview.chromium.org/2398813002/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.cc:516: case
media::VideoFrame::STORAGE_GPU_MEMORY_BUFFERS: {
On 2016/10/06 16:45:00, chfremer wrote:
> I think it is okay to keep the switch, since we are still keeping the enum.
But
> any reason why we cannot remove the complete case block for the GPU case?

I am leaving cleaning up media::VideoFrame::STORAGE_GPU_MEMORY_BUFFERS storage
type in CL#2, see https://codereview.chromium.org/2395913003/.

https://codereview.chromium.org/2398813002/diff/20001/media/base/video_captur...
File media/base/video_capture_types.h (right):

https://codereview.chromium.org/2398813002/diff/20001/media/base/video_captur...
media/base/video_capture_types.h:24: // TODO(mcasas): http://crbug.com/504160
Consider making this an enum class.
On 2016/10/06 18:31:15, mcasas wrote:
> Yeah, depending on the plans, add a TODO()
> to extend or remove this enum.

Added a TODO for chfremer.

[emircan, 2016-10-07 17:41:42]

Description was changed from

==========
Cleanup of video capture into GpuMemoryBuffer

This is CL #1 of 2 for cleaning up GpuMemoryBuffer usage for video
capture. This project is superseded by capture using mojo.

This CL cleans up:
- VideoPixelStorage type PIXEL_STORAGE_GPUMEMORYBUFFER.
- GpuMemoryBufferTracker as a VideoCaptureBufferTracker
- GpuMemoryBufferBufferHandle as a VideoCaptureBufferHandle

Note that this leads to having enums and factories that only produce
shared memory. chfremer@ is going to take over that refactor.

BUG=440843,653579
==========

to

==========
Cleanup of video capture into GpuMemoryBuffer

This is CL #1 of 2 for cleaning up GpuMemoryBuffer usage for video
capture. This project is superseded by capture using mojo.

This CL cleans up:
- VideoPixelStorage type PIXEL_STORAGE_GPUMEMORYBUFFER
- GpuMemoryBufferTracker as a VideoCaptureBufferTracker
- GpuMemoryBufferBufferHandle as a VideoCaptureBufferHandle
- kUseGpuMemoryBuffersForCapture flag

Note that this leads to having enums and factories that only produce
shared memory. chfremer@ is going to take over that refactor.

BUG=440843,653579
==========

[emircan, 2016-10-07 17:46:17]

emircan@chromium.org changed reviewers:
+ avi@chromium.org, sandersd@chromium.org

[emircan, 2016-10-07 17:46:18]

Adding for OWNERs review

avi@ can you PTAL at content/browser/BUILD.gn?
sandersd@ can you PTAL at media/base/*?

[emircan, 2016-10-07 18:58:03]

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

[commit-bot: I haz the power, 2016-10-07 18:58:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[emircan, 2016-10-07 18:58:35]

Patchset #5 (id:100001) has been deleted

[commit-bot: I haz the power, 2016-10-07 19:00:46]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[emircan, 2016-10-07 21:02:03]

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

[commit-bot: I haz the power, 2016-10-07 21:02:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sandersd (OOO until July 31), 2016-10-07 22:35:31]

/media/base lgtm.

[Avi (use Gerrit), 2016-10-07 22:38:07]

On 2016/10/07 22:35:31, sandersd wrote:
> /media/base lgtm.

build file lgtm

[emircan, 2016-10-07 22:41:36]

The CQ bit was unchecked by emircan@chromium.org

[emircan, 2016-10-07 22:41:42]

The CQ bit was checked by emircan@chromium.org

[emircan, 2016-10-07 22:41:43]

The patchset sent to the CQ was uploaded after l-g-t-m from mcasas@chromium.org,
chfremer@chromium.org
Link to the patchset: https://codereview.chromium.org/2398813002/#ps120001
(title: "Rebase")

[commit-bot: I haz the power, 2016-10-07 22:42:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-07 22:51:33]

Description was changed from

==========
Cleanup of video capture into GpuMemoryBuffer

This is CL #1 of 2 for cleaning up GpuMemoryBuffer usage for video
capture. This project is superseded by capture using mojo.

This CL cleans up:
- VideoPixelStorage type PIXEL_STORAGE_GPUMEMORYBUFFER
- GpuMemoryBufferTracker as a VideoCaptureBufferTracker
- GpuMemoryBufferBufferHandle as a VideoCaptureBufferHandle
- kUseGpuMemoryBuffersForCapture flag

Note that this leads to having enums and factories that only produce
shared memory. chfremer@ is going to take over that refactor.

BUG=440843,653579
==========

to

==========
Cleanup of video capture into GpuMemoryBuffer

This is CL #1 of 2 for cleaning up GpuMemoryBuffer usage for video
capture. This project is superseded by capture using mojo.

This CL cleans up:
- VideoPixelStorage type PIXEL_STORAGE_GPUMEMORYBUFFER
- GpuMemoryBufferTracker as a VideoCaptureBufferTracker
- GpuMemoryBufferBufferHandle as a VideoCaptureBufferHandle
- kUseGpuMemoryBuffersForCapture flag

Note that this leads to having enums and factories that only produce
shared memory. chfremer@ is going to take over that refactor.

BUG=440843,653579
==========

[commit-bot: I haz the power, 2016-10-07 22:51:34]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2016-10-07 22:55:28]

Description was changed from

==========
Cleanup of video capture into GpuMemoryBuffer

This is CL #1 of 2 for cleaning up GpuMemoryBuffer usage for video
capture. This project is superseded by capture using mojo.

This CL cleans up:
- VideoPixelStorage type PIXEL_STORAGE_GPUMEMORYBUFFER
- GpuMemoryBufferTracker as a VideoCaptureBufferTracker
- GpuMemoryBufferBufferHandle as a VideoCaptureBufferHandle
- kUseGpuMemoryBuffersForCapture flag

Note that this leads to having enums and factories that only produce
shared memory. chfremer@ is going to take over that refactor.

BUG=440843,653579
==========

to

==========
Cleanup of video capture into GpuMemoryBuffer

This is CL #1 of 2 for cleaning up GpuMemoryBuffer usage for video
capture. This project is superseded by capture using mojo.

This CL cleans up:
- VideoPixelStorage type PIXEL_STORAGE_GPUMEMORYBUFFER
- GpuMemoryBufferTracker as a VideoCaptureBufferTracker
- GpuMemoryBufferBufferHandle as a VideoCaptureBufferHandle
- kUseGpuMemoryBuffersForCapture flag

Note that this leads to having enums and factories that only produce
shared memory. chfremer@ is going to take over that refactor.

BUG=440843,653579

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

[commit-bot: I haz the power, 2016-10-07 22:55:29]

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