VideoCapture: more migration IPC-->mojo, part 6

VideoCapture: more migration IPC-->mojo, part 6

This CL does the migration of the last Vid Capture IPC
message to mojo, namely OnBufferCreated().  This
message shared a SharedMemoryHandle, which is now
send using mojo::{Wrap,Unwrap}SharedMemoryHandle().
This needs a few small changes elsewhere in the code,
in particular there is no need to ShareToProcess() a given
ShMem (and this in turn makes us think that the whole
data-sending can be simplified in later CLs).

Following suggestion in PS1, VideoCaptureBufferTracker()
is changed to
mojo::ScopedSharedBufferHandle GetHandleForTransit();,
which causes changes in a few other classes;
- SharedMemoryBufferTracker, MojoSharedMemoryBufferTracker
- VideoCaptureBufferPool & Impl,
- VideoCaptureController (no need for render process id).
(all these changes are in services/video_capture/ and
content/browser/renderer_host/media/)

VideoCaptureHost is still a BrowserMessageFilter,
and VideoCaptureMessageFilter is not completely
gone yet, but will disappear soon -- I didn't want to
mix the changes in the ShMem management with
other changes just in case.

BUG=651897
TEST=local capture, {content,capture,video_capture}_unittests

Committed: https://crrev.com/0abf0b0b42e9e974d78b6f13a2ed81afdd668287
Cr-Commit-Position: refs/heads/master@{#426500}

[mcasas, 2016-10-14 00:16:04]

Patchset #1 (id:1) has been deleted

[mcasas, 2016-10-18 18:42:13]

Description was changed from

==========
VideoCapture: more migration IPC-->mojo, part 6

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

to

==========
VideoCapture: more migration IPC-->mojo, part 6

This CL does the migration of the last Vid Capture IPC
message to mojo, namely OnBufferCreated().  This
message shared a SharedMemoryHandle, which is now
send using mojo::{Wrap,Unwrap}SharedMemoryHandle().
This needs a few small changes elsewhere in the code,
in particular there is no need to ShareToProcess() a given
ShMem (and this in turn makes us think that the whole
data-sending can be simplified in later CLs).

VideoCaptureHost is still a BrowserMessageFilter,
and VideoCaptureMessageFilter is not completely
gone yet, but will disappear soon -- I didn't want to
mix the changes in the ShMem management with
other changes just in case.

BUG=651897
TEST=local capture, unittests of multiple conditions.
==========

[mcasas, 2016-10-18 18:42:20]

Patchset #1 (id:20001) has been deleted

[mcasas, 2016-10-18 18:42:29]

Patchset #1 (id:40001) has been deleted

[mcasas, 2016-10-18 18:45:13]

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

[mcasas, 2016-10-18 18:45:13]

rockot@ PTAL at the mojo parts, and
RS the mojo/ added export.

ncarter@ RS mojo/messages plz
(tsepez@ is OOO but I believe he agrees with this work
since he has approved the previous CLs, and also I'm
killing your TODO  ;)  )

[mcasas, 2016-10-18 18:55:21]

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

[commit-bot: I haz the power, 2016-10-18 18:55:30]

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-18 19:57:46]

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

[commit-bot: I haz the power, 2016-10-18 19:57:47]

Dry run: This issue passed the CQ dry run.

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

Looks great, just one gripe about shared memory handle usage

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

https://codereview.chromium.org/2410383002/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/shared_memory_buffer_tracker.cc:41:
*new_handle = base::SharedMemory::DuplicateHandle(shared_memory_.handle());
So it looks like ShareToProcess is ultimately only called
VideoCaptureBufferPoolImpl::ShareToProcess, which in turn is only called from
VideoCaptureController::DoNewBufferOnIOThread, and then you're only using
new_handle for Mojo IPC anyway.

Given that is is not actually "sharing" to a process anymore, and
SharedMemoryHandle lifetime management is really not clear (it does -not- own
the buffer object, so AFAICT this code is leaky if you don't wrap and send
new_handle somewhere), and the resulting handle is only used for Mojo IPC, can
you please just change SharedMemoryBufferTracker::ShareToProcess to something
better like:

  mojo::ScopedSharedBufferHandle
