[Mojo Video Capture] Split OnIncomingCapturedVideoFrame() to OnNewBuffer() and OnFrameReadyInBuffer(

[Mojo Video Capture] Split OnIncomingCapturedVideoFrame() to OnNewBuffer() and OnFrameReadyInBuffer()

In interface media::VideoFrameReceiver, instead of method
OnIncomingCapturedVideoFrame() use two methods OnNewBufferHandle() and
OnFrameReadyInBuffer(). This matches the Mojo interface exposed to the Renderers
by VideoCaptureHost.

Instead of requiring the VideoFrameReceiver implementation
(VideoCaptureController) to set and release a consumer hold, we pass in a
|buffer_read_permission| that encapsulates the hold using the RAII pattern.
This allows us to remove the |frame_buffer_pool_| member from
VideoCaptureController.

Use struct mojom::VideoFrameInfo when receiving frames as
media::VideoFrameReceiver (e.g. VideoCaptureController). This will allow us to
have it receive frames from the Mojo service in a later step. This required some
changes in media/capture/BUILD.gn, because the component formerly named
"capture" (now renamed to "capture_lib") now depends on
media/capture/mojo:capture_types, which in turn depends on classes formerly part
of "capture". I moved these classes to a new component "capture_base".

Since it had to be updated, freed video_capture_device_client_unittest.cc from
unnecessary context, which allowed moving it from content/ to
media/capture/video, which is where the class under test lives.

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

BUG=584797
TEST=
  content_unittests --gtest_filter="*Video*",
  capture_unittests --gtest_filter="VideoCaptureDeviceClientTest*"
  Apprtc loopback on Debug,
  Desktop Capture Example extension on Release
  On Android content_shell, run video capture sample from [2], put to
      background, then restore from background.

[1] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxsxWslJ_X-zpOhijvI8/edit?usp=sharing
[2] https://webrtc.github.io/samples/src/content/getusermedia/gum/

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

[chfremer, 2017-02-08 00:41:46]

Description was changed from

==========
presubmit


Added missing unittest file


Apply patch 11b_link2

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

to

==========
[Mojo Video Capture] Split OnIncomingCapturedVideoFrame() to OnNewBuffer() and
OnFrameReadyInBuffer()

In interface media::VideoFrameReceiver, instead of method
OnIncomingCapturedVideoFrame() use two methods OnNewBufferHandle() and
OnFrameReadyInBuffer(). This matches the Mojo interface exposed to the Renderers
by VideoCaptureHost. 

Instead of requiring the VideoFrameReceiver implementation
(VideoCaptureController) to set and release a consumer hold, we pass in a
|buffer_read_permission| that encapsulates the hold using the RAII pattern.
This allows us to remove the |frame_buffer_pool_| member from
VideoCaptureController.

Use struct mojom::VideoFrameInfo when receiving frames as
media::VideoFrameReceiver (e.g. VideoCaptureController). This will allow us to
have it receive frames from the Mojo service in a later step. This required some
changes in media/capture/BUILD.gn, because the component formerly named 
"capture" (now renamed to "capture_lib") now depends on 
media/capture/mojo:capture_types, which in turn depends on classes formerly part
of "capture". I moved these classes to a new component "capture_base".

Since it had to be updated, freed video_capture_device_client_unittest.cc from 
unnecessary context. This also enables us to move it from content/ to 
media/capture/video, which is where the class under test lives.

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

BUG=584797
TEST=
  content_unittests --gtest_filter="*Video*",
  capture_unittests --gtest_filter="VideoCaptureDeviceClientTest*"
  Apprtc loopback on Debug,
  Desktop Capture Example extension on Release
  On Android content_shell, run video capture sample from [2], put to
      background, then restore from background.

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
[2] https://webrtc.github.io/samples/src/content/getusermedia/gum/
==========

[chfremer, 2017-02-08 00:49:54]

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

[commit-bot: I haz the power, 2017-02-08 00:50:57]

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

[chfremer, 2017-02-08 00:55:03]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2017-02-08 00:57:18]

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

[commit-bot: I haz the power, 2017-02-08 00:57:21]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[chfremer, 2017-02-08 17:30:39]

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

[commit-bot: I haz the power, 2017-02-08 17:31:03]

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

[chfremer, 2017-02-08 17:38:55]

Description was changed from

==========
[Mojo Video Capture] Split OnIncomingCapturedVideoFrame() to OnNewBuffer() and
OnFrameReadyInBuffer()

In interface media::VideoFrameReceiver, instead of method
OnIncomingCapturedVideoFrame() use two methods OnNewBufferHandle() and
OnFrameReadyInBuffer(). This matches the Mojo interface exposed to the Renderers
by VideoCaptureHost. 

Instead of requiring the VideoFrameReceiver implementation
(VideoCaptureController) to set and release a consumer hold, we pass in a
|buffer_read_permission| that encapsulates the hold using the RAII pattern.
This allows us to remove the |frame_buffer_pool_| member from
VideoCaptureController.

Use struct mojom::VideoFrameInfo when receiving frames as
media::VideoFrameReceiver (e.g. VideoCaptureController). This will allow us to
have it receive frames from the Mojo service in a later step. This required some
changes in media/capture/BUILD.gn, because the component formerly named 
"capture" (now renamed to "capture_lib") now depends on 
media/capture/mojo:capture_types, which in turn depends on classes formerly part
of "capture". I moved these classes to a new component "capture_base".

Since it had to be updated, freed video_capture_device_client_unittest.cc from 
unnecessary context. This also enables us to move it from content/ to 
media/capture/video, which is where the class under test lives.

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

BUG=584797
TEST=
  content_unittests --gtest_filter="*Video*",
  capture_unittests --gtest_filter="VideoCaptureDeviceClientTest*"
  Apprtc loopback on Debug,
  Desktop Capture Example extension on Release
  On Android content_shell, run video capture sample from [2], put to
      background, then restore from background.

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
[2] https://webrtc.github.io/samples/src/content/getusermedia/gum/
==========

to

==========
[Mojo Video Capture] Split OnIncomingCapturedVideoFrame() to OnNewBuffer() and
OnFrameReadyInBuffer()

In interface media::VideoFrameReceiver, instead of method
OnIncomingCapturedVideoFrame() use two methods OnNewBufferHandle() and
OnFrameReadyInBuffer(). This matches the Mojo interface exposed to the Renderers
by VideoCaptureHost. 

Instead of requiring the VideoFrameReceiver implementation
(VideoCaptureController) to set and release a consumer hold, we pass in a
|buffer_read_permission| that encapsulates the hold using the RAII pattern.
This allows us to remove the |frame_buffer_pool_| member from
VideoCaptureController.

