[Mojo Video Capture] Replace RESOURCE_UTILIZATION with interface ReceiverLoadObserver

[Mojo Video Capture] Replace RESOURCE_UTILIZATION with interface ReceiverLoadObserver

Replace RESOURCE_UTILIZATION with interface ReceiverLoadObserver

The VideoFrame metadata field RESOURCE_UTILIZATION is currently being used as a
hidden communication channel for consumers to report back their "utilization"
(i.e. how busy they are) to interested producers. It appears that this mechanism
is used when using Cast to transmit captured content (e.g.
desktop/window/tab sharing). In the Browser process, VideoCaptureController uses
this mechanism to report the highest utilization of its clients back to
ThreadSafeCaptureOracle, which uses a Destruction Observer to read out the
metadata field when the VideoFrame instance it had sent out is destroyed.

Besides this mechanism being non-obvious and therefore fragile, it does not
work across process boundaries (which we need when switching to a Mojo service).
This CL replaces this with a communication channel through a callback interface
ReceiverLoadObserver which can be optionally given to VideoCaptureController.
Doing so also allows us to move away from using media::VideoFrame for the
transport of video frames, at least in the Browser process. This is desirable,
because the use of media::VideoFrame has several downsides:
1.) It implies access to a memory-mapped buffer, which we may not need. Some
components need to transport video frames but do not need to access their data.
2.) Its interface fails to clearly communicate what information is actually
contained. It may be just a frame format and metadata. It may be read/write
access to a memory-mapped buffer. And it may or may not contain a shareable
handle to a buffer.

List of changes:
* Have VideoCaptureController communicate client utilization to
  VideoCaptureDevice via a new interface ConsumerFeedbackObserver.
* In VideoCaptureManager, updated factory logic to correctly establish the
  connection from the controller to the device.
* In interface media::VideoCaptureDevice::Client, added a |frame_feedback_id|
  parameter to producer methods. This is needed to be able to report which
  particular frame a reported consumer utilization corresponds to. The
  ThreadSafeCaptureOracle is interested in this information.
* Store the |frame_feedback_id| inside instances of
  media::VideoCaptureDevice::Client::Buffer, which are passed along to the
  controller.
* In VideoCaptureController we now have to keep track of the consumer count for
  each buffer in order to be able to send out the maximum consumer utilization
  when all consumers have finished consuming a buffer. We do this by introducing
  a std::map<int, BufferState> |buffer_id_to_state_map_|.
  VideoCaptureController is the right place for keeping track of the consumer
  count, because it is the place where access to a single device is shared to
  multiple clients.
* Also in video_capture_controller.h/cc:
  * Removed some sync token related code that was unused (and would otherwise
    have to be modified for this CL)
  * Instead of holding VideoFrame references in each ControllerClient in a
    std::map<int, scoped_refptr<VideoFrame>> |active_buffers|, we keep a single
    VideoFrame reference in the BufferState in |buffer_id_to_state_map|. In
    the ControllerClients we then only need to track a std::set<int>
    |buffers_in_use|.

Note that the same mechanism of using RESOURCE_UTILIZATION as a communication
channel is still in use on the Renderer side, where it is set by
media/cast/sender/video_sender.cc and read by video_capture_impl.cc for
reporting it back to the browser process. I think it would be better to use a
callback interface here as well and treat video frames as read-only. But since
this CL does not change the Renderer side, we can leave it as is for now.

The following media::VideoCaptureDevice implementations have been updated to
listen on the new communication channel:
 * desktop_capture_device_aura.h
 * web_contents_video_capture_device.h
 * screen_capture_device_android.h
TODO: There may be implementations in WebRTC that need to be updated as well.

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

BUG=584797
TEST=
  video_capture_unittests,
  content_unittests,
  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

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

[chfremer, 2016-11-23 19:00:32]

Description was changed from

==========
presubmit


Merge branch '1.9.6' into 1.9.7


* In controller, do not report utilization unless at least 1 consumer reported.
* In manager, only set FrameReceiverObserver when device is created
successfully,   and reset it to null, when the device is released. * In
device_client, protect multi-threaded access to map by posting to a single
thread.


* Fix VideoCaptureControllerTests * Added test
VideoCaptureDeviceClientTest.ReportConsumerLoad


Merge branch '1.9.6' into 1.9.7


Merge branch '1.9.6' into 1.9.7


Reverted change of comments that should go to subsequent CL instead.


Merge branch '1.9.6' into 1.9.7


Merge branch '1.9.6' into 1.9.7


Backport erasing entry from buffer_id_to_frame_id_map_ from 1.9.11


Merge branch '1.9.6' into 1.9.7


Fix controller not releasing producer hold


Apply patch to 1.9.7

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

to

==========
[Mojo Video Capture] Replace RESOURCE_UTILIZATION with interface
ReceiverLoadObserver

Replace RESOURCE_UTILIZATION with interface ReceiverLoadObserver

The VideoFrame metadata field RESOURCE_UTILIZATION is currently being used as a 
hidden communication channel for consumers to report back their "utilization" 
(i.e. how busy they are) to interested producers. It appears that this mechanism
is used when using Cast to transmit captured content (e.g. 
desktop/window/tab sharing). In the Browser process, VideoCaptureController uses

this mechanism to report the highest utilization of its clients back to
ThreadSafeCaptureOracle, which uses a Destruction Observer to read out the 
metadata field when the VideoFrame instance it had sent out is destroyed.

Besides being highly non-obvious and fragile, this communication through 
shared state does not work across process boundaries (which we need when 
switching to a Mojo service). This CL replaces this with a proper communication 
channel through a callback interface ReceiverLoadObserver which can be 
optionally given to VideoCaptureController. Doing so also allows us to move away
from using media::VideoFrame for the transport of video frames, at least in the 
Browser process. This is desirable, because the use of media::VideoFrame has 
several downsides:
1.) It implies access to a memory-mapped buffer, which we may not need. Some 
components need to transport video frames but do not need to access their data.
2.) Its interface fails to clearly communicate what information is actually 
contained. It may be just a frame format and metadata. It may be read/write 
access to a memory-mapped buffer. And it may or may not contain a shareable 
handle to a buffer.

List of changes:
* Have VideoCaptureController communicates client utilization to 
  VideoCaptureDeviceClient via a new interface FrameReceiverObserver. 
* Have VideoCaptureDeviceClient reports utilization to its VideoCaptureDevice 
  via a new interface ConsumerLoadObserver. 
* In VideoCaptureManager, updated factory logic to correctly establish all the 
  required connections.
* In interface media::VideoCaptureDevice::Client, add a |frame_id| parameter to 
  methods that receive frames. This is needed to be able to report which 
  particular frame a reported consumer utilization corresponds to. The 
  ThreadSafeCaptureOracle is interested in this information.
* In VideoCaptureDeviceClient, we now have to maintain a map between |buffer_id|

  and |frame_id|. Clients of VideoCaptureDeviceClient (i.e. 
  VideoCaptureController via the VideoFrameReceiver interface), only know about 
  buffer_ids. VideoCaptureDeviceClient needs to translate those back to 
  frame_ids when a utilization is reported. In order to avoid multi-threaded 
  access to the map, we ask factory code to pass a 
  |utilization_reporting_task_runner| into the constructor of 
  VideoCaptureDeviceClient and make sure that all mutation and access of the map
  happens on that task runner.
* In VideoCaptureController we now have to keep track of the consumer count for 
  each buffer in order to be able to send out the maximum consumer utilization 
  when all consumers have finished consuming a buffer. The 
  VideoCaptureController is the right place for keeping track of the consumer 
  count, because it is the place where access to a single device is shared to 
  multiple clients.
* Also in video_capture_controller.h/cc: 
  * Removed some sync token related code that was unused (and would otherwise 
    have to be modified for this CL)
  * Instead of holding VideoFrame references in each ControllerClient in a 
    std::map<int, scoped_refptr<VideoFrame>> |active_buffers|, we keep a single 
    VideoFrame reference in the BufferState in |buffer_id_to_state_map|. In 
    the ControllerClients we then only need to track a std::set<int> 
    |buffers_in_use|.
* Added test VideoCaptureDeviceClientTest.ReportConsumerLoad.

Note that the same mechanism of using RESOURCE_UTILIZATION as a communication 
channel is still in use on the Renderer side, where it is set by 
media/cast/sender/video_sender.cc and read by video_capture_impl.cc for 
reporting it back to the browser process. I think it would be better to use a 
callback interface here as well and treat video frames as read-only. But since 
this CL does not change the Renderer side, we can leave it as is for now.

The following media::VideoCaptureDevice implementations have been updated to 
listen on the new communication channel:
 * desktop_capture_device_aura.h
 * web_contents_video_capture_device.h
 * screen_capture_device_android.h
TODO: There may be implementations in WebRTC that need to be updated as well.

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

BUG=584797
TEST=
  video_capture_unittests, 
  content_unittests, 
  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-11-23 19:35:42]

Patchset #1 (id:1) has been deleted

[chfremer, 2016-11-23 19:36:12]

Description was changed from

==========
[Mojo Video Capture] Replace RESOURCE_UTILIZATION with interface
ReceiverLoadObserver

Replace RESOURCE_UTILIZATION with interface ReceiverLoadObserver

The VideoFrame metadata field RESOURCE_UTILIZATION is currently being used as a 
hidden communication channel for consumers to report back their "utilization" 
(i.e. how busy they are) to interested producers. It appears that this mechanism
is used when using Cast to transmit captured content (e.g. 
desktop/window/tab sharing). In the Browser process, VideoCaptureController uses

this mechanism to report the highest utilization of its clients back to
ThreadSafeCaptureOracle, which uses a Destruction Observer to read out the 
metadata field when the VideoFrame instance it had sent out is destroyed.

Besides being highly non-obvious and fragile, this communication through 
shared state does not work across process boundaries (which we need when 
switching to a Mojo service). This CL replaces this with a proper communication 
channel through a callback interface ReceiverLoadObserver which can be 
optionally given to VideoCaptureController. Doing so also allows us to move away
from using media::VideoFrame for the transport of video frames, at least in the 
Browser process. This is desirable, because the use of media::VideoFrame has 
several downsides:
1.) It implies access to a memory-mapped buffer, which we may not need. Some 
components need to transport video frames but do not need to access their data.
2.) Its interface fails to clearly communicate what information is actually 
contained. It may be just a frame format and metadata. It may be read/write 
access to a memory-mapped buffer. And it may or may not contain a shareable 
handle to a buffer.

List of changes:
* Have VideoCaptureController communicates client utilization to 
  VideoCaptureDeviceClient via a new interface FrameReceiverObserver. 
* Have VideoCaptureDeviceClient reports utilization to its VideoCaptureDevice 
  via a new interface ConsumerLoadObserver. 
* In VideoCaptureManager, updated factory logic to correctly establish all the 
  required connections.
* In interface media::VideoCaptureDevice::Client, add a |frame_id| parameter to 
  methods that receive frames. This is needed to be able to report which 
  particular frame a reported consumer utilization corresponds to. The 
  ThreadSafeCaptureOracle is interested in this information.
* In VideoCaptureDeviceClient, we now have to maintain a map between |buffer_id|

  and |frame_id|. Clients of VideoCaptureDeviceClient (i.e. 
  VideoCaptureController via the VideoFrameReceiver interface), only know about 
  buffer_ids. VideoCaptureDeviceClient needs to translate those back to 
  frame_ids when a utilization is reported. In order to avoid multi-threaded 
  access to the map, we ask factory code to pass a 
  |utilization_reporting_task_runner| into the constructor of 
  VideoCaptureDeviceClient and make sure that all mutation and access of the map
  happens on that task runner.
