WIP: media: add video capture device for ARC++ camera HAL v3 (issue 2837273004 by jcliang@chromium.org)

[jcl...@chromium.org]

Reviewers: wuchengli, henryhsu, Pawel Osciak
CL: https://codereview.chromium.org/2837273004/

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

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

Affected files (+3214, -0 lines):
M media/capture/BUILD.gn
A media/capture/video/chromeos/DEPS
A media/capture/video/chromeos/OWNERS
A media/capture/video/chromeos/camera_device_delegate.h
A media/capture/video/chromeos/camera_device_delegate.cc
A media/capture/video/chromeos/camera_hal_delegate.h
A media/capture/video/chromeos/camera_hal_delegate.cc
A media/capture/video/chromeos/camera_metadata_utils.h
A media/capture/video/chromeos/camera_metadata_utils.cc
A media/capture/video/chromeos/mojo/BUILD.gn
A media/capture/video/chromeos/mojo/OWNERS
A media/capture/video/chromeos/mojo/arc_camera3.mojom
A media/capture/video/chromeos/mojo/arc_camera3_metadata.mojom
A media/capture/video/chromeos/mojo/camera_metadata_tags.mojom
A media/capture/video/chromeos/video_capture_device_arc_chromeos.h
A media/capture/video/chromeos/video_capture_device_arc_chromeos.cc
A media/capture/video/chromeos/video_capture_device_factory_chromeos.h
A media/capture/video/chromeos/video_capture_device_factory_chromeos.cc
M media/capture/video/linux/video_capture_device_factory_linux.cc

