New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 651897 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug

Blocking:
issue 651937



Sign in to add a comment

migrate video_capture_messages.h to mojo

Project Member Reported by mcasas@chromium.org, Sep 30 2016

Issue description

Interchanged between VideoCaptureMessageFilter+VideoCaptureImpl (renderer)
and VideoCaptureHost (in browser/renderer_host).

https://cs.chromium.org/chromium/src/content/common/media/video_capture_messages.h?q=video_capture_messages&sq=package:chromium&dr
 

Comment 1 by mcasas@chromium.org, Sep 30 2016

Blocking: 651937
Project Member

Comment 2 by bugdroid1@chromium.org, Oct 3 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a2fc9a109f9280ca953e18a5f82c253ded6ddc26

commit a2fc9a109f9280ca953e18a5f82c253ded6ddc26
Author: mcasas <mcasas@chromium.org>
Date: Mon Oct 03 21:16:59 2016

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.

Review-Url: https://codereview.chromium.org/2384843002
Cr-Commit-Position: refs/heads/master@{#422534}

[modify] https://crrev.com/a2fc9a109f9280ca953e18a5f82c253ded6ddc26/content/browser/renderer_host/media/video_capture_host.cc
[modify] https://crrev.com/a2fc9a109f9280ca953e18a5f82c253ded6ddc26/content/browser/renderer_host/media/video_capture_host.h
[modify] https://crrev.com/a2fc9a109f9280ca953e18a5f82c253ded6ddc26/content/browser/renderer_host/media/video_capture_host_unittest.cc
[modify] https://crrev.com/a2fc9a109f9280ca953e18a5f82c253ded6ddc26/content/common/BUILD.gn
[modify] https://crrev.com/a2fc9a109f9280ca953e18a5f82c253ded6ddc26/content/common/media/video_capture_messages.h
[add] https://crrev.com/a2fc9a109f9280ca953e18a5f82c253ded6ddc26/content/common/video_capture.mojom

Project Member

Comment 3 by bugdroid1@chromium.org, Oct 4 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6504ef716dbf9d2bf75689960ebccf3160b8a676

commit 6504ef716dbf9d2bf75689960ebccf3160b8a676
Author: mcasas <mcasas@chromium.org>
Date: Tue Oct 04 00:55:46 2016

Revert of VideoCapture: migrate VideoCapture renderer-->host messages to mojo, part 1 (patchset #1 id:20001 of https://codereview.chromium.org/2384843002/ )

Reason for revert:
Ouch! Messages are called Stop, etc
but the mojom calls them StopCapture !
==> the new messages are not being
called :(
Reverting to reland soon.

Original issue's 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/a2fc9a109f9280ca953e18a5f82c253ded6ddc26
> Cr-Commit-Position: refs/heads/master@{#422534}

TBR=rockot@chromium.org,tibell@chromium.org,chfremer@chromium.org,tsepez@chromium.org,avi@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 651897 

Review-Url: https://codereview.chromium.org/2393483002
Cr-Commit-Position: refs/heads/master@{#422643}

[modify] https://crrev.com/6504ef716dbf9d2bf75689960ebccf3160b8a676/content/browser/renderer_host/media/video_capture_host.cc
[modify] https://crrev.com/6504ef716dbf9d2bf75689960ebccf3160b8a676/content/browser/renderer_host/media/video_capture_host.h
[modify] https://crrev.com/6504ef716dbf9d2bf75689960ebccf3160b8a676/content/browser/renderer_host/media/video_capture_host_unittest.cc
[modify] https://crrev.com/6504ef716dbf9d2bf75689960ebccf3160b8a676/content/common/BUILD.gn
[modify] https://crrev.com/6504ef716dbf9d2bf75689960ebccf3160b8a676/content/common/media/video_capture_messages.h
[delete] https://crrev.com/34b14f3281e01845d9ef7cf5512855a417798370/content/common/video_capture.mojom

Project Member

Comment 5 by bugdroid1@chromium.org, Oct 5 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fc81eb705628fbf68ee9b714c1a1a979b42c623c

commit fc81eb705628fbf68ee9b714c1a1a979b42c623c
Author: mcasas <mcasas@chromium.org>
Date: Wed Oct 05 01:45:20 2016

Video Capture Service: typemap video_capture's and media's VideoCaptureFormat

This CL introduces type mapping (via StructTraits) between
video_capture::mojom::VideoCaptureFormatDataView and
media::VideoCaptureFormat and adapts the code in
services/video_capture accordingly. This is needed to
progress in the migration of Video Capture messages between
renderer and browser being tracked in the mentioned bug.

BUG= 651897 ,  642387 
TEST= video_capture_unittests working.

Review-Url: https://codereview.chromium.org/2379253003
Cr-Commit-Position: refs/heads/master@{#423046}

[modify] https://crrev.com/fc81eb705628fbf68ee9b714c1a1a979b42c623c/mojo/public/tools/bindings/chromium_bindings_configuration.gni
[modify] https://crrev.com/fc81eb705628fbf68ee9b714c1a1a979b42c623c/services/video_capture/DEPS
[modify] https://crrev.com/fc81eb705628fbf68ee9b714c1a1a979b42c623c/services/video_capture/fake_device_descriptor_unittest.cc
[modify] https://crrev.com/fc81eb705628fbf68ee9b714c1a1a979b42c623c/services/video_capture/fake_device_unittest.cc
[modify] https://crrev.com/fc81eb705628fbf68ee9b714c1a1a979b42c623c/services/video_capture/mock_device_video_capture_service_test.cc
[modify] https://crrev.com/fc81eb705628fbf68ee9b714c1a1a979b42c623c/services/video_capture/mock_device_video_capture_service_test.h
[modify] https://crrev.com/fc81eb705628fbf68ee9b714c1a1a979b42c623c/services/video_capture/mock_device_video_capture_service_unittest.cc
[modify] https://crrev.com/fc81eb705628fbf68ee9b714c1a1a979b42c623c/services/video_capture/mojo_media_conversions.cc
[modify] https://crrev.com/fc81eb705628fbf68ee9b714c1a1a979b42c623c/services/video_capture/mojo_media_conversions.h
[add] https://crrev.com/fc81eb705628fbf68ee9b714c1a1a979b42c623c/services/video_capture/public/interfaces/typemaps.gni
[add] https://crrev.com/fc81eb705628fbf68ee9b714c1a1a979b42c623c/services/video_capture/public/interfaces/video_capture_format.typemap
[add] https://crrev.com/fc81eb705628fbf68ee9b714c1a1a979b42c623c/services/video_capture/public/interfaces/video_capture_format_traits.cc
[add] https://crrev.com/fc81eb705628fbf68ee9b714c1a1a979b42c623c/services/video_capture/public/interfaces/video_capture_format_traits.h
[modify] https://crrev.com/fc81eb705628fbf68ee9b714c1a1a979b42c623c/services/video_capture/video_capture_device_proxy_impl.cc
[modify] https://crrev.com/fc81eb705628fbf68ee9b714c1a1a979b42c623c/services/video_capture/video_capture_device_proxy_impl.h

Project Member

Comment 6 by bugdroid1@chromium.org, Oct 6 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/796b2e6cd3c150f29460c082fcc60ee2186ea9ab

commit 796b2e6cd3c150f29460c082fcc60ee2186ea9ab
Author: mcasas <mcasas@chromium.org>
Date: Thu Oct 06 01:48:14 2016

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.

Review-Url: https://codereview.chromium.org/2390103002
Cr-Commit-Position: refs/heads/master@{#423388}

[modify] https://crrev.com/796b2e6cd3c150f29460c082fcc60ee2186ea9ab/content/browser/renderer_host/media/video_capture_host.cc
[modify] https://crrev.com/796b2e6cd3c150f29460c082fcc60ee2186ea9ab/content/browser/renderer_host/media/video_capture_host.h
[modify] https://crrev.com/796b2e6cd3c150f29460c082fcc60ee2186ea9ab/content/browser/renderer_host/media/video_capture_host_unittest.cc
[modify] https://crrev.com/796b2e6cd3c150f29460c082fcc60ee2186ea9ab/content/common/BUILD.gn
[modify] https://crrev.com/796b2e6cd3c150f29460c082fcc60ee2186ea9ab/content/common/media/video_capture_messages.h
[add] https://crrev.com/796b2e6cd3c150f29460c082fcc60ee2186ea9ab/content/common/video_capture.mojom
[modify] https://crrev.com/796b2e6cd3c150f29460c082fcc60ee2186ea9ab/content/public/app/mojo/content_browser_manifest.json
[modify] https://crrev.com/796b2e6cd3c150f29460c082fcc60ee2186ea9ab/content/renderer/media/video_capture_impl.cc
[modify] https://crrev.com/796b2e6cd3c150f29460c082fcc60ee2186ea9ab/content/renderer/media/video_capture_impl.h
[modify] https://crrev.com/796b2e6cd3c150f29460c082fcc60ee2186ea9ab/content/renderer/media/video_capture_impl_manager.cc
[modify] https://crrev.com/796b2e6cd3c150f29460c082fcc60ee2186ea9ab/content/renderer/media/video_capture_impl_manager_unittest.cc
[modify] https://crrev.com/796b2e6cd3c150f29460c082fcc60ee2186ea9ab/content/renderer/media/video_capture_impl_unittest.cc
[modify] https://crrev.com/796b2e6cd3c150f29460c082fcc60ee2186ea9ab/content/renderer/media/video_capture_message_filter.cc
[modify] https://crrev.com/796b2e6cd3c150f29460c082fcc60ee2186ea9ab/content/renderer/media/video_capture_message_filter.h

Project Member

Comment 7 by bugdroid1@chromium.org, Oct 7 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c3b336b680612cc73a94138df19bf3ad7096b856

commit c3b336b680612cc73a94138df19bf3ad7096b856
Author: mcasas <mcasas@chromium.org>
Date: Fri Oct 07 14:55:45 2016

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

This CL migrates video capture IPC messages Start and Resume
from IPC to mojom. It needs 2 EnumTraits (ResolutionChangePolicy
and PowerLineFrequency) in //services/video_capture, and a
StructTraits for VideoCaptureParams in //content.

The conversion of the IPC-related classes VideoCaptureHost and
VideoCaptureImpl is quite straightforward. The bulk of the CL
is unit tests adaptation (and cleanup!) and type renaming.

BUG= 651897 
TEST=content_unittests, content_browsertests, video_capture_unittests

Review-Url: https://codereview.chromium.org/2395163002
Cr-Commit-Position: refs/heads/master@{#423859}

[modify] https://crrev.com/c3b336b680612cc73a94138df19bf3ad7096b856/content/browser/renderer_host/media/video_capture_host.cc
[modify] https://crrev.com/c3b336b680612cc73a94138df19bf3ad7096b856/content/browser/renderer_host/media/video_capture_host.h
[modify] https://crrev.com/c3b336b680612cc73a94138df19bf3ad7096b856/content/browser/renderer_host/media/video_capture_host_unittest.cc
[modify] https://crrev.com/c3b336b680612cc73a94138df19bf3ad7096b856/content/common/BUILD.gn
[modify] https://crrev.com/c3b336b680612cc73a94138df19bf3ad7096b856/content/common/DEPS
[modify] https://crrev.com/c3b336b680612cc73a94138df19bf3ad7096b856/content/common/OWNERS
[modify] https://crrev.com/c3b336b680612cc73a94138df19bf3ad7096b856/content/common/media/video_capture_messages.h
[modify] https://crrev.com/c3b336b680612cc73a94138df19bf3ad7096b856/content/common/typemaps.gni
[modify] https://crrev.com/c3b336b680612cc73a94138df19bf3ad7096b856/content/common/video_capture.mojom
[add] https://crrev.com/c3b336b680612cc73a94138df19bf3ad7096b856/content/common/video_capture.typemap
[add] https://crrev.com/c3b336b680612cc73a94138df19bf3ad7096b856/content/common/video_capture_struct_traits.cc
[add] https://crrev.com/c3b336b680612cc73a94138df19bf3ad7096b856/content/common/video_capture_struct_traits.h
[modify] https://crrev.com/c3b336b680612cc73a94138df19bf3ad7096b856/content/renderer/media/video_capture_impl.cc
[modify] https://crrev.com/c3b336b680612cc73a94138df19bf3ad7096b856/content/renderer/media/video_capture_impl_manager_unittest.cc
[modify] https://crrev.com/c3b336b680612cc73a94138df19bf3ad7096b856/content/renderer/media/video_capture_impl_unittest.cc
[modify] https://crrev.com/c3b336b680612cc73a94138df19bf3ad7096b856/services/video_capture/BUILD.gn
[modify] https://crrev.com/c3b336b680612cc73a94138df19bf3ad7096b856/services/video_capture/fake_device_descriptor_unittest.cc
[modify] https://crrev.com/c3b336b680612cc73a94138df19bf3ad7096b856/services/video_capture/fake_device_unittest.cc
[modify] https://crrev.com/c3b336b680612cc73a94138df19bf3ad7096b856/services/video_capture/mock_device_video_capture_service_unittest.cc
[delete] https://crrev.com/83034cc5fb53f6d66f73f3a8afbfdd0d14b42943/services/video_capture/mojo_media_conversions.cc
[delete] https://crrev.com/83034cc5fb53f6d66f73f3a8afbfdd0d14b42943/services/video_capture/mojo_media_conversions.h
[modify] https://crrev.com/c3b336b680612cc73a94138df19bf3ad7096b856/services/video_capture/public/interfaces/typemaps.gni
[add] https://crrev.com/c3b336b680612cc73a94138df19bf3ad7096b856/services/video_capture/public/interfaces/video_capture_device_proxy.typemap
[add] https://crrev.com/c3b336b680612cc73a94138df19bf3ad7096b856/services/video_capture/public/interfaces/video_capture_device_proxy_traits.cc
[add] https://crrev.com/c3b336b680612cc73a94138df19bf3ad7096b856/services/video_capture/public/interfaces/video_capture_device_proxy_traits.h
[modify] https://crrev.com/c3b336b680612cc73a94138df19bf3ad7096b856/services/video_capture/video_capture_device_proxy_impl.cc
[modify] https://crrev.com/c3b336b680612cc73a94138df19bf3ad7096b856/services/video_capture/video_capture_device_proxy_impl.h

Project Member

Comment 8 by bugdroid1@chromium.org, Oct 7 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f81183123c82618825cd5734c5ace89fe1723d3c

commit f81183123c82618825cd5734c5ace89fe1723d3c
Author: mcasas <mcasas@chromium.org>
Date: Fri Oct 07 17:59:12 2016

VideoCaptureImpl: check that there's still a channel in ~VideoCaptureImpl() before sending

This was not hit when using IPC due to the channel ignoring
the late message, but now we need to protect against a
disappeared VideoCaptureHost explicitly.

Also, this CL  the unused |suspended_| variable
and simplified RemoveClient() method -- I know, I
know, I shouldn't bundle modifications in a CL...
but these were just too easy to let them pass.

BUG= 653806 ,  651897 

Review-Url: https://codereview.chromium.org/2396413002
Cr-Commit-Position: refs/heads/master@{#423907}

[modify] https://crrev.com/f81183123c82618825cd5734c5ace89fe1723d3c/content/renderer/media/video_capture_impl.cc
[modify] https://crrev.com/f81183123c82618825cd5734c5ace89fe1723d3c/content/renderer/media/video_capture_impl.h

Project Member

Comment 9 by bugdroid1@chromium.org, Oct 7 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1129536744aa7afa34fdf48725e9a0012afeaee6

commit 1129536744aa7afa34fdf48725e9a0012afeaee6
Author: mcasas <mcasas@chromium.org>
Date: Fri Oct 07 19:50:17 2016

VideoCapture: moar VideoCapture renderer-->host messages to mojo, part 3

This time is GetDeviceSupportedFormats and GetDeviceFormatsInUse,
which encompass a command-with-response that are bundled into a
mojo with result in a callback (which allows for simplifying a bit
the sender side in VideoCaptureImpl, as we don't need a list
of callbacks in-flight).

BUG= 651897 

Review-Url: https://codereview.chromium.org/2400443006
Cr-Commit-Position: refs/heads/master@{#423942}

[modify] https://crrev.com/1129536744aa7afa34fdf48725e9a0012afeaee6/content/browser/renderer_host/media/video_capture_host.cc
[modify] https://crrev.com/1129536744aa7afa34fdf48725e9a0012afeaee6/content/browser/renderer_host/media/video_capture_host.h
[modify] https://crrev.com/1129536744aa7afa34fdf48725e9a0012afeaee6/content/common/media/video_capture_messages.h
[modify] https://crrev.com/1129536744aa7afa34fdf48725e9a0012afeaee6/content/common/video_capture.mojom
[modify] https://crrev.com/1129536744aa7afa34fdf48725e9a0012afeaee6/content/renderer/media/video_capture_impl.cc
[modify] https://crrev.com/1129536744aa7afa34fdf48725e9a0012afeaee6/content/renderer/media/video_capture_impl.h
[modify] https://crrev.com/1129536744aa7afa34fdf48725e9a0012afeaee6/content/renderer/media/video_capture_impl_manager_unittest.cc
[modify] https://crrev.com/1129536744aa7afa34fdf48725e9a0012afeaee6/content/renderer/media/video_capture_impl_unittest.cc
[modify] https://crrev.com/1129536744aa7afa34fdf48725e9a0012afeaee6/content/renderer/media/video_capture_message_filter.cc
[modify] https://crrev.com/1129536744aa7afa34fdf48725e9a0012afeaee6/content/renderer/media/video_capture_message_filter.h
[modify] https://crrev.com/1129536744aa7afa34fdf48725e9a0012afeaee6/content/renderer/media/video_capture_message_filter_unittest.cc

Somehow the latest CL landed on Fri didn't hit this bug,
so for completion, I'm pasting it here:

commit	https://chromium.googlesource.com/chromium/src.git/+/fe85428bd742a61f78db005f980b15c219c4464d

author	mcasas <mcasas@chromium.org>	
Fri Oct 07 22:30:13 2016

VideoCaptureHostTest: simplify testing and remove unused code

This CL cleans up the VideoCaptureHostTest code:
- removes unused DUMP video support
- removes possibility of using real cameras (we
have content_browsertests and browser_tests for it)
- removes actual ShMem mapping and bookkeeping
in MockVideoCaptureHost, because is not needed
(we just need the ids).

BUG= 651897 ,  651937 
TEST=no new production code, just unittest cleanup.

Review-Url: https://codereview.chromium.org/2403533002
Cr-Commit-Position: refs/heads/master@{#423988}

content/browser/renderer_host/media/video_capture_host.cc[diff]
content/browser/renderer_host/media/video_capture_host_unittest.cc[diff]

Project Member

Comment 11 by bugdroid1@chromium.org, Oct 11 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/32a8820f9924f0f6033d779a40c7dd103afb83e9

commit 32a8820f9924f0f6033d779a40c7dd103afb83e9
Author: mcasas <mcasas@chromium.org>
Date: Tue Oct 11 02:02:52 2016

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 

Review-Url: https://codereview.chromium.org/2407623002
Cr-Commit-Position: refs/heads/master@{#424339}

[modify] https://crrev.com/32a8820f9924f0f6033d779a40c7dd103afb83e9/content/browser/renderer_host/media/video_capture_controller_event_handler.h
[modify] https://crrev.com/32a8820f9924f0f6033d779a40c7dd103afb83e9/content/browser/renderer_host/media/video_capture_host.cc
[modify] https://crrev.com/32a8820f9924f0f6033d779a40c7dd103afb83e9/content/browser/renderer_host/media/video_capture_host.h
[modify] https://crrev.com/32a8820f9924f0f6033d779a40c7dd103afb83e9/content/browser/renderer_host/media/video_capture_host_unittest.cc
[modify] https://crrev.com/32a8820f9924f0f6033d779a40c7dd103afb83e9/content/common/BUILD.gn
[modify] https://crrev.com/32a8820f9924f0f6033d779a40c7dd103afb83e9/content/common/media/video_capture_messages.h
[modify] https://crrev.com/32a8820f9924f0f6033d779a40c7dd103afb83e9/content/common/video_capture.mojom
[modify] https://crrev.com/32a8820f9924f0f6033d779a40c7dd103afb83e9/content/renderer/media/video_capture_impl.cc
[modify] https://crrev.com/32a8820f9924f0f6033d779a40c7dd103afb83e9/content/renderer/media/video_capture_impl.h
[modify] https://crrev.com/32a8820f9924f0f6033d779a40c7dd103afb83e9/content/renderer/media/video_capture_impl_manager_unittest.cc
[modify] https://crrev.com/32a8820f9924f0f6033d779a40c7dd103afb83e9/content/renderer/media/video_capture_impl_unittest.cc
[modify] https://crrev.com/32a8820f9924f0f6033d779a40c7dd103afb83e9/content/renderer/media/video_capture_message_filter.cc
[modify] https://crrev.com/32a8820f9924f0f6033d779a40c7dd103afb83e9/content/renderer/media/video_capture_message_filter.h
[modify] https://crrev.com/32a8820f9924f0f6033d779a40c7dd103afb83e9/content/renderer/media/video_capture_message_filter_unittest.cc

Project Member

Comment 12 by bugdroid1@chromium.org, Oct 11 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2ff39f629a774ee513cc69d831d78908a6953e9d

commit 2ff39f629a774ee513cc69d831d78908a6953e9d
Author: mcasas <mcasas@chromium.org>
Date: Tue Oct 11 20:00:46 2016

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

This CL migrates the video capture messages
VideoCaptureMsg_BufferReady and FreeBuffer
to mojom.

This step of the migration was quite straightforward,
most of the +- lines come from unittest adjustments,
and also I threw away some unused code in
video_capture_host_unittest.cc.

BUG= 651897 

Review-Url: https://codereview.chromium.org/2409893003
Cr-Commit-Position: refs/heads/master@{#424520}

[modify] https://crrev.com/2ff39f629a774ee513cc69d831d78908a6953e9d/content/browser/renderer_host/media/video_capture_host.cc
[modify] https://crrev.com/2ff39f629a774ee513cc69d831d78908a6953e9d/content/browser/renderer_host/media/video_capture_host.h
[modify] https://crrev.com/2ff39f629a774ee513cc69d831d78908a6953e9d/content/browser/renderer_host/media/video_capture_host_unittest.cc
[modify] https://crrev.com/2ff39f629a774ee513cc69d831d78908a6953e9d/content/common/BUILD.gn
[modify] https://crrev.com/2ff39f629a774ee513cc69d831d78908a6953e9d/content/common/media/video_capture_messages.h
[modify] https://crrev.com/2ff39f629a774ee513cc69d831d78908a6953e9d/content/common/video_capture.mojom
[modify] https://crrev.com/2ff39f629a774ee513cc69d831d78908a6953e9d/content/renderer/media/video_capture_impl.cc
[modify] https://crrev.com/2ff39f629a774ee513cc69d831d78908a6953e9d/content/renderer/media/video_capture_impl.h
[modify] https://crrev.com/2ff39f629a774ee513cc69d831d78908a6953e9d/content/renderer/media/video_capture_impl_unittest.cc
[modify] https://crrev.com/2ff39f629a774ee513cc69d831d78908a6953e9d/content/renderer/media/video_capture_message_filter.cc
[modify] https://crrev.com/2ff39f629a774ee513cc69d831d78908a6953e9d/content/renderer/media/video_capture_message_filter.h
[modify] https://crrev.com/2ff39f629a774ee513cc69d831d78908a6953e9d/content/renderer/media/video_capture_message_filter_unittest.cc

Labels: -Pri-3 Pri-2
In the process of crafting https://crrev.com2410383002/, the
next IPC-->mojom migration CL, I stumbled upon some troubles
with sharing the SharedMemory between processes, luckily rockot@
guided me to realize that there's no need to ShareToProcess() the
capture SharedMemory [1]; instead, a SharedMemoryHandle should be
created, and a duplicate of it sent over mojom.

This is all fine and dandy, but it has other implications:
If there's no need to shared the ShMems to renderer, then
there's no need for a Preamble nor an Epilogue of the VC
message set [2] (VideoCaptureMsg_NewBuffer, and perhaps
others). Moreover, 
VideoCaptureBufferPool{Impl})::ShareToProcess() and
VideoCaptureBufferTracker{Impl}::ShareToProcess() should 
be renamed and their signature simplified.

I'll probably be doing all that against this bug, this 
is just a log entry :)


[1] https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/shared_memory_buffer_tracker.cc?cl=GROK&gsn=ShareToProcess&rcl=1476731755&l=41

[2] https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/video_capture_host.h?q=Videocapturehost&sq=package:chromium&l=6&dr=C

[3] https://cs.chromium.org/chromium/src/media/capture/video/video_capture_buffer_pool_impl.cc?cl=GROK&gsn=ShareToProcess&rcl=1476731755&l=33
Project Member

Comment 14 by bugdroid1@chromium.org, Oct 20 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0abf0b0b42e9e974d78b6f13a2ed81afdd668287

commit 0abf0b0b42e9e974d78b6f13a2ed81afdd668287
Author: mcasas <mcasas@chromium.org>
Date: Thu Oct 20 16:46:30 2016

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

Review-Url: https://chromiumcodereview.appspot.com/2410383002
Cr-Commit-Position: refs/heads/master@{#426500}

[modify] https://crrev.com/0abf0b0b42e9e974d78b6f13a2ed81afdd668287/content/browser/renderer_host/media/shared_memory_buffer_tracker.cc
[modify] https://crrev.com/0abf0b0b42e9e974d78b6f13a2ed81afdd668287/content/browser/renderer_host/media/shared_memory_buffer_tracker.h
[modify] https://crrev.com/0abf0b0b42e9e974d78b6f13a2ed81afdd668287/content/browser/renderer_host/media/video_capture_controller.cc
[modify] https://crrev.com/0abf0b0b42e9e974d78b6f13a2ed81afdd668287/content/browser/renderer_host/media/video_capture_controller.h
[modify] https://crrev.com/0abf0b0b42e9e974d78b6f13a2ed81afdd668287/content/browser/renderer_host/media/video_capture_controller_event_handler.h
[modify] https://crrev.com/0abf0b0b42e9e974d78b6f13a2ed81afdd668287/content/browser/renderer_host/media/video_capture_controller_unittest.cc
[modify] https://crrev.com/0abf0b0b42e9e974d78b6f13a2ed81afdd668287/content/browser/renderer_host/media/video_capture_host.cc
[modify] https://crrev.com/0abf0b0b42e9e974d78b6f13a2ed81afdd668287/content/browser/renderer_host/media/video_capture_host.h
[modify] https://crrev.com/0abf0b0b42e9e974d78b6f13a2ed81afdd668287/content/browser/renderer_host/media/video_capture_host_unittest.cc
[modify] https://crrev.com/0abf0b0b42e9e974d78b6f13a2ed81afdd668287/content/browser/renderer_host/media/video_capture_manager.cc
[modify] https://crrev.com/0abf0b0b42e9e974d78b6f13a2ed81afdd668287/content/browser/renderer_host/media/video_capture_manager.h
[modify] https://crrev.com/0abf0b0b42e9e974d78b6f13a2ed81afdd668287/content/browser/renderer_host/media/video_capture_manager_unittest.cc
[modify] https://crrev.com/0abf0b0b42e9e974d78b6f13a2ed81afdd668287/content/common/media/video_capture_messages.h
[modify] https://crrev.com/0abf0b0b42e9e974d78b6f13a2ed81afdd668287/content/common/video_capture.mojom
[modify] https://crrev.com/0abf0b0b42e9e974d78b6f13a2ed81afdd668287/content/renderer/media/video_capture_impl.cc
[modify] https://crrev.com/0abf0b0b42e9e974d78b6f13a2ed81afdd668287/content/renderer/media/video_capture_impl.h
[modify] https://crrev.com/0abf0b0b42e9e974d78b6f13a2ed81afdd668287/content/renderer/media/video_capture_impl_unittest.cc
[modify] https://crrev.com/0abf0b0b42e9e974d78b6f13a2ed81afdd668287/content/renderer/media/video_capture_message_filter.cc
[modify] https://crrev.com/0abf0b0b42e9e974d78b6f13a2ed81afdd668287/content/renderer/media/video_capture_message_filter.h
[modify] https://crrev.com/0abf0b0b42e9e974d78b6f13a2ed81afdd668287/content/renderer/media/video_capture_message_filter_unittest.cc
[modify] https://crrev.com/0abf0b0b42e9e974d78b6f13a2ed81afdd668287/media/capture/video/DEPS
[modify] https://crrev.com/0abf0b0b42e9e974d78b6f13a2ed81afdd668287/media/capture/video/video_capture_buffer_pool.h
[modify] https://crrev.com/0abf0b0b42e9e974d78b6f13a2ed81afdd668287/media/capture/video/video_capture_buffer_pool_impl.cc
[modify] https://crrev.com/0abf0b0b42e9e974d78b6f13a2ed81afdd668287/media/capture/video/video_capture_buffer_pool_impl.h
[modify] https://crrev.com/0abf0b0b42e9e974d78b6f13a2ed81afdd668287/media/capture/video/video_capture_buffer_tracker.h
[modify] https://crrev.com/0abf0b0b42e9e974d78b6f13a2ed81afdd668287/mojo/public/cpp/system/platform_handle.h
[modify] https://crrev.com/0abf0b0b42e9e974d78b6f13a2ed81afdd668287/services/video_capture/mojo_shared_memory_buffer_tracker.cc
[modify] https://crrev.com/0abf0b0b42e9e974d78b6f13a2ed81afdd668287/services/video_capture/mojo_shared_memory_buffer_tracker.h

Project Member

Comment 15 by bugdroid1@chromium.org, Oct 21 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/049d6417fa6d74291ef6048dd94dfd8f8b571215

commit 049d6417fa6d74291ef6048dd94dfd8f8b571215
Author: mcasas <mcasas@chromium.org>
Date: Fri Oct 21 21:53:41 2016

VideoCapture: remove last remnants of IPC

In this CL:
- On Browser: VCHost is not a BrowserMessageFilter nor
a  BrowserAssociatedInterface, and instead works like a
normal service implementation, Create()d on request and
hanging from the render process host.

-On renderer:
 - VideoCaptureMessageFilter, which was specific for
IPC, is removed altogether.
 - VCImpl (the "client"), does not need the IPC, and
instead gets an InterfacePtr to the remote VideoCaptureHost,
and uses it.

All that allows for a few simplifications around like, e.g.
VCImpl doesn't need to keep track of  |clients_pending_on_filter_|
nor of IO thread explicitly (using a ThreadChecker instead),
and also I cleaned up a bit more the unittests.

BUG= 651897 

Review-Url: https://chromiumcodereview.appspot.com/2430313007
Cr-Commit-Position: refs/heads/master@{#426903}

[modify] https://crrev.com/049d6417fa6d74291ef6048dd94dfd8f8b571215/content/browser/renderer_host/media/video_capture_host.cc
[modify] https://crrev.com/049d6417fa6d74291ef6048dd94dfd8f8b571215/content/browser/renderer_host/media/video_capture_host.h
[modify] https://crrev.com/049d6417fa6d74291ef6048dd94dfd8f8b571215/content/browser/renderer_host/media/video_capture_host_unittest.cc
[modify] https://crrev.com/049d6417fa6d74291ef6048dd94dfd8f8b571215/content/browser/renderer_host/render_process_host_impl.cc
[modify] https://crrev.com/049d6417fa6d74291ef6048dd94dfd8f8b571215/content/common/BUILD.gn
[modify] https://crrev.com/049d6417fa6d74291ef6048dd94dfd8f8b571215/content/common/content_message_generator.h
[delete] https://crrev.com/c04d1756559aee2768370f835cb7562e0f715f3a/content/common/media/video_capture_messages.h
[modify] https://crrev.com/049d6417fa6d74291ef6048dd94dfd8f8b571215/content/renderer/BUILD.gn
[modify] https://crrev.com/049d6417fa6d74291ef6048dd94dfd8f8b571215/content/renderer/media/video_capture_impl.cc
[modify] https://crrev.com/049d6417fa6d74291ef6048dd94dfd8f8b571215/content/renderer/media/video_capture_impl.h
[modify] https://crrev.com/049d6417fa6d74291ef6048dd94dfd8f8b571215/content/renderer/media/video_capture_impl_manager.cc
[modify] https://crrev.com/049d6417fa6d74291ef6048dd94dfd8f8b571215/content/renderer/media/video_capture_impl_manager.h
[modify] https://crrev.com/049d6417fa6d74291ef6048dd94dfd8f8b571215/content/renderer/media/video_capture_impl_manager_unittest.cc
[modify] https://crrev.com/049d6417fa6d74291ef6048dd94dfd8f8b571215/content/renderer/media/video_capture_impl_unittest.cc
[delete] https://crrev.com/c04d1756559aee2768370f835cb7562e0f715f3a/content/renderer/media/video_capture_message_filter.cc
[delete] https://crrev.com/c04d1756559aee2768370f835cb7562e0f715f3a/content/renderer/media/video_capture_message_filter.h
[delete] https://crrev.com/c04d1756559aee2768370f835cb7562e0f715f3a/content/renderer/media/video_capture_message_filter_unittest.cc
[modify] https://crrev.com/049d6417fa6d74291ef6048dd94dfd8f8b571215/content/renderer/render_thread_impl.cc
[modify] https://crrev.com/049d6417fa6d74291ef6048dd94dfd8f8b571215/content/test/BUILD.gn

Project Member

Comment 16 by bugdroid1@chromium.org, Oct 23 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/324c80a574fead20c0d75566b6365494de467957

commit 324c80a574fead20c0d75566b6365494de467957
Author: mcasas <mcasas@chromium.org>
Date: Sun Oct 23 13:49:33 2016

Rename VideoCaptureHostTest to VideoCaptureTest to reflect what it is

This CL renames VideoCaptureHostTest to VideoCaptureTest, since it
really is an integration test of a bunch of classes, as detailed in
the class-comments.  I've tried a few times to disentangle this
mess, in which VideoCaptureHost references MediaStreamManager knowing
that the latter is a singleton; MediaStreamManager publicly allows
everyone to access its VideoCaptureManager; VideoCaptureManager
is a concrete class owning pairs of VideoCaptureController -
VideoCaptureDevice; and VideoCaptureController uses via naked pointer
VideoCaptureHosts.  Not worth it for this unit test.

BUG= 651897 

Review-Url: https://chromiumcodereview.appspot.com/2435393003
Cr-Commit-Position: refs/heads/master@{#427001}

[modify] https://crrev.com/324c80a574fead20c0d75566b6365494de467957/content/browser/renderer_host/media/video_capture_host.h
[rename] https://crrev.com/324c80a574fead20c0d75566b6365494de467957/content/browser/renderer_host/media/video_capture_unittest.cc
[modify] https://crrev.com/324c80a574fead20c0d75566b6365494de467957/content/test/BUILD.gn

Status: Fixed (was: Assigned)
Fixed!
Project Member

Comment 18 by bugdroid1@chromium.org, Oct 27 2016

Labels: merge-merged-2840
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/796b2e6cd3c150f29460c082fcc60ee2186ea9ab

commit 796b2e6cd3c150f29460c082fcc60ee2186ea9ab
Author: mcasas <mcasas@chromium.org>
Date: Thu Oct 06 01:48:14 2016

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.

Review-Url: https://codereview.chromium.org/2390103002
Cr-Commit-Position: refs/heads/master@{#423388}

[modify] https://crrev.com/796b2e6cd3c150f29460c082fcc60ee2186ea9ab/content/browser/renderer_host/media/video_capture_host.cc
[modify] https://crrev.com/796b2e6cd3c150f29460c082fcc60ee2186ea9ab/content/browser/renderer_host/media/video_capture_host.h
[modify] https://crrev.com/796b2e6cd3c150f29460c082fcc60ee2186ea9ab/content/browser/renderer_host/media/video_capture_host_unittest.cc
[modify] https://crrev.com/796b2e6cd3c150f29460c082fcc60ee2186ea9ab/content/common/BUILD.gn
[modify] https://crrev.com/796b2e6cd3c150f29460c082fcc60ee2186ea9ab/content/common/media/video_capture_messages.h
[add] https://crrev.com/796b2e6cd3c150f29460c082fcc60ee2186ea9ab/content/common/video_capture.mojom
[modify] https://crrev.com/796b2e6cd3c150f29460c082fcc60ee2186ea9ab/content/public/app/mojo/content_browser_manifest.json
[modify] https://crrev.com/796b2e6cd3c150f29460c082fcc60ee2186ea9ab/content/renderer/media/video_capture_impl.cc
[modify] https://crrev.com/796b2e6cd3c150f29460c082fcc60ee2186ea9ab/content/renderer/media/video_capture_impl.h
[modify] https://crrev.com/796b2e6cd3c150f29460c082fcc60ee2186ea9ab/content/renderer/media/video_capture_impl_manager.cc
[modify] https://crrev.com/796b2e6cd3c150f29460c082fcc60ee2186ea9ab/content/renderer/media/video_capture_impl_manager_unittest.cc
[modify] https://crrev.com/796b2e6cd3c150f29460c082fcc60ee2186ea9ab/content/renderer/media/video_capture_impl_unittest.cc
[modify] https://crrev.com/796b2e6cd3c150f29460c082fcc60ee2186ea9ab/content/renderer/media/video_capture_message_filter.cc
[modify] https://crrev.com/796b2e6cd3c150f29460c082fcc60ee2186ea9ab/content/renderer/media/video_capture_message_filter.h

Comment 19 by dimu@google.com, Nov 4 2016

Labels: -merge-merged-2840
[Automated comment] removing mislabelled merge-merged-2840
[bulk-edit : please ignore if not applicable]

Could you please set the correct milestone for this issue?

Sign in to add a comment