Implement IPCs and VideoCapture::Client interfaces for texture capture

Implement IPCs and VideoCapture::Client interfaces for texture capture

This change implements the IPC and interface changes necessary to support
allowing video capture devices to capture to texture mailboxes.  This is in
preparation for eventually allowing zero-copy desktop/tab capture.

BUG=269312
TEST=local build, unittests, run on CrOS snow

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=254133

[sheu, 2014-01-15 09:20:46]

Adding some relevant eyes on this.

jiayl@, mcasas@: PTAL

[sheu, 2014-01-15 09:23:13]

CC += posciak@ for informational purposes

[mcasas, 2014-01-21 14:34:02]

Looks nice, just some comments here and there on 
my side -- but I'd get an expert to look at the
IPC message interchange etc just in case.

https://chromiumcodereview.appspot.com/83793004/diff/200037/content/browser/r...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://chromiumcodereview.appspot.com/83793004/diff/200037/content/browser/r...
content/browser/renderer_host/media/video_capture_controller.cc:444:
&VideoCaptureController::DoIncomingCapturedVideoFrameOnIOThread,
I think the "unification" of VCC::VCDC::OnIncomingCapturedFrame
and VCC::VCDC::OnIncomingCapturedVideoFrame calls into a
single DoIncoming....OnIOThread is great! Perhaps ncarter@ 
would also be interested to take a look at this.

https://chromiumcodereview.appspot.com/83793004/diff/200037/content/browser/r...
content/browser/renderer_host/media/video_capture_controller.cc:469: <<
"Non-I420 output buffer format " << format << " requested";
Being |format| numeric, not sure this DCHECK would be very
informative as is.
Would it be possible/interesting to add some macros for capture
format -> String text?

https://chromiumcodereview.appspot.com/83793004/diff/200037/content/browser/r...
File content/browser/renderer_host/media/video_capture_controller.h (right):

https://chromiumcodereview.appspot.com/83793004/diff/200037/content/browser/r...
content/browser/renderer_host/media/video_capture_controller.h:106: uint32
sync_point);
There's no explanation in struct MailboxHolder about
|sync_point|, would you consider adding some info here
or in the impl of ReturnBuffer()?

https://chromiumcodereview.appspot.com/83793004/diff/200037/content/browser/r...
File content/browser/renderer_host/media/video_capture_host.cc (right):

https://chromiumcodereview.appspot.com/83793004/diff/200037/content/browser/r...
content/browser/renderer_host/media/video_capture_host.cc:94:
base::Bind(&VideoCaptureHost::DoSendFilledMailboxBufferOnIOThread,
If the call to VideoCaptureHost::OnMailboxBufferReady comes
from a method in the IO thread 
VideoCaptureController::DoIncomingCapturedVideoFrameOnIOThread(),
is it needed to jump again to IO thread?

https://chromiumcodereview.appspot.com/83793004/diff/200037/content/renderer/...
File content/renderer/media/video_capture_impl.cc (right):

https://chromiumcodereview.appspot.com/83793004/diff/200037/content/renderer/...
content/renderer/media/video_capture_impl.cc:219: DCHECK_EQ(format.pixel_format,
media::PIXEL_FORMAT_I420);
Yes! (But unfortunately :( )

https://chromiumcodereview.appspot.com/83793004/diff/200037/content/renderer/...
content/renderer/media/video_capture_impl.cc:295: uint32 sync_point = 0;
uint32 sync_point = mailbox_holder ? mailbox_holder->sync_point : 0; ?

https://chromiumcodereview.appspot.com/83793004/diff/200037/media/video/captu...
File media/video/capture/video_capture_device.h (right):

https://chromiumcodereview.appspot.com/83793004/diff/200037/media/video/captu...
media/video/capture/video_capture_device.h:164: base::TimeTicks timestamp) = 0;
The change in parameter order clutters the real goal 
of this CL.

https://chromiumcodereview.appspot.com/83793004/diff/200037/media/video/captu...
File media/video/capture/video_capture_types.cc (right):

https://chromiumcodereview.appspot.com/83793004/diff/200037/media/video/captu...
media/video/capture/video_capture_types.cc:24: (frame_size.GetArea() >= 0) &&
I think a frame with dimensions (0,0) is correct in this
sense but I had heated arguments on this before with
other members of the team.

[jiayl, 2014-01-23 22:18:52]

https://codereview.chromium.org/83793004/diff/200037/content/browser/renderer...
File content/browser/renderer_host/media/video_capture_device_impl.h (right):

https://codereview.chromium.org/83793004/diff/200037/content/browser/renderer...
content/browser/renderer_host/media/video_capture_device_impl.h:148: class
CONTENT_EXPORT VideoCaptureDeviceImpl
The naming of the class seems very confusing because it does not implement
VideoCaptureDevice, and it's too general (it's only relevant for WebContents
video capture). Please consider renaming it to make it clearer that it's the
"meat" of WebContentsVideoCaptureDevice.

https://codereview.chromium.org/83793004/diff/200037/media/video/capture/vide...
File media/video/capture/video_capture_device.h (right):

