Re: [Patch] Moving v4l1 ioctls from kernel to libv4l1

[Hans de Goede <hdegoede@redhat.com>]

Hi Huzaifa,

First of all many thanks for working on this!

Comments inline.

On 05/18/2010 01:12 PM, Huzaifa Sidhpurwala wrote:
> Hi All,
> I have been working with Hans for moving the v4l1 ioctls from the kernel
> to libv4l1.
> It would be best to work on one ioctl at a time.
> The enclosed patch attempts to implement VIDIOCGCAP in the libv4l1.
>
> Note: Hans is working with Bill, asking for permission to re-use the
> v4l1-compat.c code under the LGPL
>
>
>
> diff --git a/lib/libv4l1/libv4l1.c b/lib/libv4l1/libv4l1.c
> index eae3b43..8571651 100644
> --- a/lib/libv4l1/libv4l1.c
> +++ b/lib/libv4l1/libv4l1.c
> @@ -61,6 +61,10 @@
>   #define V4L1_PIX_FMT_TOUCHED    0x04
>   #define V4L1_PIX_SIZE_TOUCHED   0x08
>
> +#ifndef min
> +	#define min( a, b ) ( ((a)<  (b)) ? (a) : (b) )
> +#endif
> +
>   static pthread_mutex_t v4l1_open_mutex = PTHREAD_MUTEX_INITIALIZER;
>   static struct v4l1_dev_info devices[V4L1_MAX_DEVICES] = {
>   	{ .fd = -1 },
> @@ -130,6 +134,45 @@ static unsigned int pixelformat_to_palette(unsigned
> int pixelformat)
>   	return 0;
>   }
>
> +static int count_inputs(int fd)
> +{
> +	struct v4l2_input input2;
> +	int i;
> +	for (i = 0;; i++) {
> +		memset(&input2, 0, sizeof(input2));
> +		input2.index = i;
> +		if (0 != SYS_IOCTL(fd, VIDIOC_ENUMINPUT,&input2))
> +			break;
> +		}
> +	return i;
> +}
> +
> +static int check_size(int fd,int *maxw,int *maxh)
> +{
> +	struct v4l2_fmtdesc desc2;
> +	struct v4l2_format fmt2;
> +
> +	memset(&desc2, 0, sizeof(desc2));
> +	memset(&fmt2, 0, sizeof(fmt2));
> +
> +	desc2.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> +	if (0 != SYS_IOCTL(fd, VIDIOC_ENUM_FMT,&desc2))
> +		goto done;
> +
> +	fmt2.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> +	fmt2.fmt.pix.width = 10000;
> +	fmt2.fmt.pix.height = 10000;
> +	fmt2.fmt.pix.pixelformat = desc2.pixelformat;
> +	if (0 != SYS_IOCTL(fd, VIDIOC_TRY_FMT,&fmt2))
> +		goto done;
> +
> +	*maxw = fmt2.fmt.pix.width;
> +	*maxh = fmt2.fmt.pix.height;
> +
> +	done:
> +	return 0;
> +}
> +

We don't need this function, see below.

>   static int v4l1_set_format(int index, unsigned int width,
>   		unsigned int height, int v4l1_pal, int width_height_may_differ)
>   {
> @@ -492,7 +535,54 @@ int v4l1_ioctl(int fd, unsigned long int request, ...)
>   	case VIDIOCGCAP: {
>   		struct video_capability *cap = arg;
>
> -		result = SYS_IOCTL(fd, request, arg);
> +		long err;
> +		struct v4l2_framebuffer fbuf;
> +		struct v4l2_capability *cap2;

The kernel mallocs struct v4l2_capability because the kernel
stack is severely limited, no need to do that in userspace, just
put the entire struct on the stack.

> +
> +		cap2 = malloc (sizeof(*cap2));
> +		if (!cap2) {
> +			return -ENOMEM;
> +		}
> +
> +		memset(cap,0,sizeof(*cap));
> +		memset(&fbuf, 0, sizeof(fbuf));
> +

You can get the memset effect, by initalizing the first field of the
struct to 0, the compiler will then 0 out the rest:
		struct v4l2_framebuffer fbuf = { 0, };
		struct v4l2_capability cap2 =  { 0, };


> +		err = SYS_IOCTL(fd, VIDIOC_QUERYCAP,cap2);
> +		if (err<  0)
> +			goto done;
> +
> +		if (cap2->capabilities&  V4L2_CAP_VIDEO_OVERLAY) {
> +			err = SYS_IOCTL(fd, VIDIOC_G_FBUF,&fbuf);
> +			if (err<  0) {
> +				memset(&fbuf, 0, sizeof(fbuf));
> +			}
> +			err = 0;
> +		}
> +		
> +		memcpy(cap->name,  cap2->card,
> min(sizeof(cap->name),sizeof(cap2->card)) );
> +		
> +		cap->name[sizeof(cap->name) -1 ] = 0;
> +	
> +		if (cap2->capabilities&  V4L2_CAP_VIDEO_CAPTURE)
> +			cap->type |= VID_TYPE_CAPTURE;
> +		if (cap2->capabilities&  V4L2_CAP_TUNER)
> +			cap->type |= VID_TYPE_TUNER;
> +		if (cap2->capabilities&  V4L2_CAP_VBI_CAPTURE)
> +			cap->type |= VID_TYPE_TELETEXT;
> +		if (cap2->capabilities&  V4L2_CAP_VIDEO_OVERLAY)
> +			cap->type |= VID_TYPE_OVERLAY;
> +		if (fbuf.capability&  V4L2_FBUF_CAP_LIST_CLIPPING)
> +			cap->type |= VID_TYPE_CLIPPING;
> +
> +		cap->channels = count_inputs(fd);
> +
> +		check_size(fd,
> +			&cap->maxwidth,&cap->maxheight);
> +					

No need to check size here, as we override that below, so this line can
be dropped.

> +		
> +	/*	result = SYS_IOCTL(fd, request, arg);*/
> +

This can be dropped too.

> +
>
>   		/* override kernel v4l1 compat min / max size with our own more
>   		   accurate values */

And the override comment can be dropped to, as we no longer will be
suing the v4l1 compat code check_size stuff.

> @@ -500,7 +590,10 @@ int v4l1_ioctl(int fd, unsigned long int request, ...)
>   		cap->minheight = devices[index].min_height;
>   		cap->maxwidth  = devices[index].max_width;
>   		cap->maxheight = devices[index].max_height;
> -		break;
> +
> +		done:
> +			free(cap2);
> +			break;
>   	}

No need to do this either when we drop the malloc.

>
>   	case VIDIOCSPICT: {
>

Thanks & Regards,

Hans