[1/3] gspca pac7302/pac7311: simplify pac_find_sof

  From: Márton Németh <nm127@freemail.hu>

Remove struct sd dependency from pac_find_sof() function implementation.
This step prepares separation of pac7302 and pac7311 specific parts of
struct sd.

Signed-off-by: Márton Németh <nm127@freemail.hu>
Cc: Thomas Kaiser <thomas@kaiser-linux.li>
Cc: Theodore Kilgore <kilgota@auburn.edu>
Cc: Kyle Guinn <elyk03@gmail.com>
---
--
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 Sun, 1 Nov 2009, Németh Márton wrote:

> From: Márton Németh <nm127@freemail.hu>
>
> Remove struct sd dependency from pac_find_sof() function implementation.
> This step prepares separation of pac7302 and pac7311 specific parts of
> struct sd.
>
> Signed-off-by: Márton Németh <nm127@freemail.hu>
> Cc: Thomas Kaiser <thomas@kaiser-linux.li>
> Cc: Theodore Kilgore <kilgota@auburn.edu>
> Cc: Kyle Guinn <elyk03@gmail.com>

<snip>

Szia Marton,

As long as these things work, I would not mind at all. But perhaps this is 
a good occasion to bring up an issue which seems to me very much related. 
It is the following:

Along with the (it seems to be never-ending) work on the mr97310a driver, 
I have been working on a driver for the sn9c2028 cameras. The driver at 
this point functions, and seems to function quite well. But it also seems 
to me that the code needs quite a bit of polishing before it is 
publicized. Since I have been very much preoccupied with finishing the 
mr97310a driver (why does the last 5% of a job sometimes take the most 
time?) this final polishing of the sn9c2028 driver has not yet occurred, 
sorry to say.

But here is the point. The sn9c2028 cameras have a structure which seems 
similar to the mr97310a cameras. They use a similar decompression 
algorithm. They have a similar frame header. Specifically, the sn9c2028 
frame header starts with the five bytes

                 0xff, 0xff, 0x00, 0xc4, 0xc4

whereas the pac_common frame header starts with the five bytes

                 0xff, 0xff, 0x00, 0xff, 0x96

Right now, for my own use, I have written a file sn9c2028.h which 
essentially duplicates the functionality of pac_common.h and contains a 
function which searches for the sn9c2028 SOF marker instead of searching 
for the pac SOF marker. Is this necessarily the good, permanent solution? 
I am not so sure about that.

Perhaps when making changes it is a good time to think over the idea of 
combining things which are in fact not very much different. After all, 
another set of cameras might come along, too, which essentially requires 
yet another minor variation on the same basic algorithm. Then we are 
supposed to have three .h files with three functions which have the same 
code and just search for slightly different strings?

I am well aware that you started out to do something different, but how 
does this strike you?


Theodore Kilgore
  

Theodore Kilgore wrote:
> 
> On Sun, 1 Nov 2009, Németh Márton wrote:
>> Remove struct sd dependency from pac_find_sof() function implementation.
>> This step prepares separation of pac7302 and pac7311 specific parts of
>> struct sd.
> [...]
> But here is the point. The sn9c2028 cameras have a structure which seems 
> similar to the mr97310a cameras. They use a similar decompression 
> algorithm. They have a similar frame header. Specifically, the sn9c2028 
> frame header starts with the five bytes
> 
>                  0xff, 0xff, 0x00, 0xc4, 0xc4
> 
> whereas the pac_common frame header starts with the five bytes
> 
>                  0xff, 0xff, 0x00, 0xff, 0x96
> 
> Right now, for my own use, I have written a file sn9c2028.h which 
> essentially duplicates the functionality of pac_common.h and contains a 
> function which searches for the sn9c2028 SOF marker instead of searching 
> for the pac SOF marker. Is this necessarily the good, permanent solution? 
> I am not so sure about that.

I think the pac_find_sof() is a special case. To find a SOF sequence in
a bigger buffer in general needs to first analyze the SOF sequence for
repeated bytes. If there are repeated bytes the search have to be
continued in a different way, see the state machine currently in the
pac_common.h. To find the sn9c2028 frame header a different state machine
is needed. It might be possible to implement a search function which
can find any SOF sequence but I am afraid that this algorithm would be
too complicated because of the search string analysis.

> Perhaps when making changes it is a good time to think over the idea of 
> combining things which are in fact not very much different. After all, 
> another set of cameras might come along, too, which essentially requires 
> yet another minor variation on the same basic algorithm. Then we are 
> supposed to have three .h files with three functions which have the same 
> code and just search for slightly different strings?
> 
> I am well aware that you started out to do something different, but how 
> does this strike you?

