Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1699553002:
Mojo Video Capture service in media/capture (Closed)
DescriptionMojo Video Capture service in media/capture
DD: https://goo.gl/Bsmk4u
(Iteration from http://crrev.com/1637793004, #ps4)
This CL:
- Adds media/capture/interfaces and media/capture/service.
- new source_set("service") for service/ (mojo module)
- has its own capture_unittests
BUG=584797
TEST= ./video_capture_service_unittests
Patch Set 1 #Patch Set 2 : added missing BUILD deps #
Total comments: 15
Patch Set 3 : rockot@s comments #
Total comments: 10
Patch Set 4 : perkj@s comments #
Total comments: 2
Patch Set 5 : emircan@ comments #
Total comments: 20
Patch Set 6 : perkj@ comments - mostly simplifications of the mojom. and renaming #
Total comments: 44
Patch Set 7 : perkj@s and magjed@s comments #
Total comments: 1
Patch Set 8 : emplace --> insert(std::make_pair) #Patch Set 9 : Rebased to using MojoSharedBufferVideoFrame #
Total comments: 25
Patch Set 10 : perkj@s comments and renaming #Messages
Total messages: 61 (38 generated)
Description was changed from ========== [WIP] Mojo Video Capture thingy, round 2 (in content/) https://goo.gl/Bsmk4u BUG=584797 ========== to ========== [WIP] Mojo Video Capture thingy, round 2 (in content/) https://goo.gl/Bsmk4u BUG=584797 TEST= ./content_unittests --gtest_filter="*VideoCaptureDeviceClientImpl*:*VideoCaptureTest*:*VideoCaptureHandlerImplTest*:*StreamImplTest*" ==========
Description was changed from ========== [WIP] Mojo Video Capture thingy, round 2 (in content/) https://goo.gl/Bsmk4u BUG=584797 TEST= ./content_unittests --gtest_filter="*VideoCaptureDeviceClientImpl*:*VideoCaptureTest*:*VideoCaptureHandlerImplTest*:*StreamImplTest*" ========== to ========== [WIP] Mojo Video Capture thingy, round 2 (in content/) DD: https://goo.gl/Bsmk4u Iteration from http://crrev.com/1637793004, (#ps4). Adds content/public/interfaces and content/browser/video_capture which only exports unittests and a BUILD.gn ATM. BUG=584797 TEST= ./content_unittests --gtest_filter="*VideoCaptureDeviceClientImpl*:*VideoCaptureTest*:*VideoCaptureHandlerImplTest*:*StreamImplTest*" ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Description was changed from ========== [WIP] Mojo Video Capture thingy, round 2 (in content/) DD: https://goo.gl/Bsmk4u Iteration from http://crrev.com/1637793004, (#ps4). Adds content/public/interfaces and content/browser/video_capture which only exports unittests and a BUILD.gn ATM. BUG=584797 TEST= ./content_unittests --gtest_filter="*VideoCaptureDeviceClientImpl*:*VideoCaptureTest*:*VideoCaptureHandlerImplTest*:*StreamImplTest*" ========== to ========== [WIP] Mojo Video Capture thingy, round 2 (in media/capture) DD: https://goo.gl/Bsmk4u Iteration from http://crrev.com/1637793004, (#ps4). Adds media/capture/interfaces and media/capture/service. BUG=584797 TEST= ./media_unittests --gtest_filter="*VideoCaptureDeviceClientImpl*:*VideoCaptureTest*:*VideoCaptureHandlerImplTest*:*StreamImplTest*" ==========
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
Patchset #2 (id:140001) has been deleted
Patchset #2 (id:160001) has been deleted
Description was changed from ========== [WIP] Mojo Video Capture thingy, round 2 (in media/capture) DD: https://goo.gl/Bsmk4u Iteration from http://crrev.com/1637793004, (#ps4). Adds media/capture/interfaces and media/capture/service. BUG=584797 TEST= ./media_unittests --gtest_filter="*VideoCaptureDeviceClientImpl*:*VideoCaptureTest*:*VideoCaptureHandlerImplTest*:*StreamImplTest*" ========== to ========== [WIP] Mojo Video Capture thingy, round 2 (in media/capture) DD: https://goo.gl/Bsmk4u (Iteration from http://crrev.com/1637793004, #ps4) This CL: - Adds media/capture/interfaces and media/capture/service. - media/ BUILD.gn is refactored: -- media/capture/ gets its own source_set("capture") except for media/capture/service has its own source_set("service") -- media/capture has its own capture_unittests - media/capture/webm_muxer* --> moved to media/filters since they have nothing to do with capture/ and a lot with filters. BUG=584797 TEST= ./media_unittests --gtest_filter="*VideoCaptureDeviceClientImpl*:*VideoCaptureTest*:*VideoCaptureHandlerImplTest*:*StreamImplTest*" ==========
Description was changed from ========== [WIP] Mojo Video Capture thingy, round 2 (in media/capture) DD: https://goo.gl/Bsmk4u (Iteration from http://crrev.com/1637793004, #ps4) This CL: - Adds media/capture/interfaces and media/capture/service. - media/ BUILD.gn is refactored: -- media/capture/ gets its own source_set("capture") except for media/capture/service has its own source_set("service") -- media/capture has its own capture_unittests - media/capture/webm_muxer* --> moved to media/filters since they have nothing to do with capture/ and a lot with filters. BUG=584797 TEST= ./media_unittests --gtest_filter="*VideoCaptureDeviceClientImpl*:*VideoCaptureTest*:*VideoCaptureHandlerImplTest*:*StreamImplTest*" ========== to ========== [WIP] Mojo Video Capture thingy, round 2 (in media/capture) DD: https://goo.gl/Bsmk4u (Iteration from http://crrev.com/1637793004, #ps4) This CL: - Adds media/capture/interfaces and media/capture/service. - media/ BUILD.gn is refactored and media/capture/ gets its own BUILD.gn with: -- a source_set("capture") for content/ and video/ stuff -- a source_set("service") for service/ (mojo module) -- has its own capture_unittests - media/capture/webm_muxer* --> moved to media/filters since they have nothing to do with capture/ and a lot with filters (**no new code in these files**). BUG=584797 TEST= ./media_unittests --gtest_filter="*VideoCaptureDeviceClientImpl*:*VideoCaptureTest*:*VideoCaptureHandlerImplTest*:*StreamImplTest*" ==========
Patchset #1 (id:120001) has been deleted
Description was changed from ========== [WIP] Mojo Video Capture thingy, round 2 (in media/capture) DD: https://goo.gl/Bsmk4u (Iteration from http://crrev.com/1637793004, #ps4) This CL: - Adds media/capture/interfaces and media/capture/service. - media/ BUILD.gn is refactored and media/capture/ gets its own BUILD.gn with: -- a source_set("capture") for content/ and video/ stuff -- a source_set("service") for service/ (mojo module) -- has its own capture_unittests - media/capture/webm_muxer* --> moved to media/filters since they have nothing to do with capture/ and a lot with filters (**no new code in these files**). BUG=584797 TEST= ./media_unittests --gtest_filter="*VideoCaptureDeviceClientImpl*:*VideoCaptureTest*:*VideoCaptureHandlerImplTest*:*StreamImplTest*" ========== to ========== [WIP] Mojo Video Capture thingy, round 2 (in media/capture) DD: https://goo.gl/Bsmk4u (Iteration from http://crrev.com/1637793004, #ps4) This CL: - Adds media/capture/interfaces and media/capture/service. - media/ BUILD.gn is refactored and media/capture/ gets its own BUILD.gn with: -- a source_set("capture") for content/ and video/ stuff -- a source_set("service") for service/ (mojo module) -- has its own capture_unittests - media/capture/webm_muxer* --> moved to media/filters (will be removed after http://crrev.com/1710713002) BUG=584797 TEST= ./media_unittests --gtest_filter="*VideoCaptureDeviceClientImpl*:*VideoCaptureTest*:*VideoCaptureHandlerImplTest*:*StreamImplTest*" ==========
Patchset #1 (id:180001) has been deleted
Patchset #1 (id:200001) has been deleted
Patchset #1 (id:220001) has been deleted
Patchset #1 (id:240001) has been deleted
Description was changed from ========== [WIP] Mojo Video Capture thingy, round 2 (in media/capture) DD: https://goo.gl/Bsmk4u (Iteration from http://crrev.com/1637793004, #ps4) This CL: - Adds media/capture/interfaces and media/capture/service. - media/ BUILD.gn is refactored and media/capture/ gets its own BUILD.gn with: -- a source_set("capture") for content/ and video/ stuff -- a source_set("service") for service/ (mojo module) -- has its own capture_unittests - media/capture/webm_muxer* --> moved to media/filters (will be removed after http://crrev.com/1710713002) BUG=584797 TEST= ./media_unittests --gtest_filter="*VideoCaptureDeviceClientImpl*:*VideoCaptureTest*:*VideoCaptureHandlerImplTest*:*StreamImplTest*" ========== to ========== [WIP] Mojo Video Capture thingy, round 2 (in media/capture) DD: https://goo.gl/Bsmk4u (Iteration from http://crrev.com/1637793004, #ps4) This CL: - Adds media/capture/interfaces and media/capture/service. - media/ BUILD.gn is refactored and media/capture/ gets its own BUILD.gn with: -- a source_set("capture") for content/ and video/ stuff -- a source_set("service") for service/ (mojo module) -- has its own capture_unittests - Dependent on http://crrev.com/1710713002, media/capture/webm_muxer* --> moved to media/filters - Dependent on http://crbug.com/587693 as well BUG=584797 TEST= ./media_unittests --gtest_filter="*VideoCaptureDeviceClientImpl*:*VideoCaptureTest*:*VideoCaptureHandlerImplTest*:*StreamImplTest*" ==========
Patchset #1 (id:260001) has been deleted
Patchset #1 (id:280001) has been deleted
Description was changed from ========== [WIP] Mojo Video Capture thingy, round 2 (in media/capture) DD: https://goo.gl/Bsmk4u (Iteration from http://crrev.com/1637793004, #ps4) This CL: - Adds media/capture/interfaces and media/capture/service. - media/ BUILD.gn is refactored and media/capture/ gets its own BUILD.gn with: -- a source_set("capture") for content/ and video/ stuff -- a source_set("service") for service/ (mojo module) -- has its own capture_unittests - Dependent on http://crrev.com/1710713002, media/capture/webm_muxer* --> moved to media/filters - Dependent on http://crbug.com/587693 as well BUG=584797 TEST= ./media_unittests --gtest_filter="*VideoCaptureDeviceClientImpl*:*VideoCaptureTest*:*VideoCaptureHandlerImplTest*:*StreamImplTest*" ========== to ========== [WIP] Mojo Video Capture thingy, round 2 (in media/capture) DD: https://goo.gl/Bsmk4u (Iteration from http://crrev.com/1637793004, #ps4) This CL: - Adds media/capture/interfaces and media/capture/service. - media/ BUILD.gn is refactored and media/capture/ gets its own BUILD.gn with: -- a source_set("capture") for content/ and video/ stuff -- a source_set("service") for service/ (mojo module) -- has its own capture_unittests - Dependent on http://crrev.com/1710713002, media/capture/webm_muxer* --> moved to media/filters - Dependent on http://crbug.com/587693 as well BUG=584797 TEST= ./capture_unittests --gtest_filter="*VideoCaptureDeviceClientImpl*:*VideoCaptureTest*:*VideoCaptureHandlerImplTest*:*StreamImplTest*" ==========
Patchset #1 (id:300001) has been deleted
Patchset #1 (id:320001) has been deleted
Patchset #1 (id:340001) has been deleted
Description was changed from ========== [WIP] Mojo Video Capture thingy, round 2 (in media/capture) DD: https://goo.gl/Bsmk4u (Iteration from http://crrev.com/1637793004, #ps4) This CL: - Adds media/capture/interfaces and media/capture/service. - media/ BUILD.gn is refactored and media/capture/ gets its own BUILD.gn with: -- a source_set("capture") for content/ and video/ stuff -- a source_set("service") for service/ (mojo module) -- has its own capture_unittests - Dependent on http://crrev.com/1710713002, media/capture/webm_muxer* --> moved to media/filters - Dependent on http://crbug.com/587693 as well BUG=584797 TEST= ./capture_unittests --gtest_filter="*VideoCaptureDeviceClientImpl*:*VideoCaptureTest*:*VideoCaptureHandlerImplTest*:*StreamImplTest*" ========== to ========== Mojo Video Capture service in media/capture DD: https://goo.gl/Bsmk4u (Iteration from http://crrev.com/1637793004, #ps4) This CL: - Adds media/capture/interfaces and media/capture/service. - media/ BUILD.gn is refactored and media/capture/ gets its own BUILD.gn with: -- a source_set("capture") for content/ and video/ stuff -- a source_set("service") for service/ (mojo module) -- has its own capture_unittests BUG=584797 TEST= ./capture_service_unittests ==========
Patchset #1 (id:360001) has been deleted
Patchset #1 (id:380001) has been deleted
Description was changed from ========== Mojo Video Capture service in media/capture DD: https://goo.gl/Bsmk4u (Iteration from http://crrev.com/1637793004, #ps4) This CL: - Adds media/capture/interfaces and media/capture/service. - media/ BUILD.gn is refactored and media/capture/ gets its own BUILD.gn with: -- a source_set("capture") for content/ and video/ stuff -- a source_set("service") for service/ (mojo module) -- has its own capture_unittests BUG=584797 TEST= ./capture_service_unittests ========== to ========== Mojo Video Capture service in media/capture DD: https://goo.gl/Bsmk4u (Iteration from http://crrev.com/1637793004, #ps4) This CL: - Adds media/capture/interfaces and media/capture/service. - media/ BUILD.gn is refactored and media/capture/ gets its own BUILD.gn with: -- a source_set("capture") for content/ and video/ stuff -- a source_set("service") for service/ (mojo module) -- has its own capture_unittests Still depending on http://crrev.com/1704273002 that will introduce a MojoVideoFrame that'll make the current one here unnecessary. BUG=584797 TEST= ./capture_service_unittests ==========
mcasas@chromium.org changed reviewers: + emircan@chromium.org, perkj@chromium.org, rockot@chromium.org
Sorry for the delay in updating this CL, shifting around code in media took forever. Nothing significant changed from when this code was in components/. rockot@ PTAL at mojo parts again. emircan@ PTAL at the rest. perkj@ PTAL at the video parts - please read the DD for context.
Description was changed from ========== Mojo Video Capture service in media/capture DD: https://goo.gl/Bsmk4u (Iteration from http://crrev.com/1637793004, #ps4) This CL: - Adds media/capture/interfaces and media/capture/service. - media/ BUILD.gn is refactored and media/capture/ gets its own BUILD.gn with: -- a source_set("capture") for content/ and video/ stuff -- a source_set("service") for service/ (mojo module) -- has its own capture_unittests Still depending on http://crrev.com/1704273002 that will introduce a MojoVideoFrame that'll make the current one here unnecessary. BUG=584797 TEST= ./capture_service_unittests ========== to ========== Mojo Video Capture service in media/capture DD: https://goo.gl/Bsmk4u (Iteration from http://crrev.com/1637793004, #ps4) This CL: - Adds media/capture/interfaces and media/capture/service. - new source_set("service") for service/ (mojo module) - has its own capture_unittests Still depending on http://crrev.com/1704273002 that will introduce a MojoVideoFrame that'll make the current one here unnecessary. BUG=584797 TEST= ./capture_service_unittests ==========
perkj@chromium.org changed reviewers: + tommi@chromium.org
Adding Tommi as well. Tommi - do you know more of mojo than I do? I will also take a look when I can. (home with sick kids today)
Looks pretty good overall. Some comments from a first pass https://codereview.chromium.org/1699553002/diff/420001/media/capture/interfac... File media/capture/interfaces/video_capture.mojom (right): https://codereview.chromium.org/1699553002/diff/420001/media/capture/interfac... media/capture/interfaces/video_capture.mojom:10: module media.video_capture; We shouldn't nest namespaces more than necessary, and we also should still use the mojom suffix everywhere. If this is going to live in media the module should be media.mojom. https://codereview.chromium.org/1699553002/diff/420001/media/capture/interfac... media/capture/interfaces/video_capture.mojom:16: int32 request_id; Shouldn't be necessary, in fact this struct should be unnecessary https://codereview.chromium.org/1699553002/diff/420001/media/capture/interfac... media/capture/interfaces/video_capture.mojom:32: int32 request_id; Shouldn't be necessary, in fact this struct should be unnecessary. See comment in the interface below. https://codereview.chromium.org/1699553002/diff/420001/media/capture/interfac... media/capture/interfaces/video_capture.mojom:37: int32 request_id; I don't think this is used? https://codereview.chromium.org/1699553002/diff/420001/media/capture/interfac... media/capture/interfaces/video_capture.mojom:112: RequestMediaDevices(MediaDevicesInfoRequest request) This should just be: RequestMediaDevices() => (array<WebSourceInfo> devices); there's no need for intermediate structs or request IDs. https://codereview.chromium.org/1699553002/diff/420001/media/capture/service/... File media/capture/service/stream_impl.cc (right): https://codereview.chromium.org/1699553002/diff/420001/media/capture/service/... media/capture/service/stream_impl.cc:116: if (client_) // A devices can notify of errors before there is a |client_|. nit: "A device cannot notify" perhaps? https://codereview.chromium.org/1699553002/diff/420001/media/capture/service/... File media/capture/service/stream_impl.h (right): https://codereview.chromium.org/1699553002/diff/420001/media/capture/service/... media/capture/service/stream_impl.h:89: // The TaskRunner where this class lives, needed for OnFrame() and OnErro() to nit: OnError() https://codereview.chromium.org/1699553002/diff/420001/media/capture/service/... File media/capture/service/video_capture_handler_impl.cc (right): https://codereview.chromium.org/1699553002/diff/420001/media/capture/service/... media/capture/service/video_capture_handler_impl.cc:60: reply->request_id = request->request_id; We should not need to use a request_id at all. This is why Mojo messages have a request-response mechanism built in. The client can tell which request a response corresponds to without this ID.
perkj@chromium.org changed reviewers: + magjed@chromium.org
Sorry but I am having trouble understanding how this fits into the current architecture and what this replaces and how it will be used. Is it possible to elaborate a bit more in your design document? After that- can you try to land this in smaller portions? https://codereview.chromium.org/1699553002/diff/440001/media/capture/interfac... File media/capture/interfaces/video_capture.mojom (right): https://codereview.chromium.org/1699553002/diff/440001/media/capture/interfac... media/capture/interfaces/video_capture.mojom:40: // TODO(mcasas): investigate if sharing this buffer per frame has ny impact in s ny/any https://codereview.chromium.org/1699553002/diff/440001/media/capture/interfac... media/capture/interfaces/video_capture.mojom:85: Pause(); Is Pause used? Can we skip this for now and use Start Stop only? https://codereview.chromium.org/1699553002/diff/440001/media/capture/interfac... media/capture/interfaces/video_capture.mojom:103: => (array<WebSourceInfo> devices); should only be possible to get one video device at the time. https://codereview.chromium.org/1699553002/diff/440001/media/capture/interfac... media/capture/interfaces/video_capture.mojom:106: RequestStream(StreamOptions options, string security_origin) Can we stick with only one method to start with.
guys PTAL https://codereview.chromium.org/1699553002/diff/420001/media/capture/interfac... File media/capture/interfaces/video_capture.mojom (right): https://codereview.chromium.org/1699553002/diff/420001/media/capture/interfac... media/capture/interfaces/video_capture.mojom:10: module media.video_capture; On 2016/03/02 02:52:08, Ken Rockot wrote: > We shouldn't nest namespaces more than necessary, and we also should still use > the mojom suffix everywhere. If this is going to live in media the module should > be media.mojom. Done. https://codereview.chromium.org/1699553002/diff/420001/media/capture/interfac... media/capture/interfaces/video_capture.mojom:16: int32 request_id; On 2016/03/02 02:52:08, Ken Rockot wrote: > Shouldn't be necessary, in fact this struct should be unnecessary Done. https://codereview.chromium.org/1699553002/diff/420001/media/capture/interfac... media/capture/interfaces/video_capture.mojom:32: int32 request_id; On 2016/03/02 02:52:07, Ken Rockot wrote: > Shouldn't be necessary, in fact this struct should be unnecessary. See comment > in the interface below. Done. https://codereview.chromium.org/1699553002/diff/420001/media/capture/interfac... media/capture/interfaces/video_capture.mojom:37: int32 request_id; On 2016/03/02 02:52:08, Ken Rockot wrote: > I don't think this is used? Gone! https://codereview.chromium.org/1699553002/diff/420001/media/capture/interfac... media/capture/interfaces/video_capture.mojom:112: RequestMediaDevices(MediaDevicesInfoRequest request) On 2016/03/02 02:52:08, Ken Rockot wrote: > This should just be: > > RequestMediaDevices() => (array<WebSourceInfo> devices); > > there's no need for intermediate structs or request IDs. Done. https://codereview.chromium.org/1699553002/diff/420001/media/capture/service/... File media/capture/service/stream_impl.cc (right): https://codereview.chromium.org/1699553002/diff/420001/media/capture/service/... media/capture/service/stream_impl.cc:116: if (client_) // A devices can notify of errors before there is a |client_|. On 2016/03/02 02:52:08, Ken Rockot wrote: > nit: "A device cannot notify" perhaps? Rewritten. https://codereview.chromium.org/1699553002/diff/420001/media/capture/service/... File media/capture/service/stream_impl.h (right): https://codereview.chromium.org/1699553002/diff/420001/media/capture/service/... media/capture/service/stream_impl.h:89: // The TaskRunner where this class lives, needed for OnFrame() and OnErro() to On 2016/03/02 02:52:08, Ken Rockot wrote: > nit: OnError() Done. https://codereview.chromium.org/1699553002/diff/440001/media/capture/interfac... File media/capture/interfaces/video_capture.mojom (right): https://codereview.chromium.org/1699553002/diff/440001/media/capture/interfac... media/capture/interfaces/video_capture.mojom:40: // TODO(mcasas): investigate if sharing this buffer per frame has ny impact in On 2016/03/02 13:14:10, perkj_chrome wrote: > s ny/any Done. https://codereview.chromium.org/1699553002/diff/440001/media/capture/interfac... media/capture/interfaces/video_capture.mojom:85: Pause(); On 2016/03/02 13:14:10, perkj_chrome wrote: > Is Pause used? Can we skip this for now and use Start Stop only? Done. https://codereview.chromium.org/1699553002/diff/440001/media/capture/interfac... media/capture/interfaces/video_capture.mojom:103: => (array<WebSourceInfo> devices); On 2016/03/02 13:14:10, perkj_chrome wrote: > should only be possible to get one video device at the time. ? This RPC enumerates the devices, it follows the UserMediaClient naming [1] [1] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... https://codereview.chromium.org/1699553002/diff/440001/media/capture/interfac... media/capture/interfaces/video_capture.mojom:106: RequestStream(StreamOptions options, string security_origin) On 2016/03/02 13:14:10, perkj_chrome wrote: > Can we stick with only one method to start with. Negative, we need at least: - enumerate all devices - start capturing using one of them to have a prototype that actually does something.
https://codereview.chromium.org/1699553002/diff/440001/media/capture/service/... File media/capture/service/mojo_video_frame.cc (right): https://codereview.chromium.org/1699553002/diff/440001/media/capture/service/... media/capture/service/mojo_video_frame.cc:46: return false; DVLOG(ERROR) here can be useful. https://codereview.chromium.org/1699553002/diff/440001/media/capture/service/... File media/capture/service/stream_impl.h (right): https://codereview.chromium.org/1699553002/diff/440001/media/capture/service/... media/capture/service/stream_impl.h:54: // VideoCaptureDeviceClientImpl::Delegate Can you make a note of which thread these run on? https://codereview.chromium.org/1699553002/diff/460001/media/capture/service/... File media/capture/service/video_capture_device_client_impl.cc (right): https://codereview.chromium.org/1699553002/diff/460001/media/capture/service/... media/capture/service/video_capture_device_client_impl.cc:316: frame->metadata()->Clear(); I don't see any frames_.pop*() calls here. You might be reusing an existing frame in flight with this code. Also, a unit test for checking the right behavior on pool behavior would be helpful here.
PTAL https://codereview.chromium.org/1699553002/diff/460001/media/capture/service/... File media/capture/service/video_capture_device_client_impl.cc (right): https://codereview.chromium.org/1699553002/diff/460001/media/capture/service/... media/capture/service/video_capture_device_client_impl.cc:316: frame->metadata()->Clear(); On 2016/03/02 21:52:10, emircan wrote: > I don't see any frames_.pop*() calls here. You might be reusing an existing > frame in flight with this code. Also, a unit test for checking the right > behavior on pool behavior would be helpful here. Good catch. Rewritten this part..
mojo bits lgtm btw
https://codereview.chromium.org/1699553002/diff/480001/media/capture/interfac... File media/capture/interfaces/video_capture.mojom (right): https://codereview.chromium.org/1699553002/diff/480001/media/capture/interfac... media/capture/interfaces/video_capture.mojom:76: // The Stream interface abstracts a distinct local source of video frame data Is this right or wrong? it looks like this is only about video camera devices which I think might simplify things. What is the idea with the other types you list here? How will that work in the future? Separate mojo components? I think you should take the opportunity to split that out from handling video cameras. https://codereview.chromium.org/1699553002/diff/480001/media/capture/interfac... media/capture/interfaces/video_capture.mojom:96: => (array<WebSourceInfo> devices); I dont think you should copy blink structs. You will have to translate into blink anyway so please use a chrome struct. To not make the change so huge, you could use the same struct as we use today StreamDeviceInfo? (not that it is any good for video since it is polluted by audio stuff) Can we call this EnumerateDevices? https://codereview.chromium.org/1699553002/diff/480001/media/capture/interfac... media/capture/interfaces/video_capture.mojom:99: RequestStream(StreamOptions options, string security_origin) Can we call this OpenCamera or OpenVideoDevice? and StreamClient -> VideoCameraClient or VideoDeviceClient depending on if this interface will be used for other video device types than cameras. ie, screen capture etc.. https://codereview.chromium.org/1699553002/diff/480001/media/capture/service/... File media/capture/service/stream_impl.cc (right): https://codereview.chromium.org/1699553002/diff/480001/media/capture/service/... media/capture/service/stream_impl.cc:49: void StreamImpl::OnFrame(const scoped_refptr<MojoVideoFrame>& frame, This is an implementation of VideoCaptureDeviceClientImpl::Delegate which is not reference counted. So how does destruction work? ie- How does this ensure that VideoCaptureDeviceClientImpl::Delegate::OnFrame is not called after StreamImpl is destroyed? https://codereview.chromium.org/1699553002/diff/480001/media/capture/service/... media/capture/service/stream_impl.cc:53: weak_factory_.GetWeakPtr(), frame, timestamp)); nit: you can cash a weak ptr in the ctor if you want and always use it here. Then you dont need to create a new one for each frame. https://codereview.chromium.org/1699553002/diff/480001/media/capture/service/... media/capture/service/stream_impl.cc:95: weak_factory_.GetWeakPtr(), buffer_id)); dito- you can use a cached weakptr here if you want. https://codereview.chromium.org/1699553002/diff/480001/media/capture/service/... media/capture/service/stream_impl.cc:109: if (video_frame == in_use_video_frames_.end()) { DCHECK this? https://codereview.chromium.org/1699553002/diff/480001/media/capture/service/... File media/capture/service/stream_impl.h (right): https://codereview.chromium.org/1699553002/diff/480001/media/capture/service/... media/capture/service/stream_impl.h:69: // Bound for the client to notify us that it's done with the VideoFrame bound to what? Bound to the video frame destructor forwarded to the client ? https://codereview.chromium.org/1699553002/diff/480001/media/capture/service/... File media/capture/service/video_capture_device_client_impl.h (right): https://codereview.chromium.org/1699553002/diff/480001/media/capture/service/... media/capture/service/video_capture_device_client_impl.h:42: class VideoCaptureDeviceClientImpl : public media::VideoCaptureDevice::Client { So this class is more or less the same as the existing VideoCaptureDeviceClient? Is it possible to show only the diff somehow? git cl upload --similarity=...
https://codereview.chromium.org/1699553002/diff/480001/media/base/video_frame.h File media/base/video_frame.h (right): https://codereview.chromium.org/1699553002/diff/480001/media/base/video_frame... media/base/video_frame.h:428: void set_data(size_t plane, uint8_t* ptr); I don't like these changes to VideoFrame. Adding setters for the data pointers and strides breaks encapsulation and ownership handling. Instead of letting MojoVideoFrame inherit from VideoFrame, can't you use one of the WrapExternal functions?
perkj@ PTAL Let me know if you want me to rename the files/classes: StreamImpl -> VideoCaptureStreamImpl MockStreamImpl -> MockVideoCaptureStreamImpl etc. So far only the mojom:: classes are changed. https://codereview.chromium.org/1699553002/diff/480001/media/base/video_frame.h File media/base/video_frame.h (right): https://codereview.chromium.org/1699553002/diff/480001/media/base/video_frame... media/base/video_frame.h:428: void set_data(size_t plane, uint8_t* ptr); On 2016/03/03 14:20:58, magjed_chromium wrote: > I don't like these changes to VideoFrame. Adding setters for the data pointers > and strides breaks encapsulation and ownership handling. Instead of letting > MojoVideoFrame inherit from VideoFrame, can't you use one of the WrapExternal > functions? Actually this is happening in //, see the description of this CL: "Still depending on http://crrev.com/1704273002 that will introduce a MojoVideoFrame that'll make the current one here unnecessary." Please feel free to add comments in that CL. https://codereview.chromium.org/1699553002/diff/480001/media/capture/interfac... File media/capture/interfaces/video_capture.mojom (right): https://codereview.chromium.org/1699553002/diff/480001/media/capture/interfac... media/capture/interfaces/video_capture.mojom:76: // The Stream interface abstracts a distinct local source of video frame data On 2016/03/03 13:12:41, perkj_chrome wrote: > Is this right or wrong? it looks like this is only about video camera devices > which I think might simplify things. I will limit the scope of the current CL to video capture devices and write a TODO for the rest. This also simplifies the current WebSourceInfo. > > What is the idea with the other types you list here? How will that work in the > future? Separate mojo components? I think you should take the opportunity to > split that out from handling video cameras. My personal opinion is that screen and tab should, at this level, be treated as regular video capture devices, and let the permissions and pickers be dealt with elsewhere (by preference as mojo services themselves). At this level we should get just a message saying "hey, start capturing device X", or "please start capturing tab Z". https://codereview.chromium.org/1699553002/diff/480001/media/capture/interfac... media/capture/interfaces/video_capture.mojom:96: => (array<WebSourceInfo> devices); On 2016/03/03 13:12:41, perkj_chrome wrote: > I dont think you should copy blink structs. You will have to translate into > blink anyway so please use a chrome struct. To not make the change so huge, you > could use the same struct as we use today StreamDeviceInfo? (not that it is any > good for video since it is polluted by audio stuff) > In the spirit of your comment before, let's call it VideoCaptureDeviceInfo. > Can we call this EnumerateDevices? Done. https://codereview.chromium.org/1699553002/diff/480001/media/capture/interfac... media/capture/interfaces/video_capture.mojom:99: RequestStream(StreamOptions options, string security_origin) On 2016/03/03 13:12:41, perkj_chrome wrote: > Can we call this OpenCamera or OpenVideoDevice? and StreamClient -> > VideoCameraClient or VideoDeviceClient depending on if this interface will be > used for other video device types than cameras. ie, screen capture etc.. > RequestStream() ==> RequestVideoCaptureStream() Stream ==> VideoCaptureStream StreamClient ==> VideoCaptureStreamClient https://codereview.chromium.org/1699553002/diff/480001/media/capture/service/... File media/capture/service/stream_impl.cc (right): https://codereview.chromium.org/1699553002/diff/480001/media/capture/service/... media/capture/service/stream_impl.cc:49: void StreamImpl::OnFrame(const scoped_refptr<MojoVideoFrame>& frame, On 2016/03/03 13:12:41, perkj_chrome wrote: > This is an implementation of VideoCaptureDeviceClientImpl::Delegate which is not > reference counted. So how does destruction work? > ie- How does this ensure that VideoCaptureDeviceClientImpl::Delegate::OnFrame is > not called after StreamImpl is destroyed? The sequence replicates the current ownership graph. In renderer_host today (again, see [1]): VCM --<owns>--> VideoCaptureDevice --<owns>--> VideoCaptureDeviceClient VCM --<owns>--> VideoCaptureController VideoCaptureDevice --<has a * to>--> VideoCaptureDeviceClient Here VCManager is VCHandler, VCC is StreamImpl. The destruction order is enforce by VCM (ToT) or by VCHandler by the way in which they are kept in the internal list. [1] https://www.chromium.org/developers/design-documents/video-capture https://codereview.chromium.org/1699553002/diff/480001/media/capture/service/... media/capture/service/stream_impl.cc:53: weak_factory_.GetWeakPtr(), frame, timestamp)); On 2016/03/03 13:12:41, perkj_chrome wrote: > nit: you can cash a weak ptr in the ctor if you want and always use it here. > Then you dont need to create a new one for each frame. The created WeakPtr is passed to the bound Callback, so it's gone. This is a very typical pattern though so nothing fancy here: https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_p... and https://code.google.com/p/chromium/codesearch#chromium/src/base/callback.h&q=... https://codereview.chromium.org/1699553002/diff/480001/media/capture/service/... media/capture/service/stream_impl.cc:95: weak_factory_.GetWeakPtr(), buffer_id)); On 2016/03/03 13:12:41, perkj_chrome wrote: > dito- you can use a cached weakptr here if you want. Acknowledged. https://codereview.chromium.org/1699553002/diff/480001/media/capture/service/... media/capture/service/stream_impl.cc:109: if (video_frame == in_use_video_frames_.end()) { On 2016/03/03 13:12:41, perkj_chrome wrote: > DCHECK this? Done. https://codereview.chromium.org/1699553002/diff/480001/media/capture/service/... File media/capture/service/stream_impl.h (right): https://codereview.chromium.org/1699553002/diff/480001/media/capture/service/... media/capture/service/stream_impl.h:69: // Bound for the client to notify us that it's done with the VideoFrame On 2016/03/03 13:12:41, perkj_chrome wrote: > bound to what? Bound to the video frame destructor forwarded to the client ? Explained. https://codereview.chromium.org/1699553002/diff/480001/media/capture/service/... File media/capture/service/video_capture_device_client_impl.h (right): https://codereview.chromium.org/1699553002/diff/480001/media/capture/service/... media/capture/service/video_capture_device_client_impl.h:42: class VideoCaptureDeviceClientImpl : public media::VideoCaptureDevice::Client { On 2016/03/03 13:12:42, perkj_chrome wrote: > So this class is more or less the same as the existing VideoCaptureDeviceClient? > Is it possible to show only the diff somehow? git cl upload --similarity=... Too many things change: - the current one has a VideoFramePool, whereas VCD relies on VCController owning it (and VCD using it) - and because of that, there is a load of media::VideoCaptureDevice::Client::Buffer being passed around that IMHO makes it very hard to reason as to what is happening. The current implementation is simplified.
lgtm. This is good start for moving things to mojo. I think it is better to address tab capture, contsraints and other capture formats in separate patches.
On 2016/03/03 19:51:16, emircan wrote: > lgtm. This is good start for moving things to mojo. I think it is better to > address tab capture, contsraints and other capture formats in separate patches. ping perkj@
If I understand things right- this is browser side only. So then - should this be a chance to write a simple interface for starting a video camera device that can be used by many clients - ie many render process. overall I think this looks good. But the cl is way to big to grasp and review all details of. So can you please split this up in several cls? The inteface and mojo parts to start with maybe? https://codereview.chromium.org/1699553002/diff/500001/media/capture/interfac... File media/capture/interfaces/video_capture.mojom (right): https://codereview.chromium.org/1699553002/diff/500001/media/capture/interfac... media/capture/interfaces/video_capture.mojom:6: // selecting one for capturing. Such a VCD is manipulated via creation of a Want to update this text with the new names. https://codereview.chromium.org/1699553002/diff/500001/media/capture/interfac... media/capture/interfaces/video_capture.mojom:17: string label; If this is from VideoCaptureDevice::Name - can we call it device_id and name then. https://codereview.chromium.org/1699553002/diff/500001/media/capture/interfac... media/capture/interfaces/video_capture.mojom:21: struct StreamOptions { VideoCaptureDeviceOptions? There are no Steams any longer which is nice. https://codereview.chromium.org/1699553002/diff/500001/media/capture/interfac... media/capture/interfaces/video_capture.mojom:52: mojo.Rect visible_rect; Do we use this now? Can't we add this if we ever need it with Video capture devices? https://codereview.chromium.org/1699553002/diff/500001/media/capture/interfac... media/capture/interfaces/video_capture.mojom:57: int64 timestamp; Why can't this be TimeTicks then instead of int64? https://codereview.chromium.org/1699553002/diff/500001/media/capture/interfac... media/capture/interfaces/video_capture.mojom:64: FrameAvailable(FrameInfo info) => (); Should this be OnFrameAvailable to be consistent with other callback interfaces? Or is there another naming convention for mojo? Or maybe even better. use the same as // VideoCaptureDeviceClientImpl::Delegate void OnFrame(const scoped_refptr<MojoVideoFrame>& frame) void OnError(const std::string& reason) override; https://codereview.chromium.org/1699553002/diff/500001/media/capture/interfac... media/capture/interfaces/video_capture.mojom:66: // Some Error has happened; the capture will in all likelihood be interrupted. Please decide what will happen when an error occur. I think it should be stopped and that this VideoCaptureStreamClient is no longer usable. https://codereview.chromium.org/1699553002/diff/500001/media/capture/interfac... media/capture/interfaces/video_capture.mojom:66: // Some Error has happened; the capture will in all likelihood be interrupted. nit: Suggest - An error has occurred..... https://codereview.chromium.org/1699553002/diff/500001/media/capture/interfac... media/capture/interfaces/video_capture.mojom:83: // component. This can used by a UserMediaClient implementation to get hold of a If this is browser side only this will not be used by UserMediaClient. https://codereview.chromium.org/1699553002/diff/500001/media/capture/interfac... media/capture/interfaces/video_capture.mojom:92: RequestVideoCaptureStream(StreamOptions options, string security_origin) why security_origin if this is browser side only? https://codereview.chromium.org/1699553002/diff/500001/media/capture/service/... File media/capture/service/stream_impl.h (right): https://codereview.chromium.org/1699553002/diff/500001/media/capture/service/... media/capture/service/stream_impl.h:80: bool paused_ = false; not used ?
This is way too much new code for me to review in one CL - please split it up. https://codereview.chromium.org/1699553002/diff/500001/media/capture/interfac... File media/capture/interfaces/video_capture.mojom (right): https://codereview.chromium.org/1699553002/diff/500001/media/capture/interfac... media/capture/interfaces/video_capture.mojom:36: handle<shared_buffer> storage_handle; |storage_handle| contains all the data, right? Don't we need plane offsets, strides etc? https://codereview.chromium.org/1699553002/diff/500001/media/capture/service/... File media/capture/service/stream_impl.cc (right): https://codereview.chromium.org/1699553002/diff/500001/media/capture/service/... media/capture/service/stream_impl.cc:92: in_use_video_frames_.insert(std::make_pair(buffer_id, frame)); Use this instead: in_use_video_frames_.emplace(buffer_id, frame); https://codereview.chromium.org/1699553002/diff/500001/media/capture/service/... media/capture/service/stream_impl.cc:110: DCHECK(video_frame == in_use_video_frames_.end()) << "Buffer id " << buffer_id Shouldn't this be DCHECK(video_frame != in_use_video_frames_.end())? Is this code executed in any test? https://codereview.chromium.org/1699553002/diff/500001/media/capture/service/... File media/capture/service/stream_impl.h (right): https://codereview.chromium.org/1699553002/diff/500001/media/capture/service/... media/capture/service/stream_impl.h:85: std::map<int32_t, const scoped_refptr<MojoVideoFrame>&> in_use_video_frames_; nit: I would prefer the variable name |video_frames_in_use_| instead. https://codereview.chromium.org/1699553002/diff/500001/media/capture/service/... media/capture/service/stream_impl.h:85: std::map<int32_t, const scoped_refptr<MojoVideoFrame>&> in_use_video_frames_; The value type in this std::map is a const-ref, which looks super dangerous. Shouldn't it be a scoped_refptr<MojoVideoFrame>, i.e. a non-ref? Also, I don't see the frames being used anywhere, so why not use a set instead of a map? Is it a map to keep the frames alive? https://codereview.chromium.org/1699553002/diff/500001/media/capture/service/... File media/capture/service/video_capture_device_client_impl.cc (right): https://codereview.chromium.org/1699553002/diff/500001/media/capture/service/... media/capture/service/video_capture_device_client_impl.cc:214: dimensions, media::PIXEL_FORMAT_I420, media::PIXEL_STORAGE_CPU); Would it be possible to use GPU memory buffers like in content::VideoCaptureDeviceClient?
PTAL. Reducing the number of files: let me rebase to the MojoVideoFrame landing (which is imminent) and we'll get rid of 4 files for starters. Then, I could take out VideoCaptureDeviceClientImpl, but honestly, you guys wrote (perkj@) or updated (magjed@) the equivalent class VideoCaptureDeviceClient, so I have a hard time seeing how you'd have a problem with this simplified version. I can take out the unittests :D and land them separately :D https://codereview.chromium.org/1699553002/diff/500001/media/capture/interfac... File media/capture/interfaces/video_capture.mojom (right): https://codereview.chromium.org/1699553002/diff/500001/media/capture/interfac... media/capture/interfaces/video_capture.mojom:6: // selecting one for capturing. Such a VCD is manipulated via creation of a On 2016/03/04 15:34:02, perkj_chrome wrote: > Want to update this text with the new names. > Done. https://codereview.chromium.org/1699553002/diff/500001/media/capture/interfac... media/capture/interfaces/video_capture.mojom:17: string label; On 2016/03/04 15:34:02, perkj_chrome wrote: > If this is from VideoCaptureDevice::Name - can we call it device_id and name > then. Corresponds to device_name.GetNameAndModel(). Done. https://codereview.chromium.org/1699553002/diff/500001/media/capture/interfac... media/capture/interfaces/video_capture.mojom:21: struct StreamOptions { On 2016/03/04 15:34:02, perkj_chrome wrote: > VideoCaptureDeviceOptions? There are no Steams any longer which is nice. These are not the VideoCaptureDevice options, but the Capture options. Renamed to VideoCaptureOptions, please instruct otherwise. https://codereview.chromium.org/1699553002/diff/500001/media/capture/interfac... media/capture/interfaces/video_capture.mojom:36: handle<shared_buffer> storage_handle; On 2016/03/04 15:41:42, magjed_chromium wrote: > |storage_handle| contains all the data, right? Don't we need plane offsets, > strides etc? Not yet. Offsets are not used in capture and the strides are the |coded_size|. https://codereview.chromium.org/1699553002/diff/500001/media/capture/interfac... media/capture/interfaces/video_capture.mojom:52: mojo.Rect visible_rect; On 2016/03/04 15:34:02, perkj_chrome wrote: > Do we use this now? Can't we add this if we ever need it with Video capture > devices? (Some) ChromeOS devices actually use this field (I tried to remove it altogether a while ago). https://codereview.chromium.org/1699553002/diff/500001/media/capture/interfac... media/capture/interfaces/video_capture.mojom:57: int64 timestamp; On 2016/03/04 15:34:02, perkj_chrome wrote: > Why can't this be TimeTicks then instead of int64? There are no marshalling primitives for TimeTicks yet in mojo (think about this definition as an IPC struct). https://codereview.chromium.org/1699553002/diff/500001/media/capture/interfac... media/capture/interfaces/video_capture.mojom:64: FrameAvailable(FrameInfo info) => (); On 2016/03/04 15:34:02, perkj_chrome wrote: > Should this be OnFrameAvailable to be consistent with other callback interfaces? > Done. > > Or is there another naming convention for mojo? > > Or maybe even better. use the same as > // VideoCaptureDeviceClientImpl::Delegate > void OnFrame(const scoped_refptr<MojoVideoFrame>& frame) > void OnError(const std::string& reason) override; Wouldn't that be confusing? https://codereview.chromium.org/1699553002/diff/500001/media/capture/interfac... media/capture/interfaces/video_capture.mojom:66: // Some Error has happened; the capture will in all likelihood be interrupted. On 2016/03/04 15:34:02, perkj_chrome wrote: > Please decide what will happen when an error occur. I think it should be stopped > and that this VideoCaptureStreamClient is no longer usable. The client doesn't need to know, because is down to the implementation to decide if the error is recoverable or not. That's why the Client interface has only FrameAvailable() and Error() methods. The client would only, maybe, see that there are no more FrameAvailable() events. https://codereview.chromium.org/1699553002/diff/500001/media/capture/interfac... media/capture/interfaces/video_capture.mojom:66: // Some Error has happened; the capture will in all likelihood be interrupted. On 2016/03/04 15:34:02, perkj_chrome wrote: > nit: Suggest - An error has occurred..... Done. https://codereview.chromium.org/1699553002/diff/500001/media/capture/interfac... media/capture/interfaces/video_capture.mojom:83: // component. This can used by a UserMediaClient implementation to get hold of a On 2016/03/04 15:34:02, perkj_chrome wrote: > If this is browser side only this will not be used by UserMediaClient. Done. https://codereview.chromium.org/1699553002/diff/500001/media/capture/interfac... media/capture/interfaces/video_capture.mojom:92: RequestVideoCaptureStream(StreamOptions options, string security_origin) On 2016/03/04 15:34:02, perkj_chrome wrote: > why security_origin if this is browser side only? Reminder of where this all started, months and months ago. Removed. https://codereview.chromium.org/1699553002/diff/500001/media/capture/service/... File media/capture/service/stream_impl.cc (right): https://codereview.chromium.org/1699553002/diff/500001/media/capture/service/... media/capture/service/stream_impl.cc:92: in_use_video_frames_.insert(std::make_pair(buffer_id, frame)); On 2016/03/04 15:41:42, magjed_chromium wrote: > Use this instead: in_use_video_frames_.emplace(buffer_id, frame); Uuuw yeah, now we have emplace() ! When this CL started, *2 months* ago, it wasn't allowed. https://codereview.chromium.org/1699553002/diff/500001/media/capture/service/... media/capture/service/stream_impl.cc:110: DCHECK(video_frame == in_use_video_frames_.end()) << "Buffer id " << buffer_id On 2016/03/04 15:41:42, magjed_chromium wrote: > Shouldn't this be DCHECK(video_frame != in_use_video_frames_.end())? Yes thanks. > Is this code executed in any test? This method is exercised in both StreamImplTest and VideoCaptureTest. https://codereview.chromium.org/1699553002/diff/500001/media/capture/service/... File media/capture/service/stream_impl.h (right): https://codereview.chromium.org/1699553002/diff/500001/media/capture/service/... media/capture/service/stream_impl.h:80: bool paused_ = false; On 2016/03/04 15:34:02, perkj_chrome wrote: > not used ? Done. https://codereview.chromium.org/1699553002/diff/500001/media/capture/service/... media/capture/service/stream_impl.h:85: std::map<int32_t, const scoped_refptr<MojoVideoFrame>&> in_use_video_frames_; On 2016/03/04 15:41:42, magjed_chromium wrote: > nit: I would prefer the variable name |video_frames_in_use_| instead. Then we'll change it of course. https://codereview.chromium.org/1699553002/diff/500001/media/capture/service/... media/capture/service/stream_impl.h:85: std::map<int32_t, const scoped_refptr<MojoVideoFrame>&> in_use_video_frames_; On 2016/03/04 15:41:42, magjed_chromium wrote: > The value type in this std::map is a const-ref, which looks super dangerous. > Shouldn't it be a scoped_refptr<MojoVideoFrame>, i.e. a non-ref? Done. > Also, I don't > see the frames being used anywhere, so why not use a set instead of a map? Is it > a map to keep the frames alive? We search for the |buffer_id| in OnFrameConsumed(). const auto video_frame = video_frames_in_use_.find(buffer_id); https://codereview.chromium.org/1699553002/diff/500001/media/capture/service/... File media/capture/service/video_capture_device_client_impl.cc (right): https://codereview.chromium.org/1699553002/diff/500001/media/capture/service/... media/capture/service/video_capture_device_client_impl.cc:214: dimensions, media::PIXEL_FORMAT_I420, media::PIXEL_STORAGE_CPU); On 2016/03/04 15:41:42, magjed_chromium wrote: > Would it be possible to use GPU memory buffers like in > content::VideoCaptureDeviceClient? GMBs are only used behind a flag and in an experimental fashion. I don't this is worth the added complexity. Also, how would you allocate them? You can't easily access the BrowserGpumemoryBufferManager from here and round trips to the GPU process will imply creating a context etc.
https://codereview.chromium.org/1699553002/diff/520001/media/capture/service/... File media/capture/service/stream_impl.cc (right): https://codereview.chromium.org/1699553002/diff/520001/media/capture/service/... media/capture/service/stream_impl.cc:92: in_use_video_frames_.emplace(buffer_id, frame); Some builders still fail with "../../media/capture/service/stream_impl.cc:92:24: error: no member named 'emplace' in 'std::map<int, scoped_refptr<media::MojoVideoFrame>, std::less<int>, std::allocator<std::pair<const int, scoped_refptr<media::MojoVideoFrame> > > >' " so I guess we don't have emplace() yet in that platform.
https://codereview.chromium.org/1699553002/diff/500001/media/capture/interfac... File media/capture/interfaces/video_capture.mojom (right): https://codereview.chromium.org/1699553002/diff/500001/media/capture/interfac... media/capture/interfaces/video_capture.mojom:36: handle<shared_buffer> storage_handle; On 2016/03/04 19:59:45, mcasas wrote: > On 2016/03/04 15:41:42, magjed_chromium wrote: > > |storage_handle| contains all the data, right? Don't we need plane offsets, > > strides etc? > > Not yet. Offsets are not used in capture and > the strides are the |coded_size|. With plane offsets I mean: how do you calculate the pointer for the U and V plane from |storage_handle|? When you say that the strides are in |coded_size|, do you mean that everything is tightly packed? Otherwise strides are not the same as |coded_size|, that's why we have both strides and coded_size in VideoFrame. https://codereview.chromium.org/1699553002/diff/500001/media/capture/service/... File media/capture/service/stream_impl.cc (right): https://codereview.chromium.org/1699553002/diff/500001/media/capture/service/... media/capture/service/stream_impl.cc:110: DCHECK(video_frame == in_use_video_frames_.end()) << "Buffer id " << buffer_id On 2016/03/04 19:59:45, mcasas wrote: > On 2016/03/04 15:41:42, magjed_chromium wrote: > > Shouldn't this be DCHECK(video_frame != in_use_video_frames_.end())? > > Yes thanks. > > > Is this code executed in any test? > > This method is exercised in both StreamImplTest and > VideoCaptureTest. Then why didn't the tests fail? https://codereview.chromium.org/1699553002/diff/500001/media/capture/service/... File media/capture/service/stream_impl.h (right): https://codereview.chromium.org/1699553002/diff/500001/media/capture/service/... media/capture/service/stream_impl.h:85: std::map<int32_t, const scoped_refptr<MojoVideoFrame>&> in_use_video_frames_; On 2016/03/04 19:59:45, mcasas wrote: > On 2016/03/04 15:41:42, magjed_chromium wrote: > > The value type in this std::map is a const-ref, which looks super dangerous. > > Shouldn't it be a scoped_refptr<MojoVideoFrame>, i.e. a non-ref? > > Done. > > > Also, I don't > > see the frames being used anywhere, so why not use a set instead of a map? Is > it > > a map to keep the frames alive? > > We search for the |buffer_id| in OnFrameConsumed(). > const auto video_frame = video_frames_in_use_.find(buffer_id); Yes, but you never access the mapped frame in the |video_frame| iterator which is of type std::pair<const key_type,mapped_type>, where key_type is int32_t and mapped_type is a MojoVideoFrame. Why have a mapped object if you don't access it? You can use the exact same code video_frames_in_use_.find(buffer_id); where |video_frames_in_use_| is a set<int32_t>. https://codereview.chromium.org/1699553002/diff/560001/media/capture/service/... File media/capture/service/video_capture_device_client_impl.cc (right): https://codereview.chromium.org/1699553002/diff/560001/media/capture/service/... media/capture/service/video_capture_device_client_impl.cc:120: // N.B.: All of the logic throughout this method is lifted directly from Can you split out the duplicated logic into a function instead? It looks like the only difference with existing VideoCaptureDeviceClient:OnIncomingCapturedData() is how you reserve the output buffer and that you don't use an external jpeg decoder. I think you can refactor the bulk of this function into: bool ConvertToPackedI420( const uint8_t* src_data, int src_length, uint8* dst_y, uint8* dst_u, uint8* dst_v, const media::VideoCaptureFormat& frame_format, int clockwise_rotation);
https://codereview.chromium.org/1699553002/diff/500001/media/capture/interfac... File media/capture/interfaces/video_capture.mojom (right): https://codereview.chromium.org/1699553002/diff/500001/media/capture/interfac... media/capture/interfaces/video_capture.mojom:36: handle<shared_buffer> storage_handle; On 2016/03/07 15:36:24, magjed_chromium wrote: > On 2016/03/04 19:59:45, mcasas wrote: > > On 2016/03/04 15:41:42, magjed_chromium wrote: > > > |storage_handle| contains all the data, right? Don't we need plane offsets, > > > strides etc? > > > > Not yet. Offsets are not used in capture and > > the strides are the |coded_size|. > > With plane offsets I mean: how do you calculate the pointer for the U and V > plane from |storage_handle|? When you say that the strides are in |coded_size|, > do you mean that everything is tightly packed? Yes. > Otherwise strides are not the > same as |coded_size|, that's why we have both strides and coded_size in > VideoFrame. Correct. But that capability of VideoFrame is not used for capturing scenarios. https://codereview.chromium.org/1699553002/diff/500001/media/capture/service/... File media/capture/service/stream_impl.cc (right): https://codereview.chromium.org/1699553002/diff/500001/media/capture/service/... media/capture/service/stream_impl.cc:110: DCHECK(video_frame == in_use_video_frames_.end()) << "Buffer id " << buffer_id On 2016/03/07 15:36:24, magjed_chromium wrote: > On 2016/03/04 19:59:45, mcasas wrote: > > On 2016/03/04 15:41:42, magjed_chromium wrote: > > > Shouldn't this be DCHECK(video_frame != in_use_video_frames_.end())? > > > > Yes thanks. > > > > > Is this code executed in any test? > > > > This method is exercised in both StreamImplTest and > > VideoCaptureTest. > > Then why didn't the tests fail? The capture tests are only compiled in gn builds and probably those do not exercise the full set of unittests. https://codereview.chromium.org/1699553002/diff/500001/media/capture/service/... File media/capture/service/stream_impl.h (right): https://codereview.chromium.org/1699553002/diff/500001/media/capture/service/... media/capture/service/stream_impl.h:85: std::map<int32_t, const scoped_refptr<MojoVideoFrame>&> in_use_video_frames_; On 2016/03/07 15:36:24, magjed_chromium wrote: > On 2016/03/04 19:59:45, mcasas wrote: > > On 2016/03/04 15:41:42, magjed_chromium wrote: > > > The value type in this std::map is a const-ref, which looks super dangerous. > > > Shouldn't it be a scoped_refptr<MojoVideoFrame>, i.e. a non-ref? > > > > Done. > > > > > Also, I don't > > > see the frames being used anywhere, so why not use a set instead of a map? > Is > > it > > > a map to keep the frames alive? > > > > We search for the |buffer_id| in OnFrameConsumed(). > > const auto video_frame = video_frames_in_use_.find(buffer_id); > > Yes, but you never access the mapped frame in the |video_frame| iterator which > is of type std::pair<const key_type,mapped_type>, where key_type is int32_t and > mapped_type is a MojoVideoFrame. Why have a mapped object if you don't access > it? You can use the exact same code video_frames_in_use_.find(buffer_id); where > |video_frames_in_use_| is a set<int32_t>. Maybe we're not understanding how I thought this was supposed to work :) VCDCImpl has a pool of Mojo...VideoFrames. Each time a frame of this type is created, an associated MojoSharedBuffer is created and mapped, and remains so during the lifetime of the said VideoFrame. A VF can only be inside the Pool, hence "available" or not, and then it's in use. When a VF is taken out of the pool, StreamImpl receives it and assigns a number to it; this number is bound to the release callback. Only the constituents of the VideoFrame are sent across the Mojo channel, where each StreamClient(s) will reconstruct a peer VideoFrame. The original VideoFrame must stay alive and "in use" until the StreamClient is finished. When the client is finished with the VideoFrame, it will ping us back via OnFrameConsumed() callback where the |buffer_id| is bound. Then we find the VideoFrame by id and remove it from the list. We need to keep the VideoFrame alive on this side of the Mojo boundary, hence I chose to keep them all lined up in a map. So basically a VF is either in the VCDCImpl's Pool or is kept here until the Mojo clients are finished. Much simpler than the current design in renderer_host. (Ok, is slightly more complex because the VFs being taken out of the Pool are wrapped into another VF.) So I'm not sure what you don't grok. Is it that you'd like to remove the |in_use_video_frames_| and bind the VideoFrame themselves to the release callback...? https://codereview.chromium.org/1699553002/diff/560001/media/capture/service/... File media/capture/service/video_capture_device_client_impl.cc (right): https://codereview.chromium.org/1699553002/diff/560001/media/capture/service/... media/capture/service/video_capture_device_client_impl.cc:120: // N.B.: All of the logic throughout this method is lifted directly from On 2016/03/07 15:36:24, magjed_chromium wrote: > Can you split out the duplicated logic into a function instead? It looks like > the only difference with existing > VideoCaptureDeviceClient:OnIncomingCapturedData() is how you reserve the output > buffer and that you don't use an external jpeg decoder. I think you can refactor > the bulk of this function into: > bool ConvertToPackedI420( > const uint8_t* src_data, > int src_length, > uint8* dst_y, > uint8* dst_u, > uint8* dst_v, > const media::VideoCaptureFormat& frame_format, > int clockwise_rotation); Are you proposing I take out the common parts into a method (e.g. of VideoCaptureDevice) that can be used from here and from content/? What would be the final goal of such a move? Note that, in time, the implementation here should be the norm and the one in content/ should be deprecated. Besides, like you say, here no MJPEG is supported and in content/, ReserveI420OutputBuffer() is used instead. IMHO there are too many differences, and I'm not looking forward to another preparatory CL taking a week to brief in all the reviewers, review and land.
A few more. https://codereview.chromium.org/1699553002/diff/500001/media/capture/interfac... File media/capture/interfaces/video_capture.mojom (right): https://codereview.chromium.org/1699553002/diff/500001/media/capture/interfac... media/capture/interfaces/video_capture.mojom:57: int64 timestamp; On 2016/03/04 19:59:45, mcasas wrote: > On 2016/03/04 15:34:02, perkj_chrome wrote: > > Why can't this be TimeTicks then instead of int64? > > There are no marshalling primitives for TimeTicks > yet in mojo (think about this definition as an IPC > struct). Acknowledged. https://codereview.chromium.org/1699553002/diff/560001/media/capture/interfac... File media/capture/interfaces/video_capture.mojom (right): https://codereview.chromium.org/1699553002/diff/560001/media/capture/interfac... media/capture/interfaces/video_capture.mojom:66: // An error has ocurred; the capture will in all likelihood be interrupted. So to continu the discussion. What is the client supposed to do when this method is called? ie Should the end result be that the track state is changed to ended in js? If so - I think this comment should make it clear that this client will no longer be usable after this have happened. If I understand the implementation, that is what happens ? https://codereview.chromium.org/1699553002/diff/560001/media/capture/service/... File media/capture/service/stream_impl.cc (right): https://codereview.chromium.org/1699553002/diff/560001/media/capture/service/... media/capture/service/stream_impl.cc:41: start_callback_.Run(); This callback seems pretty useless. The implementation in VideoCaptureHandlerImpl::OnStart do nothing. https://codereview.chromium.org/1699553002/diff/560001/media/capture/service/... File media/capture/service/stream_impl.h (right): https://codereview.chromium.org/1699553002/diff/560001/media/capture/service/... media/capture/service/stream_impl.h:32: class StreamImpl : public mojom::VideoCaptureStream, yes please change name of this class too. https://codereview.chromium.org/1699553002/diff/560001/media/capture/service/... media/capture/service/stream_impl.h:36: // |close_callback| is called when this object is stopped by its client. nit: stop_callback? https://codereview.chromium.org/1699553002/diff/560001/media/capture/service/... media/capture/service/stream_impl.h:40: const base::Closure& close_callback, dito? https://codereview.chromium.org/1699553002/diff/560001/media/capture/service/... media/capture/service/stream_impl.h:57: // The following Notify* methods mimic those in StreamClient and act as a Update this comment after StreamClient name change. https://codereview.chromium.org/1699553002/diff/560001/media/capture/service/... media/capture/service/stream_impl.h:74: const base::Closure close_callback_; nit: stop_callback_ https://codereview.chromium.org/1699553002/diff/560001/media/capture/service/... File media/capture/service/video_capture_handler_impl.cc (right): https://codereview.chromium.org/1699553002/diff/560001/media/capture/service/... media/capture/service/video_capture_handler_impl.cc:58: if (!device_names_->empty()) { ? So if this is called before video_capture_device_factory_->EnumerateDeviceNames has completed this will immediately return an empty list of devices instead of waiting? wouldnt it be easier to skip base::SystemMonitor::Get()->AddDevicesChangedObserver for now if it does not work anyway and make sure EnumerateDevices actually enumerate correctly here every time someone call this method. https://codereview.chromium.org/1699553002/diff/560001/media/capture/service/... media/capture/service/video_capture_handler_impl.cc:63: // Send a GUID of the Video Capture id to hide sensitive information. Why would this be sensitive? Doesnt this mean you would get different device id for each call to EnumerateDevices? https://codereview.chromium.org/1699553002/diff/560001/media/capture/service/... media/capture/service/video_capture_handler_impl.cc:99: // TODO(mcasas): At this point we need to ask the user for permission to use This must be handled outside this mojo and is not a problem if this will only be used from the browser process. So I think you can remove this comment. https://codereview.chromium.org/1699553002/diff/560001/media/capture/service/... media/capture/service/video_capture_handler_impl.cc:119: video_capture_device_factory_->Create(*name); So this means there can only be one VideoCaptureStream and VideoCaptureStreamClient per physical video camera? So you will still do muxing somewhere in the browser process to support multiple renderers? I guess I still dont understand how this will be used.
Patchset #11 (id:600001) has been deleted
Description was changed from ========== Mojo Video Capture service in media/capture DD: https://goo.gl/Bsmk4u (Iteration from http://crrev.com/1637793004, #ps4) This CL: - Adds media/capture/interfaces and media/capture/service. - new source_set("service") for service/ (mojo module) - has its own capture_unittests Still depending on http://crrev.com/1704273002 that will introduce a MojoVideoFrame that'll make the current one here unnecessary. BUG=584797 TEST= ./capture_service_unittests ========== to ========== Mojo Video Capture service in media/capture DD: https://goo.gl/Bsmk4u (Iteration from http://crrev.com/1637793004, #ps4) This CL: - Adds media/capture/interfaces and media/capture/service. - new source_set("service") for service/ (mojo module) - has its own capture_unittests BUG=584797 TEST= ./video_capture_service_unittests ==========
Patchset #10 (id:580001) has been deleted
perkj@ PTAL https://codereview.chromium.org/1699553002/diff/560001/media/capture/interfac... File media/capture/interfaces/video_capture.mojom (right): https://codereview.chromium.org/1699553002/diff/560001/media/capture/interfac... media/capture/interfaces/video_capture.mojom:66: // An error has ocurred; the capture will in all likelihood be interrupted. On 2016/03/07 23:58:27, perkj_chrome wrote: > So to continu the discussion. What is the client supposed to do when this method > is called? ie Should the end result be that the track state is changed to ended > in js? If so - I think this comment should make it clear that this client will > no longer be usable after this have happened. If I understand the > implementation, that is what happens ? Yes, you're right. Corrected. https://codereview.chromium.org/1699553002/diff/560001/media/capture/service/... File media/capture/service/stream_impl.cc (right): https://codereview.chromium.org/1699553002/diff/560001/media/capture/service/... media/capture/service/stream_impl.cc:41: start_callback_.Run(); On 2016/03/07 23:58:27, perkj_chrome wrote: > This callback seems pretty useless. The implementation in > VideoCaptureHandlerImpl::OnStart do nothing. Removed. https://codereview.chromium.org/1699553002/diff/560001/media/capture/service/... File media/capture/service/stream_impl.h (right): https://codereview.chromium.org/1699553002/diff/560001/media/capture/service/... media/capture/service/stream_impl.h:32: class StreamImpl : public mojom::VideoCaptureStream, On 2016/03/07 23:58:27, perkj_chrome wrote: > yes please change name of this class too. Done. https://codereview.chromium.org/1699553002/diff/560001/media/capture/service/... media/capture/service/stream_impl.h:36: // |close_callback| is called when this object is stopped by its client. On 2016/03/07 23:58:27, perkj_chrome wrote: > nit: stop_callback? Done. https://codereview.chromium.org/1699553002/diff/560001/media/capture/service/... media/capture/service/stream_impl.h:40: const base::Closure& close_callback, On 2016/03/07 23:58:27, perkj_chrome wrote: > dito? Done. https://codereview.chromium.org/1699553002/diff/560001/media/capture/service/... media/capture/service/stream_impl.h:57: // The following Notify* methods mimic those in StreamClient and act as a On 2016/03/07 23:58:27, perkj_chrome wrote: > Update this comment after StreamClient name change. Done. https://codereview.chromium.org/1699553002/diff/560001/media/capture/service/... media/capture/service/stream_impl.h:74: const base::Closure close_callback_; On 2016/03/07 23:58:27, perkj_chrome wrote: > nit: stop_callback_ Done. https://codereview.chromium.org/1699553002/diff/560001/media/capture/service/... File media/capture/service/video_capture_handler_impl.cc (right): https://codereview.chromium.org/1699553002/diff/560001/media/capture/service/... media/capture/service/video_capture_handler_impl.cc:58: if (!device_names_->empty()) { On 2016/03/07 23:58:27, perkj_chrome wrote: > ? So if this is called before > video_capture_device_factory_->EnumerateDeviceNames has completed this will > immediately return an empty list of devices instead of waiting? That should never happen because the Monitors start enumeration way before the Mojo component is created, but more importantly, the Enumeration in this Mojo service is synchronous. Why? The VideoCaptureDeviceFactory uses 2 threads: the "ui" and the one where it is created. Here, both are the same. So, all operations are synchronous. > > wouldnt it be easier to skip > base::SystemMonitor::Get()->AddDevicesChangedObserver for now if it does not > work anyway and make sure EnumerateDevices actually enumerate correctly here > every time someone call this method. https://codereview.chromium.org/1699553002/diff/560001/media/capture/service/... media/capture/service/video_capture_handler_impl.cc:63: // Send a GUID of the Video Capture id to hide sensitive information. On 2016/03/07 23:58:27, perkj_chrome wrote: > Why would this be sensitive? You can't send the OS-specific device ID to renderer, i.e. /dev/video0 cannot/should not be sent to the renderer, right? Happy to move the public-private naming dictionary to renderer :) > Doesnt this mean you would get different device id for each call to > EnumerateDevices? So far yes. But I understand this IDs are not persistent, i.e. they are only alive in until the next enumeration. Added a TODO. https://codereview.chromium.org/1699553002/diff/560001/media/capture/service/... media/capture/service/video_capture_handler_impl.cc:99: // TODO(mcasas): At this point we need to ask the user for permission to use On 2016/03/07 23:58:27, perkj_chrome wrote: > This must be handled outside this mojo and is not a problem if this will only be > used from the browser process. So I think you can remove this comment. Done. https://codereview.chromium.org/1699553002/diff/560001/media/capture/service/... media/capture/service/video_capture_handler_impl.cc:119: video_capture_device_factory_->Create(*name); On 2016/03/07 23:58:27, perkj_chrome wrote: > So this means there can only be one VideoCaptureStream and > VideoCaptureStreamClient per physical video camera? This mirrors the current cardinality: every VCDevice has (on ToT) one VideoCaptureDeviceClient and a VideoCaptureController. Here we have a VCDClientImpl and a VideoCaptureStream. What is missing (added TODO), is to replicate the incoming VideoCapture for several StreamClients. Given the fact that capturing video in two tabs (two renderers) at once is a tiny use case, I consider this a reasonable limitation for this prototype. Added a TODO anyway. > So you will still do muxing > somewhere in the browser process to support multiple renderers? > > I guess I still dont understand how this will be used. Remember that all this VideoCapture service is a prototype and a first step. Limitations are coded in the form of TODOs. Code will likely evolve.
https://codereview.chromium.org/1699553002/diff/500001/media/capture/interfac... File media/capture/interfaces/video_capture.mojom (right): https://codereview.chromium.org/1699553002/diff/500001/media/capture/interfac... media/capture/interfaces/video_capture.mojom:36: handle<shared_buffer> storage_handle; On 2016/03/07 20:57:49, mcasas wrote: > On 2016/03/07 15:36:24, magjed_chromium wrote: > > On 2016/03/04 19:59:45, mcasas wrote: > > > On 2016/03/04 15:41:42, magjed_chromium wrote: > > > > |storage_handle| contains all the data, right? Don't we need plane > offsets, > > > > strides etc? > > > > > > Not yet. Offsets are not used in capture and > > > the strides are the |coded_size|. > > > > With plane offsets I mean: how do you calculate the pointer for the U and V > > plane from |storage_handle|? When you say that the strides are in > |coded_size|, > > do you mean that everything is tightly packed? > Yes. > > Otherwise strides are not the > > same as |coded_size|, that's why we have both strides and coded_size in > > VideoFrame. > Correct. But that capability of VideoFrame is not used > for capturing scenarios. Acknowledged. https://codereview.chromium.org/1699553002/diff/500001/media/capture/service/... File media/capture/service/stream_impl.cc (right): https://codereview.chromium.org/1699553002/diff/500001/media/capture/service/... media/capture/service/stream_impl.cc:110: DCHECK(video_frame == in_use_video_frames_.end()) << "Buffer id " << buffer_id On 2016/03/07 20:57:49, mcasas wrote: > On 2016/03/07 15:36:24, magjed_chromium wrote: > > On 2016/03/04 19:59:45, mcasas wrote: > > > On 2016/03/04 15:41:42, magjed_chromium wrote: > > > > Shouldn't this be DCHECK(video_frame != in_use_video_frames_.end())? > > > > > > Yes thanks. > > > > > > > Is this code executed in any test? > > > > > > This method is exercised in both StreamImplTest and > > > VideoCaptureTest. > > > > Then why didn't the tests fail? > > The capture tests are only compiled in gn builds > and probably those do not exercise the full set > of unittests. Ok. It's a bit worrying that a DCHECK with 100% failure ratio goes unnoticed. https://codereview.chromium.org/1699553002/diff/500001/media/capture/service/... File media/capture/service/stream_impl.h (right): https://codereview.chromium.org/1699553002/diff/500001/media/capture/service/... media/capture/service/stream_impl.h:85: std::map<int32_t, const scoped_refptr<MojoVideoFrame>&> in_use_video_frames_; On 2016/03/07 20:57:49, mcasas wrote: > On 2016/03/07 15:36:24, magjed_chromium wrote: > > On 2016/03/04 19:59:45, mcasas wrote: > > > On 2016/03/04 15:41:42, magjed_chromium wrote: > > > > The value type in this std::map is a const-ref, which looks super > dangerous. > > > > Shouldn't it be a scoped_refptr<MojoVideoFrame>, i.e. a non-ref? > > > > > > Done. > > > > > > > Also, I don't > > > > see the frames being used anywhere, so why not use a set instead of a map? > > Is > > > it > > > > a map to keep the frames alive? > > > > > > We search for the |buffer_id| in OnFrameConsumed(). > > > const auto video_frame = video_frames_in_use_.find(buffer_id); > > > > Yes, but you never access the mapped frame in the |video_frame| iterator which > > is of type std::pair<const key_type,mapped_type>, where key_type is int32_t > and > > mapped_type is a MojoVideoFrame. Why have a mapped object if you don't access > > it? You can use the exact same code video_frames_in_use_.find(buffer_id); > where > > |video_frames_in_use_| is a set<int32_t>. > > Maybe we're not understanding how I thought this > was supposed to work :) > VCDCImpl has a pool of Mojo...VideoFrames. Each > time a frame of this type is created, an associated > MojoSharedBuffer is created and mapped, and remains > so during the lifetime of the said VideoFrame. A VF > can only be inside the Pool, hence "available" or not, > and then it's in use. When a VF is taken out of the pool, > StreamImpl receives it and assigns a number to it; this > number is bound to the release callback. > > Only the constituents of the VideoFrame are sent across > the Mojo channel, where each StreamClient(s) will > reconstruct a peer VideoFrame. The original VideoFrame > must stay alive and "in use" until the StreamClient is finished. > > When the client is finished with the VideoFrame, it will ping > us back via OnFrameConsumed() callback where the > |buffer_id| is bound. Then we find the VideoFrame by > id and remove it from the list. We need to keep the > VideoFrame alive on this side of the Mojo boundary, > hence I chose to keep them all lined up in a map. > > So basically a VF is either in the VCDCImpl's Pool or > is kept here until the Mojo clients are finished. Much > simpler than the current design in renderer_host. > > (Ok, is slightly more complex because the VFs being > taken out of the Pool are wrapped into another VF.) > > So I'm not sure what you don't grok. Is it that > you'd like to remove the |in_use_video_frames_| > and bind the VideoFrame themselves to the release > callback...? I didn't grok the purpose of this map since you don't use the mapped frames. The only purpose I could think of was to keep the frames alive, but you captured the scoped_refptr by-ref so that would not help the frames stay alive. Can you add a comment that the purpose of the map is to keep the frames alive? It's also a bit worrying that the tests didn't catch that the frames were not kept alive. I guess the frames were returned immediately to the pool previously so that you would never allocate more than one frame. Can you add a test that checks that a new frame is received if the previous frame is not returned? Actually, your suggestion to bind the scoped_refptrs directly to the release callback seems cleaner if you don't plan to use |in_use_video_frames_| for anything else. https://codereview.chromium.org/1699553002/diff/560001/media/capture/service/... File media/capture/service/video_capture_device_client_impl.cc (right): https://codereview.chromium.org/1699553002/diff/560001/media/capture/service/... media/capture/service/video_capture_device_client_impl.cc:120: // N.B.: All of the logic throughout this method is lifted directly from On 2016/03/07 20:57:49, mcasas wrote: > On 2016/03/07 15:36:24, magjed_chromium wrote: > > Can you split out the duplicated logic into a function instead? It looks like > > the only difference with existing > > VideoCaptureDeviceClient:OnIncomingCapturedData() is how you reserve the > output > > buffer and that you don't use an external jpeg decoder. I think you can > refactor > > the bulk of this function into: > > bool ConvertToPackedI420( > > const uint8_t* src_data, > > int src_length, > > uint8* dst_y, > > uint8* dst_u, > > uint8* dst_v, > > const media::VideoCaptureFormat& frame_format, > > int clockwise_rotation); > > Are you proposing I take out the common parts > into a method (e.g. of VideoCaptureDevice) that > can be used from here and from content/? > What would be the final goal of such a move? Note > that, in time, the implementation here should be > the norm and the one in content/ should be > deprecated. Besides, like you say, here no MJPEG > is supported and in content/, > ReserveI420OutputBuffer() is used instead. > IMHO there are too many differences, and I'm > not looking forward to another preparatory CL > taking a week to brief in all the reviewers, review > and land. Yes, I'm proposing that you take out the common parts into a non-member method: bool ConvertToPackedI420( const uint8_t* src_data, int src_length, uint8* dst_y, uint8* dst_u, uint8* dst_v, const media::VideoCaptureFormat& frame_format, int clockwise_rotation); that you can use here and from content/. The goal with such a move is to reduce the ~100 duplicated lines of code here. I understand that you plan to eventually remove the implementation in content/, but I suspect it will take months before that happens. The differences, i.e. support for external MJPEG decoder and ReserveI420OutputBuffer() are outside the proposed new helper function, i.e. you call them before the helper function, so it's not a problem. Landing just the new helper function will not take a week. IMHO if you would split this huge CL up, it wouldn't be so difficult to review, and I don't think it would take much longer than landing it in one big chunk.
On 2016/03/09 14:11:18, magjed_chromium wrote: > https://codereview.chromium.org/1699553002/diff/500001/media/capture/interfac... > File media/capture/interfaces/video_capture.mojom (right): > > https://codereview.chromium.org/1699553002/diff/500001/media/capture/interfac... > media/capture/interfaces/video_capture.mojom:36: handle<shared_buffer> > storage_handle; > On 2016/03/07 20:57:49, mcasas wrote: > > On 2016/03/07 15:36:24, magjed_chromium wrote: > > > On 2016/03/04 19:59:45, mcasas wrote: > > > > On 2016/03/04 15:41:42, magjed_chromium wrote: > > > > > |storage_handle| contains all the data, right? Don't we need plane > > offsets, > > > > > strides etc? > > > > > > > > Not yet. Offsets are not used in capture and > > > > the strides are the |coded_size|. > > > > > > With plane offsets I mean: how do you calculate the pointer for the U and V > > > plane from |storage_handle|? When you say that the strides are in > > |coded_size|, > > > do you mean that everything is tightly packed? > > Yes. > > > Otherwise strides are not the > > > same as |coded_size|, that's why we have both strides and coded_size in > > > VideoFrame. > > Correct. But that capability of VideoFrame is not used > > for capturing scenarios. > > Acknowledged. > > https://codereview.chromium.org/1699553002/diff/500001/media/capture/service/... > File media/capture/service/stream_impl.cc (right): > > https://codereview.chromium.org/1699553002/diff/500001/media/capture/service/... > media/capture/service/stream_impl.cc:110: DCHECK(video_frame == > in_use_video_frames_.end()) << "Buffer id " << buffer_id > On 2016/03/07 20:57:49, mcasas wrote: > > On 2016/03/07 15:36:24, magjed_chromium wrote: > > > On 2016/03/04 19:59:45, mcasas wrote: > > > > On 2016/03/04 15:41:42, magjed_chromium wrote: > > > > > Shouldn't this be DCHECK(video_frame != in_use_video_frames_.end())? > > > > > > > > Yes thanks. > > > > > > > > > Is this code executed in any test? > > > > > > > > This method is exercised in both StreamImplTest and > > > > VideoCaptureTest. > > > > > > Then why didn't the tests fail? > > > > The capture tests are only compiled in gn builds > > and probably those do not exercise the full set > > of unittests. > > Ok. It's a bit worrying that a DCHECK with 100% failure ratio goes unnoticed. > > https://codereview.chromium.org/1699553002/diff/500001/media/capture/service/... > File media/capture/service/stream_impl.h (right): > > https://codereview.chromium.org/1699553002/diff/500001/media/capture/service/... > media/capture/service/stream_impl.h:85: std::map<int32_t, const > scoped_refptr<MojoVideoFrame>&> in_use_video_frames_; > On 2016/03/07 20:57:49, mcasas wrote: > > On 2016/03/07 15:36:24, magjed_chromium wrote: > > > On 2016/03/04 19:59:45, mcasas wrote: > > > > On 2016/03/04 15:41:42, magjed_chromium wrote: > > > > > The value type in this std::map is a const-ref, which looks super > > dangerous. > > > > > Shouldn't it be a scoped_refptr<MojoVideoFrame>, i.e. a non-ref? > > > > > > > > Done. > > > > > > > > > Also, I don't > > > > > see the frames being used anywhere, so why not use a set instead of a > map? > > > Is > > > > it > > > > > a map to keep the frames alive? > > > > > > > > We search for the |buffer_id| in OnFrameConsumed(). > > > > const auto video_frame = video_frames_in_use_.find(buffer_id); > > > > > > Yes, but you never access the mapped frame in the |video_frame| iterator > which > > > is of type std::pair<const key_type,mapped_type>, where key_type is int32_t > > and > > > mapped_type is a MojoVideoFrame. Why have a mapped object if you don't > access > > > it? You can use the exact same code video_frames_in_use_.find(buffer_id); > > where > > > |video_frames_in_use_| is a set<int32_t>. > > > > Maybe we're not understanding how I thought this > > was supposed to work :) > > VCDCImpl has a pool of Mojo...VideoFrames. Each > > time a frame of this type is created, an associated > > MojoSharedBuffer is created and mapped, and remains > > so during the lifetime of the said VideoFrame. A VF > > can only be inside the Pool, hence "available" or not, > > and then it's in use. When a VF is taken out of the pool, > > StreamImpl receives it and assigns a number to it; this > > number is bound to the release callback. > > > > Only the constituents of the VideoFrame are sent across > > the Mojo channel, where each StreamClient(s) will > > reconstruct a peer VideoFrame. The original VideoFrame > > must stay alive and "in use" until the StreamClient is finished. > > > > When the client is finished with the VideoFrame, it will ping > > us back via OnFrameConsumed() callback where the > > |buffer_id| is bound. Then we find the VideoFrame by > > id and remove it from the list. We need to keep the > > VideoFrame alive on this side of the Mojo boundary, > > hence I chose to keep them all lined up in a map. > > > > So basically a VF is either in the VCDCImpl's Pool or > > is kept here until the Mojo clients are finished. Much > > simpler than the current design in renderer_host. > > > > (Ok, is slightly more complex because the VFs being > > taken out of the Pool are wrapped into another VF.) > > > > So I'm not sure what you don't grok. Is it that > > you'd like to remove the |in_use_video_frames_| > > and bind the VideoFrame themselves to the release > > callback...? > > I didn't grok the purpose of this map since you don't use the mapped frames. The > only purpose I could think of was to keep the frames alive, but you captured the > scoped_refptr by-ref so that would not help the frames stay alive. Can you add a > comment that the purpose of the map is to keep the frames alive? It's also a bit > worrying that the tests didn't catch that the frames were not kept alive. I > guess the frames were returned immediately to the pool previously so that you > would never allocate more than one frame. Can you add a test that checks that a > new frame is received if the previous frame is not returned? > > Actually, your suggestion to bind the scoped_refptrs directly to the release > callback seems cleaner if you don't plan to use |in_use_video_frames_| for > anything else. > > https://codereview.chromium.org/1699553002/diff/560001/media/capture/service/... > File media/capture/service/video_capture_device_client_impl.cc (right): > > https://codereview.chromium.org/1699553002/diff/560001/media/capture/service/... > media/capture/service/video_capture_device_client_impl.cc:120: // N.B.: All of > the logic throughout this method is lifted directly from > On 2016/03/07 20:57:49, mcasas wrote: > > On 2016/03/07 15:36:24, magjed_chromium wrote: > > > Can you split out the duplicated logic into a function instead? It looks > like > > > the only difference with existing > > > VideoCaptureDeviceClient:OnIncomingCapturedData() is how you reserve the > > output > > > buffer and that you don't use an external jpeg decoder. I think you can > > refactor > > > the bulk of this function into: > > > bool ConvertToPackedI420( > > > const uint8_t* src_data, > > > int src_length, > > > uint8* dst_y, > > > uint8* dst_u, > > > uint8* dst_v, > > > const media::VideoCaptureFormat& frame_format, > > > int clockwise_rotation); > > > > Are you proposing I take out the common parts > > into a method (e.g. of VideoCaptureDevice) that > > can be used from here and from content/? > > What would be the final goal of such a move? Note > > that, in time, the implementation here should be > > the norm and the one in content/ should be > > deprecated. Besides, like you say, here no MJPEG > > is supported and in content/, > > ReserveI420OutputBuffer() is used instead. > > IMHO there are too many differences, and I'm > > not looking forward to another preparatory CL > > taking a week to brief in all the reviewers, review > > and land. > > Yes, I'm proposing that you take out the common parts into a non-member method: > bool ConvertToPackedI420( > const uint8_t* src_data, > int src_length, > uint8* dst_y, > uint8* dst_u, > uint8* dst_v, > const media::VideoCaptureFormat& frame_format, > int clockwise_rotation); > that you can use here and from content/. > The goal with such a move is to reduce the ~100 duplicated lines of code here. I > understand that you plan to eventually remove the implementation in content/, > but I suspect it will take months before that happens. The differences, i.e. > support for external MJPEG decoder and ReserveI420OutputBuffer() are outside the > proposed new helper function, i.e. you call them before the helper function, so > it's not a problem. Landing just the new helper function will not take a week. > IMHO if you would split this huge CL up, it wouldn't be so difficult to review, > and I don't think it would take much longer than landing it in one big chunk. I won't be having time for this CL any time soon, so closing it. Thanks for your time reviewers!
Message was sent while issue was closed.
mcasas@chromium.org changed reviewers: - emircan@chromium.org, magjed@chromium.org, perkj@chromium.org, rockot@chromium.org, tommi@chromium.org |