SharedBufferMemoryTracker::GetHandleForTransit() {
    return mojo::WrapSharedMemoryHandle(
        base::SharedMemory::DuplicateHandle(shared_memory_.handle()),
        ...);
  }

You no longer need a ProcessHandle argument, you can propagate the simpler
interface to VideoCaptureBufferPool, and you can use a safer scoped buffer
handle type provided by Mojo.

[ncarter (slow), 2016-10-19 17:31:20]

nick@chromium.org changed reviewers:
+ nick@chromium.org
- ncarter@chromium.org

[mcasas, 2016-10-19 17:38:06]

Description was changed from

==========
VideoCapture: more migration IPC-->mojo, part 6

This CL does the migration of the last Vid Capture IPC
message to mojo, namely OnBufferCreated().  This
message shared a SharedMemoryHandle, which is now
send using mojo::{Wrap,Unwrap}SharedMemoryHandle().
This needs a few small changes elsewhere in the code,
in particular there is no need to ShareToProcess() a given
ShMem (and this in turn makes us think that the whole
data-sending can be simplified in later CLs).

VideoCaptureHost is still a BrowserMessageFilter,
and VideoCaptureMessageFilter is not completely
gone yet, but will disappear soon -- I didn't want to
mix the changes in the ShMem management with
other changes just in case.

BUG=651897
TEST=local capture, unittests of multiple conditions.
==========

to

==========
VideoCapture: more migration IPC-->mojo, part 6

This CL does the migration of the last Vid Capture IPC
message to mojo, namely OnBufferCreated().  This
message shared a SharedMemoryHandle, which is now
send using mojo::{Wrap,Unwrap}SharedMemoryHandle().
This needs a few small changes elsewhere in the code,
in particular there is no need to ShareToProcess() a given
ShMem (and this in turn makes us think that the whole
data-sending can be simplified in later CLs).

Following suggestion in PS1, VideoCaptureBufferTracker()
is changed to 
mojo::ScopedSharedBufferHandle GetHandleForTransit();,
which causes changes in a few other classes;
- SharedMemoryBufferTracker, MojoSharedMemoryBufferTracker
- VideoCaptureBufferPool & Impl, 
- VideoCaptureController (no need for render process id).


VideoCaptureHost is still a BrowserMessageFilter,
and VideoCaptureMessageFilter is not completely
gone yet, but will disappear soon -- I didn't want to
mix the changes in the ShMem management with
other changes just in case.

BUG=651897
TEST=local capture, unittests of multiple conditions.
==========

[mcasas, 2016-10-19 17:46:09]

Description was changed from

==========
VideoCapture: more migration IPC-->mojo, part 6

This CL does the migration of the last Vid Capture IPC
message to mojo, namely OnBufferCreated().  This
message shared a SharedMemoryHandle, which is now
send using mojo::{Wrap,Unwrap}SharedMemoryHandle().
This needs a few small changes elsewhere in the code,
in particular there is no need to ShareToProcess() a given
ShMem (and this in turn makes us think that the whole
data-sending can be simplified in later CLs).

Following suggestion in PS1, VideoCaptureBufferTracker()
is changed to 
mojo::ScopedSharedBufferHandle GetHandleForTransit();,
which causes changes in a few other classes;
- SharedMemoryBufferTracker, MojoSharedMemoryBufferTracker
- VideoCaptureBufferPool & Impl, 
- VideoCaptureController (no need for render process id).


VideoCaptureHost is still a BrowserMessageFilter,
and VideoCaptureMessageFilter is not completely
gone yet, but will disappear soon -- I didn't want to
mix the changes in the ShMem management with
other changes just in case.

BUG=651897
TEST=local capture, unittests of multiple conditions.
==========

to

==========
VideoCapture: more migration IPC-->mojo, part 6

This CL does the migration of the last Vid Capture IPC
message to mojo, namely OnBufferCreated().  This
message shared a SharedMemoryHandle, which is now
send using mojo::{Wrap,Unwrap}SharedMemoryHandle().
This needs a few small changes elsewhere in the code,
in particular there is no need to ShareToProcess() a given
ShMem (and this in turn makes us think that the whole
data-sending can be simplified in later CLs).