I was also thinking about not just duplicate the code but find functions
which are similar. My thinking was that first I try to separate the
pac7302 and pac7311 subdrivers and get feedback. If this change was
accepted I would look for common functions not only in pac7302 and pac7311
but also in the gspca family of subdrivers.

My first candidate would be the low level reg_w*() and reg_r*() functions.
I haven't finished the analysis but it seems that most of the time the
usb_control_msg() parameters are the same except the request and
requesttype parameter. The request contains a number specific to the
device. The requesttype usually contains USB_RECIP_DEVICE or
USB_RECIP_INTERFACE. This means that these function can be extracted
to a common helper module or to gspca_main and introduce the request
and requesttype values somehow to struct sd_desc in gspca.h.

Regards,

	Márton Németh

--
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 Mon, 2 Nov 2009, Németh Márton wrote:

> Theodore Kilgore wrote:
>>
>> On Sun, 1 Nov 2009, Németh Márton wrote:
>>> Remove struct sd dependency from pac_find_sof() function implementation.
>>> This step prepares separation of pac7302 and pac7311 specific parts of
>>> struct sd.
>> [...]
>> But here is the point. The sn9c2028 cameras have a structure which seems
>> similar to the mr97310a cameras. They use a similar decompression
>> algorithm. They have a similar frame header. Specifically, the sn9c2028
>> frame header starts with the five bytes
>>
>>                  0xff, 0xff, 0x00, 0xc4, 0xc4
>>
>> whereas the pac_common frame header starts with the five bytes
>>
>>                  0xff, 0xff, 0x00, 0xff, 0x96
>>
>> Right now, for my own use, I have written a file sn9c2028.h which
>> essentially duplicates the functionality of pac_common.h and contains a
>> function which searches for the sn9c2028 SOF marker instead of searching
>> for the pac SOF marker. Is this necessarily the good, permanent solution?
>> I am not so sure about that.
>
> I think the pac_find_sof() is a special case. To find a SOF sequence in
> a bigger buffer in general needs to first analyze the SOF sequence for
> repeated bytes. If there are repeated bytes the search have to be
> continued in a different way, see the state machine currently in the
> pac_common.h. To find the sn9c2028 frame header a different state machine
> is needed. It might be possible to implement a search function which
> can find any SOF sequence but I am afraid that this algorithm would be
> too complicated because of the search string analysis.

Well, I do not really know enough about this to be able to say something 
authoritative, but:

