VideoCapture: migrate last Renderer-->Host msg and start Host-->Renderer migration

VideoCapture: migrate Host-->Renderer OnStateUpdate message

This CL migrates the last video capture message renderer-->host
(wrongly named BufferReady, renamed to ReleaseBuffer since it's
an instruction/command). This allows for a few extra
simplifications in unit tests.

Also it begins the migration of messages from Host back
to Renderer. A mojom::VideoCaptureObserver is added,
implemented in VideoCaptureImpl via a binding valid between
Start() and Stop().   The host-->renderer OnStateUpdate
message is migrated to mojom.

BUG=651897
Committed: https://crrev.com/32a8820f9924f0f6033d779a40c7dd103afb83e9
Cr-Commit-Position: refs/heads/master@{#424339}

[mcasas, 2016-10-08 05:49:51]

Patchset #1 (id:1) has been deleted

[mcasas, 2016-10-08 16:57:32]

Patchset #1 (id:20001) has been deleted

[mcasas, 2016-10-08 16:57:41]

Patchset #1 (id:40001) has been deleted

[mcasas, 2016-10-08 22:28:06]

Patchset #1 (id:60001) has been deleted

[mcasas, 2016-10-09 16:49:43]

Description was changed from

==========
VideoCapture: migrate Host-->Renderer OnStateUpdate message

BUG=651897
==========

to

==========
VideoCapture: migrate Host-->Renderer OnStateUpdate message

This CL migrates the last video capture message renderer-->host
(wrongly named BufferReady) to mojo, which allows for a few
more simplifications in unit tests.

Also it starts migrating the communications from host back
to renderer, for that purpose a VideoCaptureObserver is
added, implemented on a VideoCaptureImpl binding valid
after Start().

BUG=651897
==========

[mcasas, 2016-10-09 16:51:05]

Description was changed from

==========
VideoCapture: migrate Host-->Renderer OnStateUpdate message

This CL migrates the last video capture message renderer-->host
(wrongly named BufferReady) to mojo, which allows for a few
more simplifications in unit tests.

Also it starts migrating the communications from host back
to renderer, for that purpose a VideoCaptureObserver is
added, implemented on a VideoCaptureImpl binding valid
after Start().

BUG=651897
==========

to

==========
VideoCapture: migrate Host-->Renderer OnStateUpdate message

This CL migrates the last video capture message renderer-->host
(wrongly named BufferReady) to mojo, which allows for a few
more simplifications in unit tests.

Also it starts migrating the communications from host back
to renderer. A mojom::VideoCaptureObserver is added, 
implemented via a VideoCaptureImpl binding valid after 
Start().  The host-->renderer OnStateUpdate message 
is migrated to mojom.

BUG=651897
==========

[mcasas, 2016-10-09 17:12:09]

Description was changed from

==========
VideoCapture: migrate Host-->Renderer OnStateUpdate message

This CL migrates the last video capture message renderer-->host
(wrongly named BufferReady) to mojo, which allows for a few
more simplifications in unit tests.

Also it starts migrating the communications from host back
to renderer. A mojom::VideoCaptureObserver is added, 
implemented via a VideoCaptureImpl binding valid after 
Start().  The host-->renderer OnStateUpdate message 
is migrated to mojom.

BUG=651897
==========

to

==========
VideoCapture: migrate Host-->Renderer OnStateUpdate message

This CL migrates the last video capture message renderer-->host
(wrongly named BufferReady, renamed to ReleaseBuffer since it's
an instruction/command). This allows for a few extra 
simplifications in unit tests.

Also it begins the migration of messages from Host back
to Renderer. A mojom::VideoCaptureObserver is added, 
implemented in VideoCaptureImpl via a binding valid between 
Start() and Stop().   The host-->renderer OnStateUpdate 
message is migrated to mojom.

BUG=651897
==========

[mcasas, 2016-10-09 17:13:56]

Patchset #4 (id:140001) has been deleted

[mcasas, 2016-10-09 20:56:19]

Patchset #1 (id:80001) has been deleted

[mcasas, 2016-10-09 20:56:27]

Patchset #1 (id:100001) has been deleted

[mcasas, 2016-10-09 20:56:35]

Patchset #1 (id:120001) has been deleted

[mcasas, 2016-10-09 20:56:45]

Patchset #1 (id:160001) has been deleted

[mcasas, 2016-10-09 21:14:28]

Patchset #1 (id:180001) has been deleted

[mcasas, 2016-10-09 21:15:53]

mcasas@chromium.org changed reviewers:
+ chfremer@chromium.org, rockot@chromium.org, tsepez@chromium.org

[mcasas, 2016-10-09 21:15:54]

rockot@ PTAL at the idioms I'm using to
get notifications back on VideoCaptureImpl
from VideoCaptureHost, mojo-wise plz.

chfremer@ PTAL/FYI

tsepez@ plz RS changes in video_capture_messages.h
and video_capture.mojom.

[Ken Rockot(use gerrit already), 2016-10-10 16:10:36]

lgtm

[Tom Sepez, 2016-10-10 17:27:35]

mojom LGTM

[chfremer, 2016-10-10 18:57:19]

lgtm % comments

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

https://codereview.chromium.org/2407623002/diff/200001/content/browser/render...
content/browser/renderer_host/media/video_capture_host.h:151: std::map<int32_t,
mojom::VideoCaptureObserverPtr> observers_;
Unclear what the int32_t parameter is.
device_id?
session_id
controller_id?

As an arbitrary example, if it was device_id, I recommend expressing that by
naming the member field either "observers_by_device_id_" or
"device_id_to_observer_map_".

https://codereview.chromium.org/2407623002/diff/200001/content/common/video_c...
File content/common/video_capture.mojom (right):

https://codereview.chromium.org/2407623002/diff/200001/content/common/video_c...
content/common/video_capture.mojom:38: // If successful, |observer| will be used
for notifications.
With Start() being an asynchronous method without response, how can clients know
whether or not the call was successful?

https://codereview.chromium.org/2407623002/diff/200001/content/renderer/media...
File content/renderer/media/video_capture_impl_unittest.cc (right):

https://codereview.chromium.org/2407623002/diff/200001/content/renderer/media...
content/renderer/media/video_capture_impl_unittest.cc:201:
EXPECT_CALL(mock_video_capture_host_, DoStart(_, kSessionId, params_small_));
Here and in the two tests below, why do we only expect a single invocation of
DoStart() and Stop()? Shouldn't we be expecting a sequence of invocations?

https://codereview.chromium.org/2407623002/diff/200001/content/renderer/media...
content/renderer/media/video_capture_impl_unittest.cc:285:
EXPECT_EQ(mock_video_capture_host_.released_buffer_count(), 0);
I am confused by this test case. 
We simulate that the host creates a new buffer and hands it to the client
(VideoCaptureImpl instance), which in turn asks the host to stop capturing. The
host then tells the client that it has destroyed the buffer with id 0, even
though the client never told the host to release the buffer.

What am I missing here?
Does StopCapture() implicitly tell the host that the client is releasing all
buffers?

[mcasas, 2016-10-10 19:39:43]

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

[mcasas, 2016-10-10 19:39:44]

avi@ plz RS content/common/BUILD.gn addition, thanks!

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

https://codereview.chromium.org/2407623002/diff/200001/content/browser/render...
content/browser/renderer_host/media/video_capture_host.h:151: std::map<int32_t,
mojom::VideoCaptureObserverPtr> observers_;
On 2016/10/10 18:57:19, chfremer wrote:
> Unclear what the int32_t parameter is.
> device_id?
> session_id
> controller_id?
> 
> As an arbitrary example, if it was device_id, I recommend expressing that by
> naming the member field either "observers_by_device_id_" or
> "device_id_to_observer_map_".

The first or the last one :) 
device_id == controller_id is an invariant of this class
(quite unfortunate, but whatever)
I like the last name though., pity about the
line breaks. Done though.