* In VideoCaptureController we now have to keep track of the consumer count for 
  each buffer in order to be able to send out the maximum consumer utilization 
  when all consumers have finished consuming a buffer. The 
  VideoCaptureController is the right place for keeping track of the consumer 
  count, because it is the place where access to a single device is shared to 
  multiple clients.
* Also in video_capture_controller.h/cc: 
  * Removed some sync token related code that was unused (and would otherwise 
    have to be modified for this CL)
  * Instead of holding VideoFrame references in each ControllerClient in a 
    std::map<int, scoped_refptr<VideoFrame>> |active_buffers|, we keep a single 
    VideoFrame reference in the BufferState in |buffer_id_to_state_map|. In 
    the ControllerClients we then only need to track a std::set<int> 
    |buffers_in_use|.
* Added test VideoCaptureDeviceClientTest.ReportConsumerLoad.

Note that the same mechanism of using RESOURCE_UTILIZATION as a communication 
channel is still in use on the Renderer side, where it is set by 
media/cast/sender/video_sender.cc and read by video_capture_impl.cc for 
reporting it back to the browser process. I think it would be better to use a 
callback interface here as well and treat video frames as read-only. But since 
this CL does not change the Renderer side, we can leave it as is for now.

The following media::VideoCaptureDevice implementations have been updated to 
listen on the new communication channel:
 * desktop_capture_device_aura.h
 * web_contents_video_capture_device.h
 * screen_capture_device_android.h
TODO: There may be implementations in WebRTC that need to be updated as well.

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

BUG=584797
TEST=
  video_capture_unittests, 
  content_unittests, 
  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] Replace RESOURCE_UTILIZATION with interface
ReceiverLoadObserver

Replace RESOURCE_UTILIZATION with interface ReceiverLoadObserver

The VideoFrame metadata field RESOURCE_UTILIZATION is currently being used as a 
hidden communication channel for consumers to report back their "utilization" 
(i.e. how busy they are) to interested producers. It appears that this mechanism
is used when using Cast to transmit captured content (e.g. 
desktop/window/tab sharing). In the Browser process, VideoCaptureController uses

this mechanism to report the highest utilization of its clients back to
ThreadSafeCaptureOracle, which uses a Destruction Observer to read out the 
metadata field when the VideoFrame instance it had sent out is destroyed.

Besides being highly non-obvious and fragile, this communication through 
shared state does not work across process boundaries (which we need when 
switching to a Mojo service). This CL replaces this with a proper communication 
channel through a callback interface ReceiverLoadObserver which can be 
optionally given to VideoCaptureController. Doing so also allows us to move away
from using media::VideoFrame for the transport of video frames, at least in the 
Browser process. This is desirable, because the use of media::VideoFrame has 
several downsides:
1.) It implies access to a memory-mapped buffer, which we may not need. Some 
components need to transport video frames but do not need to access their data.
2.) Its interface fails to clearly communicate what information is actually 
contained. It may be just a frame format and metadata. It may be read/write 
access to a memory-mapped buffer. And it may or may not contain a shareable 
handle to a buffer.

List of changes:
* Have VideoCaptureController communicates client utilization to 
  VideoCaptureDeviceClient via a new interface FrameReceiverObserver. 
* Have VideoCaptureDeviceClient reports utilization to its VideoCaptureDevice 
  via a new interface ConsumerLoadObserver. 
* In VideoCaptureManager, updated factory logic to correctly establish all the 
  required connections.
* In interface media::VideoCaptureDevice::Client, add a |frame_id| parameter to 
  methods that receive frames. This is needed to be able to report which 
  particular frame a reported consumer utilization corresponds to. The 
  ThreadSafeCaptureOracle is interested in this information.
* In VideoCaptureDeviceClient, we now have to maintain a map between |buffer_id|

  and |frame_id|. Clients of VideoCaptureDeviceClient (i.e. 
  VideoCaptureController via the VideoFrameReceiver interface), only know about 
  buffer_ids. VideoCaptureDeviceClient needs to translate those back to 
  frame_ids when a utilization is reported. In order to avoid multi-threaded 
  access to the map, we ask factory code to pass a 
  |utilization_reporting_task_runner| into the constructor of 
  VideoCaptureDeviceClient and make sure that all mutation and access of the map
  happens on that task runner.
* In VideoCaptureController we now have to keep track of the consumer count for 
  each buffer in order to be able to send out the maximum consumer utilization 
  when all consumers have finished consuming a buffer. We do this by introducing
  a std::map<int, BufferState> |buffer_id_to_state_map_|.
  VideoCaptureController is the right place for keeping track of the consumer 
  count, because it is the place where access to a single device is shared to 
  multiple clients.
* Also in video_capture_controller.h/cc: 
  * Removed some sync token related code that was unused (and would otherwise 
    have to be modified for this CL)
  * Instead of holding VideoFrame references in each ControllerClient in a 
    std::map<int, scoped_refptr<VideoFrame>> |active_buffers|, we keep a single 
    VideoFrame reference in the BufferState in |buffer_id_to_state_map|. In 
    the ControllerClients we then only need to track a std::set<int> 
    |buffers_in_use|.
* Added test VideoCaptureDeviceClientTest.ReportConsumerLoad.

Note that the same mechanism of using RESOURCE_UTILIZATION as a communication 
channel is still in use on the Renderer side, where it is set by 
media/cast/sender/video_sender.cc and read by video_capture_impl.cc for 
reporting it back to the browser process. I think it would be better to use a 
callback interface here as well and treat video frames as read-only. But since 
this CL does not change the Renderer side, we can leave it as is for now.

The following media::VideoCaptureDevice implementations have been updated to 
listen on the new communication channel:
 * desktop_capture_device_aura.h
 * web_contents_video_capture_device.h
 * screen_capture_device_android.h
TODO: There may be implementations in WebRTC that need to be updated as well.

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

BUG=584797
TEST=
  video_capture_unittests, 
  content_unittests, 
  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-11-28 20:30:21]

Patchset #1 (id:20001) has been deleted

[chfremer, 2016-11-28 20:30:42]

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

[commit-bot: I haz the power, 2016-11-28 20:32:41]

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

[chfremer, 2016-11-28 20:43:28]

Description was changed from

==========
[Mojo Video Capture] Replace RESOURCE_UTILIZATION with interface
ReceiverLoadObserver

Replace RESOURCE_UTILIZATION with interface ReceiverLoadObserver

The VideoFrame metadata field RESOURCE_UTILIZATION is currently being used as a 
hidden communication channel for consumers to report back their "utilization" 
(i.e. how busy they are) to interested producers. It appears that this mechanism
is used when using Cast to transmit captured content (e.g. 
desktop/window/tab sharing). In the Browser process, VideoCaptureController uses

this mechanism to report the highest utilization of its clients back to
ThreadSafeCaptureOracle, which uses a Destruction Observer to read out the 
metadata field when the VideoFrame instance it had sent out is destroyed.

Besides being highly non-obvious and fragile, this communication through 
shared state does not work across process boundaries (which we need when 
switching to a Mojo service). This CL replaces this with a proper communication 
channel through a callback interface ReceiverLoadObserver which can be 
optionally given to VideoCaptureController. Doing so also allows us to move away
from using media::VideoFrame for the transport of video frames, at least in the 
Browser process. This is desirable, because the use of media::VideoFrame has 
several downsides:
1.) It implies access to a memory-mapped buffer, which we may not need. Some 
components need to transport video frames but do not need to access their data.
2.) Its interface fails to clearly communicate what information is actually 
contained. It may be just a frame format and metadata. It may be read/write 
access to a memory-mapped buffer. And it may or may not contain a shareable 
handle to a buffer.

List of changes:
* Have VideoCaptureController communicates client utilization to 
  VideoCaptureDeviceClient via a new interface FrameReceiverObserver. 
* Have VideoCaptureDeviceClient reports utilization to its VideoCaptureDevice 
  via a new interface ConsumerLoadObserver. 
* In VideoCaptureManager, updated factory logic to correctly establish all the 
  required connections.
* In interface media::VideoCaptureDevice::Client, add a |frame_id| parameter to 
  methods that receive frames. This is needed to be able to report which 
  particular frame a reported consumer utilization corresponds to. The 
  ThreadSafeCaptureOracle is interested in this information.
* In VideoCaptureDeviceClient, we now have to maintain a map between |buffer_id|

  and |frame_id|. Clients of VideoCaptureDeviceClient (i.e. 
  VideoCaptureController via the VideoFrameReceiver interface), only know about 
  buffer_ids. VideoCaptureDeviceClient needs to translate those back to 
  frame_ids when a utilization is reported. In order to avoid multi-threaded 
  access to the map, we ask factory code to pass a 
  |utilization_reporting_task_runner| into the constructor of 
  VideoCaptureDeviceClient and make sure that all mutation and access of the map
  happens on that task runner.
* In VideoCaptureController we now have to keep track of the consumer count for 
  each buffer in order to be able to send out the maximum consumer utilization 
  when all consumers have finished consuming a buffer. We do this by introducing
  a std::map<int, BufferState> |buffer_id_to_state_map_|.
  VideoCaptureController is the right place for keeping track of the consumer 
  count, because it is the place where access to a single device is shared to 
  multiple clients.
* Also in video_capture_controller.h/cc: 
  * Removed some sync token related code that was unused (and would otherwise 
    have to be modified for this CL)
  * Instead of holding VideoFrame references in each ControllerClient in a 
    std::map<int, scoped_refptr<VideoFrame>> |active_buffers|, we keep a single 
    VideoFrame reference in the BufferState in |buffer_id_to_state_map|. In 
    the ControllerClients we then only need to track a std::set<int> 
    |buffers_in_use|.
* Added test VideoCaptureDeviceClientTest.ReportConsumerLoad.

Note that the same mechanism of using RESOURCE_UTILIZATION as a communication 
channel is still in use on the Renderer side, where it is set by 
media/cast/sender/video_sender.cc and read by video_capture_impl.cc for 
reporting it back to the browser process. I think it would be better to use a 
callback interface here as well and treat video frames as read-only. But since 
this CL does not change the Renderer side, we can leave it as is for now.

The following media::VideoCaptureDevice implementations have been updated to 
listen on the new communication channel:
 * desktop_capture_device_aura.h
 * web_contents_video_capture_device.h
 * screen_capture_device_android.h
TODO: There may be implementations in WebRTC that need to be updated as well.

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

BUG=584797
TEST=
  video_capture_unittests, 
  content_unittests, 
  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] Replace RESOURCE_UTILIZATION with interface
ReceiverLoadObserver

Replace RESOURCE_UTILIZATION with interface ReceiverLoadObserver

The VideoFrame metadata field RESOURCE_UTILIZATION is currently being used as a 
hidden communication channel for consumers to report back their "utilization" 
(i.e. how busy they are) to interested producers. It appears that this mechanism
is used when using Cast to transmit captured content (e.g. 
desktop/window/tab sharing). In the Browser process, VideoCaptureController uses

this mechanism to report the highest utilization of its clients back to
ThreadSafeCaptureOracle, which uses a Destruction Observer to read out the 
metadata field when the VideoFrame instance it had sent out is destroyed.

