[PATCH] for the file gspca/mr97310a.c

[PATCH] for the file gspca/mr97310a.c

[kilgota]

First time ever that I mouse-copied an address and it gained a typo. 
Amazing. So trying again. The patch works better than the mouse, though. 
Guaranteed.

---------- Forwarded message ----------
Date: Thu, 5 Mar 2009 20:09:52 -0600 (CST)
From: kilgota@banach.math.auburn.edu
To: Hans de Goede <hdegoede@redhat.com>
Cc: Kyle Guinn <elyk03@gmail.com>, Jean-Francois Moine <moinejf@free.fr>,
     linux-mmedia@vger.kernel.org
Subject: [PATCH] for the file gspca/mr97310a.c


I just realized that the message below only went in one direction and did not 
have the proper title. So I fix that, now. The purpose of the patch has been 
extensively discussed in the thread seen in the title of the forwarded message. 
The patch below improves on the previous patch submitted for discussion, by 
fixing a bug in that one. The purpose of the patch is to save the header for 
the raw frames from the MR97310a cameras, which previously was not done. The 
patch achieves this result, and, when tested with two cameras, gives nice 
results.

A parallel patch for libv4lconvert/mr97310a.c was also presented in the RFC. 
Needless to say, it is needed simultaneously, before the output from the camera 
can be properly decompressed.

Theodore Kilgore

---------- Forwarded message ----------
Date: Thu, 5 Mar 2009 19:27:57 -0600 (CST)
From: kilgota@banach.math.auburn.edu
To: Hans de Goede <hdegoede@redhat.com>
Subject: Re: RFC on proposed patches to mr97310a.c for gspca and v4l



On Fri, 6 Mar 2009, Hans de Goede wrote:

> Well 2.6.29 is getting closer, so we need to be reasonable quick with the
> kernel side changes. As we do not want to change this after a kernel
> has been released with the current behaviour.
> 
> For libv4l we can take our time. But having a kernel patch ready soon
> would be good.

Well, it did not take as long as I thought. And, as far as the libv4lconvert 
change, you _do_ have a patch, right?

So, here is a patch for one file, namely for gspca/mr97310a.c. I hope that it 
will meet all objections.

Signed-off-by: Theodore Kilgore <kilgota@auburn.edu>
----------------------------------------------------------------------
--- mr97310a.c.old	2009-02-23 23:59:07.000000000 -0600
+++ mr97310a.c	2009-03-05 19:14:13.000000000 -0600
@@ -29,9 +29,7 @@ MODULE_LICENSE("GPL");
  /* specific webcam descriptor */
  struct sd {
  	struct gspca_dev gspca_dev;  /* !! must be the first item */
-
  	u8 sof_read;
-	u8 header_read;
  };

  /* V4L2 controls supported by the driver */