https://codereview.chromium.org/2407623002/diff/200001/content/common/video_c...
File content/common/video_capture.mojom (right):

https://codereview.chromium.org/2407623002/diff/200001/content/common/video_c...
content/common/video_capture.mojom:38: // If successful, |observer| will be used
for notifications.
On 2016/10/10 18:57:19, chfremer wrote:
> With Start() being an asynchronous method without response, how can clients
know
> whether or not the call was successful?

Actually this is wrongly stated :)  -- in mojo-land, 
this call should always work, since the 
VideoCaptureHost must exist to be called upon. Potential 
errors in its execution should be received via 
VideoCaptureObserver::OnStateChanged().

https://codereview.chromium.org/2407623002/diff/200001/content/renderer/media...
File content/renderer/media/video_capture_impl_unittest.cc (right):

https://codereview.chromium.org/2407623002/diff/200001/content/renderer/media...
content/renderer/media/video_capture_impl_unittest.cc:201:
EXPECT_CALL(mock_video_capture_host_, DoStart(_, kSessionId, params_small_));
On 2016/10/10 18:57:19, chfremer wrote:
> Here and in the two tests below, why do we only expect a single invocation of
> DoStart() and Stop()? Shouldn't we be expecting a sequence of invocations?

It's misleading, but due to the current implementation, 
the first cycle of start-stop does not set VideoCaptureImpl
back in a STOPPED state, but in a STOPPING, which
essentially delays the next start-stop beyond the limits
of this unittest.  Probably something to address, but
not in this CL.

https://cs.chromium.org/chromium/src/content/renderer/media/video_capture_imp...

