[Patch] Moving v4l1 ioctls from kernel to libv4l1

[Patch] Moving v4l1 ioctls from kernel to libv4l1

[Huzaifa Sidhpurwala]

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;
+}
+
 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;
+
+		cap2 = malloc (sizeof(*cap2));
+		if (!cap2) {
+			return -ENOMEM;
+		}
+
+		memset(cap,0,sizeof(*cap));
+		memset(&fbuf, 0, sizeof(fbuf));
+
+		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);
+					
+		
+	/*	result = SYS_IOCTL(fd, request, arg);*/
+
+

 		/* override kernel v4l1 compat min / max size with our own more
 		   accurate values */
@@ -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;
 	}

 	case VIDIOCSPICT: {

-- 
Regards,
Huzaifa Sidhpurwala, RHCE, CCNA (IRC: huzaifas)
IT Desktop R&D Lead.
Global Help Desk, Pune (India)
Phone: +91 20 4005 7322 (UTC +5.5)

GnuPG Fingerprint:
3A0F DAFB 9279 02ED 273B FFE9 CC70 DCF2 DA5B DAE5

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

[Hans de Goede]

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

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

[Huzaifa Sidhpurwala]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Thanks a lot for the review, here is the improved version.


diff --git a/lib/libv4l1/libv4l1.c b/lib/libv4l1/libv4l1.c
index eae3b43..9ef9980 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,19 @@ 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 v4l1_set_format(int index, unsigned int width,
 		unsigned int height, int v4l1_pal, int width_height_may_differ)
 {
@@ -491,16 +508,46 @@ int v4l1_ioctl(int fd, unsigned long int request, ...)
 	switch (request) {
 	case VIDIOCGCAP: {
 		struct video_capability *cap = arg;
+		
+		struct v4l2_framebuffer fbuf = { 0, };
+		struct v4l2_capability cap2 =  { 0, };

- -		result = SYS_IOCTL(fd, request, arg);
+		result = SYS_IOCTL(fd, VIDIOC_QUERYCAP,&cap2);
+		if (result < 0)
+			break;
+
+		if (cap2.capabilities & V4L2_CAP_VIDEO_OVERLAY) {
+			result = SYS_IOCTL(fd, VIDIOC_G_FBUF, &fbuf);
+			if (result < 0) {
+				memset(&fbuf, 0, sizeof(fbuf));
+			}
+			result = 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);

- -		/* override kernel v4l1 compat min / max size with our own more
- -		   accurate values */
 		cap->minwidth  = devices[index].min_width;
 		cap->minheight = devices[index].min_height;
 		cap->maxwidth  = devices[index].max_width;
 		cap->maxheight = devices[index].max_height;
+
 		break;
+
 	}

 	case VIDIOCSPICT: {




Hans de Goede wrote:
> 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


- --
Regards,
Huzaifa Sidhpurwala, RHCE, CCNA (IRC: huzaifas)
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.5 (GNU/Linux)
Comment: Using GnuPG with Red Hat - http://enigmail.mozdev.org/

iD8DBQFL86S5zHDc8tpb2uURAkJHAJ4vMGhdJNcgD9rFaZUtMqQvo80KsACfbSuv
KrcNL6YjjUi7kchPHJ8gV84=
=aRtx
-----END PGP SIGNATURE-----

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

[Hans de Goede]

Hi,

Looks good. I've committed this to my tree (I did some coding style
cleanups and added the permission mails received so far for re-using
the kernel code in the LGPL libv4l1), see:
http://git.linuxtv.org/v4l-utils.git?a=commitdiff;h=80e71ff2881617be317301a30d85a9b7f038aaca

Some requests for future patches:
1) please use git send-email for sending patches, including revisions
of patches. your mail clients mangles patches
("-" at the beginning of a line becomes "- -", line wraps them too).

2) please run checkpatch.pl from the kernel tree on your patches
before sending them, in the v4l-utils dir do:
git format-patch HEAD~
Then cd into a directory with extracted linux kernel source and do:
scripts/checkpatch.pl <path>/v4l-utils/0001-<patch-name>.patch

And fix any warnings and errors.

Thanks & Regards,

Hans





On 05/19/2010 10:43 AM, Huzaifa Sidhpurwala wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Thanks a lot for the review, here is the improved version.
>
>
> diff --git a/lib/libv4l1/libv4l1.c b/lib/libv4l1/libv4l1.c
> index eae3b43..9ef9980 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,19 @@ 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 v4l1_set_format(int index, unsigned int width,
>   		unsigned int height, int v4l1_pal, int width_height_may_differ)
>   {
> @@ -491,16 +508,46 @@ int v4l1_ioctl(int fd, unsigned long int request, ...)
>   	switch (request) {
>   	case VIDIOCGCAP: {
>   		struct video_capability *cap = arg;
> +		
> +		struct v4l2_framebuffer fbuf = { 0, };
> +		struct v4l2_capability cap2 =  { 0, };
>
> - -		result = SYS_IOCTL(fd, request, arg);
> +		result = SYS_IOCTL(fd, VIDIOC_QUERYCAP,&cap2);
> +		if (result<  0)
> +			break;
> +
> +		if (cap2.capabilities&  V4L2_CAP_VIDEO_OVERLAY) {
> +			result = SYS_IOCTL(fd, VIDIOC_G_FBUF,&fbuf);
> +			if (result<  0) {
> +				memset(&fbuf, 0, sizeof(fbuf));
> +			}
> +			result = 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);
>
> - -		/* override kernel v4l1 compat min / max size with our own more
> - -		   accurate values */
>   		cap->minwidth  = devices[index].min_width;
>   		cap->minheight = devices[index].min_height;
>   		cap->maxwidth  = devices[index].max_width;
>   		cap->maxheight = devices[index].max_height;
> +
>   		break;
> +
>   	}
>
>   	case VIDIOCSPICT: {
>
>
>
>
> Hans de Goede wrote:
>> 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
>
>
> - --
> Regards,
> Huzaifa Sidhpurwala, RHCE, CCNA (IRC: huzaifas)
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.5 (GNU/Linux)
> Comment: Using GnuPG with Red Hat - http://enigmail.mozdev.org/
>
> iD8DBQFL86S5zHDc8tpb2uURAkJHAJ4vMGhdJNcgD9rFaZUtMqQvo80KsACfbSuv
> KrcNL6YjjUi7kchPHJ8gV84=
> =aRtx
> -----END PGP SIGNATURE-----
> --
> 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