[RFC] gspca: implement vidioc_enum_frameintervals

  Hi,

gspca does not implement vidioc_enum_frameintervals yet, so even if a
camera can support multiple frame rates (or frame intervals) there is
still no way to enumerate them from userspace.

The following is just a quick and dirty implementation to show the
problem and to have something to base the discussion on. In the patch
there is also a working example of use with the ov534 subdriver.

Someone with a better knowledge of gspca and v4l internals can suggest
better solutions.

The tests has been done using 'luvcview -L', the output before the
patch:
    $ luvcview -d /dev/video1 -L
    luvcview 0.2.6

    SDL information:
      Video driver: x11
      A window manager is available
    Device information:
      Device path:  /dev/video1
    { pixelformat = 'YUYV', description = 'YUYV' }
    { discrete: width = 320, height = 240 }
	    Time interval between frame:
    1/40, 1/30, { discrete: width = 640, height = 480 }
	    Time interval between frame:


And the output after it:
    $ luvcview -d /dev/video1 -L
    luvcview 0.2.6

    SDL information:
      Video driver: x11
      A window manager is available
    Device information:
      Device path:  /dev/video1
    { pixelformat = 'YUYV', description = 'YUYV' }
    { discrete: width = 320, height = 240 }
	    Time interval between frame: 1/125, 1/100, 1/75, 1/60, 1/50, 1/40, 1/30,
    { discrete: width = 640, height = 480 }
	    Time interval between frame: 1/60, 1/50, 1/40, 1/30, 1/15, 

Thanks,
   Antonio
  

Hi,