[chfr...@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/chromeos/video_capture_device_arc_chromeos.cc
File media/capture/video/chromeos/video_capture_device_arc_chromeos.cc
(right):

https://codereview.chromium.org/2837273004/diff/1/media/capture/video/chromeos/video_capture_device_arc_chromeos.cc#newcode19
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/chromeos/video_capture_device_arc_chromeos.cc#newcode141
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/chromeos/video_capture_device_factory_chromeos.cc
File
media/capture/video/chromeos/video_capture_device_factory_chromeos.cc
(right):

https://codereview.chromium.org/2837273004/diff/1/media/capture/video/chromeos/video_capture_device_factory_chromeos.cc#newcode15
media/capture/video/chromeos/video_capture_device_factory_chromeos.cc:15:
bool VideoCaptureDeviceFactoryChromeOS::Init() {
Add DCHECK(thread_checker_.CalledOnValidThread());?

https://codereview.chromium.org/2837273004/

[jcl...@chromium.org]

https://codereview.chromium.org/2837273004/diff/1/media/capture/video/chromeos/video_capture_device_arc_chromeos.cc
File media/capture/video/chromeos/video_capture_device_arc_chromeos.cc
(right):

https://codereview.chromium.org/2837273004/diff/1/media/capture/video/chromeos/video_capture_device_arc_chromeos.cc#newcode19
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/chromeos/video_capture_device_arc_chromeos.cc#newcode141
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/chromeos/video_capture_device_factory_chromeos.cc
File
media/capture/video/chromeos/video_capture_device_factory_chromeos.cc
(right):

https://codereview.chromium.org/2837273004/diff/1/media/capture/video/chromeos/video_capture_device_factory_chromeos.cc#newcode15
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.

https://codereview.chromium.org/2837273004/

[chfr...@chromium.org]

A couple more questions/comments after a first pass of reading.


https://codereview.chromium.org/2837273004/diff/20001/media/capture/video/chromeos/camera_device_delegate.h
File media/capture/video/chromeos/camera_device_delegate.h (right):

https://codereview.chromium.org/2837273004/diff/20001/media/capture/video/chromeos/camera_device_delegate.h#newcode22
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/chromeos/camera_device_delegate.h#newcode28
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/chromeos/camera_device_delegate.h#newcode65
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/chromeos/camera_device_delegate.h#newcode141
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/chromeos/camera_device_delegate.h#newcode240
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/chromeos/camera_device_delegate.h#newcode258
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/chromeos/mojo/arc_camera3.mojom
File media/capture/video/chromeos/mojo/arc_camera3.mojom (right):

https://codereview.chromium.org/2837273004/diff/20001/media/capture/video/chromeos/mojo/arc_camera3.mojom#newcode110
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/chromeos/mojo/arc_camera3.mojom#newcode131
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/

[jcl...@chromium.org]

https://codereview.chromium.org/2837273004/diff/20001/media/capture/video/chromeos/camera_device_delegate.h
File media/capture/video/chromeos/camera_device_delegate.h (right):

https://codereview.chromium.org/2837273004/diff/20001/media/capture/video/chromeos/camera_device_delegate.h#newcode22
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/chromeos/camera_device_delegate.h#newcode28
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/chromeos/camera_device_delegate.h#newcode65
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/chromeos/camera_device_delegate.h#newcode141
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/chromeos/camera_device_delegate.h#newcode240
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/chromeos/camera_device_delegate.h#newcode258
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/chromeos/mojo/arc_camera3.mojom
File media/capture/video/chromeos/mojo/arc_camera3.mojom (right):

https://codereview.chromium.org/2837273004/diff/20001/media/capture/video/chromeos/mojo/arc_camera3.mojom#newcode110
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/chromeos/mojo/arc_camera3.mojom#newcode131
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/

[wuch...@chromium.org]

https://codereview.chromium.org/2837273004/diff/40001/media/capture/video/chromeos/OWNERS
File media/capture/video/chromeos/OWNERS (right):

https://codereview.chromium.org/2837273004/diff/40001/media/capture/video/chromeos/OWNERS#newcode1
media/capture/video/chromeos/OWNERS:1: jcl...@chromium.org
wuch...@chromium.org
add me in case you are OOO.

https://codereview.chromium.org/2837273004/diff/40001/media/capture/video/chromeos/camera_hal_delegate.h
File media/capture/video/chromeos/camera_hal_delegate.h (right):

https://codereview.chromium.org/2837273004/diff/40001/media/capture/video/chromeos/camera_hal_delegate.h#newcode51
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/chromeos/video_capture_device_arc_chromeos.cc
File media/capture/video/chromeos/video_capture_device_arc_chromeos.cc
(right):

https://codereview.chromium.org/2837273004/diff/40001/media/capture/video/chromeos/video_capture_device_arc_chromeos.cc#newcode215
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/video_capture_controller.cc?dr=CSs&l=470

https://codereview.chromium.org/2837273004/diff/40001/media/capture/video/chromeos/video_capture_device_arc_chromeos.h
File media/capture/video/chromeos/video_capture_device_arc_chromeos.h
(right):

https://codereview.chromium.org/2837273004/diff/40001/media/capture/video/chromeos/video_capture_device_arc_chromeos.h#newcode5
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/chromeos/video_capture_device_factory_chromeos.h
File
media/capture/video/chromeos/video_capture_device_factory_chromeos.h
(right):

https://codereview.chromium.org/2837273004/diff/40001/media/capture/video/chromeos/video_capture_device_factory_chromeos.h#newcode19
media/capture/video/chromeos/video_capture_device_factory_chromeos.h:19:
VideoCaptureDeviceFactoryChromeOS(
explicit

https://codereview.chromium.org/2837273004/diff/40001/media/capture/video/chromeos/video_capture_device_factory_chromeos.h#newcode34
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/chromeos/video_capture_device_factory_chromeos.h#newcode42
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/linux/video_capture_device_factory_linux.cc
File media/capture/video/linux/video_capture_device_factory_linux.cc
(right):

https://codereview.chromium.org/2837273004/diff/40001/media/capture/video/linux/video_capture_device_factory_linux.cc#newcode267
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/

[jcl...@chromium.org]

https://codereview.chromium.org/2837273004/diff/40001/media/capture/video/chromeos/OWNERS
File media/capture/video/chromeos/OWNERS (right):

https://codereview.chromium.org/2837273004/diff/40001/media/capture/video/chromeos/OWNERS#newcode1
media/capture/video/chromeos/OWNERS:1: jcl...@chromium.org

On 2017/04/28 06:29:28, wuchengli wrote:
> mailto:wuch...@chromium.org

> add me in case you are OOO.

Done.

https://codereview.chromium.org/2837273004/diff/40001/media/capture/video/chromeos/camera_hal_delegate.h
File media/capture/video/chromeos/camera_hal_delegate.h (right):

https://codereview.chromium.org/2837273004/diff/40001/media/capture/video/chromeos/camera_hal_delegate.h#newcode51
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/chromeos/video_capture_device_arc_chromeos.cc
File media/capture/video/chromeos/video_capture_device_arc_chromeos.cc
(right):

https://codereview.chromium.org/2837273004/diff/40001/media/capture/video/chromeos/video_capture_device_arc_chromeos.cc#newcode215
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/video_capture_controller.cc?dr=CSs&l=470

I believe client->OnError calls to here:
https://cs.chromium.org/chromium/src/media/capture/video/video_frame_receiver_on_task_runner.cc?dr=CSs&l=47.
So the VideoFrameReceiverOnTaskRunner would sequence the task
internally.

https://codereview.chromium.org/2837273004/diff/40001/media/capture/video/chromeos/video_capture_device_arc_chromeos.h
File media/capture/video/chromeos/video_capture_device_arc_chromeos.h
(right):

https://codereview.chromium.org/2837273004/diff/40001/media/capture/video/chromeos/video_capture_device_arc_chromeos.h#newcode5
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/chromeos/video_capture_device_factory_chromeos.h
File
media/capture/video/chromeos/video_capture_device_factory_chromeos.h
(right):

https://codereview.chromium.org/2837273004/diff/40001/media/capture/video/chromeos/video_capture_device_factory_chromeos.h#newcode19
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/chromeos/video_capture_device_factory_chromeos.h#newcode34
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_factory.h?q=video_capture_device_factory.h&dr=CSs&l=16,
so it's probably outdated.

https://codereview.chromium.org/2837273004/diff/40001/media/capture/video/chromeos/video_capture_device_factory_chromeos.h#newcode42
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/linux/video_capture_device_factory_linux.cc
File media/capture/video/linux/video_capture_device_factory_linux.cc
(right):

https://codereview.chromium.org/2837273004/diff/40001/media/capture/video/linux/video_capture_device_factory_linux.cc#newcode267
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=BUILDCONFIG.gn&dr&l=415

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/

[chfr...@chromium.org]

https://codereview.chromium.org/2837273004/diff/40001/media/capture/video/chromeos/video_capture_device_factory_chromeos.h
File
media/capture/video/chromeos/video_capture_device_factory_chromeos.h
(right):

https://codereview.chromium.org/2837273004/diff/40001/media/capture/video/chromeos/video_capture_device_factory_chromeos.h#newcode34
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_factory.h?q=video_capture_device_factory.h&dr=CSs&l=16,
> 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/

[jcl...@chromium.org]

https://codereview.chromium.org/2837273004/diff/40001/media/capture/video/chromeos/video_capture_device_factory_chromeos.h
File
media/capture/video/chromeos/video_capture_device_factory_chromeos.h
(right):

https://codereview.chromium.org/2837273004/diff/40001/media/capture/video/chromeos/video_capture_device_factory_chromeos.h#newcode34
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_factory.h?q=video_capture_device_factory.h&dr=CSs&l=16,
> > 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

https://codereview.chromium.org/2837273004/

[ke...@chromium.org]

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/chromeos/camera_device_delegate.cc
File media/capture/video/chromeos/camera_device_delegate.cc (right):

https://codereview.chromium.org/2837273004/diff/80001/media/capture/video/chromeos/camera_device_delegate.cc#newcode482
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/

[jcl...@chromium.org]

https://codereview.chromium.org/2837273004/diff/80001/media/capture/video/chromeos/camera_device_delegate.cc
File media/capture/video/chromeos/camera_device_delegate.cc (right):

https://codereview.chromium.org/2837273004/diff/80001/media/capture/video/chromeos/camera_device_delegate.cc#newcode482
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.

https://codereview.chromium.org/2837273004/

[wuch...@chromium.org]

I'm still reviewing camera_hal_delegate.cc.


https://codereview.chromium.org/2837273004/diff/80001/media/capture/video/chromeos/camera_hal_delegate.cc
File media/capture/video/chromeos/camera_hal_delegate.cc (right):

https://codereview.chromium.org/2837273004/diff/80001/media/capture/video/chromeos/camera_hal_delegate.cc#newcode41
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/chromeos/camera_hal_delegate.cc#newcode45
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/chromeos/camera_hal_delegate.cc#newcode56
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/chromeos/camera_hal_delegate.cc#newcode64
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/chromeos/camera_hal_delegate.cc#newcode127
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/chromeos/camera_hal_delegate.cc#newcode128
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/chromeos/camera_hal_delegate.cc#newcode145
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/chromeos/camera_hal_delegate.cc#newcode194
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/chromeos/camera_hal_delegate.cc#newcode270
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/chromeos/camera_hal_delegate.cc#newcode306
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/chromeos/camera_hal_delegate.cc#newcode309
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/chromeos/camera_hal_delegate.cc#newcode310
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/chromeos/camera_hal_delegate.cc#newcode313
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/chromeos/camera_hal_delegate.cc#newcode315
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/chromeos/camera_hal_delegate.cc#newcode333
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/chromeos/camera_hal_delegate.cc#newcode335
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/chromeos/camera_hal_delegate.cc#newcode356
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/chromeos/camera_hal_delegate.cc#newcode364
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/chromeos/camera_hal_delegate.cc#newcode366
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/

[jcl...@chromium.org]

https://codereview.chromium.org/2837273004/diff/80001/media/capture/video/chromeos/camera_hal_delegate.cc
File media/capture/video/chromeos/camera_hal_delegate.cc (right):

https://codereview.chromium.org/2837273004/diff/80001/media/capture/video/chromeos/camera_hal_delegate.cc#newcode64
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/chromeos/camera_hal_delegate.cc#newcode127
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/chromeos/camera_hal_delegate.cc#newcode128
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/chromeos/camera_hal_delegate.cc#newcode145
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/chromeos/camera_hal_delegate.cc#newcode194
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/chromeos/camera_hal_delegate.cc#newcode270
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/chromeos/camera_hal_delegate.cc#newcode306
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/chromeos/camera_hal_delegate.cc#newcode309
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/include/hardware/camera_common.h#722

If we get here then it means the camera device is at the ENUMERATING
state:
https://chromium.googlesource.com/aosp/platform/hardware/libhardware/+/master/include/hardware/camera_common.h#433

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/chromeos/camera_hal_delegate.cc#newcode310
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/chromeos/camera_hal_delegate.cc#newcode313
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/chromeos/camera_hal_delegate.cc#newcode315
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/chromeos/camera_hal_delegate.cc#newcode333
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/chromeos/camera_hal_delegate.cc#newcode335
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/chromeos/camera_hal_delegate.cc#newcode356
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/chromeos/camera_hal_delegate.cc#newcode366
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.

https://codereview.chromium.org/2837273004/

[wuch...@chromium.org]

Finished camera_hal_delegate.cc.


https://codereview.chromium.org/2837273004/diff/120001/media/capture/video/chromeos/camera_hal_delegate.cc
File media/capture/video/chromeos/camera_hal_delegate.cc (right):

https://codereview.chromium.org/2837273004/diff/120001/media/capture/video/chromeos/camera_hal_delegate.cc#newcode42
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/chromeos/camera_hal_delegate.cc#newcode78
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/chromeos/camera_hal_delegate.cc#newcode94
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/chromeos/camera_hal_delegate.cc#newcode136
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/StreamConfigurationMap.html#getOutputMinFrameDuration(java.lang.Class<T&gt;,
android.util.Size)

https://codereview.chromium.org/2837273004/diff/120001/media/capture/video/chromeos/camera_hal_delegate.cc#newcode174
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/chromeos/camera_hal_delegate.cc#newcode183
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_device_factory_linux.cc?l=90

https://codereview.chromium.org/2837273004/diff/120001/media/capture/video/chromeos/camera_hal_delegate.cc#newcode202
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/chromeos/camera_hal_delegate.cc#newcode207
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/webui/settings/settings_media_devices_selection_handler.cc

https://codereview.chromium.org/2837273004/diff/120001/media/capture/video/chromeos/camera_hal_delegate.cc#newcode216
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/chromeos/camera_hal_delegate.cc#newcode257
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/chromeos/camera_hal_delegate.cc#newcode277
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/chromeos/camera_hal_delegate.cc#newcode278
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/chromeos/camera_hal_delegate.cc#newcode292
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/chromeos/camera_hal_delegate.cc#newcode313
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/chromeos/camera_hal_delegate.cc#newcode317
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.

https://codereview.chromium.org/2837273004/

[wuch...@chromium.org]

Finished the review of video_capture_device_arc_chromeos.h/.cc.


https://codereview.chromium.org/2837273004/diff/140001/media/capture/video/chromeos/video_capture_device_arc_chromeos.cc
File media/capture/video/chromeos/video_capture_device_arc_chromeos.cc
(right):

https://codereview.chromium.org/2837273004/diff/140001/media/capture/video/chromeos/video_capture_device_arc_chromeos.cc#newcode29
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/chromeos/video_capture_device_arc_chromeos.cc#newcode133
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/chromeos/video_capture_device_arc_chromeos.h
File media/capture/video/chromeos/video_capture_device_arc_chromeos.h
(right):

https://codereview.chromium.org/2837273004/diff/140001/media/capture/video/chromeos/video_capture_device_arc_chromeos.h#newcode45
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/

[jcl...@chromium.org]

https://codereview.chromium.org/2837273004/diff/120001/media/capture/video/chromeos/camera_hal_delegate.cc
File media/capture/video/chromeos/camera_hal_delegate.cc (right):

https://codereview.chromium.org/2837273004/diff/120001/media/capture/video/chromeos/camera_hal_delegate.cc#newcode42
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/chromeos/camera_hal_delegate.cc#newcode78
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/chromeos/camera_hal_delegate.cc#newcode94
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/chromeos/camera_hal_delegate.cc#newcode174
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/chromeos/camera_hal_delegate.cc#newcode202
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/chromeos/camera_hal_delegate.cc#newcode216
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/chromeos/camera_hal_delegate.cc#newcode257
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/chromeos/camera_hal_delegate.cc#newcode277
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/chromeos/camera_hal_delegate.cc#newcode278
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/chromeos/camera_hal_delegate.cc#newcode292
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/chromeos/camera_hal_delegate.cc#newcode313
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/chromeos/camera_hal_delegate.cc#newcode317
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/chromeos/video_capture_device_arc_chromeos.cc
File media/capture/video/chromeos/video_capture_device_arc_chromeos.cc
(right):

https://codereview.chromium.org/2837273004/diff/140001/media/capture/video/chromeos/video_capture_device_arc_chromeos.cc#newcode29
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/chromeos/video_capture_device_arc_chromeos.cc#newcode133
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.

https://codereview.chromium.org/2837273004/

[wuch...@chromium.org]

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/chromeos/camera_device_delegate.cc
File media/capture/video/chromeos/camera_device_delegate.cc (right):

https://codereview.chromium.org/2837273004/diff/160001/media/capture/video/chromeos/camera_device_delegate.cc#newcode22
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/chromeos/camera_device_delegate.cc#newcode97
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/chromeos/camera_device_delegate.cc#newcode128
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/chromeos/camera_device_delegate.cc#newcode130
media/capture/video/chromeos/camera_device_delegate.cc:130: // unbound.
Signal |closed|

https://codereview.chromium.org/2837273004/diff/160001/media/capture/video/chromeos/camera_device_delegate.cc#newcode186
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/chromeos/camera_device_delegate.cc#newcode494
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/chromeos/camera_device_delegate.cc#newcode504
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/chromeos/camera_device_delegate.cc#newcode590
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/chromeos/camera_device_delegate.h
File media/capture/video/chromeos/camera_device_delegate.h (right):

https://codereview.chromium.org/2837273004/diff/160001/media/capture/video/chromeos/camera_device_delegate.h#newcode24
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/chromeos/camera_device_delegate.h#newcode117
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/chromeos/camera_device_delegate.h#newcode186
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/chromeos/camera_device_delegate.h#newcode234
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/chromeos/camera_device_delegate.h#newcode249
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/chromeos/camera_device_delegate.h#newcode269
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/chromeos/camera_device_delegate.h#newcode277
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/chromeos/camera_device_delegate.h#newcode293
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/chromeos/video_capture_device_factory_chromeos.cc
File
media/capture/video/chromeos/video_capture_device_factory_chromeos.cc
(right):

https://codereview.chromium.org/2837273004/diff/160001/media/capture/video/chromeos/video_capture_device_factory_chromeos.cc#newcode45
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/

[jcl...@chromium.org]

https://codereview.chromium.org/2837273004/diff/160001/media/capture/video/chromeos/camera_device_delegate.cc
File media/capture/video/chromeos/camera_device_delegate.cc (right):

https://codereview.chromium.org/2837273004/diff/160001/media/capture/video/chromeos/camera_device_delegate.cc#newcode22
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/chromeos/camera_device_delegate.cc#newcode97
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/chromeos/camera_device_delegate.cc#newcode128
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/chromeos/camera_device_delegate.cc#newcode130
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/chromeos/camera_device_delegate.cc#newcode186
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/chromeos/camera_device_delegate.cc#newcode494
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/chromeos/camera_device_delegate.cc#newcode504
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/chromeos/camera_device_delegate.cc#newcode590
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/chromeos/camera_device_delegate.h
File media/capture/video/chromeos/camera_device_delegate.h (right):

https://codereview.chromium.org/2837273004/diff/160001/media/capture/video/chromeos/camera_device_delegate.h#newcode24
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/chromeos/camera_device_delegate.h#newcode117
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/chromeos/camera_device_delegate.h#newcode186
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/chromeos/camera_device_delegate.h#newcode234
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/chromeos/camera_device_delegate.h#newcode249
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/chromeos/camera_device_delegate.h#newcode269
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/chromeos/camera_device_delegate.h#newcode277
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/chromeos/camera_device_delegate.h#newcode293
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/chromeos/video_capture_device_factory_chromeos.cc
File
media/capture/video/chromeos/video_capture_device_factory_chromeos.cc
(right):

https://codereview.chromium.org/2837273004/diff/160001/media/capture/video/chromeos/video_capture_device_factory_chromeos.cc#newcode45
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.

https://codereview.chromium.org/2837273004/

[wuch...@chromium.org]

Finished camera_device_delegate.cc.


https://codereview.chromium.org/2837273004/diff/260001/media/capture/video/chromeos/camera_device_delegate.cc
File media/capture/video/chromeos/camera_device_delegate.cc (right):

https://codereview.chromium.org/2837273004/diff/260001/media/capture/video/chromeos/camera_device_delegate.cc#newcode220
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/chromeos/camera_device_delegate.cc#newcode256
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/chromeos/camera_device_delegate.cc#newcode282
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/chromeos/camera_device_delegate.cc#newcode309
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/chromeos/camera_device_delegate.cc#newcode345
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/chromeos/camera_device_delegate.cc#newcode346
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/chromeos/camera_device_delegate.cc#newcode379
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/chromeos/camera_device_delegate.cc#newcode401
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/chromeos/camera_device_delegate.cc#newcode406
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/chromeos/camera_device_delegate.cc#newcode419
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/chromeos/camera_device_delegate.cc#newcode428
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/chromeos/camera_device_delegate.cc#newcode434
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/chromeos/camera_device_delegate.cc#newcode457
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/chromeos/camera_device_delegate.cc#newcode484
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/chromeos/camera_device_delegate.cc#newcode485
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/chromeos/camera_device_delegate.cc#newcode494
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/chromeos/camera_device_delegate.cc#newcode505
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/chromeos/camera_device_delegate.cc#newcode509
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/chromeos/camera_device_delegate.cc#newcode538
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/chromeos/camera_device_delegate.cc#newcode576
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/chromeos/camera_device_delegate.cc#newcode591
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/chromeos/camera_device_delegate.cc#newcode641
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/

[jcl...@chromium.org]

https://codereview.chromium.org/2837273004/diff/260001/media/capture/video/chromeos/camera_device_delegate.cc
File media/capture/video/chromeos/camera_device_delegate.cc (right):

https://codereview.chromium.org/2837273004/diff/260001/media/capture/video/chromeos/camera_device_delegate.cc#newcode220
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/chromeos/camera_device_delegate.cc#newcode256
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/chromeos/camera_device_delegate.cc#newcode282
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/chromeos/camera_device_delegate.cc#newcode309
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/chromeos/camera_device_delegate.cc#newcode345
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/chromeos/camera_device_delegate.cc#newcode346
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/chromeos/camera_device_delegate.cc#newcode379
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/chromeos/camera_device_delegate.cc#newcode401
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/chromeos/camera_device_delegate.cc#newcode406
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/chromeos/camera_device_delegate.cc#newcode419
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/chromeos/camera_device_delegate.cc#newcode428
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/chromeos/camera_device_delegate.cc#newcode434
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/chromeos/camera_device_delegate.cc#newcode457
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/chromeos/camera_device_delegate.cc#newcode484
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/chromeos/camera_device_delegate.cc#newcode485
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/chromeos/camera_device_delegate.cc#newcode494
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/chromeos/camera_device_delegate.cc#newcode505
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/chromeos/camera_device_delegate.cc#newcode509
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/chromeos/camera_device_delegate.cc#newcode538
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/chromeos/camera_device_delegate.cc#newcode576
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/chromeos/camera_device_delegate.cc#newcode591
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/chromeos/camera_device_delegate.cc#newcode641
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.

https://codereview.chromium.org/2837273004/

[wuch...@chromium.org]

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.

https://codereview.chromium.org/2837273004/

[wuch...@chromium.org]

Finished camera_metadata_utils.h/.cc.


https://codereview.chromium.org/2837273004/diff/280001/media/capture/video/chromeos/camera_metadata_utils.cc
File media/capture/video/chromeos/camera_metadata_utils.cc (right):

https://codereview.chromium.org/2837273004/diff/280001/media/capture/video/chromeos/camera_metadata_utils.cc#newcode26
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/chromeos/camera_metadata_utils.cc#newcode32
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/chromeos/camera_metadata_utils.cc#newcode43
media/capture/video/chromeos/camera_metadata_utils.cc:43: for (const
auto& e : from->entries.value()) {
s/e/entry/

https://codereview.chromium.org/2837273004/

[wuch...@chromium.org]

Finished another round review of .gn, DEPS, and OWNERS.


https://codereview.chromium.org/2837273004/diff/280001/media/capture/video/chromeos/OWNERS
File media/capture/video/chromeos/OWNERS (right):

https://codereview.chromium.org/2837273004/diff/280001/media/capture/video/chromeos/OWNERS#newcode2
media/capture/video/chromeos/OWNERS:2: wuch...@chromium.org
Add pos...@chromium.org

https://codereview.chromium.org/2837273004/

[wuch...@chromium.org]

Finished another round of review for *.mojom.


https://codereview.chromium.org/2837273004/diff/280001/media/capture/video/chromeos/mojo/arc_camera3.mojom
File media/capture/video/chromeos/mojo/arc_camera3.mojom (right):

https://codereview.chromium.org/2837273004/diff/280001/media/capture/video/chromeos/mojo/arc_camera3.mojom#newcode9
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/chromeos/mojo/arc_camera3.mojom#newcode45
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/chromeos/mojo/arc_camera3.mojom#newcode52
media/capture/video/chromeos/mojo/arc_camera3.mojom:52: [Extensible]
remove because we don't plan to add any vendor template.

https://codereview.chromium.org/2837273004/

[wuch...@chromium.org]

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/chromeos/video_capture_device_arc_chromeos.cc
File media/capture/video/chromeos/video_capture_device_arc_chromeos.cc
(right):

https://codereview.chromium.org/2837273004/diff/280001/media/capture/video/chromeos/video_capture_device_arc_chromeos.cc#newcode147
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/chromeos/video_capture_device_arc_chromeos.cc#newcode152
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/chromeos/video_capture_device_arc_chromeos.cc#newcode153
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/chromeos/video_capture_device_arc_chromeos.cc#newcode164
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/chromeos/video_capture_device_arc_chromeos.cc#newcode168
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/chromeos/video_capture_device_arc_chromeos.cc#newcode204
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/chromeos/video_capture_device_arc_chromeos.cc#newcode205
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/chromeos/video_capture_device_arc_chromeos.cc#newcode226
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/chromeos/video_capture_device_arc_chromeos.h
File media/capture/video/chromeos/video_capture_device_arc_chromeos.h
(right):

https://codereview.chromium.org/2837273004/diff/280001/media/capture/video/chromeos/video_capture_device_arc_chromeos.h#newcode88
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/chromeos/video_capture_device_factory_chromeos.h
File
media/capture/video/chromeos/video_capture_device_factory_chromeos.h
(right):

https://codereview.chromium.org/2837273004/diff/280001/media/capture/video/chromeos/video_capture_device_factory_chromeos.h#newcode34

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/

https://codereview.chromium.org/2837273004/

[wuch...@chromium.org]

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.

https://codereview.chromium.org/2837273004/

[ke...@chromium.org]

mojom lgtm

https://codereview.chromium.org/2837273004/

[chfr...@chromium.org]

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.

[jcl...@chromium.org]

https://codereview.chromium.org/2837273004/diff/280001/media/capture/video/chromeos/OWNERS
File media/capture/video/chromeos/OWNERS (right):

https://codereview.chromium.org/2837273004/diff/280001/media/capture/video/chromeos/OWNERS#newcode2
media/capture/video/chromeos/OWNERS:2: wuch...@chromium.org

On 2017/05/18 02:52:40, wuchengli wrote:
> Add mailto:pos...@chromium.org

Done.

https://codereview.chromium.org/2837273004/diff/280001/media/capture/video/chromeos/camera_metadata_utils.cc
File media/capture/video/chromeos/camera_metadata_utils.cc (right):

https://codereview.chromium.org/2837273004/diff/280001/media/capture/video/chromeos/camera_metadata_utils.cc#newcode26
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/chromeos/camera_metadata_utils.cc#newcode32
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/chromeos/camera_metadata_utils.cc#newcode43
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/chromeos/mojo/arc_camera3.mojom
File media/capture/video/chromeos/mojo/arc_camera3.mojom (right):

https://codereview.chromium.org/2837273004/diff/280001/media/capture/video/chromeos/mojo/arc_camera3.mojom#newcode45
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/chromeos/mojo/arc_camera3.mojom#newcode52
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/chromeos/video_capture_device_arc_chromeos.cc
File media/capture/video/chromeos/video_capture_device_arc_chromeos.cc
(right):

https://codereview.chromium.org/2837273004/diff/280001/media/capture/video/chromeos/video_capture_device_arc_chromeos.cc#newcode147
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/chromeos/video_capture_device_arc_chromeos.cc#newcode152
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/chromeos/video_capture_device_arc_chromeos.cc#newcode204
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/chromeos/video_capture_device_arc_chromeos.cc#newcode205
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/chromeos/video_capture_device_arc_chromeos.cc#newcode226
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/chromeos/video_capture_device_arc_chromeos.h
File media/capture/video/chromeos/video_capture_device_arc_chromeos.h
(right):

https://codereview.chromium.org/2837273004/diff/280001/media/capture/video/chromeos/video_capture_device_arc_chromeos.h#newcode88
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/chromeos/video_capture_device_factory_chromeos.h
File
media/capture/video/chromeos/video_capture_device_factory_chromeos.h
(right):

https://codereview.chromium.org/2837273004/diff/280001/media/capture/video/chromeos/video_capture_device_factory_chromeos.h#newcode34
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.

https://codereview.chromium.org/2837273004/

[wuch...@chromium.org]

Reviewed another round of video_capture_device_arc_chromeos.h/cc.


https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/chromeos/video_capture_device_arc_chromeos.cc
File media/capture/video/chromeos/video_capture_device_arc_chromeos.cc
(right):

https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/chromeos/video_capture_device_arc_chromeos.cc#newcode155
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/chromeos/video_capture_device_arc_chromeos.cc#newcode164
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.

https://codereview.chromium.org/2837273004/

[wuch...@chromium.org]

Doing another round for CameraDeviceDelegate. Just finished review
ConfigureStreams.


https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/chromeos/camera_device_delegate.cc
File media/capture/video/chromeos/camera_device_delegate.cc (right):

https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/chromeos/camera_device_delegate.cc#newcode40
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/chromeos/camera_device_delegate.cc#newcode104
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/chromeos/camera_device_delegate.cc#newcode260
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/chromeos/camera_device_delegate.cc#newcode348
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/chromeos/camera_device_delegate.cc#newcode570
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/chromeos/camera_device_delegate.cc#newcode631
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/chromeos/camera_device_delegate.h
File media/capture/video/chromeos/camera_device_delegate.h (right):

https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/chromeos/camera_device_delegate.h#newcode27
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/chromeos/camera_device_delegate.h#newcode39
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/chromeos/camera_hal_delegate.h
File media/capture/video/chromeos/camera_hal_delegate.h (right):

https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/chromeos/camera_hal_delegate.h#newcode20
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.

https://codereview.chromium.org/2837273004/

[chfr...@chromium.org]

Some comments and questions from a pass of reading the *video_capture_device*
files.

https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/chromeos/video_capture_device_arc_chromeos.cc
File media/capture/video/chromeos/video_capture_device_arc_chromeos.cc
(right):

https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/chromeos/video_capture_device_arc_chromeos.cc#newcode17
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/chromeos/video_capture_device_arc_chromeos.cc#newcode25
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/chromeos/video_capture_device_arc_chromeos.cc#newcode62
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/chromeos/video_capture_device_arc_chromeos.cc#newcode123
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/chromeos/video_capture_device_arc_chromeos.cc#newcode164
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/chromeos/video_capture_device_arc_chromeos.cc#newcode242
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/chromeos/video_capture_device_arc_chromeos.h
File media/capture/video/chromeos/video_capture_device_arc_chromeos.h
(right):

https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/chromeos/video_capture_device_arc_chromeos.h#newcode68
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/chromeos/video_capture_device_arc_chromeos.h#newcode75
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/chromeos/video_capture_device_arc_chromeos.h#newcode79
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/chromeos/video_capture_device_arc_chromeos.h#newcode88
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/chromeos/video_capture_device_factory_chromeos.cc
File
media/capture/video/chromeos/video_capture_device_factory_chromeos.cc
(right):

https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/chromeos/video_capture_device_factory_chromeos.cc#newcode30
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/chromeos/video_capture_device_factory_chromeos.cc#newcode31
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/chromeos/video_capture_device_factory_chromeos.cc#newcode56
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/chromeos/video_capture_device_factory_chromeos.h
File
media/capture/video/chromeos/video_capture_device_factory_chromeos.h
(right):

https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/chromeos/video_capture_device_factory_chromeos.h#newcode20
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/chromeos/video_capture_device_factory_chromeos.h#newcode35
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/chromeos/video_capture_device_factory_chromeos.h#newcode48
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/video_capture_device_unittest.cc
File media/capture/video/video_capture_device_unittest.cc (right):

https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/video_capture_device_unittest.cc#newcode223
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?

https://codereview.chromium.org/2837273004/

[wuch...@chromium.org]

Finished another round of camera_device_delegate.cc.

https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/chromeos/camera_device_delegate.cc
File media/capture/video/chromeos/camera_device_delegate.cc (right):

https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/chromeos/camera_device_delegate.cc#newcode370
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/chromeos/camera_device_delegate.cc#newcode438
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/chromeos/camera_device_delegate.cc#newcode574
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/chromeos/camera_device_delegate.cc#newcode632
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/chromeos/camera_device_delegate.cc#newcode707
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/chromeos/camera_device_delegate.cc#newcode730
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/chromeos/video_capture_device_arc_chromeos.cc
File media/capture/video/chromeos/video_capture_device_arc_chromeos.cc
(right):

https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/chromeos/video_capture_device_arc_chromeos.cc#newcode164
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.

https://codereview.chromium.org/2837273004/

[wuch...@chromium.org]

[wuch...@chromium.org]

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.


https://codereview.chromium.org/2837273004/

[wuch...@chromium.org]

Finished the second round of review for camera_hal_delegate.h/.cc.


https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/chromeos/camera_hal_delegate.cc
File media/capture/video/chromeos/camera_hal_delegate.cc (right):

https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/chromeos/camera_hal_delegate.cc#newcode143
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/chromeos/camera_hal_delegate.cc#newcode150
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/chromeos/camera_hal_delegate.cc#newcode235
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/chromeos/camera_hal_delegate.cc#newcode244
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/chromeos/camera_hal_delegate.cc#newcode336
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/chromeos/camera_hal_delegate.cc#newcode343
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/chromeos/camera_hal_delegate.h
File media/capture/video/chromeos/camera_hal_delegate.h (right):

https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/chromeos/camera_hal_delegate.h#newcode30
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/

[wuch...@chromium.org]

https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/chromeos/camera_hal_delegate.cc
File media/capture/video/chromeos/camera_hal_delegate.cc (right):

https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/chromeos/camera_hal_delegate.cc#newcode229
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/linux/video_capture_device_factory_linux.cc

https://codereview.chromium.org/2837273004/

[jcl...@chromium.org]

https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/chromeos/camera_device_delegate.cc
File media/capture/video/chromeos/camera_device_delegate.cc (right):

https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/chromeos/camera_device_delegate.cc#newcode40
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/chromeos/camera_device_delegate.cc#newcode104
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/chromeos/camera_device_delegate.cc#newcode260
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/chromeos/camera_device_delegate.cc#newcode348
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/chromeos/camera_device_delegate.cc#newcode370
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/chromeos/camera_device_delegate.cc#newcode438
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/chromeos/camera_device_delegate.cc#newcode570
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/chromeos/camera_device_delegate.cc#newcode574
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/chromeos/camera_device_delegate.cc#newcode631
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/chromeos/camera_device_delegate.cc#newcode632
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/chromeos/camera_device_delegate.cc#newcode707
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/chromeos/camera_device_delegate.cc#newcode730
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/chromeos/camera_device_delegate.h
File media/capture/video/chromeos/camera_device_delegate.h (right):

https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/chromeos/camera_device_delegate.h#newcode27
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/chromeos/camera_device_delegate.h#newcode39
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/chromeos/camera_hal_delegate.cc
File media/capture/video/chromeos/camera_hal_delegate.cc (right):

https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/chromeos/camera_hal_delegate.cc#newcode143
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/chromeos/camera_hal_delegate.cc#newcode229
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/linux/video_capture_device_factory_linux.cc

Done.

https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/chromeos/camera_hal_delegate.cc#newcode235
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/chromeos/camera_hal_delegate.cc#newcode244
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/chromeos/camera_hal_delegate.cc#newcode336
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/chromeos/camera_hal_delegate.cc#newcode343
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/chromeos/camera_hal_delegate.h
File media/capture/video/chromeos/camera_hal_delegate.h (right):

https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/chromeos/camera_hal_delegate.h#newcode20
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/chromeos/camera_hal_delegate.h#newcode30
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/chromeos/video_capture_device_arc_chromeos.cc
File media/capture/video/chromeos/video_capture_device_arc_chromeos.cc
(right):

https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/chromeos/video_capture_device_arc_chromeos.cc#newcode17
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_device_chromeos.cc?dr=Ss&l=27

https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/chromeos/video_capture_device_arc_chromeos.cc#newcode25
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/chromeos/video_capture_device_arc_chromeos.cc#newcode62
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/chromeos/video_capture_device_arc_chromeos.cc#newcode123
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/chromeos/video_capture_device_arc_chromeos.cc#newcode155
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/chromeos/video_capture_device_arc_chromeos.cc#newcode164
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/chromeos/video_capture_device_arc_chromeos.cc#newcode242
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/chromeos/video_capture_device_arc_chromeos.h
File media/capture/video/chromeos/video_capture_device_arc_chromeos.h
(right):

https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/chromeos/video_capture_device_arc_chromeos.h#newcode68
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/chromeos/video_capture_device_arc_chromeos.h#newcode75
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/chromeos/video_capture_device_arc_chromeos.h#newcode79
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/chromeos/video_capture_device_arc_chromeos.h#newcode88
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/chromeos/video_capture_device_factory_chromeos.cc
File
media/capture/video/chromeos/video_capture_device_factory_chromeos.cc
(right):

https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/chromeos/video_capture_device_factory_chromeos.cc#newcode30
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/chromeos/video_capture_device_factory_chromeos.cc#newcode31
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/chromeos/video_capture_device_factory_chromeos.cc#newcode56
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/chromeos/video_capture_device_factory_chromeos.h
File
media/capture/video/chromeos/video_capture_device_factory_chromeos.h
(right):

https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/chromeos/video_capture_device_factory_chromeos.h#newcode20
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/chromeos/video_capture_device_factory_chromeos.h#newcode35
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/chromeos/video_capture_device_factory_chromeos.h#newcode48
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/video_capture_device_unittest.cc
File media/capture/video/video_capture_device_unittest.cc (right):

https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/video_capture_device_unittest.cc#newcode223
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?

https://codereview.chromium.org/2837273004/

[chfr...@chromium.org]

Some replies. No new code reviewed.

https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/chromeos/video_capture_device_arc_chromeos.cc
File media/capture/video/chromeos/video_capture_device_arc_chromeos.cc
(right):

https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/chromeos/video_capture_device_arc_chromeos.cc#newcode17
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_device_chromeos.cc?dr=Ss&l=27

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/chromeos/video_capture_device_arc_chromeos.cc#newcode62
media/capture/video/chromeos/video_capture_device_arc_chromeos.cc:62:
display::Screen* screen = display::Screen::GetScreen();

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/chromeos/video_capture_device_arc_chromeos.h
File media/capture/video/chromeos/video_capture_device_arc_chromeos.h
(right):

https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/chromeos/video_capture_device_arc_chromeos.h#newcode75
media/capture/video/chromeos/video_capture_device_arc_chromeos.h:75:
base::Thread device_thread_;

Thanks. That sounds reasonable.

https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/chromeos/video_capture_device_factory_chromeos.cc
File
media/capture/video/chromeos/video_capture_device_factory_chromeos.cc
(right):

https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/chromeos/video_capture_device_factory_chromeos.cc#newcode30
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/chromeos/video_capture_device_factory_chromeos.h
File
media/capture/video/chromeos/video_capture_device_factory_chromeos.h
(right):

https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/chromeos/video_capture_device_factory_chromeos.h#newcode35
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/video_capture_device_unittest.cc
File media/capture/video/video_capture_device_unittest.cc (right):

https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/video_capture_device_unittest.cc#newcode223
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/

[jcl...@chromium.org]

https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/chromeos/video_capture_device_arc_chromeos.cc
File media/capture/video/chromeos/video_capture_device_arc_chromeos.cc
(right):

https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/chromeos/video_capture_device_arc_chromeos.cc#newcode17
media/capture/video/chromeos/video_capture_device_arc_chromeos.cc:17: //
thread to the media thread.

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/chromeos/video_capture_device_factory_chromeos.h
File
media/capture/video/chromeos/video_capture_device_factory_chromeos.h
(right):

https://codereview.chromium.org/2837273004/diff/300001/media/capture/video/chromeos/video_capture_device_factory_chromeos.h#newcode35
media/capture/video/chromeos/video_capture_device_factory_chromeos.h:35:
// instance is created.

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.

https://codereview.chromium.org/2837273004/

[chfr...@chromium.org]

Comments and questions from a pass of looking at the camera_hal_delegate* files.


https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/chromeos/camera_hal_delegate.cc
File media/capture/video/chromeos/camera_hal_delegate.cc (right):

https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/chromeos/camera_hal_delegate.cc#newcode44
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/chromeos/camera_hal_delegate.cc#newcode113
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/chromeos/camera_hal_delegate.h
File media/capture/video/chromeos/camera_hal_delegate.h (right):

https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/chromeos/camera_hal_delegate.h#newcode24
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/chromeos/camera_hal_delegate.h#newcode35
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/chromeos/camera_hal_delegate.h#newcode39
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/chromeos/camera_hal_delegate.h#newcode45
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/chromeos/camera_hal_delegate.h#newcode55
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/chromeos/camera_hal_delegate.h#newcode74
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/chromeos/camera_hal_delegate_unittest.cc
File media/capture/video/chromeos/camera_hal_delegate_unittest.cc
(right):

https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/chromeos/camera_hal_delegate_unittest.cc#newcode42
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/chromeos/camera_hal_delegate_unittest.cc#newcode47
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/chromeos/camera_hal_delegate_unittest.cc#newcode57
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-6VLykYXyl3Pkk/edit

https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/chromeos/camera_hal_delegate_unittest.cc#newcode67
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/chromeos/camera_hal_delegate_unittest.cc#newcode117
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/chromeos/camera_hal_delegate_unittest.cc#newcode129
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/chromeos/camera_hal_delegate_unittest.h
File media/capture/video/chromeos/camera_hal_delegate_unittest.h
(right):

https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/chromeos/camera_hal_delegate_unittest.h#newcode35
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/chromeos/camera_hal_delegate_unittest.h#newcode63
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/chromeos/mojo/arc_camera3.mojom
File media/capture/video/chromeos/mojo/arc_camera3.mojom (right):

https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/chromeos/mojo/arc_camera3.mojom#newcode260
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/chromeos/mojo/arc_camera3.mojom#newcode288
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/device_factory.mojom?dr.

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/chromeos/mojo/arc_camera3.mojom#newcode297
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?

https://codereview.chromium.org/2837273004/

[chfr...@chromium.org]

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

[jcl...@chromium.org]

https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/chromeos/camera_hal_delegate.cc
File media/capture/video/chromeos/camera_hal_delegate.cc (right):

https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/chromeos/camera_hal_delegate.cc#newcode44
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/chromeos/camera_hal_delegate.cc#newcode113
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_session.cc&dr&l=433

https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/chromeos/camera_hal_delegate.h
File media/capture/video/chromeos/camera_hal_delegate.h (right):

https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/chromeos/camera_hal_delegate.h#newcode24
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/chromeos/camera_hal_delegate.h#newcode35
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/chromeos/camera_hal_delegate.h#newcode39
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/chromeos/camera_hal_delegate.h#newcode45
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/chromeos/camera_hal_delegate.h#newcode55
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/chromeos/camera_hal_delegate.h#newcode74
media/capture/video/chromeos/camera_hal_delegate.h:74: void
StartForTesting(arc::mojom::CameraModulePtrInfo info);

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/chromeos/camera_hal_delegate_unittest.cc
File media/capture/video/chromeos/camera_hal_delegate_unittest.cc
(right):

https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/chromeos/camera_hal_delegate_unittest.cc#newcode42
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/chromeos/camera_hal_delegate_unittest.cc#newcode47
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/chromeos/camera_hal_delegate_unittest.cc#newcode57
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-6VLykYXyl3Pkk/edit

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/chromeos/camera_hal_delegate_unittest.cc#newcode67
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/chromeos/camera_hal_delegate_unittest.cc#newcode117
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/chromeos/camera_hal_delegate_unittest.cc#newcode129
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/chromeos/camera_hal_delegate_unittest.h
File media/capture/video/chromeos/camera_hal_delegate_unittest.h
(right):

https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/chromeos/camera_hal_delegate_unittest.h#newcode35
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/chromeos/camera_hal_delegate_unittest.h#newcode63
media/capture/video/chromeos/camera_hal_delegate_unittest.h:63:
arc::mojom::CameraModulePtrInfo GetInterfacePtrInfo() {

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/chromeos/mojo/arc_camera3.mojom
File media/capture/video/chromeos/mojo/arc_camera3.mojom (right):

https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/chromeos/mojo/arc_camera3.mojom#newcode260
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/chromeos/mojo/arc_camera3.mojom#newcode288
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/device_factory.mojom?dr.
>
> 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/chromeos/mojo/arc_camera3.mojom#newcode297
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?

https://codereview.chromium.org/2837273004/

[chfr...@chromium.org]

Some replies and questions

https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/chromeos/camera_hal_delegate.h
File media/capture/video/chromeos/camera_hal_delegate.h (right):

https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/chromeos/camera_hal_delegate.h#newcode45
media/capture/video/chromeos/camera_hal_delegate.h:45: // on.

I see and agree that this is reasonable.

https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/chromeos/camera_hal_delegate.h#newcode74
media/capture/video/chromeos/camera_hal_delegate.h:74: void
StartForTesting(arc::mojom::CameraModulePtrInfo info);

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/chromeos/camera_hal_delegate_unittest.cc
File media/capture/video/chromeos/camera_hal_delegate_unittest.cc
(right):

https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/chromeos/camera_hal_delegate_unittest.cc#newcode117
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/chromeos/camera_hal_delegate_unittest.h
File media/capture/video/chromeos/camera_hal_delegate_unittest.h
(right):

https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/chromeos/camera_hal_delegate_unittest.h#newcode63
media/capture/video/chromeos/camera_hal_delegate_unittest.h:63:
arc::mojom::CameraModulePtrInfo GetInterfacePtrInfo() {

> 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/chromeos/mojo/arc_camera3.mojom
File media/capture/video/chromeos/mojo/arc_camera3.mojom (right):

https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/chromeos/mojo/arc_camera3.mojom#newcode297
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/chromeos/camera_hal_delegate.h
File media/capture/video/chromeos/camera_hal_delegate.h (right):

https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/chromeos/camera_hal_delegate.h#newcode35
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?

https://codereview.chromium.org/2837273004/

[chfr...@chromium.org]

A couple more comments/questions/requests, while I'm about to dive into
CameraDeviceDelegate.

https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/chromeos/camera_hal_delegate_unittest.cc
File media/capture/video/chromeos/camera_hal_delegate_unittest.cc
(right):

https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/chromeos/camera_hal_delegate_unittest.cc#newcode42
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/chromeos/camera_device_delegate.h
File media/capture/video/chromeos/camera_device_delegate.h (right):

https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/chromeos/camera_device_delegate.h#newcode228
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/chromeos/camera_device_delegate_unittest.cc
File media/capture/video/chromeos/camera_device_delegate_unittest.cc
(right):

https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/chromeos/camera_device_delegate_unittest.cc#newcode183
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/chromeos/camera_device_delegate_unittest.cc#newcode221
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/chromeos/camera_device_delegate_unittest.cc#newcode259
media/capture/video/chromeos/camera_device_delegate_unittest.cc:259:
void Wait() {
needed?

https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/chromeos/camera_device_delegate_unittest.cc#newcode278
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/chromeos/camera_hal_delegate.cc
File media/capture/video/chromeos/camera_hal_delegate.cc (right):

https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/chromeos/camera_hal_delegate.cc#newcode110
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/chromeos/camera_hal_delegate_unittest.cc
File media/capture/video/chromeos/camera_hal_delegate_unittest.cc
(right):

https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/chromeos/camera_hal_delegate_unittest.cc#newcode60
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/chromeos/camera_hal_delegate_unittest.h
File media/capture/video/chromeos/camera_hal_delegate_unittest.h
(right):

https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/chromeos/camera_hal_delegate_unittest.h#newcode6
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/

[jcl...@chromium.org]

https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/chromeos/camera_hal_delegate.h
File media/capture/video/chromeos/camera_hal_delegate.h (right):

https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/chromeos/camera_hal_delegate.h#newcode74
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:

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/chromeos/mojo/arc_camera3.mojom
File media/capture/video/chromeos/mojo/arc_camera3.mojom (right):

https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/chromeos/mojo/arc_camera3.mojom#newcode297
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/chromeos/camera_device_delegate.h
File media/capture/video/chromeos/camera_device_delegate.h (right):

https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/chromeos/camera_device_delegate.h#newcode228
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/chromeos/camera_device_delegate_unittest.cc
File media/capture/video/chromeos/camera_device_delegate_unittest.cc
(right):

https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/chromeos/camera_device_delegate_unittest.cc#newcode183
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/chromeos/camera_device_delegate_unittest.cc#newcode221
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/chromeos/camera_device_delegate_unittest.cc#newcode259
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/chromeos/camera_device_delegate_unittest.cc#newcode278
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/chromeos/camera_hal_delegate.cc
File media/capture/video/chromeos/camera_hal_delegate.cc (right):

https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/chromeos/camera_hal_delegate.cc#newcode110
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/chromeos/camera_hal_delegate.h
File media/capture/video/chromeos/camera_hal_delegate.h (right):

https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/chromeos/camera_hal_delegate.h#newcode35
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/chromeos/camera_hal_delegate_unittest.cc
File media/capture/video/chromeos/camera_hal_delegate_unittest.cc
(right):

https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/chromeos/camera_hal_delegate_unittest.cc#newcode60
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/chromeos/camera_hal_delegate_unittest.h
File media/capture/video/chromeos/camera_hal_delegate_unittest.h
(right):

https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/chromeos/camera_hal_delegate_unittest.h#newcode6
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.

https://codereview.chromium.org/2837273004/

[chfr...@chromium.org]

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/chromeos/camera_device_delegate.h
File media/capture/video/chromeos/camera_device_delegate.h (right):

https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/chromeos/camera_device_delegate.h#newcode36
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/chromeos/camera_device_delegate.h#newcode57
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/chromeos/camera_device_delegate.h#newcode125
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.

https://codereview.chromium.org/2837273004/

[yzs...@chromium.org]

https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/chromeos/camera_hal_delegate.cc
File media/capture/video/chromeos/camera_hal_delegate.cc (right):

https://codereview.chromium.org/2837273004/diff/320001/media/capture/video/chromeos/camera_hal_delegate.cc#newcode113
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.

https://codereview.chromium.org/2837273004/

[roc...@chromium.org]

lgtm



https://codereview.chromium.org/2837273004/

[jcl...@chromium.org]

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/chromeos/camera_device_delegate.h
File media/capture/video/chromeos/camera_device_delegate.h (right):

https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/chromeos/camera_device_delegate.h#newcode36
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/chromeos/camera_device_delegate.h#newcode57
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/chromeos/camera_device_delegate.h#newcode125
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/

[jcl...@chromium.org]

https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/chromeos/camera_device_delegate.h
File media/capture/video/chromeos/camera_device_delegate.h (right):

https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/chromeos/camera_device_delegate.h#newcode57
media/capture/video/chromeos/camera_device_delegate.h:57: // to process
capture requests.

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.

https://codereview.chromium.org/2837273004/

[chfr...@chromium.org]

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/chromeos/camera_device_delegate.h
File media/capture/video/chromeos/camera_device_delegate.h (right):

https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/chromeos/camera_device_delegate.h#newcode57
media/capture/video/chromeos/camera_device_delegate.h:57: // to process
capture requests.

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/chromeos/camera_device_delegate.h#newcode228
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/chromeos/camera_device_delegate.h
File media/capture/video/chromeos/camera_device_delegate.h (right):

https://codereview.chromium.org/2837273004/diff/400001/media/capture/video/chromeos/camera_device_delegate.h#newcode105
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/chromeos/camera_device_delegate_unittest.cc
File media/capture/video/chromeos/camera_device_delegate_unittest.cc
(right):

https://codereview.chromium.org/2837273004/diff/400001/media/capture/video/chromeos/camera_device_delegate_unittest.cc#newcode462
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/chromeos/camera_device_delegate_unittest.cc#newcode475
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/

[jcl...@chromium.org]

https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/chromeos/camera_device_delegate.h
File media/capture/video/chromeos/camera_device_delegate.h (right):

https://codereview.chromium.org/2837273004/diff/360001/media/capture/video/chromeos/camera_device_delegate.h#newcode57
media/capture/video/chromeos/camera_device_delegate.h:57: // to process
capture requests.

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/chromeos/camera_device_delegate.h
File media/capture/video/chromeos/camera_device_delegate.h (right):

https://codereview.chromium.org/2837273004/diff/400001/media/capture/video/chromeos/camera_device_delegate.h#newcode105
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/chromeos/camera_device_delegate_unittest.cc
File media/capture/video/chromeos/camera_device_delegate_unittest.cc
(right):

https://codereview.chromium.org/2837273004/diff/400001/media/capture/video/chromeos/camera_device_delegate_unittest.cc#newcode462
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/chromeos/camera_device_delegate_unittest.cc#newcode475
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.

https://codereview.chromium.org/2837273004/

[chfr...@chromium.org]

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/chromeos/camera_device_context.h
File media/capture/video/chromeos/camera_device_context.h (right):

https://codereview.chromium.org/2837273004/diff/420001/media/capture/video/chromeos/camera_device_context.h#newcode39
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/chromeos/camera_device_delegate.cc
File media/capture/video/chromeos/camera_device_delegate.cc (right):

https://codereview.chromium.org/2837273004/diff/420001/media/capture/video/chromeos/camera_device_delegate.cc#newcode31
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/chromeos/camera_device_delegate.cc#newcode34
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/chromeos/camera_device_delegate.cc#newcode179
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/chromeos/camera_device_delegate.cc#newcode229
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/chromeos/camera_device_delegate.cc#newcode232
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/chromeos/camera_device_delegate.cc
File media/capture/video/chromeos/camera_device_delegate.cc (right):

https://codereview.chromium.org/2837273004/diff/500001/media/capture/video/chromeos/camera_device_delegate.cc#newcode220
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/chromeos/stream_buffer_manager.cc
File media/capture/video/chromeos/stream_buffer_manager.cc (right):

https://codereview.chromium.org/2837273004/diff/500001/media/capture/video/chromeos/stream_buffer_manager.cc#newcode336
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/chromeos/stream_buffer_manager.cc#newcode456
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/chromeos/stream_buffer_manager.h
File media/capture/video/chromeos/stream_buffer_manager.h (right):

https://codereview.chromium.org/2837273004/diff/500001/media/capture/video/chromeos/stream_buffer_manager.h#newcode60
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/chromeos/stream_buffer_manager_unittest.cc
File media/capture/video/chromeos/stream_buffer_manager_unittest.cc
(right):

https://codereview.chromium.org/2837273004/diff/500001/media/capture/video/chromeos/stream_buffer_manager_unittest.cc#newcode78
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/chromeos/stream_buffer_manager_unittest.cc#newcode83
media/capture/video/chromeos/stream_buffer_manager_unittest.cc:83: new
MockStreamCaptureInterface()),
base::MakeUnique<>

https://codereview.chromium.org/2837273004/diff/500001/media/capture/video/chromeos/stream_buffer_manager_unittest.cc#newcode90
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/chromeos/stream_buffer_manager_unittest.cc#newcode316
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?

https://codereview.chromium.org/2837273004/

[jcl...@chromium.org]

Thank you so much for spending time reviewing this gigantic CL, Christian!

https://codereview.chromium.org/2837273004/diff/420001/media/capture/video/chromeos/camera_device_delegate.cc
File media/capture/video/chromeos/camera_device_delegate.cc (right):

https://codereview.chromium.org/2837273004/diff/420001/media/capture/video/chromeos/camera_device_delegate.cc#newcode31
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/chromeos/camera_device_delegate.cc#newcode34
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/chromeos/camera_device_delegate.cc#newcode179
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/chromeos/camera_device_delegate.cc#newcode229
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/chromeos/camera_device_delegate.cc#newcode232
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/chromeos/camera_device_delegate.cc
File media/capture/video/chromeos/camera_device_delegate.cc (right):

https://codereview.chromium.org/2837273004/diff/500001/media/capture/video/chromeos/camera_device_delegate.cc#newcode220
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/chromeos/stream_buffer_manager.cc
File media/capture/video/chromeos/stream_buffer_manager.cc (right):

https://codereview.chromium.org/2837273004/diff/500001/media/capture/video/chromeos/stream_buffer_manager.cc#newcode336
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/chromeos/stream_buffer_manager.cc#newcode456
media/capture/video/chromeos/stream_buffer_manager.cc:456: if
(!sync_wait(fence.get().handle, kSyncWaitTimeoutMs)) {

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/chromeos/stream_buffer_manager.h
File media/capture/video/chromeos/stream_buffer_manager.h (right):

https://codereview.chromium.org/2837273004/diff/500001/media/capture/video/chromeos/stream_buffer_manager.h#newcode60
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/chromeos/stream_buffer_manager_unittest.cc
File media/capture/video/chromeos/stream_buffer_manager_unittest.cc
(right):

https://codereview.chromium.org/2837273004/diff/500001/media/capture/video/chromeos/stream_buffer_manager_unittest.cc#newcode78
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/chromeos/stream_buffer_manager_unittest.cc#newcode83
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/chromeos/stream_buffer_manager_unittest.cc#newcode90
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/chromeos/stream_buffer_manager_unittest.cc#newcode316
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.

https://codereview.chromium.org/2837273004/

[wuch...@chromium.org]

lgtm

Congrats!


https://codereview.chromium.org/2837273004/diff/520001/media/capture/video/chromeos/camera_device_delegate.cc
File media/capture/video/chromeos/camera_device_delegate.cc (right):

https://codereview.chromium.org/2837273004/diff/520001/media/capture/video/chromeos/camera_device_delegate.cc#newcode128
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/chromeos/camera_device_delegate.cc#newcode134
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/chromeos/camera_device_delegate.cc#newcode136
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/chromeos/camera_device_delegate.cc#newcode178
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/chromeos/camera_device_delegate.h
File media/capture/video/chromeos/camera_device_delegate.h (right):

https://codereview.chromium.org/2837273004/diff/520001/media/capture/video/chromeos/camera_device_delegate.h#newcode51
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/chromeos/camera_hal_delegate.cc
File media/capture/video/chromeos/camera_hal_delegate.cc (right):

https://codereview.chromium.org/2837273004/diff/520001/media/capture/video/chromeos/camera_hal_delegate.cc#newcode94
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/chromeos/camera_hal_delegate.cc#newcode287
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/chromeos/camera_hal_delegate.cc#newcode289
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/chromeos/camera_hal_delegate.cc#newcode338
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/chromeos/camera_hal_delegate.h
File media/capture/video/chromeos/camera_hal_delegate.h (right):

https://codereview.chromium.org/2837273004/diff/520001/media/capture/video/chromeos/camera_hal_delegate.h#newcode83
media/capture/video/chromeos/camera_hal_delegate.h:83: void
ResetMojoInterfaceOnModuleThread();
ResetMojoInterfaceOnIPCThread

https://codereview.chromium.org/2837273004/

[jcl...@chromium.org]

https://codereview.chromium.org/2837273004/diff/520001/media/capture/video/chromeos/camera_device_delegate.cc
File media/capture/video/chromeos/camera_device_delegate.cc (right):

https://codereview.chromium.org/2837273004/diff/520001/media/capture/video/chromeos/camera_device_delegate.cc#newcode128
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/chromeos/camera_device_delegate.cc#newcode134
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/chromeos/camera_device_delegate.cc#newcode136
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/chromeos/camera_device_delegate.cc#newcode178
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/chromeos/camera_device_delegate.h
File media/capture/video/chromeos/camera_device_delegate.h (right):

https://codereview.chromium.org/2837273004/diff/520001/media/capture/video/chromeos/camera_device_delegate.h#newcode51
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/chromeos/camera_hal_delegate.cc
File media/capture/video/chromeos/camera_hal_delegate.cc (right):

https://codereview.chromium.org/2837273004/diff/520001/media/capture/video/chromeos/camera_hal_delegate.cc#newcode94
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/chromeos/camera_hal_delegate.cc#newcode287
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/chromeos/camera_hal_delegate.cc#newcode289
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/chromeos/camera_hal_delegate.cc#newcode338
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/chromeos/camera_hal_delegate.h
File media/capture/video/chromeos/camera_hal_delegate.h (right):

https://codereview.chromium.org/2837273004/diff/520001/media/capture/video/chromeos/camera_hal_delegate.h#newcode83
media/capture/video/chromeos/camera_hal_delegate.h:83: void
ResetMojoInterfaceOnModuleThread();

On 2017/06/09 07:43:54, wuchengli wrote:
> ResetMojoInterfaceOnIPCThread

Done.

https://codereview.chromium.org/2837273004/

[jcl...@chromium.org]

David - Can you take a look at the modifications to third_party/libsync?

https://codereview.chromium.org/2837273004/

[chfr...@chromium.org]

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/chromeos/camera_device_delegate.cc
File media/capture/video/chromeos/camera_device_delegate.cc (right):

https://codereview.chromium.org/2837273004/diff/500001/media/capture/video/chromeos/camera_device_delegate.cc#newcode220
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/chromeos/stream_buffer_manager.cc
File media/capture/video/chromeos/stream_buffer_manager.cc (right):

https://codereview.chromium.org/2837273004/diff/500001/media/capture/video/chromeos/stream_buffer_manager.cc#newcode456
media/capture/video/chromeos/stream_buffer_manager.cc:456: if
(!sync_wait(fence.get().handle, kSyncWaitTimeoutMs)) {

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/chromeos/stream_buffer_manager_unittest.cc
File media/capture/video/chromeos/stream_buffer_manager_unittest.cc
(right):

https://codereview.chromium.org/2837273004/diff/500001/media/capture/video/chromeos/stream_buffer_manager_unittest.cc#newcode90
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

[jcl...@chromium.org]

https://codereview.chromium.org/2837273004/diff/500001/media/capture/video/chromeos/stream_buffer_manager.cc
File media/capture/video/chromeos/stream_buffer_manager.cc (right):

https://codereview.chromium.org/2837273004/diff/500001/media/capture/video/chromeos/stream_buffer_manager.cc#newcode456
media/capture/video/chromeos/stream_buffer_manager.cc:456: if
(!sync_wait(fence.get().handle, kSyncWaitTimeoutMs)) {

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.

https://codereview.chromium.org/2837273004/

[pos...@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#newcode52
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/chromeos/camera_device_context.cc
File media/capture/video/chromeos/camera_device_context.cc (right):

https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/chromeos/camera_device_context.cc#newcode48
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/chromeos/camera_device_context.h
File media/capture/video/chromeos/camera_device_context.h (right):

https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/chromeos/camera_device_context.h#newcode22
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/chromeos/camera_device_context.h#newcode110
media/capture/video/chromeos/camera_device_context.h:110: int length,
size_t perhaps?

https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/chromeos/camera_device_delegate.cc
File media/capture/video/chromeos/camera_device_delegate.cc (right):

https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/chromeos/camera_device_delegate.cc#newcode15
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/chromeos/camera_device_delegate.cc#newcode24
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/chromeos/camera_device_delegate.cc#newcode69
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/chromeos/camera_device_delegate.cc#newcode72
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/chromeos/camera_device_delegate.cc#newcode99
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/chromeos/camera_device_delegate.cc#newcode219
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/chromeos/camera_device_delegate.cc#newcode260
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/chromeos/camera_device_delegate.h
File media/capture/video/chromeos/camera_device_delegate.h (right):

https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/chromeos/camera_device_delegate.h#newcode25
media/capture/video/chromeos/camera_device_delegate.h:25: virtual
~StreamCaptureInterface() {}
= default ?

https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/chromeos/camera_device_delegate.h#newcode35
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/chromeos/camera_device_delegate.h#newcode81
media/capture/video/chromeos/camera_device_delegate.h:81:
~CameraDeviceDelegate();
= default?

https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/chromeos/camera_hal_delegate.cc
File media/capture/video/chromeos/camera_hal_delegate.cc (right):

https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/chromeos/camera_hal_delegate.cc#newcode168
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/chromeos/camera_hal_delegate.cc#newcode174
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/chromeos/camera_hal_delegate.cc#newcode359
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/chromeos/camera_hal_delegate.h
File media/capture/video/chromeos/camera_hal_delegate.h (right):

https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/chromeos/camera_hal_delegate.h#newcode40
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/chromeos/camera_hal_delegate.h#newcode126
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/chromeos/stream_buffer_manager.cc
File media/capture/video/chromeos/stream_buffer_manager.cc (right):

https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/chromeos/stream_buffer_manager.cc#newcode212
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/chromeos/stream_buffer_manager.cc#newcode245
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/chromeos/stream_buffer_manager.cc#newcode307
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/chromeos/stream_buffer_manager.cc#newcode342
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/chromeos/stream_buffer_manager.cc#newcode390
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/chromeos/stream_buffer_manager.h
File media/capture/video/chromeos/stream_buffer_manager.h (right):

https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/chromeos/stream_buffer_manager.h#newcode8
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/chromeos/video_capture_device_arc_chromeos.cc
File media/capture/video/chromeos/video_capture_device_arc_chromeos.cc
(right):

https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/chromeos/video_capture_device_arc_chromeos.cc#newcode76
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/chromeos/video_capture_device_arc_chromeos.h
File media/capture/video/chromeos/video_capture_device_arc_chromeos.h
(right):

https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/chromeos/video_capture_device_arc_chromeos.h#newcode5
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/chromeos/video_capture_device_arc_chromeos.h#newcode8
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/chromeos/video_capture_device_arc_chromeos.h#newcode11
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/chromeos/video_capture_device_arc_chromeos.h#newcode79
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/chromeos/video_capture_device_factory_chromeos.cc
File
media/capture/video/chromeos/video_capture_device_factory_chromeos.cc
(right):

https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/chromeos/video_capture_device_factory_chromeos.cc#newcode62
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/video_capture_device_unittest.cc
File media/capture/video/video_capture_device_unittest.cc (right):

https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/video_capture_device_unittest.cc#newcode79
media/capture/video/video_capture_device_unittest.cc:79: #elif
defined(OS_CHROMEOS)
Doesn't this disable tests for existing V4L2 devices on CrOS?

https://codereview.chromium.org/2837273004/

[jcl...@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#newcode52
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/chromeos/camera_device_context.cc
File media/capture/video/chromeos/camera_device_context.cc (right):

https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/chromeos/camera_device_context.cc#newcode48
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/chromeos/camera_device_context.h
File media/capture/video/chromeos/camera_device_context.h (right):

https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/chromeos/camera_device_context.h#newcode22
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/chromeos/camera_device_context.h#newcode110
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.h?q=video_capture_device.h&dr=CSs&l=143

We'll need to do static_cast if we declare length as size_t.

https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/chromeos/camera_device_delegate.cc
File media/capture/video/chromeos/camera_device_delegate.cc (right):

https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/chromeos/camera_device_delegate.cc#newcode15
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/chromeos/camera_device_delegate.cc#newcode24
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/chromeos/camera_device_delegate.cc#newcode69
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/chromeos/camera_device_delegate.cc#newcode72
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/chromeos/camera_device_delegate.cc#newcode99
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/chromeos/camera_device_delegate.cc#newcode219
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/chromeos/camera_device_delegate.cc#newcode260
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/chromeos/camera_device_delegate.h
File media/capture/video/chromeos/camera_device_delegate.h (right):

https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/chromeos/camera_device_delegate.h#newcode25
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/chromeos/camera_device_delegate.h#newcode35
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/chromeos/camera_device_delegate.h#newcode81
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/chromeos/camera_hal_delegate.cc
File media/capture/video/chromeos/camera_hal_delegate.cc (right):

https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/chromeos/camera_hal_delegate.cc#newcode168
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/chromeos/camera_hal_delegate.cc#newcode174
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/chromeos/camera_hal_delegate.cc#newcode359
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/chromeos/camera_hal_delegate.h
File media/capture/video/chromeos/camera_hal_delegate.h (right):

https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/chromeos/camera_hal_delegate.h#newcode40
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/chromeos/camera_hal_delegate.h#newcode126
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/chromeos/stream_buffer_manager.cc
File media/capture/video/chromeos/stream_buffer_manager.cc (right):

https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/chromeos/stream_buffer_manager.cc#newcode212
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/chromeos/stream_buffer_manager.cc#newcode245
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/chromeos/stream_buffer_manager.cc#newcode307
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/chromeos/stream_buffer_manager.cc#newcode342
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/chromeos/stream_buffer_manager.cc#newcode390
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/chromeos/stream_buffer_manager.h
File media/capture/video/chromeos/stream_buffer_manager.h (right):

https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/chromeos/stream_buffer_manager.h#newcode8
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/chromeos/video_capture_device_arc_chromeos.cc
File media/capture/video/chromeos/video_capture_device_arc_chromeos.cc
(right):

https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/chromeos/video_capture_device_arc_chromeos.cc#newcode76
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/chromeos/video_capture_device_arc_chromeos.h
File media/capture/video/chromeos/video_capture_device_arc_chromeos.h
(right):

https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/chromeos/video_capture_device_arc_chromeos.h#newcode5
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/chromeos/video_capture_device_arc_chromeos.h#newcode8
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/chromeos/video_capture_device_arc_chromeos.h#newcode11
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/chromeos/video_capture_device_arc_chromeos.h#newcode79
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/chromeos/video_capture_device_factory_chromeos.cc
File
media/capture/video/chromeos/video_capture_device_factory_chromeos.cc
(right):

https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/chromeos/video_capture_device_factory_chromeos.cc#newcode62
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/video_capture_device_unittest.cc
File media/capture/video/video_capture_device_unittest.cc (right):

https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/video_capture_device_unittest.cc#newcode79
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.

https://codereview.chromium.org/2837273004/

[jcl...@chromium.org]

https://codereview.chromium.org/2837273004/

[pos...@chromium.org]

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/chromeos/camera_device_delegate.cc
File media/capture/video/chromeos/camera_device_delegate.cc (right):

https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/chromeos/camera_device_delegate.cc#newcode24
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/chromeos/camera_device_delegate.cc#newcode260
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/chromeos/stream_buffer_manager.cc
File media/capture/video/chromeos/stream_buffer_manager.cc (right):

https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/chromeos/stream_buffer_manager.cc#newcode245
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/chromeos/video_capture_device_factory_chromeos.cc
File
media/capture/video/chromeos/video_capture_device_factory_chromeos.cc
(right):

https://codereview.chromium.org/2837273004/diff/620001/media/capture/video/chromeos/video_capture_device_factory_chromeos.cc#newcode62
media/capture/video/chromeos/video_capture_device_factory_chromeos.cc:62:
VideoCaptureDeviceFactory::CreateVideoCaptureDeviceFactory(

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#newcode52
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/chromeos/camera_hal_delegate.cc
File media/capture/video/chromeos/camera_hal_delegate.cc (right):

https://codereview.chromium.org/2837273004/diff/680001/media/capture/video/chromeos/camera_hal_delegate.cc#newcode175
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/chromeos/stream_buffer_manager.cc
File media/capture/video/chromeos/stream_buffer_manager.cc (right):

https://codereview.chromium.org/2837273004/diff/680001/media/capture/video/chromeos/stream_buffer_manager.cc#newcode275
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/chromeos/stream_buffer_manager.cc#newcode411
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/chromeos/video_capture_device_factory_chromeos.cc
File
media/capture/video/chromeos/video_capture_device_factory_chromeos.cc
(right):

https://codereview.chromium.org/2837273004/diff/680001/media/capture/video/chromeos/video_capture_device_factory_chromeos.cc#newcode63
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.

https://codereview.chromium.org/2837273004/

[rev...@chromium.org]

media/capture/video/chromeos/DEPS lgtm

https://codereview.chromium.org/2837273004/

[jcl...@chromium.org]

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#newcode52
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/chromeos/camera_hal_delegate.cc
File media/capture/video/chromeos/camera_hal_delegate.cc (right):

https://codereview.chromium.org/2837273004/diff/680001/media/capture/video/chromeos/camera_hal_delegate.cc#newcode175
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/chromeos/stream_buffer_manager.cc
File media/capture/video/chromeos/stream_buffer_manager.cc (right):

https://codereview.chromium.org/2837273004/diff/680001/media/capture/video/chromeos/stream_buffer_manager.cc#newcode275
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/chromeos/stream_buffer_manager.cc#newcode411
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/chromeos/video_capture_device_factory_chromeos.cc
File
media/capture/video/chromeos/video_capture_device_factory_chromeos.cc
(right):

https://codereview.chromium.org/2837273004/diff/680001/media/capture/video/chromeos/video_capture_device_factory_chromeos.cc#newcode63
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.

https://codereview.chromium.org/2837273004/

[pos...@chromium.org]

thanks!

lgtm % nit (note: I did not review unittests)


https://codereview.chromium.org/2837273004/diff/700001/media/capture/video/chromeos/camera_device_delegate.h
File media/capture/video/chromeos/camera_device_delegate.h (right):

https://codereview.chromium.org/2837273004/diff/700001/media/capture/video/chromeos/camera_device_delegate.h#newcode79
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/

[jcl...@chromium.org]

https://codereview.chromium.org/2837273004/diff/700001/media/capture/video/chromeos/camera_device_delegate.h
File media/capture/video/chromeos/camera_device_delegate.h (right):

https://codereview.chromium.org/2837273004/diff/700001/media/capture/video/chromeos/camera_device_delegate.h#newcode79
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.

https://codereview.chromium.org/2837273004/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2837273004/700001

https://codereview.chromium.org/2837273004/

[commit-bot@chromium.org via codereview.chromium.org]

Committed patchset #36 (id:700001) as
https://chromium.googlesource.com/chromium/src/+/b2b84801bfe858bfe7e568354cb3ca3f2578e78e

https://codereview.chromium.org/2837273004/

[findit-for-me@appspot.gserviceaccount.com via codereview.chromium.org]

A revert of this CL (patchset #36 id:700001) has been created in
https://codereview.chromium.org/2936373002/ by
findit...@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=ag9zfmZpbmRpdC1mb3ItbWVyRAsSDVdmU3VzcGVjdGVkQ0wiMWNocm9taXVtL2IyYjg0ODAxYmZlODU4YmZlN2U1NjgzNTRjYjNjYTNmMjU3OGU3OGUM.

https://codereview.chromium.org/2837273004/