Pass GpuMemoryBuffer backed VideoFrame from browser to renderer processes

Pass GpuMemoryBuffer backed VideoFrame from browser to renderer processes

This is a CL for Video Capture using GpuMemoryBuffers plan[0]. The plans for future CLs can be seen on [1].

[0] https://docs.google.com/document/d/1lobWf168_Kq05TLNLX81gRQwY5oe4k2t3oP-XTBEpIo/edit#heading=h.lflt7hoq3oui
[1] https://docs.google.com/document/d/1lobWf168_Kq05TLNLX81gRQwY5oe4k2t3oP-XTBEpIo/edit#heading=h.exy0h9917c1d

- Changed VideoCaptureBufferPool::ShareToProcess() method to handle sharing both SharedMemory and GpuMemoryBuffer backed video frames.
- Added VideoCaptureBufferPool::GpuMemoryBufferTracker::ShareToProcess() for sharing GMB backed buffers.
- Added dimensions() method to media::VideoCaptureDevice::Client::Buffer and VideoCaptureBufferPool::BufferHandle interfaces as required by GMB mapping. Also, renamed size() to mapped_size() to avoid confusion.
- Added OnGpuMemoryBufferCreated() calls between Browser and Renderer to notify the creation of GMB backed buffers and pass handles.
- Added VideoCaptureImpl::ClientGpuMemoryBuffer on Renderer side to create&map new GMB client buffers.

Also, buffer reuse and destruction is handled behind VideoCaptureDeviceClient::AutoReleaseBuffer like listed below:

Buffer Creation
*Browser
VideoCaptureController::DoIncomingCapturedVideoFrameOnIOThread
- ShareToProcess(Renderer)
VideoCaptureHost::OnBufferCreated
*Renderer
VideoCaptureMessageFilter::OnBufferCreated
VideoCaptureImpl::OnBufferCreated
- Adds to client_buffers_

Buffer Reuse
*Renderer
VideoCaptureImpl::OnClientBufferFinished
*Browser
VideoCaptureHost::OnRendererFinishedWithBuffer
VideoCaptureController::ReturnBuffer
VideoCaptureBufferPool::RelinquishConsumerHold
~AutoReleaseBuffer
VideoCaptureBufferPool::RelinquishProducerReservation

BUG=440843, 503835
TEST=
- Without the kUseGpuMemoryBuffersForCapture flag, AppRTC loopback runs as before.
- With the flag, AppRTC loopback crashes in VideoCaptureHost::OnBufferReady as expected.

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

[emircan, 2015-08-05 20:34:10]

Patchset #1 (id:1) has been deleted

[emircan, 2015-08-05 20:34:16]

Patchset #1 (id:20001) has been deleted

[emircan, 2015-08-05 22:01:42]

Patchset #1 (id:40001) has been deleted

[emircan, 2015-08-05 22:44:54]

Patchset #1 (id:60001) has been deleted

[emircan, 2015-08-06 01:38:47]

Patchset #1 (id:80001) has been deleted

[emircan, 2015-08-06 01:38:53]

Patchset #1 (id:90001) has been deleted

[emircan, 2015-08-06 01:42:51]

Patchset #1 (id:110001) has been deleted

[emircan, 2015-08-06 20:11:54]

Patchset #1 (id:130001) has been deleted

[emircan, 2015-08-19 01:28:12]

Patchset #2 (id:170001) has been deleted

[emircan, 2015-08-19 01:28:24]

Patchset #1 (id:150001) has been deleted

[emircan, 2015-08-19 01:28:37]

Patchset #1 (id:150001) has been deleted

[emircan, 2015-08-19 01:38:19]

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

[emircan, 2015-08-19 01:42:54]

PTAL.

reveman@, I am sharing GMB handles from GPU process to the Renderer on 
"content/browser/renderer_host/media/video_capture_buffer_pool.cc". And creating
and mapping GMBs on Renderer side on
"content/renderer/media/video_capture_impl.cc". Reusing and destroying the
buffers are handled behind VideoCaptureBufferPool functionality. Please take a
look and let me know of any problems.

[emircan, 2015-08-19 01:46:11]

Patchset #1 (id:190001) has been deleted

[emircan, 2015-08-19 01:49:45]

Patchset #1 (id:210001) has been deleted

[emircan, 2015-08-19 02:05:00]

Patchset #1 (id:230001) has been deleted

[emircan, 2015-08-19 02:47:05]

Patchset #1 (id:250001) has been deleted

[reveman, 2015-08-19 16:16:09]

A few high level questions.

- Do we need both the shared memory capture buffer and this new gpu memory
buffer backed capture buffer? Can we instead limit the complexity we're adding
by replacing the shared memory buffer with this new GMB backed capture buffer?
The shared memory capture buffer is afaict the same as a single RGBA GMB.

- In the case when we capture to GMBs, can we have the client create the
textures?

- Introducing something called a "GpuMemoryBuffer" that wraps multiple GMBs is
very confusing. Is there a better name for this? VideoCaptureBuffer?

[emircan, 2015-08-19 18:05:38]

Thanks David. My reviewers are finally online and I got the first CLs in, so I
am back in this. :)

On 2015/08/19 16:16:09, reveman wrote:
> A few high level questions.
> 
> - Do we need both the shared memory capture buffer and this new gpu memory
> buffer backed capture buffer? Can we instead limit the complexity we're adding
> by replacing the shared memory buffer with this new GMB backed capture buffer?
> The shared memory capture buffer is afaict the same as a single RGBA GMB.
I discussed with niklase@ about how we should plan this, and we decided to land
it behind kUseGpuMemoryBuffersForCapture flag and turn it on initially for a
percentage of users to get metrics and crash reports. Eventually, if everything
looks good, I will come back and replace the existing Shared Memory cases.
The shared memory case is a packed I420 memory. It ends up in
VideoResourceUpdater::CreateForSoftwarePlanes for texture upload. Also, between
Browser-Renderer-WebRTC-GPU, all the buffer sharing is based on shared memory
handles at this point.

> - In the case when we capture to GMBs, can we have the client create the
> textures?
Yes, the renderer client can create the textures as well. However, consider the
multiple tab capture case. We know that Browser owns the GMB buffers that
capture would be deployed to. If we upload textures on multiple clients,
wouldn't it be redundant?