Use struct mojom::VideoFrameInfo when receiving frames as
media::VideoFrameReceiver (e.g. VideoCaptureController). This will allow us to
have it receive frames from the Mojo service in a later step. This required some
changes in media/capture/BUILD.gn, because the component formerly named 
"capture" (now renamed to "capture_lib") now depends on 
media/capture/mojo:capture_types, which in turn depends on classes formerly part
of "capture". I moved these classes to a new component "capture_base".

Since it had to be updated, freed video_capture_device_client_unittest.cc from 
unnecessary context, which allowed moving it from content/ to 
media/capture/video, which is where the class under test lives.

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

BUG=584797
TEST=
  content_unittests --gtest_filter="*Video*",
  capture_unittests --gtest_filter="VideoCaptureDeviceClientTest*"
  Apprtc loopback on Debug,
  Desktop Capture Example extension on Release
  On Android content_shell, run video capture sample from [2], put to
      background, then restore from background.

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
[2] https://webrtc.github.io/samples/src/content/getusermedia/gum/
==========

[chfremer, 2017-02-08 18:32:11]

Patchset #1 (id:20001) has been deleted

[chfremer, 2017-02-08 18:33:26]

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

[chfremer, 2017-02-08 18:33:27]

miu@: PTAL
mcasas@: PTAL

[commit-bot: I haz the power, 2017-02-08 19:17:46]

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

[commit-bot: I haz the power, 2017-02-08 19:17:47]

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

[chfremer, 2017-02-13 21:33:00]

On 2017/02/08 18:33:27, chfremer wrote:
> miu@: PTAL
> mcasas@: PTAL

ping

[miu, 2017-02-13 23:47:20]

Sorry. I was waiting for mcasas@ to make comments first, as he was a part of the
design process. ;-)

Looking now...

[miu, 2017-02-14 00:41:28]

Comments on PS1:

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

https://codereview.chromium.org/2686763002/diff/30001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.cc:49: // This
manual copy routine is needed because base::DictionaryValue does not
Maybe it'd be better to fix the mojo impl? Meaning, the copy ctor generated by
mojo should handle dictionary values correctly, because we use those all over
the Chromium project. Not sure if this means fixing typemaps, or something
internal to the mojo code generator.

The danger for leaving this as-is, is that someone could add fields to
media::mojom::VideoFrameInfo, and not account for them here. You could, I
suppose, add a DCHECK that does an "equals" comparison between |frame_info| and
|result|.

https://codereview.chromium.org/2686763002/diff/30001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.cc:52:
media::mojom::VideoFrameInfoPtr* frame_info) {
Seems that the type of this argument should be:

  const media::mojom::VideoFrameInfo& frame_info

https://codereview.chromium.org/2686763002/diff/30001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.cc:425: auto
frame_info_copy = CloneFrameInfo(&frame_info);
nit: You could just do:

  client->event_handler->OnBufferReady(
      client->controller_id, buffer_context_id,
      CloneFrameInfo(&frame_info));

https://codereview.chromium.org/2686763002/diff/30001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.cc:437: // Use
if-check, because we are in a loop and only want to do this
style nit: Don't use 2nd person in comments (i.e., "we" is not the focus).

https://codereview.chromium.org/2686763002/diff/30001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.cc:440:
buffer_context_iter->set_read_permission(
Perhaps this could be simplified by just calling set_read_permission() once
after the loop. Something like:

    for (...) {
      ...
      buffer_context_iter->IncreaseConsumerCount();
    }

    if (buffer_context_iter->HasConsumers()) {
      buffer_context_iter->set_read_permission(
          std::move(buffer_read_permission));
    }
  }

...or, if it's perfectly acceptable to always transfer the read permission (even
if it turns out there are no consumers), you could call set_read_permission()
unconditionally, either before or after the loop.

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

https://codereview.chromium.org/2686763002/diff/30001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller_event_handler.h:46:
media::mojom::VideoFrameInfoPtr frame_info) = 0;
Sanity-check: This new argument transfers ownership of the VideoFrameInfo. Is
that better than just providing a read-only "const VideoFrameInfo&"? Personally,
I lean toward whichever interface results in fewer memcpy's.

https://codereview.chromium.org/2686763002/diff/30001/media/capture/BUILD.gn
File media/capture/BUILD.gn (right):

https://codereview.chromium.org/2686763002/diff/30001/media/capture/BUILD.gn#...
media/capture/BUILD.gn:28: "//media",
Is all of //media needed, or just //media:base?

https://codereview.chromium.org/2686763002/diff/30001/media/capture/BUILD.gn#...
media/capture/BUILD.gn:123: "//media",
ditto: All of //media, or just one or two subtargets within?

https://codereview.chromium.org/2686763002/diff/30001/media/capture/BUILD.gn#...
media/capture/BUILD.gn:200: "//media",
ditto

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

https://codereview.chromium.org/2686763002/diff/30001/media/capture/video/vid...
media/capture/video/video_capture_device.h:116: std::unique_ptr<HandleProvider>
handle_provider;
style: Classes shouldn't have public-access data members. Perhaps you could add
a TakeHandleProvider() method to transfer ownership from this class to the new
owner?

Also, since the ownership can be transferred, does that mean all other
codepoints accessing this object need to include null checks?

https://codereview.chromium.org/2686763002/diff/30001/media/capture/video/vid...
media/capture/video/video_capture_device.h:117:
std::unique_ptr<ScopedAccessPermission> access_permission;
ditto

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

https://codereview.chromium.org/2686763002/diff/30001/media/capture/video/vid...
media/capture/video/video_capture_device_client.cc:378: info->metadata =
metadata.CopyInternalValues();
nit: Could you avoid this copy? Perhaps do the MergeMetadataFrom() and SetXXX()
calls on info->metadata directly rather than on the separate |metadata| local
object?

https://codereview.chromium.org/2686763002/diff/30001/media/capture/video/vid...
File media/capture/video/video_frame_receiver.h (right):

https://codereview.chromium.org/2686763002/diff/30001/media/capture/video/vid...
media/capture/video/video_frame_receiver.h:49: // A producer may reuse a retired
|buffer_id| immediately after this call.
This sounds like the producer could write new data while a consumer is still
reading from it. But, you mean only that the ID itself might be reused, right?

[chfremer, 2017-02-14 21:14:44]

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

[commit-bot: I haz the power, 2017-02-14 21:15:15]

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

[chfremer, 2017-02-14 21:15:16]

PTAL

Addressed miu@ comments.

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

https://codereview.chromium.org/2686763002/diff/30001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.cc:49: // This
manual copy routine is needed because base::DictionaryValue does not
On 2017/02/14 00:41:27, miu wrote:
> Maybe it'd be better to fix the mojo impl? Meaning, the copy ctor generated by
> mojo should handle dictionary values correctly, because we use those all over
> the Chromium project. Not sure if this means fixing typemaps, or something
> internal to the mojo code generator.
> 
> The danger for leaving this as-is, is that someone could add fields to
> media::mojom::VideoFrameInfo, and not account for them here. You could, I
> suppose, add a DCHECK that does an "equals" comparison between |frame_info|
and
> |result|.