Antonio Ospite wrote:
> Hi,
> 
> gspca does not implement vidioc_enum_frameintervals yet, so even if a
> camera can support multiple frame rates (or frame intervals) there is
> still no way to enumerate them from userspace.
> 
> The following is just a quick and dirty implementation to show the
> problem and to have something to base the discussion on. In the patch
> there is also a working example of use with the ov534 subdriver.
> 
> Someone with a better knowledge of gspca and v4l internals can suggest
> better solutions.
> 
> The tests has been done using 'luvcview -L', the output before the
> patch:
>     $ luvcview -d /dev/video1 -L
>     luvcview 0.2.6
> 
>     SDL information:
>       Video driver: x11
>       A window manager is available
>     Device information:
>       Device path:  /dev/video1
>     { pixelformat = 'YUYV', description = 'YUYV' }
>     { discrete: width = 320, height = 240 }
> 	    Time interval between frame:
>     1/40, 1/30, { discrete: width = 640, height = 480 }
> 	    Time interval between frame:
> 
> 
> And the output after it:
>     $ luvcview -d /dev/video1 -L
>     luvcview 0.2.6
> 
>     SDL information:
>       Video driver: x11
>       A window manager is available
>     Device information:
>       Device path:  /dev/video1
>     { pixelformat = 'YUYV', description = 'YUYV' }
>     { discrete: width = 320, height = 240 }
> 	    Time interval between frame: 1/125, 1/100, 1/75, 1/60, 1/50, 1/40, 1/30,
>     { discrete: width = 640, height = 480 }
> 	    Time interval between frame: 1/60, 1/50, 1/40, 1/30, 1/15, 
> 
> Thanks,
>    Antonio
> 
> diff -r 182b5f8fa160 linux/drivers/media/video/gspca/gspca.c
> --- a/linux/drivers/media/video/gspca/gspca.c	Sun Nov 15 10:05:30 2009 +0100
> +++ b/linux/drivers/media/video/gspca/gspca.c	Tue Nov 17 11:39:21 2009 +0100
> @@ -995,6 +995,37 @@
>  	return -EINVAL;
>  }
>  
> +static int vidioc_enum_frameintervals(struct file *filp, void *priv,
> +				      struct v4l2_frmivalenum *fival)
> +{
> +	struct gspca_dev *gspca_dev = priv;
> +	int mode = wxh_to_mode(gspca_dev, fival->width, fival->height);
> +	int i;
> +	__u32 index = 0;
> +
> +	if (gspca_dev->cam.mode_framerates == NULL ||
> +			gspca_dev->cam.mode_framerates[mode].nrates == 0)
> +		return -EINVAL;
> +
> +	/* FIXME: Needed? */
> +	if (fival->pixel_format !=
> +			gspca_dev->cam.cam_mode[mode].pixelformat)
> +		return -EINVAL;
> +
> +	for (i = 0; i < gspca_dev->cam.mode_framerates[mode].nrates; i++) {
> +		if (fival->index == index) {
> +			fival->type = V4L2_FRMSIZE_TYPE_DISCRETE;
> +			fival->discrete.numerator = 1;
> +			fival->discrete.denominator =
> +				gspca_dev->cam.mode_framerates[mode].rates[i];
> +			return 0;
> +		}
> +		index++;
> +	}
> +
> +	return -EINVAL;
> +}
> +
>  static void gspca_release(struct video_device *vfd)
>  {
>  	struct gspca_dev *gspca_dev = container_of(vfd, struct gspca_dev, vdev);
> @@ -1987,6 +2018,7 @@
>  	.vidioc_g_parm		= vidioc_g_parm,
>  	.vidioc_s_parm		= vidioc_s_parm,
>  	.vidioc_enum_framesizes = vidioc_enum_framesizes,
> +	.vidioc_enum_frameintervals = vidioc_enum_frameintervals,
>  #ifdef CONFIG_VIDEO_ADV_DEBUG
>  	.vidioc_g_register	= vidioc_g_register,
>  	.vidioc_s_register	= vidioc_s_register,
> diff -r 182b5f8fa160 linux/drivers/media/video/gspca/gspca.h
> --- a/linux/drivers/media/video/gspca/gspca.h	Sun Nov 15 10:05:30 2009 +0100
> +++ b/linux/drivers/media/video/gspca/gspca.h	Tue Nov 17 11:39:21 2009 +0100
> @@ -45,11 +45,17 @@
>  /* image transfers */
>  #define MAX_NURBS 4		/* max number of URBs */
>  
> +struct framerates {
> +	int *rates;
> +	int nrates;
> +};
> +
>  /* device information - set at probe time */
>  struct cam {
>  	int bulk_size;		/* buffer size when image transfer by bulk */
>  	const struct v4l2_pix_format *cam_mode;	/* size nmodes */
>  	char nmodes;
> +	const struct framerates *mode_framerates; /* size nmode, like cam_mode */
>  	__u8 bulk_nurbs;	/* number of URBs in bulk mode
>  				 * - cannot be > MAX_NURBS
>  				 * - when 0 and bulk_size != 0 means

I think that the best thing is to slip in two patches. One is related to the gspa main
and the other is a change on a driver.

> diff -r 182b5f8fa160 linux/drivers/media/video/gspca/ov534.c
> --- a/linux/drivers/media/video/gspca/ov534.c	Sun Nov 15 10:05:30 2009 +0100
> +++ b/linux/drivers/media/video/gspca/ov534.c	Tue Nov 17 11:39:21 2009 +0100
> @@ -287,6 +287,20 @@
>  	 .priv = 0},
>  };
>  
> +static int qvga_rates[] = {125, 100, 75, 60, 50, 40, 30};
> +static int vga_rates[] = {60, 50, 40, 30, 15};
> +
> +static const struct framerates ov772x_framerates[] = {
> +	{ /* 320x240 */
> +		.rates = qvga_rates,
> +		.nrates = ARRAY_SIZE(qvga_rates),
> +	},
> +	{ /* 640x480 */
> +		.rates = vga_rates,
> +		.nrates = ARRAY_SIZE(vga_rates),
> +	},
> +};
> +
>  static const struct v4l2_pix_format ov965x_mode[] = {
>  	{320, 240, V4L2_PIX_FMT_JPEG, V4L2_FIELD_NONE,
>  	 .bytesperline = 320,
> @@ -1411,6 +1425,7 @@
>  	if (sd->sensor == SENSOR_OV772X) {
>  		cam->cam_mode = ov772x_mode;
>  		cam->nmodes = ARRAY_SIZE(ov772x_mode);
> +		cam->mode_framerates = ov772x_framerates;
>  
>  		cam->bulk = 1;
>  		cam->bulk_size = 16384;
> 
> 

Michael
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

Hi,

On 11/17/2009 11:41 AM, Antonio Ospite wrote:
> Hi,
>
> gspca does not implement vidioc_enum_frameintervals yet, so even if a
> camera can support multiple frame rates (or frame intervals) there is
> still no way to enumerate them from userspace.
>
> The following is just a quick and dirty implementation to show the
> problem and to have something to base the discussion on. In the patch
> there is also a working example of use with the ov534 subdriver.
>
> Someone with a better knowledge of gspca and v4l internals can suggest
> better solutions.
>


Does the ov534 driver actually support selecting a framerate from the
list this patch adds, and does it then honor the selection ?

In my experience framerates with webcams are varying all the time, as
the lighting conditions change and the cam needs to change its exposure
setting to match, resulting in changed framerates.

So to me this does not seem very useful for webcams.

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

Hans de Goede wrote:
> Hi,
> 
> On 11/17/2009 11:41 AM, Antonio Ospite wrote:
>> Hi,
>>
>> gspca does not implement vidioc_enum_frameintervals yet, so even if a
>> camera can support multiple frame rates (or frame intervals) there is
>> still no way to enumerate them from userspace.
>>
>> The following is just a quick and dirty implementation to show the
>> problem and to have something to base the discussion on. In the patch
>> there is also a working example of use with the ov534 subdriver.
>>
>> Someone with a better knowledge of gspca and v4l internals can suggest
>> better solutions.
>>
> 
> 
> Does the ov534 driver actually support selecting a framerate from the
> list this patch adds, and does it then honor the selection ?

The ov534 is a bridge for different sensor like ov538 so it support different
frame rate, depends on the sensor too.

Michael
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

Hi,

On 11/19/2009 10:00 AM, Michael Trimarchi wrote:
> Hans de Goede wrote:
>> Hi,
>>
>> On 11/17/2009 11:41 AM, Antonio Ospite wrote:
>>> Hi,
>>>
>>> gspca does not implement vidioc_enum_frameintervals yet, so even if a
>>> camera can support multiple frame rates (or frame intervals) there is
>>> still no way to enumerate them from userspace.
>>>
>>> The following is just a quick and dirty implementation to show the
>>> problem and to have something to base the discussion on. In the patch
>>> there is also a working example of use with the ov534 subdriver.
>>>
>>> Someone with a better knowledge of gspca and v4l internals can suggest
>>> better solutions.
>>>
>>
>>
>> Does the ov534 driver actually support selecting a framerate from the
>> list this patch adds, and does it then honor the selection ?
>
> The ov534 is a bridge for different sensor like ov538 so it support
> different
> frame rate, depends on the sensor too.
>

That does not really answer my question. Does the current ov534 gspca
code allow one to set the framerate, and it will it then stay fixed
at that rate independend of the exposure setting of the sensor.

I know some uvc cams which do this, they basically send the same
frame multiple times to keep the framerate constant when the
exposure time raises above the time between frames, but most don't.

without such a feature advertising framerates makes no sense, as we
are advertising something we cannot deliver.

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

On Thu, 19 Nov 2009 09:08:22 +0100
Hans de Goede <hdegoede@redhat.com> wrote:

> Hi,
>

Hi, thanks for commenting on this.

> On 11/17/2009 11:41 AM, Antonio Ospite wrote:
> > Hi,
> >
> > gspca does not implement vidioc_enum_frameintervals yet, so even if a
> > camera can support multiple frame rates (or frame intervals) there is
> > still no way to enumerate them from userspace.
> >
> > The following is just a quick and dirty implementation to show the
> > problem and to have something to base the discussion on. In the patch
> > there is also a working example of use with the ov534 subdriver.
> >
> > Someone with a better knowledge of gspca and v4l internals can suggest
> > better solutions.
> >
> 
> 
> Does the ov534 driver actually support selecting a framerate from the
> list this patch adds, and does it then honor the selection ?
>

Yes it does, it can set framerates as per the list I added (in fact I
got the list looking at what the driver supports), and I can also see
it honors the framerate setting, from guvcview fps counter in the
capture window title. So only framerate enumeration is missing.

> In my experience framerates with webcams are varying all the time, as
> the lighting conditions change and the cam needs to change its exposure
> setting to match, resulting in changed framerates.
> 
> So to me this does not seem very useful for webcams.
>

As long as the chips involved (bridge, ISP, sensor) are powerful or
smart enough then the camera won't have problems.
I guess that for ov534/ov538 the usb bandwidth is the limiting factor
for the framerates, as we are using a raw format.

> Regards,
> 
> Hans

Btw, did you take a look at the patch anyway? Can you suggest a better
place where to put the structures needed for this functionality?

Regards,
   Antonio
  

Hi,

On 11/19/2009 11:37 AM, Antonio Ospite wrote:
> On Thu, 19 Nov 2009 09:08:22 +0100
> Hans de Goede<hdegoede@redhat.com>  wrote:
>
>> Hi,
>>
>
> Hi, thanks for commenting on this.
>
>> On 11/17/2009 11:41 AM, Antonio Ospite wrote:
>>> Hi,
>>>
>>> gspca does not implement vidioc_enum_frameintervals yet, so even if a
>>> camera can support multiple frame rates (or frame intervals) there is
>>> still no way to enumerate them from userspace.
>>>
>>> The following is just a quick and dirty implementation to show the
>>> problem and to have something to base the discussion on. In the patch
>>> there is also a working example of use with the ov534 subdriver.
>>>
>>> Someone with a better knowledge of gspca and v4l internals can suggest
>>> better solutions.
>>>
>>
>>
>> Does the ov534 driver actually support selecting a framerate from the
>> list this patch adds, and does it then honor the selection ?
>>
>
> Yes it does, it can set framerates as per the list I added (in fact I
> got the list looking at what the driver supports), and I can also see
> it honors the framerate setting, from guvcview fps counter in the
> capture window title. So only framerate enumeration is missing.
>
>>

Hmm, I see now upon expecting the code that the driver does actually
support setting the framerate, but what I see does not seem
to match your patch, the driver seems to support 50, 40, 30 and 15 fps,
where as your patch has:

static int qvga_rates[] = {125, 100, 75, 60, 50, 40, 30};
static int vga_rates[] = {60, 50, 40, 30, 15};


In my experience framerates with webcams are varying all the time, as
>> the lighting conditions change and the cam needs to change its exposure
>> setting to match, resulting in changed framerates.
>>
>> So to me this does not seem very useful for webcams.
>>
>
> As long as the chips involved (bridge, ISP, sensor) are powerful or
> smart enough then the camera won't have problems.
> I guess that for ov534/ov538 the usb bandwidth is the limiting factor
> for the framerates, as we are using a raw format.
>

Hmm, exposure could come in to play too, did you try high fps settings
in low light conditions?

Anyways as the driver clearly supports setting certain frame rates,
adding enumeration for them makes sense.


> Btw, did you take a look at the patch anyway? Can you suggest a better
> place where to put the structures needed for this functionality?
>

Yes I did, it looks ok, except I would change the following:
-in vidioc_enum_frameintervals() make i an __u32, and drop index
  (just compare i to fival->index)
-make the qvga_rates and vga_rates arrays const

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

On Thu, 19 Nov 2009 12:11:21 +0100
Hans de Goede <hdegoede@redhat.com> wrote:

> Hmm, I see now upon expecting the code that the driver does actually
> support setting the framerate, but what I see does not seem
> to match your patch, the driver seems to support 50, 40, 30 and 15
> fps, where as your patch has:
> 
> static int qvga_rates[] = {125, 100, 75, 60, 50, 40, 30};
> static int vga_rates[] = {60, 50, 40, 30, 15};

Hi,

The function set_frame_rate() is changed in my test repository.

Regards.