Besides this mechanism being non-obvious, not checked by the compiler, and 
therefore fragile, it does not work across process boundaries (which we need 
when switching to a Mojo service). This CL replaces this with a communication 
channel through a callback interface ReceiverLoadObserver which can be 
optionally given to VideoCaptureController. Doing so also allows us to move away
from using media::VideoFrame for the transport of video frames, at least in the 
Browser process. This is desirable, because the use of media::VideoFrame has 
several downsides:
1.) It implies access to a memory-mapped buffer, which we may not need. Some 
components need to transport video frames but do not need to access their data.
2.) Its interface fails to clearly communicate what information is actually 
contained. It may be just a frame format and metadata. It may be read/write 
access to a memory-mapped buffer. And it may or may not contain a shareable 
handle to a buffer.

List of changes:
* Have VideoCaptureController communicates client utilization to 
  VideoCaptureDeviceClient via a new interface FrameReceiverObserver. 
* Have VideoCaptureDeviceClient reports utilization to its VideoCaptureDevice 
  via a new interface ConsumerLoadObserver. 
* In VideoCaptureManager, updated factory logic to correctly establish all the 
  required connections.
* In interface media::VideoCaptureDevice::Client, add a |frame_id| parameter to 
  methods that receive frames. This is needed to be able to report which 
  particular frame a reported consumer utilization corresponds to. The 
  ThreadSafeCaptureOracle is interested in this information.
* In VideoCaptureDeviceClient, we now have to maintain a map between |buffer_id|

  and |frame_id|. Clients of VideoCaptureDeviceClient (i.e. 
  VideoCaptureController via the VideoFrameReceiver interface), only know about 
  buffer_ids. VideoCaptureDeviceClient needs to translate those back to 
  frame_ids when a utilization is reported. In order to avoid multi-threaded 
  access to the map, we ask factory code to pass a 
  |utilization_reporting_task_runner| into the constructor of 
  VideoCaptureDeviceClient and make sure that all mutation and access of the map
  happens on that task runner.
* In VideoCaptureController we now have to keep track of the consumer count for 
  each buffer in order to be able to send out the maximum consumer utilization 
  when all consumers have finished consuming a buffer. We do this by introducing
  a std::map<int, BufferState> |buffer_id_to_state_map_|.
  VideoCaptureController is the right place for keeping track of the consumer 
  count, because it is the place where access to a single device is shared to 
  multiple clients.
* Also in video_capture_controller.h/cc: 
  * Removed some sync token related code that was unused (and would otherwise 
    have to be modified for this CL)
  * Instead of holding VideoFrame references in each ControllerClient in a 
    std::map<int, scoped_refptr<VideoFrame>> |active_buffers|, we keep a single 
    VideoFrame reference in the BufferState in |buffer_id_to_state_map|. In 
    the ControllerClients we then only need to track a std::set<int> 
    |buffers_in_use|.
* Added test VideoCaptureDeviceClientTest.ReportConsumerLoad.

Note that the same mechanism of using RESOURCE_UTILIZATION as a communication 
channel is still in use on the Renderer side, where it is set by 
media/cast/sender/video_sender.cc and read by video_capture_impl.cc for 
reporting it back to the browser process. I think it would be better to use a 
callback interface here as well and treat video frames as read-only. But since 
this CL does not change the Renderer side, we can leave it as is for now.

The following media::VideoCaptureDevice implementations have been updated to 
listen on the new communication channel:
 * desktop_capture_device_aura.h
 * web_contents_video_capture_device.h
 * screen_capture_device_android.h
TODO: There may be implementations in WebRTC that need to be updated as well.

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

BUG=584797
TEST=
  video_capture_unittests, 
  content_unittests, 
  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-11-28 20:44:29]

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

[chfremer, 2016-11-28 20:44:29]

mcasas@: PTAL
miu@: PTAL
emircan@: FYI

[commit-bot: I haz the power, 2016-11-28 20:58:11]

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

[commit-bot: I haz the power, 2016-11-28 20:58:12]

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

[chfremer, 2016-11-28 21:50:43]

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

[commit-bot: I haz the power, 2016-11-28 21:51:48]

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-11-28 23:47:29]

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

[commit-bot: I haz the power, 2016-11-28 23:47:30]

Dry run: This issue passed the CQ dry run.

[chfremer, 2016-11-29 17:45:45]

Description was changed from

==========
[Mojo Video Capture] Replace RESOURCE_UTILIZATION with interface
ReceiverLoadObserver

Replace RESOURCE_UTILIZATION with interface ReceiverLoadObserver

The VideoFrame metadata field RESOURCE_UTILIZATION is currently being used as a 
hidden communication channel for consumers to report back their "utilization" 
(i.e. how busy they are) to interested producers. It appears that this mechanism
is used when using Cast to transmit captured content (e.g. 
desktop/window/tab sharing). In the Browser process, VideoCaptureController uses

this mechanism to report the highest utilization of its clients back to
ThreadSafeCaptureOracle, which uses a Destruction Observer to read out the 
metadata field when the VideoFrame instance it had sent out is destroyed.

Besides this mechanism being non-obvious, not checked by the compiler, and 
therefore fragile, it does not work across process boundaries (which we need 
when switching to a Mojo service). This CL replaces this with a communication 
channel through a callback interface ReceiverLoadObserver which can be 
optionally given to VideoCaptureController. Doing so also allows us to move away
from using media::VideoFrame for the transport of video frames, at least in the 
Browser process. This is desirable, because the use of media::VideoFrame has 
several downsides:
1.) It implies access to a memory-mapped buffer, which we may not need. Some 
components need to transport video frames but do not need to access their data.
2.) Its interface fails to clearly communicate what information is actually 
contained. It may be just a frame format and metadata. It may be read/write 
access to a memory-mapped buffer. And it may or may not contain a shareable 
handle to a buffer.

List of changes:
* Have VideoCaptureController communicates client utilization to 
  VideoCaptureDeviceClient via a new interface FrameReceiverObserver. 
* Have VideoCaptureDeviceClient reports utilization to its VideoCaptureDevice 
  via a new interface ConsumerLoadObserver. 
* In VideoCaptureManager, updated factory logic to correctly establish all the 
  required connections.
* In interface media::VideoCaptureDevice::Client, add a |frame_id| parameter to 
  methods that receive frames. This is needed to be able to report which 
  particular frame a reported consumer utilization corresponds to. The 
  ThreadSafeCaptureOracle is interested in this information.
* In VideoCaptureDeviceClient, we now have to maintain a map between |buffer_id|

  and |frame_id|. Clients of VideoCaptureDeviceClient (i.e. 
  VideoCaptureController via the VideoFrameReceiver interface), only know about 
  buffer_ids. VideoCaptureDeviceClient needs to translate those back to 
  frame_ids when a utilization is reported. In order to avoid multi-threaded 
  access to the map, we ask factory code to pass a 
  |utilization_reporting_task_runner| into the constructor of 
  VideoCaptureDeviceClient and make sure that all mutation and access of the map
  happens on that task runner.
* In VideoCaptureController we now have to keep track of the consumer count for 
  each buffer in order to be able to send out the maximum consumer utilization 
  when all consumers have finished consuming a buffer. We do this by introducing
  a std::map<int, BufferState> |buffer_id_to_state_map_|.
  VideoCaptureController is the right place for keeping track of the consumer 
  count, because it is the place where access to a single device is shared to 
  multiple clients.
* Also in video_capture_controller.h/cc: 
  * Removed some sync token related code that was unused (and would otherwise 
    have to be modified for this CL)
  * Instead of holding VideoFrame references in each ControllerClient in a 
    std::map<int, scoped_refptr<VideoFrame>> |active_buffers|, we keep a single 
    VideoFrame reference in the BufferState in |buffer_id_to_state_map|. In 
    the ControllerClients we then only need to track a std::set<int> 
    |buffers_in_use|.
* Added test VideoCaptureDeviceClientTest.ReportConsumerLoad.

Note that the same mechanism of using RESOURCE_UTILIZATION as a communication 
channel is still in use on the Renderer side, where it is set by 
media/cast/sender/video_sender.cc and read by video_capture_impl.cc for 
reporting it back to the browser process. I think it would be better to use a 
callback interface here as well and treat video frames as read-only. But since 
this CL does not change the Renderer side, we can leave it as is for now.

The following media::VideoCaptureDevice implementations have been updated to 
listen on the new communication channel:
 * desktop_capture_device_aura.h
 * web_contents_video_capture_device.h
 * screen_capture_device_android.h
TODO: There may be implementations in WebRTC that need to be updated as well.

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

BUG=584797
TEST=
  video_capture_unittests, 
  content_unittests, 
  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] Replace RESOURCE_UTILIZATION with interface
ReceiverLoadObserver

Replace RESOURCE_UTILIZATION with interface ReceiverLoadObserver

The VideoFrame metadata field RESOURCE_UTILIZATION is currently being used as a 
hidden communication channel for consumers to report back their "utilization" 
(i.e. how busy they are) to interested producers. It appears that this mechanism
is used when using Cast to transmit captured content (e.g. 
desktop/window/tab sharing). In the Browser process, VideoCaptureController uses

this mechanism to report the highest utilization of its clients back to
ThreadSafeCaptureOracle, which uses a Destruction Observer to read out the 
metadata field when the VideoFrame instance it had sent out is destroyed.

Besides this mechanism being non-obvious and therefore fragile, it does not 
work across process boundaries (which we need when switching to a Mojo service).

This CL replaces this with a communication channel through a callback interface 
ReceiverLoadObserver which can be optionally given to VideoCaptureController. 
Doing so also allows us to move away from using media::VideoFrame for the 
transport of video frames, at least in the Browser process. This is desirable, 
because the use of media::VideoFrame has several downsides:
1.) It implies access to a memory-mapped buffer, which we may not need. Some 
components need to transport video frames but do not need to access their data.
2.) Its interface fails to clearly communicate what information is actually 
contained. It may be just a frame format and metadata. It may be read/write 
access to a memory-mapped buffer. And it may or may not contain a shareable 
handle to a buffer.

List of changes:
* Have VideoCaptureController communicate client utilization to 
  VideoCaptureDeviceClient via a new interface FrameReceiverObserver. 
* Have VideoCaptureDeviceClient report utilization to its VideoCaptureDevice 
  via a new interface ConsumerLoadObserver. 
* In VideoCaptureManager, updated factory logic to correctly establish all the 
  required connections.
* In interface media::VideoCaptureDevice::Client, added a |frame_id| parameter 
  to methods that receive frames. This is needed to be able to report which 
  particular frame a reported consumer utilization corresponds to. The 
  ThreadSafeCaptureOracle is interested in this information.
* In VideoCaptureDeviceClient, we now have to maintain a map between |buffer_id|

  and |frame_id|. Clients of VideoCaptureDeviceClient (i.e. 
  VideoCaptureController via the VideoFrameReceiver interface), only know about 
  buffer_ids. VideoCaptureDeviceClient needs to translate those back to 
  frame_ids when a utilization is reported. In order to avoid multi-threaded 
  access to the map, we ask factory code to pass a 
  |utilization_reporting_task_runner| into the constructor of 
  VideoCaptureDeviceClient and make sure that all mutation and access of the map
  happens on that task runner.
