Send commands from webcamPrivate API to the webcam via V4L2 ioctls

Send commands from webcamPrivate API to the webcam via V4L2 ioctls

BUG=346492
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266017

[Zachary Kuznia, 2014-04-11 21:34:07]

Hello Victoria,

Could you take a look at this?

This CL always uses the default webcam, but the logic for adjusting the
parameters should be correct.  I'll have a followup CL to properly pull the
device name from MediaStreamManager next week.

Thanks,
-Zach

[perkj_chrome, 2014-04-17 09:35:48]

FYI

https://codereview.chromium.org/234843006/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/webcam_private/webcam_private_api.cc (right):

https://codereview.chromium.org/234843006/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/webcam_private/webcam_private_api.cc:190: int fd =
OpenWebcam(extension_id(), browser_context(), params->webcam_id);
There are some handy macros:

#include "base/files/file_enumerator.h"
#include "base/files/scoped_file.h"
#include "base/posix/eintr_wrapper.h"

base::ScopedFD

base::ScopedFD fd(HANDLE_EINTR

[Zachary Kuznia, 2014-04-17 20:51:31]

https://codereview.chromium.org/234843006/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/webcam_private/webcam_private_api.cc (right):

https://codereview.chromium.org/234843006/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/webcam_private/webcam_private_api.cc:190: int fd =
OpenWebcam(extension_id(), browser_context(), params->webcam_id);
On 2014/04/17 09:35:48, perkj wrote:
> There are some handy macros:
> 
> #include "base/files/file_enumerator.h"
> #include "base/files/scoped_file.h"
> #include "base/posix/eintr_wrapper.h"
> 
> base::ScopedFD
> 
> base::ScopedFD fd(HANDLE_EINTR

Done.

[vrk (LEFT CHROMIUM), 2014-04-17 23:16:56]

Sorry for the delay! I made a few notes on code style but +tommi to review the
V4L2 stuff.

https://codereview.chromium.org/234843006/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/webcam_private/webcam_private_api.cc (right):

https://codereview.chromium.org/234843006/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/webcam_private/webcam_private_api.cc:29: const int
kDefaultZoom = 100;
You should make this a static local constant in the function, since that's the
only place it's used.

https://codereview.chromium.org/234843006/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/webcam_private/webcam_private_api.cc:34: //
TODO(zork): Get device_id from content::MediaStreamManager
nit: period at end of sentence

https://codereview.chromium.org/234843006/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/webcam_private/webcam_private_api.cc:35:
std::string device_id = "/dev/video0";
Hmmm... this feels wrong to me. I know you're planning to fix this later,
though, so I defer to tommi@ on what's the best way to query the default device.
If tommi's OK with this, fine with me too.

https://codereview.chromium.org/234843006/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/webcam_private/webcam_private_api.cc:40: void
SetWebcamParameter(int fd, WebcamParameter param, int value) {
The WebcamParameter enum seems superfluous. I'd split this method up a little
differently, like:

  SetWebcamParameter(int fd, int control_id, int control_value) {
    struct v4l2_control v4l2_ctrl = {control_id, control_value};
    HANDLE_EINTR(ioctl(fd, VIDIOC_S_CTRL, &v4l2_ctrl);
  }

Then in the callsite, you can have:
  if (params->config.pan)
    SetWebcamParameter(fd.get(), V4L2_CID_PAN_ABSOLUTE, *(params->config.pan));

WDYT? I'd suggest collapsing all the {Set,Get,Reset}Parameter methods in this
manner. (You could probably get rid of the Reset method, actually.)

[Zachary Kuznia, 2014-04-18 22:44:15]

https://codereview.chromium.org/234843006/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/webcam_private/webcam_private_api.cc (right):

https://codereview.chromium.org/234843006/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/webcam_private/webcam_private_api.cc:29: const int
kDefaultZoom = 100;
On 2014/04/17 23:16:56, Victoria Kirst wrote:
> You should make this a static local constant in the function, since that's the
> only place it's used.

Done.

https://codereview.chromium.org/234843006/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/webcam_private/webcam_private_api.cc:34: //
TODO(zork): Get device_id from content::MediaStreamManager
On 2014/04/17 23:16:56, Victoria Kirst wrote:
> nit: period at end of sentence

Done.

https://codereview.chromium.org/234843006/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/webcam_private/webcam_private_api.cc:35:
std::string device_id = "/dev/video0";
On 2014/04/17 23:16:56, Victoria Kirst wrote:
> Hmmm... this feels wrong to me. I know you're planning to fix this later,
> though, so I defer to tommi@ on what's the best way to query the default
device.
> If tommi's OK with this, fine with me too.

I'll be sending out a follow-up CL for review as soon as this one lands.  If the
follow up CL hasn't landed by the time this code would be in use, then I'll be
disabling the whole API.

https://codereview.chromium.org/234843006/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/webcam_private/webcam_private_api.cc:40: void
SetWebcamParameter(int fd, WebcamParameter param, int value) {
On 2014/04/17 23:16:56, Victoria Kirst wrote:
> The WebcamParameter enum seems superfluous. I'd split this method up a little
> differently, like:
> 
>   SetWebcamParameter(int fd, int control_id, int control_value) {
>     struct v4l2_control v4l2_ctrl = {control_id, control_value};
>     HANDLE_EINTR(ioctl(fd, VIDIOC_S_CTRL, &v4l2_ctrl);
>   }
> 
> Then in the callsite, you can have:
>   if (params->config.pan)
>     SetWebcamParameter(fd.get(), V4L2_CID_PAN_ABSOLUTE,
*(params->config.pan));
> 
> WDYT? I'd suggest collapsing all the {Set,Get,Reset}Parameter methods in this
> manner. (You could probably get rid of the Reset method, actually.)

Done.

[tommi (sloooow) - chröme, 2014-04-23 14:40:00]

https://codereview.chromium.org/234843006/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/webcam_private/webcam_private_api.cc (right):

https://codereview.chromium.org/234843006/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/webcam_private/webcam_private_api.cc:19: } //
namespace content
nit: 2 spaces before //
(here and throughout)

https://codereview.chromium.org/234843006/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/webcam_private/webcam_private_api.cc:23:
base::ScopedFD OpenWebcam(const std::string& extension_id,
this seems like linux specific code.  is it supposed to work on other platforms?

Maybe this file should have a _posix postfix.

https://codereview.chromium.org/234843006/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/webcam_private/webcam_private_api.cc:46: 
only one empty line

[Zachary Kuznia, 2014-04-24 00:22:12]

https://codereview.chromium.org/234843006/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/webcam_private/webcam_private_api.cc (right):

https://codereview.chromium.org/234843006/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/webcam_private/webcam_private_api.cc:19: } //
namespace content
On 2014/04/23 14:40:00, tommi wrote:
> nit: 2 spaces before //
> (here and throughout)

Done.

https://codereview.chromium.org/234843006/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/webcam_private/webcam_private_api.cc:23:
base::ScopedFD OpenWebcam(const std::string& extension_id,
On 2014/04/23 14:40:00, tommi wrote:
> this seems like linux specific code.  is it supposed to work on other
platforms?
> 
> Maybe this file should have a _posix postfix.

Changed to webcam_private_api_chromeos.cc

https://codereview.chromium.org/234843006/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/webcam_private/webcam_private_api.cc:46: 
On 2014/04/23 14:40:00, tommi wrote:
> only one empty line

Done.

[tommi (sloooow) - chröme, 2014-04-24 15:13:51]

lgtm

[Zachary Kuznia, 2014-04-24 15:58:46]

Hi kalman,

Could you do an OWNERS review of this?

Thanks,
-Zach

[not at google - send to devlin, 2014-04-24 16:09:52]

lgtm but try to have OWNERS file for API impementations

https://codereview.chromium.org/234843006/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/api/webcam_private/webcam_private_api_chromeos.cc
(right):

https://codereview.chromium.org/234843006/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/webcam_private/webcam_private_api_chromeos.cc:3:
// found in the LICENSE file.
if this is effectively the same file then make sure it's detected as a file move
not a new file

[Zachary Kuznia, 2014-04-24 16:22:52]

On 2014/04/24 16:09:52, kalman wrote:
> lgtm but try to have OWNERS file for API impementations
> 
Added myself as OWNER

[Zachary Kuznia, 2014-04-24 16:30:04]

The CQ bit was checked by zork@chromium.org

[commit-bot: I haz the power, 2014-04-24 16:30:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zork@chromium.org/234843006/120001

[commit-bot: I haz the power, 2014-04-24 22:54:48]

Change committed as 266017