https://codereview.chromium.org/83793004/diff/200037/media/video/capture/vide...
media/video/capture/video_capture_device.h:160: virtual void
OnIncomingCapturedFrame(const uint8* data,
The naming of "OnIncomingCapturedFrame" and "OnIncomingCapturedVideoFrame"
sounds confusing to me. If the only difference is that one passes raw data and
the other passes a VideoFrame, please make the names match the difference, e.g.
OnIncomingCapturedRaw and OnIncomingCapturedFrame.

https://codereview.chromium.org/83793004/diff/200037/media/video/capture/vide...
media/video/capture/video_capture_device.h:171: virtual void
OnIncomingCapturedVideoFrame(
Did you consider extending Buffer to represent an external VideoFrame? It seems
sub-optimal to me to have both |buffer| and |frame| in the interface and that
which one holds the actually data depends on the format. For example, add a
|frame| parameter to ReserveOutputBuffer, and a buffer can represent shared
memory managed by itself, or an external VideoFrame. I think that should be a
cleaner design. Wdyt?

https://codereview.chromium.org/83793004/diff/200037/media/video/capture/vide...
File media/video/capture/video_capture_types.h (right):

https://codereview.chromium.org/83793004/diff/200037/media/video/capture/vide...
media/video/capture/video_capture_types.h:30: PIXEL_FORMAT_TEXTURE,
"texture" does not seem fitting the comment for the enum "color formats from
camera". Please add comments to clarify.

[sheu, 2014-01-29 01:19:20]

Updated.  PTAL

https://chromiumcodereview.appspot.com/83793004/diff/200037/content/browser/r...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://chromiumcodereview.appspot.com/83793004/diff/200037/content/browser/r...
content/browser/renderer_host/media/video_capture_controller.cc:469: <<
"Non-I420 output buffer format " << format << " requested";
On 2014/01/21 14:34:02, mcasas wrote:
> Being |format| numeric, not sure this DCHECK would be very
> informative as is.
> Would it be possible/interesting to add some macros for capture
> format -> String text?

Not very informative, but I think sufficient for a DCHECK.  For debugging
purposes you can just look up the enum value.

https://chromiumcodereview.appspot.com/83793004/diff/200037/content/browser/r...
File content/browser/renderer_host/media/video_capture_controller.h (right):

https://chromiumcodereview.appspot.com/83793004/diff/200037/content/browser/r...
content/browser/renderer_host/media/video_capture_controller.h:106: uint32
sync_point);
On 2014/01/21 14:34:02, mcasas wrote:
> There's no explanation in struct MailboxHolder about
> |sync_point|, would you consider adding some info here
> or in the impl of ReturnBuffer()?

Done.

https://chromiumcodereview.appspot.com/83793004/diff/200037/content/browser/r...
File content/browser/renderer_host/media/video_capture_device_impl.h (right):

https://chromiumcodereview.appspot.com/83793004/diff/200037/content/browser/r...
content/browser/renderer_host/media/video_capture_device_impl.h:148: class
CONTENT_EXPORT VideoCaptureDeviceImpl
On 2014/01/23 22:18:52, jiayl wrote:
> The naming of the class seems very confusing because it does not implement
> VideoCaptureDevice, and it's too general (it's only relevant for WebContents
> video capture). Please consider renaming it to make it clearer that it's the
> "meat" of WebContentsVideoCaptureDevice.

A little out of the scope of this change, probably.

https://chromiumcodereview.appspot.com/83793004/diff/200037/content/browser/r...
File content/browser/renderer_host/media/video_capture_host.cc (right):

https://chromiumcodereview.appspot.com/83793004/diff/200037/content/browser/r...
content/browser/renderer_host/media/video_capture_host.cc:94:
base::Bind(&VideoCaptureHost::DoSendFilledMailboxBufferOnIOThread,
On 2014/01/21 14:34:02, mcasas wrote:
> If the call to VideoCaptureHost::OnMailboxBufferReady comes
> from a method in the IO thread 
> VideoCaptureController::DoIncomingCapturedVideoFrameOnIOThread(),
> is it needed to jump again to IO thread?

Most of the trampolining seems unnecessary here, but the interface does not
enforce that the functions here are always called on the IO thread. 
Additionally, the VideoCaptureHost is created on the UI thread, so one can't
even say that the VideoCaptureHost is to be used on the thread it's created on,
and remove the trampolining that way.

https://chromiumcodereview.appspot.com/83793004/diff/200037/content/renderer/...
File content/renderer/media/video_capture_impl.cc (right):

https://chromiumcodereview.appspot.com/83793004/diff/200037/content/renderer/...
content/renderer/media/video_capture_impl.cc:295: uint32 sync_point = 0;
On 2014/01/21 14:34:02, mcasas wrote:
> uint32 sync_point = mailbox_holder ? mailbox_holder->sync_point : 0; ?

Done.

https://chromiumcodereview.appspot.com/83793004/diff/200037/media/video/captu...
File media/video/capture/video_capture_device.h (right):

https://chromiumcodereview.appspot.com/83793004/diff/200037/media/video/captu...
media/video/capture/video_capture_device.h:164: base::TimeTicks timestamp) = 0;
On 2014/01/21 14:34:02, mcasas wrote:
> The change in parameter order clutters the real goal 
> of this CL.

It is a little clutter.  In the end I thought that since this CL is doing
similar things, we might as well do it now since it's probably a small enough
change as to not be worth it on its own.  As it stands now the ordering of
parameters is more consistent with OnIncomingCapturedVideoFrame.

