From e605a94e4093e1304b153ab1baf3e5ccef7562e8 Mon Sep 17 00:00:00 2001
From: "William M. Brack" <wbrack@mmm.com.hk>
Date: Tue, 25 Mar 2008 12:33:09 +0000
Subject: [PATCH] sys/v4l2/v4l2src_calls.c: Check whether the device supports
 setting the framerate before trying to set it and then po...

Original commit message from CVS:
Based on patch by: William M. Brack <wbrack at mmm com hk>
* sys/v4l2/v4l2src_calls.c: (fractions_are_equal),
(gst_v4l2src_set_capture):
Check whether the device supports setting the framerate before
trying to set it and then posting a warning or error if it doesn't
work (#516649, #520092). Also compare fractions more correctly.
---
 ChangeLog                | 10 +++++
 sys/v4l2/v4l2src_calls.c | 79 ++++++++++++++++++++++++++--------------
 2 files changed, 61 insertions(+), 28 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 8e5fea5e37..4a95cb1a6e 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,13 @@
+2008-03-25  Tim-Philipp Müller  <tim at centricular dot net>
+
+	Based on patch by: William M. Brack <wbrack at mmm com hk>
+
+	* sys/v4l2/v4l2src_calls.c: (fractions_are_equal),
+	  (gst_v4l2src_set_capture):
+	  Check whether the device supports setting the framerate before
+	  trying to set it and then posting a warning or error if it doesn't
+	  work (#516649, #520092). Also compare fractions more correctly.
+
 2008-03-23  Tim-Philipp Müller  <tim at centricular dot net>
 
 	* gst/goom/Makefile.am:
diff --git a/sys/v4l2/v4l2src_calls.c b/sys/v4l2/v4l2src_calls.c
index 9e6a95c029..b00d97de77 100644
--- a/sys/v4l2/v4l2src_calls.c
+++ b/sys/v4l2/v4l2src_calls.c
@@ -1111,6 +1111,19 @@ qbuf_failed:
 */
 }
 
+static gboolean
+fractions_are_equal (gint num1, gint den1, gint num2, gint den2)
+{
+  GValue fraction1 = { 0, }, fraction2 = {
+  0,};
+
+  g_value_init (&fraction1, GST_TYPE_FRACTION);
+  g_value_init (&fraction2, GST_TYPE_FRACTION);
+  gst_value_set_fraction (&fraction1, num1, den1);
+  gst_value_set_fraction (&fraction2, num2, den2);
+  /* we know we don't have to unset the values in this case */
+  return (gst_value_compare (&fraction1, &fraction2) == GST_VALUE_EQUAL);
+}
 
 /******************************************************
  * gst_v4l2src_set_capture():
@@ -1153,32 +1166,52 @@ gst_v4l2src_set_capture (GstV4l2Src * v4l2src, guint32 pixelformat,
   if (format.fmt.pix.pixelformat != pixelformat)
     goto invalid_pixelformat;
 
+  /* Is there a reason we require the caller to always specify a framerate? */
+  GST_LOG_OBJECT (v4l2src, "Desired framerate: %u/%u", fps_n, fps_d);
+
   memset (&stream, 0x00, sizeof (struct v4l2_streamparm));
   stream.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
   if (ioctl (fd, VIDIOC_G_PARM, &stream) < 0) {
     GST_ELEMENT_WARNING (v4l2src, RESOURCE, SETTINGS,
         (_("Could not get parameters on device '%s'"),
             v4l2src->v4l2object->videodev), GST_ERROR_SYSTEM);
-  } else {                      /* seems no point in SET, if can't get GET */
-    /* Note: V4L2 gives us the frame interval, we need the frame rate */
-    stream.parm.capture.timeperframe.numerator = fps_d;
-    stream.parm.capture.timeperframe.denominator = fps_n;
-
-    if (ioctl (fd, VIDIOC_S_PARM, &stream) < 0) {
-      GST_ELEMENT_WARNING (v4l2src, RESOURCE, SETTINGS,
-          (_("Could not set parameters on device '%s'"),
-              v4l2src->v4l2object->videodev), GST_ERROR_SYSTEM);
-
-      /* FIXME: better test for fraction equality */
-      if (stream.parm.capture.timeperframe.numerator != fps_d
-          || stream.parm.capture.timeperframe.denominator != fps_n)
-        goto invalid_framerate;
-    }
-
-    v4l2src->fps_d = fps_d;
-    v4l2src->fps_n = fps_n;
+    goto done;
   }
 
+  /* Note: V4L2 provides the frame interval, we have the frame rate */
+  if (fractions_are_equal (stream.parm.capture.timeperframe.numerator,
+          stream.parm.capture.timeperframe.denominator, fps_d, fps_n)) {
+    GST_LOG_OBJECT (v4l2src, "Desired framerate already set, nothing to do");
+    goto done;
+  }
+
+  /* We want to change the frame rate, so check whether we can. Some cheap USB
+   * cameras don't have the capability */
+  if ((stream.parm.capture.capability & V4L2_CAP_TIMEPERFRAME) == 0) {
+    GST_DEBUG_OBJECT (v4l2src, "Not setting framerate (not supported)");
+    goto done;
+  }
+
+  GST_LOG_OBJECT (v4l2src, "Setting framerate to %u/%u", fps_n, fps_d);
+
+  /* Note: V4L2 wants the frame interval, we have the frame rate */
+  stream.parm.capture.timeperframe.numerator = fps_d;
+  stream.parm.capture.timeperframe.denominator = fps_n;
+
+  /* some cheap USB cam's won't accept any change */
+  if (ioctl (fd, VIDIOC_S_PARM, &stream) < 0) {
+    GST_ELEMENT_WARNING (v4l2src, RESOURCE, SETTINGS,
+        (_("Video input device did not accept new frame rate setting.")),
+        GST_ERROR_SYSTEM);
+    goto done;
+  }
+
+  v4l2src->fps_n = fps_n;
+  v4l2src->fps_d = fps_d;
+  GST_INFO_OBJECT (v4l2src, "Set framerate to %u/%u", fps_n, fps_d);
+
+done:
+
   return TRUE;
 
   /* ERRORS */
@@ -1219,16 +1252,6 @@ invalid_pixelformat:
             GST_FOURCC_ARGS (format.fmt.pix.pixelformat)));
     return FALSE;
   }
-invalid_framerate:
-  {
-    GST_ELEMENT_ERROR (v4l2src, RESOURCE, SETTINGS,
-        (_("Device '%s' cannot capture at %d/%d frames per second"),
-            v4l2src->v4l2object->videodev, fps_n, fps_d),
-        ("Tried to capture at %d/%d fps, but device returned %d/%d fps",
-            fps_n, fps_d, stream.parm.capture.timeperframe.denominator,
-            stream.parm.capture.timeperframe.numerator));
-    return FALSE;
-  }
 }
 
 /******************************************************