> - Introducing something called a "GpuMemoryBuffer" that wraps multiple GMBs is
> very confusing. Is there a better name for this? VideoCaptureBuffer?
My bad, I just reused the names we had there earlier for DMABuf-GMBs.
VideoCaptureBuffer doesn't distinguish between SharedMem and GMB cases.
VideoCaptureGpuMemoryBuffer?

[reveman, 2015-08-20 11:08:55]

On 2015/08/19 at 18:05:38, emircan wrote:
> Thanks David. My reviewers are finally online and I got the first CLs in, so I
am back in this. :)
> 
> On 2015/08/19 16:16:09, reveman wrote:
> > A few high level questions.
> > 
> > - Do we need both the shared memory capture buffer and this new gpu memory
> > buffer backed capture buffer? Can we instead limit the complexity we're
adding
> > by replacing the shared memory buffer with this new GMB backed capture
buffer?
> > The shared memory capture buffer is afaict the same as a single RGBA GMB.
> I discussed with niklase@ about how we should plan this, and we decided to
land it behind kUseGpuMemoryBuffersForCapture flag and turn it on initially for
a percentage of users to get metrics and crash reports. Eventually, if
everything looks good, I will come back and replace the existing Shared Memory
cases.
> The shared memory case is a packed I420 memory. It ends up in
VideoResourceUpdater::CreateForSoftwarePlanes for texture upload. Also, between
Browser-Renderer-WebRTC-GPU, all the buffer sharing is based on shared memory
handles at this point.

Can we instead start by replacing shared memory case with a minimal RGBA8888 GMB
implementation? It would basically be the same as the shared memory case except
that it would use a GMB instead of base::SharedMemory for transfer. There's no
reason this would crash or cause issues as we would be doing the same as in the
shared memory case. I'm not a fan of keeping alternative code paths that in
practice provide the same functionality. 

> 
> > - In the case when we capture to GMBs, can we have the client create the
> > textures?
> Yes, the renderer client can create the textures as well. However, consider
the multiple tab capture case. We know that Browser owns the GMB buffers that
capture would be deployed to. If we upload textures on multiple clients,
wouldn't it be redundant?
> 
> > - Introducing something called a "GpuMemoryBuffer" that wraps multiple GMBs
is
> > very confusing. Is there a better name for this? VideoCaptureBuffer?
> My bad, I just reused the names we had there earlier for DMABuf-GMBs.
VideoCaptureBuffer doesn't distinguish between SharedMem and GMB cases.
VideoCaptureGpuMemoryBuffer?

That's also why I'd rather see us replace the shared mem case. VideoCaptureFrame
is maybe better as it will contain multiple buffers.

[emircan, 2015-08-20 17:36:44]

On 2015/08/20 11:08:55, reveman wrote:
> Can we instead start by replacing shared memory case with a minimal RGBA8888
GMB
> implementation? It would basically be the same as the shared memory case
except
> that it would use a GMB instead of base::SharedMemory for transfer. There's no
> reason this would crash or cause issues as we would be doing the same as in
the
> shared memory case. I'm not a fan of keeping alternative code paths that in
> practice provide the same functionality. 

I see, replacing the existing codepath would make things much easier for me as
well. However, SharedMemory case is a packed I420 buffer, as WebRTC, SW and HW
encoders expect that as input. It ends up in
VideoResourceUpdater::CreateForSoftwarePlanes for texture upload. I don't think
RGBA8888 would be an appropriate replacement. The most appropriate replacement
is 3 R_8 buffers representing I420 as I am using now. Also, I may use YUV_420
GMB as just the data holder, but no texture upload from that would cause another
copy.
Also, there are 4 more CLs for completing this change. If I was to replace the
existing path and not break the current functionality, I would need to have it
all in one massive CL. And, I wouldn't be sure about what parts are breaking
exactly in Hangouts. There, I agree with niklase@s usggestion of landing this
behind the flag.

[chromium-reviews, 2015-08-20 18:39:40]

I would recommend adding the GMB 3buffer next to the current ShMem case and
not replacing it from the onset. The new 3buffer GMB needs to be verified
via Finch experiments and whatnot, and the current capture path is full of
platform-dependent arcanes that would be a hell to stamp out if we try a
full-out replacement.

On 20 August 2015 at 10:36, <emircan@chromium.org> wrote:

> On 2015/08/20 11:08:55, reveman wrote:
>
>> Can we instead start by replacing shared memory case with a minimal
>> RGBA8888
>>
> GMB
>
>> implementation? It would basically be the same as the shared memory case
>>
> except
>
>> that it would use a GMB instead of base::SharedMemory for transfer.
>> There's no
>> reason this would crash or cause issues as we would be doing the same as
>> in
>>
> the
>
>> shared memory case. I'm not a fan of keeping alternative code paths that
>> in
>> practice provide the same functionality.
>>
>
> I see, replacing the existing codepath would make things much easier for
> me as
> well. However, SharedMemory case is a packed I420 buffer, as WebRTC, SW
> and HW
> encoders expect that as input. It ends up in
> VideoResourceUpdater::CreateForSoftwarePlanes for texture upload. I don't
> think
> RGBA8888 would be an appropriate replacement. The most appropriate
> replacement
> is 3 R_8 buffers representing I420 as I am using now. Also, I may use
> YUV_420
> GMB as just the data holder, but no texture upload from that would cause
> another
> copy.
> Also, there are 4 more CLs for completing this change. If I was to replace
> the
> existing path and not break the current functionality, I would need to
> have it
> all in one massive CL. And, I wouldn't be sure about what parts are
> breaking
> exactly in Hangouts. There, I agree with niklase@s usggestion of landing
> this
> behind the flag.
>
> https://codereview.chromium.org/1267883002/
>



-- 

Miguel Casas-Sanchez | Gatopardo del Software | ydog <ydog@google.com> /
mcasas@google.com | +1 (650) 603 1380

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

[reveman, 2015-08-20 18:56:30]