* In VideoCaptureController we now have to keep track of the consumer count for 
  each buffer in order to be able to send out the maximum consumer utilization 
  when all consumers have finished consuming a buffer. We do this by introducing
  a std::map<int, BufferState> |buffer_id_to_state_map_|.
  VideoCaptureController is the right place for keeping track of the consumer 
  count, because it is the place where access to a single device is shared to 
  multiple clients.
* Also in video_capture_controller.h/cc: 
  * Removed some sync token related code that was unused (and would otherwise 
    have to be modified for this CL)
  * Instead of holding VideoFrame references in each ControllerClient in a 
    std::map<int, scoped_refptr<VideoFrame>> |active_buffers|, we keep a single 
    VideoFrame reference in the BufferState in |buffer_id_to_state_map|. In 
    the ControllerClients we then only need to track a std::set<int> 
    |buffers_in_use|.
* Added test VideoCaptureDeviceClientTest.ReportConsumerLoad.

Note that the same mechanism of using RESOURCE_UTILIZATION as a communication 
channel is still in use on the Renderer side, where it is set by 
media/cast/sender/video_sender.cc and read by video_capture_impl.cc for 
reporting it back to the browser process. I think it would be better to use a 
callback interface here as well and treat video frames as read-only. But since 
this CL does not change the Renderer side, we can leave it as is for now.

The following media::VideoCaptureDevice implementations have been updated to 
listen on the new communication channel:
 * desktop_capture_device_aura.h
 * web_contents_video_capture_device.h
 * screen_capture_device_android.h
TODO: There may be implementations in WebRTC that need to be updated as well.

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

BUG=584797
TEST=
  video_capture_unittests, 
  content_unittests, 
  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-01 05:25:18]

On-the-whole, looks good. Comments on PS2:

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

https://codereview.chromium.org/2518143004/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.cc:176:
buffer_pool_->HoldForConsumers(buffer_id_, 1);
Looks like the "hold count" tracking isn't needed in the buffer pool anymore.
Future cleanup?

https://codereview.chromium.org/2518143004/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.cc:416: if
(buffer_id_to_state_map_.find(buffer_id) ==
As mentioned in the header file, you might be able to eliminate the zero-arg
ctor and 3 map look-ups here by using iterators:

  // Insert if not exists.
  const auto it = buffer_id_to_state_map_.insert(
      std::make_pair(buffer_id, BufferState(...))).first;
  BufferState& buffer_state = it->second;

https://codereview.chromium.org/2518143004/diff/60001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_controller.h (right):

https://codereview.chromium.org/2518143004/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.h:153:
BufferState();
IIRC, you don't need the zero-arg constructor if you work with iterators instead
of the map[] operators. Based on some of the impl, it looks like that might also
reduce the number of look-ups you're doing.

https://codereview.chromium.org/2518143004/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.h:168: int
buffer_id_;
If you can eliminate the zero-arg ctor, several of these can become const.

https://codereview.chromium.org/2518143004/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.h:195:
std::map<int, BufferState> buffer_id_to_state_map_;
naming nit: Up to you, but |buffer_states_| or |buffer_states_map_| seems more
readable to me.

https://codereview.chromium.org/2518143004/diff/60001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

https://codereview.chromium.org/2518143004/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager.cc:135: // Phase 1:
When first created (in GetOrCreateDeviceEntry()), this consists of a
s/consists of a only a controller/consists of only a controller/

https://codereview.chromium.org/2518143004/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager.cc:531:
request->serial_id(), device_client_ptr));
Is this safe (passing the raw pointer)? Could, in some rare timings, the device
client be destroyed before the task runs? I ask because it looks like we're
going over to the device thread to start the capture device, transferring
ownership of the device client to that thread, and then we are hopping back to
the IO thread to call OnDeviceStarted().

https://codereview.chromium.org/2518143004/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager.cc:624:
device_client->SetConsumerLoadObserver(video_capture_device.get());
OOC, should we call SetConsumerLoadObserver(nullptr) before StopAndDeallocate()?
Or, do we know that it's impossible for the load observer to have its method
called any time at or after that point? Could there be pending tasks that will
call the observer's method in some other thread's task queue?

https://codereview.chromium.org/2518143004/diff/60001/media/capture/content/s...
File media/capture/content/screen_capture_device_core.cc (right):

https://codereview.chromium.org/2518143004/diff/60001/media/capture/content/s...
media/capture/content/screen_capture_device_core.cc:119:
oracle_proxy_->OnConsumerReportingUtilization(frame_id, utilization);
Could |oracle_proxy_| be null here? Not sure whether to:

1. DCHECK(oracle_proxy_);

or

2. if (state != kCapturing && state_ != kSuspended)
     return;

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

https://codereview.chromium.org/2518143004/diff/60001/media/capture/content/t...
media/capture/content/thread_safe_capture_oracle.cc:239: base::AutoLock
guard(lock_);
One of these days, I'm going to have to go through all this code and fix the
threading model; and get rid of all the mutexes. ;-) It has, over a long history
and many authors, evolved to this point.

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

https://codereview.chromium.org/2518143004/diff/60001/media/capture/video/vid...
media/capture/video/video_capture_device.h:44: class CAPTURE_EXPORT
FrameReceiverObserver {
Should this interface be in vcd_client.h instead?

https://codereview.chromium.org/2518143004/diff/60001/media/capture/video/vid...
media/capture/video/video_capture_device.h:51: static double
no_utilization_recorded() { return -1.0; }
nit: static constexpr double kNoUtilizationRecorded = -1.0;

https://codereview.chromium.org/2518143004/diff/60001/media/capture/video/vid...
media/capture/video/video_capture_device.h:54: // During processing of a video
frame delivered to |receiver|, we may report
1. Please fix: The code comment refers to "|receiver|", but this is a
class-level comment so there's no receiver variable.

2. Maybe this should be a method comment for OnConsumerReportingUtilization()
instead?

https://codereview.chromium.org/2518143004/diff/60001/media/capture/video/vid...
media/capture/video/video_capture_device.h:68: class CAPTURE_EXPORT
ConsumerLoadObserver {
History: At one point, we were wondering whether we might include other forms of
feedback with each frame. I only plumbed-in utilization because that was all
that was needed at the time.

So, you might consider naming this interface something like
ConsumerFeedbackObserver instead; and then we might add more methods in the
future (either per-frame, or maybe more aggregate feedback stats).

https://codereview.chromium.org/2518143004/diff/60001/media/capture/video/vid...
media/capture/video/video_capture_device.h:71: virtual void
OnConsumerReportingUtilization(int frame_id,
naming nit: s/frame_id/frame_feedback_id/ everywhere (i.e., a little more
descriptive to hint at how this is being used).

https://codereview.chromium.org/2518143004/diff/60001/media/capture/video/vid...
media/capture/video/video_capture_device.h:72: double utilization){};
style nit: add space before open brace

https://codereview.chromium.org/2518143004/diff/60001/media/capture/video/vid...
media/capture/video/video_capture_device.h:120: int frame_id = 0) = 0;
Please document the new argument.

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

https://codereview.chromium.org/2518143004/diff/60001/media/capture/video/vid...
media/capture/video/video_capture_device_client.cc:442: if
(utilization_reporting_task_runner_->BelongsToCurrentThread()) {
The threading in these two new methods feels a bit wishy-washy. Some
considerations:

1. Can the caller be responsible for calling on the right thread, so you can
remove the trampolining?

2. Is the rest of VideoCaptureDeviceClient called by one thread? If so, it seems
that these methods shouldn't change that (and also shouldn't require a separate
task runner).

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

https://codereview.chromium.org/2518143004/diff/60001/media/capture/video/vid...
media/capture/video/video_capture_device_client.h:137: std::map<int, int>
buffer_id_to_frame_id_map_;
Instead of this map, can you tag the frame feedback ids onto the Buffer? That
might help avoid all the extra code to maintain this separate map on the side.

[chfremer, 2016-12-02 01:22:32]

Description was changed from

==========
[Mojo Video Capture] Replace RESOURCE_UTILIZATION with interface
ReceiverLoadObserver

Replace RESOURCE_UTILIZATION with interface ReceiverLoadObserver

The VideoFrame metadata field RESOURCE_UTILIZATION is currently being used as a 
hidden communication channel for consumers to report back their "utilization" 
(i.e. how busy they are) to interested producers. It appears that this mechanism
is used when using Cast to transmit captured content (e.g. 
desktop/window/tab sharing). In the Browser process, VideoCaptureController uses

this mechanism to report the highest utilization of its clients back to
ThreadSafeCaptureOracle, which uses a Destruction Observer to read out the 
metadata field when the VideoFrame instance it had sent out is destroyed.

Besides this mechanism being non-obvious and therefore fragile, it does not 
work across process boundaries (which we need when switching to a Mojo service).

This CL replaces this with a communication channel through a callback interface 
ReceiverLoadObserver which can be optionally given to VideoCaptureController. 
Doing so also allows us to move away from using media::VideoFrame for the 
transport of video frames, at least in the Browser process. This is desirable, 
because the use of media::VideoFrame has several downsides:
1.) It implies access to a memory-mapped buffer, which we may not need. Some 
components need to transport video frames but do not need to access their data.
2.) Its interface fails to clearly communicate what information is actually 
contained. It may be just a frame format and metadata. It may be read/write 
access to a memory-mapped buffer. And it may or may not contain a shareable 
handle to a buffer.

List of changes:
* Have VideoCaptureController communicate client utilization to 
  VideoCaptureDeviceClient via a new interface FrameReceiverObserver. 
* Have VideoCaptureDeviceClient report utilization to its VideoCaptureDevice 
  via a new interface ConsumerLoadObserver. 
* In VideoCaptureManager, updated factory logic to correctly establish all the 
  required connections.
* In interface media::VideoCaptureDevice::Client, added a |frame_id| parameter 
  to methods that receive frames. This is needed to be able to report which 
  particular frame a reported consumer utilization corresponds to. The 
  ThreadSafeCaptureOracle is interested in this information.
* In VideoCaptureDeviceClient, we now have to maintain a map between |buffer_id|

  and |frame_id|. Clients of VideoCaptureDeviceClient (i.e. 
  VideoCaptureController via the VideoFrameReceiver interface), only know about 
  buffer_ids. VideoCaptureDeviceClient needs to translate those back to 
  frame_ids when a utilization is reported. In order to avoid multi-threaded 
  access to the map, we ask factory code to pass a 
  |utilization_reporting_task_runner| into the constructor of 
  VideoCaptureDeviceClient and make sure that all mutation and access of the map
  happens on that task runner.
* In VideoCaptureController we now have to keep track of the consumer count for 
  each buffer in order to be able to send out the maximum consumer utilization 
  when all consumers have finished consuming a buffer. We do this by introducing
  a std::map<int, BufferState> |buffer_id_to_state_map_|.
  VideoCaptureController is the right place for keeping track of the consumer 
  count, because it is the place where access to a single device is shared to 
  multiple clients.
* Also in video_capture_controller.h/cc: 
  * Removed some sync token related code that was unused (and would otherwise 
    have to be modified for this CL)
  * Instead of holding VideoFrame references in each ControllerClient in a 
    std::map<int, scoped_refptr<VideoFrame>> |active_buffers|, we keep a single 
    VideoFrame reference in the BufferState in |buffer_id_to_state_map|. In 
    the ControllerClients we then only need to track a std::set<int> 
    |buffers_in_use|.
* Added test VideoCaptureDeviceClientTest.ReportConsumerLoad.