Following suggestion in PS1, VideoCaptureBufferTracker()
is changed to 
mojo::ScopedSharedBufferHandle GetHandleForTransit();,
which causes changes in a few other classes;
- SharedMemoryBufferTracker, MojoSharedMemoryBufferTracker
- VideoCaptureBufferPool & Impl, 
- VideoCaptureController (no need for render process id).
(all these changes are in services/video_capture/ and
content/browser/renderer_host/media/)

VideoCaptureHost is still a BrowserMessageFilter,
and VideoCaptureMessageFilter is not completely
gone yet, but will disappear soon -- I didn't want to
mix the changes in the ShMem management with
other changes just in case.

BUG=651897
TEST=local capture, unittests of multiple conditions.
==========

[mcasas, 2016-10-19 17:47:02]

Description was changed from

==========
VideoCapture: more migration IPC-->mojo, part 6

This CL does the migration of the last Vid Capture IPC
message to mojo, namely OnBufferCreated().  This
message shared a SharedMemoryHandle, which is now
send using mojo::{Wrap,Unwrap}SharedMemoryHandle().
This needs a few small changes elsewhere in the code,
in particular there is no need to ShareToProcess() a given
ShMem (and this in turn makes us think that the whole
data-sending can be simplified in later CLs).

Following suggestion in PS1, VideoCaptureBufferTracker()
is changed to 
mojo::ScopedSharedBufferHandle GetHandleForTransit();,
which causes changes in a few other classes;
- SharedMemoryBufferTracker, MojoSharedMemoryBufferTracker
- VideoCaptureBufferPool & Impl, 
- VideoCaptureController (no need for render process id).
(all these changes are in services/video_capture/ and
content/browser/renderer_host/media/)

VideoCaptureHost is still a BrowserMessageFilter,
and VideoCaptureMessageFilter is not completely
gone yet, but will disappear soon -- I didn't want to
mix the changes in the ShMem management with
other changes just in case.

BUG=651897
TEST=local capture, unittests of multiple conditions.
==========

to

==========
VideoCapture: more migration IPC-->mojo, part 6

This CL does the migration of the last Vid Capture IPC
message to mojo, namely OnBufferCreated().  This
message shared a SharedMemoryHandle, which is now
send using mojo::{Wrap,Unwrap}SharedMemoryHandle().
This needs a few small changes elsewhere in the code,
in particular there is no need to ShareToProcess() a given
ShMem (and this in turn makes us think that the whole
data-sending can be simplified in later CLs).

Following suggestion in PS1, VideoCaptureBufferTracker()
is changed to 
mojo::ScopedSharedBufferHandle GetHandleForTransit();,
which causes changes in a few other classes;
- SharedMemoryBufferTracker, MojoSharedMemoryBufferTracker
- VideoCaptureBufferPool & Impl, 
- VideoCaptureController (no need for render process id).
(all these changes are in services/video_capture/ and
content/browser/renderer_host/media/)

VideoCaptureHost is still a BrowserMessageFilter,
and VideoCaptureMessageFilter is not completely
gone yet, but will disappear soon -- I didn't want to
mix the changes in the ShMem management with
other changes just in case.

BUG=651897
TEST=local capture, {content,capture,video_capture}_unittests
==========

[mcasas, 2016-10-19 17:53:30]

guys PTAL

(description updated as well)

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

https://codereview.chromium.org/2410383002/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/shared_memory_buffer_tracker.cc:41:
*new_handle = base::SharedMemory::DuplicateHandle(shared_memory_.handle());
On 2016/10/19 05:24:18, Ken Rockot wrote:
> So it looks like ShareToProcess is ultimately only called
> VideoCaptureBufferPoolImpl::ShareToProcess, which in turn is only called from
> VideoCaptureController::DoNewBufferOnIOThread, and then you're only using
> new_handle for Mojo IPC anyway.
> 
> Given that is is not actually "sharing" to a process anymore, and
> SharedMemoryHandle lifetime management is really not clear (it does -not- own
> the buffer object, so AFAICT this code is leaky if you don't wrap and send
> new_handle somewhere), and the resulting handle is only used for Mojo IPC, can
> you please just change SharedMemoryBufferTracker::ShareToProcess to something
> better like:
> 
>   mojo::ScopedSharedBufferHandle
> SharedBufferMemoryTracker::GetHandleForTransit() {
>     return mojo::WrapSharedMemoryHandle(
>         base::SharedMemory::DuplicateHandle(shared_memory_.handle()),
>         ...);
>   }
> 
> You no longer need a ProcessHandle argument, you can propagate the simpler
> interface to VideoCaptureBufferPool, and you can use a safer scoped buffer
> handle type provided by Mojo.