On 2015/08/20 at 17:36:44, emircan wrote:
> On 2015/08/20 11:08:55, reveman wrote:
> > Can we instead start by replacing shared memory case with a minimal RGBA8888
GMB
> > implementation? It would basically be the same as the shared memory case
except
> > that it would use a GMB instead of base::SharedMemory for transfer. There's
no
> > reason this would crash or cause issues as we would be doing the same as in
the
> > shared memory case. I'm not a fan of keeping alternative code paths that in
> > practice provide the same functionality. 
> 
> I see, replacing the existing codepath would make things much easier for me as
well. However, SharedMemory case is a packed I420 buffer, as WebRTC, SW and HW
encoders expect that as input. It ends up in
VideoResourceUpdater::CreateForSoftwarePlanes for texture upload. I don't think
RGBA8888 would be an appropriate replacement. The most appropriate replacement
is 3 R_8 buffers representing I420 as I am using now. Also, I may use YUV_420
GMB as just the data holder, but no texture upload from that would cause another
copy.
> Also, there are 4 more CLs for completing this change. If I was to replace the
existing path and not break the current functionality, I would need to have it
all in one massive CL. And, I wouldn't be sure about what parts are breaking
exactly in Hangouts. There, I agree with niklase@s usggestion of landing this
behind the flag.

Is I420 shm mem not the same as YUV420 GMB format? How would it be different to
use a GMB for transport compared to base shared memory?

[emircan, 2015-08-21 00:05:35]

Patchset #2 (id:290001) has been deleted

[emircan, 2015-08-21 00:23:22]

Patchset #3 (id:330001) has been deleted

[emircan, 2015-08-21 00:44:55]

Patchset #2 (id:310001) has been deleted

[emircan, 2015-08-21 01:14:45]

Patchset #2 (id:350001) has been deleted

[mcasas, 2015-08-21 03:57:25]

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

https://codereview.chromium.org/1267883002/diff/370001/content/browser/render...
content/browser/renderer_host/media/video_capture_buffer_pool.cc:238: const
auto& cur_gmb_handle = gpu_memory_buffers_[plane]->GetHandle();
current_gmb_handle?

https://codereview.chromium.org/1267883002/diff/370001/content/browser/render...
content/browser/renderer_host/media/video_capture_buffer_pool.cc:242: case
gfx::EMPTY_BUFFER: {
I don't think {} are needed in this or l.257-260 case,
only for l.246-254.

https://codereview.chromium.org/1267883002/diff/370001/content/browser/render...
content/browser/renderer_host/media/video_capture_buffer_pool.cc:260: }
default:
  NOTREACHED(); 

?

https://codereview.chromium.org/1267883002/diff/370001/content/browser/render...
content/browser/renderer_host/media/video_capture_buffer_pool.cc:298: if
(tracker->ShareToProcess(plane, process_handle, new_handle)) {
no {} needed

https://codereview.chromium.org/1267883002/diff/370001/content/browser/render...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://codereview.chromium.org/1267883002/diff/370001/content/browser/render...
content/browser/renderer_host/media/video_capture_controller.cc:339: } else if
(frame->IsMappable()) {
Please keep the DCHECKs for the case of opaque texture ARGB
that were before in l.325+326. And in the previous code, in that
case |buffer_id| was not inserted...

https://codereview.chromium.org/1267883002/diff/370001/content/browser/render...
File content/browser/renderer_host/media/video_capture_host.cc (right):

https://codereview.chromium.org/1267883002/diff/370001/content/browser/render...
content/browser/renderer_host/media/video_capture_host.cc:111: ++i)
Needs {}

https://codereview.chromium.org/1267883002/diff/370001/content/renderer/media...
File content/renderer/media/video_capture_impl.cc (right):

https://codereview.chromium.org/1267883002/diff/370001/content/renderer/media...
content/renderer/media/video_capture_impl.cc:46:
ClientBuffer(scoped_ptr<base::SharedMemory> buffer,
nit: Fits in one line.
   ClientBuffer(scoped_ptr<base::SharedMemory> buffer, size_t buffer_size)

https://codereview.chromium.org/1267883002/diff/370001/content/renderer/media...
content/renderer/media/video_capture_impl.cc:50: const
scoped_ptr<base::SharedMemory> buffer;
Shouldn't |buffer| and |buffer_size| be private and have const getters?

https://codereview.chromium.org/1267883002/diff/370001/content/renderer/media...
content/renderer/media/video_capture_impl.cc:74: uint8* data(int plane) { return
data_[plane]; }
const method? 
Here an l.75, 76.

https://codereview.chromium.org/1267883002/diff/370001/content/renderer/media...
content/renderer/media/video_capture_impl.cc:86: }
Insert blank line.

https://codereview.chromium.org/1267883002/diff/370001/content/renderer/media...
content/renderer/media/video_capture_impl.cc:99: void* data_ptr = NULL;
s/NULL/nullptr/

https://codereview.chromium.org/1267883002/diff/370001/content/renderer/media...
content/renderer/media/video_capture_impl.cc:232: base::SharedMemoryHandle
handle,
Revert to old indent.

https://codereview.chromium.org/1267883002/diff/370001/content/renderer/media...
File content/renderer/media/video_capture_impl.h (right):

https://codereview.chromium.org/1267883002/diff/370001/content/renderer/media...
content/renderer/media/video_capture_impl.h:43: class CONTENT_EXPORT
VideoCaptureImpl
final : public...?

