Add VideoCaptureDevice::GetDeviceSupportedFormats to interface + implementation for Linux and Fake

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 6e685c381fd83527bb3c5dbe935191d3a61dac2c..e64a4e85e5133ccc3741773e40937febdc6a7436 100644
--- a/media/video/capture/linux/video_capture_device_linux.cc
+++ b/media/video/capture/linux/video_capture_device_linux.cc
@@ -96,7 +96,7 @@ static bool HasUsableFormats(int fd) {
                   fmtdesc.pixelformat) != usable_fourccs.end())
       return true;
 
-    fmtdesc.index++;
+    ++fmtdesc.index;
   }
   return false;
 }
@@ -252,6 +252,80 @@ const VideoCaptureDevice::Name& VideoCaptureDeviceLinux::device_name() {
   return device_name_;
 }
 
+void VideoCaptureDeviceLinux::GetDeviceSupportedFormats(
+    VideoCaptureFormats* capture_formats) {

[perkj_chrome, 2013/09/13 10:33:05]

To be threadsafe - all v4l2 calls in this file is done on a separate thread
|v4l2_thread_| - this is since v4l2 require us to pull frames from a camera. 

You will need to decide on if you want to call this before or after Allocate- 
Allocate open the driver so you should not do it again.

Would it make more sence to move this code to OnAllocate  and let the supported
formats be returned in the observer interface? 
Also consider moving the requirement for a capture format to the Start method? 

[mcasas, 2013/09/24 15:02:27]

Done.

+

[scherkus (not reviewing), 2013/09/12 17:56:00]

remove blank line

[mcasas, 2013/09/24 15:02:27]

Done.

+  if (device_name_.id().empty())
+    return;
+  int fd = open(device_name_.id().c_str(), O_RDONLY);

[scherkus (not reviewing), 2013/09/12 17:56:00]

use file_util::ScopedFD

[scherkus (not reviewing), 2013/09/12 17:56:00]

this should be wrapped with HANDLE_EINTR()

[mcasas, 2013/09/24 15:02:27]

Done.

+  if (fd < 0)
+    return;

[scherkus (not reviewing), 2013/09/12 17:56:00]

DPLOG(ERROR) << "Couldn't open " << device_name_.id();

DPLOG will provide the posix last error message

[mcasas, 2013/09/24 15:02:27]

Done.

+
+  media::VideoCaptureCapability capture_format;

[scherkus (not reviewing), 2013/09/12 17:56:00]

remove media::

[mcasas, 2013/09/24 15:02:27]

Done.

+
+  v4l2_capability device;
+  // Test if this is a V4L2 capture device.
+  if ((ioctl(fd, VIDIOC_QUERYCAP, &device) == 0) &&
+      (device.capabilities & V4L2_CAP_VIDEO_CAPTURE) &&
+      !(device.capabilities & V4L2_CAP_VIDEO_OUTPUT)) {
+    // Retrieve the caps one by one, first get colorspace, then sizes, then

[scherkus (not reviewing), 2013/09/12 17:56:00]

instead of having this large block of code indented, check for the error
condition and return early

[mcasas, 2013/09/24 15:02:27]

Done.

+    // framerates. See http://linuxtv.org/downloads/v4l-dvb-apis for reference.
+    v4l2_fmtdesc pixel_format = {};
+    pixel_format.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
+    while (ioctl(fd, VIDIOC_ENUM_FMT, &pixel_format) == 0) {
+      capture_format.color = media::V4l2ColorToVideoCaptureColorFormat(

[scherkus (not reviewing), 2013/09/12 17:56:00]

remove media::

[scherkus (not reviewing), 2013/09/12 17:56:00]

do we have to cast here?

it seems we don't have to on line 418 and as far as I can tell the types are the
same

[mcasas, 2013/09/24 15:02:27]

Done.

[mcasas, 2013/09/24 15:02:27]

Done.

+          static_cast<int32>(pixel_format.pixelformat));
+
+      v4l2_frmsizeenum frame_size = {};
+      frame_size.pixel_format = pixel_format.pixelformat;
+      while (ioctl(fd, VIDIOC_ENUM_FRAMESIZES, &frame_size) == 0) {
+        if (frame_size.type == V4L2_FRMSIZE_TYPE_DISCRETE) {
+          capture_format.width = frame_size.discrete.width;
+          capture_format.height = frame_size.discrete.height;
+        } else if (frame_size.type == V4L2_FRMSIZE_TYPE_STEPWISE) {
+          // What to do here? We get a min-max pair for width and height, and a
+          // step for each dimensions. Is not a crazy format, ViVi uses it.
+          // TODO(mcasas) see crbug.com/249953

[scherkus (not reviewing), 2013/09/12 17:56:00]

nit: we format TODOs with a : after the )

also can you elaborate / provide a summary of what to do in addition to linking
to the bug?

[scherkus (not reviewing), 2013/09/12 17:56:00]

nit: use http:// in front of crbug.com (many text editors 'n web browsers will
link-ify the result when viewing the source)

[mcasas, 2013/09/24 15:02:27]

Done.

[mcasas, 2013/09/24 15:02:27]

Is an easy elaborate: support these devices :) 

+          NOTIMPLEMENTED();
+        } else if (frame_size.type == V4L2_FRMSIZE_TYPE_CONTINUOUS) {
+          // Same here as the case before: TODO(mcasas) see crbug.com/249953

[scherkus (not reviewing), 2013/09/12 17:56:00]

ditto

[mcasas, 2013/09/24 15:02:27]

Done.

+          NOTIMPLEMENTED();

[scherkus (not reviewing), 2013/09/12 17:56:00]

if these are unimplemented, do you mean to continue executing and insert a
result?

is there a use case for inserting a bunch of VideoCaptureCapabilities with 0
width/height?

[mcasas, 2013/09/24 15:02:27]

I don't know this particular case. The proposed use case for this function as a
whole is to add the getUserMedia call to decide if a particular resolution (and
frame rate etc) as requested by the user, is possible. Right now there is no
information at all coming from the device to support this decision. So if we get
any information out of it, it would be already an improvement. That was my
rationale for all these NOTIMPLEMENTED and let it continue, deferring the
semantics to higher levels...

+        }
+        v4l2_frmivalenum frame_interval = {};
+        frame_interval.pixel_format = pixel_format.pixelformat;
+        frame_interval.width = frame_size.discrete.width;
+        frame_interval.height = frame_size.discrete.height;
+        while (ioctl(fd, VIDIOC_ENUM_FRAMEINTERVALS, &frame_interval) == 0) {
+          if (frame_interval.type == V4L2_FRMIVAL_TYPE_DISCRETE) {
+            capture_format.frame_rate =
+                (frame_interval.discrete.numerator != 0)

[scherkus (not reviewing), 2013/09/12 17:56:00]

I'd just use an if () statement here -- the ?: operator isn't making things easy
to read in this case

[mcasas, 2013/09/24 15:02:27]

Done.

+                    ? (static_cast<float>(frame_interval.discrete.denominator) /
+                       static_cast<float>(frame_interval.discrete.numerator))
+                    : 0;
+
+          } else if (frame_interval.type == V4L2_FRMIVAL_TYPE_CONTINUOUS) {
+            // Same here as the case above: TODO(mcasas) see crbug.com/249953

[scherkus (not reviewing), 2013/09/12 17:56:00]

nit: try to start TODO comments at the beginning of a comment

// TODO(mcasa): Same here as above, see http://crbug.com/249953

[mcasas, 2013/09/24 15:02:27]

Done.

+            NOTIMPLEMENTED();
+            break;
+          } else if (frame_interval.type == V4L2_FRMIVAL_TYPE_STEPWISE) {
+            // Same here as the case before: TODO(mcasas) see crbug.com/249953
+            NOTIMPLEMENTED();

[scherkus (not reviewing), 2013/09/12 17:56:00]

ditto... is there a use case for inserting VideoCaptureCapabilities with an
undefined frame rate?

[mcasas, 2013/09/24 15:02:27]

This is an easier case: yes, some devices might not be willing to share the
video frame rates (think auto exposure types), but if width+height are present,
that'd be an improvement.

+            break;
+          }
+          capture_formats->insert(capture_formats->end(), capture_format);

[scherkus (not reviewing), 2013/09/12 17:56:00]

push_back() ?

[mcasas, 2013/09/24 15:02:27]

Done.

+          ++frame_interval.index;
+        }
+        ++frame_size.index;
+      }
+      ++pixel_format.index;
+    }
+  } else {
+    // The selected device is not video capture as we like it.
+    capture_formats->clear();
+  }
+  close(fd);
+}
+
 void VideoCaptureDeviceLinux::OnAllocate(int width,
                                          int height,
                                          int frame_rate,
@@ -292,7 +366,7 @@ void VideoCaptureDeviceLinux::OnAllocate(int width,
   std::list<int>::iterator best = v4l2_formats.end();
   while (ioctl(device_fd_, VIDIOC_ENUM_FMT, &fmtdesc) == 0) {
     best = std::find(v4l2_formats.begin(), best, fmtdesc.pixelformat);
-    fmtdesc.index++;
+    ++fmtdesc.index;
   }
 
   if (best == v4l2_formats.end()) {
@@ -449,7 +523,7 @@ void VideoCaptureDeviceLinux::OnCaptureTask() {
 
   // Check if select timeout.
   if (result == 0) {
-    timeout_count_++;
+    ++timeout_count_;
     if (timeout_count_ >= kContinuousTimeoutLimit) {
       SetErrorState(base::StringPrintf(
           "Continuous timeout %d times", timeout_count_));
@@ -510,7 +584,7 @@ bool VideoCaptureDeviceLinux::AllocateVideoBuffers() {
 
   // Map the buffers.
   buffer_pool_ = new Buffer[r_buffer.count];
-  for (unsigned int i = 0; i < r_buffer.count; i++) {
+  for (unsigned int i = 0; i < r_buffer.count; ++i) {
     v4l2_buffer buffer;
     memset(&buffer, 0, sizeof(buffer));
     buffer.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
@@ -542,7 +616,7 @@ void VideoCaptureDeviceLinux::DeAllocateVideoBuffers() {
     return;
 
   // Unmaps buffers.
-  for (int i = 0; i < buffer_pool_size_; i++) {
+  for (int i = 0; i < buffer_pool_size_; ++i) {
     munmap(buffer_pool_[i].start, buffer_pool_[i].length);
   }
   v4l2_requestbuffers r_buffer;