All right! I was a bit shy to dig deeper in this CL 
because the changes will spread across a few more
files but your suggestion tipped the balance.
Done.

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

lgtm

[ncarter (slow), 2016-10-19 18:58:21]

https://codereview.chromium.org/2410383002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_host.h (right):

https://codereview.chromium.org/2410383002/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_host.h:25: // This class is
essentially a super adapter between several different areas. It
This comment might be better if the first sentence more effectively summarized
the rest of the comment block.

Here's my probably-not-great draft: "VideoCaptureHost is the IO thread browser
process communication endpoint between a renderer process (which can initiate
and receive a video capture stream) and a VideoCaptureController in the browser
process (which provides the stream from an video device)."

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

https://codereview.chromium.org/2410383002/diff/80001/content/common/media/vi...
content/common/media/video_capture_messages.h:12:
IPC_MESSAGE_CONTROL0(VideoCaptureMsg_Noop)
Sounds like this is going away soon, but it wouldn't hurt to put a // TODO()
here about why we need a noop message, and that it's temporary. Sometimes
temporary things live a longer life than we expect.

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

https://codereview.chromium.org/2410383002/diff/80001/content/common/video_ca...
content/common/video_capture.mojom:38: //      | -------- ReleaseBuffer(1)
--------->   |
This might be easier to look at with more whitespace for visual rhythm, and with
the numbers from each side of the communication are vertically aligned, as if in
a table. Maybe like:

 //      |                       <-- OnBufferReady(1) |
 //      |                       <-- OnBufferReady(2) |
 //      | --> ReleaseBuffer(1)                       |
 //      |                       <-- OnBufferReady(1) |
 //      | --> ReleaseBuffer(2)                       |

[mcasas, 2016-10-19 19:29:52]

nick@ PTAL

https://codereview.chromium.org/2410383002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_host.h (right):

https://codereview.chromium.org/2410383002/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_host.h:25: // This class is
essentially a super adapter between several different areas. It
On 2016/10/19 18:58:21, ncarter wrote:
> This comment might be better if the first sentence more effectively summarized
> the rest of the comment block.
> 
> Here's my probably-not-great draft: "VideoCaptureHost is the IO thread browser
> process communication endpoint between a renderer process (which can initiate
> and receive a video capture stream) and a VideoCaptureController in the
browser
> process (which provides the stream from an video device)."

Done.

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

https://codereview.chromium.org/2410383002/diff/80001/content/common/media/vi...
content/common/media/video_capture_messages.h:12:
IPC_MESSAGE_CONTROL0(VideoCaptureMsg_Noop)
On 2016/10/19 18:58:21, ncarter wrote:
> Sounds like this is going away soon, but it wouldn't hurt to put a // TODO()
> here about why we need a noop message, and that it's temporary. Sometimes
> temporary things live a longer life than we expect.

Indeed! Done.

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

https://codereview.chromium.org/2410383002/diff/80001/content/common/video_ca...
content/common/video_capture.mojom:38: //      | -------- ReleaseBuffer(1)
--------->   |
On 2016/10/19 18:58:21, ncarter wrote:
> This might be easier to look at with more whitespace for visual rhythm, and
with
> the numbers from each side of the communication are vertically aligned, as if
in
> a table. Maybe like:
> 
>  //      |                       <-- OnBufferReady(1) |
>  //      |                       <-- OnBufferReady(2) |
>  //      | --> ReleaseBuffer(1)                       |
>  //      |                       <-- OnBufferReady(1) |
>  //      | --> ReleaseBuffer(2)                       |

Good suggestion. Done.

[ncarter (slow), 2016-10-19 20:36:54]

