Reland: VideoCapture: migrate VideoCapture renderer-->host messages to mojo, part 1

Reland: VideoCapture: migrate VideoCapture renderer-->host messages to mojo, part 1

This CL migrates the renderer (client) side of the
communication VCImp --> VCHost that was forgotten
in the reverted CL (all due to my misunderstanding,
apologies!).

Unittests are also updated, constituting the bulk of
the CL.

Original CL description ------------------------------------------------
VideoCapture: migrate VideoCapture renderer-->host messages to mojo, part 1

This CL migrates the easy-to-migrate video_capture_messages from
IPC to mojom. "Easy" refers to those messages whose data types are
readily available.

BUG=651897
TEST=./out/gn/content_unittests --gtest_filter=*VideoCapture*,
content_browsertests and video capture working as before.

Committed: https://crrev.com/796b2e6cd3c150f29460c082fcc60ee2186ea9ab
Cr-Commit-Position: refs/heads/master@{#423388}

[mcasas, 2016-10-04 02:22:11]

Patchset #2 (id:20001) has been deleted

[mcasas, 2016-10-04 22:11:19]

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

[mcasas, 2016-10-04 22:11:19]

rockot@ PTAL at the changes in PS1

(Took a look at changing also the renderer side
in this CL but turned out too much code for a
reland IMHO).

[Ken Rockot(use gerrit already), 2016-10-04 23:35:36]

You're still not hooking up the client code to call these messages. Is that
intentional?

https://codereview.chromium.org/2390103002/diff/40001/content/common/media/vi...
File content/common/media/video_capture_messages.h (right):

https://codereview.chromium.org/2390103002/diff/40001/content/common/media/vi...
content/common/media/video_capture_messages.h:99:
IPC_MESSAGE_CONTROL1(VideoCaptureHostMsg_Pause,
Please delete these messages.

https://codereview.chromium.org/2390103002/diff/40001/content/common/video_ca...
File content/common/video_capture.mojom (right):

https://codereview.chromium.org/2390103002/diff/40001/content/common/video_ca...
content/common/video_capture.mojom:7: interface VideoCaptureRendererToBrowser
So actually, conventionally we've been using the message filter name for the
mojom interface when using BrowserAssociatedInterface. It signifies that the
interface still needs to be refactored and was just a rote conversion of legacy
IPC. So, maybe VideoCaptureHost would be a good name for this? (i.e.
content::mojom::VideoCaptureHost).

[miu, 2016-10-05 07:49:28]

miu@chromium.org changed reviewers:
+ miu@chromium.org

[miu, 2016-10-05 07:49:29]

https://codereview.chromium.org/2390103002/diff/40001/content/common/video_ca...
File content/common/video_capture.mojom (right):

https://codereview.chromium.org/2390103002/diff/40001/content/common/video_ca...
content/common/video_capture.mojom:12: Stop(int32 device_id);
In the Mojo world, we're supposed to stop using IDs like this. Instead, passing
handles to objects is preferred. Also, this is a great opportunity to eliminate
the old crufty IPC messaging, and replace it with a cleaner structure. (So,
let's not just 1:1 map IPC messages into Mojo interfaces.)

For example, how about something like:

interface VideoCaptureHost {
  // Starts a VCD. If fails, it does not bind |device| but does call
|client->StateChanged(<some error>)|.
  Start(...params..., VideoCaptureDevice& device, VideoCaptureDeviceClient
client);
};

// Control interface for a started VCD.
interface VideoCaptureDevice {
  Stop();
  Pause();
  Resume();
  RequestRefreshFrame();
};

// Delivery destination for frames and state changes.
interface VideoCaptureDeviceClient {
  StateChanged(...enum value...);

  // Called to deliver next video frame. VCDClient replies once it has finished
with the frame.
  FrameReady(params, shmem handle) => (sync_token,
consumer_resource_utilization);
};

[Ken Rockot(use gerrit already), 2016-10-05 08:14:13]

On Oct 5, 2016 12:50 AM, <miu@chromium.org> wrote:
>
>
>
https://codereview.chromium.org/2390103002/diff/40001/content/common/video_ca...
> File content/common/video_capture.mojom (right):
>
>
https://codereview.chromium.org/2390103002/diff/40001/content/common/video_ca...
> content/common/video_capture.mojom:12: Stop(int32 device_id);
> In the Mojo world, we're supposed to stop using IDs like this. Instead,
> passing handles to objects is preferred. Also, this is a great
> opportunity to eliminate the old crufty IPC messaging, and replace it
> with a cleaner structure. (So, let's not just 1:1 map IPC messages into
> Mojo interfaces.)
>
> For example, how about something like:
>
> interface VideoCaptureHost {
> // Starts a VCD. If fails, it does not bind |device| but does call
> |client->StateChanged(<some error>)|.
> Start(...params..., VideoCaptureDevice& device,
> VideoCaptureDeviceClient client);
> };
>
> // Control interface for a started VCD.
> interface VideoCaptureDevice {
> Stop();
> Pause();
> Resume();
> RequestRefreshFrame();
> };
>
> // Delivery destination for frames and state changes.
> interface VideoCaptureDeviceClient {
> StateChanged(...enum value...);
>
> // Called to deliver next video frame. VCDClient replies once it has
> finished with the frame.
> FrameReady(params, shmem handle) => (sync_token,
> consumer_resource_utilization);
> };
>
> https://codereview.chromium.org/2390103002/

Right, eventually no IDs. In cases where we have to convert many related
messages together though, sometimes over multiple patches, it's often too
much complexity to both convert and refactor the messages simultaneously.

We've provided tools like BrowserAssociatedInterface specifically to suit
cases like this, and in such cases rote message conversion is acceptable or
even encouraged.

-- 
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.

[mcasas, 2016-10-05 18:23:16]

Patchset #3 (id:60001) has been deleted

[mcasas, 2016-10-05 18:24:33]

Description was changed from

==========
Reland: VideoCapture: migrate VideoCapture renderer-->host messages to mojo,
part 1


Original CL description ------------------------------------------------
VideoCapture: migrate VideoCapture renderer-->host messages to mojo, part 1

This CL migrates the easy-to-migrate video_capture_messages from
IPC to mojom. "Easy" refers to those messages whose data types are
readily available.

BUG=651897
TEST=./out/gn/content_unittests --gtest_filter=*VideoCapture*,
content_browsertests and video capture working as before.
==========

to

==========
Reland: VideoCapture: migrate VideoCapture renderer-->host messages to mojo,
part 1

This CL migrates the renderer (client) side of the 
communication VCImp --> VCHost that was forgotten
in the reverted CL (all due to my misunderstanding,
apologies!).

Unittests are also updated, constituting the bulk of
the CL.

Original CL description ------------------------------------------------
VideoCapture: migrate VideoCapture renderer-->host messages to mojo, part 1

This CL migrates the easy-to-migrate video_capture_messages from
IPC to mojom. "Easy" refers to those messages whose data types are
readily available.

BUG=651897
TEST=./out/gn/content_unittests --gtest_filter=*VideoCapture*,
content_browsertests and video capture working as before.
==========

[mcasas, 2016-10-05 18:33:00]

ptal

https://codereview.chromium.org/2390103002/diff/40001/content/common/media/vi...
File content/common/media/video_capture_messages.h (right):

https://codereview.chromium.org/2390103002/diff/40001/content/common/media/vi...
content/common/media/video_capture_messages.h:99:
IPC_MESSAGE_CONTROL1(VideoCaptureHostMsg_Pause,
On 2016/10/04 23:35:36, Ken Rockot wrote:
> Please delete these messages.

Done.

https://codereview.chromium.org/2390103002/diff/40001/content/common/video_ca...
File content/common/video_capture.mojom (right):

https://codereview.chromium.org/2390103002/diff/40001/content/common/video_ca...
content/common/video_capture.mojom:7: interface VideoCaptureRendererToBrowser
On 2016/10/04 23:35:36, Ken Rockot wrote:
> So actually, conventionally we've been using the message filter name for the
> mojom interface when using BrowserAssociatedInterface. It signifies that the
> interface still needs to be refactored and was just a rote conversion of
legacy
> IPC. So, maybe VideoCaptureHost would be a good name for this? (i.e.
> content::mojom::VideoCaptureHost).

Yeah, totally my bad. I misunderstood the
migration I thought I could do "one side" (the
service implementation) at once. After offline
discussion, things are much clearer, and I did
the client side as well.

https://codereview.chromium.org/2390103002/diff/40001/content/common/video_ca...
content/common/video_capture.mojom:12: Stop(int32 device_id);
On 2016/10/05 07:49:29, miu wrote:
> In the Mojo world, we're supposed to stop using IDs like this. Instead,
passing
> handles to objects is preferred. Also, this is a great opportunity to
eliminate
> the old crufty IPC messaging, and replace it with a cleaner structure. (So,
> let's not just 1:1 map IPC messages into Mojo interfaces.)
> 
> For example, how about something like:
> 
> interface VideoCaptureHost {
>   // Starts a VCD. If fails, it does not bind |device| but does call
> |client->StateChanged(<some error>)|.
>   Start(...params..., VideoCaptureDevice& device, VideoCaptureDeviceClient
> client);
> };
> 
> // Control interface for a started VCD.
> interface VideoCaptureDevice {
>   Stop();
>   Pause();
>   Resume();
>   RequestRefreshFrame();
> };
> 
> // Delivery destination for frames and state changes.
> interface VideoCaptureDeviceClient {
>   StateChanged(...enum value...);
> 
>   // Called to deliver next video frame. VCDClient replies once it has
finished
> with the frame.
>   FrameReady(params, shmem handle) => (sync_token,
> consumer_resource_utilization);
> };
> 

I do agree, and in fact starting migrating the whole
shebang, but... it turned out to be too much, since
it depended in typemaps (e.g. VideoCaptureParams)
and other stuff (e.g. in //services/).  So I decided to
a) migrate all these messages as they are:  in my 
 experience all this legacy is full of hidden
 contracts and expectations, which makes migration
 look approachable only to mire the developer.
b) work in // with chfremer@ on the video capture
 service API, which will eventually replace this code:
 if you take a look at his CLs, you'll see that his API
 looks more mojo, just like yours.

So the plan is : migrate this IPC to mojo, see that
things don't break, prepare the new API, launch a 
Finch experiment with the new one. Makes sense?

[miu, 2016-10-05 18:52:46]

On 2016/10/05 18:33:00, mcasas wrote:
> I do agree, and in fact starting migrating the whole
> ...
> So the plan is : migrate this IPC to mojo, see that
> things don't break, prepare the new API, launch a 
> Finch experiment with the new one. Makes sense?

Sounds fine, except that it seems like you're creating extra work for yourself
(since there's already a plan to throw-away this mojoified IPC). Maybe I'm
misunderstanding something?

[Ken Rockot(use gerrit already), 2016-10-05 19:55:23]

It's not much more work, and we're considering message format conversion to
be a worthwhile effort on its own. For most conversions there's enough
complexity just around moving various IPC structs into mojom, so the bulk
of the effort is reusable, even if the nominal interfaces we create are
throw-away.


On Wed, Oct 5, 2016 at 11:52 AM, <miu@chromium.org> wrote:

> On 2016/10/05 18:33:00, mcasas wrote:
> > I do agree, and in fact starting migrating the whole
> > ...
> > So the plan is : migrate this IPC to mojo, see that
> > things don't break, prepare the new API, launch a
> > Finch experiment with the new one. Makes sense?
>
> Sounds fine, except that it seems like you're creating extra work for
> yourself
> (since there's already a plan to throw-away this mojoified IPC). Maybe I'm
> misunderstanding something?
>
> https://codereview.chromium.org/2390103002/
>

-- 
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.

[miu, 2016-10-05 20:07:45]

k. Sounds fine. I'll remove myself as reviewer, as my original comment was just
a drive-by.

[miu, 2016-10-05 20:07:55]

miu@chromium.org changed reviewers:
- miu@chromium.org

[Ken Rockot(use gerrit already), 2016-10-05 20:59:44]

Just some nits. LGTM

https://codereview.chromium.org/2390103002/diff/80001/content/common/video_ca...
File content/common/video_capture.mojom (right):

https://codereview.chromium.org/2390103002/diff/80001/content/common/video_ca...
content/common/video_capture.mojom:7: interface VideoCaptureService
nit: Chromium C++ style, brace on same line

As mentioned previously, I would strongly prefer you call this interface
VideoCaptureHost to mirror the MessageFilter name.

Even if you don't want to do that (why?) please don't call it a "Service"

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

https://codereview.chromium.org/2390103002/diff/80001/content/renderer/media/...
content/renderer/media/video_capture_impl.cc:561: if
(video_capture_service_.get())
minor nit: a more common pattern is

if (!foo.get()) {
  // initialize foo
}
return foo.get()

fewer return paths generally feels less confusing imho

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

https://codereview.chromium.org/2390103002/diff/80001/content/renderer/media/...
content/renderer/media/video_capture_message_filter.h:145: IPC::Channel*
sender_;
nit: rename |sender_| to |channel_|?

[mcasas, 2016-10-05 22:21:18]

Patchset #4 (id:100001) has been deleted

[mcasas, 2016-10-05 22:23:49]

mcasas@chromium.org changed reviewers:
+ avi@chromium.org, tsepez@chromium.org

[mcasas, 2016-10-05 22:23:50]

tsepez@: PTAL at removed IPC messages and
corresponding mojom entries .

avi@ RS BUILD.gn addition plz.

https://codereview.chromium.org/2390103002/diff/80001/content/common/video_ca...
File content/common/video_capture.mojom (right):

https://codereview.chromium.org/2390103002/diff/80001/content/common/video_ca...
content/common/video_capture.mojom:7: interface VideoCaptureService
On 2016/10/05 20:59:44, Ken Rockot wrote:
> nit: Chromium C++ style, brace on same line
> 
> As mentioned previously, I would strongly prefer you call this interface
> VideoCaptureHost to mirror the MessageFilter name.
> 
> Even if you don't want to do that (why?) please don't call it a "Service"

Oops, done s/VideoCaptureService/VideoCaptureHost/
here and elsewhere.

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

https://codereview.chromium.org/2390103002/diff/80001/content/renderer/media/...
content/renderer/media/video_capture_impl.cc:561: if
(video_capture_service_.get())
On 2016/10/05 20:59:44, Ken Rockot wrote:
> minor nit: a more common pattern is
> 
> if (!foo.get()) {
>   // initialize foo
> }
> return foo.get()
> 
> fewer return paths generally feels less confusing imho

Done.

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

https://codereview.chromium.org/2390103002/diff/80001/content/renderer/media/...
content/renderer/media/video_capture_message_filter.h:145: IPC::Channel*
sender_;
On 2016/10/05 20:59:44, Ken Rockot wrote:
> nit: rename |sender_| to |channel_|?

Done.

[Tom Sepez, 2016-10-05 22:25:43]

lgtm

[Avi (use Gerrit), 2016-10-05 22:27:51]

LGTM

[mcasas, 2016-10-05 23:39:33]

Patchset #5 (id:140001) has been deleted

[mcasas, 2016-10-05 23:42:02]

https://codereview.chromium.org/2390103002/diff/120001/content/browser/render...
File content/browser/renderer_host/media/video_capture_host_unittest.cc (right):

https://codereview.chromium.org/2390103002/diff/120001/content/browser/render...
content/browser/renderer_host/media/video_capture_host_unittest.cc:130: class
MockVideoCaptureHost : public VideoCaptureHost {
After the renaming in this PS, the linker
started silently linking this MockVideoCaptureHost
instead of the one in video_capture_impl_unittests,
causing strange crashes on destruction in that
test.  I sorted it out and cleaned up the tests in
the process (indenting etc).

[mcasas, 2016-10-06 00:34:35]

The CQ bit was checked by mcasas@chromium.org

[mcasas, 2016-10-06 00:34:36]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
tsepez@chromium.org, avi@chromium.org
Link to the patchset: https://codereview.chromium.org/2390103002/#ps160001
(title: "Sorted out tests after linker collision. Rebase")

[commit-bot: I haz the power, 2016-10-06 00:35:06]

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-10-06 01:48:32]

Description was changed from

==========
Reland: VideoCapture: migrate VideoCapture renderer-->host messages to mojo,
part 1

This CL migrates the renderer (client) side of the 
communication VCImp --> VCHost that was forgotten
in the reverted CL (all due to my misunderstanding,
apologies!).

Unittests are also updated, constituting the bulk of
the CL.

Original CL description ------------------------------------------------
VideoCapture: migrate VideoCapture renderer-->host messages to mojo, part 1

This CL migrates the easy-to-migrate video_capture_messages from
IPC to mojom. "Easy" refers to those messages whose data types are
readily available.

BUG=651897
TEST=./out/gn/content_unittests --gtest_filter=*VideoCapture*,
content_browsertests and video capture working as before.
==========

to

==========
Reland: VideoCapture: migrate VideoCapture renderer-->host messages to mojo,
part 1

This CL migrates the renderer (client) side of the 
communication VCImp --> VCHost that was forgotten
in the reverted CL (all due to my misunderstanding,
apologies!).

Unittests are also updated, constituting the bulk of
the CL.

Original CL description ------------------------------------------------
VideoCapture: migrate VideoCapture renderer-->host messages to mojo, part 1

This CL migrates the easy-to-migrate video_capture_messages from
IPC to mojom. "Easy" refers to those messages whose data types are
readily available.

BUG=651897
TEST=./out/gn/content_unittests --gtest_filter=*VideoCapture*,
content_browsertests and video capture working as before.
==========

[commit-bot: I haz the power, 2016-10-06 01:48:33]

Committed patchset #5 (id:160001)

[commit-bot: I haz the power, 2016-10-06 01:53:11]

Description was changed from

==========
Reland: VideoCapture: migrate VideoCapture renderer-->host messages to mojo,
part 1

This CL migrates the renderer (client) side of the 
communication VCImp --> VCHost that was forgotten
in the reverted CL (all due to my misunderstanding,
apologies!).

Unittests are also updated, constituting the bulk of
the CL.

Original CL description ------------------------------------------------
VideoCapture: migrate VideoCapture renderer-->host messages to mojo, part 1

This CL migrates the easy-to-migrate video_capture_messages from
IPC to mojom. "Easy" refers to those messages whose data types are
readily available.

BUG=651897
TEST=./out/gn/content_unittests --gtest_filter=*VideoCapture*,
content_browsertests and video capture working as before.
==========

to

==========
Reland: VideoCapture: migrate VideoCapture renderer-->host messages to mojo,
part 1

This CL migrates the renderer (client) side of the
communication VCImp --> VCHost that was forgotten
in the reverted CL (all due to my misunderstanding,
apologies!).

Unittests are also updated, constituting the bulk of
the CL.

Original CL description ------------------------------------------------
VideoCapture: migrate VideoCapture renderer-->host messages to mojo, part 1

This CL migrates the easy-to-migrate video_capture_messages from
IPC to mojom. "Easy" refers to those messages whose data types are
readily available.

BUG=651897
TEST=./out/gn/content_unittests --gtest_filter=*VideoCapture*,
content_browsertests and video capture working as before.

Committed: https://crrev.com/796b2e6cd3c150f29460c082fcc60ee2186ea9ab
Cr-Commit-Position: refs/heads/master@{#423388}
==========

[commit-bot: I haz the power, 2016-10-06 01:53:13]

Patchset 5 (id:??) landed as
https://crrev.com/796b2e6cd3c150f29460c082fcc60ee2186ea9ab
Cr-Commit-Position: refs/heads/master@{#423388}