Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2837273004:
media: add video capture device for ARC++ camera HAL v3 (Closed)
|
Created:
3 years, 8 months ago by jcliang Modified:
3 years, 6 months ago Reviewers:
Ken Rockot(use gerrit already), reveman, kenrb, wuchengli, yzshen1, Pawel Osciak, chfremer CC:
Aaron Boodman, abarth-chromium, chfremer+watch_chromium.org, chromium-reviews, darin (slow to review), emircan, feature-media-reviews_chromium.org, mcasas, miu+watch_chromium.org, oshima+watch_chromium.org, posciak+watch_chromium.org, Pawel Osciak, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, xjz+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptionmedia: add video capture device for ARC++ camera HAL v3
This CL adds VideoCaptureDevice and VideoCaptureDeviceFactory
for the ARC++ camera HAL v3. The VCD and VCD factory talk to
the HAL adapter process on Chrome OS throgh Mojo IPC to access
the camera service.
BUG=b:32690003
TEST=Make sure camera preview works in hangout.
TEST=unit tests
Review-Url: https://codereview.chromium.org/2837273004
Cr-Commit-Position: refs/heads/master@{#479662}
Committed: https://chromium.googlesource.com/chromium/src/+/b2b84801bfe858bfe7e568354cb3ca3f2578e78e
Patch Set 1 #
Total comments: 6
Patch Set 2 : WIP: media: add video capture device for ARC++ camera HAL v3 #
Total comments: 16
Patch Set 3 : address chfremer@'s review comments #
Total comments: 18
Patch Set 4 : address wuchengli@'s comments #Patch Set 5 : revise some code comments #
Total comments: 36
Patch Set 6 : address kenrb@'s review comments #Patch Set 7 : remove external camera handling in camera_hal_delegate.* #
Total comments: 27
Patch Set 8 : handle error_msg in Notify #
Total comments: 5
Patch Set 9 : address wuchengli@'s comments #
Total comments: 34
Patch Set 10 : address wuchengli@'s comments #Patch Set 11 : update include_rules in DEPS #Patch Set 12 : WIP: media: Add ArcCamera3Service Mojo proxy #Patch Set 13 : restore overridden patch set #Patch Set 14 : update ConfigureStreams mojo interface #
Total comments: 44
Patch Set 15 : address wuchengli@'s comments #
Total comments: 30
Patch Set 16 : address wuchengli@'s comments #
Total comments: 91
Patch Set 17 : address wuchengli's and chfremer's comments #
Total comments: 47
Patch Set 18 : extract ScreenObserverDelegate to display_rotation_observer.h/.cc #Patch Set 19 : address chfremer's comments #
Total comments: 28
Patch Set 20 : address chfremer's comments #Patch Set 21 : add more device delegate test cases #
Total comments: 6
Patch Set 22 : address chfremer's comments #
Total comments: 11
Patch Set 23 : add unit tests for stream_buffer_manager #Patch Set 24 : refactor StreamCaptureInterface and fix up comments #Patch Set 25 : rebase on latest patch set of issue 2837273004 #Patch Set 26 : restore patch set 24 #
Total comments: 20
Patch Set 27 : address chfremer's comments #
Total comments: 20
Patch Set 28 : rebase on ToT and address wuchengli's comments #Patch Set 29 : patch BUILD.gn and sync.c in third_party/libsync #Patch Set 30 : fix chromium style checker errors #Patch Set 31 : extract libsync changes to another CL #Patch Set 32 : set CL dependency #
Total comments: 68
Patch Set 33 : address posciak's comments #Patch Set 34 : rebase on ToT #Patch Set 35 : fix style checker errors #
Total comments: 10
Patch Set 36 : address posciak's comments #
Total comments: 2
Patch Set 37 : RELAND: media: add video capture device for ARC++ camera HAL v3 #Messages
Total messages: 122 (51 generated)
Description was changed from ========== WIP: media: add video capture device for ARC++ camera HAL v3 This CL adds VideoCaptureDevice and VideoCaptureDeviceFactory for the ARC++ camera HAL v3. The VCD and VCD factory talk to the HAL adapter process on Chrome OS throgh Mojo IPC to access the camera service. BUG=b:32690003 TEST=Make sure camera preview works in hangout. ========== to ========== WIP: media: add video capture device for ARC++ camera HAL v3 This CL adds VideoCaptureDevice and VideoCaptureDeviceFactory for the ARC++ camera HAL v3. The VCD and VCD factory talk to the HAL adapter process on Chrome OS throgh Mojo IPC to access the camera service. BUG=b:32690003 TEST=Make sure camera preview works in hangout. ==========
jcliang@chromium.org changed reviewers: + henryhsu@chromium.org, posciak@chromium.org, wuchengli@chromium.org
This is the WIP version of the camera HAL v3 video capture device. I'm uploading it early to collect feedback to make sure I'm on the right direction. I still need to finish some TODOs listed in the CL, and add some tests. I've manually tested with USB camera HAL v3 with this CL and camera preview works in Chrome.
jcliang@chromium.org changed reviewers: + chfremer@chromium.org, emircan@chromium.org, mcasas@chromium.org
Just a quick few first comments. I haven't been able to go through the complete code yet. https://codereview.chromium.org/2837273004/diff/1/media/capture/video/chromeo... File media/capture/video/chromeos/video_capture_device_arc_chromeos.cc (right): https://codereview.chromium.org/2837273004/diff/1/media/capture/video/chromeo... media/capture/video/chromeos/video_capture_device_arc_chromeos.cc:19: class VideoCaptureDeviceArcChromeOS::ScreenObserverDelegate Please add comment explaining why the raw pointer |capture_device_| is safe to use. https://codereview.chromium.org/2837273004/diff/1/media/capture/video/chromeo... media/capture/video/chromeos/video_capture_device_arc_chromeos.cc:141: DCHECK(!camera_device_delegate_); Would we be hitting this when a client calls AllocateAndStart() directly after a call to StopAndDeAllocate()? It seems that the media::VideoCaptureDevice API does not have a mechanism to tell clients when it is safe again to call AllocateAndStart() after a StopAndDeAllocate(). According to the method-level comment at media::VideoCaptureDevice::StopAndDeAllocate(), the implementation is expected to delay (probably enqueue) the device restart until the device has finished stopping. https://codereview.chromium.org/2837273004/diff/1/media/capture/video/chromeo... File media/capture/video/chromeos/video_capture_device_factory_chromeos.cc (right): https://codereview.chromium.org/2837273004/diff/1/media/capture/video/chromeo... media/capture/video/chromeos/video_capture_device_factory_chromeos.cc:15: bool VideoCaptureDeviceFactoryChromeOS::Init() { Add DCHECK(thread_checker_.CalledOnValidThread());?
https://codereview.chromium.org/2837273004/diff/1/media/capture/video/chromeo... File media/capture/video/chromeos/video_capture_device_arc_chromeos.cc (right): https://codereview.chromium.org/2837273004/diff/1/media/capture/video/chromeo... media/capture/video/chromeos/video_capture_device_arc_chromeos.cc:19: class VideoCaptureDeviceArcChromeOS::ScreenObserverDelegate On 2017/04/26 21:13:05, chfremer wrote: > Please add comment explaining why the raw pointer |capture_device_| is safe to > use. Done. https://codereview.chromium.org/2837273004/diff/1/media/capture/video/chromeo... media/capture/video/chromeos/video_capture_device_arc_chromeos.cc:141: DCHECK(!camera_device_delegate_); On 2017/04/26 21:13:05, chfremer wrote: > Would we be hitting this when a client calls AllocateAndStart() directly after a > call to StopAndDeAllocate()? > It seems that the media::VideoCaptureDevice API does not have a mechanism to > tell clients when it is safe again to call AllocateAndStart() after a > StopAndDeAllocate(). According to the method-level comment at > media::VideoCaptureDevice::StopAndDeAllocate(), the implementation is expected > to delay (probably enqueue) the device restart until the device has finished > stopping. It wouldn't hit this DCHECK. I'll add some comments to explain this. AllocateAndStart, StopAndDeAllocate, and initialization/deletion of |camera_device_delegate_| all happen on the same task runner. When StopAndDeAllocate is called it either 1. resets |camera_device_delegate_| immediately if it's set, or 2. drops on-going open process in CreateDeviceOnCaptureThread below. It's safe in case 1; in case 2 the |camera_device_delegate_| is not set when StopAndDeAllocate is called, and wouldn't be set in CreateDeviceOnCaptureThread either. So once StopAndDeAllocate is called |camera_device_delegate_| is cleared until the next AllocateAndStart is called and succeeds. https://codereview.chromium.org/2837273004/diff/1/media/capture/video/chromeo... File media/capture/video/chromeos/video_capture_device_factory_chromeos.cc (right): https://codereview.chromium.org/2837273004/diff/1/media/capture/video/chromeo... media/capture/video/chromeos/video_capture_device_factory_chromeos.cc:15: bool VideoCaptureDeviceFactoryChromeOS::Init() { On 2017/04/26 21:13:05, chfremer wrote: > Add DCHECK(thread_checker_.CalledOnValidThread());? Init is called on the browser IO thread when we create the factory instance. I should add a comment to make this clear.
jcliang@chromium.org changed reviewers: + kenrb@chromium.org
A couple more questions/comments after a first pass of reading. https://codereview.chromium.org/2837273004/diff/20001/media/capture/video/chr... File media/capture/video/chromeos/camera_device_delegate.h (right): https://codereview.chromium.org/2837273004/diff/20001/media/capture/video/chr... media/capture/video/chromeos/camera_device_delegate.h:22: using namespace arc::mojom; As per Google C++ Style Guide, this may not be allowed. https://google.github.io/styleguide/cppguide.html#Namespaces https://codereview.chromium.org/2837273004/diff/20001/media/capture/video/chr... media/capture/video/chromeos/camera_device_delegate.h:28: class CAPTURE_EXPORT CameraDeviceDelegate final This class is fairly large, which makes it difficult to understanding it in its entirety. We should consider splitting it into separate classes for separate responsibilities. Not sure if it is easy to split by these, but potential responsibilities seem to be managing state, configuring/managing buffers, and communication to the HAL service. https://codereview.chromium.org/2837273004/diff/20001/media/capture/video/chr... media/capture/video/chromeos/camera_device_delegate.h:65: // When the camera device is in the kCapturing state, a capture loops is loops -> loop https://codereview.chromium.org/2837273004/diff/20001/media/capture/video/chr... media/capture/video/chromeos/camera_device_delegate.h:141: // Callback method for the Close Mojo IPC call. This function asynchronously Comment cut off? https://codereview.chromium.org/2837273004/diff/20001/media/capture/video/chr... media/capture/video/chromeos/camera_device_delegate.h:240: const scoped_refptr<base::SingleThreadTaskRunner> device_task_runner_; Would "ipc_task_runner_" be a more revealing name for this? https://codereview.chromium.org/2837273004/diff/20001/media/capture/video/chr... media/capture/video/chromeos/camera_device_delegate.h:258: std::queue<size_t> free_buffers; Are these buffers or buffer ids? Or indices into the |buffers| vector? https://codereview.chromium.org/2837273004/diff/20001/media/capture/video/chr... File media/capture/video/chromeos/mojo/arc_camera3.mojom (right): https://codereview.chromium.org/2837273004/diff/20001/media/capture/video/chr... media/capture/video/chromeos/mojo/arc_camera3.mojom:110: interface Camera3DeviceOps { I see that there are documenting comments in the C++ class headers. As a reader, I would find it more useful to see some of the documentation here with the interface. From reading the interface, I would like to get a rough idea of what the typical communication protocol between the Chromium side and the HAL service side is. https://codereview.chromium.org/2837273004/diff/20001/media/capture/video/chr... media/capture/video/chromeos/mojo/arc_camera3.mojom:131: RegisterBuffer@6(uint64 buffer_id, BufferType type, array<handle> fds, It seems the buffers are specific to a particular format, probably I420. Why is that needed? Wouldn't it suffice to just specify a size in bytes for the buffer and keep information about what format is being used separate? What about MJPEG?
https://codereview.chromium.org/2837273004/diff/20001/media/capture/video/chr... File media/capture/video/chromeos/camera_device_delegate.h (right): https://codereview.chromium.org/2837273004/diff/20001/media/capture/video/chr... media/capture/video/chromeos/camera_device_delegate.h:22: using namespace arc::mojom; On 2017/04/27 22:41:41, chfremer wrote: > As per Google C++ Style Guide, this may not be allowed. > https://google.github.io/styleguide/cppguide.html#Namespaces Removed. https://codereview.chromium.org/2837273004/diff/20001/media/capture/video/chr... media/capture/video/chromeos/camera_device_delegate.h:28: class CAPTURE_EXPORT CameraDeviceDelegate final On 2017/04/27 22:41:41, chfremer wrote: > This class is fairly large, which makes it difficult to understanding it in its > entirety. > We should consider splitting it into separate classes for separate > responsibilities. Not sure if it is easy to split by these, but potential > responsibilities seem to be managing state, configuring/managing buffers, and > communication to the HAL service. I'll try if I can split the class, but it may not be easy. The methods are deeply coupled with each other per the Android camera HAL v3 API spec. https://codereview.chromium.org/2837273004/diff/20001/media/capture/video/chr... media/capture/video/chromeos/camera_device_delegate.h:65: // When the camera device is in the kCapturing state, a capture loops is On 2017/04/27 22:41:41, chfremer wrote: > loops -> loop Done. https://codereview.chromium.org/2837273004/diff/20001/media/capture/video/chr... media/capture/video/chromeos/camera_device_delegate.h:141: // Callback method for the Close Mojo IPC call. This function asynchronously On 2017/04/27 22:41:41, chfremer wrote: > Comment cut off? Done. https://codereview.chromium.org/2837273004/diff/20001/media/capture/video/chr... media/capture/video/chromeos/camera_device_delegate.h:240: const scoped_refptr<base::SingleThreadTaskRunner> device_task_runner_; On 2017/04/27 22:41:41, chfremer wrote: > Would "ipc_task_runner_" be a more revealing name for this? Done. https://codereview.chromium.org/2837273004/diff/20001/media/capture/video/chr... media/capture/video/chromeos/camera_device_delegate.h:258: std::queue<size_t> free_buffers; On 2017/04/27 22:41:41, chfremer wrote: > Are these buffers or buffer ids? Or indices into the |buffers| vector? It's indices into the |buffers| vector. I'll update the comment to clarify this. https://codereview.chromium.org/2837273004/diff/20001/media/capture/video/chr... File media/capture/video/chromeos/mojo/arc_camera3.mojom (right): https://codereview.chromium.org/2837273004/diff/20001/media/capture/video/chr... media/capture/video/chromeos/mojo/arc_camera3.mojom:110: interface Camera3DeviceOps { On 2017/04/27 22:41:41, chfremer wrote: > I see that there are documenting comments in the C++ class headers. > As a reader, I would find it more useful to see some of the documentation here > with the interface. > From reading the interface, I would like to get a rough idea of what the typical > communication protocol between the Chromium side and the HAL service side is. I'll add some comments to briefly explain the flow of the IPC. https://codereview.chromium.org/2837273004/diff/20001/media/capture/video/chr... media/capture/video/chromeos/mojo/arc_camera3.mojom:131: RegisterBuffer@6(uint64 buffer_id, BufferType type, array<handle> fds, On 2017/04/27 22:41:41, chfremer wrote: > It seems the buffers are specific to a particular format, probably I420. Why is > that needed? Wouldn't it suffice to just specify a size in bytes for the buffer > and keep information about what format is being used separate? > > What about MJPEG? This mojom is used by both Android and Chrome to talk to the HAL adapter process on Chrome OS. This RegisterBuffer interface is needed on the Android side: we need to use some special logic to extract information from the device-specific gralloc buffer handle at Android and pass the info to HAL adapter on Chrome OS to reconstruct the buffer handle. On Android there may be multiple streams active at the same time using different types of buffers. The params in RegisterBuffer interface encapsulates all the required info to create a gralloc buffer handle of any type. For Chrome the buffer handling is much simpler and you're probably right that we can simply pass the size of the buffer and keep the buffer info elsewhere, but we would want to use the same Mojo IPC interface as Android.
https://codereview.chromium.org/2837273004/diff/40001/media/capture/video/chr... File media/capture/video/chromeos/OWNERS (right): https://codereview.chromium.org/2837273004/diff/40001/media/capture/video/chr... media/capture/video/chromeos/OWNERS:1: jcliang@chromium.org wuchengli@chromium.org add me in case you are OOO. https://codereview.chromium.org/2837273004/diff/40001/media/capture/video/chr... File media/capture/video/chromeos/camera_hal_delegate.h (right): https://codereview.chromium.org/2837273004/diff/40001/media/capture/video/chr... media/capture/video/chromeos/camera_hal_delegate.h:51: // Asynchronous method to open the camera device desginated by |camera_id|. designated https://codereview.chromium.org/2837273004/diff/40001/media/capture/video/chr... File media/capture/video/chromeos/video_capture_device_arc_chromeos.cc (right): https://codereview.chromium.org/2837273004/diff/40001/media/capture/video/chr... media/capture/video/chromeos/video_capture_device_arc_chromeos.cc:215: client->OnError(FROM_HERE, "Failed to get camera info"); Does this need to be called on IO thread? https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/vide... https://codereview.chromium.org/2837273004/diff/40001/media/capture/video/chr... File media/capture/video/chromeos/video_capture_device_arc_chromeos.h (right): https://codereview.chromium.org/2837273004/diff/40001/media/capture/video/chr... media/capture/video/chromeos/video_capture_device_arc_chromeos.h:5: // blah blah blah... Update this. https://codereview.chromium.org/2837273004/diff/40001/media/capture/video/chr... File media/capture/video/chromeos/video_capture_device_factory_chromeos.h (right): https://codereview.chromium.org/2837273004/diff/40001/media/capture/video/chr... media/capture/video/chromeos/video_capture_device_factory_chromeos.h:19: VideoCaptureDeviceFactoryChromeOS( explicit https://codereview.chromium.org/2837273004/diff/40001/media/capture/video/chr... media/capture/video/chromeos/video_capture_device_factory_chromeos.h:34: // succeeds. This method is called on the browser IO thread when the factory Check why this is on IO thread. From the code, this seems to be on UI thread. https://codereview.chromium.org/2837273004/diff/40001/media/capture/video/chr... media/capture/video/chromeos/video_capture_device_factory_chromeos.h:42: // place. Started in Init and stopped when the class instance is destroyed. Can we make Start and Stop run on the same thread? base::Thread has a commented-out DCHECK. https://codereview.chromium.org/2837273004/diff/40001/media/capture/video/lin... File media/capture/video/linux/video_capture_device_factory_linux.cc (right): https://codereview.chromium.org/2837273004/diff/40001/media/capture/video/lin... media/capture/video/linux/video_capture_device_factory_linux.cc:267: #if !defined(OS_CHROMEOS) Try controlling this in media/capture/BUILD.gn. If we can't, explain why.
https://codereview.chromium.org/2837273004/diff/40001/media/capture/video/chr... File media/capture/video/chromeos/OWNERS (right): https://codereview.chromium.org/2837273004/diff/40001/media/capture/video/chr... media/capture/video/chromeos/OWNERS:1: jcliang@chromium.org On 2017/04/28 06:29:28, wuchengli wrote: > mailto:wuchengli@chromium.org > add me in case you are OOO. Done. https://codereview.chromium.org/2837273004/diff/40001/media/capture/video/chr... File media/capture/video/chromeos/camera_hal_delegate.h (right): https://codereview.chromium.org/2837273004/diff/40001/media/capture/video/chr... media/capture/video/chromeos/camera_hal_delegate.h:51: // Asynchronous method to open the camera device desginated by |camera_id|. On 2017/04/28 06:29:28, wuchengli wrote: > designated Done. https://codereview.chromium.org/2837273004/diff/40001/media/capture/video/chr... File media/capture/video/chromeos/video_capture_device_arc_chromeos.cc (right): https://codereview.chromium.org/2837273004/diff/40001/media/capture/video/chr... media/capture/video/chromeos/video_capture_device_arc_chromeos.cc:215: client->OnError(FROM_HERE, "Failed to get camera info"); On 2017/04/28 06:29:28, wuchengli wrote: > Does this need to be called on IO thread? > https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/vide... I believe client->OnError calls to here: https://cs.chromium.org/chromium/src/media/capture/video/video_frame_receiver.... So the VideoFrameReceiverOnTaskRunner would sequence the task internally. https://codereview.chromium.org/2837273004/diff/40001/media/capture/video/chr... File media/capture/video/chromeos/video_capture_device_arc_chromeos.h (right): https://codereview.chromium.org/2837273004/diff/40001/media/capture/video/chr... media/capture/video/chromeos/video_capture_device_arc_chromeos.h:5: // blah blah blah... On 2017/04/28 06:29:28, wuchengli wrote: > Update this. Removed. https://codereview.chromium.org/2837273004/diff/40001/media/capture/video/chr... File media/capture/video/chromeos/video_capture_device_factory_chromeos.h (right): https://codereview.chromium.org/2837273004/diff/40001/media/capture/video/chr... media/capture/video/chromeos/video_capture_device_factory_chromeos.h:19: VideoCaptureDeviceFactoryChromeOS( On 2017/04/28 06:29:28, wuchengli wrote: > explicit Done. https://codereview.chromium.org/2837273004/diff/40001/media/capture/video/chr... media/capture/video/chromeos/video_capture_device_factory_chromeos.h:34: // succeeds. This method is called on the browser IO thread when the factory On 2017/04/28 06:29:28, wuchengli wrote: > Check why this is on IO thread. From the code, this seems to be on UI thread. I just verified this by manually inserting logs in this function. It's called on UI thread. The comment here says the factory is created on IO thread: https://cs.chromium.org/chromium/src/media/capture/video/video_capture_device..., so it's probably outdated. https://codereview.chromium.org/2837273004/diff/40001/media/capture/video/chr... media/capture/video/chromeos/video_capture_device_factory_chromeos.h:42: // place. Started in Init and stopped when the class instance is destroyed. On 2017/04/28 06:29:28, wuchengli wrote: > Can we make Start and Stop run on the same thread? base::Thread has a > commented-out DCHECK. The thread is started on UI thread when the browser mainloop bootstraps. The factory instance is a singleton in the browser process, so in theory it's never stopped. https://codereview.chromium.org/2837273004/diff/40001/media/capture/video/lin... File media/capture/video/linux/video_capture_device_factory_linux.cc (right): https://codereview.chromium.org/2837273004/diff/40001/media/capture/video/lin... media/capture/video/linux/video_capture_device_factory_linux.cc:267: #if !defined(OS_CHROMEOS) On 2017/04/28 06:29:28, wuchengli wrote: > Try controlling this in media/capture/BUILD.gn. If we can't, explain why. There are rules in BUILDCONFIG.gn to control what files are compiled on each OS platform: https://cs.chromium.org/chromium/src/build/config/BUILDCONFIG.gn?q=BUILDCONFI... ChromeOS is special in that is_chromeos and is_linux are both true, so all the source files with _linux.* and _chromeos.* suffix are compile on chromeos. We need to let the linux and chromeos coexist during the transition period, so I'm using #define to control which factory gets created here. We can fully control this in BUILD.gn once we fully remove dependency on the V4L2 VCD on ChromeOS.
https://codereview.chromium.org/2837273004/diff/40001/media/capture/video/chr... File media/capture/video/chromeos/video_capture_device_factory_chromeos.h (right): https://codereview.chromium.org/2837273004/diff/40001/media/capture/video/chr... media/capture/video/chromeos/video_capture_device_factory_chromeos.h:34: // succeeds. This method is called on the browser IO thread when the factory On 2017/04/28 08:27:49, jcliang wrote: > On 2017/04/28 06:29:28, wuchengli wrote: > > Check why this is on IO thread. From the code, this seems to be on UI thread. > > I just verified this by manually inserting logs in this function. It's called on > UI thread. > > The comment here says the factory is created on IO thread: > https://cs.chromium.org/chromium/src/media/capture/video/video_capture_device..., > so it's probably outdated. That is my bad. I changed this threading in a recent CL [1] but failed to find and change the comment. Could you do me a favor and update the comment? [1] https://codereview.chromium.org/2787703004/
https://codereview.chromium.org/2837273004/diff/40001/media/capture/video/chr... File media/capture/video/chromeos/video_capture_device_factory_chromeos.h (right): https://codereview.chromium.org/2837273004/diff/40001/media/capture/video/chr... media/capture/video/chromeos/video_capture_device_factory_chromeos.h:34: // succeeds. This method is called on the browser IO thread when the factory On 2017/04/28 16:57:18, chfremer wrote: > On 2017/04/28 08:27:49, jcliang wrote: > > On 2017/04/28 06:29:28, wuchengli wrote: > > > Check why this is on IO thread. From the code, this seems to be on UI > thread. > > > > I just verified this by manually inserting logs in this function. It's called > on > > UI thread. > > > > The comment here says the factory is created on IO thread: > > > https://cs.chromium.org/chromium/src/media/capture/video/video_capture_device..., > > so it's probably outdated. > > That is my bad. I changed this threading in a recent CL [1] but failed to find > and change the comment. > Could you do me a favor and update the comment? > > [1] https://codereview.chromium.org/2787703004/ Uploaded another CL for the fix: https://codereview.chromium.org/2849023002
For IPC review, this looks okay modulo one comment which is probably a nit. If I am understanding this correctly, the callback interfaces are receiving data from the HAL, which would already be at a significantly higher privilege and therefore it would be fine to consider the incoming data trustworthy. Presumably the Camera3DeviceOps and CameraModule interfaces are yet to be implemented? Please ensure that those implementations receive security review, even if the mojoms remain unchanged. https://codereview.chromium.org/2837273004/diff/80001/media/capture/video/chr... File media/capture/video/chromeos/camera_device_delegate.cc (right): https://codereview.chromium.org/2837273004/diff/80001/media/capture/video/chr... media/capture/video/chromeos/camera_device_delegate.cc:482: for (size_t i = 0; i < result->num_output_buffers; ++i) { This looks like it causes a problem if num_output_buffers mismatches the actual number of items in the output_buffer array. Is that field necessary, why not just use the array size?
https://codereview.chromium.org/2837273004/diff/80001/media/capture/video/chr... File media/capture/video/chromeos/camera_device_delegate.cc (right): https://codereview.chromium.org/2837273004/diff/80001/media/capture/video/chr... media/capture/video/chromeos/camera_device_delegate.cc:482: for (size_t i = 0; i < result->num_output_buffers; ++i) { On 2017/05/01 23:08:34, kenrb wrote: > This looks like it causes a problem if num_output_buffers mismatches the actual > number of items in the output_buffer array. Is that field necessary, why not > just use the array size? You're right we don't need num_output_buffers here. I was simply mapping the struct fields in Android API and didn't notice that we can remove num_output_buffers in mojom.
I'm still reviewing camera_hal_delegate.cc. https://codereview.chromium.org/2837273004/diff/80001/media/capture/video/chr... File media/capture/video/chromeos/camera_hal_delegate.cc (right): https://codereview.chromium.org/2837273004/diff/80001/media/capture/video/chr... media/capture/video/chromeos/camera_hal_delegate.cc:41: camera_module_callbacks_(this) { note to myself: this is run on browser UI thread. https://codereview.chromium.org/2837273004/diff/80001/media/capture/video/chr... media/capture/video/chromeos/camera_hal_delegate.cc:45: CameraHalDelegate::~CameraHalDelegate() { note to myself: CameraHalDelegate is singleton, created with VideoCaptureDeviceFactory. https://codereview.chromium.org/2837273004/diff/80001/media/capture/video/chr... media/capture/video/chromeos/camera_hal_delegate.cc:56: bool CameraHalDelegate::StartCameraModuleIpc() { note to myself: this is called on browser UI thread. https://codereview.chromium.org/2837273004/diff/80001/media/capture/video/chr... media/capture/video/chromeos/camera_hal_delegate.cc:64: LOG(ERROR) << "fcntl(F_GETFL) failed"; PLOG. Same for other places in this file. https://codereview.chromium.org/2837273004/diff/80001/media/capture/video/chr... media/capture/video/chromeos/camera_hal_delegate.cc:127: // The camera may be removed already or is not ready yet. How does the client handle the case that camera is not ready? Does the client call GetSupportedFormats again later? https://codereview.chromium.org/2837273004/diff/80001/media/capture/video/chr... media/capture/video/chromeos/camera_hal_delegate.cc:128: VLOG(1) << "Invalid camera_id: " << camera_id; How can the camera be removed already? If UpdateCameraInfo returns true in line 121, camera_info_ should still have the data (may be obsoleted though). Right? https://codereview.chromium.org/2837273004/diff/80001/media/capture/video/chr... media/capture/video/chromeos/camera_hal_delegate.cc:145: // There may be multiple FPS ranges. We simply use the maximum FPS of all the Do we want to use the maximum FPS of all FPS ranges here? The original video capture device linux hard-codes to 30fps? What if we choose a 60FPS and rendering is not smooth? https://codereview.chromium.org/2837273004/diff/80001/media/capture/video/chr... media/capture/video/chromeos/camera_hal_delegate.cc:194: } Explain if a device is unplugged at this time, the client will get the old device descriptor. The next CreateDevice should fail. https://codereview.chromium.org/2837273004/diff/80001/media/capture/video/chr... media/capture/video/chromeos/camera_hal_delegate.cc:270: camera_info_.clear(); Should we set num_cameras_ to 0 here? It's more consistent to clear them both at the same time. On the other hand, it's hard to understand we write to num_cameras_ on two different threads. https://codereview.chromium.org/2837273004/diff/80001/media/capture/video/chr... media/capture/video/chromeos/camera_hal_delegate.cc:306: DCHECK(!camera_info_updated_.IsSignaled()); Should we move DCHECK after line 311? I'm thinking about this call flow. Can this happen? - Suppose there are two cameras and camera_info_ have been initialized. - GetCameraInfo and GetCameraInfoOnModuleThread are called. - One camera is unplugged. - CameraDeviceStatusChange is called and camera_info_updated_ is reset. - OnGotCameraInfoOnModuleThread is called. https://codereview.chromium.org/2837273004/diff/80001/media/capture/video/chr... media/capture/video/chromeos/camera_hal_delegate.cc:309: // enumerating state or is no longer present. If the device is no longer present, shouldn't we update num_cameras_? https://codereview.chromium.org/2837273004/diff/80001/media/capture/video/chr... media/capture/video/chromeos/camera_hal_delegate.cc:310: VLOG(1) << "Failed to get camera info. Device id: " << camera_id; If get_camera_info fails, should we return here? https://codereview.chromium.org/2837273004/diff/80001/media/capture/video/chr... media/capture/video/chromeos/camera_hal_delegate.cc:313: camera_info_[camera_id] = std::move(camera_info); VideoCaptureDeviceArcChromeOS can call GetCameraInfo and pass a GetCameraInfoCallback. Does VideoCaptureDeviceArcChromeOS also need to set camera_info_ and camera_info_updated_ in its callback? https://codereview.chromium.org/2837273004/diff/80001/media/capture/video/chr... media/capture/video/chromeos/camera_hal_delegate.cc:315: camera_info_updated_.Signal(); Is it possible we get CameraDeviceStatusChange before OnGotCameraInfoOnModuleThread? Will camera_info_updated_ be reset first and get signaled? https://codereview.chromium.org/2837273004/diff/80001/media/capture/video/chr... media/capture/video/chromeos/camera_hal_delegate.cc:333: void CameraHalDelegate::OnSetCallbacksOnModuleThread(int32_t result) { What thread runs this function? https://codereview.chromium.org/2837273004/diff/80001/media/capture/video/chr... media/capture/video/chromeos/camera_hal_delegate.cc:335: LOG(ERROR) << "Failed to set camera module callbacks"; I think we should set num_camera_ to 0, clear camera_info_, and signal camera_info_updated_ to unblock the client. Otherwise the client will be blocked forever. https://codereview.chromium.org/2837273004/diff/80001/media/capture/video/chr... media/capture/video/chromeos/camera_hal_delegate.cc:356: arc::mojom::CameraDeviceStatus::CAMERA_DEVICE_STATUS_NOT_PRESENT) { Probably we won't be able to enable this for USB and MIPI camera HALs at the same time. Enabling MIPI has higher priority over USB. For MIPI, we don't need this yet because they are all built-in cameras. There are many review comments about camera_info_updated_.Reset in this file. I'd prefer making this a TODO so the CL is smaller and we get unblock the vendor sooner. https://codereview.chromium.org/2837273004/diff/80001/media/capture/video/chr... media/capture/video/chromeos/camera_hal_delegate.cc:364: camera_info_updated_.Reset(); As discussed, how do we fix the issue the client may accidentally use a different camera after status change? This sounds serious. What if one camera is pointing at a scene that users do not want other people to see? I'm curious how chrome handles two external USB cameras with the same pid/vid. Does it have the same problem? https://codereview.chromium.org/2837273004/diff/80001/media/capture/video/chr... media/capture/video/chromeos/camera_hal_delegate.cc:366: } What about CAMERA_DEVICE_STATUS_ENUMERATING? I think we should also reset camera_info_updated_. Is it simpler just always reset camera_info_updated_ without checking |new_status| and |camera_info_|?
https://codereview.chromium.org/2837273004/diff/80001/media/capture/video/chr... File media/capture/video/chromeos/camera_hal_delegate.cc (right): https://codereview.chromium.org/2837273004/diff/80001/media/capture/video/chr... media/capture/video/chromeos/camera_hal_delegate.cc:64: LOG(ERROR) << "fcntl(F_GETFL) failed"; On 2017/05/02 16:11:42, wuchengli wrote: > PLOG. Same for other places in this file. Done. https://codereview.chromium.org/2837273004/diff/80001/media/capture/video/chr... media/capture/video/chromeos/camera_hal_delegate.cc:127: // The camera may be removed already or is not ready yet. On 2017/05/02 16:11:42, wuchengli wrote: > How does the client handle the case that camera is not ready? Does the client > call GetSupportedFormats again later? I think Chrome would find the camera unusable, and would fail to use the camera on this failed call. Subsequent attempt to use the camera will query the camera info again. https://codereview.chromium.org/2837273004/diff/80001/media/capture/video/chr... media/capture/video/chromeos/camera_hal_delegate.cc:128: VLOG(1) << "Invalid camera_id: " << camera_id; On 2017/05/02 16:11:42, wuchengli wrote: > How can the camera be removed already? If UpdateCameraInfo returns true in line > 121, camera_info_ should still have the data (may be obsoleted though). Right? If the camera is not ready then camera_info_[camera_id] may be null, as indicated in line 313. This behavior corresponds to the HAL API specification where get_camera_info() returns null if the HAL finds the camera not ready to accept any request (e.g. the camera is plugged in but not hardware initialized yet, or the camera is removed and the |camera_id| is invalid). The above scenario is a race condition between the software and user interaction, so it's not something we can avoid here. https://codereview.chromium.org/2837273004/diff/80001/media/capture/video/chr... media/capture/video/chromeos/camera_hal_delegate.cc:145: // There may be multiple FPS ranges. We simply use the maximum FPS of all the On 2017/05/02 16:11:42, wuchengli wrote: > Do we want to use the maximum FPS of all FPS ranges here? The original video > capture device linux hard-codes to 30fps? What if we choose a 60FPS and > rendering is not smooth? The VideoCaptureDeviceFactoryLinux asks the kernel to enumerate all frame intervals of a pixel format with VIDIOC_ENUM_FRAMEINTERVALS and reports all the (pixel_format, frame_rate) combinations. https://codereview.chromium.org/2837273004/diff/80001/media/capture/video/chr... media/capture/video/chromeos/camera_hal_delegate.cc:194: } On 2017/05/02 16:11:42, wuchengli wrote: > Explain if a device is unplugged at this time, the client will get the old > device descriptor. The next CreateDevice should fail. Done. https://codereview.chromium.org/2837273004/diff/80001/media/capture/video/chr... media/capture/video/chromeos/camera_hal_delegate.cc:270: camera_info_.clear(); On 2017/05/02 16:11:42, wuchengli wrote: > Should we set num_cameras_ to 0 here? It's more consistent to clear them both at > the same time. On the other hand, it's hard to understand we write to > num_cameras_ on two different threads. Yes we should set |num_cameras_| to 0 here to be consistent. |num_cameras_| should only be modified on |module_task_runner_| so that it's protected by |camera_info_updated_| as well. I added a DCHECK in OnSetCallbacksOnModuleThread to make sure it runs on |module_task_runner_|. https://codereview.chromium.org/2837273004/diff/80001/media/capture/video/chr... media/capture/video/chromeos/camera_hal_delegate.cc:306: DCHECK(!camera_info_updated_.IsSignaled()); On 2017/05/02 16:11:41, wuchengli wrote: > Should we move DCHECK after line 311? I'm thinking about this call flow. Can > this happen? > - Suppose there are two cameras and camera_info_ have been initialized. > - GetCameraInfo and GetCameraInfoOnModuleThread are called. > - One camera is unplugged. > - CameraDeviceStatusChange is called and camera_info_updated_ is reset. > - OnGotCameraInfoOnModuleThread is called. This won't happen for now as we only deal with built-in cameras. Hot-plugging external cameras will be handled in another CL. https://codereview.chromium.org/2837273004/diff/80001/media/capture/video/chr... media/capture/video/chromeos/camera_hal_delegate.cc:309: // enumerating state or is no longer present. On 2017/05/02 16:11:41, wuchengli wrote: > If the device is no longer present, shouldn't we update num_cameras_? Currently I'm following the HAL API spec: https://chromium.googlesource.com/aosp/platform/hardware/libhardware/+/master... If we get here then it means the camera device is at the ENUMERATING state: https://chromium.googlesource.com/aosp/platform/hardware/libhardware/+/master... If the device is no longer present then we would get an empty |camera_info| here. We still keep the same |num_cameras_| and insert the empty |camera_info| into |camera_info_|. GetSupportedFormats and GetDeviceDescriptors above would ignore the empty camera info. This should simply things as we only rely on the CameraDeviceStatusChange callback from HAL below to trigger an update to |camera_info_| once the ENUMERATING camera device is PRESENT or NOT_PRESENT. https://codereview.chromium.org/2837273004/diff/80001/media/capture/video/chr... media/capture/video/chromeos/camera_hal_delegate.cc:310: VLOG(1) << "Failed to get camera info. Device id: " << camera_id; On 2017/05/02 16:11:42, wuchengli wrote: > If get_camera_info fails, should we return here? We continue here because of the reason above. https://codereview.chromium.org/2837273004/diff/80001/media/capture/video/chr... media/capture/video/chromeos/camera_hal_delegate.cc:313: camera_info_[camera_id] = std::move(camera_info); On 2017/05/02 16:11:42, wuchengli wrote: > VideoCaptureDeviceArcChromeOS can call GetCameraInfo and pass a > GetCameraInfoCallback. Does VideoCaptureDeviceArcChromeOS also need to set > camera_info_ and camera_info_updated_ in its callback? |camera_info_| is only used internally by CameraHalDelegate, and |camera_info_updated_| is used to serialize the access to |camera_info_| internally in CameraHalDelegate. GetCameraInfo is meant to be called by external user and thus it carries its own |callback| in the parameters. When GetCameraInfo is called with a |camera_id| we IPC to HAL to call get_camera_info directly without using |camera_info_| at all. When the IPC returns the supplied |callback| would be called and it would not go through here. https://codereview.chromium.org/2837273004/diff/80001/media/capture/video/chr... media/capture/video/chromeos/camera_hal_delegate.cc:315: camera_info_updated_.Signal(); On 2017/05/02 16:11:42, wuchengli wrote: > Is it possible we get CameraDeviceStatusChange before > OnGotCameraInfoOnModuleThread? Will camera_info_updated_ be reset first and get > signaled? The code is refactored to only handle built-in cameras and this case wouldn't happen for built-in cameras. We'll need locking for handling the external cameras. I'm trying to understand how the external camera status change event works as the camera HAL v3 API is not very clear about how the signaling of external camera works. I'll handle external camera in another CL. https://codereview.chromium.org/2837273004/diff/80001/media/capture/video/chr... media/capture/video/chromeos/camera_hal_delegate.cc:333: void CameraHalDelegate::OnSetCallbacksOnModuleThread(int32_t result) { On 2017/05/02 16:11:42, wuchengli wrote: > What thread runs this function? Added a DCHECK to make sure this runs on |module_task_runner_|. https://codereview.chromium.org/2837273004/diff/80001/media/capture/video/chr... media/capture/video/chromeos/camera_hal_delegate.cc:335: LOG(ERROR) << "Failed to set camera module callbacks"; On 2017/05/02 16:11:42, wuchengli wrote: > I think we should set num_camera_ to 0, clear camera_info_, and signal > camera_info_updated_ to unblock the client. Otherwise the client will be blocked > forever. |camera_info_| is empty here. I've set |num_cameras_| to 0 and signaled |camera_info_updated_|. I'm using TimedWait on |camera_info_updated_| above so it should time out eventually, but we can explicitly signal here to reducing latency. https://codereview.chromium.org/2837273004/diff/80001/media/capture/video/chr... media/capture/video/chromeos/camera_hal_delegate.cc:356: arc::mojom::CameraDeviceStatus::CAMERA_DEVICE_STATUS_NOT_PRESENT) { On 2017/05/02 16:11:41, wuchengli wrote: > Probably we won't be able to enable this for USB and MIPI camera HALs at the > same time. Enabling MIPI has higher priority over USB. For MIPI, we don't need > this yet because they are all built-in cameras. There are many review comments > about camera_info_updated_.Reset in this file. I'd prefer making this a TODO so > the CL is smaller and we get unblock the vendor sooner. Done. https://codereview.chromium.org/2837273004/diff/80001/media/capture/video/chr... media/capture/video/chromeos/camera_hal_delegate.cc:366: } On 2017/05/02 16:11:42, wuchengli wrote: > What about CAMERA_DEVICE_STATUS_ENUMERATING? I think we should also reset > camera_info_updated_. Is it simpler just always reset camera_info_updated_ > without checking |new_status| and |camera_info_|? We can simply ignore the ENUMERATING state, since when a device is in this state querying its info will return error and null info. The ENUMERATING will transition to PRESENT or NOT_PRESENT eventually and we can handle the event accordingly like above.
Finished camera_hal_delegate.cc. https://codereview.chromium.org/2837273004/diff/120001/media/capture/video/ch... File media/capture/video/chromeos/camera_hal_delegate.cc (right): https://codereview.chromium.org/2837273004/diff/120001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:42: thread_checker_.DetachFromThread(); Document we need to detach because constructor runs on UI thread and VideoCaptureDeviceFactory functions run on capture thread (?). https://codereview.chromium.org/2837273004/diff/120001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:78: PLOG(ERROR) << "PlatformChannelRecvmsg failed: "; return false; https://codereview.chromium.org/2837273004/diff/120001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:94: mojo::edk::CreateChildMessagePipe(std::string(token, 32)); Use constant for 32 (also line 73). https://codereview.chromium.org/2837273004/diff/120001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:136: ANDROID_CONTROL_AE_AVAILABLE_TARGET_FPS_RANGES); Should we use SCALER_AVAILABLE_MIN_FRAME_DURATIONS? If a resolution supports 30fps and 60fps, this will be 60fps. But chrome may only want to use 30fps. References: frameworks/base/core/java/android/hardware/camera2/impl/CameraMetadataNative.java, getStreamConfigurationMap() https://developer.android.com/reference/android/hardware/camera2/params/Strea..., android.util.Size) https://codereview.chromium.org/2837273004/diff/120001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:174: VLOG(1) << "[" << std::hex << format << " " << std::dec << width << " " DVLOG https://codereview.chromium.org/2837273004/diff/120001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:183: supported_formats->emplace_back(gfx::Size(width, height), max_fps, From video_capture_device_factory_linux.cc GetFrameRateList, it puts all fps to supported_formats. https://cs.chromium.org/chromium/src/media/capture/video/linux/video_capture_... https://codereview.chromium.org/2837273004/diff/120001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:202: desc.capture_api = VideoCaptureApi::ANDROID_API2_LIMITED; Chrome media team may know what |capture_api| is for. We don't see any use of |capture_api|. https://codereview.chromium.org/2837273004/diff/120001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:207: desc.display_name = std::string("Back Camera"); What's display_name is used for? Should we follow #1? If end users can see this string, we should translate it. #1: https://codereview.chromium.org/2703393007/diff/140001/chrome/browser/ui/webu... https://codereview.chromium.org/2837273004/diff/120001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:216: break; Document mojo makes sure |facing| is valid. https://codereview.chromium.org/2837273004/diff/120001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:257: DCHECK(!builtin_camera_info_updated_.IsSignaled()); remove because line 252 just checks it. https://codereview.chromium.org/2837273004/diff/120001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:277: DCHECK(module_task_runner_->BelongsToCurrentThread()); Do not set num_builtin_cameras_ if num_cameras < 0. We cannot trust HAL. https://codereview.chromium.org/2837273004/diff/120001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:278: num_builtin_cameras_ = num_cameras; Add VLOG to print the number of cameras here so it's easier to debug. https://codereview.chromium.org/2837273004/diff/120001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:292: LOG(ERROR) << "Failed to set camera module callbacks"; Print |result| if it contains an error code. https://codereview.chromium.org/2837273004/diff/120001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:313: DCHECK(module_task_runner_->BelongsToCurrentThread()); Add DVLOG to print camera_id so it's easier to debug if needed. https://codereview.chromium.org/2837273004/diff/120001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:317: VLOG(1) << "Failed to get camera info. Device id: " << camera_id; s/Device/Camera/ Print |result| if it contains an error code.
Finished the review of video_capture_device_arc_chromeos.h/.cc. https://codereview.chromium.org/2837273004/diff/140001/media/capture/video/ch... File media/capture/video/chromeos/video_capture_device_arc_chromeos.cc (right): https://codereview.chromium.org/2837273004/diff/140001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_arc_chromeos.cc:29: ui_task_runner_(ui_task_runner), use std::move to reduce a copy https://codereview.chromium.org/2837273004/diff/140001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_arc_chromeos.cc:133: void VideoCaptureDeviceArcChromeOS::AllocateAndStart( What happens when AllocateAndStart, StopAndDeAllocate, and AllocateAndStart are called consecutively? Will the second AllocateAndStart fail? Should we block AllocateAndStart and StopAndDeAllocate until everything finishes? https://codereview.chromium.org/2837273004/diff/140001/media/capture/video/ch... File media/capture/video/chromeos/video_capture_device_arc_chromeos.h (right): https://codereview.chromium.org/2837273004/diff/140001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_arc_chromeos.h:45: scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner, We discussed offline. Not using const & means the implementation will increase the ref count.
https://codereview.chromium.org/2837273004/diff/120001/media/capture/video/ch... File media/capture/video/chromeos/camera_hal_delegate.cc (right): https://codereview.chromium.org/2837273004/diff/120001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:42: thread_checker_.DetachFromThread(); On 2017/05/03 06:34:00, wuchengli wrote: > Document we need to detach because constructor runs on UI thread and > VideoCaptureDeviceFactory functions run on capture thread (?). Done. https://codereview.chromium.org/2837273004/diff/120001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:78: PLOG(ERROR) << "PlatformChannelRecvmsg failed: "; On 2017/05/03 06:33:59, wuchengli wrote: > return false; Done. https://codereview.chromium.org/2837273004/diff/120001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:94: mojo::edk::CreateChildMessagePipe(std::string(token, 32)); On 2017/05/03 06:33:59, wuchengli wrote: > Use constant for 32 (also line 73). Done. https://codereview.chromium.org/2837273004/diff/120001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:174: VLOG(1) << "[" << std::hex << format << " " << std::dec << width << " " On 2017/05/03 06:34:00, wuchengli wrote: > DVLOG Done. https://codereview.chromium.org/2837273004/diff/120001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:202: desc.capture_api = VideoCaptureApi::ANDROID_API2_LIMITED; On 2017/05/03 06:34:00, wuchengli wrote: > Chrome media team may know what |capture_api| is for. We don't see any use of > |capture_api|. I checked with chfremer@ and we can either declare a new enum for our case or reuse the existing ANDROID_API2_* enums. I'll use ANDROID_API2_LIMITED here for now. https://codereview.chromium.org/2837273004/diff/120001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:216: break; On 2017/05/03 06:33:59, wuchengli wrote: > Document mojo makes sure |facing| is valid. Done. https://codereview.chromium.org/2837273004/diff/120001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:257: DCHECK(!builtin_camera_info_updated_.IsSignaled()); On 2017/05/03 06:33:59, wuchengli wrote: > remove because line 252 just checks it. Done. https://codereview.chromium.org/2837273004/diff/120001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:277: DCHECK(module_task_runner_->BelongsToCurrentThread()); On 2017/05/03 06:33:59, wuchengli wrote: > Do not set num_builtin_cameras_ if num_cameras < 0. We cannot trust HAL. Done. https://codereview.chromium.org/2837273004/diff/120001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:278: num_builtin_cameras_ = num_cameras; On 2017/05/03 06:34:00, wuchengli wrote: > Add VLOG to print the number of cameras here so it's easier to debug. Done. https://codereview.chromium.org/2837273004/diff/120001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:292: LOG(ERROR) << "Failed to set camera module callbacks"; On 2017/05/03 06:34:00, wuchengli wrote: > Print |result| if it contains an error code. Done. https://codereview.chromium.org/2837273004/diff/120001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:313: DCHECK(module_task_runner_->BelongsToCurrentThread()); On 2017/05/03 06:33:59, wuchengli wrote: > Add DVLOG to print camera_id so it's easier to debug if needed. Done. https://codereview.chromium.org/2837273004/diff/120001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:317: VLOG(1) << "Failed to get camera info. Device id: " << camera_id; On 2017/05/03 06:33:59, wuchengli wrote: > s/Device/Camera/ > > Print |result| if it contains an error code. Done. https://codereview.chromium.org/2837273004/diff/140001/media/capture/video/ch... File media/capture/video/chromeos/video_capture_device_arc_chromeos.cc (right): https://codereview.chromium.org/2837273004/diff/140001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_arc_chromeos.cc:29: ui_task_runner_(ui_task_runner), On 2017/05/05 08:10:55, wuchengli wrote: > use std::move to reduce a copy Done. The std::move() is added in VideoCaptureDeviceArcChromeOS()'s initialization list below. https://codereview.chromium.org/2837273004/diff/140001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_arc_chromeos.cc:133: void VideoCaptureDeviceArcChromeOS::AllocateAndStart( On 2017/05/05 08:10:55, wuchengli wrote: > What happens when AllocateAndStart, StopAndDeAllocate, and AllocateAndStart are > called consecutively? Will the second AllocateAndStart fail? Should we block > AllocateAndStart and StopAndDeAllocate until everything finishes? Right...there's a tiny chance that the second open would fail due to race condition. I've changed AllocateAndStart to be synchronous so that when AllocateAndStart returns the device is already opened in the camera HAL.
Finished the review of camera_device_delegate.h. camera_device_delegate.cc is reviewed until Initialize(). https://codereview.chromium.org/2837273004/diff/160001/media/capture/video/ch... File media/capture/video/chromeos/camera_device_delegate.cc (right): https://codereview.chromium.org/2837273004/diff/160001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:22: } const kSupportedFormats[] = { Document more. (1) why we need to use video recording format (2) why chrome doesn't know the actual format of implementation defined (3) video recording will support YCbCr_420_888 (4) preview only supports _IMPLEMENTATION_DEFINED https://codereview.chromium.org/2837273004/diff/160001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:97: partial_results_.clear(); Do we need to reset rotation_? Reset first_frame_shutter_time_ for consistency. https://codereview.chromium.org/2837273004/diff/160001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:128: if (!device_ops_.is_bound() || state_ == kStopping) { StopAndDeAllocate shouldn't be called twice in a row. Otherwise closed_ will be overridden. DCHECK_NE(kStopping, state_); https://codereview.chromium.org/2837273004/diff/160001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:130: // unbound. Signal |closed| https://codereview.chromium.org/2837273004/diff/160001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:186: // channel before we do. Document this means we don't get OnClosed from HAL. https://codereview.chromium.org/2837273004/diff/160001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:494: CaptureResult& partial_result = partial_results_[frame_number]; Document a new partial result may be created here or Notify. Initialize partial_stage. https://codereview.chromium.org/2837273004/diff/160001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:504: partial_result.buffers.end()) { Let's use LOG(ERROR) to make this is printed. Client may only use VLOG. https://codereview.chromium.org/2837273004/diff/160001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:590: CaptureResult& partial_result = partial_results_[frame_number]; Document a new partial result may be created here or ProcessCaptureResult. We need to initialize partial_stage here. Otherwise line 603 may hit. https://codereview.chromium.org/2837273004/diff/160001/media/capture/video/ch... File media/capture/video/chromeos/camera_device_delegate.h (right): https://codereview.chromium.org/2837273004/diff/160001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.h:24: // in CameraDeviceDelegate runs on |ipc_task_runner_| and hence all the s/runs/run/ https://codereview.chromium.org/2837273004/diff/160001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.h:117: // Sets the frame rotation angle in |rotation_|. Document this in more detail. What's this for? Is this clockwise or counter clockwise? https://codereview.chromium.org/2837273004/diff/160001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.h:186: // The capture loop runs asynchronously. (1), (2), (3) and (4) may be "The capture loop runs asynchronously." is not very clear. Rephrase this. https://codereview.chromium.org/2837273004/diff/160001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.h:234: base::WaitableEvent* closed_; Document the client owns this. https://codereview.chromium.org/2837273004/diff/160001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.h:249: uint32_t frame_number_; How does HAL handle wrap-around? If the camera is used for monitoring, camera can run a very long time and reach int32. https://codereview.chromium.org/2837273004/diff/160001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.h:269: auto it = streams_.find(stream_type); The style guides says the implementation should not be in the header file. https://codereview.chromium.org/2837273004/diff/160001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.h:277: std::unordered_map<arc::mojom::Camera3RequestTemplate, StreamContext> Let's use array or vector because we'll need to access the map 30 times per second. The index is from 1 (or 0?) to number of types. The overhead can be less. https://codereview.chromium.org/2837273004/diff/160001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.h:293: std::unordered_map<arc::mojom::Camera3RequestTemplate, JPEG (picture callback) won't be implemented anytime soon (not in Q2 or Q3). Let's hard code the type to preview so this file is simpler. https://codereview.chromium.org/2837273004/diff/160001/media/capture/video/ch... File media/capture/video/chromeos/video_capture_device_factory_chromeos.cc (right): https://codereview.chromium.org/2837273004/diff/160001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_factory_chromeos.cc:45: #if defined(OS_CHROMEOS) We need to use the new capture device only for MIPI camera HAL so this CL can be merged first.
https://codereview.chromium.org/2837273004/diff/160001/media/capture/video/ch... File media/capture/video/chromeos/camera_device_delegate.cc (right): https://codereview.chromium.org/2837273004/diff/160001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:22: } const kSupportedFormats[] = { On 2017/05/08 04:22:02, wuchengli wrote: > Document more. (1) why we need to use video recording format (2) why chrome > doesn't know the actual format of implementation defined (3) video recording > will support YCbCr_420_888 (4) preview only supports _IMPLEMENTATION_DEFINED Done. https://codereview.chromium.org/2837273004/diff/160001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:97: partial_results_.clear(); On 2017/05/08 04:22:02, wuchengli wrote: > Do we need to reset rotation_? > > Reset first_frame_shutter_time_ for consistency. We shouldn't reset |rotation_| because it should be controlled by ScreenObserverDelegate in VideoCaptureDeviceArcChromeOS. |first_frame_shutter_time_| is reset here for consistency. https://codereview.chromium.org/2837273004/diff/160001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:128: if (!device_ops_.is_bound() || state_ == kStopping) { On 2017/05/08 04:22:02, wuchengli wrote: > StopAndDeAllocate shouldn't be called twice in a row. Otherwise closed_ will be > overridden. > DCHECK_NE(kStopping, state_); Done. https://codereview.chromium.org/2837273004/diff/160001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:130: // unbound. On 2017/05/08 04:22:02, wuchengli wrote: > Signal |closed| Done. https://codereview.chromium.org/2837273004/diff/160001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:186: // channel before we do. On 2017/05/08 04:22:02, wuchengli wrote: > Document this means we don't get OnClosed from HAL. Done. https://codereview.chromium.org/2837273004/diff/160001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:494: CaptureResult& partial_result = partial_results_[frame_number]; On 2017/05/08 04:22:02, wuchengli wrote: > Document a new partial result may be created here or Notify. Initialize > partial_stage. Documented. |partial_stage| is initialized in CaptureResult's constructor. https://codereview.chromium.org/2837273004/diff/160001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:504: partial_result.buffers.end()) { On 2017/05/08 04:22:02, wuchengli wrote: > Let's use LOG(ERROR) to make this is printed. Client may only use VLOG. Done. https://codereview.chromium.org/2837273004/diff/160001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:590: CaptureResult& partial_result = partial_results_[frame_number]; On 2017/05/08 04:22:02, wuchengli wrote: > Document a new partial result may be created here or ProcessCaptureResult. > > We need to initialize partial_stage here. Otherwise line 603 may hit. Done. https://codereview.chromium.org/2837273004/diff/160001/media/capture/video/ch... File media/capture/video/chromeos/camera_device_delegate.h (right): https://codereview.chromium.org/2837273004/diff/160001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.h:24: // in CameraDeviceDelegate runs on |ipc_task_runner_| and hence all the On 2017/05/08 04:22:03, wuchengli wrote: > s/runs/run/ Done. https://codereview.chromium.org/2837273004/diff/160001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.h:117: // Sets the frame rotation angle in |rotation_|. On 2017/05/08 04:22:02, wuchengli wrote: > Document this in more detail. What's this for? Is this clockwise or counter > clockwise? Done. https://codereview.chromium.org/2837273004/diff/160001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.h:186: // The capture loop runs asynchronously. (1), (2), (3) and (4) may be On 2017/05/08 04:22:03, wuchengli wrote: > "The capture loop runs asynchronously." is not very clear. Rephrase this. I've remove this since it's not important information. https://codereview.chromium.org/2837273004/diff/160001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.h:234: base::WaitableEvent* closed_; On 2017/05/08 04:22:02, wuchengli wrote: > Document the client owns this. Done. https://codereview.chromium.org/2837273004/diff/160001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.h:249: uint32_t frame_number_; On 2017/05/08 04:22:02, wuchengli wrote: > How does HAL handle wrap-around? If the camera is used for monitoring, camera > can run a very long time and reach int32. It should be okay for frame number to wrap around. The frame number should only need to be unique among the maximum possible of concurrently in-flight frames, which is typically < 10. https://codereview.chromium.org/2837273004/diff/160001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.h:269: auto it = streams_.find(stream_type); On 2017/05/08 04:22:02, wuchengli wrote: > The style guides says the implementation should not be in the header file. Done. https://codereview.chromium.org/2837273004/diff/160001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.h:277: std::unordered_map<arc::mojom::Camera3RequestTemplate, StreamContext> On 2017/05/08 04:22:03, wuchengli wrote: > Let's use array or vector because we'll need to access the map 30 times per > second. The index is from 1 (or 0?) to number of types. The overhead can be > less. This is now a unique ptr of a StreamContext after I refactor the code to only use preview stream. https://codereview.chromium.org/2837273004/diff/160001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.h:293: std::unordered_map<arc::mojom::Camera3RequestTemplate, On 2017/05/08 04:22:03, wuchengli wrote: > JPEG (picture callback) won't be implemented anytime soon (not in Q2 or Q3). > Let's hard code the type to preview so this file is simpler. Done. https://codereview.chromium.org/2837273004/diff/160001/media/capture/video/ch... File media/capture/video/chromeos/video_capture_device_factory_chromeos.cc (right): https://codereview.chromium.org/2837273004/diff/160001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_factory_chromeos.cc:45: #if defined(OS_CHROMEOS) On 2017/05/08 04:22:03, wuchengli wrote: > We need to use the new capture device only for MIPI camera HAL so this CL can be > merged first. Done.
Finished camera_device_delegate.cc. https://codereview.chromium.org/2837273004/diff/260001/media/capture/video/ch... File media/capture/video/chromeos/camera_device_delegate.cc (right): https://codereview.chromium.org/2837273004/diff/260001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:220: } Clear stream_context_ here. Document why we should not clear it in StopAndDeAllocate. https://codereview.chromium.org/2837273004/diff/260001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:256: DCHECK(state_ == kInitialized || state_ == kStopping); remove state_ == kStopping https://codereview.chromium.org/2837273004/diff/260001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:282: SetErrorState(FROM_HERE, "Wrong number of streams configured"); print updated_config->streams.size() https://codereview.chromium.org/2837273004/diff/260001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:309: buffer->Create(options); check return value and print error. Same for buffer->Map. https://codereview.chromium.org/2837273004/diff/260001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:345: // start still capture while the preview capture is running. This comment can be removed because we don't support still capture https://codereview.chromium.org/2837273004/diff/260001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:346: DCHECK(state_ == kStreamConfigured || state_ == kCapturing || remove state_ == kCapturing https://codereview.chromium.org/2837273004/diff/260001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:379: SetErrorState(FROM_HERE, "Unsupported video pixel format"); print buffer_format https://codereview.chromium.org/2837273004/diff/260001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:401: strides[i] = VideoFrame::RowBytes(i, buffer_format, stream->width); Use params.requested_format.frame_size.width to be consistent with line 406. https://codereview.chromium.org/2837273004/diff/260001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:406: VideoFrame::PlaneSize(buffer_format, i, i-1 https://codereview.chromium.org/2837273004/diff/260001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:419: void CameraDeviceDelegate::OnRegisteredBuffer(size_t buffer_index, s/buffer_index/buffer_id/ https://codereview.chromium.org/2837273004/diff/260001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:428: SetErrorState(FROM_HERE, "Failed to register buffer"); print result. Same for other IPC result if it contains an error code. https://codereview.chromium.org/2837273004/diff/260001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:434: void CameraDeviceDelegate::ProcessCaptureRequest(size_t buffer_index) { s/buffer_index/buffer_id/ https://codereview.chromium.org/2837273004/diff/260001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:457: frame_number_++; Handle wraparound. Start with 1 so the initial timestamp is not set again? https://codereview.chromium.org/2837273004/diff/260001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:484: client_->OnError(FROM_HERE, Use SetErrorState https://codereview.chromium.org/2837273004/diff/260001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:485: "Incorrect number of output buffers received"); print output_buffers->size() https://codereview.chromium.org/2837273004/diff/260001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:494: if (!partial_result.buffer.is_null()) { Use SetErrorState so the preview will be stopped and we can catch the bug earlier. https://codereview.chromium.org/2837273004/diff/260001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:505: partial_result.partial_stage = result->partial_result; Check Android API if partial_result is increasing. https://codereview.chromium.org/2837273004/diff/260001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:509: if (partial_result.partial_stage == partial_result_count_ && Let's do some error checking because HAL will definitely have bugs. For example, result->partial_result <= partial_result_count_ https://codereview.chromium.org/2837273004/diff/260001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:538: // buffers will be reused in SubmitCaptureResult. Make this this is printed because this shouldn't happen. Or use LOG(WARNING). Same for OnLog below. https://codereview.chromium.org/2837273004/diff/260001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:576: uint64_t shutter_time = message->message->get_shutter()->timestamp; Check if the timestamp is not 0. Otherwise, preview will be stuck. https://codereview.chromium.org/2837273004/diff/260001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:591: partial_result.timestamp = reference_time - first_frame_shutter_time_; This can be extracted. reference_time - first_frame_shutter_time_ will be 0 if the frame number is 0. https://codereview.chromium.org/2837273004/diff/260001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:641: ipc_task_runner_->PostTask( Call RegisterBuffer directly because it's the same thread.
https://codereview.chromium.org/2837273004/diff/260001/media/capture/video/ch... File media/capture/video/chromeos/camera_device_delegate.cc (right): https://codereview.chromium.org/2837273004/diff/260001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:220: } On 2017/05/17 03:40:12, wuchengli wrote: > Clear stream_context_ here. Document why we should not clear it in > StopAndDeAllocate. Done. https://codereview.chromium.org/2837273004/diff/260001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:256: DCHECK(state_ == kInitialized || state_ == kStopping); On 2017/05/17 03:40:12, wuchengli wrote: > remove state_ == kStopping Done. https://codereview.chromium.org/2837273004/diff/260001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:282: SetErrorState(FROM_HERE, "Wrong number of streams configured"); On 2017/05/17 03:40:13, wuchengli wrote: > print updated_config->streams.size() Done. https://codereview.chromium.org/2837273004/diff/260001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:309: buffer->Create(options); On 2017/05/17 03:40:12, wuchengli wrote: > check return value and print error. Same for buffer->Map. Done. https://codereview.chromium.org/2837273004/diff/260001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:345: // start still capture while the preview capture is running. On 2017/05/17 03:40:13, wuchengli wrote: > This comment can be removed because we don't support still capture Done. https://codereview.chromium.org/2837273004/diff/260001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:346: DCHECK(state_ == kStreamConfigured || state_ == kCapturing || On 2017/05/17 03:40:13, wuchengli wrote: > remove state_ == kCapturing Done. https://codereview.chromium.org/2837273004/diff/260001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:379: SetErrorState(FROM_HERE, "Unsupported video pixel format"); On 2017/05/17 03:40:12, wuchengli wrote: > print buffer_format Done. https://codereview.chromium.org/2837273004/diff/260001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:401: strides[i] = VideoFrame::RowBytes(i, buffer_format, stream->width); On 2017/05/17 03:40:13, wuchengli wrote: > Use params.requested_format.frame_size.width to be consistent with line 406. Done. https://codereview.chromium.org/2837273004/diff/260001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:406: VideoFrame::PlaneSize(buffer_format, i, On 2017/05/17 03:40:13, wuchengli wrote: > i-1 Done. https://codereview.chromium.org/2837273004/diff/260001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:419: void CameraDeviceDelegate::OnRegisteredBuffer(size_t buffer_index, On 2017/05/17 03:40:13, wuchengli wrote: > s/buffer_index/buffer_id/ Done. https://codereview.chromium.org/2837273004/diff/260001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:428: SetErrorState(FROM_HERE, "Failed to register buffer"); On 2017/05/17 03:40:13, wuchengli wrote: > print result. Same for other IPC result if it contains an error code. Done. https://codereview.chromium.org/2837273004/diff/260001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:434: void CameraDeviceDelegate::ProcessCaptureRequest(size_t buffer_index) { On 2017/05/17 03:40:13, wuchengli wrote: > s/buffer_index/buffer_id/ Done. https://codereview.chromium.org/2837273004/diff/260001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:457: frame_number_++; On 2017/05/17 03:40:12, wuchengli wrote: > Handle wraparound. Start with 1 so the initial timestamp is not set again? Done. https://codereview.chromium.org/2837273004/diff/260001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:484: client_->OnError(FROM_HERE, On 2017/05/17 03:40:13, wuchengli wrote: > Use SetErrorState Done. https://codereview.chromium.org/2837273004/diff/260001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:485: "Incorrect number of output buffers received"); On 2017/05/17 03:40:13, wuchengli wrote: > print output_buffers->size() Done. https://codereview.chromium.org/2837273004/diff/260001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:494: if (!partial_result.buffer.is_null()) { On 2017/05/17 03:40:12, wuchengli wrote: > Use SetErrorState so the preview will be stopped and we can catch the bug > earlier. Done. https://codereview.chromium.org/2837273004/diff/260001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:505: partial_result.partial_stage = result->partial_result; On 2017/05/17 03:40:13, wuchengli wrote: > Check Android API if partial_result is increasing. Per camera HALv3 API it's not required for partial_result to be increasing; the partial_result only needs to be distinct. I've changed the handling of partial_result below. https://codereview.chromium.org/2837273004/diff/260001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:509: if (partial_result.partial_stage == partial_result_count_ && On 2017/05/17 03:40:13, wuchengli wrote: > Let's do some error checking because HAL will definitely have bugs. For example, > result->partial_result <= partial_result_count_ Done. https://codereview.chromium.org/2837273004/diff/260001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:538: // buffers will be reused in SubmitCaptureResult. On 2017/05/17 03:40:13, wuchengli wrote: > Make this this is printed because this shouldn't happen. Or use LOG(WARNING). > Same for OnLog below. Done. https://codereview.chromium.org/2837273004/diff/260001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:576: uint64_t shutter_time = message->message->get_shutter()->timestamp; On 2017/05/17 03:40:12, wuchengli wrote: > Check if the timestamp is not 0. Otherwise, preview will be stuck. Done. https://codereview.chromium.org/2837273004/diff/260001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:591: partial_result.timestamp = reference_time - first_frame_shutter_time_; On 2017/05/17 03:40:13, wuchengli wrote: > This can be extracted. > reference_time - first_frame_shutter_time_ will be 0 if the frame number is 0. Done. https://codereview.chromium.org/2837273004/diff/260001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:641: ipc_task_runner_->PostTask( On 2017/05/17 03:40:13, wuchengli wrote: > Call RegisterBuffer directly because it's the same thread. Done.
Does this patch pass VideoCaptureDeviceUnittest? Add that in TEST= after passing the test. We can remove "WIP". This should be merged in a few weeks.
Finished camera_metadata_utils.h/.cc. https://codereview.chromium.org/2837273004/diff/280001/media/capture/video/ch... File media/capture/video/chromeos/camera_metadata_utils.cc (right): https://codereview.chromium.org/2837273004/diff/280001/media/capture/video/ch... media/capture/video/chromeos/camera_metadata_utils.cc:26: arc::mojom::CameraMetadataPtr result = arc::mojom::CameraMetadata::New(); remove. unused https://codereview.chromium.org/2837273004/diff/280001/media/capture/video/ch... media/capture/video/chromeos/camera_metadata_utils.cc:32: if (!from->entries.has_value()) return; https://codereview.chromium.org/2837273004/diff/280001/media/capture/video/ch... media/capture/video/chromeos/camera_metadata_utils.cc:43: for (const auto& e : from->entries.value()) { s/e/entry/
Finished another round review of .gn, DEPS, and OWNERS. https://codereview.chromium.org/2837273004/diff/280001/media/capture/video/ch... File media/capture/video/chromeos/OWNERS (right): https://codereview.chromium.org/2837273004/diff/280001/media/capture/video/ch... media/capture/video/chromeos/OWNERS:2: wuchengli@chromium.org Add posciak@chromium.org
Finished another round of review for *.mojom. https://codereview.chromium.org/2837273004/diff/280001/media/capture/video/ch... File media/capture/video/chromeos/mojo/arc_camera3.mojom (right): https://codereview.chromium.org/2837273004/diff/280001/media/capture/video/ch... media/capture/video/chromeos/mojo/arc_camera3.mojom:9: [Extensible] Discussed offline. Keep extensible because android has lots of other HAL_PIXEL_FORMAT_ and we only list what we use here. https://codereview.chromium.org/2837273004/diff/280001/media/capture/video/ch... media/capture/video/chromeos/mojo/arc_camera3.mojom:45: [Extensible] remove because we don't plan to add any vendor mode. https://codereview.chromium.org/2837273004/diff/280001/media/capture/video/ch... media/capture/video/chromeos/mojo/arc_camera3.mojom:52: [Extensible] remove because we don't plan to add any vendor template.
Finished another round of video_capture_device_factory_chromeos.h/.cc. Reviewing video_capture_device_arc_chromeos.cc. We were still discussing the threading and lifecycle. https://codereview.chromium.org/2837273004/diff/280001/media/capture/video/ch... File media/capture/video/chromeos/video_capture_device_arc_chromeos.cc (right): https://codereview.chromium.org/2837273004/diff/280001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_arc_chromeos.cc:147: device_thread_.Start(); check return value https://codereview.chromium.org/2837273004/diff/280001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_arc_chromeos.cc:152: base::Passed(&client), base::Unretained(&started))); |started| will be destroyed after timeout and |camera_device_delegate_| is still alive. Discussed offline. This class doesn't own camera_hal_delegate_. If we put |started| in a class variable, it's still not safe. https://codereview.chromium.org/2837273004/diff/280001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_arc_chromeos.cc:153: started.TimedWait(kEventWaitTimeoutMs); print a warning if it times out. Same for other WaitableEvent. https://codereview.chromium.org/2837273004/diff/280001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_arc_chromeos.cc:164: camera_device_delegate_, base::Unretained(&closed))); |closed| may be destroyed when |camera_device_delegate_| is still alive. Discussed offline. camera_device_delegate_ operates on |device_thread_|. |this| owns |device_thread_|. https://codereview.chromium.org/2837273004/diff/280001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_arc_chromeos.cc:168: device_thread_.Stop(); Do we need |closed| if Stop will return until all tasks finish? https://codereview.chromium.org/2837273004/diff/280001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_arc_chromeos.cc:204: if (result) { Print an error log. Same for line 226. https://codereview.chromium.org/2837273004/diff/280001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_arc_chromeos.cc:205: client->OnError(FROM_HERE, "Failed to get camera info"); started->Signal(); https://codereview.chromium.org/2837273004/diff/280001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_arc_chromeos.cc:226: client->OnError(FROM_HERE, "Failed to open camera device"); started->Signal(); https://codereview.chromium.org/2837273004/diff/280001/media/capture/video/ch... File media/capture/video/chromeos/video_capture_device_arc_chromeos.h (right): https://codereview.chromium.org/2837273004/diff/280001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_arc_chromeos.h:88: // place. Started in CreateDeviceOnCaptureThread and stopped in s/CreateDeviceOnCaptureThread/AllocateAndStart https://codereview.chromium.org/2837273004/diff/280001/media/capture/video/ch... File media/capture/video/chromeos/video_capture_device_factory_chromeos.h (right): https://codereview.chromium.org/2837273004/diff/280001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_factory_chromeos.h:34: // succeeds. This method is called on the browser IO thread when the factory s/IO/UI/
Emircan, Miguel, and Christian. I've finished a round of review and should finish the second round next week. I think this patch is ready for an owner to review. May I know who will be the reviewer(s) for this patch? We can have a meeting to go through the CL if you want. Ken. Can you look at IPC files again? Thanks.
mojom lgtm
On 2017/05/18 06:25:10, wuchengli wrote: > Emircan, Miguel, and Christian. I've finished a round of review and should > finish the second round next week. I think this patch is ready for an owner to > review. May I know who will be the reviewer(s) for this patch? We can have a > meeting to go through the CL if you want. Assuming Emircan and Miguel do not object, I think I can do this review. I'll start working my way through it next week and will let you know if I reach a point where going through the CL together in a meeting would be most effective. > > Ken. Can you look at IPC files again? Thanks.
https://codereview.chromium.org/2837273004/diff/280001/media/capture/video/ch... File media/capture/video/chromeos/OWNERS (right): https://codereview.chromium.org/2837273004/diff/280001/media/capture/video/ch... media/capture/video/chromeos/OWNERS:2: wuchengli@chromium.org On 2017/05/18 02:52:40, wuchengli wrote: > Add mailto:posciak@chromium.org Done. https://codereview.chromium.org/2837273004/diff/280001/media/capture/video/ch... File media/capture/video/chromeos/camera_metadata_utils.cc (right): https://codereview.chromium.org/2837273004/diff/280001/media/capture/video/ch... media/capture/video/chromeos/camera_metadata_utils.cc:26: arc::mojom::CameraMetadataPtr result = arc::mojom::CameraMetadata::New(); On 2017/05/18 02:43:05, wuchengli wrote: > remove. unused Done. https://codereview.chromium.org/2837273004/diff/280001/media/capture/video/ch... media/capture/video/chromeos/camera_metadata_utils.cc:32: On 2017/05/18 02:43:05, wuchengli wrote: > if (!from->entries.has_value()) > return; Done. https://codereview.chromium.org/2837273004/diff/280001/media/capture/video/ch... media/capture/video/chromeos/camera_metadata_utils.cc:43: for (const auto& e : from->entries.value()) { On 2017/05/18 02:43:05, wuchengli wrote: > s/e/entry/ Done. https://codereview.chromium.org/2837273004/diff/280001/media/capture/video/ch... File media/capture/video/chromeos/mojo/arc_camera3.mojom (right): https://codereview.chromium.org/2837273004/diff/280001/media/capture/video/ch... media/capture/video/chromeos/mojo/arc_camera3.mojom:45: [Extensible] On 2017/05/18 03:23:42, wuchengli wrote: > remove because we don't plan to add any vendor mode. Done. https://codereview.chromium.org/2837273004/diff/280001/media/capture/video/ch... media/capture/video/chromeos/mojo/arc_camera3.mojom:52: [Extensible] On 2017/05/18 03:23:42, wuchengli wrote: > remove because we don't plan to add any vendor template. Done. https://codereview.chromium.org/2837273004/diff/280001/media/capture/video/ch... File media/capture/video/chromeos/video_capture_device_arc_chromeos.cc (right): https://codereview.chromium.org/2837273004/diff/280001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_arc_chromeos.cc:147: device_thread_.Start(); On 2017/05/18 04:26:14, wuchengli wrote: > check return value Done. https://codereview.chromium.org/2837273004/diff/280001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_arc_chromeos.cc:152: base::Passed(&client), base::Unretained(&started))); On 2017/05/18 04:26:14, wuchengli wrote: > |started| will be destroyed after timeout and |camera_device_delegate_| is still > alive. > > Discussed offline. This class doesn't own camera_hal_delegate_. If we put > |started| in a class variable, it's still not safe. These are removed after I moved the logic into CameraDeviceDelegate. Same for StopAndDeAllocate. https://codereview.chromium.org/2837273004/diff/280001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_arc_chromeos.cc:204: if (result) { On 2017/05/18 04:26:14, wuchengli wrote: > Print an error log. Same for line 226. Done. https://codereview.chromium.org/2837273004/diff/280001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_arc_chromeos.cc:205: client->OnError(FROM_HERE, "Failed to get camera info"); On 2017/05/18 04:26:14, wuchengli wrote: > started->Signal(); Done. https://codereview.chromium.org/2837273004/diff/280001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_arc_chromeos.cc:226: client->OnError(FROM_HERE, "Failed to open camera device"); On 2017/05/18 04:26:14, wuchengli wrote: > started->Signal(); Done. https://codereview.chromium.org/2837273004/diff/280001/media/capture/video/ch... File media/capture/video/chromeos/video_capture_device_arc_chromeos.h (right): https://codereview.chromium.org/2837273004/diff/280001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_arc_chromeos.h:88: // place. Started in CreateDeviceOnCaptureThread and stopped in On 2017/05/18 04:26:14, wuchengli wrote: > s/CreateDeviceOnCaptureThread/AllocateAndStart Done. https://codereview.chromium.org/2837273004/diff/280001/media/capture/video/ch... File media/capture/video/chromeos/video_capture_device_factory_chromeos.h (right): https://codereview.chromium.org/2837273004/diff/280001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_factory_chromeos.h:34: // succeeds. This method is called on the browser IO thread when the factory On 2017/05/18 04:26:14, wuchengli wrote: > s/IO/UI/ Done.
Description was changed from ========== WIP: media: add video capture device for ARC++ camera HAL v3 This CL adds VideoCaptureDevice and VideoCaptureDeviceFactory for the ARC++ camera HAL v3. The VCD and VCD factory talk to the HAL adapter process on Chrome OS throgh Mojo IPC to access the camera service. BUG=b:32690003 TEST=Make sure camera preview works in hangout. ========== to ========== media: add video capture device for ARC++ camera HAL v3 This CL adds VideoCaptureDevice and VideoCaptureDeviceFactory for the ARC++ camera HAL v3. The VCD and VCD factory talk to the HAL adapter process on Chrome OS throgh Mojo IPC to access the camera service. BUG=b:32690003 TEST=Make sure camera preview works in hangout. TEST=unit tests ==========
wuchengli@chromium.org changed reviewers: - emircan@chromium.org, henryhsu@chromium.org, mcasas@chromium.org, posciak@chromium.org
Reviewed another round of video_capture_device_arc_chromeos.h/cc. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... File media/capture/video/chromeos/video_capture_device_arc_chromeos.cc (right): https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_arc_chromeos.cc:155: if (!camera_device_delegate_) return; In case device_thread_ fails to start in line 139. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_arc_chromeos.cc:164: while (!camera_device_delegate_->HasOneRef()) { Note to myself: need to think about this code.
Doing another round for CameraDeviceDelegate. Just finished review ConfigureStreams. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... File media/capture/video/chromeos/camera_device_delegate.cc (right): https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:40: // TODO(jcliang): Change NV12 to I420 after the camera HAL supports hanlding handling https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:104: base::Bind(&CameraDeviceDelegate::OnGotCameraInfoOnModuleDelegate, this)); There seems to be a utility macro of function to bind the thread when calling this. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:260: if (state_ == kStopping) { Document mojo will disconnect because we don't keep a reference to it. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:348: DCHECK(state_ == kInitialized || state_ == kStopping); Discussed offline. Having DCHECK depends on the implementation of HAL to serialize IPCs. For example, ConfigureStreams and Close. So CameraDeviceDelegate won't get OnConfigureStreams after OnClose. We can change the code to if (state_ != kInitialized && state_ != kStopping) return; So this doesn't depend on HAL implementation. Also, it's better not to have any chance of crashing browser process. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:570: return if state is kStopping or kStopped https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:631: return if state is kStopping or kStopped https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... File media/capture/video/chromeos/camera_device_delegate.h (right): https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.h:27: // access to member variables is sequenced. Document why we need this to be reference counted. - HAL may be stuck and IPC doesn't return. When this happens, chrome doesn't wait forever and will timeout. The client can destroy VideoCaptureDeviceArcChromeOS. VideoCaptureDeviceArcChromeOS will release the reference to CameraDeviceDelegate and stops the thread. Bind will keep a reference to CameraDeviceDelegate. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.h:39: // The kStarting state is set in AllocateAndStart(). As discussed, add some documentation. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... File media/capture/video/chromeos/camera_hal_delegate.h (right): https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.h:20: Explain why we need to make this reference counted. - video_capture_device_factory_chromeos.cc needs to use CameraHalDelegate. - camera_device_delegate.cc needs to use CameraHalDelegate. video_capture_device_factory_chromeos may be destroyed when camera_device_delegate is still alive.
Some comments and questions from a pass of reading the *video_capture_device* files. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... File media/capture/video/chromeos/video_capture_device_arc_chromeos.cc (right): https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_arc_chromeos.cc:17: // thread to the media thread. What is "the media thread"? https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_arc_chromeos.cc:25: // ScreenObserverDelegate instance. I am not convinced that this is accurate. Looking at the implementation, it seems that ~VideoCaptureDeviceArcChromeOS() calls RemoveObserver(), which, in turn, posts to ui_task_runner with shared ownership to |this|. Could it be that the SceenObserverDelegate does outlive |capture_device| and the reason the raw pointer is safe is because we reset it to nullptr in RemoveObserver()? https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_arc_chromeos.cc:62: display::Screen* screen = display::Screen::GetScreen(); When I see a dependency on a global like this, I get worried about testability. It is probably okay to not unittest SceenObserverDelegate, since it is not very complex. Not sure about VideoCaptureDeviceArcChromeOS, though. We should consider injecting a ScreenObserverDelegate instance into its constructor instead of creating one in there. This would break the dependency on the global and allow testing in environments where display::Screen::GetScreen() is not available. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_arc_chromeos.cc:123: return CameraDeviceDelegate::PixFormatHalToChromium(from); nit: Since these are public static both here and in CameraDeviceDelegate, we could probably just extract them into a separate class that clients could use directly. This would eliminate the need for these forwarding methods. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_arc_chromeos.cc:164: while (!camera_device_delegate_->HasOneRef()) { On 2017/05/23 02:56:02, wuchengli wrote: > Note to myself: need to think about this code. I also have a strange feeling about this spinning and polling. Feels like a we are using the reference count as a hidden communication channel between this and other modules. If we need to wait for other modules to release their references here, could these modules send us a callback when they are done, instead? https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_arc_chromeos.cc:242: if (device_thread_.IsRunning()) { This check seems to indicate that rotation events may arrive while the device is stopped. If that is the case, do we need to cache the rotation information and provide it to the device once it is started? Or does it get the rotation information from somewhere else when it is started? https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... File media/capture/video/chromeos/video_capture_device_arc_chromeos.h (right): https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_arc_chromeos.h:68: // are called. ... are expected to be called on? https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_arc_chromeos.h:75: base::Thread device_thread_; At this point, as a reader, I do not understand why we need two separate threads, |module_thread_| for operating CameraHalDelegate and |device_thread_| for operating CameraDeviceDelegate. Since the operation should mostly consist of sending and receiving Mojo messages, I would not expect a lot of congestion, so why is a single thread for all Mojo communication not sufficient? https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_arc_chromeos.h:79: // |camera_hal_delegate_| in OnOpenedDevice and deleted in StopAndDeAllocate Isn't this created in AllocateAndStart()? https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_arc_chromeos.h:88: void SetDisplayRotation(const display::Display& display); Please move class forward declaration and methods above members inside the private section. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... File media/capture/video/chromeos/video_capture_device_factory_chromeos.cc (right): https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_factory_chromeos.cc:30: DCHECK(thread_checker_.CalledOnValidThread()); Not an issue introduced by this CL, but it feels weird to me how |thread_checker_| is a protected member of the base class, that the base class does not actually use. Because of it, as a reader, I have to go and first read the implementation of the base class to understand that this thread checker does not do anything in the base class itself. If you agree, could we clean this up? And if we do, we should probably use the new SEQUENCE_CHECKER macros instead of base::ThreadChecker, unless we really need thread-affinity. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_factory_chromeos.cc:31: return camera_hal_delegate_->CreateDevice(ui_task_runner_, device_descriptor); Nit: Should we DCHECK that Init() has been called before |camera_hal_delegate_| is used? https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_factory_chromeos.cc:56: std::unique_ptr<VideoCaptureDeviceFactoryChromeOS> factory( Please use base::MakeUnique<> here https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... File media/capture/video/chromeos/video_capture_device_factory_chromeos.h (right): https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_factory_chromeos.h:20: scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner); With the move to the video capture Mojo service, this may actually be running in a utility process. In this context it is unclear what "ui_task_runner" would mean. I recommend we rename this to indicate what the task runner is actually being used for, e.g. task_runner_for_xyz. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_factory_chromeos.h:35: // instance is created. Note, that with the move to the video capture Mojo service, VideoCaptureDeviceFactories are no longer necessarily created and operated in the browser process. This code should not make any assumptions about being run on the browser UI thread. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_factory_chromeos.h:48: // issues and receives must be sequenced through |module_thread_|. Nit: |camera_hal_delegate_| is actually create in the call to Init(), and there is nothing in this class guaranteeing that this is the same thread as the on calling the constructor. If we want to guarantee it, let us add a base::SequenceChecker to enforce it. If we don't want to guarantee it, let us relax the claims made in the comment. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/vi... File media/capture/video/video_capture_device_unittest.cc (right): https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/vi... media/capture/video/video_capture_device_unittest.cc:223: class MojoTestEnvironment final : public testing::Environment { Despite the name, this class does not seem to have anything specific to video capture. Would it make sense to rename it to something like "MojoEnabledTestEnvironment" and move it to some more general place, where it can be reused by all tests that need to initialize Mojo?
Finished another round of camera_device_delegate.cc. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... File media/capture/video/chromeos/camera_device_delegate.cc (right): https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:370: stream_context_->stream->max_buffers = updated_stream->max_buffers; Let's have a limit here in case HAL has a bug and give us a very huge number. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:438: SetState(kCapturing); Document we cannot use a loop to register all the buffers here because halv3 API says we cannot call ProcessCaptureRequest if the previous one hasn't returned yet. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:574: CaptureResult& partial_result = partial_results_[frame_number]; Print an error or error out if the number of partial_results_ is bigger than the number of buffers. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:632: if (message->type == arc::mojom::Camera3MsgType::CAMERA3_MSG_ERROR) { This function is long. Line 633-695 can be moved to a function. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:707: CaptureResult& partial_result = partial_results_[frame_number]; Print an error or error out if the number of partial_results_ is bigger than the number of buffers. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:730: SetErrorState(FROM_HERE, "Received out-of-order frames from HAL"); print partial_results_.begin()->first and frame_number https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... File media/capture/video/chromeos/video_capture_device_arc_chromeos.cc (right): https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_arc_chromeos.cc:164: while (!camera_device_delegate_->HasOneRef()) { On 2017/05/24 23:36:43, chfremer wrote: > On 2017/05/23 02:56:02, wuchengli wrote: > > Note to myself: need to think about this code. > > I also have a strange feeling about this spinning and polling. Feels like a we > are using the reference count as a hidden communication channel between this and > other modules. If we need to wait for other modules to release their references > here, could these modules send us a callback when they are done, instead? Ricky. You can consider to move device_thread_ to CameraDeviceDelegate. CameraDeviceDelegate can use WaitableEvenet internally.
Finished another round of camera_device_delegate.cc. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... File media/capture/video/chromeos/camera_device_delegate.cc (right): https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:370: stream_context_->stream->max_buffers = updated_stream->max_buffers; Let's have a limit here in case HAL has a bug and give us a very huge number. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:438: SetState(kCapturing); Document we cannot use a loop to register all the buffers here because halv3 API says we cannot call ProcessCaptureRequest if the previous one hasn't returned yet. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:574: CaptureResult& partial_result = partial_results_[frame_number]; Print an error or error out if the number of partial_results_ is bigger than the number of buffers. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:632: if (message->type == arc::mojom::Camera3MsgType::CAMERA3_MSG_ERROR) { This function is long. Line 633-695 can be moved to a function. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:707: CaptureResult& partial_result = partial_results_[frame_number]; Print an error or error out if the number of partial_results_ is bigger than the number of buffers. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:730: SetErrorState(FROM_HERE, "Received out-of-order frames from HAL"); print partial_results_.begin()->first and frame_number https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... File media/capture/video/chromeos/video_capture_device_arc_chromeos.cc (right): https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_arc_chromeos.cc:164: while (!camera_device_delegate_->HasOneRef()) { On 2017/05/24 23:36:43, chfremer wrote: > On 2017/05/23 02:56:02, wuchengli wrote: > > Note to myself: need to think about this code. > > I also have a strange feeling about this spinning and polling. Feels like a we > are using the reference count as a hidden communication channel between this and > other modules. If we need to wait for other modules to release their references > here, could these modules send us a callback when they are done, instead? Ricky. You can consider to move device_thread_ to CameraDeviceDelegate. CameraDeviceDelegate can use WaitableEvenet internally.
Note to myself: The files I haven't reviewed in the second round: - camera_hal_delegate.h/.cc - camera_device_delegate_unittest.cc - camera_hal_delegate_unittest.h/.cc.
Finished the second round of review for camera_hal_delegate.h/.cc. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... File media/capture/video/chromeos/camera_hal_delegate.cc (right): https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:143: VLOG(1) << "Invalid camera_id: " << camera_id; We have only built-in cameras now. LOG(ERROR) https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:150: supported_formats->emplace_back(gfx::Size(1280, 720), 60.0, Remember to change this to 30 or remove the hack before merging. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:235: // VideoCaptureDeviceArcChromeOS to query camera info. s/VideoCaptureDeviceArcChromeOS/CameraDeviceDelegate/ https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:244: // VideoCaptureDeviceArcChromeOS to open a camera device. s/VideoCaptureDeviceArcChromeOS/CameraDeviceDelegate/ https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:336: // enumerating state or is no longer present. Update the comment because we only have built-in cameras now. LOG(ERROR) https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:343: // info of external cameras as well so we need to check each id of the Update the comment because we only have built-in cameras now. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... File media/capture/video/chromeos/camera_hal_delegate.h (right): https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.h:30: // Called by VideoCaptureDeviceFactoryChromeOS::Init on the browser IO thread. s/IO/UI/
https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... File media/capture/video/chromeos/camera_hal_delegate.cc (right): https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:229: } Let's sort and put the front camera first. It seems Javascript API is not ready yet (http://crbug.com/543997). std::sort(device_descriptors->begin(), device_descriptors->end()); https://codereview.chromium.org/2648973003/diff/180001/media/capture/video/li...
https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... File media/capture/video/chromeos/camera_device_delegate.cc (right): https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:40: // TODO(jcliang): Change NV12 to I420 after the camera HAL supports hanlding On 2017/05/23 04:29:07, wuchengli wrote: > handling Done. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:104: base::Bind(&CameraDeviceDelegate::OnGotCameraInfoOnModuleDelegate, this)); On 2017/05/23 04:29:07, wuchengli wrote: > There seems to be a utility macro of function to bind the thread when calling > this. Found it: https://cs.chromium.org/chromium/src/media/base/bind_to_current_loop.h https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:260: if (state_ == kStopping) { On 2017/05/23 04:29:07, wuchengli wrote: > Document mojo will disconnect because we don't keep a reference to it. Done. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:348: DCHECK(state_ == kInitialized || state_ == kStopping); On 2017/05/23 04:29:07, wuchengli wrote: > Discussed offline. Having DCHECK depends on the implementation of HAL to > serialize IPCs. For example, ConfigureStreams and Close. So CameraDeviceDelegate > won't get OnConfigureStreams after OnClose. We can change the code to > if (state_ != kInitialized && state_ != kStopping) return; > So this doesn't depend on HAL implementation. Also, it's better not to have any > chance of crashing browser process. Done. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:370: stream_context_->stream->max_buffers = updated_stream->max_buffers; On 2017/05/25 07:59:06, wuchengli wrote: > Let's have a limit here in case HAL has a bug and give us a very huge number. Done. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:438: SetState(kCapturing); On 2017/05/25 07:59:06, wuchengli wrote: > Document we cannot use a loop to register all the buffers here because halv3 API > says we cannot call ProcessCaptureRequest if the previous one hasn't returned > yet. Done. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:570: On 2017/05/23 04:29:07, wuchengli wrote: > return if state is kStopping or kStopped Done. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:574: CaptureResult& partial_result = partial_results_[frame_number]; On 2017/05/25 07:59:05, wuchengli wrote: > Print an error or error out if the number of partial_results_ is bigger than the > number of buffers. Done. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:631: On 2017/05/23 04:29:07, wuchengli wrote: > return if state is kStopping or kStopped Done. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:632: if (message->type == arc::mojom::Camera3MsgType::CAMERA3_MSG_ERROR) { On 2017/05/25 07:59:06, wuchengli wrote: > This function is long. Line 633-695 can be moved to a function. Done. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:707: CaptureResult& partial_result = partial_results_[frame_number]; On 2017/05/25 07:59:06, wuchengli wrote: > Print an error or error out if the number of partial_results_ is bigger than the > number of buffers. Done. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:730: SetErrorState(FROM_HERE, "Received out-of-order frames from HAL"); On 2017/05/25 07:59:06, wuchengli wrote: > print partial_results_.begin()->first and frame_number Done. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... File media/capture/video/chromeos/camera_device_delegate.h (right): https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.h:27: // access to member variables is sequenced. On 2017/05/23 04:29:07, wuchengli wrote: > Document why we need this to be reference counted. > - HAL may be stuck and IPC doesn't return. When this happens, chrome doesn't > wait forever and will timeout. The client can destroy > VideoCaptureDeviceArcChromeOS. VideoCaptureDeviceArcChromeOS will release the > reference to CameraDeviceDelegate and stops the thread. Bind will keep a > reference to CameraDeviceDelegate. Done. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.h:39: // The kStarting state is set in AllocateAndStart(). On 2017/05/23 04:29:07, wuchengli wrote: > As discussed, add some documentation. Done. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... File media/capture/video/chromeos/camera_hal_delegate.cc (right): https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:143: VLOG(1) << "Invalid camera_id: " << camera_id; On 2017/05/25 08:24:12, wuchengli wrote: > We have only built-in cameras now. LOG(ERROR) Done. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:229: } On 2017/05/25 09:06:41, wuchengli wrote: > Let's sort and put the front camera first. It seems Javascript API is not ready > yet (http://crbug.com/543997). > > std::sort(device_descriptors->begin(), device_descriptors->end()); > > https://codereview.chromium.org/2648973003/diff/180001/media/capture/video/li... Done. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:235: // VideoCaptureDeviceArcChromeOS to query camera info. On 2017/05/25 08:24:12, wuchengli wrote: > s/VideoCaptureDeviceArcChromeOS/CameraDeviceDelegate/ Done. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:244: // VideoCaptureDeviceArcChromeOS to open a camera device. On 2017/05/25 08:24:12, wuchengli wrote: > s/VideoCaptureDeviceArcChromeOS/CameraDeviceDelegate/ Done. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:336: // enumerating state or is no longer present. On 2017/05/25 08:24:12, wuchengli wrote: > Update the comment because we only have built-in cameras now. > LOG(ERROR) Done. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:343: // info of external cameras as well so we need to check each id of the On 2017/05/25 08:24:12, wuchengli wrote: > Update the comment because we only have built-in cameras now. Done. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... File media/capture/video/chromeos/camera_hal_delegate.h (right): https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.h:20: On 2017/05/23 04:29:07, wuchengli wrote: > Explain why we need to make this reference counted. > - video_capture_device_factory_chromeos.cc needs to use CameraHalDelegate. > - camera_device_delegate.cc needs to use CameraHalDelegate. > video_capture_device_factory_chromeos may be destroyed when > camera_device_delegate is still alive. Done. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.h:30: // Called by VideoCaptureDeviceFactoryChromeOS::Init on the browser IO thread. On 2017/05/25 08:24:12, wuchengli wrote: > s/IO/UI/ Done. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... File media/capture/video/chromeos/video_capture_device_arc_chromeos.cc (right): https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_arc_chromeos.cc:17: // thread to the media thread. On 2017/05/24 23:36:43, chfremer wrote: > What is "the media thread"? I believe it's the media capture thread on which the VCD interface methods are called. The ScreenObserverDelegate class is copied from linux/video_capture_device_chromeos.cc: https://cs.chromium.org/chromium/src/media/capture/video/linux/video_capture_... https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_arc_chromeos.cc:25: // ScreenObserverDelegate instance. On 2017/05/24 23:36:43, chfremer wrote: > I am not convinced that this is accurate. > > Looking at the implementation, it seems that ~VideoCaptureDeviceArcChromeOS() > calls RemoveObserver(), which, in turn, posts to ui_task_runner with shared > ownership to |this|. Could it be that the SceenObserverDelegate does outlive > |capture_device| and the reason the raw pointer is safe is because we reset it > to nullptr in RemoveObserver()? Yes you are correct. I've updated the comments. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_arc_chromeos.cc:62: display::Screen* screen = display::Screen::GetScreen(); On 2017/05/24 23:36:43, chfremer wrote: > When I see a dependency on a global like this, I get worried about testability. > It is probably okay to not unittest SceenObserverDelegate, since it is not very > complex. > Not sure about VideoCaptureDeviceArcChromeOS, though. We should consider > injecting a ScreenObserverDelegate instance into its constructor instead of > creating one in there. This would break the dependency on the global and allow > testing in environments where display::Screen::GetScreen() is not available. I agree having direct dependency on ScreenObserverDelegate in VideoCaptureDeviceArcChromeOS is bad for testing. I've kept the logic in VideoCaptureDeviceArcChromeOS simple and delegating all the heavy lifting to CameraDeviceDelegate. I'm testing CameraDeviceDelegate instead. If somehow we need to test VideoCaptureDeviceArcChromeOS I can refactor the code in another CL. Since the implementation of ScreenObserverDelegate is copied from linux/video_capture_device_chromeos.cc we should fix both places. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_arc_chromeos.cc:123: return CameraDeviceDelegate::PixFormatHalToChromium(from); On 2017/05/24 23:36:43, chfremer wrote: > nit: Since these are public static both here and in CameraDeviceDelegate, we > could probably just extract them into a separate class that clients could use > directly. This would eliminate the need for these forwarding methods. Done. Moved to pixel_format_utils.h/.cc https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_arc_chromeos.cc:155: On 2017/05/23 02:56:02, wuchengli wrote: > if (!camera_device_delegate_) > return; > > In case device_thread_ fails to start in line 139. Done. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_arc_chromeos.cc:164: while (!camera_device_delegate_->HasOneRef()) { On 2017/05/25 07:59:06, wuchengli wrote: > On 2017/05/24 23:36:43, chfremer wrote: > > On 2017/05/23 02:56:02, wuchengli wrote: > > > Note to myself: need to think about this code. > > > > I also have a strange feeling about this spinning and polling. Feels like a we > > are using the reference count as a hidden communication channel between this > and > > other modules. If we need to wait for other modules to release their > references > > here, could these modules send us a callback when they are done, instead? > Ricky. You can consider to move device_thread_ to CameraDeviceDelegate. > CameraDeviceDelegate can use WaitableEvenet internally. > > I'll look into it, but it'll likely be a sizable refactor. Let me post a new patch set addressing other comments first. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_arc_chromeos.cc:242: if (device_thread_.IsRunning()) { On 2017/05/24 23:36:42, chfremer wrote: > This check seems to indicate that rotation events may arrive while the device is > stopped. > If that is the case, do we need to cache the rotation information and provide it > to the device once it is started? > Or does it get the rotation information from somewhere else when it is started? Oh yes! We should preserve the rotation information and set it to |camera_device_delegate_| upon StartAndAllocate. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... File media/capture/video/chromeos/video_capture_device_arc_chromeos.h (right): https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_arc_chromeos.h:68: // are called. On 2017/05/24 23:36:43, chfremer wrote: > ... are expected to be called on? Done. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_arc_chromeos.h:75: base::Thread device_thread_; On 2017/05/24 23:36:43, chfremer wrote: > At this point, as a reader, I do not understand why we need two separate > threads, |module_thread_| for operating CameraHalDelegate and |device_thread_| > for operating CameraDeviceDelegate. Since the operation should mostly consist of > sending and receiving Mojo messages, I would not expect a lot of congestion, so > why is a single thread for all Mojo communication not sufficient? I've thought about this. The reason that I keep them two separate threads is because the CameraDeviceDelegate may need to do some heavy lifting for the captured frame: in CameraDeviceDelegate::SubmitCaptureResult it calls client_->OnIncomingCapturedData which does cpu-hogging format conversion and/or memory copy. The workload gets heavier when we have higher resolution cameras, and Chrome can open multiple cameras at the same time. Running two (high resolution) preview streams on the same thread may increase latency significantly. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_arc_chromeos.h:79: // |camera_hal_delegate_| in OnOpenedDevice and deleted in StopAndDeAllocate On 2017/05/24 23:36:43, chfremer wrote: > Isn't this created in AllocateAndStart()? Done. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_arc_chromeos.h:88: void SetDisplayRotation(const display::Display& display); On 2017/05/24 23:36:43, chfremer wrote: > Please move class forward declaration and methods above members inside the > private section. Done. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... File media/capture/video/chromeos/video_capture_device_factory_chromeos.cc (right): https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_factory_chromeos.cc:30: DCHECK(thread_checker_.CalledOnValidThread()); On 2017/05/24 23:36:43, chfremer wrote: > Not an issue introduced by this CL, but it feels weird to me how > |thread_checker_| is a protected member of the base class, that the base class > does not actually use. Because of it, as a reader, I have to go and first read > the implementation of the base class to understand that this thread checker does > not do anything in the base class itself. If you agree, could we clean this up? > > And if we do, we should probably use the new SEQUENCE_CHECKER macros instead of > base::ThreadChecker, unless we really need thread-affinity. I agree it's a bit weird. Can we do the clean up in another CL because this CL is already a huge one? https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_factory_chromeos.cc:31: return camera_hal_delegate_->CreateDevice(ui_task_runner_, device_descriptor); On 2017/05/24 23:36:43, chfremer wrote: > Nit: Should we DCHECK that Init() has been called before |camera_hal_delegate_| > is used? Done. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_factory_chromeos.cc:56: std::unique_ptr<VideoCaptureDeviceFactoryChromeOS> factory( On 2017/05/24 23:36:43, chfremer wrote: > Please use base::MakeUnique<> here Done. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... File media/capture/video/chromeos/video_capture_device_factory_chromeos.h (right): https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_factory_chromeos.h:20: scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner); On 2017/05/24 23:36:43, chfremer wrote: > With the move to the video capture Mojo service, this may actually be running in > a utility process. In this context it is unclear what "ui_task_runner" would > mean. I recommend we rename this to indicate what the task runner is actually > being used for, e.g. task_runner_for_xyz. Done. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_factory_chromeos.h:35: // instance is created. On 2017/05/24 23:36:43, chfremer wrote: > Note, that with the move to the video capture Mojo service, > VideoCaptureDeviceFactories are no longer necessarily created and operated in > the browser process. This code should not make any assumptions about being run > on the browser UI thread. We do not depend on the thread being run on the browser UI thread. We can remove or update the comments when we move the code to video capture Mojo service. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_factory_chromeos.h:48: // issues and receives must be sequenced through |module_thread_|. On 2017/05/24 23:36:43, chfremer wrote: > Nit: |camera_hal_delegate_| is actually create in the call to Init(), and there > is nothing in this class guaranteeing that this is the same thread as the on > calling the constructor. If we want to guarantee it, let us add a > base::SequenceChecker to enforce it. If we don't want to guarantee it, let us > relax the claims made in the comment. Updated the comment. We don't need to guarantee it. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/vi... File media/capture/video/video_capture_device_unittest.cc (right): https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/vi... media/capture/video/video_capture_device_unittest.cc:223: class MojoTestEnvironment final : public testing::Environment { On 2017/05/24 23:36:43, chfremer wrote: > Despite the name, this class does not seem to have anything specific to video > capture. Would it make sense to rename it to something like > "MojoEnabledTestEnvironment" and move it to some more general place, where it > can be reused by all tests that need to initialize Mojo? Sounds good. Could you point me an appropriate place where I can put this class for general use?
Some replies. No new code reviewed. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... File media/capture/video/chromeos/video_capture_device_arc_chromeos.cc (right): https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_arc_chromeos.cc:17: // thread to the media thread. On 2017/05/25 14:23:47, jcliang wrote: > On 2017/05/24 23:36:43, chfremer wrote: > > What is "the media thread"? > > I believe it's the media capture thread on which the VCD interface methods are > called. > > The ScreenObserverDelegate class is copied from > linux/video_capture_device_chromeos.cc: > > https://cs.chromium.org/chromium/src/media/capture/video/linux/video_capture_... Oh, I see. In that case, can we please avoid duplicating this code by moving the class ScreenObserverDelegate to a pair of files of its own and reuse it in both places? Instead of the concrete types for |capture_device|, use an new interface DisplayRotationObserver and make both VideoCaptureDeviceArcChromeOS and VideoCaptureDeviceChromeOS implement it. IMHO, the reference to "media thread" is not a good idea, because it makes assumptions on the context the class is used in. It would be better to just describe what the class itself does independently of its context, which is "Registers itself as an observer at |display::Screen::GetScreen()| and forwards rotation change events to the given DisplayRotationObserver on the thread where ScreenObserverDelegate was created." https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_arc_chromeos.cc:62: display::Screen* screen = display::Screen::GetScreen(); On 2017/05/25 14:23:47, jcliang wrote: > On 2017/05/24 23:36:43, chfremer wrote: > > When I see a dependency on a global like this, I get worried about > testability. > > It is probably okay to not unittest SceenObserverDelegate, since it is not > very > > complex. > > Not sure about VideoCaptureDeviceArcChromeOS, though. We should consider > > injecting a ScreenObserverDelegate instance into its constructor instead of > > creating one in there. This would break the dependency on the global and allow > > testing in environments where display::Screen::GetScreen() is not available. > > I agree having direct dependency on ScreenObserverDelegate in > VideoCaptureDeviceArcChromeOS is bad for testing. I've kept the logic in > VideoCaptureDeviceArcChromeOS simple and delegating all the heavy lifting to > CameraDeviceDelegate. I'm testing CameraDeviceDelegate instead. > > If somehow we need to test VideoCaptureDeviceArcChromeOS I can refactor the code > in another CL. Since the implementation of ScreenObserverDelegate is copied from > linux/video_capture_device_chromeos.cc we should fix both places. Sounds good. Agreed that VideoCaptureDeviceArcChromeOS is a relatively thin adapter class, which probably does not need to be tested in isolation. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... File media/capture/video/chromeos/video_capture_device_arc_chromeos.h (right): https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_arc_chromeos.h:75: base::Thread device_thread_; On 2017/05/25 14:23:47, jcliang wrote: > On 2017/05/24 23:36:43, chfremer wrote: > > At this point, as a reader, I do not understand why we need two separate > > threads, |module_thread_| for operating CameraHalDelegate and |device_thread_| > > for operating CameraDeviceDelegate. Since the operation should mostly consist > of > > sending and receiving Mojo messages, I would not expect a lot of congestion, > so > > why is a single thread for all Mojo communication not sufficient? > > I've thought about this. The reason that I keep them two separate threads is > because the CameraDeviceDelegate may need to do some heavy lifting for the > captured frame: in CameraDeviceDelegate::SubmitCaptureResult it calls > client_->OnIncomingCapturedData which does cpu-hogging format conversion and/or > memory copy. The workload gets heavier when we have higher resolution cameras, > and Chrome can open multiple cameras at the same time. Running two (high > resolution) preview streams on the same thread may increase latency > significantly. Thanks. That sounds reasonable. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... File media/capture/video/chromeos/video_capture_device_factory_chromeos.cc (right): https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_factory_chromeos.cc:30: DCHECK(thread_checker_.CalledOnValidThread()); On 2017/05/25 14:23:48, jcliang wrote: > On 2017/05/24 23:36:43, chfremer wrote: > > Not an issue introduced by this CL, but it feels weird to me how > > |thread_checker_| is a protected member of the base class, that the base class > > does not actually use. Because of it, as a reader, I have to go and first read > > the implementation of the base class to understand that this thread checker > does > > not do anything in the base class itself. If you agree, could we clean this > up? > > > > And if we do, we should probably use the new SEQUENCE_CHECKER macros instead > of > > base::ThreadChecker, unless we really need thread-affinity. > > I agree it's a bit weird. Can we do the clean up in another CL because this CL > is already a huge one? Agreed. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... File media/capture/video/chromeos/video_capture_device_factory_chromeos.h (right): https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_factory_chromeos.h:35: // instance is created. On 2017/05/25 14:23:48, jcliang wrote: > On 2017/05/24 23:36:43, chfremer wrote: > > Note, that with the move to the video capture Mojo service, > > VideoCaptureDeviceFactories are no longer necessarily created and operated in > > the browser process. This code should not make any assumptions about being run > > on the browser UI thread. > > We do not depend on the thread being run on the browser UI thread. We can remove > or update the comments when we move the code to video capture Mojo service. The thing is that the code does not need to be moved to be used in the video capture Mojo service. This code can (already today) be used in multiple contexts, two of which are the in-process video capture stack and the video capture Mojo service. A third one could be unit tests. In general, I recommend not specifying anything about the context where a class gets used in as part of the class itself, because, even if correct today, this information very easily becomes inaccurate when the context changes and the comment is not updated. If there is any specific requirement on the thread that Init() must be called on, please call out that requirement is specifically, e.g. "must be called from a thread on which it is legal to do XYZ". https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/vi... File media/capture/video/video_capture_device_unittest.cc (right): https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/vi... media/capture/video/video_capture_device_unittest.cc:223: class MojoTestEnvironment final : public testing::Environment { On 2017/05/25 14:23:48, jcliang wrote: > On 2017/05/24 23:36:43, chfremer wrote: > > Despite the name, this class does not seem to have anything specific to video > > capture. Would it make sense to rename it to something like > > "MojoEnabledTestEnvironment" and move it to some more general place, where it > > can be reused by all tests that need to initialize Mojo? > > Sounds good. Could you point me an appropriate place where I can put this class > for general use? I am not sure either, but keeping it here will lead to duplication when other tests need something similar (or maybe some already do and we don't know it because it is also hidden in their .cc file?). I am not an owner of that directory, but src/mojo/edk/embedder/ seems like a reasonable place to me, since this is where mojo::edk::ScopedIPCSupport lives.
https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... File media/capture/video/chromeos/video_capture_device_arc_chromeos.cc (right): https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_arc_chromeos.cc:17: // thread to the media thread. On 2017/05/25 17:13:07, chfremer wrote: > On 2017/05/25 14:23:47, jcliang wrote: > > On 2017/05/24 23:36:43, chfremer wrote: > > > What is "the media thread"? > > > > I believe it's the media capture thread on which the VCD interface methods are > > called. > > > > The ScreenObserverDelegate class is copied from > > linux/video_capture_device_chromeos.cc: > > > > > https://cs.chromium.org/chromium/src/media/capture/video/linux/video_capture_... > > Oh, I see. In that case, can we please avoid duplicating this code by moving the > class ScreenObserverDelegate to a pair of files of its own and reuse it in both > places? Instead of the concrete types for |capture_device|, use an new interface > DisplayRotationObserver and make both VideoCaptureDeviceArcChromeOS and > VideoCaptureDeviceChromeOS implement it. > > IMHO, the reference to "media thread" is not a good idea, because it makes > assumptions on the context the class is used in. It would be better to just > describe what the class itself does independently of its context, which is > "Registers itself as an observer at |display::Screen::GetScreen()| and forwards > rotation change events to the given DisplayRotationObserver on the thread where > ScreenObserverDelegate was created." Agreed. I've extracted the common codes to display_rotation_observer.h/.cc. I didn't extract the codes before because originally our plan was to completely remove linux/video_capture_device_chromeos.h/.cc and migrate to chromeos/video_capture_device_arc_chromeos.h/.cc, but now it looks like the two VCDs have to co-exist for some time during the transition period. https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... File media/capture/video/chromeos/video_capture_device_factory_chromeos.h (right): https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_factory_chromeos.h:35: // instance is created. On 2017/05/25 17:13:07, chfremer wrote: > On 2017/05/25 14:23:48, jcliang wrote: > > On 2017/05/24 23:36:43, chfremer wrote: > > > Note, that with the move to the video capture Mojo service, > > > VideoCaptureDeviceFactories are no longer necessarily created and operated > in > > > the browser process. This code should not make any assumptions about being > run > > > on the browser UI thread. > > > > We do not depend on the thread being run on the browser UI thread. We can > remove > > or update the comments when we move the code to video capture Mojo service. > > The thing is that the code does not need to be moved to be used in the video > capture Mojo service. This code can (already today) be used in multiple > contexts, two of which are the in-process video capture stack and the video > capture Mojo service. A third one could be unit tests. > > In general, I recommend not specifying anything about the context where a class > gets used in as part of the class itself, because, even if correct today, this > information very easily becomes inaccurate when the context changes and the > comment is not updated. > > If there is any specific requirement on the thread that Init() must be called > on, please call out that requirement is specifically, e.g. "must be called from > a thread on which it is legal to do XYZ". I see. Thanks for explaining. I'll remove comments that specifically refer to any browser process threads. Are we using the video capture Mojo service in Chrome already? How can I force the video capture device to run as a Mojo service? I'd like to verify running the VCD in both the browser-process and the Mojo service contexts.
Comments and questions from a pass of looking at the camera_hal_delegate* files. https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... File media/capture/video/chromeos/camera_hal_delegate.cc (right): https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:44: module_task_runner_(module_task_runner), For passing scoped_refptr<>, I believe it is now recommended to pass by value and then use std::move() if no copy is needed. https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:113: } The contents of the above method look reasonable, but I am not familiar with the APIs for establishing Mojo connections. I recommend asking rockot@ or yzshen@ to help review this part. https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... File media/capture/video/chromeos/camera_hal_delegate.h (right): https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.h:24: // function calls to CameraHalDelegate. As a reader, here, I would expect to find a description of what this class does, which is missing here and is not obvious from the class name. The information who is using the class is useful, but dangerous to include in the class-level description, because it is easy to become inaccurate. IMHO, it is better to rely on the code search tool to find usages. https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.h:35: const scoped_refptr<base::SingleThreadTaskRunner> module_task_runner); Either here or in the class-level description it would be helpful to have an explanation of what |module_task_runner| is needed for. https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.h:39: // Called by VideoCaptureDeviceFactoryChromeOS::Init on the browser UI thread. I recommend removing the information about by who and on which thread this gets called (it is already inaccurate). https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.h:45: // on. This raises the following question (in me, as a reader): If the interface exposed here is so similar to VideoCaptureDeviceFactory, what is actually the purpose of VideoCaptureDeviceFactoryChromeOS? Why do we need it? What does it do that is worth having it separate from this class? A good place for this information would be the class-level description. https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.h:55: // be called on any thread. Hmm, the method signature does look asynchronous, though. Does the comment above mean that this implementation guarantees that the callback is invoked synchronously? If so, why not use a return value instead of a callback? https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.h:74: void StartForTesting(arc::mojom::CameraModulePtrInfo info); The need for a test-only API like this is often an indicator of a design issue. In the case of this class, this is caused by the class wanting to do both establishing the Mojo connection and using it, which are almost unrelated responsibilities. I recommend resolving this by splitting out the responsibility of establishing the Mojo connection to a factory class, something like: class ServiceCameraHalDelegateFactory { public: scoped_refptr<CameraHalDelegate> CreateCameraHalDelegate(scoped_refptr<base::SingleThreadTaskRunner> module_task_runner); // ... }; This class would contain the logic from StartCameraModuleIpc(). The class CameraHalDelegate, would get its |camera_module_| via dependency injection from the constructor, i.e.: class CameraHalDelegate { public: CameraHalDelegate( const scoped_refptr<base::SingleThreadTaskRunner> module_task_runner, arc::mojom::CameraModulePtr camera_module); }; Unit tests can then instantiate CameraHalDelegate through its public constructor using a mock for |camera_module|. https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... File media/capture/video/chromeos/camera_hal_delegate_unittest.cc (right): https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate_unittest.cc:42: } Same as in the production code, this while loop looks suspicious and can probably be replaced with a more official way of other reference holders signaling us when they have finished whatever we need to wait for here. https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate_unittest.cc:47: void Wait() { Couldn't find where this is used. Can it be removed? https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate_unittest.cc:57: std::unique_ptr<base::MessageLoop> message_loop_; IIUC, it is now encouraged to use a ScopedTaskEnvironment instead, see https://docs.google.com/document/d/1QabRo8c7D9LsYY3cEcaPQbOCLo8Tu-6VLykYXyl3P... https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate_unittest.cc:67: }; I would be okay with this, but not sure if this may violate the Chromium C++11 allowed feature list. Assuming the lambda expression leads to the auto type expanding to std::function<...>, which is on the banned feature list. If so, I guess we could just define the lambdas directly where they are used instead of storing them in a temporary variable. https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate_unittest.cc:117: ASSERT_EQ(std::to_string(0), descriptors[0].device_id); Where does this expectation come from? I do not see the deivce_id being set up in the mock. https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate_unittest.cc:129: ASSERT_EQ(PIXEL_FORMAT_NV12, supported_formats[0].pixel_format); Where do these expected values come from? https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... File media/capture/video/chromeos/camera_hal_delegate_unittest.h (right): https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate_unittest.h:35: void OpenDevice(int32_t camera_id, OpenDeviceCallback callback) { should we use the "override" keyword here (and for the other overriden methods, including the destructor)? https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate_unittest.h:63: arc::mojom::CameraModulePtrInfo GetInterfacePtrInfo() { This could be simplified by just creating an instance and binding for this in the test setup code that wants to use this. Together with my other recommendation of using dependency injection to pass CameraHalDelegate its |camera_module_|, this might look something like this: class CameraHalDelegateTest : public ::testing::Test { public: CameraHalDelegateTest() : mock_camera_module_binding_(&mock_camera_module_), hal_delegate_thread_("HalDelegateThread") {} void SetUp() override { hal_delegate_thread_.Start(); CameraModulePtr camera_module; mock_camera_module_binding_.Bind(mojo::MakeRequest(&camera_module)); camera_hal_delegate_ = new CameraHalDelegate(hal_delegate_thread_.task_runner(), std::move(camera_module)); } protected: scoped_refptr<CameraHalDelegate> camera_hal_delegate_; testing::StrictMock<unittest_internal::MockCameraModule> mock_camera_module_; mojo::Binding<CameraModule> mock_camera_module_binding_; }; https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... File media/capture/video/chromeos/mojo/arc_camera3.mojom (right): https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... media/capture/video/chromeos/mojo/arc_camera3.mojom:260: // Camera3CallbackOps, see the comments about Camear3DeviceOps above. typo: Camera3DeviceOps https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... media/capture/video/chromeos/mojo/arc_camera3.mojom:288: OpenDevice@0(int32 camera_id) => (int32 result, Camera3DeviceOps? device_ops); When I worked on the video_capture service mojoms, I was told that the standard "Mojo" way of "returning" an instance of an interface is not to use an output parameter but instead pass-in a "request" along with the input parameters, see for example https://cs.chromium.org/chromium/src/services/video_capture/public/interfaces.... Assuming that is still the case, we may want to do this here as well. https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... media/capture/video/chromeos/mojo/arc_camera3.mojom:297: SetCallbacks@3(CameraModuleCallbacks callbacks) => (int32 result); Any reason why we wouldn't want to use an enum for result codes instead of an int32?
On 2017/05/26 04:33:33, jcliang wrote: > https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... > File media/capture/video/chromeos/video_capture_device_arc_chromeos.cc (right): > > https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... > media/capture/video/chromeos/video_capture_device_arc_chromeos.cc:17: // thread > to the media thread. > On 2017/05/25 17:13:07, chfremer wrote: > > On 2017/05/25 14:23:47, jcliang wrote: > > > On 2017/05/24 23:36:43, chfremer wrote: > > > > What is "the media thread"? > > > > > > I believe it's the media capture thread on which the VCD interface methods > are > > > called. > > > > > > The ScreenObserverDelegate class is copied from > > > linux/video_capture_device_chromeos.cc: > > > > > > > > > https://cs.chromium.org/chromium/src/media/capture/video/linux/video_capture_... > > > > Oh, I see. In that case, can we please avoid duplicating this code by moving > the > > class ScreenObserverDelegate to a pair of files of its own and reuse it in > both > > places? Instead of the concrete types for |capture_device|, use an new > interface > > DisplayRotationObserver and make both VideoCaptureDeviceArcChromeOS and > > VideoCaptureDeviceChromeOS implement it. > > > > IMHO, the reference to "media thread" is not a good idea, because it makes > > assumptions on the context the class is used in. It would be better to just > > describe what the class itself does independently of its context, which is > > "Registers itself as an observer at |display::Screen::GetScreen()| and > forwards > > rotation change events to the given DisplayRotationObserver on the thread > where > > ScreenObserverDelegate was created." > > Agreed. I've extracted the common codes to display_rotation_observer.h/.cc. > > I didn't extract the codes before because originally our plan was to completely > remove linux/video_capture_device_chromeos.h/.cc and migrate to > chromeos/video_capture_device_arc_chromeos.h/.cc, but now it looks like the two > VCDs have to co-exist for some time during the transition period. > > https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... > File media/capture/video/chromeos/video_capture_device_factory_chromeos.h > (right): > > https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/ch... > media/capture/video/chromeos/video_capture_device_factory_chromeos.h:35: // > instance is created. > On 2017/05/25 17:13:07, chfremer wrote: > > On 2017/05/25 14:23:48, jcliang wrote: > > > On 2017/05/24 23:36:43, chfremer wrote: > > > > Note, that with the move to the video capture Mojo service, > > > > VideoCaptureDeviceFactories are no longer necessarily created and operated > > in > > > > the browser process. This code should not make any assumptions about being > > run > > > > on the browser UI thread. > > > > > > We do not depend on the thread being run on the browser UI thread. We can > > remove > > > or update the comments when we move the code to video capture Mojo service. > > > > The thing is that the code does not need to be moved to be used in the video > > capture Mojo service. This code can (already today) be used in multiple > > contexts, two of which are the in-process video capture stack and the video > > capture Mojo service. A third one could be unit tests. > > > > In general, I recommend not specifying anything about the context where a > class > > gets used in as part of the class itself, because, even if correct today, this > > information very easily becomes inaccurate when the context changes and the > > comment is not updated. > > > > If there is any specific requirement on the thread that Init() must be called > > on, please call out that requirement is specifically, e.g. "must be called > from > > a thread on which it is legal to do XYZ". > > I see. Thanks for explaining. I'll remove comments that specifically refer to > any browser process threads. > > Are we using the video capture Mojo service in Chrome already? How can I force > the video capture device to run as a Mojo service? I'd like to verify running > the VCD in both the browser-process and the Mojo service contexts. It is not enabled by default, but we recently landed it behind a feature flag. You should be able to activate it by passing the following command-line argument to the Chrome startup: --enable_features=MojoVideoCapture. Please let me know if it works. There are integration tests running on the bots, but they use fake devices, and on ChromeOS I have not yet tried running it with a real camera.
https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... File media/capture/video/chromeos/camera_hal_delegate.cc (right): https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:44: module_task_runner_(module_task_runner), On 2017/05/26 17:40:57, chfremer wrote: > For passing scoped_refptr<>, I believe it is now recommended to pass by value > and then use std::move() if no copy is needed. Done. https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:113: } On 2017/05/26 17:40:57, chfremer wrote: > The contents of the above method look reasonable, but I am not familiar with the > APIs for establishing Mojo connections. I recommend asking rockot@ or yzshen@ to > help review this part. I believe this is the common flow to establish a legacy Mojo channel through unix domain socket to another process on Chrome OS or inside Android container. An example of merged codes in Chrome: https://cs.chromium.org/chromium/src/components/arc/arc_session.cc?q=arc_sess... https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... File media/capture/video/chromeos/camera_hal_delegate.h (right): https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.h:24: // function calls to CameraHalDelegate. On 2017/05/26 17:40:57, chfremer wrote: > As a reader, here, I would expect to find a description of what this class does, > which is missing here and is not obvious from the class name. The information > who is using the class is useful, but dangerous to include in the class-level > description, because it is easy to become inaccurate. IMHO, it is better to rely > on the code search tool to find usages. Done. https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.h:35: const scoped_refptr<base::SingleThreadTaskRunner> module_task_runner); On 2017/05/26 17:40:57, chfremer wrote: > Either here or in the class-level description it would be helpful to have an > explanation of what |module_task_runner| is needed for. Done. https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.h:39: // Called by VideoCaptureDeviceFactoryChromeOS::Init on the browser UI thread. On 2017/05/26 17:40:57, chfremer wrote: > I recommend removing the information about by who and on which thread this gets > called (it is already inaccurate). Done. https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.h:45: // on. On 2017/05/26 17:40:57, chfremer wrote: > This raises the following question (in me, as a reader): If the interface > exposed here is so similar to VideoCaptureDeviceFactory, what is actually the > purpose of VideoCaptureDeviceFactoryChromeOS? Why do we need it? What does it do > that is worth having it separate from this class? > > A good place for this information would be the class-level description. The reason that VideoCaptureDeviceFactoryChromeOS and CameraHalDelegate need to be separate is because the static function VideoCaptureDeviceFactory::CreateVideoCaptureDeviceFactory must return a raw pointer while we need CameraHalDelegate to be ref-counted. I also have a follow-up CL that will add a ArcCamera3Service Mojo proxy which will be started by VideoCaptureDeviceFactoryChromeOS: https://codereview.chromium.org/2878233002/. It's not a big change to VideoCaptureDeviceFactoryChromeOS though; the VideoCaptureDeviceFactoryChromeOS simply serves as an entry point to start the ArcCamera3Service Mojo proxy. The raw pointer v.s. ref-counted pointer is still the blocker for me to merge these two classes. https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.h:55: // be called on any thread. On 2017/05/26 17:40:57, chfremer wrote: > Hmm, the method signature does look asynchronous, though. Does the comment above > mean that this implementation guarantees that the callback is invoked > synchronously? If so, why not use a return value instead of a callback? Fixed the comment. I forgot to fix the comment after I changed the function from synchronous to asynchronous. https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.h:74: void StartForTesting(arc::mojom::CameraModulePtrInfo info); On 2017/05/26 17:40:57, chfremer wrote: > The need for a test-only API like this is often an indicator of a design issue. > In the case of this class, this is caused by the class wanting to do both > establishing the Mojo connection and using it, which are almost unrelated > responsibilities. I recommend resolving this by splitting out the responsibility > of establishing the Mojo connection to a factory class, something like: > > class ServiceCameraHalDelegateFactory { > public: > scoped_refptr<CameraHalDelegate> > CreateCameraHalDelegate(scoped_refptr<base::SingleThreadTaskRunner> > module_task_runner); > // ... > }; > > This class would contain the logic from StartCameraModuleIpc(). The class > CameraHalDelegate, would get its |camera_module_| via dependency injection from > the constructor, i.e.: > > class CameraHalDelegate { > public: > CameraHalDelegate( > const scoped_refptr<base::SingleThreadTaskRunner> module_task_runner, > arc::mojom::CameraModulePtr camera_module); > }; > > Unit tests can then instantiate CameraHalDelegate through its public constructor > using a mock for |camera_module|. This test-only API is only to make this CL self-contained to be able to run the unit tests. The test-only API is transient and will be removed with the CL that adds the ArcCamera3Service Mojo proxy CL: https://codereview.chromium.org/2878233002/. The ArcCamera3Service Mojo proxy acts like a factory class like you suggested above. I separate the ArcCamera3Service out to another CL to make this CL easier to review. https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... File media/capture/video/chromeos/camera_hal_delegate_unittest.cc (right): https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate_unittest.cc:42: } On 2017/05/26 17:40:57, chfremer wrote: > Same as in the production code, this while loop looks suspicious and can > probably be replaced with a more official way of other reference holders > signaling us when they have finished whatever we need to wait for here. We can remove the while loop here by explicitly calling the camera_hal_delegate_.Reset() to queue the task to reset all Mojo interface/binding on the task runner, and then count on hal_delegate_thread_.Stop() to wait until the task is done. The scenario in the production code in VideoCaptureDeviceArcChromeOS is more complex though, as it involves a Close() Mojo IPC call which in theory could block indefinitely (say, the camera HAL has a bug). https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate_unittest.cc:47: void Wait() { On 2017/05/26 17:40:57, chfremer wrote: > Couldn't find where this is used. Can it be removed? Done. https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate_unittest.cc:57: std::unique_ptr<base::MessageLoop> message_loop_; On 2017/05/26 17:40:58, chfremer wrote: > IIUC, it is now encouraged to use a ScopedTaskEnvironment instead, see > https://docs.google.com/document/d/1QabRo8c7D9LsYY3cEcaPQbOCLo8Tu-6VLykYXyl3P... Thanks for the pointer. I'm removing this for now. I'll use ScopedTaskEnvironment when I need to use Wait() for other test cases later. https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate_unittest.cc:67: }; On 2017/05/26 17:40:58, chfremer wrote: > I would be okay with this, but not sure if this may violate the Chromium C++11 > allowed feature list. Assuming the lambda expression leads to the auto type > expanding to std::function<...>, which is on the banned feature list. > > If so, I guess we could just define the lambdas directly where they are used > instead of storing them in a temporary variable. I see that binding captureless lambda is documented in https://cs.chromium.org/chromium/src/docs/callback.md?type=cs&l=97. I guess this means it is allowed. https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate_unittest.cc:117: ASSERT_EQ(std::to_string(0), descriptors[0].device_id); On 2017/05/26 17:40:58, chfremer wrote: > Where does this expectation come from? I do not see the deivce_id being set up > in the mock. The device_id is filled in camera_hal_delegate_->GetDeviceDescriptors(); it is simply the camera_id as a string. https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate_unittest.cc:129: ASSERT_EQ(PIXEL_FORMAT_NV12, supported_formats[0].pixel_format); On 2017/05/26 17:40:58, chfremer wrote: > Where do these expected values come from? This was from a workaround I had in CameraHalDelegate::GetSupportedFormats. I've updated the unit test to use fake metadata instead. https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... File media/capture/video/chromeos/camera_hal_delegate_unittest.h (right): https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate_unittest.h:35: void OpenDevice(int32_t camera_id, OpenDeviceCallback callback) { On 2017/05/26 17:40:58, chfremer wrote: > should we use the "override" keyword here (and for the other overriden methods, > including the destructor)? Done. https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate_unittest.h:63: arc::mojom::CameraModulePtrInfo GetInterfacePtrInfo() { On 2017/05/26 17:40:58, chfremer wrote: > This could be simplified by just creating an instance and binding for this in > the test setup code that wants to use this. > Together with my other recommendation of using dependency injection to pass > CameraHalDelegate its |camera_module_|, this might look something like this: > > class CameraHalDelegateTest : public ::testing::Test { > public: > CameraHalDelegateTest() > : mock_camera_module_binding_(&mock_camera_module_), > hal_delegate_thread_("HalDelegateThread") {} > > void SetUp() override { > hal_delegate_thread_.Start(); > CameraModulePtr camera_module; > mock_camera_module_binding_.Bind(mojo::MakeRequest(&camera_module)); > camera_hal_delegate_ = > new CameraHalDelegate(hal_delegate_thread_.task_runner(), > std::move(camera_module)); > } > > protected: > scoped_refptr<CameraHalDelegate> camera_hal_delegate_; > testing::StrictMock<unittest_internal::MockCameraModule> mock_camera_module_; > mojo::Binding<CameraModule> mock_camera_module_binding_; > }; Would you mind if I incorporate your suggestions when I change the unit tests in a new patch set of https://codereview.chromium.org/2878233002/? The mechanism to establish the Mojo connection in this CL is temporary. In the end we are going to use the ArcCamera3Service Mojo proxy in https://codereview.chromium.org/2878233002/. https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... File media/capture/video/chromeos/mojo/arc_camera3.mojom (right): https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... media/capture/video/chromeos/mojo/arc_camera3.mojom:260: // Camera3CallbackOps, see the comments about Camear3DeviceOps above. On 2017/05/26 17:40:58, chfremer wrote: > typo: Camera3DeviceOps Done. https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... media/capture/video/chromeos/mojo/arc_camera3.mojom:288: OpenDevice@0(int32 camera_id) => (int32 result, Camera3DeviceOps? device_ops); On 2017/05/26 17:40:58, chfremer wrote: > When I worked on the video_capture service mojoms, I was told that the standard > "Mojo" way of "returning" an instance of an interface is not to use an output > parameter but instead pass-in a "request" along with the input parameters, see > for example > https://cs.chromium.org/chromium/src/services/video_capture/public/interfaces.... > > Assuming that is still the case, we may want to do this here as well. Done. https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... media/capture/video/chromeos/mojo/arc_camera3.mojom:297: SetCallbacks@3(CameraModuleCallbacks callbacks) => (int32 result); On 2017/05/26 17:40:58, chfremer wrote: > Any reason why we wouldn't want to use an enum for result codes instead of an > int32? The returned result codes are either 0 or the standard errno defined in errno.h. Is it a convention in Chrome to explicitly define the standard errno in mojom?
Some replies and questions https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... File media/capture/video/chromeos/camera_hal_delegate.h (right): https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.h:45: // on. On 2017/05/31 14:58:06, jcliang wrote: > On 2017/05/26 17:40:57, chfremer wrote: > > This raises the following question (in me, as a reader): If the interface > > exposed here is so similar to VideoCaptureDeviceFactory, what is actually the > > purpose of VideoCaptureDeviceFactoryChromeOS? Why do we need it? What does it > do > > that is worth having it separate from this class? > > > > A good place for this information would be the class-level description. > > The reason that VideoCaptureDeviceFactoryChromeOS and CameraHalDelegate need to > be separate is because the static function > VideoCaptureDeviceFactory::CreateVideoCaptureDeviceFactory must return a raw > pointer while we need CameraHalDelegate to be ref-counted. > > I also have a follow-up CL that will add a ArcCamera3Service Mojo proxy which > will be started by VideoCaptureDeviceFactoryChromeOS: > https://codereview.chromium.org/2878233002/. It's not a big change to > VideoCaptureDeviceFactoryChromeOS though; the VideoCaptureDeviceFactoryChromeOS > simply serves as an entry point to start the ArcCamera3Service Mojo proxy. The > raw pointer v.s. ref-counted pointer is still the blocker for me to merge these > two classes. I see and agree that this is reasonable. https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.h:74: void StartForTesting(arc::mojom::CameraModulePtrInfo info); On 2017/05/31 14:58:06, jcliang wrote: > On 2017/05/26 17:40:57, chfremer wrote: > > The need for a test-only API like this is often an indicator of a design > issue. > > In the case of this class, this is caused by the class wanting to do both > > establishing the Mojo connection and using it, which are almost unrelated > > responsibilities. I recommend resolving this by splitting out the > responsibility > > of establishing the Mojo connection to a factory class, something like: > > > > class ServiceCameraHalDelegateFactory { > > public: > > scoped_refptr<CameraHalDelegate> > > CreateCameraHalDelegate(scoped_refptr<base::SingleThreadTaskRunner> > > module_task_runner); > > // ... > > }; > > > > This class would contain the logic from StartCameraModuleIpc(). The class > > CameraHalDelegate, would get its |camera_module_| via dependency injection > from > > the constructor, i.e.: > > > > class CameraHalDelegate { > > public: > > CameraHalDelegate( > > const scoped_refptr<base::SingleThreadTaskRunner> module_task_runner, > > arc::mojom::CameraModulePtr camera_module); > > }; > > > > Unit tests can then instantiate CameraHalDelegate through its public > constructor > > using a mock for |camera_module|. > > This test-only API is only to make this CL self-contained to be able to run the > unit tests. The test-only API is transient and will be removed with the CL that > adds the ArcCamera3Service Mojo proxy CL: > https://codereview.chromium.org/2878233002/. The ArcCamera3Service Mojo proxy > acts like a factory class like you suggested above. I separate the > ArcCamera3Service out to another CL to make this CL easier to review. I don't see any test for CameraHalDelegate in https://codereview.chromium.org/2878233002/. It does not currently seem to be a follow-up based on the code of this CL. If you have a newer version, could you please update it so I can see the diff from this CL? If your follow-up work already removes the need for this testing-only API, then I don't want to ask for any unnecessary extra work to remove them from this CL. Otherwise, I don't see why the cleaner solution using dependency injection would not lead to a self-contained CL. https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... File media/capture/video/chromeos/camera_hal_delegate_unittest.cc (right): https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate_unittest.cc:117: ASSERT_EQ(std::to_string(0), descriptors[0].device_id); On 2017/05/31 14:58:07, jcliang wrote: > On 2017/05/26 17:40:58, chfremer wrote: > > Where does this expectation come from? I do not see the deivce_id being set up > > in the mock. > > The device_id is filled in camera_hal_delegate_->GetDeviceDescriptors(); it is > simply the camera_id as a string. I see. So this basically just verifies that CameraHalDelegate uses a string representation of the device index as |device_id|. https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... File media/capture/video/chromeos/camera_hal_delegate_unittest.h (right): https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate_unittest.h:63: arc::mojom::CameraModulePtrInfo GetInterfacePtrInfo() { On 2017/05/31 14:58:07, jcliang wrote: > On 2017/05/26 17:40:58, chfremer wrote: > > This could be simplified by just creating an instance and binding for this in > > the test setup code that wants to use this. > > Together with my other recommendation of using dependency injection to pass > > CameraHalDelegate its |camera_module_|, this might look something like this: > > > > class CameraHalDelegateTest : public ::testing::Test { > > public: > > CameraHalDelegateTest() > > : mock_camera_module_binding_(&mock_camera_module_), > > hal_delegate_thread_("HalDelegateThread") {} > > > > void SetUp() override { > > hal_delegate_thread_.Start(); > > CameraModulePtr camera_module; > > mock_camera_module_binding_.Bind(mojo::MakeRequest(&camera_module)); > > camera_hal_delegate_ = > > new CameraHalDelegate(hal_delegate_thread_.task_runner(), > > std::move(camera_module)); > > } > > > > protected: > > scoped_refptr<CameraHalDelegate> camera_hal_delegate_; > > testing::StrictMock<unittest_internal::MockCameraModule> > mock_camera_module_; > > mojo::Binding<CameraModule> mock_camera_module_binding_; > > }; > > Would you mind if I incorporate your suggestions when I change the unit tests in > a new patch set of https://codereview.chromium.org/2878233002/ The mechanism to > establish the Mojo connection in this CL is temporary. In the end we are going > to use the ArcCamera3Service Mojo proxy in > https://codereview.chromium.org/2878233002/. I wouldn't mind. No need to put extra work for changing this into this CL if it goes away anyway in your follow-up. https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... File media/capture/video/chromeos/mojo/arc_camera3.mojom (right): https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... media/capture/video/chromeos/mojo/arc_camera3.mojom:297: SetCallbacks@3(CameraModuleCallbacks callbacks) => (int32 result); On 2017/05/31 14:58:07, jcliang wrote: > On 2017/05/26 17:40:58, chfremer wrote: > > Any reason why we wouldn't want to use an enum for result codes instead of an > > int32? > > The returned result codes are either 0 or the standard errno defined in errno.h. > Is it a convention in Chrome to explicitly define the standard errno in mojom? I assume you are referring to errno.h from the C standard library? I don't know about any conventions, but as a reader (and someone who does not use C much), I had no clue or expectation that error codes could be assumed to be related to some C standard. If you prefer to use them, please document it somewhere in the description. Otherwise, I feel an explicit enum would be more clear. https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/ch... File media/capture/video/chromeos/camera_hal_delegate.h (right): https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.h:35: // All the Mojo IPC operations happen on |module_task_runner|. Both here and in the class-level description I don't really understand what "module" refers to. As a result, I don't have a clear understanding of why |module_task_runnner| is a suitable name. If it is used exclusively for IPC operations, would |ipc_task_runner| also be a suitable name?
A couple more comments/questions/requests, while I'm about to dive into CameraDeviceDelegate. https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... File media/capture/video/chromeos/camera_hal_delegate_unittest.cc (right): https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate_unittest.cc:42: } On 2017/05/31 14:58:06, jcliang wrote: > On 2017/05/26 17:40:57, chfremer wrote: > > Same as in the production code, this while loop looks suspicious and can > > probably be replaced with a more official way of other reference holders > > signaling us when they have finished whatever we need to wait for here. > > We can remove the while loop here by explicitly calling the > camera_hal_delegate_.Reset() to queue the task to reset all Mojo > interface/binding on the task runner, and then count on > hal_delegate_thread_.Stop() to wait until the task is done. It appears to me that a lot of the code in this CL is designed with synchronous shutdown methods (or destructors) that block until shutdown is complete. This TearDown() method is one example. An alternative design approach would be to make the shutdown methods asynchronous and non-blocking, and use callbacks to notify the caller when the shutdown is complete. I guess both approaches can work, and I leave it up to you to decide what is most suited for this component. In general I would usually go for the asynchronous approach, because it gives the caller the flexibility to decide if and when to block. > The scenario in the production code in VideoCaptureDeviceArcChromeOS is more > complex though, as it involves a Close() Mojo IPC call which in theory could > block indefinitely (say, the camera HAL has a bug). https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/ch... File media/capture/video/chromeos/camera_device_delegate.h (right): https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.h:228: This class is quite big and has a large number of private methods. This is typically an indicator that this class has too many responsibilities, some of which should be extracted into classes of their own that, in-turn, can then be tested, changed, and digested by readers in isolation, which should make the life of future engineers reading and touching this code much easier. https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/ch... File media/capture/video/chromeos/camera_device_delegate_unittest.cc (right): https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate_unittest.cc:183: arc::mojom::Camera3DeviceOpsPtrInfo GetInterfacePtrInfo() { This is the pattern as in MockCameraModule, which I commented on. Is this also going to change in the follow-up CL? https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate_unittest.cc:221: message_loop_(new base::MessageLoop), Please use ScopedTaskEnvironment instead https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate_unittest.cc:259: void Wait() { needed? https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate_unittest.cc:278: TEST_F(CameraDeviceDelegateTest, AllocateAndStop) { Please add more test cases to get coverage for the rest of the functionality implemented by CameraDeviceDelegate. https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/ch... File media/capture/video/chromeos/camera_hal_delegate.cc (right): https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:110: base::Unretained(this))); Please add a comment explaining why the use of Unretained is safe here. https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/ch... File media/capture/video/chromeos/camera_hal_delegate_unittest.cc (right): https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate_unittest.cc:60: TEST_F(CameraHalDelegateTest, GetBuiltinCameraInfo) { This test case provides coverage for GetDeviceDescriptors() and GetSupportedFormats(). What about the rest of the public API of CameraHalDelegate? https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/ch... File media/capture/video/chromeos/camera_hal_delegate_unittest.h (right): https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate_unittest.h:6: #define MEDIA_CAPTURE_VIDEO_CHROMEOS_CAMERA_HAL_DELEGATE_UNITTEST_H_ This files only contains a mock for interface arc::mojom::CameraModule. We should name it accordingly, e.g. mock_camera_module.h.
https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... File media/capture/video/chromeos/camera_hal_delegate.h (right): https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.h:74: void StartForTesting(arc::mojom::CameraModulePtrInfo info); On 2017/05/31 18:30:41, chfremer wrote: > On 2017/05/31 14:58:06, jcliang wrote: > > On 2017/05/26 17:40:57, chfremer wrote: > > > The need for a test-only API like this is often an indicator of a design > > issue. > > > In the case of this class, this is caused by the class wanting to do both > > > establishing the Mojo connection and using it, which are almost unrelated > > > responsibilities. I recommend resolving this by splitting out the > > responsibility > > > of establishing the Mojo connection to a factory class, something like: > > > > > > class ServiceCameraHalDelegateFactory { > > > public: > > > scoped_refptr<CameraHalDelegate> > > > CreateCameraHalDelegate(scoped_refptr<base::SingleThreadTaskRunner> > > > module_task_runner); > > > // ... > > > }; > > > > > > This class would contain the logic from StartCameraModuleIpc(). The class > > > CameraHalDelegate, would get its |camera_module_| via dependency injection > > from > > > the constructor, i.e.: > > > > > > class CameraHalDelegate { > > > public: > > > CameraHalDelegate( > > > const scoped_refptr<base::SingleThreadTaskRunner> module_task_runner, > > > arc::mojom::CameraModulePtr camera_module); > > > }; > > > > > > Unit tests can then instantiate CameraHalDelegate through its public > > constructor > > > using a mock for |camera_module|. > > > > This test-only API is only to make this CL self-contained to be able to run > the > > unit tests. The test-only API is transient and will be removed with the CL > that > > adds the ArcCamera3Service Mojo proxy CL: > > https://codereview.chromium.org/2878233002/. The ArcCamera3Service Mojo proxy > > acts like a factory class like you suggested above. I separate the > > ArcCamera3Service out to another CL to make this CL easier to review. > > I don't see any test for CameraHalDelegate in > https://codereview.chromium.org/2878233002/. It does not currently seem to be a > follow-up based on the code of this CL. If you have a newer version, could you > please update it so I can see the diff from this CL? > > If your follow-up work already removes the need for this testing-only API, then > I don't want to ask for any unnecessary extra work to remove them from this CL. > Otherwise, I don't see why the cleaner solution using dependency injection would > not lead to a self-contained CL. Sorry, by self-contained I mean I need to make this CL work with the current camera HAL process on Chrome OS, hence I need the StartCameraModuleIpc() method above to establish the Mojo connection. The follow-up CL will change the Mojo process model to register-observer mode, which will remove StartCameraModuleIpc() and add another SetCameraModule() method (https://codereview.chromium.org/2878233002/patch/1/10005). I will be using the SetCameraModule() method to inject MockCameraModule, which is similar to the dependency-injection approach you suggested. I haven't started on the new version of unit tests in the follow-up CL as it largely depend on this CL. I need to rebase every time I make any changes in this CL. I'm waiting for this CL to stabilize first. https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... File media/capture/video/chromeos/mojo/arc_camera3.mojom (right): https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... media/capture/video/chromeos/mojo/arc_camera3.mojom:297: SetCallbacks@3(CameraModuleCallbacks callbacks) => (int32 result); On 2017/05/31 18:30:41, chfremer wrote: > On 2017/05/31 14:58:07, jcliang wrote: > > On 2017/05/26 17:40:58, chfremer wrote: > > > Any reason why we wouldn't want to use an enum for result codes instead of > an > > > int32? > > > > The returned result codes are either 0 or the standard errno defined in > errno.h. > > Is it a convention in Chrome to explicitly define the standard errno in mojom? > > I assume you are referring to errno.h from the C standard library? > I don't know about any conventions, but as a reader (and someone who does not > use C much), I had no clue or expectation that error codes could be assumed to > be related to some C standard. If you prefer to use them, please document it > somewhere in the description. Otherwise, I feel an explicit enum would be more > clear. Yes I mean the errno.h from C standard library. I can change the return values to enum but I'd like to do that in another CL. These Mojo interfaces are in active use between Chrome OS and Android container now. I'll need to collect all the error numbers we use in every function. https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/ch... File media/capture/video/chromeos/camera_device_delegate.h (right): https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.h:228: On 2017/06/01 00:16:27, chfremer wrote: > This class is quite big and has a large number of private methods. This is > typically an indicator that this class has too many responsibilities, some of > which should be extracted into classes of their own that, in-turn, can then be > tested, changed, and digested by readers in isolation, which should make the > life of future engineers reading and touching this code much easier. I've split the buffer allocation and circulation into a StreamBufferManager class. The CameraDeviceDelegate now opens and initializes the camera device, while the StreamBufferManager allocates the buffers and sends / receives capture requests / results. This header file is still large, but the implementations are split into two .cc files. https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/ch... File media/capture/video/chromeos/camera_device_delegate_unittest.cc (right): https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate_unittest.cc:183: arc::mojom::Camera3DeviceOpsPtrInfo GetInterfacePtrInfo() { On 2017/06/01 00:16:27, chfremer wrote: > This is the pattern as in MockCameraModule, which I commented on. Is this also > going to change in the follow-up CL? This I can fix in this CL since the follow-up CL does not change CameraDeviceDelegate. https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate_unittest.cc:221: message_loop_(new base::MessageLoop), On 2017/06/01 00:16:27, chfremer wrote: > Please use ScopedTaskEnvironment instead Done. https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate_unittest.cc:259: void Wait() { On 2017/06/01 00:16:27, chfremer wrote: > needed? This is needed after I extend the test. https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate_unittest.cc:278: TEST_F(CameraDeviceDelegateTest, AllocateAndStop) { On 2017/06/01 00:16:27, chfremer wrote: > Please add more test cases to get coverage for the rest of the functionality > implemented by CameraDeviceDelegate. I've extended the test case to allocate a capture device, initialize and set up the stream, capture one frame, and finally stop and deallocate the device. This should have basic coverage of the entire capture flow. I also plan to add some more test cases to cover error handling. https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/ch... File media/capture/video/chromeos/camera_hal_delegate.cc (right): https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:110: base::Unretained(this))); On 2017/06/01 00:16:27, chfremer wrote: > Please add a comment explaining why the use of Unretained is safe here. This does not need to be Unretained now since it's moved out of the destructor. I'll change it to refcounted. https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/ch... File media/capture/video/chromeos/camera_hal_delegate.h (right): https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.h:35: // All the Mojo IPC operations happen on |module_task_runner|. On 2017/05/31 18:30:41, chfremer wrote: > Both here and in the class-level description I don't really understand what > "module" refers to. > As a result, I don't have a clear understanding of why |module_task_runnner| is > a suitable name. If it is used exclusively for IPC operations, would > |ipc_task_runner| also be a suitable name? Agreed. I'll change this to ipc_task_runner. https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/ch... File media/capture/video/chromeos/camera_hal_delegate_unittest.cc (right): https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate_unittest.cc:60: TEST_F(CameraHalDelegateTest, GetBuiltinCameraInfo) { On 2017/06/01 00:16:27, chfremer wrote: > This test case provides coverage for GetDeviceDescriptors() and > GetSupportedFormats(). What about the rest of the public API of > CameraHalDelegate? OpenDevice is covered by the tests in video_capture_device_unittests. GetCameraInfo and OpenDevice are thin wrappers of the corresponding Mojo calls so there aren't much to test. StartCameraModuleIpc will be removed in the follow-up CL so I didn't add a test here. I plan to add some test cases to test error handling. https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/ch... File media/capture/video/chromeos/camera_hal_delegate_unittest.h (right): https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate_unittest.h:6: #define MEDIA_CAPTURE_VIDEO_CHROMEOS_CAMERA_HAL_DELEGATE_UNITTEST_H_ On 2017/06/01 00:16:27, chfremer wrote: > This files only contains a mock for interface arc::mojom::CameraModule. > We should name it accordingly, e.g. mock_camera_module.h. Done.
Overall, things look sound. The main thing I feel we need to do before we can land this is to increase the test coverage. Especially the class CameraDeviceDelegate contains a ton of complexity and edge cases that call for thorough testing. To facilitate testing, I recommend extracting the large state machine that is currently inside the private scope of CameraDeviceDelegate into a new class that allows setting up and testing all the interesting cases more directly than having to go through the public API of CameraDeviceDelegate. https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/ch... File media/capture/video/chromeos/camera_device_delegate.h (right): https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.h:36: enum State { I don't see any public API of this class that involves the State enum, so why does State need to be public? https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.h:57: // to process capture requests. For a reader unfamiliar with the larger context (including myself), this raises the following questions: * What is a capture stream? * Can there be more than one capture stream? * When I use an instance of this class, do I need to configure these streams through its public API or is that something that happens internally? If knowing the answers to all these is relevant to the clients of the public API of this class, ideally the class-level description would include a high-level overview of these concepts. https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.h:125: // Sets the state. This comment is redundant. I recommend only using comments where the method signature is not sufficient to convey what it does.
yzshen@chromium.org changed reviewers: + rockot@chromium.org, yzshen@chromium.org
https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... File media/capture/video/chromeos/camera_hal_delegate.cc (right): https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:113: } On 2017/05/26 17:40:57, chfremer wrote: > The contents of the above method look reasonable, but I am not familiar with the > APIs for establishing Mojo connections. I recommend asking rockot@ or yzshen@ to > help review this part. +rockot who knows this the best.
lgtm
Uploading a new patch set first to address chfremer's comments and add some more test cases in camera_device_delegate_unittest.cc. I'll need some more cycles to add unit tests for stream_buffer_manager.cc. https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/ch... File media/capture/video/chromeos/camera_device_delegate.h (right): https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.h:36: enum State { On 2017/06/01 17:16:48, chfremer wrote: > I don't see any public API of this class that involves the State enum, so why > does State need to be public? With the new test cases I added in camera_device_delegate_unittest.cc I now need to access CameraDeviceDelegate::State in the unit test. https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.h:57: // to process capture requests. On 2017/06/01 17:16:48, chfremer wrote: > For a reader unfamiliar with the larger context (including myself), this raises > the following questions: > * What is a capture stream? > * Can there be more than one capture stream? > * When I use an instance of this class, do I need to configure these streams > through its public API or is that something that happens internally? > > If knowing the answers to all these is relevant to the clients of the public API > of this class, ideally the class-level description would include a high-level > overview of these concepts. A capture stream is a set of parameters (width, height, buffer format, etc..) which define a input to / output from the camera HAL. There can be multiple streams. In the Chrome use case here we only use one preview stream. The stream configuration happens internally through some HAL-specific API calls. The users of the public APIs (i.e. the VideoCaptureDevice interface) do not need to worry about it. I've moved the State enum to private to make it clear that the states and stream configurations are internal to the implementation. https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.h:125: // Sets the state. On 2017/06/01 17:16:49, chfremer wrote: > This comment is redundant. I recommend only using comments where the method > signature is not sufficient to convey what it does. Done.
https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/ch... File media/capture/video/chromeos/camera_device_delegate.h (right): https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.h:57: // to process capture requests. On 2017/06/06 16:04:29, jcliang wrote: > On 2017/06/01 17:16:48, chfremer wrote: > > For a reader unfamiliar with the larger context (including myself), this > raises > > the following questions: > > * What is a capture stream? > > * Can there be more than one capture stream? > > * When I use an instance of this class, do I need to configure these streams > > through its public API or is that something that happens internally? > > > > If knowing the answers to all these is relevant to the clients of the public > API > > of this class, ideally the class-level description would include a high-level > > overview of these concepts. > > A capture stream is a set of parameters (width, height, buffer format, etc..) > which define a input to / output from the camera HAL. There can be multiple > streams. In the Chrome use case here we only use one preview stream. The stream > configuration happens internally through some HAL-specific API calls. The users > of the public APIs (i.e. the VideoCaptureDevice interface) do not need to worry > about it. > > I've moved the State enum to private to make it clear that the states and stream > configurations are internal to the implementation. Oops, I should have deleted the last paragraph. The State enum is still in the public section because I need to access it in the unit tests.
A few more suggestions. As a next step, I am planning to read through CameraDeviceDelegate and StreamBufferManager and make notes if I find responsibilities that could be extracted (leading to simpler tests and easier to digest and maintain code) or code paths that I feel deserve testing but are not yet covered by the tests. I am hope this will be helpful. https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/ch... File media/capture/video/chromeos/camera_device_delegate.h (right): https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.h:57: // to process capture requests. On 2017/06/06 16:09:23, jcliang wrote: > On 2017/06/06 16:04:29, jcliang wrote: > > On 2017/06/01 17:16:48, chfremer wrote: > > > For a reader unfamiliar with the larger context (including myself), this > > raises > > > the following questions: > > > * What is a capture stream? > > > * Can there be more than one capture stream? > > > * When I use an instance of this class, do I need to configure these streams > > > through its public API or is that something that happens internally? > > > > > > If knowing the answers to all these is relevant to the clients of the public > > API > > > of this class, ideally the class-level description would include a > high-level > > > overview of these concepts. > > > > A capture stream is a set of parameters (width, height, buffer format, etc..) > > which define a input to / output from the camera HAL. There can be multiple > > streams. In the Chrome use case here we only use one preview stream. The > stream > > configuration happens internally through some HAL-specific API calls. The > users > > of the public APIs (i.e. the VideoCaptureDevice interface) do not need to > worry > > about it. > > > > I've moved the State enum to private to make it clear that the states and > stream > > configurations are internal to the implementation. > > Oops, I should have deleted the last paragraph. The State enum is still in the > public section because I need to access it in the unit tests. If I understand correctly, the situation is as follows. The enum State is not relevant for readers or clients of the public API of CameraDeviceDelegate. But it is relevant for unit tests of the implementation details. IMHO, the clean approach for this situation is to move the implementation details out to a class of its own. This way, the things that are private or irrelevant to clients of CameraDeviceDelegate do not have to appear here, but can still be public to clients to who they are relevant, e.g. the unit tests that verify their behavior or readers who are interested in the implementation details at that level. https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.h:228: On 2017/06/01 17:11:17, jcliang wrote: > On 2017/06/01 00:16:27, chfremer wrote: > > This class is quite big and has a large number of private methods. This is > > typically an indicator that this class has too many responsibilities, some of > > which should be extracted into classes of their own that, in-turn, can then be > > tested, changed, and digested by readers in isolation, which should make the > > life of future engineers reading and touching this code much easier. > > I've split the buffer allocation and circulation into a StreamBufferManager > class. The CameraDeviceDelegate now opens and initializes the camera device, > while the StreamBufferManager allocates the buffers and sends / receives capture > requests / results. > > This header file is still large, but the implementations are split into two .cc > files. Thanks. That is definitely an improvement. From my previous read through, I believe there may be a couple more responsibilities that could be extracted into classes of their own. If I see them, I will try to point them out in my next read through. https://codereview.chromium.org/2837273004/diff/400001/media/capture/video/ch... File media/capture/video/chromeos/camera_device_delegate.h (right): https://codereview.chromium.org/2837273004/diff/400001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.h:105: class CAPTURE_EXPORT StreamBufferManager final I recommend moving this class to its own files. I don't see any advantage in nesting it here, and I think that it would make things easier to read and digest if they were separate. https://codereview.chromium.org/2837273004/diff/400001/media/capture/video/ch... File media/capture/video/chromeos/camera_device_delegate_unittest.cc (right): https://codereview.chromium.org/2837273004/diff/400001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate_unittest.cc:462: TEST_F(CameraDeviceDelegateTest, StopAfterInitialized) { For readers of the could, I found it helpful to add a brief description comment above each test method that starts with "Tests that ...", unless the test is small enough that the name of the test method already conveys all that is needed to understand what the test verifies. https://codereview.chromium.org/2837273004/diff/400001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate_unittest.cc:475: auto stop_on_configure_stream = Separating the lambda into a temporary variable like this forces us to assign a name to it, in this case "stop_on_configure_stream". If the lambda was placed directly into the Invoke() part at l.487, I feel it would be easier to understand, because the context "EXPECT_CALL(mock_camera_device_, DoConfigureStreams(_, _))" does a better job of making it clear when this is going to happen than the (redundant) temporary variable name "stop_on_configure_stream".
https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/ch... File media/capture/video/chromeos/camera_device_delegate.h (right): https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.h:57: // to process capture requests. On 2017/06/06 17:09:25, chfremer wrote: > On 2017/06/06 16:09:23, jcliang wrote: > > On 2017/06/06 16:04:29, jcliang wrote: > > > On 2017/06/01 17:16:48, chfremer wrote: > > > > For a reader unfamiliar with the larger context (including myself), this > > > raises > > > > the following questions: > > > > * What is a capture stream? > > > > * Can there be more than one capture stream? > > > > * When I use an instance of this class, do I need to configure these > streams > > > > through its public API or is that something that happens internally? > > > > > > > > If knowing the answers to all these is relevant to the clients of the > public > > > API > > > > of this class, ideally the class-level description would include a > > high-level > > > > overview of these concepts. > > > > > > A capture stream is a set of parameters (width, height, buffer format, > etc..) > > > which define a input to / output from the camera HAL. There can be multiple > > > streams. In the Chrome use case here we only use one preview stream. The > > stream > > > configuration happens internally through some HAL-specific API calls. The > > users > > > of the public APIs (i.e. the VideoCaptureDevice interface) do not need to > > worry > > > about it. > > > > > > I've moved the State enum to private to make it clear that the states and > > stream > > > configurations are internal to the implementation. > > > > Oops, I should have deleted the last paragraph. The State enum is still in the > > public section because I need to access it in the unit tests. > > If I understand correctly, the situation is as follows. The enum State is not > relevant for readers or clients of the public API of CameraDeviceDelegate. But > it is relevant for unit tests of the implementation details. IMHO, the clean > approach for this situation is to move the implementation details out to a class > of its own. This way, the things that are private or irrelevant to clients of > CameraDeviceDelegate do not have to appear here, but can still be public to > clients to who they are relevant, e.g. the unit tests that verify their behavior > or readers who are interested in the implementation details at that level. This is a great suggestion! I've extracted the state and some other member variable into a CameraDeviceContext class. https://codereview.chromium.org/2837273004/diff/400001/media/capture/video/ch... File media/capture/video/chromeos/camera_device_delegate.h (right): https://codereview.chromium.org/2837273004/diff/400001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.h:105: class CAPTURE_EXPORT StreamBufferManager final On 2017/06/06 17:09:25, chfremer wrote: > I recommend moving this class to its own files. I don't see any advantage in > nesting it here, and I think that it would make things easier to read and digest > if they were separate. Done. It was kept here to access some private methods of CameraDeviceDelegate, but I can extract it out after adding the CameraDeviceContext class. https://codereview.chromium.org/2837273004/diff/400001/media/capture/video/ch... File media/capture/video/chromeos/camera_device_delegate_unittest.cc (right): https://codereview.chromium.org/2837273004/diff/400001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate_unittest.cc:462: TEST_F(CameraDeviceDelegateTest, StopAfterInitialized) { On 2017/06/06 17:09:25, chfremer wrote: > For readers of the could, I found it helpful to add a brief description comment > above each test method that starts with "Tests that ...", unless the test is > small enough that the name of the test method already conveys all that is needed > to understand what the test verifies. Done. https://codereview.chromium.org/2837273004/diff/400001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate_unittest.cc:475: auto stop_on_configure_stream = On 2017/06/06 17:09:25, chfremer wrote: > Separating the lambda into a temporary variable like this forces us to assign a > name to it, in this case "stop_on_configure_stream". If the lambda was placed > directly into the Invoke() part at l.487, I feel it would be easier to > understand, because the context "EXPECT_CALL(mock_camera_device_, > DoConfigureStreams(_, _))" does a better job of making it clear when this is > going to happen than the (redundant) temporary variable name > "stop_on_configure_stream". Done.
Alright, so after finishing a read-through of StreamBufferManager and its tests, I think it makes sense to go ahead and land this, before the CL size gets even more out of hand. Overall, and especially with the break down into smaller pieces and added test cases, I think the code is now already much easier to digest than at the beginning and is in good shape. There are still things that could be improved further, but I think we can work on those in subsequent steps, after this CL has landed. Thanks for incorporating so many of the suggestions! LGTM https://codereview.chromium.org/2837273004/diff/420001/media/capture/video/ch... File media/capture/video/chromeos/camera_device_context.h (right): https://codereview.chromium.org/2837273004/diff/420001/media/capture/video/ch... media/capture/video/chromeos/camera_device_context.h:39: // Initialize() -> OnInitialized() As much as I appreciate the detailed documentation, the level of detail at this location makes it feel hard for me, as a reader, to see the bigger picture. At this point in my read through, I am not that much interested in where each state gets set. And if I were, I could just click on it in code search and find out. What I would be more interested in is what the typical state sequence looks like during normal operation, e.g. kStopped -> kStarting -> kInitialized -> ... . If we want to keep the detailed documentation, maybe we can move it to a separate location (e.g. class-level comment or a readme file), so that it does not overpower the actual code? https://codereview.chromium.org/2837273004/diff/420001/media/capture/video/ch... File media/capture/video/chromeos/camera_device_delegate.cc (right): https://codereview.chromium.org/2837273004/diff/420001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:31: DCHECK_EQ(device_context_->GetState(), CameraDeviceContext::State::kStopped); This check looks like something that could be checked in a unit test instead of here. https://codereview.chromium.org/2837273004/diff/420001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:34: int32_t camera_id = std::stoi(device_descriptor_.device_id); Can this fail? It is probably illegal to call CameraDeviceDelegate() with a descriptor whose device_id is not an integer number. How about we do the conversion to int32_t in the constructor and add a DCHECK to make sure it succeeded? https://codereview.chromium.org/2837273004/diff/420001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:179: int32_t camera_id = std::stoi(device_descriptor_.device_id); Can probably be removed here if we did this in the constructor. https://codereview.chromium.org/2837273004/diff/420001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:229: CameraDeviceContext::State::kStopping); Do we need to call OnClosed(0) here? Since the state and error checking code appears at least 3 times in almost identical form, can we factor it out and reuse it? https://codereview.chromium.org/2837273004/diff/420001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:232: if (result) { In the other methods, at this spot we are doing a LOG(ERROR). Why not here? Let's make it consistent. https://codereview.chromium.org/2837273004/diff/500001/media/capture/video/ch... File media/capture/video/chromeos/camera_device_delegate.cc (right): https://codereview.chromium.org/2837273004/diff/500001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:220: new StreamCaptureInterfaceImpl(this)), Would it be possible to have CameraDeviceDelegate directly implement StreamCaptureInterface and have StreamBufferManager's constructor accept a scoped_refptr<StreamCaptureInterface>? Unless there is a technical reason preventing this, it would allow us to get rid of StreamCaptureInterfaceImpl, and the invocation here would become new StreamBufferManager( std::move(callback_ops_request), this, device_context_.get(), ipc_task_runner_); https://codereview.chromium.org/2837273004/diff/500001/media/capture/video/ch... File media/capture/video/chromeos/stream_buffer_manager.cc (right): https://codereview.chromium.org/2837273004/diff/500001/media/capture/video/ch... media/capture/video/chromeos/stream_buffer_manager.cc:336: CaptureResult& partial_result = partial_results_[frame_number]; Idea for a possible improvement: If we wanted to further break down the complexity into smaller and easier digestable/testable/maintainable pieces, one potential strategy to do so might be to move the logic for accumulating these partial results, verifying their correctness and correct order into a separate class, e.g. something that could be called PartialResultsAccumulator. This would leave the class StreamBufferManager with the core flow of the capture loop as its single responsibility, and with the methods becoming shorter and free of other responsibilities, this flow would become nicely obvious. We could then even think about renaming the class to something like CaptureMainLoop. https://codereview.chromium.org/2837273004/diff/500001/media/capture/video/ch... media/capture/video/chromeos/stream_buffer_manager.cc:456: if (!sync_wait(fence.get().handle, kSyncWaitTimeoutMs)) { So, if I understand this correctly, for each frame, we are sharing a buffer with the HAL service. The service maps the buffer to its memory space, places data into the buffer and then unmaps the buffer again. Here we wait for the unmapping to complete. What is the resource cost / performance overhead of the repeated sharing/mapping/unmapping? The buffer sharing between video capture service, Browser, and Renderers works differently, and does so explicitly to avoid the cost of repeated mapping and unmapping. What is done there is to keep buffers shared (up to a given maximum number of them). When the frame data producer has placed data into a buffer, it does not unmap, but simply sends a message that says that data is available. When the consumer has finished consuming the data, it sends a message to the producer to let it know that the buffer is ready to be reused. Ideally, we would have a single reusable implementation for transporting frame data across process boundaries and would be applying it wherever needed (not just for video data). It seem we are currently going the way of having separate implementations for each case, non of which is reusable. Fixing this would probably be quite a bit of work, and I don't want this to block the progress on the ARC++ HALv3 work, but I think this should eventually be our goal. https://codereview.chromium.org/2837273004/diff/500001/media/capture/video/ch... File media/capture/video/chromeos/stream_buffer_manager.h (right): https://codereview.chromium.org/2837273004/diff/500001/media/capture/video/ch... media/capture/video/chromeos/stream_buffer_manager.h:60: friend class StreamBufferManagerTest; nit: I am not asking you to change this now, but as a general design recommendation, tests should not have to know anything about the private section of the thing they test. If the private section contains complexity that requires more direct testing than possible through the public API, this is a sign that this complexity should really be exposed publicly through a class of its own, which can then be tested. https://codereview.chromium.org/2837273004/diff/500001/media/capture/video/ch... File media/capture/video/chromeos/stream_buffer_manager_unittest.cc (right): https://codereview.chromium.org/2837273004/diff/500001/media/capture/video/ch... media/capture/video/chromeos/stream_buffer_manager_unittest.cc:78: device_context_.reset(new CameraDeviceContext(std::move(mock_client))); nit: Not sure what the style guide says here, but I ususally prefer to use base::MakeUnique<> over new wherever possible, e.g. auto mock_client = base::MakeUnique<unittest_internal::MockVideoCaptureClient>(); device_context_ = base::MakeUnique<CameraDeviceContext>(std::move(mock_client)); Or we could even get rid of the temp var and do device_context_ = base::MakeUnique<CameraDeviceContext>(base::MakeUnique<unittest_internal::MockVideoCaptureClient>()); https://codereview.chromium.org/2837273004/diff/500001/media/capture/video/ch... media/capture/video/chromeos/stream_buffer_manager_unittest.cc:83: new MockStreamCaptureInterface()), base::MakeUnique<> https://codereview.chromium.org/2837273004/diff/500001/media/capture/video/ch... media/capture/video/chromeos/stream_buffer_manager_unittest.cc:90: } I still don't think that this is a clean solution. Do we have any plans on the roadmap to change this mechanism or is this the proposed target solution? https://codereview.chromium.org/2837273004/diff/500001/media/capture/video/ch... media/capture/video/chromeos/stream_buffer_manager_unittest.cc:316: arc::mojom::Camera3ErrorMsgCode::CAMERA3_MSG_ERROR_DEVICE)); Out of curiosity, does the order of running |callback| vs. calling Notify() matter?
Thank you so much for spending time reviewing this gigantic CL, Christian! https://codereview.chromium.org/2837273004/diff/420001/media/capture/video/ch... File media/capture/video/chromeos/camera_device_delegate.cc (right): https://codereview.chromium.org/2837273004/diff/420001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:31: DCHECK_EQ(device_context_->GetState(), CameraDeviceContext::State::kStopped); On 2017/06/08 21:58:38, chfremer wrote: > This check looks like something that could be checked in a unit test instead of > here. Done.Let me remove this DCHECK. It's kind of unnecessary from hindsight. https://codereview.chromium.org/2837273004/diff/420001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:34: int32_t camera_id = std::stoi(device_descriptor_.device_id); On 2017/06/08 21:58:38, chfremer wrote: > Can this fail? It is probably illegal to call CameraDeviceDelegate() with a > descriptor whose device_id is not an integer number. How about we do the > conversion to int32_t in the constructor and add a DCHECK to make sure it > succeeded? Done. https://codereview.chromium.org/2837273004/diff/420001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:179: int32_t camera_id = std::stoi(device_descriptor_.device_id); On 2017/06/08 21:58:38, chfremer wrote: > Can probably be removed here if we did this in the constructor. Done. https://codereview.chromium.org/2837273004/diff/420001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:229: CameraDeviceContext::State::kStopping); On 2017/06/08 21:58:37, chfremer wrote: > Do we need to call OnClosed(0) here? > > Since the state and error checking code appears at least 3 times in almost > identical form, can we factor it out and reuse it? We don't need to call OnClosed(0) because it will be handled by either the Mojo call back of the Close() call in StopAndDeAllocate or by OnMojoConnectionError(). We need to call OnClosed(0) before in OnGotDeviceInfo and OnOpenedDevice because |device_ops_| is bound only after OnOpenedDevice returns. https://codereview.chromium.org/2837273004/diff/420001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:232: if (result) { On 2017/06/08 21:58:38, chfremer wrote: > In the other methods, at this spot we are doing a LOG(ERROR). Why not here? > Let's make it consistent. I should remove the LOG(ERROR) in other methods that calls SetErrorState() because there's a LOG(ERROR) inside SetErrorState(). https://codereview.chromium.org/2837273004/diff/500001/media/capture/video/ch... File media/capture/video/chromeos/camera_device_delegate.cc (right): https://codereview.chromium.org/2837273004/diff/500001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:220: new StreamCaptureInterfaceImpl(this)), On 2017/06/08 21:58:38, chfremer wrote: > Would it be possible to have CameraDeviceDelegate directly implement > StreamCaptureInterface and have StreamBufferManager's constructor accept a > scoped_refptr<StreamCaptureInterface>? Unless there is a technical reason > preventing this, it would allow us to get rid of StreamCaptureInterfaceImpl, and > the invocation here would become > > new StreamBufferManager( > std::move(callback_ops_request), this, > device_context_.get(), ipc_task_runner_); IMHO making StreamBufferManager a refptr complicates the ownership relations and we don't have a reason to make StreamBufferManager ref-counted. CameraDeviceDelegate is ref-counted because it's used in Mojo callbacks. https://codereview.chromium.org/2837273004/diff/500001/media/capture/video/ch... File media/capture/video/chromeos/stream_buffer_manager.cc (right): https://codereview.chromium.org/2837273004/diff/500001/media/capture/video/ch... media/capture/video/chromeos/stream_buffer_manager.cc:336: CaptureResult& partial_result = partial_results_[frame_number]; On 2017/06/08 21:58:38, chfremer wrote: > Idea for a possible improvement: If we wanted to further break down the > complexity into smaller and easier digestable/testable/maintainable pieces, one > potential strategy to do so might be to move the logic for accumulating these > partial results, verifying their correctness and correct order into a separate > class, e.g. something that could be called PartialResultsAccumulator. This would > leave the class StreamBufferManager with the core flow of the capture loop as > its single responsibility, and with the methods becoming shorter and free of > other responsibilities, this flow would become nicely obvious. We could then > even think about renaming the class to something like CaptureMainLoop. This sounds like a good design improvement. I'll see if I can split the responsibilities in another CL. I think it may help the befriending test class issue you mentioned. https://codereview.chromium.org/2837273004/diff/500001/media/capture/video/ch... media/capture/video/chromeos/stream_buffer_manager.cc:456: if (!sync_wait(fence.get().handle, kSyncWaitTimeoutMs)) { On 2017/06/08 21:58:38, chfremer wrote: > So, if I understand this correctly, for each frame, we are sharing a buffer with > the HAL service. The service maps the buffer to its memory space, places data > into the buffer and then unmaps the buffer again. Here we wait for the unmapping > to complete. > > What is the resource cost / performance overhead of the repeated > sharing/mapping/unmapping? > The buffer sharing between video capture service, Browser, and Renderers works > differently, and does so explicitly to avoid the cost of repeated mapping and > unmapping. What is done there is to keep buffers shared (up to a given maximum > number of them). When the frame data producer has placed data into a buffer, it > does not unmap, but simply sends a message that says that data is available. > When the consumer has finished consuming the data, it sends a message to the > producer to let it know that the buffer is ready to be reused. > > Ideally, we would have a single reusable implementation for transporting frame > data across process boundaries and would be applying it wherever needed (not > just for video data). It seem we are currently going the way of having separate > implementations for each case, non of which is reusable. Fixing this would > probably be quite a bit of work, and I don't want this to block the progress on > the ARC++ HALv3 work, but I think this should eventually be our goal. Yes your understanding regarding the buffer circulation and operation is correct. The map/unmap operations are unavoidable currently due to several reasons: 1. The Android HALv3 API explicitly specifies that the camera HAL must expect that the buffer handle it receives in each capture request to be a new buffer which it has never seen before. 2. The user space processes need to map/unmap buffers because I'm using SharedMemory in this implementation. 1. is the fundamental limitation that stops us to use a fixed set of shared SharedMemory buffers. In the optimized scenario the user space processes do not need to map/unmap the buffers. The buffers can be passed to the hardware for processing during the pipeline (e.g. give the buffer fd to the camera driver or image signal processor driver directly and let the driver map the buffer in IOMMU). To achieve this would require several changes (on Chrome OS and Linux, I don't know about Windows and Mac :/): a. Use dmabuf not shared-memory, so that the buffers can be imported and used by kernel drivers easily. In general shared-memory is not very friendly to driver use cases. We've run into various tricky alignment issues with Chrome's shared-memory on Chrome OS. b. Refactor Chrome to use dmabuf and only pass the buffer FD whenever possible. a. is in my TODO list. I'm going to change the SharedMemory to dmabuf on the ARC++ HALv3 VCD (i.e. change the buffer allocation in the SetUpStreamAndBuffers() above). b. on the other hand would require some code refactors that affect all platforms. For example, in the current OnIncomingCapturedData implementation it has to do a memcpy even if the received buffer is of format I420. In the world on modern Linux kernel the "single reusable" implementation is to use dmabuf everywhere whenever possible, but as Chrome also needs to run on Windows and Mac I'm not sure how we can design this buffer circulation mechanism (I know too little about how Chrome works on other platforms). https://codereview.chromium.org/2837273004/diff/500001/media/capture/video/ch... File media/capture/video/chromeos/stream_buffer_manager.h (right): https://codereview.chromium.org/2837273004/diff/500001/media/capture/video/ch... media/capture/video/chromeos/stream_buffer_manager.h:60: friend class StreamBufferManagerTest; On 2017/06/08 21:58:38, chfremer wrote: > nit: I am not asking you to change this now, but as a general design > recommendation, tests should not have to know anything about the private section > of the thing they test. If the private section contains complexity that requires > more direct testing than possible through the public API, this is a sign that > this complexity should really be exposed publicly through a class of its own, > which can then be tested. I agree. I'll see how I can refactor the codes to strip dependencies on friend test classes. https://codereview.chromium.org/2837273004/diff/500001/media/capture/video/ch... File media/capture/video/chromeos/stream_buffer_manager_unittest.cc (right): https://codereview.chromium.org/2837273004/diff/500001/media/capture/video/ch... media/capture/video/chromeos/stream_buffer_manager_unittest.cc:78: device_context_.reset(new CameraDeviceContext(std::move(mock_client))); On 2017/06/08 21:58:38, chfremer wrote: > nit: Not sure what the style guide says here, but I ususally prefer to use > base::MakeUnique<> over new wherever possible, e.g. > auto mock_client = > base::MakeUnique<unittest_internal::MockVideoCaptureClient>(); > device_context_ = base::MakeUnique<CameraDeviceContext>(std::move(mock_client)); > > Or we could even get rid of the temp var and do > device_context_ = > base::MakeUnique<CameraDeviceContext>(base::MakeUnique<unittest_internal::MockVideoCaptureClient>()); Done. https://codereview.chromium.org/2837273004/diff/500001/media/capture/video/ch... media/capture/video/chromeos/stream_buffer_manager_unittest.cc:83: new MockStreamCaptureInterface()), On 2017/06/08 21:58:38, chfremer wrote: > base::MakeUnique<> Done. https://codereview.chromium.org/2837273004/diff/500001/media/capture/video/ch... media/capture/video/chromeos/stream_buffer_manager_unittest.cc:90: } On 2017/06/08 21:58:38, chfremer wrote: > I still don't think that this is a clean solution. Do we have any plans on the > roadmap to change this mechanism or is this the proposed target solution? This pattern in the unit test is mainly to make sure all the on-going tasks on various threads are settled before we destroy the Mojo channel. I've hit some DCHECKs in Mojo without doing so. Since there's no public interface in the StreamBufferManager for me to query if all the tasks that hold a reference to StreamBufferManager are done, the only way I can think of is to query the ref counter. In VideoCaptureDeviceArcChromeOS::StopAndDeAllocate, I'm using the ref counter as a indicator of whether the close operation complete because of a fundamental assumption on the behavior of the camera HAL: the camera HAL is not guaranteed to return a Mojo call (e.g. there's a deadlock inside the camera HAL). We made this assumption because Google does not implement the camera HAL of each device; the camera HAL is provided by our partners. We do our best to review their codes but it's hard to cover everything. For example, the camera HAL for our latest Intel device has 90,000+ lines of codes and we can only look at the big picture in the given time frame. Given that the camera HAL may deadlock on the Close call, using either a WaitableEvent or a callback function is not guaranteed to work as we may have a live-lock if we wait on a WaitableEvent or callback without a time-out. If we set a timeout of the wait call of a WaitableEvent, it brings up another potential race condition: we have to make sure a WaitableEvent is not accessed after it's destroyed to avoid segfault, which could happen if the HAL calls the callback with a delay greater than the timeout we set. I may be able to remove the HasOneRef usage in VideoCaptureDeviceArcChromeOS::StopAndDeAllocate by moving the ipc thread into CameraDeviceDelegate, but I'd like to fix it in another CL. https://codereview.chromium.org/2837273004/diff/500001/media/capture/video/ch... media/capture/video/chromeos/stream_buffer_manager_unittest.cc:316: arc::mojom::Camera3ErrorMsgCode::CAMERA3_MSG_ERROR_DEVICE)); On 2017/06/08 21:58:38, chfremer wrote: > Out of curiosity, does the order of running |callback| vs. calling Notify() > matter? It shouldn't matter in this case. I believe device error may happen any time. |callback| may never run in real cases. I'm calling |callback| here just to avoid a DCHECK in Mojo complaining a callback passed to the Mojo interface call is never run.
The CQ bit was checked by jcliang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm Congrats! https://codereview.chromium.org/2837273004/diff/520001/media/capture/video/ch... File media/capture/video/chromeos/camera_device_delegate.cc (right): https://codereview.chromium.org/2837273004/diff/520001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:128: // channel before we do, in which case the OnClosed callback would not be s/would/will/ https://codereview.chromium.org/2837273004/diff/520001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:134: ResetMojoInterface(); Move this after line 136 so the call order is similar to OnClosed. It's easier to read the code. https://codereview.chromium.org/2837273004/diff/520001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:136: device_context_->SetErrorState(FROM_HERE, "Mojo connection error"); Document we cannot clear device_context_ here because the client will call StopAndDeallocate later. https://codereview.chromium.org/2837273004/diff/520001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:178: mojo::MakeRequest(&device_ops_); Document device_ops_ will be bound here. https://codereview.chromium.org/2837273004/diff/520001/media/capture/video/ch... File media/capture/video/chromeos/camera_device_delegate.h (right): https://codereview.chromium.org/2837273004/diff/520001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.h:51: // on the module thread of |hal_delegate_| and the device thread which is s/module/module ipc/ s/device/device ipc/ https://codereview.chromium.org/2837273004/diff/520001/media/capture/video/ch... File media/capture/video/chromeos/camera_hal_delegate.cc (right): https://codereview.chromium.org/2837273004/diff/520001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:94: DCHECK(child_pipe.is_valid()); return false if !child_pipe.is_valid() https://codereview.chromium.org/2837273004/diff/520001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:287: if (num_cameras < 0) { <= 0 because it shouldn't happen. Printing it in log will be easier to debug. https://codereview.chromium.org/2837273004/diff/520001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:289: LOG(ERROR) << "Failed to get number of cameras"; print num_cameras https://codereview.chromium.org/2837273004/diff/520001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:338: if (!builtin_camera_info_updated_.IsSignaled()) { These code can be removed. https://codereview.chromium.org/2837273004/diff/520001/media/capture/video/ch... File media/capture/video/chromeos/camera_hal_delegate.h (right): https://codereview.chromium.org/2837273004/diff/520001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.h:83: void ResetMojoInterfaceOnModuleThread(); ResetMojoInterfaceOnIPCThread
https://codereview.chromium.org/2837273004/diff/520001/media/capture/video/ch... File media/capture/video/chromeos/camera_device_delegate.cc (right): https://codereview.chromium.org/2837273004/diff/520001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:128: // channel before we do, in which case the OnClosed callback would not be On 2017/06/09 07:43:53, wuchengli wrote: > s/would/will/ Done. https://codereview.chromium.org/2837273004/diff/520001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:134: ResetMojoInterface(); On 2017/06/09 07:43:53, wuchengli wrote: > Move this after line 136 so the call order is similar to OnClosed. It's easier > to read the code. Done. https://codereview.chromium.org/2837273004/diff/520001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:136: device_context_->SetErrorState(FROM_HERE, "Mojo connection error"); On 2017/06/09 07:43:54, wuchengli wrote: > Document we cannot clear device_context_ here because the client will call > StopAndDeallocate later. Done. https://codereview.chromium.org/2837273004/diff/520001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:178: mojo::MakeRequest(&device_ops_); On 2017/06/09 07:43:53, wuchengli wrote: > Document device_ops_ will be bound here. Done. https://codereview.chromium.org/2837273004/diff/520001/media/capture/video/ch... File media/capture/video/chromeos/camera_device_delegate.h (right): https://codereview.chromium.org/2837273004/diff/520001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.h:51: // on the module thread of |hal_delegate_| and the device thread which is On 2017/06/09 07:43:54, wuchengli wrote: > s/module/module ipc/ > s/device/device ipc/ Done. https://codereview.chromium.org/2837273004/diff/520001/media/capture/video/ch... File media/capture/video/chromeos/camera_hal_delegate.cc (right): https://codereview.chromium.org/2837273004/diff/520001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:94: DCHECK(child_pipe.is_valid()); On 2017/06/09 07:43:54, wuchengli wrote: > return false if !child_pipe.is_valid() Done. https://codereview.chromium.org/2837273004/diff/520001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:287: if (num_cameras < 0) { On 2017/06/09 07:43:54, wuchengli wrote: > <= 0 because it shouldn't happen. Printing it in log will be easier to debug. Done. https://codereview.chromium.org/2837273004/diff/520001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:289: LOG(ERROR) << "Failed to get number of cameras"; On 2017/06/09 07:43:54, wuchengli wrote: > print num_cameras Done. https://codereview.chromium.org/2837273004/diff/520001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:338: if (!builtin_camera_info_updated_.IsSignaled()) { On 2017/06/09 07:43:54, wuchengli wrote: > These code can be removed. Done. https://codereview.chromium.org/2837273004/diff/520001/media/capture/video/ch... File media/capture/video/chromeos/camera_hal_delegate.h (right): https://codereview.chromium.org/2837273004/diff/520001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.h:83: void ResetMojoInterfaceOnModuleThread(); On 2017/06/09 07:43:54, wuchengli wrote: > ResetMojoInterfaceOnIPCThread Done.
The CQ bit was checked by jcliang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
jcliang@chromium.org changed reviewers: + reveman@chromium.org
David - Can you take a look at the modifications to third_party/libsync?
The CQ bit was checked by jcliang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jcliang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Following up on some of the discussion points. These are just ideas for future work or questions out of curiosity, so still okay to land. https://codereview.chromium.org/2837273004/diff/500001/media/capture/video/ch... File media/capture/video/chromeos/camera_device_delegate.cc (right): https://codereview.chromium.org/2837273004/diff/500001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:220: new StreamCaptureInterfaceImpl(this)), On 2017/06/09 05:16:01, jcliang wrote: > On 2017/06/08 21:58:38, chfremer wrote: > > Would it be possible to have CameraDeviceDelegate directly implement > > StreamCaptureInterface and have StreamBufferManager's constructor accept a > > scoped_refptr<StreamCaptureInterface>? Unless there is a technical reason > > preventing this, it would allow us to get rid of StreamCaptureInterfaceImpl, > and > > the invocation here would become > > > > new StreamBufferManager( > > std::move(callback_ops_request), this, > > device_context_.get(), ipc_task_runner_); > > IMHO making StreamBufferManager a refptr complicates the ownership relations and > we don't have a reason to make StreamBufferManager ref-counted. > CameraDeviceDelegate is ref-counted because it's used in Mojo callbacks. Just to make sure this is not a misunderstanding: I am not suggesting to make StreamBufferManager ref-counted. My suggestion is to make CameraDeviceDelegate implement StreamCaptureInterface in order to eliminate the need for class StreamCaptureInterfaceImpl. https://codereview.chromium.org/2837273004/diff/500001/media/capture/video/ch... File media/capture/video/chromeos/stream_buffer_manager.cc (right): https://codereview.chromium.org/2837273004/diff/500001/media/capture/video/ch... media/capture/video/chromeos/stream_buffer_manager.cc:456: if (!sync_wait(fence.get().handle, kSyncWaitTimeoutMs)) { On 2017/06/09 05:16:01, jcliang wrote: > On 2017/06/08 21:58:38, chfremer wrote: > > So, if I understand this correctly, for each frame, we are sharing a buffer > with > > the HAL service. The service maps the buffer to its memory space, places data > > into the buffer and then unmaps the buffer again. Here we wait for the > unmapping > > to complete. > > > > What is the resource cost / performance overhead of the repeated > > sharing/mapping/unmapping? > > The buffer sharing between video capture service, Browser, and Renderers works > > differently, and does so explicitly to avoid the cost of repeated mapping and > > unmapping. What is done there is to keep buffers shared (up to a given maximum > > number of them). When the frame data producer has placed data into a buffer, > it > > does not unmap, but simply sends a message that says that data is available. > > When the consumer has finished consuming the data, it sends a message to the > > producer to let it know that the buffer is ready to be reused. > > > > Ideally, we would have a single reusable implementation for transporting frame > > data across process boundaries and would be applying it wherever needed (not > > just for video data). It seem we are currently going the way of having > separate > > implementations for each case, non of which is reusable. Fixing this would > > probably be quite a bit of work, and I don't want this to block the progress > on > > the ARC++ HALv3 work, but I think this should eventually be our goal. > > Yes your understanding regarding the buffer circulation and operation is > correct. The map/unmap operations are unavoidable currently due to several > reasons: > > 1. The Android HALv3 API explicitly specifies that the camera HAL must expect > that the buffer handle it receives in each capture request to be a new buffer > which it has never seen before. > > 2. The user space processes need to map/unmap buffers because I'm using > SharedMemory in this implementation. > > 1. is the fundamental limitation that stops us to use a fixed set of shared > SharedMemory buffers. > > In the optimized scenario the user space processes do not need to map/unmap the > buffers. The buffers can be passed to the hardware for processing during the > pipeline (e.g. give the buffer fd to the camera driver or image signal processor > driver directly and let the driver map the buffer in IOMMU). To achieve this > would require several changes (on Chrome OS and Linux, I don't know about > Windows and Mac :/): > > a. Use dmabuf not shared-memory, so that the buffers can be imported and used by > kernel drivers easily. In general shared-memory is not very friendly to driver > use cases. We've run into various tricky alignment issues with Chrome's > shared-memory on Chrome OS. > > b. Refactor Chrome to use dmabuf and only pass the buffer FD whenever possible. > > a. is in my TODO list. I'm going to change the SharedMemory to dmabuf on the > ARC++ HALv3 VCD (i.e. change the buffer allocation in the > SetUpStreamAndBuffers() above). > > b. on the other hand would require some code refactors that affect all > platforms. For example, in the current OnIncomingCapturedData implementation it > has to do a memcpy even if the received buffer is of format I420. > > In the world on modern Linux kernel the "single reusable" implementation is to > use dmabuf everywhere whenever possible, but as Chrome also needs to run on > Windows and Mac I'm not sure how we can design this buffer circulation mechanism > (I know too little about how Chrome works on other platforms). I have to say that I don't fully understand the context, so apologies if my questions may seem a bit silly here. I was assuming that this code here running in Chromium is talking to a Chrome OS service (running with system privileges, but not being a kernel module) which we designed and implemented as well, and which is also used by Android-on-ChromeOS. If this system service currently wants to treat each buffer handle as new and map/unmap it for each frame I understand that the Chromium code here needs to match that. However, if we control the design and implementation of the system service, why couldn't we change it to allow reusing shared buffers if that would help us? Is it not possible/allowed to have shared memory mapped simultaneously by both a user-space process and a system service? https://codereview.chromium.org/2837273004/diff/500001/media/capture/video/ch... File media/capture/video/chromeos/stream_buffer_manager_unittest.cc (right): https://codereview.chromium.org/2837273004/diff/500001/media/capture/video/ch... media/capture/video/chromeos/stream_buffer_manager_unittest.cc:90: } On 2017/06/09 05:16:01, jcliang wrote: > On 2017/06/08 21:58:38, chfremer wrote: > > I still don't think that this is a clean solution. Do we have any plans on the > > roadmap to change this mechanism or is this the proposed target solution? > > This pattern in the unit test is mainly to make sure all the on-going tasks on > various threads are settled before we destroy the Mojo channel. I've hit some > DCHECKs in Mojo without doing so. Since there's no public interface in the > StreamBufferManager for me to query if all the tasks that hold a reference to > StreamBufferManager are done, the only way I can think of is to query the ref > counter. Wouldn't it be possible to add a method to StreamBufferManager to have it shutdown and invoke a callback when done, e.g. void Shutdown(base::OnceClosure done_cb); ? > In VideoCaptureDeviceArcChromeOS::StopAndDeAllocate, I'm using the ref counter > as a indicator of whether the close operation complete because of a fundamental > assumption on the behavior of the camera HAL: the camera HAL is not guaranteed > to return a Mojo call (e.g. there's a deadlock inside the camera HAL). We made > this assumption because Google does not implement the camera HAL of each device; > the camera HAL is provided by our partners. We do our best to review their codes > but it's hard to cover everything. For example, the camera HAL for our latest > Intel device has 90,000+ lines of codes and we can only look at the big picture > in the given time frame. I see. It seems media::VideoCaptureDevice::StopAndDeAllocate() is currently meant to either block until shutdown is complete or perform the shutdown asynchronously without telling the caller when it is done. This design is actually problematic, and I am currently looking into ways to improve it, see [1]. But even without a change to the design of media::VideoCaptureDevice::StopAndDeAllocate(), you could already support asynchronous shutdown internally by making both CameraDeviceDelegate::StopAndDeAllocate() and StreamBufferManager::StopCapture() take a |base::OnceClosure done_cb| as parameter. Inside VideoCaptureDeviceArcChromeOS::StopAndDeAllocate() you would then have the option to wait for the callback using a WaitableEvent instead of the polling loop. A timeout waiting for the camera HAL makes sense. If we are not required by the API to block the thread that invokes us, we could simply implement this by posting a delayed task to ourselves for aborting in case the camera HAL does not respond in time. Let's consider this as an idea for future improvements. [1] https://bugs.chromium.org/p/chromium/issues/detail?id=725271#c5 > Given that the camera HAL may deadlock on the Close call, using either a > WaitableEvent or a callback function is not guaranteed to work as we may have a > live-lock if we wait on a WaitableEvent or callback without a time-out. If we > set a timeout of the wait call of a WaitableEvent, it brings up another > potential race condition: we have to make sure a WaitableEvent is not accessed > after it's destroyed to avoid segfault, which could happen if the HAL calls the > callback with a delay greater than the timeout we set. > > I may be able to remove the HasOneRef usage in > VideoCaptureDeviceArcChromeOS::StopAndDeAllocate by moving the ipc thread into > CameraDeviceDelegate, but I'd like to fix it in another CL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2837273004/diff/500001/media/capture/video/ch... File media/capture/video/chromeos/stream_buffer_manager.cc (right): https://codereview.chromium.org/2837273004/diff/500001/media/capture/video/ch... media/capture/video/chromeos/stream_buffer_manager.cc:456: if (!sync_wait(fence.get().handle, kSyncWaitTimeoutMs)) { On 2017/06/09 17:53:17, chfremer wrote: > On 2017/06/09 05:16:01, jcliang wrote: > > On 2017/06/08 21:58:38, chfremer wrote: > > > So, if I understand this correctly, for each frame, we are sharing a buffer > > with > > > the HAL service. The service maps the buffer to its memory space, places > data > > > into the buffer and then unmaps the buffer again. Here we wait for the > > unmapping > > > to complete. > > > > > > What is the resource cost / performance overhead of the repeated > > > sharing/mapping/unmapping? > > > The buffer sharing between video capture service, Browser, and Renderers > works > > > differently, and does so explicitly to avoid the cost of repeated mapping > and > > > unmapping. What is done there is to keep buffers shared (up to a given > maximum > > > number of them). When the frame data producer has placed data into a buffer, > > it > > > does not unmap, but simply sends a message that says that data is available. > > > When the consumer has finished consuming the data, it sends a message to the > > > producer to let it know that the buffer is ready to be reused. > > > > > > Ideally, we would have a single reusable implementation for transporting > frame > > > data across process boundaries and would be applying it wherever needed (not > > > just for video data). It seem we are currently going the way of having > > separate > > > implementations for each case, non of which is reusable. Fixing this would > > > probably be quite a bit of work, and I don't want this to block the progress > > on > > > the ARC++ HALv3 work, but I think this should eventually be our goal. > > > > Yes your understanding regarding the buffer circulation and operation is > > correct. The map/unmap operations are unavoidable currently due to several > > reasons: > > > > 1. The Android HALv3 API explicitly specifies that the camera HAL must expect > > that the buffer handle it receives in each capture request to be a new buffer > > which it has never seen before. > > > > 2. The user space processes need to map/unmap buffers because I'm using > > SharedMemory in this implementation. > > > > 1. is the fundamental limitation that stops us to use a fixed set of shared > > SharedMemory buffers. > > > > In the optimized scenario the user space processes do not need to map/unmap > the > > buffers. The buffers can be passed to the hardware for processing during the > > pipeline (e.g. give the buffer fd to the camera driver or image signal > processor > > driver directly and let the driver map the buffer in IOMMU). To achieve this > > would require several changes (on Chrome OS and Linux, I don't know about > > Windows and Mac :/): > > > > a. Use dmabuf not shared-memory, so that the buffers can be imported and used > by > > kernel drivers easily. In general shared-memory is not very friendly to driver > > use cases. We've run into various tricky alignment issues with Chrome's > > shared-memory on Chrome OS. > > > > b. Refactor Chrome to use dmabuf and only pass the buffer FD whenever > possible. > > > > a. is in my TODO list. I'm going to change the SharedMemory to dmabuf on the > > ARC++ HALv3 VCD (i.e. change the buffer allocation in the > > SetUpStreamAndBuffers() above). > > > > b. on the other hand would require some code refactors that affect all > > platforms. For example, in the current OnIncomingCapturedData implementation > it > > has to do a memcpy even if the received buffer is of format I420. > > > > In the world on modern Linux kernel the "single reusable" implementation is to > > use dmabuf everywhere whenever possible, but as Chrome also needs to run on > > Windows and Mac I'm not sure how we can design this buffer circulation > mechanism > > (I know too little about how Chrome works on other platforms). > > I have to say that I don't fully understand the context, so apologies if my > questions may seem a bit silly here. > > I was assuming that this code here running in Chromium is talking to a Chrome OS > service (running with system privileges, but not being a kernel module) which we > designed and implemented as well, and which is also used by Android-on-ChromeOS. > If this system service currently wants to treat each buffer handle as new and > map/unmap it for each frame I understand that the Chromium code here needs to > match that. However, if we control the design and implementation of the system > service, why couldn't we change it to allow reusing shared buffers if that would > help us? The camera service on Chrome OS has two parts: 1. A camera HAL adapter which loads a device-specific camera HAL library, and talk to Chrome and Android container to provide them with camera functions. This part is designed and implemented by Google. 2. A device-specific camera HAL library which include codes that are tailored for specific hardware. This part is provided by our partners. The interface we use between 1. and 2. is the Android camera HAL v3 APIs, which specifies the constraints on the buffers. We can change or relax the constraints, but that would break the original API contract and our partners will need to modify their HAL library accordingly. AFAIK, we do relax this constraint on some Android devices to improve the performance. In general it's easier for Android to do per-device optimizations. In Chrome OS we have to maintain dozens of different devices at the same time and doing board-specific code changes in general camera code path is not trivial. > > Is it not possible/allowed to have shared memory mapped simultaneously by both a > user-space process and a system service? > Mapping the same shared memory into different processes is not a problem, but we'd like to follow the Android API specification for now.
The CQ bit was checked by jcliang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
posciak@chromium.org changed reviewers: + posciak@chromium.org
https://codereview.chromium.org/2837273004/diff/620001/media/capture/BUILD.gn File media/capture/BUILD.gn (right): https://codereview.chromium.org/2837273004/diff/620001/media/capture/BUILD.gn... media/capture/BUILD.gn:52: "video/chromeos/camera_device_context.cc", Could any of these also be under if (is_chromeos)? https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... File media/capture/video/chromeos/camera_device_context.cc (right): https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/camera_device_context.cc:48: client_->OnIncomingCapturedData(data, length, frame_format, rotation_, DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); ? https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... File media/capture/video/chromeos/camera_device_context.h (right): https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/camera_device_context.h:22: enum State { could this be enum class? https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/camera_device_context.h:110: int length, size_t perhaps? https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... File media/capture/video/chromeos/camera_device_delegate.cc (right): https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:15: #include "third_party/libsync/include/sync/sync.h" How/where is this used? https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:24: : camera_device_delegate_(std::move(camera_device_delegate)) {} Is std::move needed, here and in other similar locations in this CL? https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:69: // We need to get the static camera metadata of the camera deivce first. s/deivce/device/ https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:72: &CameraDeviceDelegate::OnGotCameraInfo, this))); I'm wondering, do we in general need to explicitly have the callbacks own |this|? Would weak ptr be feasible here and in other places? https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:99: // TODO(jcliang): Implement TakePhoto. Perhaps NOTIMPLEMENTED() << "...", here and in other places? https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:219: device_context_.get(), ipc_task_runner_); stream_buffer_manager_ is a scoped_refptr, while device_context_ is a unique_ptr. Could stream_buffer_manager_ be a unique_ptr as well? https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:260: arc::mojom::HalPixelFormat::HAL_PIXEL_FORMAT_YCbCr_420_888; Would it be useful to have a (D)CHECK on chrome_capture_params_.requested_format.pixel_format here just in case? https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... File media/capture/video/chromeos/camera_device_delegate.h (right): https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.h:25: virtual ~StreamCaptureInterface() {} = default ? https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.h:35: std::vector<uint32_t> strides, Perhaps a struct Plane {uint32_t stride; uint32_t offset;} to avoid having to check if the vectors are of the same size? https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.h:81: ~CameraDeviceDelegate(); = default? https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... File media/capture/video/chromeos/camera_hal_delegate.cc (right): https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:168: int32_t format = static_cast<int32_t>(iter[kStreamFormatOffset]); checked_cast ? https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:174: float max_fps = 1.0 * 1000000000LL / duration; Should we check for duration != 0 ? https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:359: // TODO(jcliang): Handle status change for external cameras. NOTIMPLEMENTED() ? https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... File media/capture/video/chromeos/camera_hal_delegate.h (right): https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.h:40: // should be called before any other methods of CameraHalDelegate is called. Would it perhaps make sense to call it from the constructor, or have a factory method constructing and calling this method to guarantee the above? https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.h:126: uint32_t num_builtin_cameras_; Could this be size_t? https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... File media/capture/video/chromeos/stream_buffer_manager.cc (right): https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/stream_buffer_manager.cc:212: // In case |frame_number_| wraps around, we start at 1 to avoid resetting If we keep first_frame_shutter_time_ at default 0 instead of Now() in constructor, could we check for first_frame_shutter_time_.is_null() instead of frame_number == 0 in Notify, and not have this as well? https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/stream_buffer_manager.cc:245: if (partial_results_.size() > stream_context_->stream->max_buffers) { Should we check first for >=, and then grow partial_result? Otherwise I think we'll keep growing partial_result even if we exceed max? https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/stream_buffer_manager.cc:307: if (partial_result.partial_metadata_received.size() == Perhaps move this if and the same one in Notify() (and maybe in HandleNotifyError()) to SubmitCaptureResult() and make it SubmitCaptureResultIfComplete()? Also maybe the check for capture time? https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/stream_buffer_manager.cc:342: if (partial_results_.size() > stream_context_->stream->max_buffers) { Should we check first for >=, and then grow partial_result? Otherwise I think we'll keep growing partial_result even if we exceed max? https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/stream_buffer_manager.cc:390: case arc::mojom::Camera3ErrorMsgCode::CAMERA3_MSG_ERROR_RESULT: { Should we be dropping partial results for other errors than this one? https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... File media/capture/video/chromeos/stream_buffer_manager.h (right): https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/stream_buffer_manager.h:8: #include "base/memory/shared_memory.h" Perhaps use a forward declaration for SharedMemory instead? https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... File media/capture/video/chromeos/video_capture_device_arc_chromeos.cc (right): https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_arc_chromeos.cc:76: // Wait until all other references to |camera_device_delegate_| are dropped to Is it required for HAL IPC to finish all IPCs, or could we drop and close the connection? In either case, could we do this in the delegate's destructor instead? https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... File media/capture/video/chromeos/video_capture_device_arc_chromeos.h (right): https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_arc_chromeos.h:5: #ifndef MEDIA_CAPTURE_VIDEO_CHROMEOS_VIDEO_CAPTURE_DEVICE_CHROMEOS_H_ MEDIA_CAPTURE_VIDEO_CHROMEOS_VIDEO_CAPTURE_DEVICE_ARC_CHROMEOS_H_ ? https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_arc_chromeos.h:8: #include <stdint.h> Is this header needed? https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_arc_chromeos.h:11: #include <string> Is this header needed? https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_arc_chromeos.h:79: scoped_refptr<CameraDeviceDelegate> camera_device_delegate_; Would it make sense to consider unique_ptr and posting to weak pointers instead? Unless I'm missing something, if the delegate is not used by anyone else, if this class is destroyed, we could just invalidate the weak pointers and drop any callbacks. Or actually, if camera_device_ipc_thread_ is fully owned and used only in this class (which it appears so?), then we could just post to unretained, just ensuring that the thread got destroyed before the delegate...? https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... File media/capture/video/chromeos/video_capture_device_factory_chromeos.cc (right): https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_factory_chromeos.cc:62: VideoCaptureDeviceFactory::CreateVideoCaptureDeviceFactory( What would you think about naming the current VideoCaptureDeviceFactoryChromeOS something like "VideoCaptureDeviceFactoryAndroidHal" or similar instead, and maybe even having it on the same level as VideoCaptureDeviceFactoryLinux to avoid confusion and emphasize both are used depending on the existence of the Hal service? Something like: VideoCaptureDeviceFactory::CreateVideoCaptureDeviceFactory() { if (base::PathExists(kArcCamera3Service)) { return new VideoCaptureDeviceFactoryAndroidHal(...); } else { return new VideoCaptureDeviceFactoryLinux(...); } } https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/vi... File media/capture/video/video_capture_device_unittest.cc (right): https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/vi... media/capture/video/video_capture_device_unittest.cc:79: #elif defined(OS_CHROMEOS) Doesn't this disable tests for existing V4L2 devices on CrOS?
jcliang@chromium.org changed reviewers: - reveman@chromium.org
https://codereview.chromium.org/2837273004/diff/620001/media/capture/BUILD.gn File media/capture/BUILD.gn (right): https://codereview.chromium.org/2837273004/diff/620001/media/capture/BUILD.gn... media/capture/BUILD.gn:52: "video/chromeos/camera_device_context.cc", On 2017/06/13 08:40:15, Pawel Osciak wrote: > Could any of these also be under if (is_chromeos)? Done. https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... File media/capture/video/chromeos/camera_device_context.cc (right): https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/camera_device_context.cc:48: client_->OnIncomingCapturedData(data, length, frame_format, rotation_, On 2017/06/13 08:40:15, Pawel Osciak wrote: > DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); ? The |sequence_checker_| is mainly for making sure modifications made to |state_| is serialized. Since client_->OnIncomingCapturedData already allows to be called on any thread we don't need to do sequence check here. https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... File media/capture/video/chromeos/camera_device_context.h (right): https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/camera_device_context.h:22: enum State { On 2017/06/13 08:40:15, Pawel Osciak wrote: > could this be enum class? Done. https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/camera_device_context.h:110: int length, On 2017/06/13 08:40:15, Pawel Osciak wrote: > size_t perhaps? I'm using int here because it's what VideoCaptureDevice::Client::OnIncomingCapturedData expects: https://cs.chromium.org/chromium/src/media/capture/video/video_capture_device... We'll need to do static_cast if we declare length as size_t. https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... File media/capture/video/chromeos/camera_device_delegate.cc (right): https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:15: #include "third_party/libsync/include/sync/sync.h" On 2017/06/13 08:40:15, Pawel Osciak wrote: > How/where is this used? This is moved to stream_buffer_manager.h. I should remove the include here. https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:24: : camera_device_delegate_(std::move(camera_device_delegate)) {} On 2017/06/13 08:40:15, Pawel Osciak wrote: > Is std::move needed, here and in other similar locations in this CL? I think without std::move it invokes the copy constructor of scoped_refptr, and as a result we increase the reference by one in the constructor of |camera_device_delegate_| and then decrease the reference by one in the destructor of |camera_device_delegate| after we're out of the scope of this method. With std::move it calls the move constructor and the reference is simply transferred from |camera_device_delegate| to |camera_device_delegate_|. It's also more clear to the readers that |camera_deivce_delegate| does not hold any reference by reading just this one line of code. https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:69: // We need to get the static camera metadata of the camera deivce first. On 2017/06/13 08:40:16, Pawel Osciak wrote: > s/deivce/device/ Done. https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:72: &CameraDeviceDelegate::OnGotCameraInfo, this))); On 2017/06/13 08:40:15, Pawel Osciak wrote: > I'm wondering, do we in general need to explicitly have the callbacks own > |this|? Would weak ptr be feasible here and in other places? We can't use weak ptr of CameraDeviceDelegate in callbacks because it's bound to callbacks that will run on two different threads: ipc threads of CameraHalDelegate and CameraDeviceDelegate. We need to reference and invalidate a weak ptr on the same thread (SequencedTaskRunner actually) to avoid race conditions. https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:99: // TODO(jcliang): Implement TakePhoto. On 2017/06/13 08:40:15, Pawel Osciak wrote: > Perhaps NOTIMPLEMENTED() << "...", here and in other places? Done. https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:219: device_context_.get(), ipc_task_runner_); On 2017/06/13 08:40:16, Pawel Osciak wrote: > stream_buffer_manager_ is a scoped_refptr, while device_context_ is a > unique_ptr. Could stream_buffer_manager_ be a unique_ptr as well? Yes we can do this. StreamBufferManager only runs on the device ipc thread so we can bind it as weak ptr. I've changed |stream_buffer_manager_| to unique_ptr. https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:260: arc::mojom::HalPixelFormat::HAL_PIXEL_FORMAT_YCbCr_420_888; On 2017/06/13 08:40:15, Pawel Osciak wrote: > Would it be useful to have a (D)CHECK on > chrome_capture_params_.requested_format.pixel_format here just in case? The pixel format given in |chrome_capture_params_.requested_format.pixel_format| actually has nothing to do with the format that the VCD uses, which is a weird design IMO. In fact Chrome always set the requested_format to I420 and let the VCD implementation choose the most efficient (or preferred) format. There's a format conversion when we submit the captured data in client_->OnIncomingCapturedData. https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... File media/capture/video/chromeos/camera_device_delegate.h (right): https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.h:25: virtual ~StreamCaptureInterface() {} On 2017/06/13 08:40:16, Pawel Osciak wrote: > = default ? I'm hitting style checker errors in the trybots with default constructors and destructors: https://www.chromium.org/developers/coding-style/chromium-style-checker-errors I think we now discourage using default constructors / destructors to avoid code bloat. https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.h:35: std::vector<uint32_t> strides, On 2017/06/13 08:40:16, Pawel Osciak wrote: > Perhaps a struct Plane {uint32_t stride; uint32_t offset;} to avoid having to > check if the vectors are of the same size? Done. https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.h:81: ~CameraDeviceDelegate(); On 2017/06/13 08:40:16, Pawel Osciak wrote: > = default? Same here. Default destructor is not favored by the style checker. https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... File media/capture/video/chromeos/camera_hal_delegate.cc (right): https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:168: int32_t format = static_cast<int32_t>(iter[kStreamFormatOffset]); On 2017/06/13 08:40:16, Pawel Osciak wrote: > checked_cast ? Done. https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:174: float max_fps = 1.0 * 1000000000LL / duration; On 2017/06/13 08:40:16, Pawel Osciak wrote: > Should we check for duration != 0 ? Done. https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:359: // TODO(jcliang): Handle status change for external cameras. On 2017/06/13 08:40:16, Pawel Osciak wrote: > NOTIMPLEMENTED() ? Done. https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... File media/capture/video/chromeos/camera_hal_delegate.h (right): https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.h:40: // should be called before any other methods of CameraHalDelegate is called. On 2017/06/13 08:40:16, Pawel Osciak wrote: > Would it perhaps make sense to call it from the constructor, or have a factory > method constructing and calling this method to guarantee the above? The VideoCaptureDeviceFactory::GetVideoCaptureDeviceFactory is the factory method used by Chrome, and we call this function there. https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.h:126: uint32_t num_builtin_cameras_; On 2017/06/13 08:40:16, Pawel Osciak wrote: > Could this be size_t? Done. https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... File media/capture/video/chromeos/stream_buffer_manager.cc (right): https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/stream_buffer_manager.cc:212: // In case |frame_number_| wraps around, we start at 1 to avoid resetting On 2017/06/13 08:40:16, Pawel Osciak wrote: > If we keep first_frame_shutter_time_ at default 0 instead of Now() in > constructor, could we check for first_frame_shutter_time_.is_null() instead of > frame_number == 0 in Notify, and not have this as well? Done. Good idea! I didn't realize we can have a "null" base::TimeTicks. https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/stream_buffer_manager.cc:245: if (partial_results_.size() > stream_context_->stream->max_buffers) { On 2017/06/13 08:40:16, Pawel Osciak wrote: > Should we check first for >=, and then grow partial_result? Otherwise I think > we'll keep growing partial_result even if we exceed max? We can't do this since the results may be delivered in stages and there may be multiple results with the same frame_number. When we call the SetErrorState below Chrome will destroy the VCD and stop the capture completely. https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/stream_buffer_manager.cc:307: if (partial_result.partial_metadata_received.size() == On 2017/06/13 08:40:16, Pawel Osciak wrote: > Perhaps move this if and the same one in Notify() (and maybe in > HandleNotifyError()) to SubmitCaptureResult() and make it > SubmitCaptureResultIfComplete()? Also maybe the check for capture time? Done. https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/stream_buffer_manager.cc:342: if (partial_results_.size() > stream_context_->stream->max_buffers) { On 2017/06/13 08:40:16, Pawel Osciak wrote: > Should we check first for >=, and then grow partial_result? Otherwise I think > we'll keep growing partial_result even if we exceed max? Same as the comments in ProcessCaptureResult. https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/stream_buffer_manager.cc:390: case arc::mojom::Camera3ErrorMsgCode::CAMERA3_MSG_ERROR_RESULT: { On 2017/06/13 08:40:16, Pawel Osciak wrote: > Should we be dropping partial results for other errors than this one? For simplicity I've changed it to always drop the partial metadata. In theory we only need to drop the metadata for this one, but since Chrome doesn't consume the result metadata we can always drop it on error to make the codes simpler. https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... File media/capture/video/chromeos/stream_buffer_manager.h (right): https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/stream_buffer_manager.h:8: #include "base/memory/shared_memory.h" On 2017/06/13 08:40:16, Pawel Osciak wrote: > Perhaps use a forward declaration for SharedMemory instead? Done. https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... File media/capture/video/chromeos/video_capture_device_arc_chromeos.cc (right): https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_arc_chromeos.cc:76: // Wait until all other references to |camera_device_delegate_| are dropped to On 2017/06/13 08:40:16, Pawel Osciak wrote: > Is it required for HAL IPC to finish all IPCs, or could we drop and close the > connection? > In either case, could we do this in the delegate's destructor instead? In theory we can just kill the Mojo channel here and the camera HAL should close the device, but the timing of the close call in the HAL may be unconventional. We're trying to be nice here and give camera HAL a chance to properly shut down. We can't move this to the delegate's destructor if we want to wait for the close call to return. https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... File media/capture/video/chromeos/video_capture_device_arc_chromeos.h (right): https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_arc_chromeos.h:5: #ifndef MEDIA_CAPTURE_VIDEO_CHROMEOS_VIDEO_CAPTURE_DEVICE_CHROMEOS_H_ On 2017/06/13 08:40:16, Pawel Osciak wrote: > MEDIA_CAPTURE_VIDEO_CHROMEOS_VIDEO_CAPTURE_DEVICE_ARC_CHROMEOS_H_ ? Done. https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_arc_chromeos.h:8: #include <stdint.h> On 2017/06/13 08:40:16, Pawel Osciak wrote: > Is this header needed? Removed. https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_arc_chromeos.h:11: #include <string> On 2017/06/13 08:40:16, Pawel Osciak wrote: > Is this header needed? Removed. https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_arc_chromeos.h:79: scoped_refptr<CameraDeviceDelegate> camera_device_delegate_; On 2017/06/13 08:40:16, Pawel Osciak wrote: > Would it make sense to consider unique_ptr and posting to weak pointers instead? > Unless I'm missing something, if the delegate is not used by anyone else, if > this class is destroyed, we could just invalidate the weak pointers and drop any > callbacks. > > Or actually, if camera_device_ipc_thread_ is fully owned and used only in this > class (which it appears so?), then we could just post to unretained, just > ensuring that the thread got destroyed before the delegate...? The CameraDeviceDelegate posts itself to the ipc thread on CameraHalDelegate when it needs to query camera info (in AllocateAndStart) and open camera devices (in OnGotCameraInfo), so it needs to be a scoped_refptr here. I have some rough ideas to split it to two classes: one for creating the device on the ipc thread of CameraHalDelegate, and one for talking to the device on the ipc thread of CameraDeviceDelegate. This will need some refactor of the current code. I'll submit another CL to improve this if it works. https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... File media/capture/video/chromeos/video_capture_device_factory_chromeos.cc (right): https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_factory_chromeos.cc:62: VideoCaptureDeviceFactory::CreateVideoCaptureDeviceFactory( On 2017/06/13 08:40:16, Pawel Osciak wrote: > What would you think about naming the current VideoCaptureDeviceFactoryChromeOS > something like "VideoCaptureDeviceFactoryAndroidHal" or similar instead, and > maybe even having it on the same level as VideoCaptureDeviceFactoryLinux to > avoid confusion and emphasize both are used depending on the existence of the > Hal service? > > Something like: > > VideoCaptureDeviceFactory::CreateVideoCaptureDeviceFactory() { > if (base::PathExists(kArcCamera3Service)) { > return new VideoCaptureDeviceFactoryAndroidHal(...); > } else { > return new VideoCaptureDeviceFactoryLinux(...); > } > } IMO VideoCaptureDeviceFactoryAndroidHal is a bit inaccurate. We're using the Android HAL v3 API but the HAL implementation is quite different from the one usable on a Android device. Also I think it's a convention to make the factory class with the platform name as suffix. I've added some comments here to document the behavior to make it clear that we have to support two use cases. https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/vi... File media/capture/video/video_capture_device_unittest.cc (right): https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/vi... media/capture/video/video_capture_device_unittest.cc:79: #elif defined(OS_CHROMEOS) On 2017/06/13 08:40:16, Pawel Osciak wrote: > Doesn't this disable tests for existing V4L2 devices on CrOS? Right. I've changed to skip the tests based on a runtime check.
The CQ bit was checked by jcliang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by jcliang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
jcliang@chromium.org changed reviewers: + reveman@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jcliang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
Thanks! Just some small comments in addition to the offline chat discussion about wait cycle for HAL to clean up. https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... File media/capture/video/chromeos/camera_device_delegate.cc (right): https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:24: : camera_device_delegate_(std::move(camera_device_delegate)) {} On 2017/06/14 04:46:05, jcliang wrote: > On 2017/06/13 08:40:15, Pawel Osciak wrote: > > Is std::move needed, here and in other similar locations in this CL? > > I think without std::move it invokes the copy constructor of scoped_refptr, and > as a result we increase the reference by one in the constructor of > |camera_device_delegate_| and then decrease the reference by one in the > destructor of |camera_device_delegate| after we're out of the scope of this > method. > > With std::move it calls the move constructor and the reference is simply > transferred from |camera_device_delegate| to |camera_device_delegate_|. > > It's also more clear to the readers that |camera_deivce_delegate| does not hold > any reference by reading just this one line of code. Acknowledged. https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.cc:260: arc::mojom::HalPixelFormat::HAL_PIXEL_FORMAT_YCbCr_420_888; On 2017/06/14 04:46:05, jcliang wrote: > On 2017/06/13 08:40:15, Pawel Osciak wrote: > > Would it be useful to have a (D)CHECK on > > chrome_capture_params_.requested_format.pixel_format here just in case? > > The pixel format given in |chrome_capture_params_.requested_format.pixel_format| > actually has nothing to do with the format that the VCD uses, which is a weird > design IMO. In fact Chrome always set the requested_format to I420 and let the > VCD implementation choose the most efficient (or preferred) format. There's a > format conversion when we submit the captured data in > client_->OnIncomingCapturedData. Acknowledged. https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... File media/capture/video/chromeos/stream_buffer_manager.cc (right): https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/stream_buffer_manager.cc:245: if (partial_results_.size() > stream_context_->stream->max_buffers) { On 2017/06/14 04:46:07, jcliang wrote: > On 2017/06/13 08:40:16, Pawel Osciak wrote: > > Should we check first for >=, and then grow partial_result? Otherwise I think > > we'll keep growing partial_result even if we exceed max? > > We can't do this since the results may be delivered in stages and there may be > multiple results with the same frame_number. When we call the SetErrorState > below Chrome will destroy the VCD and stop the capture completely. Acknowledged. https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... File media/capture/video/chromeos/video_capture_device_factory_chromeos.cc (right): https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_factory_chromeos.cc:62: VideoCaptureDeviceFactory::CreateVideoCaptureDeviceFactory( On 2017/06/14 04:46:08, jcliang wrote: > On 2017/06/13 08:40:16, Pawel Osciak wrote: > > What would you think about naming the current > VideoCaptureDeviceFactoryChromeOS > > something like "VideoCaptureDeviceFactoryAndroidHal" or similar instead, and > > maybe even having it on the same level as VideoCaptureDeviceFactoryLinux to > > avoid confusion and emphasize both are used depending on the existence of the > > Hal service? > > > > Something like: > > > > VideoCaptureDeviceFactory::CreateVideoCaptureDeviceFactory() { > > if (base::PathExists(kArcCamera3Service)) { > > return new VideoCaptureDeviceFactoryAndroidHal(...); > > } else { > > return new VideoCaptureDeviceFactoryLinux(...); > > } > > } > > IMO VideoCaptureDeviceFactoryAndroidHal is a bit inaccurate. We're using the > Android HAL v3 API but the HAL implementation is quite different from the one > usable on a Android device. Also I think it's a convention to make the factory > class with the platform name as suffix. > > I've added some comments here to document the behavior to make it clear that we > have to support two use cases. Acknowledged. https://codereview.chromium.org/2837273004/diff/680001/media/capture/BUILD.gn File media/capture/BUILD.gn (right): https://codereview.chromium.org/2837273004/diff/680001/media/capture/BUILD.gn... media/capture/BUILD.gn:52: "video/chromeos/display_rotation_observer.cc", Per offline chat, this should also be movable. https://codereview.chromium.org/2837273004/diff/680001/media/capture/video/ch... File media/capture/video/chromeos/camera_hal_delegate.cc (right): https://codereview.chromium.org/2837273004/diff/680001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:175: LOG(ERROR) << "Ignoring invalid frame durtaion: " << duration; s/durtaion/duration/ https://codereview.chromium.org/2837273004/diff/680001/media/capture/video/ch... File media/capture/video/chromeos/stream_buffer_manager.cc (right): https://codereview.chromium.org/2837273004/diff/680001/media/capture/video/ch... media/capture/video/chromeos/stream_buffer_manager.cc:275: // will drop and resuse the buffer. s/resuse/reuse/ https://codereview.chromium.org/2837273004/diff/680001/media/capture/video/ch... media/capture/video/chromeos/stream_buffer_manager.cc:411: // On error we simply drop all the result metadata for the given frame. We Could we perhaps instead have: SubmitCaptureResultIfComplete() { if (partial_result.partial_metadata_received.size() < partial_result_count_ || partial_result.buffer.is_null() || partial_result.reference_time == base::TimeTicks()) { SubmitCaptureResult(); } } SubmitCaptureResult() { // the actual code to submit } And call SubmitCaptureResult() directly only from here (the error case?). Or, perhaps it would be enough to have the conditions in SubmitCaptureResultIfComplete() checked only if partial_result.buffer->status was not an error instead? https://codereview.chromium.org/2837273004/diff/680001/media/capture/video/ch... File media/capture/video/chromeos/video_capture_device_factory_chromeos.cc (right): https://codereview.chromium.org/2837273004/diff/680001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_factory_chromeos.cc:63: const base::FilePath kArcCamera3Service("/usr/bin/arc_camera3_service"); This could technically also be under #if defined(OS_CHROMEOS), but not a big issue.
media/capture/video/chromeos/DEPS lgtm
https://codereview.chromium.org/2837273004/diff/680001/media/capture/BUILD.gn File media/capture/BUILD.gn (right): https://codereview.chromium.org/2837273004/diff/680001/media/capture/BUILD.gn... media/capture/BUILD.gn:52: "video/chromeos/display_rotation_observer.cc", On 2017/06/14 09:07:43, Pawel Osciak wrote: > Per offline chat, this should also be movable. Done. https://codereview.chromium.org/2837273004/diff/680001/media/capture/video/ch... File media/capture/video/chromeos/camera_hal_delegate.cc (right): https://codereview.chromium.org/2837273004/diff/680001/media/capture/video/ch... media/capture/video/chromeos/camera_hal_delegate.cc:175: LOG(ERROR) << "Ignoring invalid frame durtaion: " << duration; On 2017/06/14 09:07:43, Pawel Osciak wrote: > s/durtaion/duration/ Done. https://codereview.chromium.org/2837273004/diff/680001/media/capture/video/ch... File media/capture/video/chromeos/stream_buffer_manager.cc (right): https://codereview.chromium.org/2837273004/diff/680001/media/capture/video/ch... media/capture/video/chromeos/stream_buffer_manager.cc:275: // will drop and resuse the buffer. On 2017/06/14 09:07:43, Pawel Osciak wrote: > s/resuse/reuse/ Done. https://codereview.chromium.org/2837273004/diff/680001/media/capture/video/ch... media/capture/video/chromeos/stream_buffer_manager.cc:411: // On error we simply drop all the result metadata for the given frame. We On 2017/06/14 09:07:43, Pawel Osciak wrote: > Could we perhaps instead have: > > SubmitCaptureResultIfComplete() { > if (partial_result.partial_metadata_received.size() < partial_result_count_ || > partial_result.buffer.is_null() || > partial_result.reference_time == base::TimeTicks()) { > SubmitCaptureResult(); > } > } > > SubmitCaptureResult() { > // the actual code to submit > } > > And call SubmitCaptureResult() directly only from here (the error case?). > > Or, perhaps it would be enough to have the conditions in > SubmitCaptureResultIfComplete() checked only if partial_result.buffer->status > was not an error instead? Done. https://codereview.chromium.org/2837273004/diff/680001/media/capture/video/ch... File media/capture/video/chromeos/video_capture_device_factory_chromeos.cc (right): https://codereview.chromium.org/2837273004/diff/680001/media/capture/video/ch... media/capture/video/chromeos/video_capture_device_factory_chromeos.cc:63: const base::FilePath kArcCamera3Service("/usr/bin/arc_camera3_service"); On 2017/06/14 09:07:44, Pawel Osciak wrote: > This could technically also be under #if defined(OS_CHROMEOS), but not a big > issue. This whole file is compiled only on chromeos build, so it doesn't really matter if we have #if defned(OS_CHROMEOS) or not.
The CQ bit was checked by jcliang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
thanks! lgtm % nit (note: I did not review unittests) https://codereview.chromium.org/2837273004/diff/700001/media/capture/video/ch... File media/capture/video/chromeos/camera_device_delegate.h (right): https://codereview.chromium.org/2837273004/diff/700001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.h:79: base::WeakPtr<CameraDeviceDelegate> GetWeakPtr(); I think you could also derive from base::SupportsWeakPtr for this (https://cs.chromium.org/chromium/src/base/memory/weak_ptr.h?l=314).
https://codereview.chromium.org/2837273004/diff/700001/media/capture/video/ch... File media/capture/video/chromeos/camera_device_delegate.h (right): https://codereview.chromium.org/2837273004/diff/700001/media/capture/video/ch... media/capture/video/chromeos/camera_device_delegate.h:79: base::WeakPtr<CameraDeviceDelegate> GetWeakPtr(); On 2017/06/15 09:27:07, Pawel Osciak wrote: > I think you could also derive from base::SupportsWeakPtr for this > (https://cs.chromium.org/chromium/src/base/memory/weak_ptr.h?l=314). Yes, but as the document suggests base::SupportsWeakPtr may lead to subtle use-after-destroy issues. I think when CameraDeviceDelegate inherits base::SupportsWeakPtr, the destruction order is CameraDeviceDelegate -> base::SupportsWeakPtr and there's a tiny window that the members of CameraDeviceDelegate are deleted while the weak ptr is still valid. So I thought it's safer to use a base::WeakPtrFactory and put the variable as the last member of the class.
The CQ bit was checked by jcliang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kenrb@chromium.org, rockot@chromium.org, chfremer@chromium.org, wuchengli@chromium.org, reveman@chromium.org Link to the patchset: https://codereview.chromium.org/2837273004/#ps700001 (title: "address posciak's comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 700001, "attempt_start_ts": 1497522251943770,
"parent_rev": "c9fb57013ff1b06ebae362c1942ff9a1868268f8", "commit_rev":
"b2b84801bfe858bfe7e568354cb3ca3f2578e78e"}
Message was sent while issue was closed.
Description was changed from ========== media: add video capture device for ARC++ camera HAL v3 This CL adds VideoCaptureDevice and VideoCaptureDeviceFactory for the ARC++ camera HAL v3. The VCD and VCD factory talk to the HAL adapter process on Chrome OS throgh Mojo IPC to access the camera service. BUG=b:32690003 TEST=Make sure camera preview works in hangout. TEST=unit tests ========== to ========== media: add video capture device for ARC++ camera HAL v3 This CL adds VideoCaptureDevice and VideoCaptureDeviceFactory for the ARC++ camera HAL v3. The VCD and VCD factory talk to the HAL adapter process on Chrome OS throgh Mojo IPC to access the camera service. BUG=b:32690003 TEST=Make sure camera preview works in hangout. TEST=unit tests Review-Url: https://codereview.chromium.org/2837273004 Cr-Commit-Position: refs/heads/master@{#479662} Committed: https://chromium.googlesource.com/chromium/src/+/b2b84801bfe858bfe7e568354cb3... ==========
Message was sent while issue was closed.
Committed patchset #36 (id:700001) as https://chromium.googlesource.com/chromium/src/+/b2b84801bfe858bfe7e568354cb3...
Message was sent while issue was closed.
A revert of this CL (patchset #36 id:700001) has been created in https://codereview.chromium.org/2936373002/ by findit-for-me@appspot.gserviceaccount.com. The reason for reverting is: Findit (https://goo.gl/kROfz5) identified CL at revision 479662 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb.... |