1. There is an obvious limitation on the length of the SOF marker. If it 
is agreed upon that the SOF marker is 5 bytes or less, then it ought not 
to be so terrible a thing to do. Namely, your state machine should accept 
an input, consisting of a pointer to the proper SOF marker and use that 
one instead of what is "hard wired" in your code. So, for example,

                 switch (sd->sof_read) {
                 case 0:
                         if (m[i] == 0xff)
                                 sd->sof_read = 1;
                         break;
                 case 1:
                         if (m[i] == 0xff)
                                 sd->sof_read = 2;
                         else
                                 sd->sof_read = 0;
                         break;
 			(and so on)

could read instead as

                 switch (sd->sof_read) {
                 case 0:
                         if (m[i] == sof_marker[0])
                                 sd->sof_read = 1;
                         break;
                 case 1:
                         if (m[i] == sof_marker[1])
                                 sd->sof_read = 2;
                         else
                                 sd->sof_read = 0;
                         break;
 			(and so on)


The problem would come if the SOF marker were six bytes long instead. The 
way to solve that would be to figure out what is the longest SOF marker 
that one wants to deal with, beforehand. I am not sure what is the 
prevailing number of bytes in such an SOF marker, or the maximum number. 
But it would be possible to prescribe some reasonable maximum number 
and take that into account, I think.

2. Although it seems to me it would not be a terribly big deal to code the 
above, there is the question whether it would significantly slow the 
algorithm down to be required to go and read the variable sof_marker[]. My 
instinct is that it would cost practically nothing to do that, but I am 
not the expert on such matters, which is why I put the disclaimer at the 
top. If somebody who knows what he is talking about says this idea causes 
a speed or timing problem, then forget about it. In that case it is better 
to have another function for every camera.

Theodore Kilgore
  

Theodore Kilgore írta:
> 
> On Mon, 2 Nov 2009, Németh Márton wrote:
> 
>> Theodore Kilgore wrote:
>>> On Sun, 1 Nov 2009, Németh Márton wrote:
>>>> Remove struct sd dependency from pac_find_sof() function implementation.
>>>> This step prepares separation of pac7302 and pac7311 specific parts of
>>>> struct sd.
>>> [...]
>>> But here is the point. The sn9c2028 cameras have a structure which seems
>>> similar to the mr97310a cameras. They use a similar decompression
>>> algorithm. They have a similar frame header. Specifically, the sn9c2028
>>> frame header starts with the five bytes
>>>
>>>                  0xff, 0xff, 0x00, 0xc4, 0xc4
>>>
>>> whereas the pac_common frame header starts with the five bytes
>>>
>>>                  0xff, 0xff, 0x00, 0xff, 0x96
>>>
>>> Right now, for my own use, I have written a file sn9c2028.h which
>>> essentially duplicates the functionality of pac_common.h and contains a
>>> function which searches for the sn9c2028 SOF marker instead of searching
>>> for the pac SOF marker. Is this necessarily the good, permanent solution?
>>> I am not so sure about that.
>> I think the pac_find_sof() is a special case. To find a SOF sequence in
>> a bigger buffer in general needs to first analyze the SOF sequence for
>> repeated bytes. If there are repeated bytes the search have to be
>> continued in a different way, see the state machine currently in the
>> pac_common.h. To find the sn9c2028 frame header a different state machine
>> is needed. It might be possible to implement a search function which
>> can find any SOF sequence but I am afraid that this algorithm would be
>> too complicated because of the search string analysis.
> 
> Well, I do not really know enough about this to be able to say something 
> authoritative, but:
> 
> 1. There is an obvious limitation on the length of the SOF marker. If it 
> is agreed upon that the SOF marker is 5 bytes or less, then it ought not 
> to be so terrible a thing to do. Namely, your state machine should accept 
> an input, consisting of a pointer to the proper SOF marker and use that 
> one instead of what is "hard wired" in your code. So, for example,
> 
>                  switch (sd->sof_read) {
>                  case 0:
>                          if (m[i] == 0xff)
>                                  sd->sof_read = 1;
>                          break;
>                  case 1:
>                          if (m[i] == 0xff)
>                                  sd->sof_read = 2;
>                          else
>                                  sd->sof_read = 0;
>                          break;
>  			(and so on)
> 
> could read instead as
> 
>                  switch (sd->sof_read) {
>                  case 0:
>                          if (m[i] == sof_marker[0])
>                                  sd->sof_read = 1;
>                          break;
>                  case 1:
>                          if (m[i] == sof_marker[1])
>                                  sd->sof_read = 2;
>                          else
>                                  sd->sof_read = 0;
>                          break;
>  			(and so on)
> 
> 
> The problem would come if the SOF marker were six bytes long instead. The 
> way to solve that would be to figure out what is the longest SOF marker 
> that one wants to deal with, beforehand. I am not sure what is the 
> prevailing number of bytes in such an SOF marker, or the maximum number. 
> But it would be possible to prescribe some reasonable maximum number 
> and take that into account, I think.

I am afraid you missed an important point: the state machine depends on the
*contents* of the SOF marker:

From pac_common.h:

   The following state machine finds the SOF marker sequence
   0xff, 0xff, 0x00, 0xff, 0x96 in a byte stream.

           +----------+
           | 0: START |<---------------\
           +----------+<-\             |
             |       \---/otherwise    |
             v 0xff                    |
           +----------+ otherwise      |
           |     1    |--------------->*
           |          |                ^
           +----------+                |
             |                         |
             v 0xff                    |
           +----------+<-\0xff         |
        /->|          |--/             |
        |  |     2    |--------------->*
        |  |          | otherwise      ^
        |  +----------+                |
        |    |                         |
        |    v 0x00                    |
        |  +----------+                |
        |  |     3    |                |
        |  |          |--------------->*
        |  +----------+ otherwise      ^
        |    |                         |
   0xff |    v 0xff                    |
        |  +----------+                |
        \--|     4    |                |
           |          |----------------/
           +----------+ otherwise
             |
             v 0x96
           +----------+
           |  FOUND   |
           +----------+

Please have a closer look of the transients 2->2 and
4->2. They heavily depend on the 0xff bytes found in the
SOF marker. You might also want to have a look at the attached
test cases to see why the state machine is necessary.

Regards,

	Márton Németh
  

On Mon, 2 Nov 2009, Németh Márton wrote:

> Theodore Kilgore írta:
>>
>> On Mon, 2 Nov 2009, Németh Márton wrote:
>>
>>> Theodore Kilgore wrote:
>>>> On Sun, 1 Nov 2009, Németh Márton wrote:
>>>>> Remove struct sd dependency from pac_find_sof() function implementation.
>>>>> This step prepares separation of pac7302 and pac7311 specific parts of
>>>>> struct sd.
>>>> [...]
>>>> But here is the point. The sn9c2028 cameras have a structure which seems
>>>> similar to the mr97310a cameras. They use a similar decompression
>>>> algorithm. They have a similar frame header. Specifically, the sn9c2028
>>>> frame header starts with the five bytes
>>>>
>>>>                  0xff, 0xff, 0x00, 0xc4, 0xc4
>>>>
>>>> whereas the pac_common frame header starts with the five bytes
>>>>
>>>>                  0xff, 0xff, 0x00, 0xff, 0x96
>>>>
>>>> Right now, for my own use, I have written a file sn9c2028.h which
>>>> essentially duplicates the functionality of pac_common.h and contains a
>>>> function which searches for the sn9c2028 SOF marker instead of searching
>>>> for the pac SOF marker. Is this necessarily the good, permanent solution?
>>>> I am not so sure about that.
>>> I think the pac_find_sof() is a special case. To find a SOF sequence in
>>> a bigger buffer in general needs to first analyze the SOF sequence for
>>> repeated bytes. If there are repeated bytes the search have to be
>>> continued in a different way, see the state machine currently in the
>>> pac_common.h. To find the sn9c2028 frame header a different state machine
>>> is needed. It might be possible to implement a search function which
>>> can find any SOF sequence but I am afraid that this algorithm would be
>>> too complicated because of the search string analysis.
>>
>> Well, I do not really know enough about this to be able to say something
>> authoritative, but:
>>
>> 1. There is an obvious limitation on the length of the SOF marker. If it
>> is agreed upon that the SOF marker is 5 bytes or less, then it ought not
>> to be so terrible a thing to do. Namely, your state machine should accept
>> an input, consisting of a pointer to the proper SOF marker and use that
>> one instead of what is "hard wired" in your code. So, for example,
>>
>>                  switch (sd->sof_read) {
>>                  case 0:
>>                          if (m[i] == 0xff)
>>                                  sd->sof_read = 1;
>>                          break;
>>                  case 1:
>>                          if (m[i] == 0xff)
>>                                  sd->sof_read = 2;
>>                          else
>>                                  sd->sof_read = 0;
>>                          break;
>>  			(and so on)
>>
>> could read instead as
>>
>>                  switch (sd->sof_read) {
>>                  case 0:
>>                          if (m[i] == sof_marker[0])
>>                                  sd->sof_read = 1;
>>                          break;
>>                  case 1:
>>                          if (m[i] == sof_marker[1])
>>                                  sd->sof_read = 2;
>>                          else
>>                                  sd->sof_read = 0;
>>                          break;
>>  			(and so on)
>>
>>
>> The problem would come if the SOF marker were six bytes long instead. The
>> way to solve that would be to figure out what is the longest SOF marker
>> that one wants to deal with, beforehand. I am not sure what is the
>> prevailing number of bytes in such an SOF marker, or the maximum number.
>> But it would be possible to prescribe some reasonable maximum number
>> and take that into account, I think.
>
> I am afraid you missed an important point: the state machine depends on the
> *contents* of the SOF marker:
>
>> From pac_common.h:
>
>   The following state machine finds the SOF marker sequence
>   0xff, 0xff, 0x00, 0xff, 0x96 in a byte stream.
>
>           +----------+
>           | 0: START |<---------------\
>           +----------+<-\             |
>             |       \---/otherwise    |
>             v 0xff                    |
>           +----------+ otherwise      |
>           |     1    |--------------->*
>           |          |                ^
>           +----------+                |
>             |                         |
>             v 0xff                    |
>           +----------+<-\0xff         |
>        /->|          |--/             |
>        |  |     2    |--------------->*
>        |  |          | otherwise      ^
>        |  +----------+                |
>        |    |                         |
>        |    v 0x00                    |
>        |  +----------+                |
>        |  |     3    |                |
>        |  |          |--------------->*
>        |  +----------+ otherwise      ^
>        |    |                         |
>   0xff |    v 0xff                    |
>        |  +----------+                |
>        \--|     4    |                |
>           |          |----------------/
>           +----------+ otherwise
>             |
>             v 0x96
>           +----------+
>           |  FOUND   |
>           +----------+
>
> Please have a closer look of the transients 2->2 and
> 4->2. They heavily depend on the 0xff bytes found in the
> SOF marker.

Yes, on second look I see what you mean. Your routine is built around the 
_specific_ contents of the frame header. If that makes it work with fewer 
errors, then that is the end of the discussion.

Theodore Kilgore