Note that the same mechanism of using RESOURCE_UTILIZATION as a communication 
channel is still in use on the Renderer side, where it is set by 
media/cast/sender/video_sender.cc and read by video_capture_impl.cc for 
reporting it back to the browser process. I think it would be better to use a 
callback interface here as well and treat video frames as read-only. But since 
this CL does not change the Renderer side, we can leave it as is for now.

The following media::VideoCaptureDevice implementations have been updated to 
listen on the new communication channel:
 * desktop_capture_device_aura.h
 * web_contents_video_capture_device.h
 * screen_capture_device_android.h
TODO: There may be implementations in WebRTC that need to be updated as well.

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

BUG=584797
TEST=
  video_capture_unittests, 
  content_unittests, 
  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] Replace RESOURCE_UTILIZATION with interface
ReceiverLoadObserver

Replace RESOURCE_UTILIZATION with interface ReceiverLoadObserver

The VideoFrame metadata field RESOURCE_UTILIZATION is currently being used as a 
hidden communication channel for consumers to report back their "utilization" 
(i.e. how busy they are) to interested producers. It appears that this mechanism
is used when using Cast to transmit captured content (e.g. 
desktop/window/tab sharing). In the Browser process, VideoCaptureController uses

this mechanism to report the highest utilization of its clients back to
ThreadSafeCaptureOracle, which uses a Destruction Observer to read out the 
metadata field when the VideoFrame instance it had sent out is destroyed.

Besides this mechanism being non-obvious and therefore fragile, it does not 
work across process boundaries (which we need when switching to a Mojo service).

This CL replaces this with a communication channel through a callback interface 
ReceiverLoadObserver which can be optionally given to VideoCaptureController. 
Doing so also allows us to move away from using media::VideoFrame for the 
transport of video frames, at least in the Browser process. This is desirable, 
because the use of media::VideoFrame has several downsides:
1.) It implies access to a memory-mapped buffer, which we may not need. Some 
components need to transport video frames but do not need to access their data.
2.) Its interface fails to clearly communicate what information is actually 
contained. It may be just a frame format and metadata. It may be read/write 
access to a memory-mapped buffer. And it may or may not contain a shareable 
handle to a buffer.

List of changes:
* Have VideoCaptureController communicate client utilization to 
  VideoCaptureDevice via a new interface ConsumerFeedbackObserver. 
* In VideoCaptureManager, updated factory logic to correctly establish the 
  connection from the controller to the device.
* In interface media::VideoCaptureDevice::Client, added a |frame_feedback_id| 
  parameter to producer methods. This is needed to be able to report which 
  particular frame a reported consumer utilization corresponds to. The 
  ThreadSafeCaptureOracle is interested in this information.
* Store the |frame_feedback_id| inside instances of 
  media::VideoCaptureDevice::Client::Buffer, which are passed along to the 
  controller.
* In VideoCaptureController we now have to keep track of the consumer count for 
  each buffer in order to be able to send out the maximum consumer utilization 
  when all consumers have finished consuming a buffer. We do this by introducing
  a std::map<int, BufferState> |buffer_id_to_state_map_|.
  VideoCaptureController is the right place for keeping track of the consumer 
  count, because it is the place where access to a single device is shared to 
  multiple clients.
* Also in video_capture_controller.h/cc: 
  * Removed some sync token related code that was unused (and would otherwise 
    have to be modified for this CL)
  * Instead of holding VideoFrame references in each ControllerClient in a 
    std::map<int, scoped_refptr<VideoFrame>> |active_buffers|, we keep a single 
    VideoFrame reference in the BufferState in |buffer_id_to_state_map|. In 
    the ControllerClients we then only need to track a std::set<int> 
    |buffers_in_use|.

Note that the same mechanism of using RESOURCE_UTILIZATION as a communication 
channel is still in use on the Renderer side, where it is set by 
media/cast/sender/video_sender.cc and read by video_capture_impl.cc for 
reporting it back to the browser process. I think it would be better to use a 
callback interface here as well and treat video frames as read-only. But since 
this CL does not change the Renderer side, we can leave it as is for now.

The following media::VideoCaptureDevice implementations have been updated to 
listen on the new communication channel:
 * desktop_capture_device_aura.h
 * web_contents_video_capture_device.h
 * screen_capture_device_android.h
TODO: There may be implementations in WebRTC that need to be updated as well.

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

BUG=584797
TEST=
  video_capture_unittests, 
  content_unittests, 
  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-02 01:28:29]

I followed miu's suggestion to get rid of the map from buffer ids to frame ids
and instead place a frame_feedback_id into the
media::VideoCaptureDevice::Client::Buffer instances, and this enabled a huge
simplification.

The consumer feedback can now be communicated directly from the
VideoCaptureController to the VideoCaptureDevice. We no longer need to go
through the VideoCaptureDeviceClient for the translation from buffer ids to
frame ids. This also eliminates the need for extra threading complexities and
for the extra unit test.

As another change, I am now making sure that the consumer feedback is sent to
the VideoCaptureDevice on the "device thread" instead of the IO thread.

Unfortunately, the current patch set produces a hanging test
WebContentsVideoCaptureDeviceTest.ProvidesRefreshFrames and I have yet to figure
out why that happens. 

Please feel free to already have a look, and let me know if you can see what is
causing the hang from just looking at the code.

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

https://codereview.chromium.org/2518143004/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.cc:176:
buffer_pool_->HoldForConsumers(buffer_id_, 1);
On 2016/12/01 05:25:17, miu wrote:
> Looks like the "hold count" tracking isn't needed in the buffer pool anymore.
> Future cleanup?

Excellent observation. I put it on the list of future cleanups in the design
doc.
Actually, with one of the follow-up CLs we may even no longer need to
differentiate between consumer hold and producer hold (but I am getting ahead of
myself here). We may be able to do apply both of those simplifications to the
buffer pool in one go.

https://codereview.chromium.org/2518143004/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.cc:416: if
(buffer_id_to_state_map_.find(buffer_id) ==
On 2016/12/01 05:25:17, miu wrote:
> As mentioned in the header file, you might be able to eliminate the zero-arg
> ctor and 3 map look-ups here by using iterators:
> 
>   // Insert if not exists.
>   const auto it = buffer_id_to_state_map_.insert(
>       std::make_pair(buffer_id, BufferState(...))).first;
>   BufferState& buffer_state = it->second;

Very nice solution. Thanks.
Unfortunately, in order to make the invariant members of BufferState const, we
now have to make a copy, but I think it is worth it. If only our compiler would
support std::map::emplace...

https://codereview.chromium.org/2518143004/diff/60001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_controller.h (right):

https://codereview.chromium.org/2518143004/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.h:153:
BufferState();
On 2016/12/01 05:25:17, miu wrote:
> IIRC, you don't need the zero-arg constructor if you work with iterators
instead
> of the map[] operators. Based on some of the impl, it looks like that might
also
> reduce the number of look-ups you're doing.

Done.

https://codereview.chromium.org/2518143004/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.h:168: int
buffer_id_;
On 2016/12/01 05:25:17, miu wrote:
> If you can eliminate the zero-arg ctor, several of these can become const.

Done.

https://codereview.chromium.org/2518143004/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.h:195:
std::map<int, BufferState> buffer_id_to_state_map_;
On 2016/12/01 05:25:17, miu wrote:
> naming nit: Up to you, but |buffer_states_| or |buffer_states_map_| seems more
> readable to me.

I could live with |buffer_states_|, but I prefer |buffer_id_to_state_map_|
because it makes it obvious to the reader that it is a map indexed by buffer
ids. With the shorter name, the reader would have to hunt down usages or a
comment in order to get that information.

@Miguel, please comment. If you prefer the shorter name as well, I will make the
change.

https://codereview.chromium.org/2518143004/diff/60001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

https://codereview.chromium.org/2518143004/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager.cc:135: // Phase 1:
When first created (in GetOrCreateDeviceEntry()), this consists of a
On 2016/12/01 05:25:18, miu wrote:
> s/consists of a only a controller/consists of only a controller/

Done.

https://codereview.chromium.org/2518143004/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager.cc:531:
request->serial_id(), device_client_ptr));
On 2016/12/01 05:25:17, miu wrote:
> Is this safe (passing the raw pointer)? Could, in some rare timings, the
device
> client be destroyed before the task runs? I ask because it looks like we're
> going over to the device thread to start the capture device, transferring
> ownership of the device client to that thread, and then we are hopping back to
> the IO thread to call OnDeviceStarted().

Yes, but that fact that you had to raise the question indicates that it was too
non-obvious why it worked. 

The good news is that with the simplifications enabled by your other
suggestions, we no longer need to do this. The controller can now talk directly
to the device instead of going through the device_client.

https://codereview.chromium.org/2518143004/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager.cc:624:
device_client->SetConsumerLoadObserver(video_capture_device.get());
On 2016/12/01 05:25:17, miu wrote:
> OOC, should we call SetConsumerLoadObserver(nullptr) before
StopAndDeallocate()?
> Or, do we know that it's impossible for the load observer to have its method
> called any time at or after that point? Could there be pending tasks that will
> call the observer's method in some other thread's task queue?

Again, this is no longer needed with the other simplifications.
I added the following comment to the place where the controller is connected to
the device:
    // Passing raw pointer |device.get()| to the controller is safe,
    // because we transfer ownership of it to |entry|. We are calling
    // SetConsumerFeedbackObserver(nullptr) before releasing
    // |entry->video_capture_device_| on the |device_task_runner_|.

https://codereview.chromium.org/2518143004/diff/60001/media/capture/content/s...
File media/capture/content/screen_capture_device_core.cc (right):

https://codereview.chromium.org/2518143004/diff/60001/media/capture/content/s...
media/capture/content/screen_capture_device_core.cc:119:
oracle_proxy_->OnConsumerReportingUtilization(frame_id, utilization);
On 2016/12/01 05:25:18, miu wrote:
> Could |oracle_proxy_| be null here? Not sure whether to:
> 
> 1. DCHECK(oracle_proxy_);
> 
> or
> 
> 2. if (state != kCapturing && state_ != kSuspended)
>      return;

Thanks for raising this.
This makes me realize that it is not a good idea to have this method be called
from threads other than the other methods (probably the "device thread"). I now
made changes to the factory logic in VideoCaptureManager and
VideoCaptureController to ensure that consumer utilization is always reported on
the device thread.

With this change, it should be guaranteed that no utilization reports arrive
after StopAndDeAllicate has been called. Therefore, I am going to opt for a
DCHECK.

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

https://codereview.chromium.org/2518143004/diff/60001/media/capture/content/t...
media/capture/content/thread_safe_capture_oracle.cc:239: base::AutoLock
guard(lock_);
On 2016/12/01 05:25:18, miu wrote:
> One of these days, I'm going to have to go through all this code and fix the
> threading model; and get rid of all the mutexes. ;-) It has, over a long
history
> and many authors, evolved to this point.

I hope it helps that calls to this method are now guaranteed to arrive from the
"device thread" (assuming no additional thread hopping happens between the
interface media::VideoCaptureDevice and here).

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