https://chromiumcodereview.appspot.com/83793004/diff/200037/media/video/captu...
media/video/capture/video_capture_device.h:171: virtual void
OnIncomingCapturedVideoFrame(
On 2014/01/23 22:18:52, jiayl wrote:
> Did you consider extending Buffer to represent an external VideoFrame? It
seems
> sub-optimal to me to have both |buffer| and |frame| in the interface and that
> which one holds the actually data depends on the format. For example, add a
> |frame| parameter to ReserveOutputBuffer, and a buffer can represent shared
> memory managed by itself, or an external VideoFrame. I think that should be a
> cleaner design. Wdyt?

The cleanest design (one that did exist a few months ago) would be to allow
ReserveOutputBuffer() to construct a frame outright and return it, thus
obviating the need for Buffer.  Unfortunately, for our texture capture pipeline
we won't be able to do that, because we don't know the captured texture mailbox
name or dimensions at the time that we request a reservation.  This also objects
to a design in which the frame is passed in to ReserveOutputBuffer().

https://chromiumcodereview.appspot.com/83793004/diff/200037/media/video/captu...
File media/video/capture/video_capture_types.h (right):

https://chromiumcodereview.appspot.com/83793004/diff/200037/media/video/captu...
media/video/capture/video_capture_types.h:30: PIXEL_FORMAT_TEXTURE,
On 2014/01/23 22:18:52, jiayl wrote:
> "texture" does not seem fitting the comment for the enum "color formats from
> camera". Please add comments to clarify.

Done.

[jiayl, 2014-01-31 19:32:38]

https://chromiumcodereview.appspot.com/83793004/diff/200037/content/browser/r...
File content/browser/renderer_host/media/video_capture_device_impl.h (right):

https://chromiumcodereview.appspot.com/83793004/diff/200037/content/browser/r...
content/browser/renderer_host/media/video_capture_device_impl.h:148: class
CONTENT_EXPORT VideoCaptureDeviceImpl
On 2014/01/29 01:19:20, sheu wrote:
> On 2014/01/23 22:18:52, jiayl wrote:
> > The naming of the class seems very confusing because it does not implement
> > VideoCaptureDevice, and it's too general (it's only relevant for WebContents
> > video capture). Please consider renaming it to make it clearer that it's the
> > "meat" of WebContentsVideoCaptureDevice.
> 
> A little out of the scope of this change, probably.

Then open a bug and add a TODO linked to the bug.

https://chromiumcodereview.appspot.com/83793004/diff/200037/media/video/captu...
File media/video/capture/video_capture_device.h (right):

https://chromiumcodereview.appspot.com/83793004/diff/200037/media/video/captu...
media/video/capture/video_capture_device.h:171: virtual void
OnIncomingCapturedVideoFrame(
On 2014/01/29 01:19:20, sheu wrote:
> On 2014/01/23 22:18:52, jiayl wrote:
> > Did you consider extending Buffer to represent an external VideoFrame? It
> seems
> > sub-optimal to me to have both |buffer| and |frame| in the interface and
that
> > which one holds the actually data depends on the format. For example, add a
> > |frame| parameter to ReserveOutputBuffer, and a buffer can represent shared
> > memory managed by itself, or an external VideoFrame. I think that should be
a
> > cleaner design. Wdyt?
> 
> The cleanest design (one that did exist a few months ago) would be to allow
> ReserveOutputBuffer() to construct a frame outright and return it, thus
> obviating the need for Buffer.  Unfortunately, for our texture capture
pipeline
> we won't be able to do that, because we don't know the captured texture
mailbox
> name or dimensions at the time that we request a reservation.  This also
objects
> to a design in which the frame is passed in to ReserveOutputBuffer().

Can you update the reserved buffer when the texture mailbox name or dimensions
are known to pass a VideoFrame?

https://chromiumcodereview.appspot.com/83793004/diff/630001/content/browser/r...
File content/browser/renderer_host/media/video_capture_controller.h (right):

https://chromiumcodereview.appspot.com/83793004/diff/630001/content/browser/r...
content/browser/renderer_host/media/video_capture_controller.h:104: //
destroying or recycling the texture.
The use of |sync_point| still seems unclear to someone who doesn't know about
the implementation details of mailbox/texture. Who will wait on it? What does it
mean by "waiting on" a uint32? What VideoCaptureController supposed to do with
it? If it's hard to explain in the context of VideoCaptureController, it
probably not suitable to be exposed at this level of the API. The interface of
VideoCaptureController should not depend on a impl detail of the client.
I don't have a more concrete suggestion though, since I don't really understand
the concept of sync_point.

[sheu, 2014-02-07 00:37:24]

https://chromiumcodereview.appspot.com/83793004/diff/200037/content/browser/r...
File content/browser/renderer_host/media/video_capture_device_impl.h (right):

https://chromiumcodereview.appspot.com/83793004/diff/200037/content/browser/r...
content/browser/renderer_host/media/video_capture_device_impl.h:148: class
CONTENT_EXPORT VideoCaptureDeviceImpl
On 2014/01/31 19:32:38, jiayl wrote:
> On 2014/01/29 01:19:20, sheu wrote:
> > On 2014/01/23 22:18:52, jiayl wrote:
> > > The naming of the class seems very confusing because it does not implement
> > > VideoCaptureDevice, and it's too general (it's only relevant for
WebContents
> > > video capture). Please consider renaming it to make it clearer that it's
the
> > > "meat" of WebContentsVideoCaptureDevice.
> > 
> > A little out of the scope of this change, probably.
> 
> Then open a bug and add a TODO linked to the bug.

It's also used for the desktop capture device in Aura.  The use case is pretty
much any capturer that needs to be event-driven and throttled -- the immediate
case is tab/screen capturing.

I'm thinking ContentVideoCaptureDeviceimpl.  WDYT?

https://chromiumcodereview.appspot.com/83793004/diff/200037/media/video/captu...
File media/video/capture/video_capture_device.h (right):

