[Mojo Video Capture] Simplify media::VideoCaptureDevice::Client:Buffer to a struct

[Mojo Video Capture] Simplify media::VideoCaptureDevice::Client:Buffer to a struct

In interface media::VideoCaptureDevice::Client, change interface Buffer to a 
move-only struct. The new struct separates the concerns of providing access 
handles (for in-process as well as for inter-process transit) from access 
permissions. This allows clients to explicitly obtain in-process memory-mapped 
handles when they need it. Clients who only want to pass along the buffer or 
move it across process boundaries do not need memory-mapped handles.

In interface media::VideoCaptureBufferPool, renamed 
 GetBufferHandle() -> GetHandleForInProcessAccess()
 GetHandleForTransit() -> GetHandleForInterProcessTransit().

Removed outdated implementation in video_capture Mojo service. We leave the 
implementation empty, until refactoring the interfaces has been completed. 

This CL is part of the Mojo Video Capture work. For the bigger picture, 
see [1] CL1.9.10

For a peek at the follow-up CLs, please see 
https://codereview.chromium.org/2607203002/
https://codereview.chromium.org/2602983002/

BUG=584797
TEST=
  content_unittests --gtest_filter="*Video*", 
  video_capture_unittests, 
  Apprtc loopback on Debug, 
  Desktop Capture Example extension on Release

[1] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxsxWslJ_X-zpOhijvI8/edit?usp=sharing