lgtm

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

https://codereview.chromium.org/2410383002/diff/100001/content/common/video_c...
content/common/video_capture.mojom:45: //      | ---> ReleaseBuffer(1)          
                      |
should be ReleaseBuffer(2)

[mcasas, 2016-10-19 21:30:59]

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

https://codereview.chromium.org/2410383002/diff/100001/content/common/video_c...
content/common/video_capture.mojom:45: //      | ---> ReleaseBuffer(1)          
                      |
On 2016/10/19 20:36:54, ncarter wrote:
> should be ReleaseBuffer(2)

Done.

[mcasas, 2016-10-19 21:31:05]

The CQ bit was checked by mcasas@chromium.org

[mcasas, 2016-10-19 21:31:05]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
nick@chromium.org
Link to the patchset: https://codereview.chromium.org/2410383002/#ps120001
(title: "Comment correction")

[commit-bot: I haz the power, 2016-10-19 21:32:07]

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-19 22:19:10]

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

[commit-bot: I haz the power, 2016-10-19 22:19:11]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[mcasas, 2016-10-19 23:27:49]

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

[mcasas, 2016-10-19 23:27:49]

dcheng@ plz RS mojom/messages.h and/or PTAL

[dcheng, 2016-10-20 08:37:56]

mojo lgtm

i wonder if we can get rid of buffer ID altogether in the future? i guess that'd
be tricky in this case without wrapping each one in an additional interface...

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

https://codereview.chromium.org/2410383002/diff/120001/content/browser/render...
content/browser/renderer_host/media/video_capture_controller.cc:358:
NOTREACHED();
Incidentally, I'm wondering if things like this should be
mojo::ReportBadMessage?

https://codereview.chromium.org/2410383002/diff/120001/services/video_capture...
File services/video_capture/mojo_shared_memory_buffer_tracker.cc (right):

https://codereview.chromium.org/2410383002/diff/120001/services/video_capture...
services/video_capture/mojo_shared_memory_buffer_tracker.cc:47: return
mojo::ScopedSharedBufferHandle();
I'm not entirely sure I understand the purpose of this class. Is this a
NOTREACHED() kind of thing?

(Perhaps a class-level comment could be useful)

[mcasas, 2016-10-20 15:09:51]

Re. 
"i wonder if we can get rid of buffer ID altogether in the 
future? i guess that'd be tricky in this case without 
wrapping each one in an additional interface..."

The whole communication preamble, pre-sharing the
buffers, was added relatively recently because of the
added cost of doing a Map() for every received buffer,
but this introduced the buffer id concept.  I think we 
should evaluate it vs mojo Data Pipes and let the best
win, but this'll happen during https://crbug.com/584797
which is a successor to the present bug.

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

https://codereview.chromium.org/2410383002/diff/120001/content/browser/render...
content/browser/renderer_host/media/video_capture_controller.cc:358:
NOTREACHED();
On 2016/10/20 08:37:56, dcheng wrote:
> Incidentally, I'm wondering if things like this should be
> mojo::ReportBadMessage?

Oh, that's a good one. I'll add it in the next patch
in VideoCaptureHost, who is the one actually
receiving the messages.

https://codereview.chromium.org/2410383002/diff/120001/services/video_capture...
File services/video_capture/mojo_shared_memory_buffer_tracker.cc (right):

https://codereview.chromium.org/2410383002/diff/120001/services/video_capture...
services/video_capture/mojo_shared_memory_buffer_tracker.cc:47: return
mojo::ScopedSharedBufferHandle();
On 2016/10/20 08:37:56, dcheng wrote:
> I'm not entirely sure I understand the purpose of this class. Is this a
> NOTREACHED() kind of thing?
> 
> (Perhaps a class-level comment could be useful)

//services/video_capture is still under development [1],
but when it's ready, we should consider better mojo
ways to share data from service impl to client, e.g.
a mojo DataPipe, but for the time being it needs to
comply to the VideoCaptureBufferTracker, which 
was designed in a pre-mojo world.

[1] https://crbug.com/584797

[mcasas, 2016-10-20 15:10:07]

The CQ bit was checked by mcasas@chromium.org