https://chromiumcodereview.appspot.com/83793004/diff/200037/media/video/captu...
media/video/capture/video_capture_device.h:160: virtual void
OnIncomingCapturedFrame(const uint8* data,
On 2014/01/23 22:18:52, jiayl wrote:
> The naming of "OnIncomingCapturedFrame" and "OnIncomingCapturedVideoFrame"
> sounds confusing to me. If the only difference is that one passes raw data and
> the other passes a VideoFrame, please make the names match the difference,
e.g.
> OnIncomingCapturedRaw and OnIncomingCapturedFrame.

Done.  Renamed to OnIncomingCapturedData().

https://chromiumcodereview.appspot.com/83793004/diff/200037/media/video/captu...
media/video/capture/video_capture_device.h:171: virtual void
OnIncomingCapturedVideoFrame(
On 2014/01/31 19:32:38, jiayl wrote:
> On 2014/01/29 01:19:20, sheu wrote:
> > On 2014/01/23 22:18:52, jiayl wrote:
> > > Did you consider extending Buffer to represent an external VideoFrame? It
> > seems
> > > sub-optimal to me to have both |buffer| and |frame| in the interface and
> that
> > > which one holds the actually data depends on the format. For example, add
a
> > > |frame| parameter to ReserveOutputBuffer, and a buffer can represent
shared
> > > memory managed by itself, or an external VideoFrame. I think that should
be
> a
> > > cleaner design. Wdyt?
> > 
> > The cleanest design (one that did exist a few months ago) would be to allow
> > ReserveOutputBuffer() to construct a frame outright and return it, thus
> > obviating the need for Buffer.  Unfortunately, for our texture capture
> pipeline
> > we won't be able to do that, because we don't know the captured texture
> mailbox
> > name or dimensions at the time that we request a reservation.  This also
> objects
> > to a design in which the frame is passed in to ReserveOutputBuffer().
> 
> Can you update the reserved buffer when the texture mailbox name or dimensions
> are known to pass a VideoFrame?

By the time those are known, we're ready to pass them back to the
VideoCaptureDevice, hence these entry points with both a Buffer and a
VideoFrame.

https://chromiumcodereview.appspot.com/83793004/diff/630001/content/browser/r...
File content/browser/renderer_host/media/video_capture_controller.h (right):

https://chromiumcodereview.appspot.com/83793004/diff/630001/content/browser/r...
content/browser/renderer_host/media/video_capture_controller.h:104: //
destroying or recycling the texture.
On 2014/01/31 19:32:39, jiayl wrote:
> The use of |sync_point| still seems unclear to someone who doesn't know about
> the implementation details of mailbox/texture. Who will wait on it? What does
it
> mean by "waiting on" a uint32? What VideoCaptureController supposed to do with
> it? If it's hard to explain in the context of VideoCaptureController, it
> probably not suitable to be exposed at this level of the API. The interface of
> VideoCaptureController should not depend on a impl detail of the client.
> I don't have a more concrete suggestion though, since I don't really
understand
> the concept of sync_point.

Syncpoints are used for cross-process/cross-context GL context synchronization,
and they're passed around as a uint32 index.  There's not a better data
abstraction for it, unfortunately.  In the context of dealing with textures and
GL contexts, this is pretty well-described and I don't think it's a problem at
this level of the API.

[jiayl, 2014-02-07 01:22:07]

https://chromiumcodereview.appspot.com/83793004/diff/200037/content/browser/r...
File content/browser/renderer_host/media/video_capture_device_impl.h (right):

https://chromiumcodereview.appspot.com/83793004/diff/200037/content/browser/r...
content/browser/renderer_host/media/video_capture_device_impl.h:148: class
CONTENT_EXPORT VideoCaptureDeviceImpl
I hope we can avoid "impl", e.g. VideoCaptureDeviceCore?

On 2014/02/07 00:37:25, sheu wrote:
> On 2014/01/31 19:32:38, jiayl wrote:
> > On 2014/01/29 01:19:20, sheu wrote:
> > > On 2014/01/23 22:18:52, jiayl wrote:
> > > > The naming of the class seems very confusing because it does not
implement
> > > > VideoCaptureDevice, and it's too general (it's only relevant for
> WebContents
> > > > video capture). Please consider renaming it to make it clearer that it's
> the
> > > > "meat" of WebContentsVideoCaptureDevice.
> > > 
> > > A little out of the scope of this change, probably.
> > 
> > Then open a bug and add a TODO linked to the bug.
> 
> It's also used for the desktop capture device in Aura.  The use case is pretty
> much any capturer that needs to be event-driven and throttled -- the immediate
> case is tab/screen capturing.
> 
> I'm thinking ContentVideoCaptureDeviceimpl.  WDYT?

https://chromiumcodereview.appspot.com/83793004/diff/200037/media/video/captu...
File media/video/capture/video_capture_device.h (right):

https://chromiumcodereview.appspot.com/83793004/diff/200037/media/video/captu...
media/video/capture/video_capture_device.h:171: virtual void
OnIncomingCapturedVideoFrame(
OK, thanks.

On 2014/02/07 00:37:25, sheu wrote:
> On 2014/01/31 19:32:38, jiayl wrote:
> > On 2014/01/29 01:19:20, sheu wrote:
> > > On 2014/01/23 22:18:52, jiayl wrote:
> > > > Did you consider extending Buffer to represent an external VideoFrame?
It
> > > seems
> > > > sub-optimal to me to have both |buffer| and |frame| in the interface and
> > that
> > > > which one holds the actually data depends on the format. For example,
add
> a
> > > > |frame| parameter to ReserveOutputBuffer, and a buffer can represent
> shared
> > > > memory managed by itself, or an external VideoFrame. I think that should
> be
> > a
> > > > cleaner design. Wdyt?
> > > 
> > > The cleanest design (one that did exist a few months ago) would be to
allow
> > > ReserveOutputBuffer() to construct a frame outright and return it, thus
> > > obviating the need for Buffer.  Unfortunately, for our texture capture
> > pipeline
> > > we won't be able to do that, because we don't know the captured texture
> > mailbox
> > > name or dimensions at the time that we request a reservation.  This also
> > objects
> > > to a design in which the frame is passed in to ReserveOutputBuffer().
> > 
> > Can you update the reserved buffer when the texture mailbox name or
dimensions
> > are known to pass a VideoFrame?
> 
> By the time those are known, we're ready to pass them back to the
> VideoCaptureDevice, hence these entry points with both a Buffer and a
> VideoFrame.

https://chromiumcodereview.appspot.com/83793004/diff/630001/content/browser/r...
File content/browser/renderer_host/media/video_capture_controller.h (right):

https://chromiumcodereview.appspot.com/83793004/diff/630001/content/browser/r...
content/browser/renderer_host/media/video_capture_controller.h:104: //
destroying or recycling the texture.
Is it possible to add a link in the comment to provide more information, e.g.
where it is used in the context of texture?

On 2014/02/07 00:37:25, sheu wrote:
> On 2014/01/31 19:32:39, jiayl wrote:
> > The use of |sync_point| still seems unclear to someone who doesn't know
about
> > the implementation details of mailbox/texture. Who will wait on it? What
does
> it
> > mean by "waiting on" a uint32? What VideoCaptureController supposed to do
with
> > it? If it's hard to explain in the context of VideoCaptureController, it
> > probably not suitable to be exposed at this level of the API. The interface
of
> > VideoCaptureController should not depend on a impl detail of the client.
> > I don't have a more concrete suggestion though, since I don't really
> understand
> > the concept of sync_point.
> 
> Syncpoints are used for cross-process/cross-context GL context
synchronization,
> and they're passed around as a uint32 index.  There's not a better data
> abstraction for it, unfortunately.  In the context of dealing with textures
and
> GL contexts, this is pretty well-described and I don't think it's a problem at
> this level of the API.

[sheu, 2014-02-14 01:59:28]

Rebased and such.  The VideoCaptureDeviceImpl rename has been done in another
CL.

jiayl@: PTAL

https://chromiumcodereview.appspot.com/83793004/diff/630001/content/browser/r...
File content/browser/renderer_host/media/video_capture_controller.h (right):

https://chromiumcodereview.appspot.com/83793004/diff/630001/content/browser/r...
content/browser/renderer_host/media/video_capture_controller.h:104: //
destroying or recycling the texture.
On 2014/02/07 01:22:08, jiayl wrote:
> Is it possible to add a link in the comment to provide more information, e.g.
> where it is used in the context of texture?
> 
> On 2014/02/07 00:37:25, sheu wrote:
> > On 2014/01/31 19:32:39, jiayl wrote:
> > > The use of |sync_point| still seems unclear to someone who doesn't know
> about
> > > the implementation details of mailbox/texture. Who will wait on it? What
> does
> > it
> > > mean by "waiting on" a uint32? What VideoCaptureController supposed to do
> with
> > > it? If it's hard to explain in the context of VideoCaptureController, it
> > > probably not suitable to be exposed at this level of the API. The
interface
> of
> > > VideoCaptureController should not depend on a impl detail of the client.
> > > I don't have a more concrete suggestion though, since I don't really
> > understand
> > > the concept of sync_point.
> > 
> > Syncpoints are used for cross-process/cross-context GL context
> synchronization,
> > and they're passed around as a uint32 index.  There's not a better data
> > abstraction for it, unfortunately.  In the context of dealing with textures
> and
> > GL contexts, this is pretty well-described and I don't think it's a problem
at
> > this level of the API.
> 

Honestly, I think that the syncpoint mechanism should be pretty clear to people
working with cross-context transport of textures, as in this case, and adding in
an explanation would be a little pedantic.

[jiayl, 2014-02-14 17:39:43]

https://chromiumcodereview.appspot.com/83793004/diff/630001/content/browser/r...
File content/browser/renderer_host/media/video_capture_controller.h (right):

https://chromiumcodereview.appspot.com/83793004/diff/630001/content/browser/r...
content/browser/renderer_host/media/video_capture_controller.h:104: //
destroying or recycling the texture.
But the code will be read, used, and maintained by people not working with
cross-context transport of textures since this API is more general than that.
"sync_point" is a new concept to the class/API, at the minimum it should be
explained what the API method ReturnBuffer is expected to do with sync_point. Is
that hard to do?

On 2014/02/14 01:59:29, sheu wrote:
> On 2014/02/07 01:22:08, jiayl wrote:
> > Is it possible to add a link in the comment to provide more information,
e.g.
> > where it is used in the context of texture?
> > 
> > On 2014/02/07 00:37:25, sheu wrote:
> > > On 2014/01/31 19:32:39, jiayl wrote:
> > > > The use of |sync_point| still seems unclear to someone who doesn't know
> > about
> > > > the implementation details of mailbox/texture. Who will wait on it? What
> > does
> > > it
> > > > mean by "waiting on" a uint32? What VideoCaptureController supposed to
do
> > with
> > > > it? If it's hard to explain in the context of VideoCaptureController, it
> > > > probably not suitable to be exposed at this level of the API. The
> interface
> > of
> > > > VideoCaptureController should not depend on a impl detail of the client.
> > > > I don't have a more concrete suggestion though, since I don't really
> > > understand
> > > > the concept of sync_point.
> > > 
> > > Syncpoints are used for cross-process/cross-context GL context
> > synchronization,
> > > and they're passed around as a uint32 index.  There's not a better data
> > > abstraction for it, unfortunately.  In the context of dealing with
textures
> > and
> > > GL contexts, this is pretty well-described and I don't think it's a
problem
> at
> > > this level of the API.
> > 
> 
> Honestly, I think that the syncpoint mechanism should be pretty clear to
people
> working with cross-context transport of textures, as in this case, and adding
in
> an explanation would be a little pedantic.

[sheu, 2014-02-14 20:23:18]

https://chromiumcodereview.appspot.com/83793004/diff/630001/content/browser/r...
File content/browser/renderer_host/media/video_capture_controller.h (right):

https://chromiumcodereview.appspot.com/83793004/diff/630001/content/browser/r...
content/browser/renderer_host/media/video_capture_controller.h:104: //
destroying or recycling the texture.
On 2014/02/14 17:39:44, jiayl wrote:
> But the code will be read, used, and maintained by people not working with
> cross-context transport of textures since this API is more general than that.
> "sync_point" is a new concept to the class/API, at the minimum it should be
> explained what the API method ReturnBuffer is expected to do with sync_point.
Is
> that hard to do?

I'm sorry if I sound a little obstinate or obstructionist or some such.  I don't
mean it that way; I'm just trying to keep things clean and I hope we can come to
consensus here.

Anyhow, the comment already describes what ReturnBuffer is supposed to do with
the |sync_point|; it will be waited on before destroying or recycling the
texture.  My point is that "waiting on a sync point before operating on a
texture" is a fairly well-understood operation in the graphics context, and an
explanation here I think would add more clutter than it needs.  That would be on
par with, say, explaining what at VideoCaptureControllerID is every time it
comes up, when in common practice it should be already understood.

[jiayl, 2014-02-15 01:01:24]

On 2014/02/14 20:23:18, sheu wrote:
>
https://chromiumcodereview.appspot.com/83793004/diff/630001/content/browser/r...
> File content/browser/renderer_host/media/video_capture_controller.h (right):
> 
>
https://chromiumcodereview.appspot.com/83793004/diff/630001/content/browser/r...
> content/browser/renderer_host/media/video_capture_controller.h:104: //
> destroying or recycling the texture.
> On 2014/02/14 17:39:44, jiayl wrote:
> > But the code will be read, used, and maintained by people not working with
> > cross-context transport of textures since this API is more general than
that.
> > "sync_point" is a new concept to the class/API, at the minimum it should be
> > explained what the API method ReturnBuffer is expected to do with
sync_point.
> Is
> > that hard to do?
> 
> I'm sorry if I sound a little obstinate or obstructionist or some such.  I
don't
> mean it that way; I'm just trying to keep things clean and I hope we can come
to
> consensus here.
> 
> Anyhow, the comment already describes what ReturnBuffer is supposed to do with
> the |sync_point|; it will be waited on before destroying or recycling the
> texture.  My point is that "waiting on a sync point before operating on a
> texture" is a fairly well-understood operation in the graphics context, and an
> explanation here I think would add more clutter than it needs.  That would be
on
> par with, say, explaining what at VideoCaptureControllerID is every time it
> comes up, when in common practice it should be already understood.

I'm just trying to point out the difficulty another developer may have when they
come across the code. The clearer you make it, the less likely it may be broken
by 
someone else in the future. The concept of sync point is not well known outside
the graphics context and VideoCaptureController is outside the graphics context.
That's
different from VideoCaptureControllerID, which is a well defined type and one
can easily
find its definition and references. That's why I was asking to add some
reference to the 
context of sync point: because it's out of its native context. 
"it will be waited on..." is vague; from the implementation, what 
it actually does is to pass the sync point to the MailBoxHolder. IMO it'll be
much clearer 
if the comment says something more concrete like "sync_point is passed to the
MailBoxHolder 
for cross-context synchronization of textures". 

Anyway, it's up to you to decide how to address it. 

lgtm.

[sheu, 2014-02-19 19:46:09]

OWNERS reviews now.

wjia@: PTAL.  Thanks!

[wjia(left Chromium), 2014-02-19 22:05:18]

https://codereview.chromium.org/83793004/diff/1090001/content/browser/rendere...
File content/browser/renderer_host/media/content_video_capture_device_core.h
(right):

https://codereview.chromium.org/83793004/diff/1090001/content/browser/rendere...
content/browser/renderer_host/media/content_video_capture_device_core.h:19: 
nit: remove the blank line here and line 21.

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

https://codereview.chromium.org/83793004/diff/1090001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.cc:81:
std::map<int, scoped_refptr<media::VideoFrame> > active_buffers;
Could you typedef this map since it's used more than once in the code?

https://codereview.chromium.org/83793004/diff/1090001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.cc:543:
DCHECK(inserted) << "Unexpected duplicate buffer: " << buffer->id();
Would |inserted| be an unused variable in release build?

https://codereview.chromium.org/83793004/diff/1090001/content/browser/rendere...
File
content/browser/renderer_host/media/video_capture_controller_event_handler.h
(right):

https://codereview.chromium.org/83793004/diff/1090001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller_event_handler.h:13:

nit: no blank line is needed here and below.

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

https://codereview.chromium.org/83793004/diff/1090001/content/renderer/media/...
content/renderer/media/video_capture_impl.cc:282:
base::Passed(scoped_ptr<gpu::MailboxHolder>().Pass()))));
extra Pass()?

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