Excellent suggestion. I didn't fully realize how dangerous this copy routine
was.
I had a look at the Mojo code, and found that it should be relatively simple to 
support cloning DictionaryValue by adding a specialization for it to template 
CloneTraits. Added to next patch set and adding rockot@ to check if that is 
a valid approach.

https://codereview.chromium.org/2686763002/diff/30001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.cc:52:
media::mojom::VideoFrameInfoPtr* frame_info) {
On 2017/02/14 00:41:27, miu wrote:
> Seems that the type of this argument should be:
> 
>   const media::mojom::VideoFrameInfo& frame_info

Done.

https://codereview.chromium.org/2686763002/diff/30001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.cc:425: auto
frame_info_copy = CloneFrameInfo(&frame_info);
On 2017/02/14 00:41:27, miu wrote:
> nit: You could just do:
> 
>   client->event_handler->OnBufferReady(
>       client->controller_id, buffer_context_id,
>       CloneFrameInfo(&frame_info));

Done.

https://codereview.chromium.org/2686763002/diff/30001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.cc:437: // Use
if-check, because we are in a loop and only want to do this
On 2017/02/14 00:41:27, miu wrote:
> style nit: Don't use 2nd person in comments (i.e., "we" is not the focus).

Done.

https://codereview.chromium.org/2686763002/diff/30001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.cc:440:
buffer_context_iter->set_read_permission(
On 2017/02/14 00:41:27, miu wrote:
> Perhaps this could be simplified by just calling set_read_permission() once
> after the loop. Something like:
> 
>     for (...) {
>       ...
>       buffer_context_iter->IncreaseConsumerCount();
>     }
> 
>     if (buffer_context_iter->HasConsumers()) {
>       buffer_context_iter->set_read_permission(
>           std::move(buffer_read_permission));
>     }
>   }
> 
> ...or, if it's perfectly acceptable to always transfer the read permission
(even
> if it turns out there are no consumers), you could call set_read_permission()
> unconditionally, either before or after the loop.

Thanks. Moving the check to after the loop makes it much more clear and
eliminates 
the need for the comment trying to explain it :-).

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

https://codereview.chromium.org/2686763002/diff/30001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller_event_handler.h:46:
media::mojom::VideoFrameInfoPtr frame_info) = 0;
On 2017/02/14 00:41:27, miu wrote:
> Sanity-check: This new argument transfers ownership of the VideoFrameInfo. Is
> that better than just providing a read-only "const VideoFrameInfo&"?
Personally,
> I lean toward whichever interface results in fewer memcpy's.

Yes. Transfer of ownership was intended here.
Not sure if it is better than passing a const reference, though.
Someone, either the caller (VideoCaptureController) or the receiver
(VideoCaptureHost) of the method has to make a copy of |frame_info|. It doesn't
really matter which side does the cloning. My thought was to let
VideoCapturecontroller do the cloning, since it has the responsibility of
"broadcasting" the frame to multiple clients. But since VideoCaptureHost may
decide to just drop the frame, I guess we could (in edge cases) avoid
unnecessary copies if we let VideoCaptureHost do the cloning.

Done.

https://codereview.chromium.org/2686763002/diff/30001/media/capture/BUILD.gn
File media/capture/BUILD.gn (right):

https://codereview.chromium.org/2686763002/diff/30001/media/capture/BUILD.gn#...
media/capture/BUILD.gn:28: "//media",
On 2017/02/14 00:41:27, miu wrote:
> Is all of //media needed, or just //media:base?

//media:base is a non-public part of //media, so we cannot depend on it
directly. (At least this is the case after having removed the visibility
exception that was previously granted for media/capture.)

https://codereview.chromium.org/2686763002/diff/30001/media/capture/BUILD.gn#...
media/capture/BUILD.gn:123: "//media",
On 2017/02/14 00:41:27, miu wrote:
> ditto: All of //media, or just one or two subtargets within?

same as above

https://codereview.chromium.org/2686763002/diff/30001/media/capture/BUILD.gn#...
media/capture/BUILD.gn:200: "//media",
On 2017/02/14 00:41:27, miu wrote:
> ditto

same as above

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

https://codereview.chromium.org/2686763002/diff/30001/media/capture/video/vid...
media/capture/video/video_capture_device.h:116: std::unique_ptr<HandleProvider>
handle_provider;
On 2017/02/14 00:41:27, miu wrote:
> style: Classes shouldn't have public-access data members. Perhaps you could
add
> a TakeHandleProvider() method to transfer ownership from this class to the new
> owner?
> 
> Also, since the ownership can be transferred, does that mean all other
> codepoints accessing this object need to include null checks?

I still feel this should be a plain struct. This was harder to argue for in the
previous CL, since it was passed around more and the ownership of the unique_ptr
members was never moved out. But with this CL this is really only used to bundle
input/output parameters in the communication between VideoCaptureDevice and its
Client.

I am proposing to make this a struct.

https://codereview.chromium.org/2686763002/diff/30001/media/capture/video/vid...
media/capture/video/video_capture_device.h:117:
std::unique_ptr<ScopedAccessPermission> access_permission;
On 2017/02/14 00:41:27, miu wrote:
> ditto

Acknowledged.

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

https://codereview.chromium.org/2686763002/diff/30001/media/capture/video/vid...
media/capture/video/video_capture_device_client.cc:378: info->metadata =
metadata.CopyInternalValues();
On 2017/02/14 00:41:27, miu wrote:
> nit: Could you avoid this copy? Perhaps do the MergeMetadataFrom() and
SetXXX()
> calls on info->metadata directly rather than on the separate |metadata| local
> object?

Unfortunately no. The type of info->metadata is
std::unique_ptr<base::DictionaryValue>, and it does not know these methods,
which are specific to VideoFrameMetadata.

https://codereview.chromium.org/2686763002/diff/30001/media/capture/video/vid...
File media/capture/video/video_frame_receiver.h (right):

https://codereview.chromium.org/2686763002/diff/30001/media/capture/video/vid...
media/capture/video/video_frame_receiver.h:49: // A producer may reuse a retired
|buffer_id| immediately after this call.
On 2017/02/14 00:41:27, miu wrote:
> This sounds like the producer could write new data while a consumer is still
> reading from it. But, you mean only that the ID itself might be reused, right?

Yes, the ID would be reused with a new buffer, not the old one. I updated the
comment to make it more clear. Please check if the updated comment is an
improvement.

[chfremer, 2017-02-14 21:17:28]

chfremer@chromium.org changed reviewers:
+ rockot@chromium.org

[chfremer, 2017-02-14 21:17:29]