[commit-bot: I haz the power, 2016-10-20 15:10:31]

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

[Ken Rockot(use gerrit already), 2016-10-20 15:23:15]

On Oct 20, 2016 8:09 AM, <mcasas@chromium.org> wrote:
>
> Re.
> "i wonder if we can get rid of buffer ID altogether in the
> future? i guess that'd be tricky in this case without
> wrapping each one in an additional interface..."
>
> The whole communication preamble, pre-sharing the
> buffers, was added relatively recently because of the
> added cost of doing a Map() for every received buffer,
> but this introduced the buffer id concept. I think we
> should evaluate it vs mojo Data Pipes and let the best
> win, but this'll happen during https://crbug.com/584797
> which is a successor to the present bug.
>

I agree it's good to defer this for now, but data pipes do nicely solve
this exact use case, i.e., a rotation of fixed-size shared buffers.

>
>
>
>
https://codereview.chromium.org/2410383002/diff/120001/content/browser/render...
> File content/browser/renderer_host/media/video_capture_controller.cc
> (right):
>
>
https://codereview.chromium.org/2410383002/diff/120001/content/browser/render...
> content/browser/renderer_host/media/video_capture_controller.cc:358:
> NOTREACHED();
> On 2016/10/20 08:37:56, dcheng wrote:
> > Incidentally, I'm wondering if things like this should be
> > mojo::ReportBadMessage?
>
> Oh, that's a good one. I'll add it in the next patch
> in VideoCaptureHost, who is the one actually
> receiving the messages.
>
>
>
https://codereview.chromium.org/2410383002/diff/120001/services/video_capture...
> File services/video_capture/mojo_shared_memory_buffer_tracker.cc
> (right):
>
>
https://codereview.chromium.org/2410383002/diff/120001/services/video_capture...
> services/video_capture/mojo_shared_memory_buffer_tracker.cc:47: return
> mojo::ScopedSharedBufferHandle();
> On 2016/10/20 08:37:56, dcheng wrote:
> > I'm not entirely sure I understand the purpose of this class. Is this
> a
> > NOTREACHED() kind of thing?
> >
> > (Perhaps a class-level comment could be useful)
>
> //services/video_capture is still under development [1],
> but when it's ready, we should consider better mojo
> ways to share data from service impl to client, e.g.
> a mojo DataPipe, but for the time being it needs to
> comply to the VideoCaptureBufferTracker, which
> was designed in a pre-mojo world.
>
> [1] https://crbug.com/584797
>
> https://codereview.chromium.org/2410383002/

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

[Tom Sepez, 2016-10-20 16:44:16]

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

[Tom Sepez, 2016-10-20 16:44:36]

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

[Tom Sepez, 2016-10-20 16:44:36]

Deferring to dcheng, removing myself as reviewer.

[commit-bot: I haz the power, 2016-10-20 16:46:54]

Description was changed from

==========
VideoCapture: more migration IPC-->mojo, part 6

This CL does the migration of the last Vid Capture IPC
message to mojo, namely OnBufferCreated().  This
message shared a SharedMemoryHandle, which is now
send using mojo::{Wrap,Unwrap}SharedMemoryHandle().
This needs a few small changes elsewhere in the code,
in particular there is no need to ShareToProcess() a given
ShMem (and this in turn makes us think that the whole
data-sending can be simplified in later CLs).

Following suggestion in PS1, VideoCaptureBufferTracker()
is changed to 
mojo::ScopedSharedBufferHandle GetHandleForTransit();,
which causes changes in a few other classes;
- SharedMemoryBufferTracker, MojoSharedMemoryBufferTracker
- VideoCaptureBufferPool & Impl, 
- VideoCaptureController (no need for render process id).
(all these changes are in services/video_capture/ and
content/browser/renderer_host/media/)

VideoCaptureHost is still a BrowserMessageFilter,
and VideoCaptureMessageFilter is not completely
gone yet, but will disappear soon -- I didn't want to
mix the changes in the ShMem management with
other changes just in case.

BUG=651897
TEST=local capture, {content,capture,video_capture}_unittests
==========

to

==========
VideoCapture: more migration IPC-->mojo, part 6