https://codereview.chromium.org/1267883002/diff/370001/content/renderer/media...
content/renderer/media/video_capture_impl.h:92: void OnBufferCreated(
Don't change the indent.

https://codereview.chromium.org/1267883002/diff/370001/content/renderer/media...
File content/renderer/media/video_capture_message_filter.cc (right):

https://codereview.chromium.org/1267883002/diff/370001/content/renderer/media...
content/renderer/media/video_capture_message_filter.cc:129: Delegate* delegate =
find_delegate(device_id);
Delegate* const delegate

https://codereview.chromium.org/1267883002/diff/370001/content/renderer/media...
content/renderer/media/video_capture_message_filter.cc:133: Send(new
VideoCaptureHostMsg_BufferReady(device_id, buffer_id, 0, -1.0));
0? -1.0??

Proposal:
0 /* release_sync_point */, -1.0  /* consumer_resource_utilization */

Here and in l.117.

[emircan, 2015-08-21 21:09:57]

Patchset #3 (id:390001) has been deleted

[emircan, 2015-08-22 01:49:33]

Patchset #5 (id:450001) has been deleted

[emircan, 2015-08-22 03:13:27]

PTAL.

mcasas@, I addressed your comments in PS#3.
reveman@, I renamed the functions in PS#4 and removed multiple texture uploads
in PS#5. I will refactor and clean texture uploads from browser process in a
follow-up CL to keep things clean.

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

https://codereview.chromium.org/1267883002/diff/370001/content/browser/render...
content/browser/renderer_host/media/video_capture_buffer_pool.cc:238: const
auto& cur_gmb_handle = gpu_memory_buffers_[plane]->GetHandle();
On 2015/08/21 03:57:25, mcasas wrote:
> current_gmb_handle?

Done.

https://codereview.chromium.org/1267883002/diff/370001/content/browser/render...
content/browser/renderer_host/media/video_capture_buffer_pool.cc:242: case
gfx::EMPTY_BUFFER: {
On 2015/08/21 03:57:25, mcasas wrote:
> I don't think {} are needed in this or l.257-260 case,
> only for l.246-254.

Done.

https://codereview.chromium.org/1267883002/diff/370001/content/browser/render...
content/browser/renderer_host/media/video_capture_buffer_pool.cc:260: }
On 2015/08/21 03:57:25, mcasas wrote:
> default:
>   NOTREACHED(); 
> 
> ?

If new formats are added, it should throw a compile error, so people are aware
of this.

https://codereview.chromium.org/1267883002/diff/370001/content/browser/render...
content/browser/renderer_host/media/video_capture_buffer_pool.cc:298: if
(tracker->ShareToProcess(plane, process_handle, new_handle)) {
On 2015/08/21 03:57:24, mcasas wrote:
> no {} needed

Done.

https://codereview.chromium.org/1267883002/diff/370001/content/browser/render...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://codereview.chromium.org/1267883002/diff/370001/content/browser/render...
content/browser/renderer_host/media/video_capture_controller.cc:339: } else if
(frame->IsMappable()) {
On 2015/08/21 03:57:25, mcasas wrote:
> Please keep the DCHECKs for the case of opaque texture ARGB
> that were before in l.325+326. And in the previous code, in that
> case |buffer_id| was not inserted...

Done. I restructured this method to make it clear on the cases we handle.

https://codereview.chromium.org/1267883002/diff/370001/content/browser/render...
File content/browser/renderer_host/media/video_capture_host.cc (right):

https://codereview.chromium.org/1267883002/diff/370001/content/browser/render...
content/browser/renderer_host/media/video_capture_host.cc:111: ++i)
On 2015/08/21 03:57:25, mcasas wrote:
> Needs {}

Done.

https://codereview.chromium.org/1267883002/diff/370001/content/renderer/media...
File content/renderer/media/video_capture_impl.cc (right):

https://codereview.chromium.org/1267883002/diff/370001/content/renderer/media...
content/renderer/media/video_capture_impl.cc:46:
ClientBuffer(scoped_ptr<base::SharedMemory> buffer,
On 2015/08/21 03:57:25, mcasas wrote:
> nit: Fits in one line.
>    ClientBuffer(scoped_ptr<base::SharedMemory> buffer, size_t buffer_size)

Done.

https://codereview.chromium.org/1267883002/diff/370001/content/renderer/media...
content/renderer/media/video_capture_impl.cc:50: const
scoped_ptr<base::SharedMemory> buffer;
On 2015/08/21 03:57:25, mcasas wrote:
> Shouldn't |buffer| and |buffer_size| be private and have const getters?

Done.

https://codereview.chromium.org/1267883002/diff/370001/content/renderer/media...
content/renderer/media/video_capture_impl.cc:74: uint8* data(int plane) { return
data_[plane]; }
On 2015/08/21 03:57:25, mcasas wrote:
> const method? 
> Here an l.75, 76.

Done.

https://codereview.chromium.org/1267883002/diff/370001/content/renderer/media...
content/renderer/media/video_capture_impl.cc:86: }
On 2015/08/21 03:57:25, mcasas wrote:
> Insert blank line.

Done.

https://codereview.chromium.org/1267883002/diff/370001/content/renderer/media...
content/renderer/media/video_capture_impl.cc:99: void* data_ptr = NULL;
On 2015/08/21 03:57:25, mcasas wrote:
> s/NULL/nullptr/

Done.

https://codereview.chromium.org/1267883002/diff/370001/content/renderer/media...
content/renderer/media/video_capture_impl.cc:232: base::SharedMemoryHandle
handle,
On 2015/08/21 03:57:25, mcasas wrote:
> Revert to old indent.

Done.

https://codereview.chromium.org/1267883002/diff/370001/content/renderer/media...
File content/renderer/media/video_capture_impl.h (right):

https://codereview.chromium.org/1267883002/diff/370001/content/renderer/media...
content/renderer/media/video_capture_impl.h:43: class CONTENT_EXPORT
VideoCaptureImpl
On 2015/08/21 03:57:25, mcasas wrote:
> final : public...?

Tried, but the bots turn red as unit tests derive from it.

https://codereview.chromium.org/1267883002/diff/370001/content/renderer/media...
content/renderer/media/video_capture_impl.h:92: void OnBufferCreated(
On 2015/08/21 03:57:25, mcasas wrote:
> Don't change the indent.

Done.

[reveman, 2015-08-26 11:54:05]

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

https://codereview.chromium.org/1267883002/diff/470001/content/browser/render...
content/browser/renderer_host/media/video_capture_buffer_pool.cc:126:
process_handle, static_cast<base::SharedMemoryHandle*>(new_handle));
can you refactor this to avoid this static_cast and void* return value?

https://codereview.chromium.org/1267883002/diff/470001/content/browser/render...
content/browser/renderer_host/media/video_capture_buffer_pool.cc:261: return
true;
NOTREACHED() before this?

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

https://codereview.chromium.org/1267883002/diff/470001/content/browser/render...
content/browser/renderer_host/media/video_capture_controller.cc:313: CHECK(
why CHECK and not DCHECK?

https://codereview.chromium.org/1267883002/diff/470001/content/browser/render...
File
content/browser/renderer_host/media/video_capture_controller_event_handler.h
(right):

https://codereview.chromium.org/1267883002/diff/470001/content/browser/render...
content/browser/renderer_host/media/video_capture_controller_event_handler.h:39:
const std::vector<gfx::GpuMemoryBufferHandle>& gmb_handles,
nit: s/gmb_handles/handles/ to avoid abbreviations

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

https://codereview.chromium.org/1267883002/diff/470001/content/browser/render...
content/browser/renderer_host/media/video_capture_controller_unittest.cc:54:
MOCK_METHOD1(DoGpuMemoryBufferCreated, void(VideoCaptureControllerID));
DoBufferCreated2?

https://codereview.chromium.org/1267883002/diff/470001/content/browser/render...
content/browser/renderer_host/media/video_capture_controller_unittest.cc:73:
const std::vector<gfx::GpuMemoryBufferHandle>& gmb_handles,
nit: s/gmb_handles/handles/

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

https://codereview.chromium.org/1267883002/diff/470001/content/browser/render...
content/browser/renderer_host/media/video_capture_device_client.cc:358: // 
DCHECK_GE(static_cast<size_t>(length), frame_format.ImageAllocationSize());
why comment this out?

https://codereview.chromium.org/1267883002/diff/470001/content/browser/render...
File content/browser/renderer_host/media/video_capture_host.cc (right):

https://codereview.chromium.org/1267883002/diff/470001/content/browser/render...
content/browser/renderer_host/media/video_capture_host.cc:71: const
std::vector<gfx::GpuMemoryBufferHandle>& gmb_handles,
nit: s/gmb_handles/handles/

https://codereview.chromium.org/1267883002/diff/470001/content/browser/render...
content/browser/renderer_host/media/video_capture_host.cc:75: if
(entries_.find(controller_id) == entries_.end())
can this be a DCHECK?

https://codereview.chromium.org/1267883002/diff/470001/content/browser/render...
File content/browser/renderer_host/media/video_capture_host.h (right):

https://codereview.chromium.org/1267883002/diff/470001/content/browser/render...
content/browser/renderer_host/media/video_capture_host.h:87: const
std::vector<gfx::GpuMemoryBufferHandle>& gmb_handles,
nit: s/gmb_handles/handles/

https://codereview.chromium.org/1267883002/diff/470001/content/browser/render...
File content/browser/renderer_host/media/video_capture_manager_unittest.cc
(right):

https://codereview.chromium.org/1267883002/diff/470001/content/browser/render...
content/browser/renderer_host/media/video_capture_manager_unittest.cc:53: const
std::vector<gfx::GpuMemoryBufferHandle>& gmb_handles,
nit: s/gmb_handles/handles/

https://codereview.chromium.org/1267883002/diff/470001/content/common/media/v...
File content/common/media/video_capture_messages.h (right):

https://codereview.chromium.org/1267883002/diff/470001/content/common/media/v...
content/common/media/video_capture_messages.h:67:
std::vector<gfx::GpuMemoryBufferHandle> /* gmb_handles */,
nit: s/gmb_handles/handles/

https://codereview.chromium.org/1267883002/diff/470001/content/renderer/media...
File content/renderer/media/video_capture_impl.cc (right):

https://codereview.chromium.org/1267883002/diff/470001/content/renderer/media...
content/renderer/media/video_capture_impl.cc:65: class
VideoCaptureImpl::ClientVideoCaptureBuffer
ClientBuffer2?

https://codereview.chromium.org/1267883002/diff/470001/content/renderer/media...
content/renderer/media/video_capture_impl.cc:69: const
std::vector<gfx::GpuMemoryBufferHandle>& client_gmb_handles,
nit: s/gmb_handles/handles/

btw, why does this take some handles while the ::ClientBuffer impl above take
instances?

https://codereview.chromium.org/1267883002/diff/470001/content/renderer/media...
content/renderer/media/video_capture_impl.cc:91: void Init() {
why the Init function instead of doing this in the ctor?

https://codereview.chromium.org/1267883002/diff/470001/content/renderer/media...
content/renderer/media/video_capture_impl.cc:111: const
std::vector<gfx::GpuMemoryBufferHandle> gmb_handles_;
nit: s/gmb_handles/handles/

https://codereview.chromium.org/1267883002/diff/470001/content/renderer/media...
content/renderer/media/video_capture_impl.cc:113: const media::VideoPixelFormat
format_;
this member doesn't seem useful. Init is hardcoded for a specific format and
changing this to something other than I420 will not work. I think it's better to
hardcode this in Init as well.

https://codereview.chromium.org/1267883002/diff/470001/content/renderer/media...
content/renderer/media/video_capture_impl.cc:114: // Owned instances of GMBs.
nit: unnecessary comment. Owned is implied from ScopedVector container.

https://codereview.chromium.org/1267883002/diff/470001/content/renderer/media...
content/renderer/media/video_capture_impl.cc:115:
ScopedVector<gfx::GpuMemoryBuffer> gmbs_;
nit: s/gmbs_/buffers_/ or gpu_memory_buffers_

https://codereview.chromium.org/1267883002/diff/470001/content/renderer/media...
content/renderer/media/video_capture_impl.cc:260: const
std::vector<gfx::GpuMemoryBufferHandle>& gmb_handles,
nit: s/gmb_handles/handles/

https://codereview.chromium.org/1267883002/diff/470001/content/renderer/media...
content/renderer/media/video_capture_impl.cc:287: } else if (cgmb_iter !=
client_video_capture_buffers_.end()) {
hm, why not a OnBufferDestroyed2 instead?

https://codereview.chromium.org/1267883002/diff/470001/content/renderer/media...
File content/renderer/media/video_capture_impl.h (right):

https://codereview.chromium.org/1267883002/diff/470001/content/renderer/media...
content/renderer/media/video_capture_impl.h:89: class ClientVideoCaptureBuffer;
ClientBuffer2? I think the existing comment is sufficient. GMBs are shared
memory..

https://codereview.chromium.org/1267883002/diff/470001/content/renderer/media...
content/renderer/media/video_capture_impl.h:96: const
std::vector<gfx::GpuMemoryBufferHandle>& gmb_handles,
nit: s/gmb_handles/handles/

https://codereview.chromium.org/1267883002/diff/470001/content/renderer/media...
content/renderer/media/video_capture_impl.h:108: const gpu::MailboxHolder&
mailbox_holders) override;
why "holders??

https://codereview.chromium.org/1267883002/diff/470001/content/renderer/media...
File content/renderer/media/video_capture_message_filter.cc (right):

https://codereview.chromium.org/1267883002/diff/470001/content/renderer/media...
content/renderer/media/video_capture_message_filter.cc:126: const
std::vector<gfx::GpuMemoryBufferHandle>& gmb_handles,
nit: s/gmb_handles/handles/

https://codereview.chromium.org/1267883002/diff/470001/content/renderer/media...
File content/renderer/media/video_capture_message_filter_unittest.cc (right):

https://codereview.chromium.org/1267883002/diff/470001/content/renderer/media...
content/renderer/media/video_capture_message_filter_unittest.cc:36: void(const
std::vector<gfx::GpuMemoryBufferHandle>& gmb_handles,
nit: s/gmb_handles/handles/

[emircan, 2015-08-26 21:23:12]

PTAL

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

https://codereview.chromium.org/1267883002/diff/470001/content/browser/render...
content/browser/renderer_host/media/video_capture_buffer_pool.cc:126:
process_handle, static_cast<base::SharedMemoryHandle*>(new_handle));
On 2015/08/26 11:54:04, reveman wrote:
> can you refactor this to avoid this static_cast and void* return value?

Done.

https://codereview.chromium.org/1267883002/diff/470001/content/browser/render...
content/browser/renderer_host/media/video_capture_buffer_pool.cc:261: return
true;
On 2015/08/26 11:54:04, reveman wrote:
> NOTREACHED() before this?

Done.

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

https://codereview.chromium.org/1267883002/diff/470001/content/browser/render...
content/browser/renderer_host/media/video_capture_controller.cc:313: CHECK(
On 2015/08/26 11:54:04, reveman wrote:
> why CHECK and not DCHECK?

I was keeping the old CHECK that was there, but there isn't any reason to keep
that as far as I see as the cases inside cover that at this point. I am changing
to DCHECK.

https://codereview.chromium.org/1267883002/diff/470001/content/browser/render...
File
content/browser/renderer_host/media/video_capture_controller_event_handler.h
(right):

https://codereview.chromium.org/1267883002/diff/470001/content/browser/render...
content/browser/renderer_host/media/video_capture_controller_event_handler.h:39:
const std::vector<gfx::GpuMemoryBufferHandle>& gmb_handles,
On 2015/08/26 11:54:04, reveman wrote:
> nit: s/gmb_handles/handles/ to avoid abbreviations

Done.

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

https://codereview.chromium.org/1267883002/diff/470001/content/browser/render...
content/browser/renderer_host/media/video_capture_controller_unittest.cc:54:
MOCK_METHOD1(DoGpuMemoryBufferCreated, void(VideoCaptureControllerID));
On 2015/08/26 11:54:04, reveman wrote:
> DoBufferCreated2?

Done.

https://codereview.chromium.org/1267883002/diff/470001/content/browser/render...
content/browser/renderer_host/media/video_capture_controller_unittest.cc:73:
const std::vector<gfx::GpuMemoryBufferHandle>& gmb_handles,
On 2015/08/26 11:54:04, reveman wrote:
> nit: s/gmb_handles/handles/

Done.

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

https://codereview.chromium.org/1267883002/diff/470001/content/browser/render...
content/browser/renderer_host/media/video_capture_device_client.cc:358: // 
DCHECK_GE(static_cast<size_t>(length), frame_format.ImageAllocationSize());
On 2015/08/26 11:54:04, reveman wrote:
> why comment this out?

My bad, leftover from crbug.com/520740.

https://codereview.chromium.org/1267883002/diff/470001/content/browser/render...
File content/browser/renderer_host/media/video_capture_host.cc (right):

https://codereview.chromium.org/1267883002/diff/470001/content/browser/render...
content/browser/renderer_host/media/video_capture_host.cc:71: const
std::vector<gfx::GpuMemoryBufferHandle>& gmb_handles,
On 2015/08/26 11:54:04, reveman wrote:
> nit: s/gmb_handles/handles/

Done.

https://codereview.chromium.org/1267883002/diff/470001/content/browser/render...
content/browser/renderer_host/media/video_capture_host.cc:75: if
(entries_.find(controller_id) == entries_.end())
On 2015/08/26 11:54:04, reveman wrote:
> can this be a DCHECK?

I read through and it all goes down to this CL [0], and it looks like they are
trying to avoid some race conditions. I will try this in a seperate CL and see
how bots react.

[0] https://chromiumcodereview.appspot.com/10537057

https://codereview.chromium.org/1267883002/diff/470001/content/browser/render...
File content/browser/renderer_host/media/video_capture_host.h (right):

https://codereview.chromium.org/1267883002/diff/470001/content/browser/render...
content/browser/renderer_host/media/video_capture_host.h:87: const
std::vector<gfx::GpuMemoryBufferHandle>& gmb_handles,
On 2015/08/26 11:54:04, reveman wrote:
> nit: s/gmb_handles/handles/

Done.

https://codereview.chromium.org/1267883002/diff/470001/content/browser/render...
File content/browser/renderer_host/media/video_capture_manager_unittest.cc
(right):

https://codereview.chromium.org/1267883002/diff/470001/content/browser/render...
content/browser/renderer_host/media/video_capture_manager_unittest.cc:53: const
std::vector<gfx::GpuMemoryBufferHandle>& gmb_handles,
On 2015/08/26 11:54:04, reveman wrote:
> nit: s/gmb_handles/handles/

Done.

https://codereview.chromium.org/1267883002/diff/470001/content/common/media/v...
File content/common/media/video_capture_messages.h (right):

https://codereview.chromium.org/1267883002/diff/470001/content/common/media/v...
content/common/media/video_capture_messages.h:67:
std::vector<gfx::GpuMemoryBufferHandle> /* gmb_handles */,
On 2015/08/26 11:54:04, reveman wrote:
> nit: s/gmb_handles/handles/

Done.

https://codereview.chromium.org/1267883002/diff/470001/content/renderer/media...
File content/renderer/media/video_capture_impl.cc (right):

https://codereview.chromium.org/1267883002/diff/470001/content/renderer/media...
content/renderer/media/video_capture_impl.cc:65: class
VideoCaptureImpl::ClientVideoCaptureBuffer
On 2015/08/26 11:54:05, reveman wrote:
> ClientBuffer2?

Done.

https://codereview.chromium.org/1267883002/diff/470001/content/renderer/media...
content/renderer/media/video_capture_impl.cc:69: const
std::vector<gfx::GpuMemoryBufferHandle>& client_gmb_handles,
On 2015/08/26 11:54:05, reveman wrote:
> nit: s/gmb_handles/handles/
> 
> btw, why does this take some handles while the ::ClientBuffer impl above take
> instances?

::ClientBuffer relies on ShMem being mapped earlier[0], and the destructor does
the unmap for it [1]. I wanted to have the Map&Unmap clearly stated in the new
::ClientBuffer2.

[0]
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...
[1]
https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/shared...

https://codereview.chromium.org/1267883002/diff/470001/content/renderer/media...
content/renderer/media/video_capture_impl.cc:91: void Init() {
On 2015/08/26 11:54:05, reveman wrote:
> why the Init function instead of doing this in the ctor?

Done. Moved Init() to ctor.

https://codereview.chromium.org/1267883002/diff/470001/content/renderer/media...
content/renderer/media/video_capture_impl.cc:111: const
std::vector<gfx::GpuMemoryBufferHandle> gmb_handles_;
On 2015/08/26 11:54:05, reveman wrote:
> nit: s/gmb_handles/handles/

Done.

https://codereview.chromium.org/1267883002/diff/470001/content/renderer/media...
content/renderer/media/video_capture_impl.cc:113: const media::VideoPixelFormat
format_;
On 2015/08/26 11:54:05, reveman wrote:
> this member doesn't seem useful. Init is hardcoded for a specific format and
> changing this to something other than I420 will not work. I think it's better
to
> hardcode this in Init as well.

Done.

https://codereview.chromium.org/1267883002/diff/470001/content/renderer/media...
content/renderer/media/video_capture_impl.cc:114: // Owned instances of GMBs.
On 2015/08/26 11:54:05, reveman wrote:
> nit: unnecessary comment. Owned is implied from ScopedVector container.

Done.

https://codereview.chromium.org/1267883002/diff/470001/content/renderer/media...
content/renderer/media/video_capture_impl.cc:115:
ScopedVector<gfx::GpuMemoryBuffer> gmbs_;
On 2015/08/26 11:54:05, reveman wrote:
> nit: s/gmbs_/buffers_/ or gpu_memory_buffers_

Done.

https://codereview.chromium.org/1267883002/diff/470001/content/renderer/media...
content/renderer/media/video_capture_impl.cc:260: const
std::vector<gfx::GpuMemoryBufferHandle>& gmb_handles,
On 2015/08/26 11:54:05, reveman wrote:
> nit: s/gmb_handles/handles/

Done.

https://codereview.chromium.org/1267883002/diff/470001/content/renderer/media...
content/renderer/media/video_capture_impl.cc:287: } else if (cgmb_iter !=
client_video_capture_buffers_.end()) {
On 2015/08/26 11:54:05, reveman wrote:
> hm, why not a OnBufferDestroyed2 instead?

Browser sends the buffer_id and we do not exactly know if it is GMB or shmem
backed [0]. I refactored the class. 

[0]
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...

https://codereview.chromium.org/1267883002/diff/470001/content/renderer/media...
File content/renderer/media/video_capture_impl.h (right):

https://codereview.chromium.org/1267883002/diff/470001/content/renderer/media...
content/renderer/media/video_capture_impl.h:89: class ClientVideoCaptureBuffer;
On 2015/08/26 11:54:05, reveman wrote:
> ClientBuffer2? I think the existing comment is sufficient. GMBs are shared
> memory..

Done.

https://codereview.chromium.org/1267883002/diff/470001/content/renderer/media...
content/renderer/media/video_capture_impl.h:96: const
std::vector<gfx::GpuMemoryBufferHandle>& gmb_handles,
On 2015/08/26 11:54:05, reveman wrote:
> nit: s/gmb_handles/handles/

Done.

https://codereview.chromium.org/1267883002/diff/470001/content/renderer/media...
content/renderer/media/video_capture_impl.h:108: const gpu::MailboxHolder&
mailbox_holders) override;
On 2015/08/26 11:54:05, reveman wrote:
> why "holders??

Done.

https://codereview.chromium.org/1267883002/diff/470001/content/renderer/media...
File content/renderer/media/video_capture_message_filter.cc (right):

https://codereview.chromium.org/1267883002/diff/470001/content/renderer/media...
content/renderer/media/video_capture_message_filter.cc:126: const
std::vector<gfx::GpuMemoryBufferHandle>& gmb_handles,
On 2015/08/26 11:54:05, reveman wrote:
> nit: s/gmb_handles/handles/

Done.

https://codereview.chromium.org/1267883002/diff/470001/content/renderer/media...
File content/renderer/media/video_capture_message_filter_unittest.cc (right):

https://codereview.chromium.org/1267883002/diff/470001/content/renderer/media...
content/renderer/media/video_capture_message_filter_unittest.cc:36: void(const
std::vector<gfx::GpuMemoryBufferHandle>& gmb_handles,
On 2015/08/26 11:54:05, reveman wrote:
> nit: s/gmb_handles/handles/

Done.

[reveman, 2015-08-27 00:21:24]

lgtm

https://codereview.chromium.org/1267883002/diff/470001/content/browser/render...
File content/browser/renderer_host/media/video_capture_host.cc (right):

https://codereview.chromium.org/1267883002/diff/470001/content/browser/render...
content/browser/renderer_host/media/video_capture_host.cc:75: if
(entries_.find(controller_id) == entries_.end())
On 2015/08/26 at 21:23:11, emircan wrote:
> On 2015/08/26 11:54:04, reveman wrote:
> > can this be a DCHECK?
> 
> I read through and it all goes down to this CL [0], and it looks like they are
trying to avoid some race conditions. I will try this in a seperate CL and see
how bots react.
> 
> [0] https://chromiumcodereview.appspot.com/10537057

Sounds good. Thanks.

[mcasas, 2015-08-27 18:26:56]

LGTM % comments/questions

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

https://codereview.chromium.org/1267883002/diff/490001/content/browser/render...
content/browser/renderer_host/media/video_capture_buffer_pool.cc:248:
gfx::GpuMemoryBufferHandle* const new_gmb_handle =
Remove |new_gmb_handle| as talked offline.

https://codereview.chromium.org/1267883002/diff/490001/content/browser/render...
content/browser/renderer_host/media/video_capture_buffer_pool.cc:261:
new_gmb_handle->type = gfx::SHARED_MEMORY_BUFFER;
When |new_gmb_handle| is removed, should this
change to DCHECK(), or still we'd have to set
it here?

https://codereview.chromium.org/1267883002/diff/490001/content/browser/render...
content/browser/renderer_host/media/video_capture_buffer_pool.cc:312: bool
VideoCaptureBufferPool::ShareToProcess2(
See my comments in video_capture_controller.cc
about this signature (and VCDC correspondent).

https://codereview.chromium.org/1267883002/diff/490001/content/browser/render...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://codereview.chromium.org/1267883002/diff/490001/content/browser/render...
content/browser/renderer_host/media/video_capture_controller.cc:313: DCHECK(
Why the change to DCHECK() (and reformatting)?

https://codereview.chromium.org/1267883002/diff/490001/content/browser/render...
content/browser/renderer_host/media/video_capture_controller.cc:335:
buffer_pool_->ShareToProcess2(
Since you're adding ShareToProcess2(), why
not make it accept a 
const std::vector<gfx::GpuMemoryBufferHandle*>&

and do the for loop inside? 
That way it'd look a a bit more like 
OnBufferCreated2() that works with vectors.

https://codereview.chromium.org/1267883002/diff/490001/content/browser/render...
content/browser/renderer_host/media/video_capture_controller.cc:385:
buffer_pool_->HoldForConsumers(buffer_id, count);
nit: not your fault, but this |count| is just further
cluttering this method.

s/count/controller_clients_.size()/  ?
(and remove |count| of course)

[emircan, 2015-08-27 22:01:06]

Thanks.

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

https://codereview.chromium.org/1267883002/diff/490001/content/browser/render...
content/browser/renderer_host/media/video_capture_buffer_pool.cc:248:
gfx::GpuMemoryBufferHandle* const new_gmb_handle =
On 2015/08/27 18:26:55, mcasas wrote:
> Remove |new_gmb_handle| as talked offline.

Done.

https://codereview.chromium.org/1267883002/diff/490001/content/browser/render...
content/browser/renderer_host/media/video_capture_buffer_pool.cc:261:
new_gmb_handle->type = gfx::SHARED_MEMORY_BUFFER;
On 2015/08/27 18:26:55, mcasas wrote:
> When |new_gmb_handle| is removed, should this
> change to DCHECK(), or still we'd have to set
> it here?

We still need to set it because |new_handle| comes empty.

https://codereview.chromium.org/1267883002/diff/490001/content/browser/render...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://codereview.chromium.org/1267883002/diff/490001/content/browser/render...
content/browser/renderer_host/media/video_capture_controller.cc:313: DCHECK(
On 2015/08/27 18:26:55, mcasas wrote:
> Why the change to DCHECK() (and reformatting)?

reveman@ pointed it out on PS#6. Do you think we should check that for every
frame? |is_new_buffer| covers all these cases and makes sure any other pixel
type buffers are not created.

https://codereview.chromium.org/1267883002/diff/490001/content/browser/render...
content/browser/renderer_host/media/video_capture_controller.cc:335:
buffer_pool_->ShareToProcess2(
On 2015/08/27 18:26:55, mcasas wrote:
> Since you're adding ShareToProcess2(), why
> not make it accept a 
> const std::vector<gfx::GpuMemoryBufferHandle*>&
> 
> and do the for loop inside? 
> That way it'd look a a bit more like 
> OnBufferCreated2() that works with vectors.

Acknowledged. We would need to pass |std::vector<gfx::GpuMemoryBufferHandle>*|
so we can push_back or edit values, which lets the function to grow the vector
as it wants. I think, keeping the function responsible for filling a single
GpuMemoryBufferHandle is cleaner, and more flexible in case we were to add
DMA_BUF GMB sharing and such.

https://codereview.chromium.org/1267883002/diff/490001/content/browser/render...
content/browser/renderer_host/media/video_capture_controller.cc:385:
buffer_pool_->HoldForConsumers(buffer_id, count);
On 2015/08/27 18:26:55, mcasas wrote:
> nit: not your fault, but this |count| is just further
> cluttering this method.
> 
> s/count/controller_clients_.size()/  ?
> (and remove |count| of course)

Done.

[emircan, 2015-08-27 22:03:29]

emircan@chromium.org changed reviewers:
+ dalecurtis@chromium.org, wfh@chromium.org

[emircan, 2015-08-27 22:03:29]

wfh@chromium.org: Please review changes in
content/common/media/video_capture_messages.h

dalecurtis@chromium.org: Please review changes in content/renderer/* and
content/browser/media/capture/web_contents_video_capture_device_unittest.cc

[DaleCurtis, 2015-08-27 22:27:50]

rs lgtm

[Will Harris, 2015-08-27 22:31:36]

The new IPC - VideoCaptureMsg_NewBuffer2 takes buffer from ShareToProcess2 which
isn't implemented in this CL.

lgtm on this CL and adding this IPC in
content/common/media/video_capture_messages.h, but can you make sure you have
tsepez or me on follow up CLs that end up implementing ShareToProcess2 and
associated security sensitive code. Thanks.

[emircan, 2015-08-27 22:46:02]

On 2015/08/27 22:31:36, Will Harris wrote:
> The new IPC - VideoCaptureMsg_NewBuffer2 takes buffer from ShareToProcess2
which
> isn't implemented in this CL.

It goes like:
VideoCaptureController::DoIncomingCapturedVideoFrameOnIOThread ->
VideoCaptureHost::OnBufferCreated2 -> VideoCaptureMsg_NewBuffer2

> lgtm on this CL and adding this IPC in
> content/common/media/video_capture_messages.h, but can you make sure you have
> tsepez or me on follow up CLs that end up implementing ShareToProcess2 and
> associated security sensitive code. Thanks.

Thanks. I will keep you in the loop for everything touching IPC calls.

[emircan, 2015-08-31 17:46:55]

The CQ bit was checked by emircan@chromium.org

[emircan, 2015-08-31 17:46:55]

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

[commit-bot: I haz the power, 2015-08-31 17:47:42]

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

[commit-bot: I haz the power, 2015-08-31 17:53:20]

Committed patchset #8 (id:530001)

[commit-bot: I haz the power, 2015-08-31 17:54:03]

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