https://codereview.chromium.org/2518143004/diff/60001/media/capture/video/vid...
media/capture/video/video_capture_device.h:44: class CAPTURE_EXPORT
FrameReceiverObserver {
On 2016/12/01 05:25:18, miu wrote:
> Should this interface be in vcd_client.h instead?

With the simplification, this interface is no longer needed.
Removed.

https://codereview.chromium.org/2518143004/diff/60001/media/capture/video/vid...
media/capture/video/video_capture_device.h:51: static double
no_utilization_recorded() { return -1.0; }
On 2016/12/01 05:25:18, miu wrote:
> nit: static constexpr double kNoUtilizationRecorded = -1.0;

Done.

https://codereview.chromium.org/2518143004/diff/60001/media/capture/video/vid...
media/capture/video/video_capture_device.h:54: // During processing of a video
frame delivered to |receiver|, we may report
On 2016/12/01 05:25:18, miu wrote:
> 1. Please fix: The code comment refers to "|receiver|", but this is a
> class-level comment so there's no receiver variable.
> 
> 2. Maybe this should be a method comment for OnConsumerReportingUtilization()
> instead?

Done.

https://codereview.chromium.org/2518143004/diff/60001/media/capture/video/vid...
media/capture/video/video_capture_device.h:68: class CAPTURE_EXPORT
ConsumerLoadObserver {
On 2016/12/01 05:25:18, miu wrote:
> History: At one point, we were wondering whether we might include other forms
of
> feedback with each frame. I only plumbed-in utilization because that was all
> that was needed at the time.
> 
> So, you might consider naming this interface something like
> ConsumerFeedbackObserver instead; and then we might add more methods in the
> future (either per-frame, or maybe more aggregate feedback stats).

Done.

https://codereview.chromium.org/2518143004/diff/60001/media/capture/video/vid...
media/capture/video/video_capture_device.h:71: virtual void
OnConsumerReportingUtilization(int frame_id,
On 2016/12/01 05:25:18, miu wrote:
> naming nit: s/frame_id/frame_feedback_id/ everywhere (i.e., a little more
> descriptive to hint at how this is being used).

Done.

https://codereview.chromium.org/2518143004/diff/60001/media/capture/video/vid...
media/capture/video/video_capture_device.h:72: double utilization){};
On 2016/12/01 05:25:18, miu wrote:
> style nit: add space before open brace

Oops. Seems there was an extra ";" which caused the auto format to go crazy.
Fixed.

https://codereview.chromium.org/2518143004/diff/60001/media/capture/video/vid...
media/capture/video/video_capture_device.h:120: int frame_id = 0) = 0;
On 2016/12/01 05:25:18, miu wrote:
> Please document the new argument.

Done.

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

https://codereview.chromium.org/2518143004/diff/60001/media/capture/video/vid...
media/capture/video/video_capture_device_client.cc:442: if
(utilization_reporting_task_runner_->BelongsToCurrentThread()) {
Luckily, we were able to eliminate the need for all three of the new methods
based on your other suggestions :).

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

https://codereview.chromium.org/2518143004/diff/60001/media/capture/video/vid...
media/capture/video/video_capture_device_client.h:137: std::map<int, int>
buffer_id_to_frame_id_map_;
On 2016/12/01 05:25:18, miu wrote:
> Instead of this map, can you tag the frame feedback ids onto the Buffer? That
> might help avoid all the extra code to maintain this separate map on the side.

Excellent suggestions. This greatly simplifies things.

[chfremer, 2016-12-02 18:43:43]

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

[commit-bot: I haz the power, 2016-12-02 18:44:10]

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-02 18:49:05]

Fixed the issue with the hanging test.
Turns out I was a bit careless in PatchSet #2 and thought I could get away with
reshuffling some statements in thread_safe_capture_oracle.cc. 

Fixed in PatchSet #3 by reverting the reshuffling and instead adding a method
VideoCaptureOracle::next_frame_number() that allows us to obtain the next frame
number before calling RecordCapture().

[chfremer, 2016-12-02 18:50:06]

miu@: PTAL
mcasas@: PTAL

[commit-bot: I haz the power, 2016-12-02 19:00:17]

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

[commit-bot: I haz the power, 2016-12-02 19:00:18]

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

[chfremer, 2016-12-02 19:24:16]

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

[commit-bot: I haz the power, 2016-12-02 19:25:01]

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-02 21:26:49]

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

[commit-bot: I haz the power, 2016-12-02 21:26:49]

Dry run: This issue passed the CQ dry run.

[chfremer, 2016-12-02 21:44:25]

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

[commit-bot: I haz the power, 2016-12-02 21:45:33]

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

[miu, 2016-12-02 23:29:33]

PS6 lgtm % a few nits/considerations:

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

https://codereview.chromium.org/2518143004/diff/140001/content/browser/render...
content/browser/renderer_host/media/video_capture_controller.cc:120:
std::set<int> buffers_in_use;
Note on use of std::set/map/list/unordered_map: These are actually very
inefficient data structures when containing small numbers (less than a hundred
or so) of small-sized (16 bytes or less) elements. There's a "Using vector based
sets/maps to improve performance" thread on chromium-dev@ right now, and past
threads on that mailing list and the project-trim@ list about this.

Even when performance or efficiency does not seem to be an issue, use of these
data structures tends to "bust" the CPU cache, which causes other, unrelated
"hot" code paths to take a unexpected performance penalty.

The new guidance is to use std::vector for storage, and to do linear scans
before inserting (use std::find(vect.begin(), vect.end(), value)) if avoiding an
insert of duplicate entries is required. This will be more CPU and memory
efficient on modern architectures. Someone is looking into adding a "flat map"
in src/base, which will provide the std::map API, but be backed by the
more-efficient std::vector.

So, in new code (and if you prefer, fix old code as you come across it), please
try to use std::vector for small-sized values and a small numbers of elements.

https://codereview.chromium.org/2518143004/diff/140001/content/browser/render...
content/browser/renderer_host/media/video_capture_controller.cc:143: const
scoped_refptr<media::VideoCaptureBufferPool>& buffer_pool,
New smart pointer style guidelines: Both the |buffer_pool| and |frame| arguments
should not be passed by const-ref. Instead:

  scoped_refptr<media::VideoCaptureBufferPool> buffer_pool,
  scoped_refptr<media::VideoFrame> frame

...and then use std::move(x) in the initializer list to avoid redundant inc/dec
of the ref-counters.

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

https://codereview.chromium.org/2518143004/diff/140001/media/capture/video/vi...
media/capture/video/video_capture_device_client.cc:49: pool_(std::move(pool)),
nit: pool_ should go first as a data member, and in this initializer list, to
retain the same ordering as the ctor args.

[commit-bot: I haz the power, 2016-12-02 23:33:51]

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

[commit-bot: I haz the power, 2016-12-02 23:33:52]

Dry run: This issue passed the CQ dry run.

[chfremer, 2016-12-03 00:30:04]

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

[chfremer, 2016-12-03 00:30:36]

Addressed miu's comments.

mcasas@: PTAL

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

https://codereview.chromium.org/2518143004/diff/140001/content/browser/render...
content/browser/renderer_host/media/video_capture_controller.cc:120:
std::set<int> buffers_in_use;
On 2016/12/02 23:29:32, miu wrote:
> Note on use of std::set/map/list/unordered_map: These are actually very
> inefficient data structures when containing small numbers (less than a hundred
> or so) of small-sized (16 bytes or less) elements. There's a "Using vector
based
> sets/maps to improve performance" thread on chromium-dev@ right now, and past
> threads on that mailing list and the project-trim@ list about this.
> 
> Even when performance or efficiency does not seem to be an issue, use of these
> data structures tends to "bust" the CPU cache, which causes other, unrelated
> "hot" code paths to take a unexpected performance penalty.
> 
> The new guidance is to use std::vector for storage, and to do linear scans
> before inserting (use std::find(vect.begin(), vect.end(), value)) if avoiding
an
> insert of duplicate entries is required. This will be more CPU and memory
> efficient on modern architectures. Someone is looking into adding a "flat map"
> in src/base, which will provide the std::map API, but be backed by the
> more-efficient std::vector.
> 
> So, in new code (and if you prefer, fix old code as you come across it),
please
> try to use std::vector for small-sized values and a small numbers of elements.

Done.

https://codereview.chromium.org/2518143004/diff/140001/content/browser/render...
content/browser/renderer_host/media/video_capture_controller.cc:143: const
scoped_refptr<media::VideoCaptureBufferPool>& buffer_pool,
On 2016/12/02 23:29:32, miu wrote:
> New smart pointer style guidelines: Both the |buffer_pool| and |frame|
arguments
> should not be passed by const-ref. Instead:
> 
>   scoped_refptr<media::VideoCaptureBufferPool> buffer_pool,
>   scoped_refptr<media::VideoFrame> frame
> 
> ...and then use std::move(x) in the initializer list to avoid redundant
inc/dec
> of the ref-counters.

Done.

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

https://codereview.chromium.org/2518143004/diff/140001/media/capture/video/vi...
media/capture/video/video_capture_device_client.cc:49: pool_(std::move(pool)),
On 2016/12/02 23:29:32, miu wrote:
> nit: pool_ should go first as a data member, and in this initializer list, to
> retain the same ordering as the ctor args.

Done.

[commit-bot: I haz the power, 2016-12-03 00:31:15]

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

[mcasas, 2016-12-03 01:35:48]

lgtm with some comments to consider.

Also, the CL description is daunting... 
in the future, can we make them more 
summarised?

Finally, feel free to remove patches with 
no comments from reviewers on them.

https://codereview.chromium.org/2518143004/diff/160001/content/browser/render...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

https://codereview.chromium.org/2518143004/diff/160001/content/browser/render...
content/browser/renderer_host/media/video_capture_manager.cc:54: class
ConsumerFeedbackObserverOnTaskRunner
I would s/ConsumerFeedbackObserverOnTaskRunner/ConsumerFeedbackObserverImpl/
since working on a Task Runner should be an impl detail.

https://codereview.chromium.org/2518143004/diff/160001/content/browser/render...
content/browser/renderer_host/media/video_capture_manager.cc:72:
media::ConsumerFeedbackObserver* observer_;
media::ConsumerFeedbackObserver* const

https://codereview.chromium.org/2518143004/diff/160001/content/browser/render...
content/browser/renderer_host/media/video_capture_manager.cc:73:
scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
const

https://codereview.chromium.org/2518143004/diff/160001/content/browser/render...
content/browser/renderer_host/media/video_capture_manager.cc:164: // is
connected to the VideoCaptureController as the ConsumerFeedbackObserver.
This reads like: VCD is connected to the VCC as a ConsumerFeedbackObserver?
because the "it" is unclear in l.164.

Shouldn't it say that a new ConsumerFeedbackObserverOnTaskRunner
is created and ownership passed to the VCC?

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

https://codereview.chromium.org/2518143004/diff/160001/media/capture/video/vi...
media/capture/video/video_capture_device.h:44: class CAPTURE_EXPORT
ConsumerFeedbackObserver {
As a general naming comment, "ConsumerFeedbackObserver"
is a bit tenuous in the media:: namespace; perhaps sth
like PipelineUtilizationObserver or UpstreamUsageObserver
or even madness: {Pipeline,Usptream}UllageObserver :-)
(VideoCapturePipelineUtilizationObserver
is just ridiculous, but very definitory).

Same for the method's name:
s/OnConsumerReportingUtilization/OnUtilizationReport/ ?
since this class doesn't need to know if it's the Consumer
or someone else reporting the pipeline fillup level.