This CL does the migration of the last Vid Capture IPC
message to mojo, namely OnBufferCreated().  This
message shared a SharedMemoryHandle, which is now
send using mojo::{Wrap,Unwrap}SharedMemoryHandle().
This needs a few small changes elsewhere in the code,
in particular there is no need to ShareToProcess() a given
ShMem (and this in turn makes us think that the whole
data-sending can be simplified in later CLs).

Following suggestion in PS1, VideoCaptureBufferTracker()
is changed to 
mojo::ScopedSharedBufferHandle GetHandleForTransit();,
which causes changes in a few other classes;
- SharedMemoryBufferTracker, MojoSharedMemoryBufferTracker
- VideoCaptureBufferPool & Impl, 
- VideoCaptureController (no need for render process id).
(all these changes are in services/video_capture/ and
content/browser/renderer_host/media/)

VideoCaptureHost is still a BrowserMessageFilter,
and VideoCaptureMessageFilter is not completely
gone yet, but will disappear soon -- I didn't want to
mix the changes in the ShMem management with
other changes just in case.

BUG=651897
TEST=local capture, {content,capture,video_capture}_unittests
==========

[commit-bot: I haz the power, 2016-10-20 16:46:56]

Committed patchset #4 (id:120001)

[commit-bot: I haz the power, 2016-10-21 13:19:02]

Description was changed from

==========
VideoCapture: more migration IPC-->mojo, part 6

This CL does the migration of the last Vid Capture IPC
message to mojo, namely OnBufferCreated().  This
message shared a SharedMemoryHandle, which is now
send using mojo::{Wrap,Unwrap}SharedMemoryHandle().
This needs a few small changes elsewhere in the code,
in particular there is no need to ShareToProcess() a given
ShMem (and this in turn makes us think that the whole
data-sending can be simplified in later CLs).

Following suggestion in PS1, VideoCaptureBufferTracker()
is changed to 
mojo::ScopedSharedBufferHandle GetHandleForTransit();,
which causes changes in a few other classes;
- SharedMemoryBufferTracker, MojoSharedMemoryBufferTracker
- VideoCaptureBufferPool & Impl, 
- VideoCaptureController (no need for render process id).
(all these changes are in services/video_capture/ and
content/browser/renderer_host/media/)

VideoCaptureHost is still a BrowserMessageFilter,
and VideoCaptureMessageFilter is not completely
gone yet, but will disappear soon -- I didn't want to
mix the changes in the ShMem management with
other changes just in case.

BUG=651897
TEST=local capture, {content,capture,video_capture}_unittests
==========

to

==========
VideoCapture: more migration IPC-->mojo, part 6

This CL does the migration of the last Vid Capture IPC
message to mojo, namely OnBufferCreated().  This
message shared a SharedMemoryHandle, which is now
send using mojo::{Wrap,Unwrap}SharedMemoryHandle().
This needs a few small changes elsewhere in the code,
in particular there is no need to ShareToProcess() a given
ShMem (and this in turn makes us think that the whole
data-sending can be simplified in later CLs).

Following suggestion in PS1, VideoCaptureBufferTracker()
is changed to
mojo::ScopedSharedBufferHandle GetHandleForTransit();,
which causes changes in a few other classes;
- SharedMemoryBufferTracker, MojoSharedMemoryBufferTracker
- VideoCaptureBufferPool & Impl,
- VideoCaptureController (no need for render process id).
(all these changes are in services/video_capture/ and
content/browser/renderer_host/media/)

VideoCaptureHost is still a BrowserMessageFilter,
and VideoCaptureMessageFilter is not completely
gone yet, but will disappear soon -- I didn't want to
mix the changes in the ShMem management with
other changes just in case.

BUG=651897
TEST=local capture, {content,capture,video_capture}_unittests

Committed: https://crrev.com/0abf0b0b42e9e974d78b6f13a2ed81afdd668287
Cr-Commit-Position: refs/heads/master@{#426500}
==========

[commit-bot: I haz the power, 2016-10-21 13:19:03]

Patchset 4 (id:??) landed as
https://crrev.com/0abf0b0b42e9e974d78b6f13a2ed81afdd668287
Cr-Commit-Position: refs/heads/master@{#426500}