rockot@: Please review changes in mojo/public/cpp.

I added a template specialization for CloneTraits to support
std::unique_ptr<base::DictionaryValue>.

[commit-bot: I haz the power, 2017-02-14 21:44:32]

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

[commit-bot: I haz the power, 2017-02-14 21:44:33]

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, 2017-02-14 22:39:00]

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

[commit-bot: I haz the power, 2017-02-14 22:39:33]

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

[commit-bot: I haz the power, 2017-02-15 02:38:08]

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

[commit-bot: I haz the power, 2017-02-15 02:38:09]

Dry run: This issue passed the CQ dry run.

[miu, 2017-02-15 22:15:00]

PS3 lgtm (but I'll defer to rockot@ on the CloneTraits for
base::DictionaryValue).

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

https://codereview.chromium.org/2686763002/diff/70001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.h:137: bool
HasZeroConsumerHoldCount() const;
nit: Looking at the definition of the "Has" methods, it seems it would be better
to inline them since they're simple accessors. Otherwise, there will be extra
codegen (bloat in binary size).

https://codereview.chromium.org/2686763002/diff/70001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.h:138: bool
HasConsumers() const;
Your call, but HasConsumers() seems to be identical to
!HasZeroConsumerHoldCount(); so maybe you don't need it.

[Ken Rockot(use gerrit already), 2017-02-15 22:20:34]

On 2017/02/14 at 21:17:29, chfremer wrote:
> rockot@: Please review changes in mojo/public/cpp.
> 
> I added a template specialization for CloneTraits to support
std::unique_ptr<base::DictionaryValue>.

Can you please point me to where this is used? It doesn't seem like something we
should define in public/cpp/bindings.

[chfremer, 2017-02-15 22:34:11]

On 2017/02/15 22:20:34, Ken Rockot wrote:
> On 2017/02/14 at 21:17:29, chfremer wrote:
> > rockot@: Please review changes in mojo/public/cpp.
> > 
> > I added a template specialization for CloneTraits to support
> std::unique_ptr<base::DictionaryValue>.
> 
> Can you please point me to where this is used? It doesn't seem like something
we
> should define in public/cpp/bindings.

rockot@, sorry about the lack of context.
The reason I am proposing to add the CloneTraits specialization is that I need
to 
be able to clone instances of Mojo struct VideoFrameInfo. In PatchSet#3, the
clone
happens in file video_capture_host.cc, see 
https://codereview.chromium.org/2686763002/diff/70001/content/browser/rendere...

Without the extra CloneTraits, the Clone() method leads to a compiler error 
complaining that std::unique_ptr<base::DictionaryValue> cannot be copied.

I first added a manual copy routine in PatchSet#1, but miu@ correctly pointed
out 
the dangers of doing this.

If CloneTraits is not the right approach, what would be your recommendation?

[Ken Rockot(use gerrit already), 2017-02-15 22:39:37]

On 2017/02/15 at 22:34:11, chfremer wrote:
> On 2017/02/15 22:20:34, Ken Rockot wrote:
> > On 2017/02/14 at 21:17:29, chfremer wrote:
> > > rockot@: Please review changes in mojo/public/cpp.
> > > 
> > > I added a template specialization for CloneTraits to support
> > std::unique_ptr<base::DictionaryValue>.
> > 
> > Can you please point me to where this is used? It doesn't seem like
something we
> > should define in public/cpp/bindings.
> 
> rockot@, sorry about the lack of context.
> The reason I am proposing to add the CloneTraits specialization is that I need
to 
> be able to clone instances of Mojo struct VideoFrameInfo. In PatchSet#3, the
clone

Ah, this is what I meant. So VideoFrameInfo uses
mojo.common.mojom.DictionaryValue.

> happens in file video_capture_host.cc, see 
>
https://codereview.chromium.org/2686763002/diff/70001/content/browser/rendere...
> 
> Without the extra CloneTraits, the Clone() method leads to a compiler error 
> complaining that std::unique_ptr<base::DictionaryValue> cannot be copied.
> 
> I first added a manual copy routine in PatchSet#1, but miu@ correctly pointed
out 
> the dangers of doing this.
> 
> If CloneTraits is not the right approach, what would be your recommendation?

CloneTraits is absolutely the right approach for this problem, but this is the
wrong place to define them. They should be defined in
//mojo/common/values_struct_traits.h&cc (please don't inline the impl in the
header) where the other base/values related traits live.

[chfremer, 2017-02-15 23:07:32]

Patchset #4 (id:90001) has been deleted

[chfremer, 2017-02-15 23:24:21]

On 2017/02/15 22:39:37, Ken Rockot wrote:
> On 2017/02/15 at 22:34:11, chfremer wrote:
> > On 2017/02/15 22:20:34, Ken Rockot wrote:
> > > On 2017/02/14 at 21:17:29, chfremer wrote:
> > > > rockot@: Please review changes in mojo/public/cpp.
> > > > 
> > > > I added a template specialization for CloneTraits to support
> > > std::unique_ptr<base::DictionaryValue>.
> > > 
> > > Can you please point me to where this is used? It doesn't seem like
> something we
> > > should define in public/cpp/bindings.
> > 
> > rockot@, sorry about the lack of context.
> > The reason I am proposing to add the CloneTraits specialization is that I
need
> to 
> > be able to clone instances of Mojo struct VideoFrameInfo. In PatchSet#3, the
> clone
> 
> Ah, this is what I meant. So VideoFrameInfo uses
> mojo.common.mojom.DictionaryValue.
> 
> > happens in file video_capture_host.cc, see 
> >
>
https://codereview.chromium.org/2686763002/diff/70001/content/browser/rendere...
> > 
> > Without the extra CloneTraits, the Clone() method leads to a compiler error 
> > complaining that std::unique_ptr<base::DictionaryValue> cannot be copied.
> > 
> > I first added a manual copy routine in PatchSet#1, but miu@ correctly
pointed
> out 
> > the dangers of doing this.
> > 
> > If CloneTraits is not the right approach, what would be your recommendation?
> 
> CloneTraits is absolutely the right approach for this problem, but this is the
> wrong place to define them. They should be defined in
> //mojo/common/values_struct_traits.h&cc (please don't inline the impl in the
> header) where the other base/values related traits live.

Thanks, Ken. I got it moved to values_struct_traits.h/cc now.
Some questions that came up while doing so:

1.) video_capture_host.cc now has to include mojo/common/values_struct_traits.h.
Is that okay, considering the include is not from mojo/public/*?
The connection to the Clone() method of VideoFrameInfo is not very obvious.
Is there a way to get this header included at a level where everyone using 
VideoFrameInfo would automatically get it?

2.) Is it okay that values_struct_traits.h adds something to the namespace 
mojo::internal?

[Ken Rockot(use gerrit already), 2017-02-15 23:37:00]

On Wed, Feb 15, 2017 at 3:24 PM, <chfremer@chromium.org> wrote:

> On 2017/02/15 22:39:37, Ken Rockot wrote:
> > On 2017/02/15 at 22:34:11, chfremer wrote:
> > > On 2017/02/15 22:20:34, Ken Rockot wrote:
> > > > On 2017/02/14 at 21:17:29, chfremer wrote:
> > > > > rockot@: Please review changes in mojo/public/cpp.
> > > > >
> > > > > I added a template specialization for CloneTraits to support
> > > > std::unique_ptr<base::DictionaryValue>.
> > > >
> > > > Can you please point me to where this is used? It doesn't seem like
> > something we
> > > > should define in public/cpp/bindings.
> > >
> > > rockot@, sorry about the lack of context.
> > > The reason I am proposing to add the CloneTraits specialization is
> that I
> need
> > to
> > > be able to clone instances of Mojo struct VideoFrameInfo. In
> PatchSet#3, the
> > clone
> >
> > Ah, this is what I meant. So VideoFrameInfo uses
> > mojo.common.mojom.DictionaryValue.
> >
> > > happens in file video_capture_host.cc, see
> > >
> >
> https://codereview.chromium.org/2686763002/diff/70001/
> content/browser/renderer_host/media/video_capture_host.cc
> > >
> > > Without the extra CloneTraits, the Clone() method leads to a compiler
> error
> > > complaining that std::unique_ptr<base::DictionaryValue> cannot be
> copied.
> > >
> > > I first added a manual copy routine in PatchSet#1, but miu@ correctly
> pointed
> > out
> > > the dangers of doing this.
> > >
> > > If CloneTraits is not the right approach, what would be your
> recommendation?
> >
> > CloneTraits is absolutely the right approach for this problem, but this
> is the
> > wrong place to define them. They should be defined in
> > //mojo/common/values_struct_traits.h&cc (please don't inline the impl
> in the
> > header) where the other base/values related traits live.
>
> Thanks, Ken. I got it moved to values_struct_traits.h/cc now.
> Some questions that came up while doing so:
>
> 1.) video_capture_host.cc now has to include mojo/common/values_struct_
> traits.h.
> Is that okay, considering the include is not from mojo/public/*?
> The connection to the Clone() method of VideoFrameInfo is not very obvious.
> Is there a way to get this header included at a level where everyone using
> VideoFrameInfo would automatically get it?
>

Ah, that's unfortunate. I suppose this isn't quite right. We apparently
don't otherwise need to expose a type's StructTraits headers from its
generated mojom header, because the traits are only used internally by
de/serialization code.


>
> 2.) Is it okay that values_struct_traits.h adds something to the namespace
> mojo::internal?
>

Hmm. Sorry, I hadn't noticed that. That's unfortunate and probably not
desirable. Perhaps we should instead support adding a static Clone method
to StructTraits. +yzshen for input


>
>
> https://codereview.chromium.org/2686763002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ken Rockot(use gerrit already), 2017-02-16 05:23:56]

rockot@chromium.org changed reviewers:
+ yzshen@chromium.org

[Ken Rockot(use gerrit already), 2017-02-16 05:23:57]

Actually adding yzshen to the reviewer list to make sure he sees this. Yuzhu
please see the question in the question(s) regarding CloneTraits

[yzshen1, 2017-02-16 07:26:28]

On 2017/02/16 05:23:57, Ken Rockot wrote:
> Actually adding yzshen to the reviewer list to make sure he sees this. Yuzhu
> please see the question in the question(s) regarding CloneTraits

Thanks for pointing me to this!

It seems a little bit odd to add Clone() to StructTraits, because StructTraits
deals with the conversion between CustomTypeFoo and MojomTypeFoo, while Clone()
has nothing to do with MojomTypeFoo. Imagine we have two StructTraits mapping
both MojomTypeFoo1 and MojomTypeFoo2 to CustomTypeFoo, it would be pretty weird
that we have to duplicate Clone() for CustomTypeFoo in these two StructTraits.
(People may argue that similarly IsNull() and SetToNull() shouldn't be part of
StructTraits. But I feel that they are different from Clone(). They deal with
conversion of the "nullable" bit of information.)

Maybe we should move CloneTraits outside of the internal:: namespace. But we
still have to figure out how to let users plug in the specialization. It is a
little unfortunate that CloneTraits is needed in the generated .h file instead
of the .cc file. So it cannot be placed in the "traits_headers" list of the
typemap configuration. You could probably put it in the "public_headers" list
but that is a little confusing. :/

I will need to think about it and get back to you.
In the meantime, if you don't want this issue to block your CL, you may want to
do manual clone instead. WDYT?

[yzshen1, 2017-02-16 17:23:17]

I looked at the code a little closer. How about:

1) move CloneTraits out of the internal:: namespace.
2) manually include your CloneTraits specialization if you need it, just like
what you have done in video_capture_host.cc.

It is a minority case to implement your own CloneTraits. Besides, if you forget
to include your own CloneTraits, compilation simply fails, so it won't be hard
to figure out (maybe we could make the code output an easier-to-read message
when it couldn't find appropriate specialization).

We are already doing similar things: in a StructTraits::Read implementation, for
example, if you want to read a field Foo out as CustomTypeFoo, you will need to
include StructTraits<CustomTypeFoo, MojomTypeFoo>.

WDYT?

https://codereview.chromium.org/2686763002/diff/130001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/clone_equals_util.h (right):

https://codereview.chromium.org/2686763002/diff/130001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/clone_equals_util.h:15: #include "base/values.h"
Is it really necessary to include base/values.h (and the others)?

[chfremer, 2017-02-16 18:13:26]

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

[commit-bot: I haz the power, 2017-02-16 18:13:47]

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

[chfremer, 2017-02-16 18:15:50]

yzshen@: PTAL

I moved CloneTraits out of the namespace internal.

https://codereview.chromium.org/2686763002/diff/130001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/clone_equals_util.h (right):

https://codereview.chromium.org/2686763002/diff/130001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/clone_equals_util.h:15: #include "base/values.h"
On 2017/02/16 17:23:16, yzshen1 wrote:
> Is it really necessary to include base/values.h (and the others)?
> 

Sorry, I forgot to remove those.
Done.

[commit-bot: I haz the power, 2017-02-16 18:19:02]

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

[commit-bot: I haz the power, 2017-02-16 18:19:04]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[yzshen1, 2017-02-16 18:36:11]

The changes regarding CloneTraits LGTM

[chfremer, 2017-02-16 18:37:56]

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

[commit-bot: I haz the power, 2017-02-16 18:38:36]

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

[commit-bot: I haz the power, 2017-02-16 20:40:32]

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

[commit-bot: I haz the power, 2017-02-16 20:40:33]

Dry run: This issue passed the CQ dry run.

[mcasas, 2017-02-16 22:35:56]

Looking good, a few comments/suggestions.

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

https://codereview.chromium.org/2686763002/diff/170001/content/browser/render...
content/browser/renderer_host/media/video_capture_controller.cc:149: }
Agree with miu@, keep HasConsumers() and change 
the calls sites of HasZeroConsumerHoldCount()
with !HasConsumers()  (which seems quite understandable
to me).

https://codereview.chromium.org/2686763002/diff/170001/content/browser/render...
content/browser/renderer_host/media/video_capture_controller.cc:402: if
(is_new_buffer) {
Alternative for l. 393-401 (not your problem, but since you're
in the area):

      bool is_new_buffer = false;
      if (!base::ContainsValue(client->known_buffer_context_ids,
                               buffer_context_id)) {
        is_new_buffer = true;
        client->known_buffer_context_ids.push_back(buffer_context_id);
      }

And maybe you could even remove |is_new_buffer|
and do inside the if(!...) the new commands in l.403-411.

https://codereview.chromium.org/2686763002/diff/170001/content/browser/render...
content/browser/renderer_host/media/video_capture_controller.cc:403: size_t
mapped_size =
const ?

https://codereview.chromium.org/2686763002/diff/170001/content/browser/render...
content/browser/renderer_host/media/video_capture_controller.cc:422:
DCHECK(false) << "Unexpected duplicate buffer: " << buffer_context_id;
What about l416-422 just:

  if (!base::ContainsValue(client->buffers_in_use, buffer_context_id))
    client->buffers_in_use.push_back(buffer_context_id);
  else 
    NOTREACHED() << "Unexpected duplicate buffer: " << buffer_context_id;

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

https://codereview.chromium.org/2686763002/diff/170001/content/browser/render...
content/browser/renderer_host/media/video_capture_controller.h:139:
mojo::ScopedSharedBufferHandle CreateHandleCopy();
nit: s/CreateHandleCopy/CloneHandle/ ?

https://codereview.chromium.org/2686763002/diff/170001/content/browser/render...
content/browser/renderer_host/media/video_capture_controller.h:166:
std::vector<BufferContext>::iterator FindUnretiredBufferContextFromBufferId(
This is totally unrelated to this CL, but wdyt about
s/FindUnretiredBufferContextFromBufferId
FindActiveBufferContextFromBufferId/  ?

In the sense of Buffer Contexts can be Active/Retired,
more than active/retired....

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

https://codereview.chromium.org/2686763002/diff/170001/media/capture/video/vi...
media/capture/video/video_capture_device_client.cc:57: class
BufferPoolConsumerReservationReleaser
This name is a bit large and from what it does
it's a bit like a ScopedXYZ, what about
ScopedBufferPoolConsumerReservation ?

Actually it's quite similar to  
BufferPoolProducerReservationReleaser, so 
what about this:


class Producer {
 public:
  static void Release(const scoped_refptr<VideoCaptureBufferPool>& buffer_pool,
                      int buffer_id) {
    buffer_pool->RelinquishProducerReservation(buffer_id);
  }
};

class Consumer {
 public:
  static void Release(const scoped_refptr<VideoCaptureBufferPool>& buffer_pool,
                      int buffer_id) {
    buffer_pool->RelinquishConsumerHold(buffer_id, 1);
  }
};

template<typename T>
class ScopedBufferPoolReservation
    : public VideoCaptureDevice::Client::Buffer::ScopedAccessPermission {
 public:
  ScopedBufferPoolReservation(scoped_refptr<VideoCaptureBufferPool> buffer_pool,
                              int buffer_id)
      : buffer_pool_(std::move(buffer_pool)), buffer_id_(buffer_id) {}

  ~ScopedBufferPoolReservation() override {
    T::Release(buffer_pool_, buffer_id_);
  }

 private:
  const scoped_refptr<VideoCaptureBufferPool> buffer_pool_;
  const int buffer_id_;
};

 private:
  const scoped_refptr<VideoCaptureBufferPool> buffer_pool_;
  const int buffer_id_;
};

And in the instantiation, e.g.
s/BufferPoolProducerReservationReleaser/BufferPoolReservationReleaser<Producer>/
etc


Producer and Consumer classes need to be there because 
a Template typename cannot be a function :(

[chfremer, 2017-02-16 23:37:57]

mcasas@: PTAL

Incorporated most of the suggestions.

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

https://codereview.chromium.org/2686763002/diff/70001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.h:137: bool
HasZeroConsumerHoldCount() const;
On 2017/02/15 22:15:00, miu wrote:
> nit: Looking at the definition of the "Has" methods, it seems it would be
better
> to inline them since they're simple accessors. Otherwise, there will be extra
> codegen (bloat in binary size).

Done.

https://codereview.chromium.org/2686763002/diff/70001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.h:138: bool
HasConsumers() const;
On 2017/02/15 22:15:00, miu wrote:
> Your call, but HasConsumers() seems to be identical to
> !HasZeroConsumerHoldCount(); so maybe you don't need it.

Done.

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

https://codereview.chromium.org/2686763002/diff/170001/content/browser/render...
content/browser/renderer_host/media/video_capture_controller.cc:149: }
On 2017/02/16 22:35:56, mcasas wrote:
> Agree with miu@, keep HasConsumers() and change 
> the calls sites of HasZeroConsumerHoldCount()
> with !HasConsumers()  (which seems quite understandable
> to me). 

Done.

https://codereview.chromium.org/2686763002/diff/170001/content/browser/render...
content/browser/renderer_host/media/video_capture_controller.cc:402: if
(is_new_buffer) {
On 2017/02/16 22:35:55, mcasas wrote:
> Alternative for l. 393-401 (not your problem, but since you're
> in the area):
> 
>       bool is_new_buffer = false;
>       if (!base::ContainsValue(client->known_buffer_context_ids,
>                                buffer_context_id)) {
>         is_new_buffer = true;
>         client->known_buffer_context_ids.push_back(buffer_context_id);
>       }
> 
> And maybe you could even remove |is_new_buffer|
> and do inside the if(!...) the new commands in l.403-411.

Excellent suggestion. Thanks.
Done.

https://codereview.chromium.org/2686763002/diff/170001/content/browser/render...
content/browser/renderer_host/media/video_capture_controller.cc:403: size_t
mapped_size =
On 2017/02/16 22:35:56, mcasas wrote:
> const ?

Done.

https://codereview.chromium.org/2686763002/diff/170001/content/browser/render...
content/browser/renderer_host/media/video_capture_controller.cc:422:
DCHECK(false) << "Unexpected duplicate buffer: " << buffer_context_id;
On 2017/02/16 22:35:56, mcasas wrote:
> What about l416-422 just:
> 
>   if (!base::ContainsValue(client->buffers_in_use, buffer_context_id))
>     client->buffers_in_use.push_back(buffer_context_id);
>   else 
>     NOTREACHED() << "Unexpected duplicate buffer: " << buffer_context_id;

Done.

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

https://codereview.chromium.org/2686763002/diff/170001/content/browser/render...
content/browser/renderer_host/media/video_capture_controller.h:139:
mojo::ScopedSharedBufferHandle CreateHandleCopy();
On 2017/02/16 22:35:56, mcasas wrote:
> nit: s/CreateHandleCopy/CloneHandle/ ?

Done.

https://codereview.chromium.org/2686763002/diff/170001/content/browser/render...
content/browser/renderer_host/media/video_capture_controller.h:166:
std::vector<BufferContext>::iterator FindUnretiredBufferContextFromBufferId(
On 2017/02/16 22:35:56, mcasas wrote:
> This is totally unrelated to this CL, but wdyt about
> s/FindUnretiredBufferContextFromBufferId
> FindActiveBufferContextFromBufferId/  ?
> 
> In the sense of Buffer Contexts can be Active/Retired,
> more than active/retired....

I considered your proposal, but ended up deciding it may be better to keep it as
is.
The reason is that introducing the term "Active" requires an explanation
somewhere saying that a BufferContext can be either Active or Retired, i.e.
these are mutually exclusive states. The best way to express that in code (to
avoid settling for a comment) would be using an enum class.
On the other hand, by using the term "Unretired" it is fairly clear that this
means that the |is_retired_| flag is not set, and we can do this with just a
boolean and no comment or enum class needed.

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

https://codereview.chromium.org/2686763002/diff/170001/media/capture/video/vi...
media/capture/video/video_capture_device_client.cc:57: class
BufferPoolConsumerReservationReleaser
On 2017/02/16 22:35:56, mcasas wrote:
> This name is a bit large and from what it does
> it's a bit like a ScopedXYZ, what about
> ScopedBufferPoolConsumerReservation ?
> 
> Actually it's quite similar to  
> BufferPoolProducerReservationReleaser, so 
> what about this:
> 
> 
> class Producer {
>  public:
>   static void Release(const scoped_refptr<VideoCaptureBufferPool>&
buffer_pool,
>                       int buffer_id) {
>     buffer_pool->RelinquishProducerReservation(buffer_id);
>   }
> };
> 
> class Consumer {
>  public:
>   static void Release(const scoped_refptr<VideoCaptureBufferPool>&
buffer_pool,
>                       int buffer_id) {
>     buffer_pool->RelinquishConsumerHold(buffer_id, 1);
>   }
> };
> 
> template<typename T>
> class ScopedBufferPoolReservation
>     : public VideoCaptureDevice::Client::Buffer::ScopedAccessPermission {
>  public:
>   ScopedBufferPoolReservation(scoped_refptr<VideoCaptureBufferPool>
buffer_pool,
>                               int buffer_id)
>       : buffer_pool_(std::move(buffer_pool)), buffer_id_(buffer_id) {}
> 
>   ~ScopedBufferPoolReservation() override {
>     T::Release(buffer_pool_, buffer_id_);
>   }
> 
>  private:
>   const scoped_refptr<VideoCaptureBufferPool> buffer_pool_;
>   const int buffer_id_;
> };
> 
>  private:
>   const scoped_refptr<VideoCaptureBufferPool> buffer_pool_;
>   const int buffer_id_;
> };
> 
> And in the instantiation, e.g.
>
s/BufferPoolProducerReservationReleaser/BufferPoolReservationReleaser<Producer>/
> etc
> 
> 
> Producer and Consumer classes need to be there because 
> a Template typename cannot be a function :(

Done.

[chfremer, 2017-02-16 23:38:34]

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

[commit-bot: I haz the power, 2017-02-16 23:39:39]

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

[mcasas, 2017-02-16 23:45:08]

lgtm % tiny comments

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

https://codereview.chromium.org/2686763002/diff/190001/content/browser/render...
content/browser/renderer_host/media/video_capture_controller.h:137: bool
has_consumers() const { return consumer_hold_count_ > 0; }
nit: hacker case for methods is reserved for plain
accessors, since this one does a calculation, it should
be called HasConsumers().

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

https://codereview.chromium.org/2686763002/diff/190001/media/capture/video/vi...
media/capture/video/video_capture_device_client.cc:73:
ScopedBufferPoolReservation<ConsumerReleaseTraits>;
This using seems unused, and I don't think is worth it.

[chfremer, 2017-02-16 23:57:19]

chfremer@chromium.org changed reviewers:
+ tsepez@chromium.org, xhwang@chromium.org

[chfremer, 2017-02-16 23:57:20]

xhwang@: please RS media/base/BUILD.gn
tsepez@: please RS media/capture/mojo/* and mojo/common/*

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

https://codereview.chromium.org/2686763002/diff/190001/content/browser/render...
content/browser/renderer_host/media/video_capture_controller.h:137: bool
has_consumers() const { return consumer_hold_count_ > 0; }
On 2017/02/16 23:45:07, mcasas wrote:
> nit: hacker case for methods is reserved for plain
> accessors, since this one does a calculation, it should
> be called HasConsumers().

Done.

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

https://codereview.chromium.org/2686763002/diff/190001/media/capture/video/vi...
media/capture/video/video_capture_device_client.cc:73:
ScopedBufferPoolReservation<ConsumerReleaseTraits>;
On 2017/02/16 23:45:07, mcasas wrote:
> This using seems unused, and I don't think is worth it.

Sorry, this was a temporary idea I later discarded and meant to remove before
uploading.
Done.

[Tom Sepez, 2017-02-17 00:45:09]

RS LGTM

[xhwang, 2017-02-21 20:52:35]

media/base/BUILD.gn LGTM, thank for the clean-up!

[chfremer, 2017-02-21 21:34:39]

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

[commit-bot: I haz the power, 2017-02-21 21:35:29]

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

[commit-bot: I haz the power, 2017-02-21 23:38:54]

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

[commit-bot: I haz the power, 2017-02-21 23:38:56]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)

[chfremer, 2017-02-22 02:22:20]

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

[commit-bot: I haz the power, 2017-02-22 02:23:28]

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

[commit-bot: I haz the power, 2017-02-22 04:26:15]

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

[commit-bot: I haz the power, 2017-02-22 04:26:18]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)

[chfremer, 2017-02-22 17:11:44]

The CQ bit was checked by chfremer@chromium.org

[chfremer, 2017-02-22 17:11:45]

The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org,
yzshen@chromium.org, mcasas@chromium.org
Link to the patchset: https://codereview.chromium.org/2686763002/#ps210001
(title: "mcasas@ suggestions mk2")

[commit-bot: I haz the power, 2017-02-22 17:12:30]

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

[commit-bot: I haz the power, 2017-02-22 20:07:07]

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

[commit-bot: I haz the power, 2017-02-22 20:07:10]

Failed to apply patch for media/capture/video/fake_video_capture_device.cc:
While running git apply --index -p1;
  error: patch failed: media/capture/video/fake_video_capture_device.cc:326
  error: media/capture/video/fake_video_capture_device.cc: patch does not apply

Patch:       media/capture/video/fake_video_capture_device.cc
Index: media/capture/video/fake_video_capture_device.cc
diff --git a/media/capture/video/fake_video_capture_device.cc
b/media/capture/video/fake_video_capture_device.cc
index
4aa961cc42c4f94e6d4090d3a0a5d1ea1bac5931..e91b7acfa26a97ce43f061519cad31ee911ea690
100644
--- a/media/capture/video/fake_video_capture_device.cc
+++ b/media/capture/video/fake_video_capture_device.cc
@@ -326,7 +326,7 @@ void FakeVideoCaptureDevice::CaptureUsingClientBuffers(
   DLOG_IF(ERROR, !capture_buffer.is_valid())
       << "Couldn't allocate Capture Buffer";
   auto buffer_access =
-      capture_buffer.handle_provider()->GetHandleForInProcessAccess();
+      capture_buffer.handle_provider->GetHandleForInProcessAccess();
   DCHECK(buffer_access->data()) << "Buffer has NO backing memory";
 
   DCHECK_EQ(PIXEL_STORAGE_CPU, capture_format_.pixel_storage);

[chfremer, 2017-02-22 22:50:20]

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

[commit-bot: I haz the power, 2017-02-22 22:51:24]

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

[commit-bot: I haz the power, 2017-02-23 01:47:41]

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

[commit-bot: I haz the power, 2017-02-23 01:47:43]

Dry run: This issue passed the CQ dry run.

[chfremer, 2017-02-23 17:51:13]

The CQ bit was checked by chfremer@chromium.org

[chfremer, 2017-02-23 17:51:14]

The patchset sent to the CQ was uploaded after l-g-t-m from mcasas@chromium.org,
yzshen@chromium.org, miu@chromium.org, tsepez@chromium.org, xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/2686763002/#ps230001
(title: "rebase")

[commit-bot: I haz the power, 2017-02-23 17:51:41]

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

[commit-bot: I haz the power, 2017-02-23 17:57:08]

CQ is committing da patch.

Bot data: {"patchset_id": 230001, "attempt_start_ts": 1487872273835200,
"parent_rev": "2c766c7081a39d69bdde6f008f1c5e0dc6d4388c", "commit_rev":
"59f15453e6e5a25f11c4740a4f24979abcd2684c"}

[commit-bot: I haz the power, 2017-02-23 18:01:55]

CQ is committing da patch.

Bot data: {"patchset_id": 230001, "attempt_start_ts": 1487872273835200,
"parent_rev": "1667265d233f9d54d66bc9c4ffe4cda1b2b0f1a7", "commit_rev":
"e09db164377ade3a5559dd9b1fae1f47756894b0"}

[commit-bot: I haz the power, 2017-02-23 18:03:37]

Description was changed from

==========
[Mojo Video Capture] Split OnIncomingCapturedVideoFrame() to OnNewBuffer() and
OnFrameReadyInBuffer()

In interface media::VideoFrameReceiver, instead of method
OnIncomingCapturedVideoFrame() use two methods OnNewBufferHandle() and
OnFrameReadyInBuffer(). This matches the Mojo interface exposed to the Renderers
by VideoCaptureHost. 

Instead of requiring the VideoFrameReceiver implementation
(VideoCaptureController) to set and release a consumer hold, we pass in a
|buffer_read_permission| that encapsulates the hold using the RAII pattern.
This allows us to remove the |frame_buffer_pool_| member from
VideoCaptureController.

Use struct mojom::VideoFrameInfo when receiving frames as
media::VideoFrameReceiver (e.g. VideoCaptureController). This will allow us to
have it receive frames from the Mojo service in a later step. This required some
changes in media/capture/BUILD.gn, because the component formerly named 
"capture" (now renamed to "capture_lib") now depends on 
media/capture/mojo:capture_types, which in turn depends on classes formerly part
of "capture". I moved these classes to a new component "capture_base".

Since it had to be updated, freed video_capture_device_client_unittest.cc from 
unnecessary context, which allowed moving it from content/ to 
media/capture/video, which is where the class under test lives.

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

BUG=584797
TEST=
  content_unittests --gtest_filter="*Video*",
  capture_unittests --gtest_filter="VideoCaptureDeviceClientTest*"
  Apprtc loopback on Debug,
  Desktop Capture Example extension on Release
  On Android content_shell, run video capture sample from [2], put to
      background, then restore from background.

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
[2] https://webrtc.github.io/samples/src/content/getusermedia/gum/
==========

to

==========
[Mojo Video Capture] Split OnIncomingCapturedVideoFrame() to OnNewBuffer() and
OnFrameReadyInBuffer()

In interface media::VideoFrameReceiver, instead of method
OnIncomingCapturedVideoFrame() use two methods OnNewBufferHandle() and
OnFrameReadyInBuffer(). This matches the Mojo interface exposed to the Renderers
by VideoCaptureHost.

Instead of requiring the VideoFrameReceiver implementation
(VideoCaptureController) to set and release a consumer hold, we pass in a
|buffer_read_permission| that encapsulates the hold using the RAII pattern.
This allows us to remove the |frame_buffer_pool_| member from
VideoCaptureController.

Use struct mojom::VideoFrameInfo when receiving frames as
media::VideoFrameReceiver (e.g. VideoCaptureController). This will allow us to
have it receive frames from the Mojo service in a later step. This required some
changes in media/capture/BUILD.gn, because the component formerly named
"capture" (now renamed to "capture_lib") now depends on
media/capture/mojo:capture_types, which in turn depends on classes formerly part
of "capture". I moved these classes to a new component "capture_base".

Since it had to be updated, freed video_capture_device_client_unittest.cc from
unnecessary context, which allowed moving it from content/ to
media/capture/video, which is where the class under test lives.

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

BUG=584797
TEST=
  content_unittests --gtest_filter="*Video*",
  capture_unittests --gtest_filter="VideoCaptureDeviceClientTest*"
  Apprtc loopback on Debug,
  Desktop Capture Example extension on Release
  On Android content_shell, run video capture sample from [2], put to
      background, then restore from background.

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
[2] https://webrtc.github.io/samples/src/content/getusermedia/gum/

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

[commit-bot: I haz the power, 2017-02-23 18:03:40]

Committed patchset #10 (id:230001) as
https://chromium.googlesource.com/chromium/src/+/e09db164377ade3a5559dd9b1fae...