Linux Video Capture: small cleanup

media/video/capture/linux/video_capture_device_linux.cc

Index: media/video/capture/linux/video_capture_device_linux.cc
diff --git a/media/video/capture/linux/video_capture_device_linux.cc b/media/video/capture/linux/video_capture_device_linux.cc
index 6097a1e753c075f90df35a1ec7e54fac1724c832..c42c4c74da2f4c623e6c4a8c023e11a3fdc68b1a 100644
--- a/media/video/capture/linux/video_capture_device_linux.cc
+++ b/media/video/capture/linux/video_capture_device_linux.cc
@@ -26,6 +26,8 @@
 
 namespace media {
 
+#define GET_V4L2_FOURCC_CHAR(a, index) ((char)( ((a) >> (8 * index)) & 0xff))
+
 // Max number of video buffers VideoCaptureDeviceLinux can allocate.
 enum { kMaxVideoBuffers = 2 };
 // Timeout in milliseconds v4l2_thread_ blocks waiting for a frame from the hw.
@@ -55,7 +57,7 @@ static const char kVidPathTemplate[] =
 static const char kPidPathTemplate[] =
     "/sys/class/video4linux/%s/device/../idProduct";
 
-bool ReadIdFile(const std::string path, std::string* id) {
+static bool ReadIdFile(const std::string path, std::string* id) {
   char id_buf[kVidPidSize];
   FILE* file = fopen(path.c_str(), "rb");
   if (!file)
@@ -68,11 +70,18 @@ bool ReadIdFile(const std::string path, std::string* id) {
   return true;
 }
 
+//static
+int VideoCaptureDeviceLinux::Ioctl(const base::ScopedFD& device_fd,
+                                   int request,
+                                   void* arg) {
+  return HANDLE_EINTR(ioctl(device_fd.get(), request, arg));

[perkj_chrome, 2014/12/16 10:34:43]

I think this is less readable than calling HANDLE_EINTR(ioctl(device_fd.get(),
request, arg)) inline. 

codesearch seems to agree with me mostly, but not 100%. 
The virtual method Ioctl you reference seems to be there since there are
different implementations that need to be called.

[mcasas, 2014/12/16 18:08:17]

Reverted then.

Just FTR I'd say it was cleaner to refer to a local Ioctl()
method that encapsulated the HANDLE_EINTR part, and would
homogeneise the <= 0, < 0, == -1 etc comparisons all over,
all meaning "if ok" or "if not ok".

+}
+
 // This function translates Video4Linux pixel formats to Chromium pixel formats,
 // should only support those listed in GetListOfUsableFourCCs.
 // static
-VideoPixelFormat VideoCaptureDeviceLinux::V4l2ColorToVideoCaptureColorFormat(
-    int32 v4l2_fourcc) {
+VideoPixelFormat VideoCaptureDeviceLinux::V4l2FourCcToChromiumPixelFormat(
+    uint32 v4l2_fourcc) {
   VideoPixelFormat result = PIXEL_FORMAT_UNKNOWN;
   switch (v4l2_fourcc) {
     case V4L2_PIX_FMT_YUV420:
@@ -89,24 +98,30 @@ VideoPixelFormat VideoCaptureDeviceLinux::V4l2ColorToVideoCaptureColorFormat(
       result = PIXEL_FORMAT_MJPEG;
       break;
     default:
-      DVLOG(1) << "Unsupported pixel format " << std::hex << v4l2_fourcc;
+      DVLOG(1) << "Unsupported pixel format: "
+          << GET_V4L2_FOURCC_CHAR(v4l2_fourcc, 0)
+          << GET_V4L2_FOURCC_CHAR(v4l2_fourcc, 1)
+          << GET_V4L2_FOURCC_CHAR(v4l2_fourcc, 2)
+          << GET_V4L2_FOURCC_CHAR(v4l2_fourcc, 3);
   }
   return result;
 }
 
 // static
-void VideoCaptureDeviceLinux::GetListOfUsableFourCCs(bool favour_mjpeg,
-                                                     std::list<int>* fourccs) {
+std::list<int> VideoCaptureDeviceLinux::GetListOfUsableFourCCs(
+    bool favour_mjpeg) {
+  std::list<int> fourccs;
   for (size_t i = 0; i < arraysize(kV4l2RawFmts); ++i)
-    fourccs->push_back(kV4l2RawFmts[i]);
+    fourccs.push_back(kV4l2RawFmts[i]);
   if (favour_mjpeg)
-    fourccs->push_front(V4L2_PIX_FMT_MJPEG);
+    fourccs.push_front(V4L2_PIX_FMT_MJPEG);
   else
-    fourccs->push_back(V4L2_PIX_FMT_MJPEG);
+    fourccs.push_back(V4L2_PIX_FMT_MJPEG);
 
   // JPEG works as MJPEG on some gspca webcams from field reports.
   // Put it as the least preferred format.
-  fourccs->push_back(V4L2_PIX_FMT_JPEG);
+  fourccs.push_back(V4L2_PIX_FMT_JPEG);
+  return fourccs;
 }
 
 const std::string VideoCaptureDevice::Name::GetModel() const {
@@ -216,7 +231,7 @@ void VideoCaptureDeviceLinux::OnAllocateAndStart(int width,
 
   // Test if this is a V4L2 capture device.
   v4l2_capability cap;
-  if (!((HANDLE_EINTR(ioctl(device_fd_.get(), VIDIOC_QUERYCAP, &cap)) == 0) &&
+  if (!((Ioctl(device_fd_, VIDIOC_QUERYCAP, &cap) == 0) &&
         (cap.capabilities & V4L2_CAP_VIDEO_CAPTURE) &&
         !(cap.capabilities & V4L2_CAP_VIDEO_OUTPUT))) {
     // This is not a V4L2 video capture device.
@@ -227,22 +242,16 @@ void VideoCaptureDeviceLinux::OnAllocateAndStart(int width,
 
   // Get supported video formats in preferred order.
   // For large resolutions, favour mjpeg over raw formats.
-  std::list<int> v4l2_formats;
-  GetListOfUsableFourCCs(width > kMjpegWidth || height > kMjpegHeight,
-                         &v4l2_formats);
+  const std::list<int>& desired_v4l2_formats =
+      GetListOfUsableFourCCs(width > kMjpegWidth || height > kMjpegHeight);
+  std::list<int>::const_iterator best = desired_v4l2_formats.end();
 
   v4l2_fmtdesc fmtdesc = {0};
   fmtdesc.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
-
-  // Enumerate image formats.
-  std::list<int>::iterator best = v4l2_formats.end();
-  while (HANDLE_EINTR(ioctl(device_fd_.get(), VIDIOC_ENUM_FMT, &fmtdesc)) ==
-         0) {
-    best = std::find(v4l2_formats.begin(), best, fmtdesc.pixelformat);
-    fmtdesc.index++;
+  for (; Ioctl(device_fd_, VIDIOC_ENUM_FMT, &fmtdesc) == 0; ++fmtdesc.index) {
+    best = std::find(desired_v4l2_formats.begin(), best, fmtdesc.pixelformat);
   }
-
-  if (best == v4l2_formats.end()) {
+  if (best == desired_v4l2_formats.end()) {
     SetErrorState("Failed to find a supported camera format.");
     return;
   }
@@ -256,7 +265,7 @@ void VideoCaptureDeviceLinux::OnAllocateAndStart(int width,
   video_fmt.fmt.pix.height = height;
   video_fmt.fmt.pix.pixelformat = *best;
 
-  if (HANDLE_EINTR(ioctl(device_fd_.get(), VIDIOC_S_FMT, &video_fmt)) < 0) {
+  if (Ioctl(device_fd_, VIDIOC_S_FMT, &video_fmt) < 0) {
     SetErrorState(
         base::StringPrintf("Failed to set camera format: %s", strerror(errno)));
     return;
@@ -267,7 +276,7 @@ void VideoCaptureDeviceLinux::OnAllocateAndStart(int width,
   memset(&streamparm, 0, sizeof(v4l2_streamparm));
   streamparm.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
   // The following line checks that the driver knows about framerate get/set.
-  if (HANDLE_EINTR(ioctl(device_fd_.get(), VIDIOC_G_PARM, &streamparm)) >= 0) {
+  if (Ioctl(device_fd_, VIDIOC_G_PARM, &streamparm) >= 0) {
     // Now check if the device is able to accept a capture framerate set.
     if (streamparm.parm.capture.capability & V4L2_CAP_TIMEPERFRAME) {
       // |frame_rate| is float, approximate by a fraction.
@@ -277,8 +286,7 @@ void VideoCaptureDeviceLinux::OnAllocateAndStart(int width,
           (frame_rate * media::kFrameRatePrecision) :
           (kTypicalFramerate * media::kFrameRatePrecision);
 
-      if (HANDLE_EINTR(ioctl(device_fd_.get(), VIDIOC_S_PARM, &streamparm)) <
-          0) {
+      if (Ioctl(device_fd_, VIDIOC_S_PARM, &streamparm) < 0) {
         SetErrorState("Failed to set camera framerate");
         return;
       }
@@ -308,18 +316,16 @@ void VideoCaptureDeviceLinux::OnAllocateAndStart(int width,
                                      video_fmt.fmt.pix.height);
   capture_format_.frame_rate = frame_rate;
   capture_format_.pixel_format =
-      V4l2ColorToVideoCaptureColorFormat(video_fmt.fmt.pix.pixelformat);
+      V4l2FourCcToChromiumPixelFormat(video_fmt.fmt.pix.pixelformat);
 
-  // Start capturing.
   if (!AllocateVideoBuffers()) {
-    // Error, We can not recover.
-    SetErrorState("Allocate buffer failed");
+    SetErrorState("Allocate buffers failed");
     return;
   }
 
   // Start UVC camera.
   v4l2_buf_type type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
-  if (HANDLE_EINTR(ioctl(device_fd_.get(), VIDIOC_STREAMON, &type)) == -1) {
+  if (Ioctl(device_fd_, VIDIOC_STREAMON, &type) == -1) {
     SetErrorState("VIDIOC_STREAMON failed");
     return;
   }
@@ -336,17 +342,16 @@ void VideoCaptureDeviceLinux::OnStopAndDeAllocate() {
   DCHECK_EQ(v4l2_thread_.message_loop(), base::MessageLoop::current());
 
   v4l2_buf_type type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
-  if (HANDLE_EINTR(ioctl(device_fd_.get(), VIDIOC_STREAMOFF, &type)) < 0) {
+  if (Ioctl(device_fd_, VIDIOC_STREAMOFF, &type) < 0) {
     SetErrorState("VIDIOC_STREAMOFF failed");
     return;
   }
-  // We don't dare to deallocate the buffers if we can't stop
-  // the capture device.
+  // We don't dare to deallocate the buffers if we can't stop the capture
+  // device.
   DeAllocateVideoBuffers();
 
-  // We need to close and open the device if we want to change the settings
-  // Otherwise VIDIOC_S_FMT will return error
-  // Sad but true.
+  // We need to close and open the device if we want to change the settings.
+  // Otherwise VIDIOC_S_FMT will return error. Sad but true.
   device_fd_.reset();
   is_capturing_ = false;
   client_.reset();
@@ -387,7 +392,7 @@ void VideoCaptureDeviceLinux::OnCaptureTask() {
     buffer.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
     buffer.memory = V4L2_MEMORY_MMAP;
     // Dequeue a buffer.
-    if (HANDLE_EINTR(ioctl(device_fd_.get(), VIDIOC_DQBUF, &buffer)) == 0) {
+    if (Ioctl(device_fd_, VIDIOC_DQBUF, &buffer) == 0) {
       client_->OnIncomingCapturedData(
           static_cast<uint8*>(buffer_pool_[buffer.index].start),
           buffer.bytesused,
@@ -396,7 +401,7 @@ void VideoCaptureDeviceLinux::OnCaptureTask() {
           base::TimeTicks::Now());
 
       // Enqueue the buffer again.
-      if (HANDLE_EINTR(ioctl(device_fd_.get(), VIDIOC_QBUF, &buffer)) == -1) {
+      if (Ioctl(device_fd_, VIDIOC_QBUF, &buffer) == -1) {
         SetErrorState(base::StringPrintf(
             "Failed to enqueue capture buffer errno %d", errno));
       }