@@ -100,12 +98,9 @@ static int sd_init(struct gspca_dev *gsp

  static int sd_start(struct gspca_dev *gspca_dev)
  {
-	struct sd *sd = (struct sd *) gspca_dev;
  	__u8 *data = gspca_dev->usb_buf;
  	int err_code;

-	sd->sof_read = 0;
-
  	/* Note:  register descriptions guessed from MR97113A driver */

  	data[0] = 0x01;
@@ -285,40 +280,29 @@ static void sd_pkt_scan(struct gspca_dev
  			__u8 *data,                   /* isoc packet */
  			int len)                      /* iso packet length */
  {
-	struct sd *sd = (struct sd *) gspca_dev;
  	unsigned char *sof;

  	sof = pac_find_sof(gspca_dev, data, len);
  	if (sof) {
  		int n;
-
+		int marker_len = sizeof pac_sof_marker;
  		/* finish decoding current frame */
  		n = sof - data;
-		if (n > sizeof pac_sof_marker)
-			n -= sizeof pac_sof_marker;
+		if (n > marker_len)
+			n -= marker_len;
  		else
  			n = 0;
  		frame = gspca_frame_add(gspca_dev, LAST_PACKET, frame,
  					data, n);
-		sd->header_read = 0;
-		gspca_frame_add(gspca_dev, FIRST_PACKET, frame, NULL, 0);
-		len -= sof - data;
+		/* Start next frame. */
+		gspca_frame_add(gspca_dev, FIRST_PACKET, frame,
+			pac_sof_marker, marker_len);
+		len -= n;
+		len -= marker_len;
+		if (len < 0)
+			len = 0;
  		data = sof;
  	}
-	if (sd->header_read < 7) {
-		int needed;
-
-		/* skip the rest of the header */
-		needed = 7 - sd->header_read;
-		if (len <= needed) {
-			sd->header_read += len;
-			return;
-		}
-		data += needed;
-		len -= needed;
-		sd->header_read = 7;
-	}
-
  	gspca_frame_add(gspca_dev, INTER_PACKET, frame, data, len);
  }

@@ -337,6 +321,7 @@ static const struct sd_desc sd_desc = {
  /* -- module initialisation -- */
  static const __devinitdata struct usb_device_id device_table[] = {
  	{USB_DEVICE(0x08ca, 0x0111)},
+	{USB_DEVICE(0x093a, 0x010f)},
  	{}
  };
  MODULE_DEVICE_TABLE(usb, device_table);

Re: [PATCH] for the file gspca/mr97310a.c

[Kyle Guinn]

On Thursday 05 March 2009 20:34:27 kilgota@banach.math.auburn.edu wrote:
> Signed-off-by: Theodore Kilgore <kilgota@auburn.edu>
> ----------------------------------------------------------------------
> --- mr97310a.c.old	2009-02-23 23:59:07.000000000 -0600
> +++ mr97310a.c	2009-03-05 19:14:13.000000000 -0600
> @@ -29,9 +29,7 @@ MODULE_LICENSE("GPL");
>   /* specific webcam descriptor */
>   struct sd {
>   	struct gspca_dev gspca_dev;  /* !! must be the first item */
> -
>   	u8 sof_read;
> -	u8 header_read;
>   };
>
>   /* V4L2 controls supported by the driver */
> @@ -100,12 +98,9 @@ static int sd_init(struct gspca_dev *gsp
>
>   static int sd_start(struct gspca_dev *gspca_dev)
>   {
> -	struct sd *sd = (struct sd *) gspca_dev;
>   	__u8 *data = gspca_dev->usb_buf;
>   	int err_code;
>
> -	sd->sof_read = 0;
> -

Good catch, I didn't realize this was kzalloc'd.

>   	/* Note:  register descriptions guessed from MR97113A driver */
>
>   	data[0] = 0x01;
> @@ -285,40 +280,29 @@ static void sd_pkt_scan(struct gspca_dev
>   			__u8 *data,                   /* isoc packet */
>   			int len)                      /* iso packet length */
>   {
> -	struct sd *sd = (struct sd *) gspca_dev;
>   	unsigned char *sof;
>
>   	sof = pac_find_sof(gspca_dev, data, len);
>   	if (sof) {
>   		int n;
> -
> +		int marker_len = sizeof pac_sof_marker;

The value doesn't change; there's no need to use a variable for this.

>   		/* finish decoding current frame */
>   		n = sof - data;
> -		if (n > sizeof pac_sof_marker)
> -			n -= sizeof pac_sof_marker;
> +		if (n > marker_len)
> +			n -= marker_len;
>   		else
>   			n = 0;
>   		frame = gspca_frame_add(gspca_dev, LAST_PACKET, frame,
>   					data, n);
> -		sd->header_read = 0;
> -		gspca_frame_add(gspca_dev, FIRST_PACKET, frame, NULL, 0);
> -		len -= sof - data;
> +		/* Start next frame. */
> +		gspca_frame_add(gspca_dev, FIRST_PACKET, frame,
> +			pac_sof_marker, marker_len);
> +		len -= n;
> +		len -= marker_len;
> +		if (len < 0)
> +			len = 0;

len -= sof - data; is a shorter way to find the remaining length.

>   		data = sof;
>   	}
> -	if (sd->header_read < 7) {
> -		int needed;
> -
> -		/* skip the rest of the header */
> -		needed = 7 - sd->header_read;
> -		if (len <= needed) {
> -			sd->header_read += len;
> -			return;
> -		}
> -		data += needed;
> -		len -= needed;
> -		sd->header_read = 7;
> -	}
> -
>   	gspca_frame_add(gspca_dev, INTER_PACKET, frame, data, len);
>   }
>
> @@ -337,6 +321,7 @@ static const struct sd_desc sd_desc = {
>   /* -- module initialisation -- */
>   static const __devinitdata struct usb_device_id device_table[] = {
>   	{USB_DEVICE(0x08ca, 0x0111)},
> +	{USB_DEVICE(0x093a, 0x010f)},

This change is unrelated; maybe it should be in a different patch?  Don't 
forget to update Documentation/video4linux/gspca.txt with the new camera.

-Kyle

Re: [PATCH] for the file gspca/mr97310a.c

[kilgota]

On Thu, 5 Mar 2009, Kyle Guinn wrote:

> On Thursday 05 March 2009 20:34:27 kilgota@banach.math.auburn.edu wrote:
>> Signed-off-by: Theodore Kilgore <kilgota@auburn.edu>
>> ----------------------------------------------------------------------
>> --- mr97310a.c.old	2009-02-23 23:59:07.000000000 -0600
>> +++ mr97310a.c	2009-03-05 19:14:13.000000000 -0600
>> @@ -29,9 +29,7 @@ MODULE_LICENSE("GPL");
>>   /* specific webcam descriptor */
>>   struct sd {
>>   	struct gspca_dev gspca_dev;  /* !! must be the first item */
>> -
>>   	u8 sof_read;
>> -	u8 header_read;
>>   };
>>
>>   /* V4L2 controls supported by the driver */
>> @@ -100,12 +98,9 @@ static int sd_init(struct gspca_dev *gsp
>>
>>   static int sd_start(struct gspca_dev *gspca_dev)
>>   {
>> -	struct sd *sd = (struct sd *) gspca_dev;
>>   	__u8 *data = gspca_dev->usb_buf;
>>   	int err_code;
>>
>> -	sd->sof_read = 0;
>> -
>
> Good catch, I didn't realize this was kzalloc'd.

Hmmm. Perhaps I cut too much and _that_ should go back in. What if one 
stops the streaming and then restarts it? OTOH, one only risks losing one 
frame. OTTH, one might really want that frame. I will put it back.

>
>>   	/* Note:  register descriptions guessed from MR97113A driver */
>>
>>   	data[0] = 0x01;
>> @@ -285,40 +280,29 @@ static void sd_pkt_scan(struct gspca_dev
>>   			__u8 *data,                   /* isoc packet */
>>   			int len)                      /* iso packet length */
>>   {
>> -	struct sd *sd = (struct sd *) gspca_dev;
>>   	unsigned char *sof;
>>
>>   	sof = pac_find_sof(gspca_dev, data, len);
>>   	if (sof) {
>>   		int n;
>> -
>> +		int marker_len = sizeof pac_sof_marker;
>
> The value doesn't change; there's no need to use a variable for this.

True. I was just working for legibility, and trying to substitute a 
shorter symbol for something which is long and cumbersome and screws up 
80-character lines. If it is bad to do that, then I can take it right back 
out, of course.

>
>>   		/* finish decoding current frame */
>>   		n = sof - data;
>> -		if (n > sizeof pac_sof_marker)
>> -			n -= sizeof pac_sof_marker;
>> +		if (n > marker_len)
>> +			n -= marker_len;
>>   		else
>>   			n = 0;
>>   		frame = gspca_frame_add(gspca_dev, LAST_PACKET, frame,
>>   					data, n);
>> -		sd->header_read = 0;
>> -		gspca_frame_add(gspca_dev, FIRST_PACKET, frame, NULL, 0);
>> -		len -= sof - data;
>> +		/* Start next frame. */
>> +		gspca_frame_add(gspca_dev, FIRST_PACKET, frame,
>> +			pac_sof_marker, marker_len);
>> +		len -= n;
>> +		len -= marker_len;
>> +		if (len < 0)
>> +			len = 0;
>
> len -= sof - data; is a shorter way to find the remaining length.

Now, why did I try that and it did not work, but now it does? Weird. OK, 
you are right.

>
>>   		data = sof;
>>   	}
>> -	if (sd->header_read < 7) {
>> -		int needed;
>> -
>> -		/* skip the rest of the header */
>> -		needed = 7 - sd->header_read;
>> -		if (len <= needed) {
>> -			sd->header_read += len;
>> -			return;
>> -		}
>> -		data += needed;
>> -		len -= needed;
>> -		sd->header_read = 7;
>> -	}
>> -
>>   	gspca_frame_add(gspca_dev, INTER_PACKET, frame, data, len);
>>   }
>>
>> @@ -337,6 +321,7 @@ static const struct sd_desc sd_desc = {
>>   /* -- module initialisation -- */
>>   static const __devinitdata struct usb_device_id device_table[] = {
>>   	{USB_DEVICE(0x08ca, 0x0111)},
>> +	{USB_DEVICE(0x093a, 0x010f)},
>
> This change is unrelated; maybe it should be in a different patch?

Suspecting that the main business of this patch is more urgent, I will 
just take the other camera out, in that case. Then that is the next patch.

Don't
> forget to update Documentation/video4linux/gspca.txt with the new camera.

Interesting. Sometimes previous experience is not a good guide. What I 
learned is that one does not mess with someone else's stuff, 
without consulting with the person who is (at least informally) in charge 
of it, usually the person who wrote it. Example: In libgphoto2 I can add 
a camera driver any time I want, and I could in theory go and mess with 
any code anywhere in the tree, because I have commit privileges. But if 
someone else is specializing in X (for example X = project documentation) 
I will make suggestions to him, not just go and mess around in his work.

By analogy, I would not have even dreamed of going over into 
linux/Documentation and started to mess with the contents of gspca.txt. I 
am "over here" working on the code. So, gspca is a new project for me and 
I see that things are done differently.

So, what you are also reminding me, is that it is my duty to do that for 
the SQ905C cameras, too, which I did not do at all in the patch for them! 
Or is that a bit different because I did not hear anything back about that 
yet? I wonder. Perhaps what could happen is that the patch is not accepted 
because it is incomplete because I did not submit any patch for gspca.txt 
to add three USB ids and the name of the module, and I was not doing that 
because by previous experience I would have thought that gspca.txt is none 
of my business to go and edit, unless I am specifically asked. So perhaps 
I am caught with Catch 22.

Too late to finish this tonight. So, tomorrow.


Theodore Kilgore

Re: [PATCH] for the file gspca/mr97310a.c

[Hans de Goede]

kilgota@banach.math.auburn.edu wrote:
> 
> 
> On Thu, 5 Mar 2009, Kyle Guinn wrote:
> 
>> On Thursday 05 March 2009 20:34:27 kilgota@banach.math.auburn.edu wrote:
>>> Signed-off-by: Theodore Kilgore <kilgota@auburn.edu>
>>> ----------------------------------------------------------------------
>>> --- mr97310a.c.old    2009-02-23 23:59:07.000000000 -0600
>>> +++ mr97310a.c    2009-03-05 19:14:13.000000000 -0600
>>> @@ -29,9 +29,7 @@ MODULE_LICENSE("GPL");
>>>   /* specific webcam descriptor */
>>>   struct sd {
>>>       struct gspca_dev gspca_dev;  /* !! must be the first item */
>>> -
>>>       u8 sof_read;
>>> -    u8 header_read;
>>>   };
>>>
>>>   /* V4L2 controls supported by the driver */
>>> @@ -100,12 +98,9 @@ static int sd_init(struct gspca_dev *gsp
>>>
>>>   static int sd_start(struct gspca_dev *gspca_dev)
>>>   {
>>> -    struct sd *sd = (struct sd *) gspca_dev;
>>>       __u8 *data = gspca_dev->usb_buf;
>>>       int err_code;
>>>
>>> -    sd->sof_read = 0;
>>> -
>>
>> Good catch, I didn't realize this was kzalloc'd.
> 
> Hmmm. Perhaps I cut too much and _that_ should go back in. What if one 
> stops the streaming and then restarts it? OTOH, one only risks losing 
> one frame. OTTH, one might really want that frame. I will put it back.
> 

Ack I was about to make a comment along the same lines, please put it back in.

>>
>>>       /* Note:  register descriptions guessed from MR97113A driver */
>>>
>>>       data[0] = 0x01;
>>> @@ -285,40 +280,29 @@ static void sd_pkt_scan(struct gspca_dev
>>>               __u8 *data,                   /* isoc packet */
>>>               int len)                      /* iso packet length */
>>>   {
>>> -    struct sd *sd = (struct sd *) gspca_dev;
>>>       unsigned char *sof;
>>>
>>>       sof = pac_find_sof(gspca_dev, data, len);
>>>       if (sof) {
>>>           int n;
>>> -
>>> +        int marker_len = sizeof pac_sof_marker;
>>
>> The value doesn't change; there's no need to use a variable for this.
> 
> True. I was just working for legibility, and trying to substitute a 
> shorter symbol for something which is long and cumbersome and screws up 
> 80-character lines. If it is bad to do that, then I can take it right 
> back out, of course.
> 
>>
>>>           /* finish decoding current frame */
>>>           n = sof - data;
>>> -        if (n > sizeof pac_sof_marker)
>>> -            n -= sizeof pac_sof_marker;
>>> +        if (n > marker_len)
>>> +            n -= marker_len;
>>>           else
>>>               n = 0;
>>>           frame = gspca_frame_add(gspca_dev, LAST_PACKET, frame,
>>>                       data, n);
>>> -        sd->header_read = 0;
>>> -        gspca_frame_add(gspca_dev, FIRST_PACKET, frame, NULL, 0);
>>> -        len -= sof - data;
>>> +        /* Start next frame. */
>>> +        gspca_frame_add(gspca_dev, FIRST_PACKET, frame,
>>> +            pac_sof_marker, marker_len);
>>> +        len -= n;
>>> +        len -= marker_len;
>>> +        if (len < 0)
>>> +            len = 0;
>>
>> len -= sof - data; is a shorter way to find the remaining length.
> 
> Now, why did I try that and it did not work, but now it does? Weird. OK, 
> you are right.
> 

Actually, that is the right thing to do, as in cases where the frame only contained
the last part of a sof sequence, your code from the patch as send won't work.

<snip>

Regards,

Hans

Re: [PATCH] for the file gspca/mr97310a.c (revisions)

[kilgota]

Here is a revised version of the patch to the file gspca/mr97310a.c

Signed-off-by: Theodore Kilgore <kilgota@auburn.edu>
----------------------------------------------------------------------------
--- mr97310a.c.orig	2009-02-18 14:40:03.000000000 -0600
+++ mr97310a.c	2009-03-06 15:12:14.000000000 -0600
@@ -29,9 +29,7 @@ MODULE_LICENSE("GPL");
  /* specific webcam descriptor */
  struct sd {
  	struct gspca_dev gspca_dev;  /* !! must be the first item */
-
  	u8 sof_read;
-	u8 header_read;
  };

  /* V4L2 controls supported by the driver */
@@ -285,7 +283,6 @@ static void sd_pkt_scan(struct gspca_dev
  			__u8 *data,                   /* isoc packet */
  			int len)                      /* iso packet length */
  {
-	struct sd *sd = (struct sd *) gspca_dev;
  	unsigned char *sof;

  	sof = pac_find_sof(gspca_dev, data, len);
@@ -300,25 +297,12 @@ static void sd_pkt_scan(struct gspca_dev
  			n = 0;
  		frame = gspca_frame_add(gspca_dev, LAST_PACKET, frame,
  					data, n);
-		sd->header_read = 0;
-		gspca_frame_add(gspca_dev, FIRST_PACKET, frame, NULL, 0);
+		/* Start next frame. */
+		gspca_frame_add(gspca_dev, FIRST_PACKET, frame,
+			pac_sof_marker, sizeof pac_sof_marker);
  		len -= sof - data;
  		data = sof;
  	}
-	if (sd->header_read < 7) {
-		int needed;
-
-		/* skip the rest of the header */
-		needed = 7 - sd->header_read;
-		if (len <= needed) {
-			sd->header_read += len;
-			return;
-		}
-		data += needed;
-		len -= needed;
-		sd->header_read = 7;
-	}
-
  	gspca_frame_add(gspca_dev, INTER_PACKET, frame, data, len);
  }

A question about documentation.

[kilgota]

Recently, I submitted a driver for the SQ905C cameras, for which I (partly 
due to being new to kernel video support) did not provide any update to 
Documentation/video4linux/gspca.txt.

I have not heard what has happened to the driver. I assume that what is 
going on is there is a race condition about getting things into 2.6.29, 
and there is not time to do a proper evaluation. While I am curious about 
the actual problem, though, that is not my question here.

Someone pointed out to me that I probably should have updated the 
aforementioned doc file. So I do have a question about that.

Here is the way that gspca.txt looks now (excerpt follows):

List of the webcams known by gspca.

The modules are:
         gspca_main      main driver
         gspca_xxxx      subdriver module with xxxx as follows

xxxx            vend:prod
----
spca501         0000:0000       MystFromOri Unknow Camera
m5602           0402:5602       ALi Video Camera Controller
spca501         040a:0002       Kodak DVC-325
spca500         040a:0300       Kodak EZ200

(and so on)

Well, now comes the question. I notice that each line in the file 
corresponds to a USB Vendor:Product ID, and each line corresponds to one 
camera, by brand name and model. Apparently, there is some blessed 
universe in which I have not been living the last few years, in which 
there is such a one-to-one correspondence.

Below, I give the list of supported cameras. extracted from 
libgphoto2/camlibs/digigr8. which as far as I am able to know is 
supporting precisely the same set of cameras as does the module sq905c.c

My question is obvious: Which of them do I list? All of them? A sampling 
of them? Or what?

Theodore Kilgore

(list follows)

----------------------------------------------------

static const struct {
         char *name;
         CameraDriverStatus status;
         unsigned short idVendor;
         unsigned short idProduct;
} models[] = {
         {"Digigr8", 				0x2770, 0x905c},
         {"Che-Ez Snap SNAP-U", 			0x2770, 0x905c},
         {"DC-N130t", 				0x2770, 0x905C},
         {"Soundstar TDC-35", 			0x2770, 0x905c},
         {"Nexxtech Mini Digital Camera", 	0x2770, 0x905c},
 	{"Vivitar Vivicam35", 			0x2770, 0x905c},
         {"Praktica Slimpix", 			0x2770, 0x905c},
         {"ZINA Mini Digital Keychain Camera", 	0x2770, 0x905c},
         {"Pixie Princess Jelly-Soft", 		0x2770, 0x905c},
         {"Sakar Micro Digital 2428x", 		0x2770, 0x905c},
         {"Jazz JDC9", 				0x2770, 0x905c},
         {"Disney pix micro", 			0x2770, 0x9050},
         {"Suprema Digital Keychain Camera",	0x2770, 0x913d},
 	{"Sakar 28290 and 28292  Digital Concepts Styleshot",
                         			0x2770, 0x913d},
 	{"Sakar 23070  Crayola Digital Camera", 0x2770, 0x913d},
 	{"Sakar 92045  Spiderman", 		0x2770, 0x913d},
 	{NULL,0,0,0}
};

As I said, which one(s) of these to list?