https://codereview.chromium.org/2518143004/diff/160001/media/capture/video/vi...
media/capture/video/video_capture_device.h:67: double utilization) {}
I'm dubious as to |frame_feedback_id|. Although
I see the point of frames being discarded out of 
(sent) order, what is a humble implementation to
do about it? Adding the parameter essentially 
forces the implementers to track a potentially 
unordered stream of utilization notifications, but
in all likelihood they shouldn't care too much
but just take the last such notification face value,
right?  

Or, perhaps the current final destination of the
utilization reports (i.e. the Oracles) are super 
sophisticated and indeed track individual reports 
when they don't really need to and can be 
simplified? (in another CL woud be ok ofc.).

[commit-bot: I haz the power, 2016-12-03 02:39:47]

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

[commit-bot: I haz the power, 2016-12-03 02:39:48]

Dry run: This issue passed the CQ dry run.

[chfremer, 2016-12-05 18:17:00]

Addressed mcasas@ comments.

> Also, the CL description is daunting... 
> in the future, can we make them more 
> summarised?

Absolutely yes. The detailed summary was intended as a courtesy to the
reviewers. I was hoping it would save you time otherwise spent reconstructing
the steps and the reasoning taken in this rather large change. I am happy to
adapt the level of detail to match the sweet spot. Will choose a higher level of
description for the next one.

https://codereview.chromium.org/2518143004/diff/160001/content/browser/render...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

https://codereview.chromium.org/2518143004/diff/160001/content/browser/render...
content/browser/renderer_host/media/video_capture_manager.cc:54: class
ConsumerFeedbackObserverOnTaskRunner
On 2016/12/03 01:35:48, mcasas wrote:
> I would s/ConsumerFeedbackObserverOnTaskRunner/ConsumerFeedbackObserverImpl/
> since working on a Task Runner should be an impl detail.

I am not sure I follow why it would be a good idea to hide this information from
the name of the class. The only code seeing the name of the class is the factory
logic that instantiates it, which is the following code snippet:

    entry->video_capture_controller()->SetConsumerFeedbackObserver(
        base::MakeUnique<ConsumerFeedbackObserverOnTaskRunner>(
            device.get(), device_task_runner_));

For the factory logic it is very much desirable to have the class name
communicate what the instance is going to do, because otherwise, the reader
would have to go and hunt down the actual implementation to read and infer what
it does.

For the clients who operate the instance, we use the abstraction
"ConsumerFeedbackObserver" which hides away all implementation details including
the name and construction details of the chosen implementation.

https://codereview.chromium.org/2518143004/diff/160001/content/browser/render...
content/browser/renderer_host/media/video_capture_manager.cc:72:
media::ConsumerFeedbackObserver* observer_;
On 2016/12/03 01:35:48, mcasas wrote:
> media::ConsumerFeedbackObserver* const

Done.

https://codereview.chromium.org/2518143004/diff/160001/content/browser/render...
content/browser/renderer_host/media/video_capture_manager.cc:73:
scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
On 2016/12/03 01:35:48, mcasas wrote:
> const 

Done.

https://codereview.chromium.org/2518143004/diff/160001/content/browser/render...
content/browser/renderer_host/media/video_capture_manager.cc:164: // is
connected to the VideoCaptureController as the ConsumerFeedbackObserver.
On 2016/12/03 01:35:48, mcasas wrote:
> This reads like: VCD is connected to the VCC as a ConsumerFeedbackObserver?

That is correct. This is the message the comment is trying to convey.

> because the "it" is unclear in l.164.

Agreed. See latest patch set for my attempt to improve this.
 
> Shouldn't it say that a new ConsumerFeedbackObserverOnTaskRunner
> is created and ownership passed to the VCC?

I feel the threading and ownership details of the connection should be omitted
in this high-level description.

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

https://codereview.chromium.org/2518143004/diff/160001/media/capture/video/vi...
media/capture/video/video_capture_device.h:44: class CAPTURE_EXPORT
ConsumerFeedbackObserver {
On 2016/12/03 01:35:48, mcasas wrote:
> As a general naming comment, "ConsumerFeedbackObserver"
> is a bit tenuous in the media:: namespace; perhaps sth
> like PipelineUtilizationObserver or UpstreamUsageObserver
> or even madness: {Pipeline,Usptream}UllageObserver :-)
> (VideoCapturePipelineUtilizationObserver
> is just ridiculous, but very definitory).
> 
> Same for the method's name:
> s/OnConsumerReportingUtilization/OnUtilizationReport/ ?
> since this class doesn't need to know if it's the Consumer
> or someone else reporting the pipeline fillup level.

Excellent point.
miu@ suggested "feedback" instead of "utilization" anticipating that we may in
the future want to add other forms of feedback from frame consumers to
producers. We can keep the term "utilization" in the method name. I am proposing
to name the interface "VideoFrameConsumerFeedbackObserver" and the method
"OnUtilizationReport".

We now have a ton of classes with "VideoFrame..." prefix in the media namespace.
This seems to indicate that it would be useful to create a namespace like
media::video_capture.

https://codereview.chromium.org/2518143004/diff/160001/media/capture/video/vi...
media/capture/video/video_capture_device.h:67: double utilization) {}
On 2016/12/03 01:35:48, mcasas wrote:
> I'm dubious as to |frame_feedback_id|. Although
> I see the point of frames being discarded out of 
> (sent) order, what is a humble implementation to
> do about it? Adding the parameter essentially 
> forces the implementers to track a potentially 
> unordered stream of utilization notifications, but
> in all likelihood they shouldn't care too much
> but just take the last such notification face value,
> right?  
> 
> Or, perhaps the current final destination of the
> utilization reports (i.e. the Oracles) are super 
> sophisticated and indeed track individual reports 
> when they don't really need to and can be 
> simplified? (in another CL woud be ok ofc.).

Good point. And I share the feeling.
I also agree that if we want to simplify, we should do it separately from this
CL, since this CL is trying to be a refactoring-only CL.

[mcasas, 2016-12-05 18:26:44]

still lgtm

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

https://codereview.chromium.org/2518143004/diff/160001/media/capture/video/vi...
media/capture/video/video_capture_device.h:67: double utilization) {}
On 2016/12/05 18:17:00, chfremer wrote:
> On 2016/12/03 01:35:48, mcasas wrote:
> > I'm dubious as to |frame_feedback_id|. Although
> > I see the point of frames being discarded out of 
> > (sent) order, what is a humble implementation to
> > do about it? Adding the parameter essentially 
> > forces the implementers to track a potentially 
> > unordered stream of utilization notifications, but
> > in all likelihood they shouldn't care too much
> > but just take the last such notification face value,
> > right?  
> > 
> > Or, perhaps the current final destination of the
> > utilization reports (i.e. the Oracles) are super 
> > sophisticated and indeed track individual reports 
> > when they don't really need to and can be 
> > simplified? (in another CL woud be ok ofc.).
> 
> Good point. And I share the feeling.
> I also agree that if we want to simplify, we should do it separately from this
> CL, since this CL is trying to be a refactoring-only CL.

Yup.

[chfremer, 2016-12-05 18:36:03]

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

[commit-bot: I haz the power, 2016-12-05 18:36: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-05 19:13:04]

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

[commit-bot: I haz the power, 2016-12-05 19:13:40]

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-05 23:07:50]

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

[commit-bot: I haz the power, 2016-12-05 23:07:53]

Dry run: This issue passed the CQ dry run.

[chfremer, 2016-12-05 23:31:37]

The CQ bit was checked by chfremer@chromium.org

[chfremer, 2016-12-05 23:31:38]

The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org,
mcasas@chromium.org
Link to the patchset: https://codereview.chromium.org/2518143004/#ps200001
(title: "Fix for android-only code")

[commit-bot: I haz the power, 2016-12-05 23:32:25]

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-05 23:38:11]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1480980697623430,
"parent_rev": "2f37a455fd2ef1dd216770264f30755e8de178a0", "commit_rev":
"4ac56e72b47b93e3fd64f5edbd4c18c237299bf1"}

[commit-bot: I haz the power, 2016-12-05 23:38:48]

Description was changed from

==========
[Mojo Video Capture] Replace RESOURCE_UTILIZATION with interface
ReceiverLoadObserver

Replace RESOURCE_UTILIZATION with interface ReceiverLoadObserver

The VideoFrame metadata field RESOURCE_UTILIZATION is currently being used as a 
hidden communication channel for consumers to report back their "utilization" 
(i.e. how busy they are) to interested producers. It appears that this mechanism
is used when using Cast to transmit captured content (e.g. 
desktop/window/tab sharing). In the Browser process, VideoCaptureController uses

this mechanism to report the highest utilization of its clients back to
ThreadSafeCaptureOracle, which uses a Destruction Observer to read out the 
metadata field when the VideoFrame instance it had sent out is destroyed.

Besides this mechanism being non-obvious and therefore fragile, it does not 
work across process boundaries (which we need when switching to a Mojo service).

This CL replaces this with a communication channel through a callback interface 
ReceiverLoadObserver which can be optionally given to VideoCaptureController. 
Doing so also allows us to move away from using media::VideoFrame for the 
transport of video frames, at least in the Browser process. This is desirable, 
because the use of media::VideoFrame has several downsides:
1.) It implies access to a memory-mapped buffer, which we may not need. Some 
components need to transport video frames but do not need to access their data.
2.) Its interface fails to clearly communicate what information is actually 
contained. It may be just a frame format and metadata. It may be read/write 
access to a memory-mapped buffer. And it may or may not contain a shareable 
handle to a buffer.

List of changes:
* Have VideoCaptureController communicate client utilization to 
  VideoCaptureDevice via a new interface ConsumerFeedbackObserver. 
* In VideoCaptureManager, updated factory logic to correctly establish the 
  connection from the controller to the device.
* In interface media::VideoCaptureDevice::Client, added a |frame_feedback_id| 
  parameter to producer methods. This is needed to be able to report which 
  particular frame a reported consumer utilization corresponds to. The 
  ThreadSafeCaptureOracle is interested in this information.
* Store the |frame_feedback_id| inside instances of 
  media::VideoCaptureDevice::Client::Buffer, which are passed along to the 
  controller.
* In VideoCaptureController we now have to keep track of the consumer count for 
  each buffer in order to be able to send out the maximum consumer utilization 
  when all consumers have finished consuming a buffer. We do this by introducing
  a std::map<int, BufferState> |buffer_id_to_state_map_|.
  VideoCaptureController is the right place for keeping track of the consumer 
  count, because it is the place where access to a single device is shared to 
  multiple clients.
* Also in video_capture_controller.h/cc: 
  * Removed some sync token related code that was unused (and would otherwise 
    have to be modified for this CL)
  * Instead of holding VideoFrame references in each ControllerClient in a 
    std::map<int, scoped_refptr<VideoFrame>> |active_buffers|, we keep a single 
    VideoFrame reference in the BufferState in |buffer_id_to_state_map|. In 
    the ControllerClients we then only need to track a std::set<int> 
    |buffers_in_use|.

Note that the same mechanism of using RESOURCE_UTILIZATION as a communication 
channel is still in use on the Renderer side, where it is set by 
media/cast/sender/video_sender.cc and read by video_capture_impl.cc for 
reporting it back to the browser process. I think it would be better to use a 
callback interface here as well and treat video frames as read-only. But since 
this CL does not change the Renderer side, we can leave it as is for now.

The following media::VideoCaptureDevice implementations have been updated to 
listen on the new communication channel:
 * desktop_capture_device_aura.h
 * web_contents_video_capture_device.h
 * screen_capture_device_android.h