https://codereview.chromium.org/83793004/diff/1090001/content/renderer/media/...
content/renderer/media/video_capture_impl.h:42: 
ditto.

https://codereview.chromium.org/83793004/diff/1090001/content/renderer/media/...
File content/renderer/media/video_capture_message_filter.h (right):

https://codereview.chromium.org/83793004/diff/1090001/content/renderer/media/...
content/renderer/media/video_capture_message_filter.h:22: 
ditto.

[sheu, 2014-02-19 23:18:34]

Updated.  wjia@: PTAL, thanks!

https://codereview.chromium.org/83793004/diff/1090001/content/browser/rendere...
File content/browser/renderer_host/media/content_video_capture_device_core.h
(right):

https://codereview.chromium.org/83793004/diff/1090001/content/browser/rendere...
content/browser/renderer_host/media/content_video_capture_device_core.h:19: 
On 2014/02/19 22:05:19, wjia wrote:
> nit: remove the blank line here and line 21.

Done.

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

https://codereview.chromium.org/83793004/diff/1090001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.cc:81:
std::map<int, scoped_refptr<media::VideoFrame> > active_buffers;
On 2014/02/19 22:05:19, wjia wrote:
> Could you typedef this map since it's used more than once in the code?

Sure.

https://codereview.chromium.org/83793004/diff/1090001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.cc:543:
DCHECK(inserted) << "Unexpected duplicate buffer: " << buffer->id();
On 2014/02/19 22:05:19, wjia wrote:
> Would |inserted| be an unused variable in release build?