https://codereview.chromium.org/2407623002/diff/200001/content/renderer/media...
content/renderer/media/video_capture_impl_unittest.cc:285:
EXPECT_EQ(mock_video_capture_host_.released_buffer_count(), 0);
On 2016/10/10 18:57:19, chfremer wrote:
> I am confused by this test case. 
> We simulate that the host creates a new buffer and hands it to the client
> (VideoCaptureImpl instance), which in turn asks the host to stop capturing.
The
> host then tells the client that it has destroyed the buffer with id 0, even
> though the client never told the host to release the buffer.
> 
> What am I missing here?
> Does StopCapture() implicitly tell the host that the client is releasing all
> buffers?

VideoCaptureImpl's client has a callback to release
the received buffer (bound in [1]). In this code, the
bound method is &VideoCaptureImplTest::OnFrameReady,
which is mocked but does nothing, so there is no
"releasing" of the received buffer, hence l.277 has a 
Times(0) to indicate that we don't expect to receive
anything there.

Since I just wanted to migrate IPC-->Mojom with as
little changes as possible, I didn't touch this, but I
might have made it more evident :)

I'm totally in favour of extending all these unit
tests though, and will probably do. Stay tuned.



[1]
https://cs.chromium.org/chromium/src/content/renderer/media/video_capture_imp...

[mcasas, 2016-10-10 21:32:44]

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

[commit-bot: I haz the power, 2016-10-10 21:33:11]

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-10-10 23:48:48]

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

[commit-bot: I haz the power, 2016-10-10 23:48:49]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Avi (use Gerrit), 2016-10-11 00:19:11]

content/common/BUILD.gn lgtm

[mcasas, 2016-10-11 01:54:42]

The CQ bit was checked by mcasas@chromium.org

[mcasas, 2016-10-11 01:54:42]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
rockot@chromium.org, chfremer@chromium.org
Link to the patchset: https://codereview.chromium.org/2407623002/#ps220001
(title: "chfremer@ comments")

[commit-bot: I haz the power, 2016-10-11 01:55:03]

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-11 02:03:10]

Description was changed from

==========
VideoCapture: migrate Host-->Renderer OnStateUpdate message

This CL migrates the last video capture message renderer-->host
(wrongly named BufferReady, renamed to ReleaseBuffer since it's
an instruction/command). This allows for a few extra 
simplifications in unit tests.

Also it begins the migration of messages from Host back
to Renderer. A mojom::VideoCaptureObserver is added, 
implemented in VideoCaptureImpl via a binding valid between 
Start() and Stop().   The host-->renderer OnStateUpdate 
message is migrated to mojom.

BUG=651897
==========

to

==========
VideoCapture: migrate Host-->Renderer OnStateUpdate message

This CL migrates the last video capture message renderer-->host
(wrongly named BufferReady, renamed to ReleaseBuffer since it's
an instruction/command). This allows for a few extra 
simplifications in unit tests.

Also it begins the migration of messages from Host back
to Renderer. A mojom::VideoCaptureObserver is added, 
implemented in VideoCaptureImpl via a binding valid between 
Start() and Stop().   The host-->renderer OnStateUpdate 
message is migrated to mojom.

BUG=651897
==========

[commit-bot: I haz the power, 2016-10-11 02:03:12]

Committed patchset #2 (id:220001)

[commit-bot: I haz the power, 2016-10-11 02:10:05]

Description was changed from

==========
VideoCapture: migrate Host-->Renderer OnStateUpdate message

This CL migrates the last video capture message renderer-->host
(wrongly named BufferReady, renamed to ReleaseBuffer since it's
an instruction/command). This allows for a few extra 
simplifications in unit tests.

Also it begins the migration of messages from Host back
to Renderer. A mojom::VideoCaptureObserver is added, 
implemented in VideoCaptureImpl via a binding valid between 
Start() and Stop().   The host-->renderer OnStateUpdate 
message is migrated to mojom.

BUG=651897
==========

to

==========
VideoCapture: migrate Host-->Renderer OnStateUpdate message

This CL migrates the last video capture message renderer-->host
(wrongly named BufferReady, renamed to ReleaseBuffer since it's
an instruction/command). This allows for a few extra
simplifications in unit tests.

Also it begins the migration of messages from Host back
to Renderer. A mojom::VideoCaptureObserver is added,
implemented in VideoCaptureImpl via a binding valid between
Start() and Stop().   The host-->renderer OnStateUpdate
message is migrated to mojom.

BUG=651897

Committed: https://crrev.com/32a8820f9924f0f6033d779a40c7dd103afb83e9
Cr-Commit-Position: refs/heads/master@{#424339}
==========

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

Patchset 2 (id:??) landed as
https://crrev.com/32a8820f9924f0f6033d779a40c7dd103afb83e9
Cr-Commit-Position: refs/heads/master@{#424339}