TODO: There may be implementations in WebRTC that need to be updated as well.

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

BUG=584797
TEST=
  video_capture_unittests, 
  content_unittests, 
  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] Replace RESOURCE_UTILIZATION with interface
ReceiverLoadObserver

Replace RESOURCE_UTILIZATION with interface ReceiverLoadObserver

The VideoFrame metadata field RESOURCE_UTILIZATION is currently being used as a 
hidden communication channel for consumers to report back their "utilization" 
(i.e. how busy they are) to interested producers. It appears that this mechanism
is used when using Cast to transmit captured content (e.g. 
desktop/window/tab sharing). In the Browser process, VideoCaptureController uses

this mechanism to report the highest utilization of its clients back to
ThreadSafeCaptureOracle, which uses a Destruction Observer to read out the 
metadata field when the VideoFrame instance it had sent out is destroyed.

Besides this mechanism being non-obvious and therefore fragile, it does not 
work across process boundaries (which we need when switching to a Mojo service).

This CL replaces this with a communication channel through a callback interface 
ReceiverLoadObserver which can be optionally given to VideoCaptureController. 
Doing so also allows us to move away from using media::VideoFrame for the 
transport of video frames, at least in the Browser process. This is desirable, 
because the use of media::VideoFrame has several downsides:
1.) It implies access to a memory-mapped buffer, which we may not need. Some 
components need to transport video frames but do not need to access their data.
2.) Its interface fails to clearly communicate what information is actually 
contained. It may be just a frame format and metadata. It may be read/write 
access to a memory-mapped buffer. And it may or may not contain a shareable 
handle to a buffer.

List of changes:
* Have VideoCaptureController communicate client utilization to 
  VideoCaptureDevice via a new interface ConsumerFeedbackObserver. 
* In VideoCaptureManager, updated factory logic to correctly establish the 
  connection from the controller to the device.
* In interface media::VideoCaptureDevice::Client, added a |frame_feedback_id| 
  parameter to producer methods. This is needed to be able to report which 
  particular frame a reported consumer utilization corresponds to. The 
  ThreadSafeCaptureOracle is interested in this information.
* Store the |frame_feedback_id| inside instances of 
  media::VideoCaptureDevice::Client::Buffer, which are passed along to the 
  controller.
* In VideoCaptureController we now have to keep track of the consumer count for 
  each buffer in order to be able to send out the maximum consumer utilization 
  when all consumers have finished consuming a buffer. We do this by introducing
  a std::map<int, BufferState> |buffer_id_to_state_map_|.
  VideoCaptureController is the right place for keeping track of the consumer 
  count, because it is the place where access to a single device is shared to 
  multiple clients.
* Also in video_capture_controller.h/cc: 
  * Removed some sync token related code that was unused (and would otherwise 
    have to be modified for this CL)
  * Instead of holding VideoFrame references in each ControllerClient in a 
    std::map<int, scoped_refptr<VideoFrame>> |active_buffers|, we keep a single 
    VideoFrame reference in the BufferState in |buffer_id_to_state_map|. In 
    the ControllerClients we then only need to track a std::set<int> 
    |buffers_in_use|.

Note that the same mechanism of using RESOURCE_UTILIZATION as a communication 
channel is still in use on the Renderer side, where it is set by 
media/cast/sender/video_sender.cc and read by video_capture_impl.cc for 
reporting it back to the browser process. I think it would be better to use a 
callback interface here as well and treat video frames as read-only. But since 
this CL does not change the Renderer side, we can leave it as is for now.

The following media::VideoCaptureDevice implementations have been updated to 
listen on the new communication channel:
 * desktop_capture_device_aura.h
 * web_contents_video_capture_device.h
 * screen_capture_device_android.h
TODO: There may be implementations in WebRTC that need to be updated as well.

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

BUG=584797
TEST=
  video_capture_unittests, 
  content_unittests, 
  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-05 23:38:50]

Committed patchset #9 (id:200001)

[commit-bot: I haz the power, 2016-12-05 23:41:45]

Description was changed from

==========
[Mojo Video Capture] Replace RESOURCE_UTILIZATION with interface
ReceiverLoadObserver

Replace RESOURCE_UTILIZATION with interface ReceiverLoadObserver

The VideoFrame metadata field RESOURCE_UTILIZATION is currently being used as a 
hidden communication channel for consumers to report back their "utilization" 
(i.e. how busy they are) to interested producers. It appears that this mechanism
is used when using Cast to transmit captured content (e.g. 
desktop/window/tab sharing). In the Browser process, VideoCaptureController uses

this mechanism to report the highest utilization of its clients back to
ThreadSafeCaptureOracle, which uses a Destruction Observer to read out the 
metadata field when the VideoFrame instance it had sent out is destroyed.

Besides this mechanism being non-obvious and therefore fragile, it does not 
work across process boundaries (which we need when switching to a Mojo service).

This CL replaces this with a communication channel through a callback interface 
ReceiverLoadObserver which can be optionally given to VideoCaptureController. 
Doing so also allows us to move away from using media::VideoFrame for the 
transport of video frames, at least in the Browser process. This is desirable, 
because the use of media::VideoFrame has several downsides:
1.) It implies access to a memory-mapped buffer, which we may not need. Some 
components need to transport video frames but do not need to access their data.
2.) Its interface fails to clearly communicate what information is actually 
contained. It may be just a frame format and metadata. It may be read/write 
access to a memory-mapped buffer. And it may or may not contain a shareable 
handle to a buffer.

List of changes:
* Have VideoCaptureController communicate client utilization to 
  VideoCaptureDevice via a new interface ConsumerFeedbackObserver. 
* In VideoCaptureManager, updated factory logic to correctly establish the 
  connection from the controller to the device.
* In interface media::VideoCaptureDevice::Client, added a |frame_feedback_id| 
  parameter to producer methods. This is needed to be able to report which 
  particular frame a reported consumer utilization corresponds to. The 
  ThreadSafeCaptureOracle is interested in this information.
* Store the |frame_feedback_id| inside instances of 
  media::VideoCaptureDevice::Client::Buffer, which are passed along to the 
  controller.
* In VideoCaptureController we now have to keep track of the consumer count for 
  each buffer in order to be able to send out the maximum consumer utilization 
  when all consumers have finished consuming a buffer. We do this by introducing
  a std::map<int, BufferState> |buffer_id_to_state_map_|.
  VideoCaptureController is the right place for keeping track of the consumer 
  count, because it is the place where access to a single device is shared to 
  multiple clients.
* Also in video_capture_controller.h/cc: 
  * Removed some sync token related code that was unused (and would otherwise 
    have to be modified for this CL)
  * Instead of holding VideoFrame references in each ControllerClient in a 
    std::map<int, scoped_refptr<VideoFrame>> |active_buffers|, we keep a single 
    VideoFrame reference in the BufferState in |buffer_id_to_state_map|. In 
    the ControllerClients we then only need to track a std::set<int> 
    |buffers_in_use|.

Note that the same mechanism of using RESOURCE_UTILIZATION as a communication 
channel is still in use on the Renderer side, where it is set by 
media/cast/sender/video_sender.cc and read by video_capture_impl.cc for 
reporting it back to the browser process. I think it would be better to use a 
callback interface here as well and treat video frames as read-only. But since 
this CL does not change the Renderer side, we can leave it as is for now.

The following media::VideoCaptureDevice implementations have been updated to 
listen on the new communication channel:
 * desktop_capture_device_aura.h
 * web_contents_video_capture_device.h
 * screen_capture_device_android.h
TODO: There may be implementations in WebRTC that need to be updated as well.

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

BUG=584797
TEST=
  video_capture_unittests, 
  content_unittests, 
  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] Replace RESOURCE_UTILIZATION with interface
ReceiverLoadObserver

Replace RESOURCE_UTILIZATION with interface ReceiverLoadObserver

The VideoFrame metadata field RESOURCE_UTILIZATION is currently being used as a
hidden communication channel for consumers to report back their "utilization"
(i.e. how busy they are) to interested producers. It appears that this mechanism
is used when using Cast to transmit captured content (e.g.
desktop/window/tab sharing). In the Browser process, VideoCaptureController uses
this mechanism to report the highest utilization of its clients back to
ThreadSafeCaptureOracle, which uses a Destruction Observer to read out the
metadata field when the VideoFrame instance it had sent out is destroyed.

Besides this mechanism being non-obvious and therefore fragile, it does not
work across process boundaries (which we need when switching to a Mojo service).
This CL replaces this with a communication channel through a callback interface
ReceiverLoadObserver which can be optionally given to VideoCaptureController.
Doing so also allows us to move away from using media::VideoFrame for the
transport of video frames, at least in the Browser process. This is desirable,
because the use of media::VideoFrame has several downsides:
1.) It implies access to a memory-mapped buffer, which we may not need. Some
components need to transport video frames but do not need to access their data.
2.) Its interface fails to clearly communicate what information is actually
contained. It may be just a frame format and metadata. It may be read/write
access to a memory-mapped buffer. And it may or may not contain a shareable
handle to a buffer.

List of changes:
* Have VideoCaptureController communicate client utilization to
  VideoCaptureDevice via a new interface ConsumerFeedbackObserver.
* In VideoCaptureManager, updated factory logic to correctly establish the
  connection from the controller to the device.
* In interface media::VideoCaptureDevice::Client, added a |frame_feedback_id|
  parameter to producer methods. This is needed to be able to report which
  particular frame a reported consumer utilization corresponds to. The
  ThreadSafeCaptureOracle is interested in this information.
* Store the |frame_feedback_id| inside instances of
  media::VideoCaptureDevice::Client::Buffer, which are passed along to the
  controller.
* In VideoCaptureController we now have to keep track of the consumer count for
  each buffer in order to be able to send out the maximum consumer utilization
  when all consumers have finished consuming a buffer. We do this by introducing
  a std::map<int, BufferState> |buffer_id_to_state_map_|.
  VideoCaptureController is the right place for keeping track of the consumer
  count, because it is the place where access to a single device is shared to
  multiple clients.
* Also in video_capture_controller.h/cc:
  * Removed some sync token related code that was unused (and would otherwise
    have to be modified for this CL)
  * Instead of holding VideoFrame references in each ControllerClient in a
    std::map<int, scoped_refptr<VideoFrame>> |active_buffers|, we keep a single
    VideoFrame reference in the BufferState in |buffer_id_to_state_map|. In
    the ControllerClients we then only need to track a std::set<int>
    |buffers_in_use|.

Note that the same mechanism of using RESOURCE_UTILIZATION as a communication
channel is still in use on the Renderer side, where it is set by
media/cast/sender/video_sender.cc and read by video_capture_impl.cc for
reporting it back to the browser process. I think it would be better to use a
callback interface here as well and treat video frames as read-only. But since
this CL does not change the Renderer side, we can leave it as is for now.

The following media::VideoCaptureDevice implementations have been updated to
listen on the new communication channel:
 * desktop_capture_device_aura.h
 * web_contents_video_capture_device.h
 * screen_capture_device_android.h
TODO: There may be implementations in WebRTC that need to be updated as well.

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

BUG=584797
TEST=
  video_capture_unittests,
  content_unittests,
  capture_unittests,
  Apprtc loopback on Debug,
  Desktop Capture Example extension on Release

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

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

[commit-bot: I haz the power, 2016-12-05 23:41:46]

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