Yes, but it doesn't seem to have been a problem so far.  (ToT version has this
too.)  Is this going to be otherwise a problem?

https://codereview.chromium.org/83793004/diff/1090001/content/browser/rendere...
File
content/browser/renderer_host/media/video_capture_controller_event_handler.h
(right):

https://codereview.chromium.org/83793004/diff/1090001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller_event_handler.h:13:

On 2014/02/19 22:05:19, wjia wrote:
> nit: no blank line is needed here and below.

Done.

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

https://codereview.chromium.org/83793004/diff/1090001/content/renderer/media/...
content/renderer/media/video_capture_impl.cc:282:
base::Passed(scoped_ptr<gpu::MailboxHolder>().Pass()))));
On 2014/02/19 22:05:19, wjia wrote:
> extra Pass()?

TL;DR: not extraneous :-)

base::Passed() does have overload for the case where you pass it an address-of
scoped_ptr.  But you can't take the address of a temporary, which is what I'm
doing here; hence the explicit Pass().

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

https://codereview.chromium.org/83793004/diff/1090001/content/renderer/media/...
content/renderer/media/video_capture_impl.h:42: 
On 2014/02/19 22:05:19, wjia wrote:
> ditto.

Done.

https://codereview.chromium.org/83793004/diff/1090001/content/renderer/media/...
File content/renderer/media/video_capture_message_filter.h (right):

