On Wed, 4 Mar 2009, Kyle Guinn wrote:
On Tuesday 03 March 2009 18:12:33 kilgota@xxxxxxxxxxxxxxxxxxxxxx wrote:
contents of file mr97310a.patch follow, for gspca/mr97310a.c
--------------------------------------------------------
--- mr97310a.c.old 2009-02-23 23:59:07.000000000 -0600
+++ mr97310a.c.new 2009-03-03 17:19:06.000000000 -0600
@@ -302,21 +302,9 @@ static void sd_pkt_scan(struct gspca_dev
data, n);
sd->header_read = 0;
gspca_frame_add(gspca_dev, FIRST_PACKET, frame, NULL, 0);
- 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;
+ /* keep the header, including sof marker, for coming frame */
+ len -= n;
+ data = sof - sizeof pac_sof_marker;;
}
gspca_frame_add(gspca_dev, INTER_PACKET, frame, data, len);
A few notes:
1. There is an extra semicolon on that last added line.
Oops. My bifocals.
2. sd->header_read no longer seems necessary.
This is very likely true.
3. If the SOF marker is split over two transfers then everything falls apart.
Are you sure about that?
I don't know if the camera will allow that to happen, but it's better to be
safe.
Agreed.
Note that this was a RFC. So if it is agreed that something needs to be
done, probably things should be put into a more formal patch structure.
Also I have a question of my own while thinking about this. It relates not
to the module but to the decompression code. What do we have over there?
Well,
void v4lconvert_decode_mr97310a(const unsigned char *inp, unsigned char
*outp,
int width, int height)
and then in my patch it does
/* remove the header */
inp += 12;
Perhaps this is not good, and what ought to be done instead is to "pass
off" the inp to an internal variable, and then increase it, instead.
Possibly an even better alternative exists. The easiest way, I think,
would be to take the two previous lines back out again, but
instead go back to the function
static inline unsigned char get_byte(const unsigned char *inp,
unsigned int bitpos)
{
const unsigned char *addr;
addr = inp + (bitpos >> 3);
return (addr[0] << (bitpos & 7)) | (addr[1] >> (8 - (bitpos &
7)));
}
and put the 12-byte shift into the line which computes addr, like this:
addr = inp + 12 + (bitpos >> 3);
or if one really wants to get fancy about it then give a
#define MR97310a_HEADERSIZE 12
at the top of the file and then one could say
addr = inp + (bitpos >> 3) + MR97310a_HEADERSIZE;
As I said, if doing any of these then one would leave strictly alone the
decoding function and any of its contents. I am not sure if messing with
the start of the inp variable is a good idea. Frankly, I do not know
enough details to be certain. But on second look my instincts are against
screwing with something like that. The thing that worries me is that what
exactly happens to those 12 bytes. Do they disappear down a black hole, or
what? For, in the end they will have to be deallocated somewhere. And
what then? The alternative which I give above would do what is needed and
also does not mess with the start location of inp.
Theodore Kilgore
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