Review-Url: https://codereview.chromium.org/2573223002
Cr-Commit-Position: refs/heads/master@{#442066}
Committed: https://chromium.googlesource.com/chromium/src/+/e1aac992b0399db7776661f4ac2c43094c265de7

[chfremer, 2016-12-14 22:16:32]

Description was changed from

==========
Merge branch '1.9.9' into 1.9.10


Fix after merge


Merge branch '1.9.9' into 1.9.10


Fix after merge


Merge branch '1.9.9' into 1.9.10


Fix after merge


Merge branch '1.9.9' into 1.9.10


Merge branch '1.9.9' into 1.9.10


Merge branch '1.9.9' into 1.9.10


Merge branch '1.9.9' into 1.9.10


Merge branch '1.9.9' into 1.9.10


Merge branch '1.9.9' into 1.9.10


Merge branch '1.9.9' into 1.9.10


Fix from splitting


Merge branch '1.9.9' into 1.9.10


Apply patch to 1.9.10pre

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

to

==========
[Mojo Video Capture] Simplify media::VideoCaptureDevice::Client:Buffer to a
struct

In interface media::VideoCaptureDevice::Client, change interface Buffer to a
move-only struct. The new struct separates the concerns of providing read/write
access, providing a transit handle for transport across processes, as well as a
usage reservation. This allows clients to explicitly obtain read/write access 
only when they need it. Clients who only want to pass along the buffer or move 
it across process boundaries do not need read/write access.
Separating the buffer reservation into an object with empy interface Ownership 
has the advantage of allowing us to pass around these reservations separately 
from the buffer handles. This will be particularly useful in the subsequent CL,
where we are going to do exactly that and update the VideoFrameReceiver 
interface to make it clear how frame delivery involves a set of pre-shared 
buffers that are alternately filled by the producer.

In interface media::VideoCaptureBufferPool, renamed method GetBufferHandle() to
GetReadWriteAccess() to better reveal what it does.

Removed outdated implementation in video_capture Mojo service. We leave the
implementation empty, until refactoring the interfaces has been completed. 

Tested: Apprtc loopback on Debug, Desktop Capture Example extension on Release

This CL is part of the Mojo Video Capture work. For the bigger picture, 
see [1] CL1.9.10

BUG=584797
TEST=
  content_unittests --gtest_filter="*Video*", 
  video_capture_unittests, 
  Apprtc loopback on Debug, 
  Desktop Capture Example extension on Release

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
==========

[chfremer, 2016-12-14 22:17:04]

Description was changed from

==========
[Mojo Video Capture] Simplify media::VideoCaptureDevice::Client:Buffer to a
struct

In interface media::VideoCaptureDevice::Client, change interface Buffer to a
move-only struct. The new struct separates the concerns of providing read/write
access, providing a transit handle for transport across processes, as well as a
usage reservation. This allows clients to explicitly obtain read/write access 
only when they need it. Clients who only want to pass along the buffer or move 
it across process boundaries do not need read/write access.
Separating the buffer reservation into an object with empy interface Ownership 
has the advantage of allowing us to pass around these reservations separately 
from the buffer handles. This will be particularly useful in the subsequent CL,
where we are going to do exactly that and update the VideoFrameReceiver 
interface to make it clear how frame delivery involves a set of pre-shared 
buffers that are alternately filled by the producer.

In interface media::VideoCaptureBufferPool, renamed method GetBufferHandle() to
GetReadWriteAccess() to better reveal what it does.

Removed outdated implementation in video_capture Mojo service. We leave the
implementation empty, until refactoring the interfaces has been completed. 

Tested: Apprtc loopback on Debug, Desktop Capture Example extension on Release

This CL is part of the Mojo Video Capture work. For the bigger picture, 
see [1] CL1.9.10

BUG=584797
TEST=
  content_unittests --gtest_filter="*Video*", 
  video_capture_unittests, 
  Apprtc loopback on Debug, 
  Desktop Capture Example extension on Release

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
==========

to

==========
[Mojo Video Capture] Simplify media::VideoCaptureDevice::Client:Buffer to a
struct

In interface media::VideoCaptureDevice::Client, change interface Buffer to a 
move-only struct. The new struct separates the concerns of providing read/write 
access, providing a transit handle for transport across processes, as well as a
usage reservation. This allows clients to explicitly obtain read/write access 
only when they need it. Clients who only want to pass along the buffer or move 
it across process boundaries do not need read/write access.
Separating the buffer reservation into an object with empy interface Ownership 
has the advantage of allowing us to pass around these reservations separately 
from the buffer handles. This will be particularly useful in the subsequent CL,
where we are going to do exactly that and update the VideoFrameReceiver 
interface to make it clear how frame delivery involves a set of pre-shared 
buffers that are alternately filled by the producer.

In interface media::VideoCaptureBufferPool, renamed method GetBufferHandle() to 
GetReadWriteAccess() to better reveal what it does.

Removed outdated implementation in video_capture Mojo service. We leave the 
implementation empty, until refactoring the interfaces has been completed. 

Tested: Apprtc loopback on Debug, Desktop Capture Example extension on Release

This CL is part of the Mojo Video Capture work. For the bigger picture, 
see [1] CL1.9.10

BUG=584797
TEST=
  content_unittests --gtest_filter="*Video*", 
  video_capture_unittests, 
  Apprtc loopback on Debug, 
  Desktop Capture Example extension on Release

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
==========

[chfremer, 2016-12-14 22:17:25]

Description was changed from

==========
[Mojo Video Capture] Simplify media::VideoCaptureDevice::Client:Buffer to a
struct

In interface media::VideoCaptureDevice::Client, change interface Buffer to a 
move-only struct. The new struct separates the concerns of providing read/write 
access, providing a transit handle for transport across processes, as well as a
usage reservation. This allows clients to explicitly obtain read/write access 
only when they need it. Clients who only want to pass along the buffer or move 
it across process boundaries do not need read/write access.
Separating the buffer reservation into an object with empy interface Ownership 
has the advantage of allowing us to pass around these reservations separately 
from the buffer handles. This will be particularly useful in the subsequent CL,
where we are going to do exactly that and update the VideoFrameReceiver 
interface to make it clear how frame delivery involves a set of pre-shared 
buffers that are alternately filled by the producer.

In interface media::VideoCaptureBufferPool, renamed method GetBufferHandle() to 
GetReadWriteAccess() to better reveal what it does.

Removed outdated implementation in video_capture Mojo service. We leave the 
implementation empty, until refactoring the interfaces has been completed. 

Tested: Apprtc loopback on Debug, Desktop Capture Example extension on Release

This CL is part of the Mojo Video Capture work. For the bigger picture, 
see [1] CL1.9.10

BUG=584797
TEST=
  content_unittests --gtest_filter="*Video*", 
  video_capture_unittests, 
  Apprtc loopback on Debug, 
  Desktop Capture Example extension on Release

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
==========

to

==========
[Mojo Video Capture] Simplify media::VideoCaptureDevice::Client:Buffer to a
struct

In interface media::VideoCaptureDevice::Client, change interface Buffer to a 
move-only struct. The new struct separates the concerns of providing read/write 
access, providing a transit handle for transport across processes, as well as a
usage reservation. This allows clients to explicitly obtain read/write access 
only when they need it. Clients who only want to pass along the buffer or move 
it across process boundaries do not need read/write access.
Separating the buffer reservation into an object with empy interface Ownership 
has the advantage of allowing us to pass around these reservations separately 
from the buffer handles. This will be particularly useful in the subsequent CL,
where we are going to do exactly that and update the VideoFrameReceiver 
interface to make it clear how frame delivery involves a set of pre-shared 
buffers that are alternately filled by the producer.

In interface media::VideoCaptureBufferPool, renamed method GetBufferHandle() to 
GetReadWriteAccess() to better reveal what it does.

Removed outdated implementation in video_capture Mojo service. We leave the 
implementation empty, until refactoring the interfaces has been completed. 

Tested: Apprtc loopback on Debug, Desktop Capture Example extension on Release

This CL is part of the Mojo Video Capture work. For the bigger picture, 
see [1] CL1.9.10

BUG=584797
TEST=
  content_unittests --gtest_filter="*Video*", 
  video_capture_unittests, 
  Apprtc loopback on Debug, 
  Desktop Capture Example extension on Release

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
==========

[chfremer, 2016-12-14 22:17:40]

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

[commit-bot: I haz the power, 2016-12-14 22:18:14]

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

[chfremer, 2016-12-14 22:24:58]

Description was changed from

==========
[Mojo Video Capture] Simplify media::VideoCaptureDevice::Client:Buffer to a
struct

In interface media::VideoCaptureDevice::Client, change interface Buffer to a 
move-only struct. The new struct separates the concerns of providing read/write 
access, providing a transit handle for transport across processes, as well as a
usage reservation. This allows clients to explicitly obtain read/write access 
only when they need it. Clients who only want to pass along the buffer or move 
it across process boundaries do not need read/write access.
Separating the buffer reservation into an object with empy interface Ownership 
has the advantage of allowing us to pass around these reservations separately 
from the buffer handles. This will be particularly useful in the subsequent CL,
where we are going to do exactly that and update the VideoFrameReceiver 
interface to make it clear how frame delivery involves a set of pre-shared 
buffers that are alternately filled by the producer.

In interface media::VideoCaptureBufferPool, renamed method GetBufferHandle() to 
GetReadWriteAccess() to better reveal what it does.

Removed outdated implementation in video_capture Mojo service. We leave the 
implementation empty, until refactoring the interfaces has been completed. 

Tested: Apprtc loopback on Debug, Desktop Capture Example extension on Release

This CL is part of the Mojo Video Capture work. For the bigger picture, 
see [1] CL1.9.10

BUG=584797
TEST=
  content_unittests --gtest_filter="*Video*", 
  video_capture_unittests, 
  Apprtc loopback on Debug, 
  Desktop Capture Example extension on Release

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
==========

to

==========
[Mojo Video Capture] Simplify media::VideoCaptureDevice::Client:Buffer to a
struct

In interface media::VideoCaptureDevice::Client, change interface Buffer to a 
move-only struct. The new struct separates the concerns of providing read/write 
access, providing a transit handle for transport across processes, as well as a
usage reservation. This allows clients to explicitly obtain read/write access 
only when they need it. Clients who only want to pass along the buffer or move 
it across process boundaries do not need read/write access.
Separating the buffer reservation into an object with empty interface 
"Ownership" has the advantage of allowing us to pass around these reservations
separately from the buffer handles. This will be particularly useful in the 
subsequent CL, where we are going to do exactly that and update the 
VideoFrameReceiver interface to make it clear how frame delivery involves a set 
of pre-shared buffers that are alternately filled by the producer.

In interface media::VideoCaptureBufferPool, renamed method GetBufferHandle() to 
GetReadWriteAccess() to better reveal what it does.

Removed outdated implementation in video_capture Mojo service. We leave the 
implementation empty, until refactoring the interfaces has been completed. 

Tested: Apprtc loopback on Debug, Desktop Capture Example extension on Release

This CL is part of the Mojo Video Capture work. For the bigger picture, 
see [1] CL1.9.10

BUG=584797
TEST=
  content_unittests --gtest_filter="*Video*", 
  video_capture_unittests, 
  Apprtc loopback on Debug, 
  Desktop Capture Example extension on Release

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
==========

[chfremer, 2016-12-14 22:25:48]

Description was changed from

==========
[Mojo Video Capture] Simplify media::VideoCaptureDevice::Client:Buffer to a
struct

In interface media::VideoCaptureDevice::Client, change interface Buffer to a 
move-only struct. The new struct separates the concerns of providing read/write 
access, providing a transit handle for transport across processes, as well as a
usage reservation. This allows clients to explicitly obtain read/write access 
only when they need it. Clients who only want to pass along the buffer or move 
it across process boundaries do not need read/write access.
Separating the buffer reservation into an object with empty interface 
"Ownership" has the advantage of allowing us to pass around these reservations
separately from the buffer handles. This will be particularly useful in the 
subsequent CL, where we are going to do exactly that and update the 
VideoFrameReceiver interface to make it clear how frame delivery involves a set 
of pre-shared buffers that are alternately filled by the producer.

In interface media::VideoCaptureBufferPool, renamed method GetBufferHandle() to 
GetReadWriteAccess() to better reveal what it does.

Removed outdated implementation in video_capture Mojo service. We leave the 
implementation empty, until refactoring the interfaces has been completed. 

Tested: Apprtc loopback on Debug, Desktop Capture Example extension on Release

This CL is part of the Mojo Video Capture work. For the bigger picture, 
see [1] CL1.9.10

BUG=584797
TEST=
  content_unittests --gtest_filter="*Video*", 
  video_capture_unittests, 
  Apprtc loopback on Debug, 
  Desktop Capture Example extension on Release

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
==========

to

==========
[Mojo Video Capture] Simplify media::VideoCaptureDevice::Client:Buffer to a
struct

In interface media::VideoCaptureDevice::Client, change interface Buffer to a 
move-only struct. The new struct separates the concerns of providing read/write 
access, providing a transit handle for transport across processes, as well as a
usage reservation. This allows clients to explicitly obtain read/write access 
only when they need it. Clients who only want to pass along the buffer or move 
it across process boundaries do not need read/write access.
Separating the buffer reservation into an object with empty interface 
"Ownership" has the advantage of allowing us to pass around these reservations
separately from the buffer handles. This will be particularly useful in the 
subsequent CL, where we are going to do exactly that and update the 
VideoFrameReceiver interface to make it clear how frame delivery involves a set 
of pre-shared buffers that are alternately filled by the producer.

In interface media::VideoCaptureBufferPool, renamed method GetBufferHandle() to 
GetReadWriteAccess() to better reveal what it does.

Removed outdated implementation in video_capture Mojo service. We leave the 
implementation empty, until refactoring the interfaces has been completed. 

This CL is part of the Mojo Video Capture work. For the bigger picture, 
see [1] CL1.9.10

BUG=584797
TEST=
  content_unittests --gtest_filter="*Video*", 
  video_capture_unittests, 
  Apprtc loopback on Debug, 
  Desktop Capture Example extension on Release

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
==========

[chfremer, 2016-12-14 22:26:26]

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

[chfremer, 2016-12-14 22:26:27]

miu@: PTAL
mcasas@: PTAL

[commit-bot: I haz the power, 2016-12-14 22:44:33]

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

[commit-bot: I haz the power, 2016-12-14 22:44:35]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[chfremer, 2016-12-14 22:52:00]

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

[commit-bot: I haz the power, 2016-12-14 22:52:22]

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

[commit-bot: I haz the power, 2016-12-15 01:15:18]

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

[commit-bot: I haz the power, 2016-12-15 01:15:18]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[chfremer, 2016-12-15 17:29:46]

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

[commit-bot: I haz the power, 2016-12-15 17:30:13]

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

[commit-bot: I haz the power, 2016-12-15 19:16:42]

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

[commit-bot: I haz the power, 2016-12-15 19:16:42]

Dry run: This issue passed the CQ dry run.

[chfremer, 2016-12-15 19:27:22]

Description was changed from

==========
[Mojo Video Capture] Simplify media::VideoCaptureDevice::Client:Buffer to a
struct

In interface media::VideoCaptureDevice::Client, change interface Buffer to a 
move-only struct. The new struct separates the concerns of providing read/write 
access, providing a transit handle for transport across processes, as well as a
usage reservation. This allows clients to explicitly obtain read/write access 
only when they need it. Clients who only want to pass along the buffer or move 
it across process boundaries do not need read/write access.
Separating the buffer reservation into an object with empty interface 
"Ownership" has the advantage of allowing us to pass around these reservations
separately from the buffer handles. This will be particularly useful in the 
subsequent CL, where we are going to do exactly that and update the 
VideoFrameReceiver interface to make it clear how frame delivery involves a set 
of pre-shared buffers that are alternately filled by the producer.

In interface media::VideoCaptureBufferPool, renamed method GetBufferHandle() to 
GetReadWriteAccess() to better reveal what it does.

Removed outdated implementation in video_capture Mojo service. We leave the 
implementation empty, until refactoring the interfaces has been completed. 

This CL is part of the Mojo Video Capture work. For the bigger picture, 
see [1] CL1.9.10

BUG=584797
TEST=
  content_unittests --gtest_filter="*Video*", 
  video_capture_unittests, 
  Apprtc loopback on Debug, 
  Desktop Capture Example extension on Release

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
==========

to

==========
[Mojo Video Capture] Simplify media::VideoCaptureDevice::Client:Buffer to a
struct

In interface media::VideoCaptureDevice::Client, change interface Buffer to a 
move-only struct. The new struct separates the concerns of providing read/write 
access, providing a transit handle for transport across processes, as well as a
usage reservation. This allows clients to explicitly obtain read/write access 
only when they need it. Clients who only want to pass along the buffer or move 
it across process boundaries do not need read/write access.
Separating the buffer reservation into an object with empty 
(i.e., destructor-only) interface "Ownership" has the advantage of allowing us 
to pass around these reservations separately from the buffer handles. This will 
be particularly useful in the subsequent CL, where we are going to do exactly 
that and update the VideoFrameReceiver interface to make it clear how frame 
delivery involves a set of pre-shared buffers that are alternately filled by the
producer. See [2] for a peek at the follow-up CL.

In interface media::VideoCaptureBufferPool, renamed method GetBufferHandle() to 
GetReadWriteAccess() to better reveal what it does.

Removed outdated implementation in video_capture Mojo service. We leave the 
implementation empty, until refactoring the interfaces has been completed. 

This CL is part of the Mojo Video Capture work. For the bigger picture, 
see [1] CL1.9.10

BUG=584797
TEST=
  content_unittests --gtest_filter="*Video*", 
  video_capture_unittests, 
  Apprtc loopback on Debug, 
  Desktop Capture Example extension on Release

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
[2] https://codereview.chromium.org/2583603002/
==========

[chfremer, 2016-12-15 19:32:26]

I updated the CL description to contain a link to the follow-up CL, which may be
useful in demonstrating the intended use of the Ownership interface.

See https://codereview.chromium.org/2583603002/

[chfremer, 2016-12-15 23:13:53]

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

[commit-bot: I haz the power, 2016-12-15 23:15:14]

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

[chfremer, 2016-12-15 23:22:56]

Patchset #4 (id:60001) has been deleted

[chfremer, 2016-12-15 23:23:53]

Description was changed from

==========
[Mojo Video Capture] Simplify media::VideoCaptureDevice::Client:Buffer to a
struct

In interface media::VideoCaptureDevice::Client, change interface Buffer to a 
move-only struct. The new struct separates the concerns of providing read/write 
access, providing a transit handle for transport across processes, as well as a
usage reservation. This allows clients to explicitly obtain read/write access 
only when they need it. Clients who only want to pass along the buffer or move 
it across process boundaries do not need read/write access.
Separating the buffer reservation into an object with empty 
(i.e., destructor-only) interface "Ownership" has the advantage of allowing us 
to pass around these reservations separately from the buffer handles. This will 
be particularly useful in the subsequent CL, where we are going to do exactly 
that and update the VideoFrameReceiver interface to make it clear how frame 
delivery involves a set of pre-shared buffers that are alternately filled by the
producer. See [2] for a peek at the follow-up CL.

In interface media::VideoCaptureBufferPool, renamed method GetBufferHandle() to 
GetReadWriteAccess() to better reveal what it does.

Removed outdated implementation in video_capture Mojo service. We leave the 
implementation empty, until refactoring the interfaces has been completed. 

This CL is part of the Mojo Video Capture work. For the bigger picture, 
see [1] CL1.9.10

BUG=584797
TEST=
  content_unittests --gtest_filter="*Video*", 
  video_capture_unittests, 
  Apprtc loopback on Debug, 
  Desktop Capture Example extension on Release

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
[2] https://codereview.chromium.org/2583603002/
==========

to

==========
[Mojo Video Capture] Simplify media::VideoCaptureDevice::Client:Buffer to a
struct

In interface media::VideoCaptureDevice::Client, change interface Buffer to a 
move-only struct. The new struct separates the concerns of providing read/write 
access, providing a transit handle for transport across processes, as well as a
usage reservation. This allows clients to explicitly obtain read/write access 
only when they need it. Clients who only want to pass along the buffer or move 
it across process boundaries do not need read/write access.

In interface media::VideoCaptureBufferPool, renamed method GetBufferHandle() to 
GetReadWriteAccess() to better reveal what it does.

Removed outdated implementation in video_capture Mojo service. We leave the 
implementation empty, until refactoring the interfaces has been completed. 

This CL is part of the Mojo Video Capture work. For the bigger picture, 
see [1] CL1.9.10

BUG=584797
TEST=
  content_unittests --gtest_filter="*Video*", 
  video_capture_unittests, 
  Apprtc loopback on Debug, 
  Desktop Capture Example extension on Release

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
==========

[chfremer, 2016-12-15 23:24:26]

mcasas, emircan, and I had an offline discussion about the usage of an empty
interface "Ownership". Even though the conclusion was that this can be a valid
pattern, mcasas brought up the idea of using a single interface that does both
providing methods for accessing the buffer as well as encapsulating ownership of
a usage reservation.

This made me realize that handling the usage reservation separate from the
BufferAccessProvider is indeed not needed, at least at this stage. I uploaded a
PatchSet that merges the usage reservation into the BufferAccessProvider, which 
makes things a little simpler.

[chfremer, 2016-12-15 23:25:18]

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

[commit-bot: I haz the power, 2016-12-15 23:26:33]

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

[commit-bot: I haz the power, 2016-12-16 01:55:53]

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

[commit-bot: I haz the power, 2016-12-16 01:55:54]

Dry run: This issue passed the CQ dry run.

[miu, 2016-12-20 22:25:37]

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

https://codereview.chromium.org/2573223002/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.cc:375: const int
buffer_id = buffer.id;
If |frame_buffer_pool_| is null, should we return early here?

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

https://codereview.chromium.org/2573223002/diff/80001/media/capture/video/vid...
media/capture/video/video_capture_device.h:98: struct CAPTURE_EXPORT Buffer {
naming: I'd suggest renaming this to BufferPtr because:

1. It carries pointer/ID information only, not the actual payload.
2. Is a move-only type that represents ownership.
3. Conventionally similar to objects of similar behavior (e.g., Mojo FooPtrs).

https://codereview.chromium.org/2573223002/diff/80001/media/capture/video/vid...
media/capture/video/video_capture_device.h:98: struct CAPTURE_EXPORT Buffer {
Random thinking out-loud: IIUC the usage cases correctly, it seems that this
class is really just a smart pointer type with a couple of ID fields as metadata
to be carried alongside and eventually passed-back to VCD::Client. Thus, I'm
wondering if it would simplify a little to write it this way:

  class CAPTURE_EXPORT BufferPtr : public std::unique_ptr<BufferAccessProvider>
{
   public:
    ...ctors/dtor as before...

    int id() const { return id_; }
    int frame_feedback_id() const { return frame_feedback_id_; }

   private:
    int id_, frame_feedback_id_;
  };

Then, code that uses this might look like:

  BufferPtr buffer = client->ReserveOutputBuffer(...);
  if (buffer) {
    MyRoutineToPopulateVideoFrameBuffer(buffer->GetReadWriteAccess());
    client->OnIncomingCapturedBuffer(std::move(buffer), ...);
  } else {
    LOG(ERROR) << "Failed to reserve output buffer for next video frame.";
  }

https://codereview.chromium.org/2573223002/diff/80001/media/capture/video/vid...
media/capture/video/video_capture_device.h:98: struct CAPTURE_EXPORT Buffer {
Putting the prior two comments together, it then seems that we can rename
VideoCaptureBufferHandle to just "VideoCaptureBuffer" to better reflect that it
is the container of the data.

https://codereview.chromium.org/2573223002/diff/80001/media/capture/video/vid...
media/capture/video/video_capture_device.h:108: int id;
It seems weird to expose these data members publicly. Do you expect client code
to need to change these?

IMHO, it feels like this should go back to its prior "class" form without
virtualizing the methods.

https://codereview.chromium.org/2573223002/diff/80001/media/capture/video/vid...
File media/capture/video/video_capture_device_client.cc (right):

https://codereview.chromium.org/2573223002/diff/80001/media/capture/video/vid...
media/capture/video/video_capture_device_client.cc:46: :
buffer_pool_(std::move(buffer_pool)), buffer_id_(buffer_id) {}
Suggestion: In the ctor body, add a comment to the effect of:

  // The code that invoked this contstructor has already made a
  // producer reservation on the buffer from pool.

I mention this because, when I saw the dtor relinquishing the reservation, I
immediately asked, "Wait...Where was the reservation made to begin with?"

https://codereview.chromium.org/2573223002/diff/80001/media/capture/video/vid...
media/capture/video/video_capture_device_client.cc:362:
VideoCaptureDevice::Client::Buffer VideoCaptureDeviceClient::MakeBufferStruct(
naming: If you take certain of my other suggestions, this function should
probably be named "MakeBufferPtr" instead.

https://codereview.chromium.org/2573223002/diff/80001/media/capture/video/vid...
File media/capture/video/video_capture_device_client.h (right):

https://codereview.chromium.org/2573223002/diff/80001/media/capture/video/vid...
media/capture/video/video_capture_device_client.h:84: static Buffer
MakeBufferStruct(
Outside of this class, it seems this function is only called for unit tests.
Would it be possible for the unit tests to call ReserveOutputBuffer() instead?
Then, you don't need this.

[chfremer, 2016-12-22 18:50:41]

Description was changed from

==========
[Mojo Video Capture] Simplify media::VideoCaptureDevice::Client:Buffer to a
struct

In interface media::VideoCaptureDevice::Client, change interface Buffer to a 
move-only struct. The new struct separates the concerns of providing read/write 
access, providing a transit handle for transport across processes, as well as a
usage reservation. This allows clients to explicitly obtain read/write access 
only when they need it. Clients who only want to pass along the buffer or move 
it across process boundaries do not need read/write access.

In interface media::VideoCaptureBufferPool, renamed method GetBufferHandle() to 
GetReadWriteAccess() to better reveal what it does.

Removed outdated implementation in video_capture Mojo service. We leave the 
implementation empty, until refactoring the interfaces has been completed. 

This CL is part of the Mojo Video Capture work. For the bigger picture, 
see [1] CL1.9.10

BUG=584797
TEST=
  content_unittests --gtest_filter="*Video*", 
  video_capture_unittests, 
  Apprtc loopback on Debug, 
  Desktop Capture Example extension on Release

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
==========

to

==========
[Mojo Video Capture] Simplify media::VideoCaptureDevice::Client:Buffer to a
struct

In interface media::VideoCaptureDevice::Client, change interface Buffer to a 
move-only struct. The new struct separates the concerns of providing read/write 
access, providing a transit handle for transport across processes, as well as a
usage reservation. This allows clients to explicitly obtain read/write access 
only when they need it. Clients who only want to pass along the buffer or move 
it across process boundaries do not need read/write access.

In interface media::VideoCaptureBufferPool, renamed method GetBufferHandle() to 
GetReadWriteAccess() to better reveal what it does.

Removed outdated implementation in video_capture Mojo service. We leave the 
implementation empty, until refactoring the interfaces has been completed. 

This CL is part of the Mojo Video Capture work. For the bigger picture, 
see [1] CL1.9.10

For a peek at the follow-up CL, please see 
https://codereview.chromium.org/2583603002/

BUG=584797
TEST=
  content_unittests --gtest_filter="*Video*", 
  video_capture_unittests, 
  Apprtc loopback on Debug, 
  Desktop Capture Example extension on Release

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
==========

[chfremer, 2016-12-22 18:58:22]

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

[commit-bot: I haz the power, 2016-12-22 18:58:43]

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

[chfremer, 2016-12-22 18:59:08]

Patchset #2 (id:20001) has been deleted

[chfremer, 2016-12-22 18:59:19]

Patchset #1 (id:1) has been deleted

[chfremer, 2016-12-22 18:59:35]

Patchset #1 (id:40001) has been deleted

[chfremer, 2016-12-22 19:01:20]

Addressed miu's comments.

miu@: PTAL
mcasas@: PTAL

Also note the follow-up CL, in case you want to have a look:
https://codereview.chromium.org/2583603002/

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

https://codereview.chromium.org/2573223002/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.cc:375: const int
buffer_id = buffer.id;
On 2016/12/20 22:25:37, miu wrote:
> If |frame_buffer_pool_| is null, should we return early here?

No. Before this CL |frame_buffer_pool_| == null was illegal, because it was
needed in DoNewBufferOnIOThread(). After this CL it is legal. We may add an
entry to |buffer_id_to_state_map_| with |frame_buffer_pool_| == null, and if
SetFrameBufferPool() is called after that, the entry will get updated.

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

https://codereview.chromium.org/2573223002/diff/80001/media/capture/video/vid...
media/capture/video/video_capture_device.h:98: struct CAPTURE_EXPORT Buffer {
On 2016/12/20 22:25:37, miu wrote:
> Putting the prior two comments together, it then seems that we can rename
> VideoCaptureBufferHandle to just "VideoCaptureBuffer" to better reflect that
it
> is the container of the data.

Hmm, the way I see it, VideoCaptureBufferHandle is an abstraction to something
that provides read/write access to a block of memory. The ...Handle suffix seems
appropriate for this. Note that implementations of VideoCaptureBufferHandle may
or may not be directly owning the block of memory. In our case, the
implementation is SharedMemoryBufferHandle, which does not own the memory. It
just points to a SharedMemoryBufferTracker, which, in turn, is owned by the
buffer pool.

https://codereview.chromium.org/2573223002/diff/80001/media/capture/video/vid...
media/capture/video/video_capture_device.h:98: struct CAPTURE_EXPORT Buffer {
On 2016/12/20 22:25:37, miu wrote:
> Random thinking out-loud: IIUC the usage cases correctly, it seems that this
> class is really just a smart pointer type with a couple of ID fields as
metadata
> to be carried alongside and eventually passed-back to VCD::Client. Thus, I'm
> wondering if it would simplify a little to write it this way:
> 
>   class CAPTURE_EXPORT BufferPtr : public
std::unique_ptr<BufferAccessProvider>
> {
>    public:
>     ...ctors/dtor as before...
> 
>     int id() const { return id_; }
>     int frame_feedback_id() const { return frame_feedback_id_; }
> 
>    private:
>     int id_, frame_feedback_id_;
>   };
> 
> Then, code that uses this might look like:
> 
>   BufferPtr buffer = client->ReserveOutputBuffer(...);
>   if (buffer) {
>     MyRoutineToPopulateVideoFrameBuffer(buffer->GetReadWriteAccess());
>     client->OnIncomingCapturedBuffer(std::move(buffer), ...);
>   } else {
>     LOG(ERROR) << "Failed to reserve output buffer for next video frame.";
>   }

I like this idea, and I would go with it if I didn't already know where things
are going in the follow-up CL, see [1]. In there, I am proposing to separate the
buffer reservation aspect from the |access_provider| into a separate object
|reservation|. This allows expressing the (pre-)sharing the buffers separately
from the usage reservations more clearly. 

Because of that, this struct is supposed to be nothing more than a plain old set
of members that can be accessed individually as needed. It only needs to exist
to allow multiple return values from things like ReserveOutputBuffer().

[1] https://codereview.chromium.org/2583603002/

https://codereview.chromium.org/2573223002/diff/80001/media/capture/video/vid...
media/capture/video/video_capture_device.h:98: struct CAPTURE_EXPORT Buffer {
On 2016/12/20 22:25:37, miu wrote:
> naming: I'd suggest renaming this to BufferPtr because:
> 
> 1. It carries pointer/ID information only, not the actual payload.
> 2. Is a move-only type that represents ownership.
> 3. Conventionally similar to objects of similar behavior (e.g., Mojo FooPtrs).

I agree this would make sense if we were to follow your suggestion to make it
inherit std::unique_ptr<BufferAccessProvider>. Please see my other response (and
maybe the subsequent CL [1]) as for why I recommend we don't do this. And when
treating this type as a plain set of members, we probably also don't want to go
with the ...Ptr suffix.

[1] https://codereview.chromium.org/2583603002/

https://codereview.chromium.org/2573223002/diff/80001/media/capture/video/vid...
media/capture/video/video_capture_device.h:108: int id;
On 2016/12/20 22:25:37, miu wrote:
> It seems weird to expose these data members publicly. Do you expect client
code
> to need to change these?
> 
> IMHO, it feels like this should go back to its prior "class" form without
> virtualizing the methods.

Done. Maybe my idea of "plain old set of members" was a bit too "plain". We want
|id| and |frame_feedback_id| to be read-only, but cannot make them const if we
want the type to be movable. As per your example, I will make them private and
provide get-only accessors.

|access_provider| will still stay a public field, so I am not sure if we should
call it a struct or a class then.

https://codereview.chromium.org/2573223002/diff/80001/media/capture/video/vid...
File media/capture/video/video_capture_device_client.cc (right):

https://codereview.chromium.org/2573223002/diff/80001/media/capture/video/vid...
media/capture/video/video_capture_device_client.cc:46: :
buffer_pool_(std::move(buffer_pool)), buffer_id_(buffer_id) {}
On 2016/12/20 22:25:37, miu wrote:
> Suggestion: In the ctor body, add a comment to the effect of:
> 
>   // The code that invoked this contstructor has already made a
>   // producer reservation on the buffer from pool.
> 
> I mention this because, when I saw the dtor relinquishing the reservation, I
> immediately asked, "Wait...Where was the reservation made to begin with?"

Done.

https://codereview.chromium.org/2573223002/diff/80001/media/capture/video/vid...
media/capture/video/video_capture_device_client.cc:362:
VideoCaptureDevice::Client::Buffer VideoCaptureDeviceClient::MakeBufferStruct(
On 2016/12/20 22:25:37, miu wrote:
> naming: If you take certain of my other suggestions, this function should
> probably be named "MakeBufferPtr" instead.

As per my response to the other suggestions, I recommend to not use the ...Ptr
suffix.

https://codereview.chromium.org/2573223002/diff/80001/media/capture/video/vid...
File media/capture/video/video_capture_device_client.h (right):

https://codereview.chromium.org/2573223002/diff/80001/media/capture/video/vid...
media/capture/video/video_capture_device_client.h:84: static Buffer
MakeBufferStruct(
On 2016/12/20 22:25:37, miu wrote:
> Outside of this class, it seems this function is only called for unit tests.
> Would it be possible for the unit tests to call ReserveOutputBuffer() instead?
> Then, you don't need this.

Note, that this method is static, which already indicates, that it does not
really belong here. The only reason it is here is that it uses the internal
class BufferPoolBufferAccessProvider defined in video_capture_device_client.cc.
If you like, we could move this to a new pair of files, something like
"video_capture_buffer_utils.h/cc". Please let me know if you want me to do that,
because otherwise I would default to leaving it here.

The unit test that is using it (in
web_contents_video_capture_device_unittest.cc) is doing so in order to provide a
stub implementation for media::VideoCaptureDevice::Client, so there is no
instance of VideoCaptureDeviceClient and I can't see where it would call
ReserveOutputBuffer on.

[commit-bot: I haz the power, 2016-12-22 20:50:45]

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

[commit-bot: I haz the power, 2016-12-22 20:50:46]

Dry run: This issue passed the CQ dry run.

[miu, 2016-12-27 23:38:49]

Comment threads from PS2 continued, and new PS3 comments:

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

https://codereview.chromium.org/2573223002/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.cc:375: const int
buffer_id = buffer.id;
On 2016/12/22 19:01:20, chfremer wrote:
> On 2016/12/20 22:25:37, miu wrote:
> > If |frame_buffer_pool_| is null, should we return early here?
> 
> No. Before this CL |frame_buffer_pool_| == null was illegal, because it was
> needed in DoNewBufferOnIOThread(). After this CL it is legal. We may add an
> entry to |buffer_id_to_state_map_| with |frame_buffer_pool_| == null, and if
> SetFrameBufferPool() is called after that, the entry will get updated.

Acknowledged.

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

https://codereview.chromium.org/2573223002/diff/80001/media/capture/video/vid...
media/capture/video/video_capture_device.h:98: struct CAPTURE_EXPORT Buffer {
On 2016/12/22 19:01:20, chfremer wrote:
> On 2016/12/20 22:25:37, miu wrote:
> > Random thinking out-loud: IIUC the usage cases correctly, it seems that this
> > class is really just a smart pointer type with a couple of ID fields as
> metadata
> > to be carried alongside and eventually passed-back to VCD::Client. Thus, I'm
> 
> I like this idea, and I would go with it if I didn't already know where things
> are going in the follow-up CL, see [1]. In there, I am proposing to separate
the
> buffer reservation aspect from the |access_provider| into a separate object
> |reservation|. This allows expressing the (pre-)sharing the buffers separately
> from the usage reservations more clearly. 

You have a better grasp on the big picture than I do; but, my understanding was
that there was an advantage in making the reservations closely tied to the
access of the buffers: Read-write access to a buffer implied a "producer
reservation" exists around it, and read-only access to a buffer implied a
"consumer reservation" exists. If these things become separated, wouldn't that
code structure make it easier to have unintended bugs where reservations can
begin and end while data access is taking place? I may be misunderstanding the
end game here. ;-)

https://codereview.chromium.org/2573223002/diff/80001/media/capture/video/vid...
media/capture/video/video_capture_device.h:108: int id;
On 2016/12/22 19:01:20, chfremer wrote:
> On 2016/12/20 22:25:37, miu wrote:
> > It seems weird to expose these data members publicly. Do you expect client
> code
> > to need to change these?
> 
> |access_provider| will still stay a public field, so I am not sure if we
should
> call it a struct or a class then.

Seems fine, since the intent is to allow public modification. IMHO it would be
overkill to write getXXX()/setXXX()/takeXXX() methods around it ("less code that
does more" philosophy).

https://codereview.chromium.org/2573223002/diff/80001/media/capture/video/vid...
File media/capture/video/video_capture_device_client.h (right):

https://codereview.chromium.org/2573223002/diff/80001/media/capture/video/vid...
media/capture/video/video_capture_device_client.h:84: static Buffer
MakeBufferStruct(
On 2016/12/22 19:01:20, chfremer wrote:
> On 2016/12/20 22:25:37, miu wrote:
> > Outside of this class, it seems this function is only called for unit tests.
> > Would it be possible for the unit tests to call ReserveOutputBuffer()
instead?
> > Then, you don't need this.
> 
> Note, that this method is static, which already indicates, that it does not
> really belong here. The only reason it is here is that it uses the internal
> class BufferPoolBufferAccessProvider defined in
video_capture_device_client.cc.
> If you like, we could move this to a new pair of files, something like
> "video_capture_buffer_utils.h/cc". Please let me know if you want me to do
that,
> because otherwise I would default to leaving it here.
> 
> The unit test that is using it (in
> web_contents_video_capture_device_unittest.cc) is doing so in order to provide
a
> stub implementation for media::VideoCaptureDevice::Client, so there is no
> instance of VideoCaptureDeviceClient and I can't see where it would call
> ReserveOutputBuffer on.

Acknowledged.

https://codereview.chromium.org/2573223002/diff/120001/media/capture/video/vi...
File media/capture/video/video_capture_device.h (right):

https://codereview.chromium.org/2573223002/diff/120001/media/capture/video/vi...
media/capture/video/video_capture_device.h:108:
std::unique_ptr<BufferAccessProvider> access_provider;
style-guide nit: Data members go after methods.

https://codereview.chromium.org/2573223002/diff/120001/media/capture/video/vi...
media/capture/video/video_capture_device.h:109: bool is_valid() { return
access_provider != nullptr; }
nit: These three methods should be const.

[chfremer, 2016-12-28 01:06:45]

miu@: PTAL

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

https://codereview.chromium.org/2573223002/diff/80001/media/capture/video/vid...
media/capture/video/video_capture_device.h:98: struct CAPTURE_EXPORT Buffer {
On 2016/12/27 23:38:48, miu wrote:
> On 2016/12/22 19:01:20, chfremer wrote:
> > On 2016/12/20 22:25:37, miu wrote:
> > > Random thinking out-loud: IIUC the usage cases correctly, it seems that
this
> > > class is really just a smart pointer type with a couple of ID fields as
> > metadata
> > > to be carried alongside and eventually passed-back to VCD::Client. Thus,
I'm
> > 
> > I like this idea, and I would go with it if I didn't already know where
things
> > are going in the follow-up CL, see [1]. In there, I am proposing to separate
> the
> > buffer reservation aspect from the |access_provider| into a separate object
> > |reservation|. This allows expressing the (pre-)sharing the buffers
separately
> > from the usage reservations more clearly. 
> 
> You have a better grasp on the big picture than I do; but, my understanding
was
> that there was an advantage in making the reservations closely tied to the
> access of the buffers: Read-write access to a buffer implied a "producer
> reservation" exists around it, and read-only access to a buffer implied a
> "consumer reservation" exists. If these things become separated, wouldn't that
> code structure make it easier to have unintended bugs where reservations can
> begin and end while data access is taking place? I may be misunderstanding the
> end game here. ;-)

You are making a good point, which makes me realize that I have not really been
clear on the design myself. The way I see it now is as follows. There are 3
"levels" of access to the shared buffers:
1. "Handle Access", i.e. access to a handle (either a pointer to mapped memory
for in-process consumers or an abstract handle that can be shared with
out-of-process consumers). Access to such a handle does imply neither read nor
write access to the contents of the buffer. "Handle Access" is provided
separately before any frame data are delivered.
2. "Read Access" is given to consumers when frame data is ready for consumption.
3. "ReadWrite Access" is only needed by the producer.

So far, these access levels were not clearly separated, which resulted in the
sharing and delivery mechanism for the frames being very hard to grasp based on
the interfaces. Currently, every call to
VideoFrameReceiver::OnIncomingCapturedVideoFrame() gets ReadWrite Access plus an
in-process handle plus access to a shareable out-of-process handle. The end game
here is to make things clear by giving only the type of access that is needed
when it is needed.

I have to admit, my current version of CL1.9.11 is still not quite there yet. I
will rework the Interfaces in there to make things clearer.

For this CL (1.9.10), this does not require any additional changes.
Am I correct in assuming that with this explanation you are okay with this
struct for now then? If not, please reply back to this thread. Otherwise, let us
continue the discussion on this in CL1.9.11.

https://codereview.chromium.org/2573223002/diff/120001/media/capture/video/vi...
File media/capture/video/video_capture_device.h (right):

https://codereview.chromium.org/2573223002/diff/120001/media/capture/video/vi...
media/capture/video/video_capture_device.h:108:
std::unique_ptr<BufferAccessProvider> access_provider;
On 2016/12/27 23:38:49, miu wrote:
> style-guide nit: Data members go after methods.

Done.

https://codereview.chromium.org/2573223002/diff/120001/media/capture/video/vi...
media/capture/video/video_capture_device.h:109: bool is_valid() { return
access_provider != nullptr; }
On 2016/12/27 23:38:49, miu wrote:
> nit: These three methods should be const.

Done.

[chfremer, 2016-12-29 01:15:21]

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

[commit-bot: I haz the power, 2016-12-29 01:15:36]

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

[chfremer, 2016-12-29 01:29:08]

Description was changed from

==========
[Mojo Video Capture] Simplify media::VideoCaptureDevice::Client:Buffer to a
struct

In interface media::VideoCaptureDevice::Client, change interface Buffer to a 
move-only struct. The new struct separates the concerns of providing read/write 
access, providing a transit handle for transport across processes, as well as a
usage reservation. This allows clients to explicitly obtain read/write access 
only when they need it. Clients who only want to pass along the buffer or move 
it across process boundaries do not need read/write access.

In interface media::VideoCaptureBufferPool, renamed method GetBufferHandle() to 
GetReadWriteAccess() to better reveal what it does.

Removed outdated implementation in video_capture Mojo service. We leave the 
implementation empty, until refactoring the interfaces has been completed. 

This CL is part of the Mojo Video Capture work. For the bigger picture, 
see [1] CL1.9.10

For a peek at the follow-up CL, please see 
https://codereview.chromium.org/2583603002/

BUG=584797
TEST=
  content_unittests --gtest_filter="*Video*", 
  video_capture_unittests, 
  Apprtc loopback on Debug, 
  Desktop Capture Example extension on Release

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
==========

to

==========
[Mojo Video Capture] Simplify media::VideoCaptureDevice::Client:Buffer to a
struct

In interface media::VideoCaptureDevice::Client, change interface Buffer to a 
move-only struct. The new struct separates the concerns of providing access
handles (for in-process as well as for inter-process transit) from access
permissions. This allows clients to explicitly obtain in-process memory-mapped 
handles when they need it. Clients who only want to pass along the buffer or
move it across process boundaries do not need memory-mapped handles.

In interface media::VideoCaptureBufferPool, renamed 
 GetBufferHandle() -> GetHandleForInProcessAccess()
 GetHandleForTransit() -> GetHandleForInterProcessTransit().

Removed outdated implementation in video_capture Mojo service. We leave the 
implementation empty, until refactoring the interfaces has been completed. 

This CL is part of the Mojo Video Capture work. For the bigger picture, 
see [1] CL1.9.10

For a peek at the follow-up CL, please see 
https://codereview.chromium.org/2583603002/

BUG=584797
TEST=
  content_unittests --gtest_filter="*Video*", 
  video_capture_unittests, 
  Apprtc loopback on Debug, 
  Desktop Capture Example extension on Release

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
==========

[chfremer, 2016-12-29 01:29:39]

Description was changed from

==========
[Mojo Video Capture] Simplify media::VideoCaptureDevice::Client:Buffer to a
struct

In interface media::VideoCaptureDevice::Client, change interface Buffer to a 
move-only struct. The new struct separates the concerns of providing access
handles (for in-process as well as for inter-process transit) from access
permissions. This allows clients to explicitly obtain in-process memory-mapped 
handles when they need it. Clients who only want to pass along the buffer or
move it across process boundaries do not need memory-mapped handles.

In interface media::VideoCaptureBufferPool, renamed 
 GetBufferHandle() -> GetHandleForInProcessAccess()
 GetHandleForTransit() -> GetHandleForInterProcessTransit().

Removed outdated implementation in video_capture Mojo service. We leave the 
implementation empty, until refactoring the interfaces has been completed. 

This CL is part of the Mojo Video Capture work. For the bigger picture, 
see [1] CL1.9.10

For a peek at the follow-up CL, please see 
https://codereview.chromium.org/2583603002/

BUG=584797
TEST=
  content_unittests --gtest_filter="*Video*", 
  video_capture_unittests, 
  Apprtc loopback on Debug, 
  Desktop Capture Example extension on Release

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
==========

to

==========
[Mojo Video Capture] Simplify media::VideoCaptureDevice::Client:Buffer to a
struct

In interface media::VideoCaptureDevice::Client, change interface Buffer to a 
move-only struct. The new struct separates the concerns of providing access 
handles (for in-process as well as for inter-process transit) from access 
permissions. This allows clients to explicitly obtain in-process memory-mapped 
handles when they need it. Clients who only want to pass along the buffer or 
move it across process boundaries do not need memory-mapped handles.

In interface media::VideoCaptureBufferPool, renamed 
 GetBufferHandle() -> GetHandleForInProcessAccess()
 GetHandleForTransit() -> GetHandleForInterProcessTransit().

Removed outdated implementation in video_capture Mojo service. We leave the 
implementation empty, until refactoring the interfaces has been completed. 

This CL is part of the Mojo Video Capture work. For the bigger picture, 
see [1] CL1.9.10

For a peek at the follow-up CL, please see 
https://codereview.chromium.org/2583603002/

BUG=584797
TEST=
  content_unittests --gtest_filter="*Video*", 
  video_capture_unittests, 
  Apprtc loopback on Debug, 
  Desktop Capture Example extension on Release

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
==========

[commit-bot: I haz the power, 2016-12-29 02:48:21]

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

[commit-bot: I haz the power, 2016-12-29 02:48:22]

Dry run: This issue passed the CQ dry run.

[chfremer, 2016-12-29 20:04:16]

Description was changed from

==========
[Mojo Video Capture] Simplify media::VideoCaptureDevice::Client:Buffer to a
struct

In interface media::VideoCaptureDevice::Client, change interface Buffer to a 
move-only struct. The new struct separates the concerns of providing access 
handles (for in-process as well as for inter-process transit) from access 
permissions. This allows clients to explicitly obtain in-process memory-mapped 
handles when they need it. Clients who only want to pass along the buffer or 
move it across process boundaries do not need memory-mapped handles.

In interface media::VideoCaptureBufferPool, renamed 
 GetBufferHandle() -> GetHandleForInProcessAccess()
 GetHandleForTransit() -> GetHandleForInterProcessTransit().

Removed outdated implementation in video_capture Mojo service. We leave the 
implementation empty, until refactoring the interfaces has been completed. 

This CL is part of the Mojo Video Capture work. For the bigger picture, 
see [1] CL1.9.10

For a peek at the follow-up CL, please see 
https://codereview.chromium.org/2583603002/

BUG=584797
TEST=
  content_unittests --gtest_filter="*Video*", 
  video_capture_unittests, 
  Apprtc loopback on Debug, 
  Desktop Capture Example extension on Release

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
==========

to

==========
[Mojo Video Capture] Simplify media::VideoCaptureDevice::Client:Buffer to a
struct

In interface media::VideoCaptureDevice::Client, change interface Buffer to a 
move-only struct. The new struct separates the concerns of providing access 
handles (for in-process as well as for inter-process transit) from access 
permissions. This allows clients to explicitly obtain in-process memory-mapped 
handles when they need it. Clients who only want to pass along the buffer or 
move it across process boundaries do not need memory-mapped handles.

In interface media::VideoCaptureBufferPool, renamed 
 GetBufferHandle() -> GetHandleForInProcessAccess()
 GetHandleForTransit() -> GetHandleForInterProcessTransit().

Removed outdated implementation in video_capture Mojo service. We leave the 
implementation empty, until refactoring the interfaces has been completed. 

This CL is part of the Mojo Video Capture work. For the bigger picture, 
see [1] CL1.9.10

For a peek at the follow-up CLs, please see 
https://codereview.chromium.org/2607203002/
https://codereview.chromium.org/2602983002/

BUG=584797
TEST=
  content_unittests --gtest_filter="*Video*", 
  video_capture_unittests, 
  Apprtc loopback on Debug, 
  Desktop Capture Example extension on Release

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
==========

[miu, 2016-12-30 01:03:22]

PS5 lgtm % suggestion in comment below.

I really like the new naming in PS5, as it really makes clear why things are
structured this way and how objects/interfaces should be used. You've taken a
very complex situation here and, IMHO, made it much easier for future
developers, who would be unfamiliar with this code, to understand what's going
on.

https://codereview.chromium.org/2573223002/diff/160001/media/capture/video/vi...
File media/capture/video/video_capture_device.h (right):

https://codereview.chromium.org/2573223002/diff/160001/media/capture/video/vi...
media/capture/video/video_capture_device.h:89: class CAPTURE_EXPORT Ownership {
I think I've figured out why we've all been flip-flopping on this
"destructor-only interface." I think your current patch set shows clearly why it
is needed and desired, however the naming is incredibly abstract, so it always
"feels weird" to see it.

Thus, I suggest more-descriptive naming here. Something that says why this is
needed, but also rationalizes why it's hiding something internal that external
code need not know about. Here are some suggestions I've come up with, but feel
free to roll your own:

  ScopedPermission
  ScopedReservation
  PermissionLock

(BTW--The "ScopedXXX" is very consistent with the naming around similar usage
patterns throughout Chromium.)

One other thing to consider, but this is totally your call: I would suggest
making these classes (i.e., BufferHandleProvider and "whatever Ownership is
called next") inner classes of the VCD::Client::Buffer struct. Also, if
BufferHandleProvider is an inner class of Buffer, you could just name it
HandleProvider.

[chfremer, 2016-12-30 04:04:56]

On 2016/12/30 01:03:22, miu wrote:
> PS5 lgtm % suggestion in comment below.
> 
> I really like the new naming in PS5, as it really makes clear why things are
> structured this way and how objects/interfaces should be used. You've taken a
> very complex situation here and, IMHO, made it much easier for future
> developers, who would be unfamiliar with this code, to understand what's going
> on.

Thanks so much for seeing that. That makes all the work put into it feel worth
it.

>
https://codereview.chromium.org/2573223002/diff/160001/media/capture/video/vi...
> File media/capture/video/video_capture_device.h (right):
> 
>
https://codereview.chromium.org/2573223002/diff/160001/media/capture/video/vi...
> media/capture/video/video_capture_device.h:89: class CAPTURE_EXPORT Ownership
{
> I think I've figured out why we've all been flip-flopping on this
> "destructor-only interface." I think your current patch set shows clearly why
it
> is needed and desired, however the naming is incredibly abstract, so it always
> "feels weird" to see it.
> 
> Thus, I suggest more-descriptive naming here. Something that says why this is
> needed, but also rationalizes why it's hiding something internal that external
> code need not know about. Here are some suggestions I've come up with, but
feel
> free to roll your own:
> 
>   ScopedPermission
>   ScopedReservation
>   PermissionLock
> 
> (BTW--The "ScopedXXX" is very consistent with the naming around similar usage
> patterns throughout Chromium.)

I'm probably not gonna make a decision before the new year, but just some quick 
thought on the naming here: ScopedXXX seems reasonable, especially considering 
similar usage in Chromium. ScopedPermission makes sense in this particular
example.
However, I feel it would be wise (here and in general) to keep the type name
abstract 
and instead use the variable name to indicate the specifics of what an instance
means 
in a particular context.
There will be an example showcasing why, coming up in CL1.9.15. In that CL I
will be 
proposing to use a destructor-only interface for passing temporary shared
ownership 
of required context to a method that runs asynchronously. The temporary 
ownership guarantees that the required context stays alive even if the caller of
the 
method is destroyed while the method executes in the background. I feel that 
"ScopedPermission" would not be a good name for this case. And I don't think it 
would be wise to to introduce a separate destructor-only interface with
different name.
That is why I would choose "Ownership" as the type name, and variable name 
|buffer_read_permission| in one use case and |context_reference| in the other
use case.
Please let me know if you have any early thoughts on that.
I guess "ScopedOwnership", or "ScopedReference", or "ScopedOwnedReference" would

work in all cases. Also, I would be okay with going with "ScopedPermission" for
now and 
then continue discussing in context of a concrete example once we reach
CL1.9.15.
   
> One other thing to consider, but this is totally your call: I would suggest
> making these classes (i.e., BufferHandleProvider and "whatever Ownership is
> called next") inner classes of the VCD::Client::Buffer struct. Also, if
> BufferHandleProvider is an inner class of Buffer, you could just name it
> HandleProvider.

[miu, 2016-12-30 04:55:17]

On 2016/12/30 04:04:56, chfremer wrote:
> That is why I would choose "Ownership" as the type name, and variable name 
> |buffer_read_permission| in one use case and |context_reference| in the other
> use case.
> Please let me know if you have any early thoughts on that.
> I guess "ScopedOwnership", or "ScopedReference", or "ScopedOwnedReference"
would
> 
> work in all cases. Also, I would be okay with going with "ScopedPermission"
for
> now and 
> then continue discussing in context of a concrete example once we reach
> CL1.9.15.

In situations like this, sometimes I knowingly check-in the code that is "best
for now," even knowing I will have to change it again in a soon-upcoming CL.
When you decide to change it to Ownership in the next CL, we'll be able to
discuss that in a more relevant context.

[chfremer, 2017-01-03 23:15:38]

Incorporated miu's suggestions.

mcasas@: PTAL

https://codereview.chromium.org/2573223002/diff/160001/media/capture/video/vi...
File media/capture/video/video_capture_device.h (right):

https://codereview.chromium.org/2573223002/diff/160001/media/capture/video/vi...
media/capture/video/video_capture_device.h:89: class CAPTURE_EXPORT Ownership {
On 2016/12/30 01:03:22, miu wrote:
> I think I've figured out why we've all been flip-flopping on this
> "destructor-only interface." I think your current patch set shows clearly why
it
> is needed and desired, however the naming is incredibly abstract, so it always
> "feels weird" to see it.
> 
> Thus, I suggest more-descriptive naming here. Something that says why this is
> needed, but also rationalizes why it's hiding something internal that external
> code need not know about. Here are some suggestions I've come up with, but
feel
> free to roll your own:
> 
>   ScopedPermission
>   ScopedReservation
>   PermissionLock
> 
> (BTW--The "ScopedXXX" is very consistent with the naming around similar usage
> patterns throughout Chromium.)
> 
> One other thing to consider, but this is totally your call: I would suggest
> making these classes (i.e., BufferHandleProvider and "whatever Ownership is
> called next") inner classes of the VCD::Client::Buffer struct. Also, if
> BufferHandleProvider is an inner class of Buffer, you could just name it
> HandleProvider.

Done.

[chfremer, 2017-01-03 23:16:16]

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

[commit-bot: I haz the power, 2017-01-03 23:16:52]

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

[commit-bot: I haz the power, 2017-01-04 01:40:17]

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

[commit-bot: I haz the power, 2017-01-04 01:40:18]

Dry run: This issue passed the CQ dry run.

[mcasas, 2017-01-05 20:37:02]

https://codereview.chromium.org/2573223002/diff/200001/content/browser/render...
File content/browser/renderer_host/media/video_capture_controller.h (right):

https://codereview.chromium.org/2573223002/diff/200001/content/browser/render...
content/browser/renderer_host/media/video_capture_controller.h:73: // before
they can provide the observer. (This is the case with
If you're using Sublime, Sublime-wrap-plus [1] can help reflowing
this paragraph (e.g. I see "separate" can go in l.72, right?) 


[1] https://github.com/ehuss/Sublime-Wrap-Plus

https://codereview.chromium.org/2573223002/diff/200001/media/capture/video/vi...
File media/capture/video/video_capture_device.cc (right):

https://codereview.chromium.org/2573223002/diff/200001/media/capture/video/vi...
media/capture/video/video_capture_device.cc:37: }
Can be put in l.37, and I think my clang-format does that.

https://codereview.chromium.org/2573223002/diff/200001/media/capture/video/vi...
File media/capture/video/video_capture_device.h (right):

https://codereview.chromium.org/2573223002/diff/200001/media/capture/video/vi...
media/capture/video/video_capture_device.h:92: struct CAPTURE_EXPORT Buffer {
A struct with private members and accessors and subclasses
is more a class, really -- also because |handle_provider| and
|buffer_read_write_permission| are unique_ptr<>s, so what 
is exactly happening if they're accessed (move, or access by
pointer) is not clear from the data type.  I'd vote for class and
accessors.

https://codereview.chromium.org/2573223002/diff/200001/media/capture/video/vi...
media/capture/video/video_capture_device.h:115:
std::unique_ptr<ScopedAccessPermission> buffer_read_write_permission);
s/buffer_read_write_permission/access_permission/ ?
Also the member.

https://codereview.chromium.org/2573223002/diff/200001/media/capture/video/vi...
media/capture/video/video_capture_device.h:130: };
I think that to make Buffer uncopyable, we have to add
here
DISALLOW_COPY_AND_ASSIGN(Buffer)

to explicitly delete the "normal" copy and assign ctors, right?

https://cs.chromium.org/chromium/src/base/macros.h?q=DISALLOW_COPY&sq=package...

https://codereview.chromium.org/2573223002/diff/200001/media/capture/video/vi...
File media/capture/video/video_capture_device_client.cc (right):

https://codereview.chromium.org/2573223002/diff/200001/media/capture/video/vi...
media/capture/video/video_capture_device_client.cc:48: // The code invoking this
contstructor is supposed to have already made a
s/contstructor/constructor/

would there be a way to enforce this comment with
static_assert()s or DCHECK()s?

https://codereview.chromium.org/2573223002/diff/200001/media/capture/video/vi...
File media/capture/video/video_capture_device_client.h (right):

https://codereview.chromium.org/2573223002/diff/200001/media/capture/video/vi...
media/capture/video/video_capture_device_client.h:87: int frame_feedback_id);
Usually class functions are declared at the top of the
section (public/private).

[chfremer, 2017-01-05 22:44:48]

mcasas@: PTAL

https://codereview.chromium.org/2573223002/diff/200001/content/browser/render...
File content/browser/renderer_host/media/video_capture_controller.h (right):

https://codereview.chromium.org/2573223002/diff/200001/content/browser/render...
content/browser/renderer_host/media/video_capture_controller.h:73: // before
they can provide the observer. (This is the case with
On 2017/01/05 20:37:01, mcasas wrote:
> If you're using Sublime, Sublime-wrap-plus [1] can help reflowing
> this paragraph (e.g. I see "separate" can go in l.72, right?) 
> 
> 
> [1] https://github.com/ehuss/Sublime-Wrap-Plus

Done. Thanks for the link to the plugin.
Super useful.

https://codereview.chromium.org/2573223002/diff/200001/media/capture/video/vi...
File media/capture/video/video_capture_device.cc (right):

https://codereview.chromium.org/2573223002/diff/200001/media/capture/video/vi...
media/capture/video/video_capture_device.cc:37: }
On 2017/01/05 20:37:01, mcasas wrote:
> Can be put in l.37, and I think my clang-format does that.

Done.

https://codereview.chromium.org/2573223002/diff/200001/media/capture/video/vi...
File media/capture/video/video_capture_device.h (right):

https://codereview.chromium.org/2573223002/diff/200001/media/capture/video/vi...
media/capture/video/video_capture_device.h:92: struct CAPTURE_EXPORT Buffer {
On 2017/01/05 20:37:02, mcasas wrote:
> A struct with private members and accessors and subclasses
> is more a class, really -- also because |handle_provider| and
> |buffer_read_write_permission| are unique_ptr<>s, so what 
> is exactly happening if they're accessed (move, or access by
> pointer) is not clear from the data type.  I'd vote for class and
> accessors.

Done.

https://codereview.chromium.org/2573223002/diff/200001/media/capture/video/vi...
media/capture/video/video_capture_device.h:115:
std::unique_ptr<ScopedAccessPermission> buffer_read_write_permission);
On 2017/01/05 20:37:02, mcasas wrote:
> s/buffer_read_write_permission/access_permission/ ?
> Also the member.

Done.

https://codereview.chromium.org/2573223002/diff/200001/media/capture/video/vi...
media/capture/video/video_capture_device.h:130: };
On 2017/01/05 20:37:02, mcasas wrote:
> I think that to make Buffer uncopyable, we have to add
> here
> DISALLOW_COPY_AND_ASSIGN(Buffer)
> 
> to explicitly delete the "normal" copy and assign ctors, right?
> 
>
https://cs.chromium.org/chromium/src/base/macros.h?q=DISALLOW_COPY&sq=package...

Well, no. It seems that by providing a move constructor and move assignment
operator, we are implicitly deleting the copy and assign ctors. The compiler
says "error: call to implicitly-deleted copy constructor" when trying to use the
copy constructor.
Even if it didn't implicitly delete, the std::unique_ptr<> members would raise a
compile-time error when attempting a copy operation (only with a maybe less
direct error message).

Since there is not much cost to the extra line of code, I guess making it
explicit is probably better.

Done.

https://codereview.chromium.org/2573223002/diff/200001/media/capture/video/vi...
File media/capture/video/video_capture_device_client.cc (right):

https://codereview.chromium.org/2573223002/diff/200001/media/capture/video/vi...
media/capture/video/video_capture_device_client.cc:48: // The code invoking this
contstructor is supposed to have already made a
On 2017/01/05 20:37:02, mcasas wrote:
> s/contstructor/constructor/
> 
> would there be a way to enforce this comment with
> static_assert()s or DCHECK()s?

Hmm, not without changing the public API of VideoCaptureBufferPool.

That makes me realize that the actual flaw here is a misleading class name
BufferPoolProducerReservation. I will change it to
BufferPoolProducerReservationReleaser, which reveals the actual design intent
and should eliminate the need for the comment and any check.

https://codereview.chromium.org/2573223002/diff/200001/media/capture/video/vi...
File media/capture/video/video_capture_device_client.h (right):

https://codereview.chromium.org/2573223002/diff/200001/media/capture/video/vi...
media/capture/video/video_capture_device_client.h:87: int frame_feedback_id);
On 2017/01/05 20:37:02, mcasas wrote:
> Usually class functions are declared at the top of the
> section (public/private).

Done.

[mcasas, 2017-01-06 18:54:52]

On 2017/01/05 22:44:48, chfremer wrote:
> mcasas@: PTAL
> 
>
https://codereview.chromium.org/2573223002/diff/200001/content/browser/render...
> File content/browser/renderer_host/media/video_capture_controller.h (right):
> 
>
https://codereview.chromium.org/2573223002/diff/200001/content/browser/render...
> content/browser/renderer_host/media/video_capture_controller.h:73: // before
> they can provide the observer. (This is the case with
> On 2017/01/05 20:37:01, mcasas wrote:
> > If you're using Sublime, Sublime-wrap-plus [1] can help reflowing
> > this paragraph (e.g. I see "separate" can go in l.72, right?) 
> > 
> > 
> > [1] https://github.com/ehuss/Sublime-Wrap-Plus
> 
> Done. Thanks for the link to the plugin.
> Super useful.
> 
>
https://codereview.chromium.org/2573223002/diff/200001/media/capture/video/vi...
> File media/capture/video/video_capture_device.cc (right):
> 
>
https://codereview.chromium.org/2573223002/diff/200001/media/capture/video/vi...
> media/capture/video/video_capture_device.cc:37: }
> On 2017/01/05 20:37:01, mcasas wrote:
> > Can be put in l.37, and I think my clang-format does that.
> 
> Done.
> 
>
https://codereview.chromium.org/2573223002/diff/200001/media/capture/video/vi...
> File media/capture/video/video_capture_device.h (right):
> 
>
https://codereview.chromium.org/2573223002/diff/200001/media/capture/video/vi...
> media/capture/video/video_capture_device.h:92: struct CAPTURE_EXPORT Buffer {
> On 2017/01/05 20:37:02, mcasas wrote:
> > A struct with private members and accessors and subclasses
> > is more a class, really -- also because |handle_provider| and
> > |buffer_read_write_permission| are unique_ptr<>s, so what 
> > is exactly happening if they're accessed (move, or access by
> > pointer) is not clear from the data type.  I'd vote for class and
> > accessors.
> 
> Done.
> 
>
https://codereview.chromium.org/2573223002/diff/200001/media/capture/video/vi...
> media/capture/video/video_capture_device.h:115:
> std::unique_ptr<ScopedAccessPermission> buffer_read_write_permission);
> On 2017/01/05 20:37:02, mcasas wrote:
> > s/buffer_read_write_permission/access_permission/ ?
> > Also the member.
> 
> Done.
> 
>
https://codereview.chromium.org/2573223002/diff/200001/media/capture/video/vi...
> media/capture/video/video_capture_device.h:130: };
> On 2017/01/05 20:37:02, mcasas wrote:
> > I think that to make Buffer uncopyable, we have to add
> > here
> > DISALLOW_COPY_AND_ASSIGN(Buffer)
> > 
> > to explicitly delete the "normal" copy and assign ctors, right?
> > 
> >
>
https://cs.chromium.org/chromium/src/base/macros.h?q=DISALLOW_COPY&sq=package...
> 
> Well, no. It seems that by providing a move constructor and move assignment
> operator, we are implicitly deleting the copy and assign ctors. The compiler
> says "error: call to implicitly-deleted copy constructor" when trying to use
the
> copy constructor.
> Even if it didn't implicitly delete, the std::unique_ptr<> members would raise
a
> compile-time error when attempting a copy operation (only with a maybe less
> direct error message).
> 
> Since there is not much cost to the extra line of code, I guess making it
> explicit is probably better.
> 
> Done.
> 
>
https://codereview.chromium.org/2573223002/diff/200001/media/capture/video/vi...
> File media/capture/video/video_capture_device_client.cc (right):
> 
>
https://codereview.chromium.org/2573223002/diff/200001/media/capture/video/vi...
> media/capture/video/video_capture_device_client.cc:48: // The code invoking
this
> contstructor is supposed to have already made a
> On 2017/01/05 20:37:02, mcasas wrote:
> > s/contstructor/constructor/
> > 
> > would there be a way to enforce this comment with
> > static_assert()s or DCHECK()s?
> 
> Hmm, not without changing the public API of VideoCaptureBufferPool.
> 
> That makes me realize that the actual flaw here is a misleading class name
> BufferPoolProducerReservation. I will change it to
> BufferPoolProducerReservationReleaser, which reveals the actual design intent
> and should eliminate the need for the comment and any check.
> 
>
https://codereview.chromium.org/2573223002/diff/200001/media/capture/video/vi...
> File media/capture/video/video_capture_device_client.h (right):
> 
>
https://codereview.chromium.org/2573223002/diff/200001/media/capture/video/vi...
> media/capture/video/video_capture_device_client.h:87: int frame_feedback_id);
> On 2017/01/05 20:37:02, mcasas wrote:
> > Usually class functions are declared at the top of the
> > section (public/private).
> 
> Done.

lgtm (!)

[chfremer, 2017-01-06 18:59:59]

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

[commit-bot: I haz the power, 2017-01-06 19:00:35]

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

[commit-bot: I haz the power, 2017-01-06 21:19:35]

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

[commit-bot: I haz the power, 2017-01-06 21:19:36]

Dry run: This issue passed the CQ dry run.

[chfremer, 2017-01-06 21:45:26]

The CQ bit was checked by chfremer@chromium.org

[chfremer, 2017-01-06 21:45:27]

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

[commit-bot: I haz the power, 2017-01-06 21:45:54]

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

[commit-bot: I haz the power, 2017-01-06 21:52:02]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1483739126453230,
"parent_rev": "a82983afecb4762005ac0d57809c55382aa6d1dd", "commit_rev":
"e1aac992b0399db7776661f4ac2c43094c265de7"}

[commit-bot: I haz the power, 2017-01-06 21:52:58]

Description was changed from

==========
[Mojo Video Capture] Simplify media::VideoCaptureDevice::Client:Buffer to a
struct

In interface media::VideoCaptureDevice::Client, change interface Buffer to a 
move-only struct. The new struct separates the concerns of providing access 
handles (for in-process as well as for inter-process transit) from access 
permissions. This allows clients to explicitly obtain in-process memory-mapped 
handles when they need it. Clients who only want to pass along the buffer or 
move it across process boundaries do not need memory-mapped handles.

In interface media::VideoCaptureBufferPool, renamed 
 GetBufferHandle() -> GetHandleForInProcessAccess()
 GetHandleForTransit() -> GetHandleForInterProcessTransit().

Removed outdated implementation in video_capture Mojo service. We leave the 
implementation empty, until refactoring the interfaces has been completed. 

This CL is part of the Mojo Video Capture work. For the bigger picture, 
see [1] CL1.9.10

For a peek at the follow-up CLs, please see 
https://codereview.chromium.org/2607203002/
https://codereview.chromium.org/2602983002/

BUG=584797
TEST=
  content_unittests --gtest_filter="*Video*", 
  video_capture_unittests, 
  Apprtc loopback on Debug, 
  Desktop Capture Example extension on Release

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
==========

to

==========
[Mojo Video Capture] Simplify media::VideoCaptureDevice::Client:Buffer to a
struct

In interface media::VideoCaptureDevice::Client, change interface Buffer to a 
move-only struct. The new struct separates the concerns of providing access 
handles (for in-process as well as for inter-process transit) from access 
permissions. This allows clients to explicitly obtain in-process memory-mapped 
handles when they need it. Clients who only want to pass along the buffer or 
move it across process boundaries do not need memory-mapped handles.

In interface media::VideoCaptureBufferPool, renamed 
 GetBufferHandle() -> GetHandleForInProcessAccess()
 GetHandleForTransit() -> GetHandleForInterProcessTransit().

Removed outdated implementation in video_capture Mojo service. We leave the 
implementation empty, until refactoring the interfaces has been completed. 

This CL is part of the Mojo Video Capture work. For the bigger picture, 
see [1] CL1.9.10

For a peek at the follow-up CLs, please see 
https://codereview.chromium.org/2607203002/
https://codereview.chromium.org/2602983002/

BUG=584797
TEST=
  content_unittests --gtest_filter="*Video*", 
  video_capture_unittests, 
  Apprtc loopback on Debug, 
  Desktop Capture Example extension on Release

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...

Review-Url: https://codereview.chromium.org/2573223002
Cr-Commit-Position: refs/heads/master@{#442066}
Committed:
https://chromium.googlesource.com/chromium/src/+/e1aac992b0399db7776661f4ac2c...
==========

[commit-bot: I haz the power, 2017-01-06 21:52:59]

Committed patchset #8 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/e1aac992b0399db7776661f4ac2c...

[chfremer, 2017-01-07 01:04:56]

A revert of this CL (patchset #8 id:220001) has been created in
https://codereview.chromium.org/2613793007/ by chfremer@chromium.org.

The reason for reverting is: In order to revert
https://codereview.chromium.org/2592303002/, need to revert this first, since it
depends on it..