https://codereview.chromium.org/83793004/diff/1090001/content/renderer/media/...
content/renderer/media/video_capture_message_filter.h:22: 
On 2014/02/19 22:05:19, wjia wrote:
> ditto.

Done.

[wjia(left Chromium), 2014-02-20 00:07:10]

lgtm

[sheu, 2014-02-20 01:06:31]

cevans@: IPC review please for content/common/media/video_capture_messages.h. 
Thanks!

[sheu, 2014-02-21 02:04:35]

cevans@: PTAL, thanks!

[sheu, 2014-02-22 00:56:47]

cdn@: PTAL at content/common/media/video_capture_messages.h.  Thanks!

[Cris Neckar, 2014-02-24 18:37:26]

On 2014/02/22 00:56:47, sheu wrote:
> cdn@: PTAL at content/common/media/video_capture_messages.h.  Thanks!

IPC changes lgtm

[sheu, 2014-02-24 22:05:16]

The CQ bit was checked by sheu@chromium.org

[commit-bot: I haz the power, 2014-02-24 22:08:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sheu@chromium.org/83793004/1600001

[commit-bot: I haz the power, 2014-02-24 22:39:14]

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

[commit-bot: I haz the power, 2014-02-24 22:39:15]

Retried try job too often on win_rel for step(s) app_list_unittests,
ash_unittests, aura_unittests, cacheinvalidation_unittests, cc_unittests,
check_deps, check_deps2git, chrome_elf_unittests, chromedriver_unittests,
components_unittests, compositor_unittests, content_browsertests,
content_unittests, crypto_unittests, events_unittests, google_apis_unittests,
gpu_unittests, installer_util_unittests, ipc_tests, jingle_unittests,
media_unittests, mini_installer_test, nacl_integration, ppapi_unittests,
printing_unittests, remoting_unittests, sql_unittests, sync_integration_tests,
sync_unit_tests, telemetry_perf_unittests, telemetry_unittests, views_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[sheu, 2014-02-24 22:49:39]

The CQ bit was checked by sheu@chromium.org

[commit-bot: I haz the power, 2014-02-24 22:53:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sheu@chromium.org/83793004/1600001

[commit-bot: I haz the power, 2014-02-24 23:47:38]

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

[commit-bot: I haz the power, 2014-02-24 23:47:39]

Retried try job too often on android_clang_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[sheu, 2014-02-24 23:49:35]

The CQ bit was checked by sheu@chromium.org

[commit-bot: I haz the power, 2014-02-24 23:52:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sheu@chromium.org/83793004/1600001

[commit-bot: I haz the power, 2014-02-25 01:42:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sheu@chromium.org/83793004/1600001

[commit-bot: I haz the power, 2014-02-25 02:30:22]

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

[commit-bot: I haz the power, 2014-02-25 02:30:23]

Retried try job too often on android_clang_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[mcasas, 2014-02-25 08:37:40]

On 2014/02/25 02:30:23, I haz the power (commit-bot) wrote:
> Retried try job too often on android_clang_dbg for step(s) slave_steps
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

Just for the record, LGTM.

[sheu, 2014-02-25 08:38:42]

The CQ bit was checked by sheu@chromium.org

[commit-bot: I haz the power, 2014-02-25 08:39:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sheu@chromium.org/83793004/1600001

[commit-bot: I haz the power, 2014-02-25 08:39:45]

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

[commit-bot: I haz the power, 2014-02-25 08:39:47]

Failed to apply patch for
content/browser/renderer_host/media/content_video_capture_device_core.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  can't find file to patch at input line 6
  Perhaps you used the wrong -p or --strip option?
  The text leading up to this was:
  --------------------------
  |Index:
content/browser/renderer_host/media/content_video_capture_device_core.cc
  |diff --git
a/content/browser/renderer_host/media/content_video_capture_device_core.cc
b/content/browser/renderer_host/media/content_video_capture_device_core.cc
  |index
f710134489ec0e12dfc8bf5ea0d4a9a5a1acbfd9..8f6cd54f5451e3630024b0ee5f63abf3c5663b6e
100644
  |---
a/content/browser/renderer_host/media/content_video_capture_device_core.cc
  |+++
b/content/browser/renderer_host/media/content_video_capture_device_core.cc
  --------------------------
  No file to patch.  Skipping patch.
  4 out of 4 hunks ignored

Patch:      
content/browser/renderer_host/media/content_video_capture_device_core.cc
Index: content/browser/renderer_host/media/content_video_capture_device_core.cc
diff --git
a/content/browser/renderer_host/media/content_video_capture_device_core.cc
b/content/browser/renderer_host/media/content_video_capture_device_core.cc
index
f710134489ec0e12dfc8bf5ea0d4a9a5a1acbfd9..8f6cd54f5451e3630024b0ee5f63abf3c5663b6e
100644
--- a/content/browser/renderer_host/media/content_video_capture_device_core.cc
+++ b/content/browser/renderer_host/media/content_video_capture_device_core.cc
@@ -110,10 +110,6 @@ bool ThreadSafeCaptureOracle::ObserveEventAndDecideCapture(
   TRACE_EVENT_ASYNC_BEGIN2("mirroring", "Capture", output_buffer.get(),
                            "frame_number", frame_number,
                            "trigger", event_name);
-  *callback = base::Bind(&ThreadSafeCaptureOracle::DidCaptureFrame,
-                         this,
-                         output_buffer,
-                         frame_number);
   *storage = media::VideoFrame::WrapExternalPackedMemory(
       media::VideoFrame::I420,
       capture_size_,
@@ -124,6 +120,11 @@ bool ThreadSafeCaptureOracle::ObserveEventAndDecideCapture(
       base::SharedMemory::NULLHandle(),
       base::TimeDelta(),
       base::Closure());
+  *callback = base::Bind(&ThreadSafeCaptureOracle::DidCaptureFrame,
+                         this,
+                         output_buffer,
+                         *storage,
+                         frame_number);
   return true;
 }
 
@@ -167,7 +168,8 @@ void ThreadSafeCaptureOracle::ReportError(const std::string&
reason) {
 }
 
 void ThreadSafeCaptureOracle::DidCaptureFrame(
-    scoped_refptr<media::VideoCaptureDevice::Client::Buffer> buffer,
+    const scoped_refptr<media::VideoCaptureDevice::Client::Buffer>& buffer,
+    const scoped_refptr<media::VideoFrame>& frame,
     int frame_number,
     base::TimeTicks timestamp,
     bool success) {
@@ -181,11 +183,12 @@ void ThreadSafeCaptureOracle::DidCaptureFrame(
 
   if (success) {
     if (oracle_->CompleteCapture(frame_number, timestamp)) {
-      client_->OnIncomingCapturedBuffer(buffer,
-                                        media::VideoFrame::I420,
-                                        capture_size_,
-                                        timestamp,
-                                        frame_rate_);
+      client_->OnIncomingCapturedVideoFrame(
+          buffer,
+          media::VideoCaptureFormat(
+              capture_size_, frame_rate_, media::PIXEL_FORMAT_I420),
+          frame,
+          timestamp);
     }
   }
 }

[sheu, 2014-02-26 22:09:43]

The CQ bit was checked by sheu@chromium.org

[commit-bot: I haz the power, 2014-02-26 22:10:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sheu@chromium.org/83793004/1620001

[commit-bot: I haz the power, 2014-02-26 22:41:50]

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

[commit-bot: I haz the power, 2014-02-26 22:41:52]

Retried try job too often on win_rel for step(s) app_list_unittests,
ash_unittests, aura_unittests, cacheinvalidation_unittests, cc_unittests,
check_deps, check_deps2git, chrome_elf_unittests, chromedriver_unittests,
components_unittests, compositor_unittests, content_browsertests,
content_unittests, crypto_unittests, events_unittests, google_apis_unittests,
gpu_unittests, installer_util_unittests, ipc_tests, jingle_unittests,
media_unittests, mini_installer_test, nacl_integration, ppapi_unittests,
printing_unittests, remoting_unittests, sql_unittests, sync_integration_tests,
sync_unit_tests, telemetry_perf_unittests, telemetry_unittests, views_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[sheu, 2014-02-28 08:49:05]

The CQ bit was checked by sheu@chromium.org

[commit-bot: I haz the power, 2014-02-28 08:49:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sheu@chromium.org/83793004/1660002

[commit-bot: I haz the power, 2014-02-28 09:29:04]

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

[commit-bot: I haz the power, 2014-02-28 09:29:04]

Try jobs failed on following builders: linux_chromium_rel

[sheu, 2014-02-28 13:54:51]

The CQ bit was checked by sheu@chromium.org

[commit-bot: I haz the power, 2014-02-28 13:55:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sheu@chromium.org/83793004/1660002

[commit-bot: I haz the power, 2014-02-28 15:43:53]

Change committed as 254133