RFC on proposed patches to mr97310a.c for gspca and v4l
Hans, Jean-Francois, and Kyle,
The proposed patches are not very long, so I will give each of them, with
my comments after each, to explain why I believe that these changes are a
good idea.
First, the patch to libv4lconvert is short and sweet:
contents of file mr97310av4l.patch follow
--
--- mr97310a.c.old 2009-03-01 15:37:38.0 -0600
+++ mr97310a.c.new 2009-02-18 22:39:48.0 -0600
@@ -102,6 +102,9 @@ void v4lconvert_decode_mr97310a(const un
if (!decoder_initialized)
init_mr97310a_decoder();
+ /* remove the header */
+ inp += 12;
+
bitpos = 0;
/* main decoding loop */
- here ends the v4lconvert patch --
The reason I want to do this should be obvious. It is to preserve the
entire header of each frame over in the gspca driver, and to throw it away
over here. The SOF marker FF FF 00 FF 96 is also kept. The reason why all
of this should be kept is that it makes it possible to look at a raw
output and to know if it is exactly aligned or not. Furthermore, the next
byte after the 96 is a code for the compression algorithm used, and the
bytes after that in the header might be useful in the future for better
image processing. In other words, these headers contain information which
might be useful in the future and they should not be jettisoned in the
kernel module.
Now, the kernel module ought to keep and send along the header and SOF
marker instead of throwing them away. This is the topic of the next patch.
It also has the virtue of simplifying and shortening the code in the
module at the same time, because one is not going through contortions to
skip over and throw away some data which ought to be kept anyway.
contents of file mr97310a.patch follow, for gspca/mr97310a.c
--- mr97310a.c.old 2009-02-23 23:59:07.0 -0600
+++ mr97310a.c.new 2009-03-03 17:19:06.0 -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);
@@ -337,6 +325,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);
end of mr97310a.patch -
You will also notice that I have added a USB ID. As I have mentioned, I
have four cameras with this ID. The story with them is that two of them
will not work at all. The module will not initialize the camera. As far as
the other two of them are concerned, the module and the accompanying
change in libv4lconvert work very well. I have mentioned this previously,
and I did not get any comment about what is good to do. So now I decided
to submit the ID number in the patch.
Theodore Kilgore
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: RFC on proposed patches to mr97310a.c for gspca and v4l
On Wednesday 04 March 2009 02:41:05 Thomas Kaiser wrote: > Hello Theodore > > kilg...@banach.math.auburn.edu wrote: > > Also, after the byte indicator for the compression algorithm there are > > some more bytes, and these almost definitely contain information which > > could be valuable while doing image processing on the output. If they > > are already kept and passed out of the module over to libv4lconvert, > > then it would be very easy to do something with those bytes if it is > > ever figured out precisely what they mean. But if it is not done now it > > would have to be done then and would cause even more trouble. > > I sent it already in private mail to you. Here is the observation I made > for the PAC207 SOF some years ago: > > From usb snoop. > FF FF 00 FF 96 64 xx 00 xx xx xx xx xx xx 00 00 > 1. xx: looks like random value > 2. xx: changed from 0x03 to 0x0b > 3. xx: changed from 0x06 to 0x49 > 4. xx: changed from 0x07 to 0x55 > 5. xx: static 0x96 > 6. xx: static 0x80 > 7. xx: static 0xa0 > > And I did play in Linux and could identify some fields :-) . > In Linux the header looks like this: > > FF FF 00 FF 96 64 xx 00 xx xx xx xx xx xx F0 00 > 1. xx: don't know but value is changing between 0x00 to 0x07 > 2. xx: this is the actual pixel clock > 3. xx: this is changing according light conditions from 0x03 (dark) to > 0xfc (bright) (center) > 4. xx: this is changing according light conditions from 0x03 (dark) to > 0xfc (bright) (edge) > 5. xx: set value "Digital Gain of Red" > 6. xx: set value "Digital Gain of Green" > 7. xx: set value "Digital Gain of Blue" > > Thomas I've been looking through the frame headers sent by the MR97310A (the Aiptek PenCam VGA+, 08ca:0111). Here are my observations. FF FF 00 FF 96 6x x0 xx xx xx xx xx In binary that looks something like this: 10010110 011001aa a101 All of the values look to be MSbit first. A looks like a 3-bit frame sequence number that seems to start with 1 and increments for each frame. B, C, and D might be brightness and contrast; minimum and maximum values for these vary with the image size. For 640x480, 320x240, and 160x120: dark scene (all black): B: near 0 C: 0x000 D: 0xC60 bright scene (all white): B: usually 0xC15C C: 0xC60 D: 0x000 For 352x288 and 176x144: dark scene (all black): B: near 0 C: 0x000 D: 0xB5B bright scene (all white): B: usually 0xB0FE C: 0xB53 D: 0x007 B increases with increasing brightness. C increases with more white pixels and D increases with more black pixels. A gray image has C and D near zero, while a high-contrast image (half black, half white) has C and D around 0x600 or so. The sum of C and D is not a constant. I don't know how brightness and contrast are handled in V4L. Hopefully someone can put this to use. -Kyle -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: RFC on proposed patches to mr97310a.c for gspca and v4l
On Thu, 16 Apr 2009, Kyle Guinn wrote: On Wednesday 04 March 2009 02:41:05 Thomas Kaiser wrote: Hello Theodore kilg...@banach.math.auburn.edu wrote: Also, after the byte indicator for the compression algorithm there are some more bytes, and these almost definitely contain information which could be valuable while doing image processing on the output. If they are already kept and passed out of the module over to libv4lconvert, then it would be very easy to do something with those bytes if it is ever figured out precisely what they mean. But if it is not done now it would have to be done then and would cause even more trouble. I sent it already in private mail to you. Here is the observation I made for the PAC207 SOF some years ago: From usb snoop. FF FF 00 FF 96 64 xx 00 xx xx xx xx xx xx 00 00 1. xx: looks like random value 2. xx: changed from 0x03 to 0x0b 3. xx: changed from 0x06 to 0x49 4. xx: changed from 0x07 to 0x55 5. xx: static 0x96 6. xx: static 0x80 7. xx: static 0xa0 And I did play in Linux and could identify some fields :-) . In Linux the header looks like this: FF FF 00 FF 96 64 xx 00 xx xx xx xx xx xx F0 00 1. xx: don't know but value is changing between 0x00 to 0x07 2. xx: this is the actual pixel clock 3. xx: this is changing according light conditions from 0x03 (dark) to 0xfc (bright) (center) 4. xx: this is changing according light conditions from 0x03 (dark) to 0xfc (bright) (edge) 5. xx: set value "Digital Gain of Red" 6. xx: set value "Digital Gain of Green" 7. xx: set value "Digital Gain of Blue" Thomas I've been looking through the frame headers sent by the MR97310A (the Aiptek PenCam VGA+, 08ca:0111). Here are my observations. FF FF 00 FF 96 6x x0 xx xx xx xx xx In binary that looks something like this: 10010110 011001aa a101 All of the values look to be MSbit first. A looks like a 3-bit frame sequence number that seems to start with 1 and increments for each frame. Hmmm. This I never noticed. What you are saying is that the two bytes 6x and x0 are variable? You are sure about that? What I have previously experienced is that the first is always 64 with these cameras, and the second one indicates the absence of compression (in which case it is 0, which of course only arises for still cameras), or if there is data compression then it is not zero. I have never seen this byte to change during a session with a camera. Here is a little table of what I have previously witnessed, and perhaps you can suggest what to do in order to see this is not happening: Camera that byte compression solved, or not streaming Aiptek 00 no N/A no Aiptek 50 yes yes both the Sakar cam 00 no N/A no ditto 50 yes yes both Argus QuikClix 20 yes no doesn't work Argus DC1620 50 yes yes doesn't work CIF cameras 00 no N/A no ditto 50 yes yes no ditto d0 yes no yes Other strange facts are -- that the Sakar camera, the Argus QuikClix, and the DC1620 all share the same USB ID of 0x93a:0x010f and yet only one of them will stream with the existing driver. The other two go through the motions, but the isoc packets do not actually get sent, so there is no image coming out. I do not understand the reason for this; I have been trying to figure it out and it is rather weird. I should add that, yes, those two cameras were said to be capable of streaming when I bought them. Could it be a problem of age? I don't expect that, but maybe. -- the CIF cameras all share the USB id of 0x93a:0x010e (I bought several of them) and they all are using a different compression algorithm while streaming from what they use if running as still cameras in compressed mode. This leads to the question whether it is possible to set the compression algorithm during the initialization sequence, so that the camera also uses the "0x50" mode while streaming, because we already know how to use that mode. But I have never seen the 0x64 0xX0 bytes used to count the frames. Could you tell me how to repeat that? It certainly would knock down the validity of the above table wouldn't it? B, C, and D might be brightness and contrast; minimum and maximum values for these vary with the image size. For 640x480, 320x240, and 160x120: dark scene (all black): B: near 0 C: 0x000 D: 0xC60 bright scene (all white): B: usually 0xC15C C: 0xC60 D: 0x000 For 352x288 and 176x144: dark scene (all black): B: near 0 C: 0x000 D: 0xB5B bright scene (all white): B: usually 0xB0FE C: 0xB53 D: 0x007 B increases with increasing brightness. C increases with more wh
Re: RFC on proposed patches to mr97310a.c for gspca and v4l
On Thursday 16 April 2009 13:22:11 Theodore Kilgore wrote: > On Thu, 16 Apr 2009, Kyle Guinn wrote: > > On Wednesday 04 March 2009 02:41:05 Thomas Kaiser wrote: > >> Hello Theodore > >> > >> kilg...@banach.math.auburn.edu wrote: > >>> Also, after the byte indicator for the compression algorithm there are > >>> some more bytes, and these almost definitely contain information which > >>> could be valuable while doing image processing on the output. If they > >>> are already kept and passed out of the module over to libv4lconvert, > >>> then it would be very easy to do something with those bytes if it is > >>> ever figured out precisely what they mean. But if it is not done now it > >>> would have to be done then and would cause even more trouble. > >> > >> I sent it already in private mail to you. Here is the observation I made > >> for the PAC207 SOF some years ago: > >> > >> From usb snoop. > >> FF FF 00 FF 96 64 xx 00 xx xx xx xx xx xx 00 00 > >> 1. xx: looks like random value > >> 2. xx: changed from 0x03 to 0x0b > >> 3. xx: changed from 0x06 to 0x49 > >> 4. xx: changed from 0x07 to 0x55 > >> 5. xx: static 0x96 > >> 6. xx: static 0x80 > >> 7. xx: static 0xa0 > >> > >> And I did play in Linux and could identify some fields :-) . > >> In Linux the header looks like this: > >> > >> FF FF 00 FF 96 64 xx 00 xx xx xx xx xx xx F0 00 > >> 1. xx: don't know but value is changing between 0x00 to 0x07 > >> 2. xx: this is the actual pixel clock > >> 3. xx: this is changing according light conditions from 0x03 (dark) to > >> 0xfc (bright) (center) > >> 4. xx: this is changing according light conditions from 0x03 (dark) to > >> 0xfc (bright) (edge) > >> 5. xx: set value "Digital Gain of Red" > >> 6. xx: set value "Digital Gain of Green" > >> 7. xx: set value "Digital Gain of Blue" > >> > >> Thomas > > > > I've been looking through the frame headers sent by the MR97310A (the > > Aiptek PenCam VGA+, 08ca:0111). Here are my observations. > > > > FF FF 00 FF 96 6x x0 xx xx xx xx xx > > > > In binary that looks something like this: > > > > > > 10010110 011001aa a101 > > > > > > All of the values look to be MSbit first. A looks like a 3-bit frame > > sequence number that seems to start with 1 and increments for each frame. > > Hmmm. This I never noticed. What you are saying is that the two bytes 6x > and x0 are variable? You are sure about that? What I have previously > experienced is that the first is always 64 with these cameras, and the > second one indicates the absence of compression (in which case it is 0, > which of course only arises for still cameras), or if there is data > compression then it is not zero. I have never seen this byte to change > during a session with a camera. Here is a little table of what I have > previously witnessed, and perhaps you can suggest what to do in order to > see this is not happening: > > Camerathat byte compression solved, or not > streaming > Aiptek00 no N/A no > Aiptek50 yes yes both > the Sakar cam 00 no N/A no > ditto 50 yes yes both > Argus QuikClix20 yes no doesn't work > Argus DC1620 50 yes yes doesn't work > CIF cameras 00 no N/A no > ditto 50 yes yes no > ditto d0 yes no yes > > Other strange facts are > > -- that the Sakar camera, the Argus QuikClix, and the > DC1620 all share the same USB ID of 0x93a:0x010f and yet only one of them > will stream with the existing driver. The other two go through the > motions, but the isoc packets do not actually get sent, so there is no > image coming out. I do not understand the reason for this; I have been > trying to figure it out and it is rather weird. I should add that, yes, > those two cameras were said to be capable of streaming when I bought them. > Could it be a problem of age? I don't expect that, but maybe. > > -- the CIF cameras all share the USB id of 0x93a:0x010e (I bought several > of them) and they all are using a different compression algorithm while > streaming from what they use if running as still cameras in compressed > mode. This leads to the question whether it is possible to set the > compression algorithm during the initialization sequence, so that the > camera also uses the "0x50" mode while streaming, because we already know > how to use that mode. > > But I have never seen the 0x64 0xX0 bytes used to count the frames. Could > you tell me how to repeat that? It certainly would knock down the validity > of the above table wouldn't it? > I've modified libv4l to print out the 12-byte header before it skips over it.
Re: RFC on proposed patches to mr97310a.c for gspca and v4l
On Thu, 16 Apr 2009, Kyle Guinn wrote: On Thursday 16 April 2009 13:22:11 Theodore Kilgore wrote: On Thu, 16 Apr 2009, Kyle Guinn wrote: On Wednesday 04 March 2009 02:41:05 Thomas Kaiser wrote: Hello Theodore kilg...@banach.math.auburn.edu wrote: Also, after the byte indicator for the compression algorithm there are some more bytes, and these almost definitely contain information which could be valuable while doing image processing on the output. If they are already kept and passed out of the module over to libv4lconvert, then it would be very easy to do something with those bytes if it is ever figured out precisely what they mean. But if it is not done now it would have to be done then and would cause even more trouble. I sent it already in private mail to you. Here is the observation I made for the PAC207 SOF some years ago: From usb snoop. FF FF 00 FF 96 64 xx 00 xx xx xx xx xx xx 00 00 1. xx: looks like random value 2. xx: changed from 0x03 to 0x0b 3. xx: changed from 0x06 to 0x49 4. xx: changed from 0x07 to 0x55 5. xx: static 0x96 6. xx: static 0x80 7. xx: static 0xa0 And I did play in Linux and could identify some fields :-) . In Linux the header looks like this: FF FF 00 FF 96 64 xx 00 xx xx xx xx xx xx F0 00 1. xx: don't know but value is changing between 0x00 to 0x07 2. xx: this is the actual pixel clock 3. xx: this is changing according light conditions from 0x03 (dark) to 0xfc (bright) (center) 4. xx: this is changing according light conditions from 0x03 (dark) to 0xfc (bright) (edge) 5. xx: set value "Digital Gain of Red" 6. xx: set value "Digital Gain of Green" 7. xx: set value "Digital Gain of Blue" Thomas I've been looking through the frame headers sent by the MR97310A (the Aiptek PenCam VGA+, 08ca:0111). Here are my observations. FF FF 00 FF 96 6x x0 xx xx xx xx xx In binary that looks something like this: 10010110 011001aa a101 All of the values look to be MSbit first. A looks like a 3-bit frame sequence number that seems to start with 1 and increments for each frame. Hmmm. This I never noticed. What you are saying is that the two bytes 6x and x0 are variable? You are sure about that? What I have previously experienced is that the first is always 64 with these cameras, and the second one indicates the absence of compression (in which case it is 0, which of course only arises for still cameras), or if there is data compression then it is not zero. I have never seen this byte to change during a session with a camera. Here is a little table of what I have previously witnessed, and perhaps you can suggest what to do in order to see this is not happening: Camera that byte compression solved, or not streaming Aiptek 00 no N/A no Aiptek 50 yes yes both the Sakar cam 00 no N/A no ditto 50 yes yes both Argus QuikClix 20 yes no doesn't work Argus DC162050 yes yes doesn't work CIF cameras 00 no N/A no ditto 50 yes yes no ditto d0 yes no yes Other strange facts are -- that the Sakar camera, the Argus QuikClix, and the DC1620 all share the same USB ID of 0x93a:0x010f and yet only one of them will stream with the existing driver. The other two go through the motions, but the isoc packets do not actually get sent, so there is no image coming out. I do not understand the reason for this; I have been trying to figure it out and it is rather weird. I should add that, yes, those two cameras were said to be capable of streaming when I bought them. Could it be a problem of age? I don't expect that, but maybe. -- the CIF cameras all share the USB id of 0x93a:0x010e (I bought several of them) and they all are using a different compression algorithm while streaming from what they use if running as still cameras in compressed mode. This leads to the question whether it is possible to set the compression algorithm during the initialization sequence, so that the camera also uses the "0x50" mode while streaming, because we already know how to use that mode. But I have never seen the 0x64 0xX0 bytes used to count the frames. Could you tell me how to repeat that? It certainly would knock down the validity of the above table wouldn't it? I've modified libv4l to print out the 12-byte header before it skips over it. Good idea, and an obvious one. Why did I not think of that? Then when I fire up mplayer it prints out each header as each frame is received. The framerate is only about 5 fps so there isn't a ton of data to parse through. When I point the camera into a light I get this (at 640x480): ... ff ff
Re: RFC on proposed patches to mr97310a.c for gspca and v4l
On Friday 17 April 2009 12:50:51 Theodore Kilgore wrote: > On Thu, 16 Apr 2009, Kyle Guinn wrote: > > On Thursday 16 April 2009 13:22:11 Theodore Kilgore wrote: > >> On Thu, 16 Apr 2009, Kyle Guinn wrote: > >>> I've been looking through the frame headers sent by the MR97310A (the > >>> Aiptek PenCam VGA+, 08ca:0111). Here are my observations. > >>> > >>> FF FF 00 FF 96 6x x0 xx xx xx xx xx > >>> > >>> In binary that looks something like this: > >>> > >>> > >>> 10010110 011001aa a101 > >>> > >>> > >>> All of the values look to be MSbit first. A looks like a 3-bit frame > >>> sequence number that seems to start with 1 and increments for each > >>> frame. > >> > >> Hmmm. This I never noticed. What you are saying is that the two bytes 6x > >> and x0 are variable? You are sure about that? What I have previously > >> experienced is that the first is always 64 with these cameras, and the > >> second one indicates the absence of compression (in which case it is 0, > >> which of course only arises for still cameras), or if there is data > >> compression then it is not zero. I have never seen this byte to change > >> during a session with a camera. Here is a little table of what I have > >> previously witnessed, and perhaps you can suggest what to do in order to > >> see this is not happening: > >> > >> Camera that byte compression solved, or not > >> streaming > >> Aiptek 00 no N/A no > >> Aiptek 50 yes yes both > >> the Sakar cam 00 no N/A no > >> ditto 50 yes yes both > >> Argus QuikClix 20 yes no doesn't work > >> Argus DC1620 50 yes yes doesn't work > >> CIF cameras00 no N/A no > >> ditto 50 yes yes no > >> ditto d0 yes no yes > >> > >> Other strange facts are > >> > >> -- that the Sakar camera, the Argus QuikClix, and the > >> DC1620 all share the same USB ID of 0x93a:0x010f and yet only one of > >> them will stream with the existing driver. The other two go through the > >> motions, but the isoc packets do not actually get sent, so there is no > >> image coming out. I do not understand the reason for this; I have been > >> trying to figure it out and it is rather weird. I should add that, yes, > >> those two cameras were said to be capable of streaming when I bought > >> them. Could it be a problem of age? I don't expect that, but maybe. > >> > >> -- the CIF cameras all share the USB id of 0x93a:0x010e (I bought > >> several of them) and they all are using a different compression > >> algorithm while streaming from what they use if running as still cameras > >> in compressed mode. This leads to the question whether it is possible to > >> set the compression algorithm during the initialization sequence, so > >> that the camera also uses the "0x50" mode while streaming, because we > >> already know how to use that mode. > >> > >> But I have never seen the 0x64 0xX0 bytes used to count the frames. > >> Could you tell me how to repeat that? It certainly would knock down the > >> validity of the above table wouldn't it? > > > > I've modified libv4l to print out the 12-byte header before it skips over > > it. > > Good idea, and an obvious one. Why did I not think of that? > > > Then when I fire up mplayer it prints out each header as each frame is > > received. The framerate is only about 5 fps so there isn't a ton of data > > to parse through. When I point the camera into a light I get this (at > > 640x480): > > > > ... > > ff ff 00 ff 96 64 d0 c1 5c c6 00 00 > > ff ff 00 ff 96 65 50 c1 5c c6 00 00 > > ff ff 00 ff 96 65 d0 c1 5c c6 00 00 > > ff ff 00 ff 96 66 50 c1 5c c6 00 00 > > ff ff 00 ff 96 66 d0 c1 5c c6 00 00 > > ff ff 00 ff 96 67 50 c1 5c c6 00 00 > > ff ff 00 ff 96 67 d0 c1 5c c6 00 00 > > ff ff 00 ff 96 64 50 c1 5c c6 00 00 > > ff ff 00 ff 96 64 d0 c1 5c c6 00 00 > > ff ff 00 ff 96 65 50 c1 5c c6 00 00 > > ... > > Which camera is this? Is it the Aiptek Pencam VGA+? If so, then I can try > it, too, because I also have one of them. > Yes, that's the one. Try your others if you can and let me know what happens. > > Only those 3 bits change, and it looks like a counter to me. Take a look > > at the gspca-mars (MR97113A?) subdriver. I think it tries to accommodate > > the frame sequence number when looking for the SOF. > > No, I don't see that, sorry. What I see is that it looks for the SOF, > which is declared in pac_common.h to be the well-known FF FF 00 FF 96 and > no more bytes after that. > I'm talking about this code from gspca/mars.c. Look in sd_pkt_scan(). if (data[0 + p] == 0xff &
Re: RFC on proposed patches to mr97310a.c for gspca and v4l
On Fri, 17 Apr 2009, Kyle Guinn wrote:
On Friday 17 April 2009 12:50:51 Theodore Kilgore wrote:
On Thu, 16 Apr 2009, Kyle Guinn wrote:
On Thursday 16 April 2009 13:22:11 Theodore Kilgore wrote:
On Thu, 16 Apr 2009, Kyle Guinn wrote:
ff ff 00 ff 96 64 d0 c1 5c c6 00 00
ff ff 00 ff 96 65 50 c1 5c c6 00 00
ff ff 00 ff 96 65 d0 c1 5c c6 00 00
ff ff 00 ff 96 66 50 c1 5c c6 00 00
ff ff 00 ff 96 66 d0 c1 5c c6 00 00
ff ff 00 ff 96 67 50 c1 5c c6 00 00
ff ff 00 ff 96 67 d0 c1 5c c6 00 00
ff ff 00 ff 96 64 50 c1 5c c6 00 00
ff ff 00 ff 96 64 d0 c1 5c c6 00 00
ff ff 00 ff 96 65 50 c1 5c c6 00 00
...
Which camera is this? Is it the Aiptek Pencam VGA+? If so, then I can try
it, too, because I also have one of them.
Yes, that's the one. Try your others if you can and let me know what happens.
OK. I will.
Only those 3 bits change, and it looks like a counter to me. Take a look
at the gspca-mars (MR97113A?) subdriver. I think it tries to accommodate
the frame sequence number when looking for the SOF.
No, I don't see that, sorry. What I see is that it looks for the SOF,
which is declared in pac_common.h to be the well-known FF FF 00 FF 96 and
no more bytes after that.
I'm talking about this code from gspca/mars.c. Look in sd_pkt_scan().
if (data[0 + p] == 0xff
&& data[1 + p] == 0xff
&& data[2 + p] == 0x00
&& data[3 + p] == 0xff
&& data[4 + p] == 0x96) {
if (data[5 + p] == 0x64
|| data[5 + p] == 0x65
|| data[5 + p] == 0x66
|| data[5 + p] == 0x67) {
Ah, so: mars.c not mr97310a.c
You lost me, there, for a minute. Yes, this sequence is there.
Thanks,
Theodore Kilgore
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: RFC on proposed patches to mr97310a.c for gspca and v4l
On Tuesday 03 March 2009 18:12:33 kilg...@banach.math.auburn.edu wrote: > Hans, Jean-Francois, and Kyle, > > The proposed patches are not very long, so I will give each of them, with > my comments after each, to explain why I believe that these changes are a > good idea. > > First, the patch to libv4lconvert is short and sweet: > > contents of file mr97310av4l.patch follow > -- > --- mr97310a.c.old2009-03-01 15:37:38.0 -0600 > +++ mr97310a.c.new2009-02-18 22:39:48.0 -0600 > @@ -102,6 +102,9 @@ void v4lconvert_decode_mr97310a(const un > if (!decoder_initialized) > init_mr97310a_decoder(); > > + /* remove the header */ > + inp += 12; > + > bitpos = 0; > > /* main decoding loop */ > > - here ends the v4lconvert patch -- > > The reason I want to do this should be obvious. It is to preserve the > entire header of each frame over in the gspca driver, and to throw it away > over here. The SOF marker FF FF 00 FF 96 is also kept. The reason why all > of this should be kept is that it makes it possible to look at a raw > output and to know if it is exactly aligned or not. Furthermore, the next > byte after the 96 is a code for the compression algorithm used, and the > bytes after that in the header might be useful in the future for better > image processing. In other words, these headers contain information which > might be useful in the future and they should not be jettisoned in the > kernel module. > No complaints here. I copied off of the pac207 driver, thinking that one compression format == one pixel format and that all mr97310a cameras use the same compression. I was hesitant to say that the mr97310a pixel format can correspond to multiple compression formats, especially since I only have one such camera and I don't know if it's preferred to use multiple pixel formats for this reason. >From what I understand, sending the frame header to userspace solves at least two problems (if indeed the compression is specified in the header): * One frame may be compressed and the next frame isn't, or the next frame uses a different compression. * Two cameras with the same vendor/product ID use different compression formats. Distinguishing the two cameras in the kernel driver could be messy. Just a random thought, but maybe the pac207 driver can benefit from such a change as well? -Kyle -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: RFC on proposed patches to mr97310a.c for gspca and v4l
On Tue, 3 Mar 2009, Kyle Guinn wrote: On Tuesday 03 March 2009 18:12:33 kilg...@banach.math.auburn.edu wrote: Hans, Jean-Francois, and Kyle, The proposed patches are not very long, so I will give each of them, with my comments after each, to explain why I believe that these changes are a good idea. First, the patch to libv4lconvert is short and sweet: contents of file mr97310av4l.patch follow -- --- mr97310a.c.old 2009-03-01 15:37:38.0 -0600 +++ mr97310a.c.new 2009-02-18 22:39:48.0 -0600 @@ -102,6 +102,9 @@ void v4lconvert_decode_mr97310a(const un if (!decoder_initialized) init_mr97310a_decoder(); + /* remove the header */ + inp += 12; + bitpos = 0; /* main decoding loop */ - here ends the v4lconvert patch -- The reason I want to do this should be obvious. It is to preserve the entire header of each frame over in the gspca driver, and to throw it away over here. The SOF marker FF FF 00 FF 96 is also kept. The reason why all of this should be kept is that it makes it possible to look at a raw output and to know if it is exactly aligned or not. Furthermore, the next byte after the 96 is a code for the compression algorithm used, and the bytes after that in the header might be useful in the future for better image processing. In other words, these headers contain information which might be useful in the future and they should not be jettisoned in the kernel module. No complaints here. I copied off of the pac207 driver, thinking that one compression format == one pixel format and that all mr97310a cameras use the same compression. I was hesitant to say that the mr97310a pixel format can correspond to multiple compression formats, especially since I only have one such camera and I don't know if it's preferred to use multiple pixel formats for this reason. Well, it is a fact that different compression formats are used by some cameras. First, the two 0x093a:0x010f cameras which I have that do *not* work with this module actually do use different compression algorithms. The proof is that what will convert the raw files of one of them, will not work on the other. The only place this is visible is in the header of the raw file (see previous discussion about this on the list). Well, OK, these cameras do not work. But then there are the 0x093a:0x010e cameras. They work very nicely with all of your code, up to the point that they use a different compressed format for the raw output and the frames come out looking wrong. Again, the only place this is marked is there is an indicator byte for the compression algorithm, and it is in the header. From what I understand, sending the frame header to userspace solves at least two problems (if indeed the compression is specified in the header): It is. Really. * One frame may be compressed and the next frame isn't, or the next frame uses a different compression. These are very unlikely scenarios for a webcam. They assuredly do occur with still cameras, true. At least, one finds that the still camera will support a compressed mode, and an uncompressed mode. And, yes, the different kinds can be all mixed together. For, the user can reset the compression setting before each picture is shot. * Two cameras with the same vendor/product ID use different compression formats. Distinguishing the two cameras in the kernel driver could be messy. Well, sending the header along takes care of that. Once it is known how to decompress them, all that one needs to do is to look at the telltale byte from the header, and one knows which algorithm to use. Simple, actually. Just a random thought, but maybe the pac207 driver can benefit from such a change as well? Probably. It just isn't my business. I would really be curious what those bytes are that are in the pac207 header, too, for comparison purposes and because someone ought to make a record of these things. Thus, if it were left to me I would probably rewrite the pac_common.h file change all apps which use it, in accord with the changes there and in accord with what I proposed here. But those would be too many changes which involve too many people at once, and something can go wrong when one does that. So better just to change the one driver I am interested in, hoping that you would not mind, and because I have a couple of cameras that I could test it with and I can say it works well after the changes. Why would I change pac_common.h? Well, the sof marker should not be tossed, either, IMHO, because it is after all an sof marker. It is very comforting to be able to look at a raw output and to know for certain that at least it starts out right because it begins with an sof marker. One knows then that things are going well. That after all is part of the reason an sof marker is put there in the first place. To know where the
Re: RFC on proposed patches to mr97310a.c for gspca and v4l
kilg...@banach.math.auburn.edu wrote:
Hans, Jean-Francois, and Kyle,
The proposed patches are not very long, so I will give each of them,
with my comments after each, to explain why I believe that these changes
are a good idea.
First, the patch to libv4lconvert is short and sweet:
contents of file mr97310av4l.patch follow
--
--- mr97310a.c.old2009-03-01 15:37:38.0 -0600
+++ mr97310a.c.new2009-02-18 22:39:48.0 -0600
@@ -102,6 +102,9 @@ void v4lconvert_decode_mr97310a(const un
if (!decoder_initialized)
init_mr97310a_decoder();
+/* remove the header */
+inp += 12;
+
bitpos = 0;
/* main decoding loop */
- here ends the v4lconvert patch --
The reason I want to do this should be obvious. It is to preserve the
entire header of each frame over in the gspca driver, and to throw it
away over here. The SOF marker FF FF 00 FF 96 is also kept. The reason
why all of this should be kept is that it makes it possible to look at a
raw output and to know if it is exactly aligned or not. Furthermore, the
next byte after the 96 is a code for the compression algorithm used, and
the bytes after that in the header might be useful in the future for
better image processing. In other words, these headers contain
information which might be useful in the future and they should not be
jettisoned in the kernel module.
+1
Now, the kernel module ought to keep and send along the header and SOF
marker instead of throwing them away. This is the topic of the next
patch. It also has the virtue of simplifying and shortening the code in
the module at the same time, because one is not going through
contortions to skip over and throw away some data which ought to be kept
anyway.
+1
contents of file mr97310a.patch follow, for gspca/mr97310a.c
--- mr97310a.c.old2009-02-23 23:59:07.0 -0600
+++ mr97310a.c.new2009-03-03 17:19:06.0 -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);
@@ -337,6 +325,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);
end of mr97310a.patch -
You will also notice that I have added a USB ID. As I have mentioned, I
have four cameras with this ID. The story with them is that two of them
will not work at all. The module will not initialize the camera. As far
as the other two of them are concerned, the module and the accompanying
change in libv4lconvert work very well. I have mentioned this
previously, and I did not get any comment about what is good to do. So
now I decided to submit the ID number in the patch.
Adding the USB-ID sounds like the right thing to do.
Regards,
Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: RFC on proposed patches to mr97310a.c for gspca and v4l
Kyle Guinn wrote: On Tuesday 03 March 2009 18:12:33 kilg...@banach.math.auburn.edu wrote: Just a random thought, but maybe the pac207 driver can benefit from such a change as well? It could, but it is to late for that, the pac207 driver and corresponding libv4l functionality has been out there for 2 kernel releases now, so we cannot change that. Which also makes me wonder about the same change for the mr97310a, is that cam already supported in a released kernel ? If not we MUST make sure we get this change in before 2.6.29 final, if it is I'm afraid we cannot make these changes. In that case if we ever need to header data we need to define a new PIXFMT for mr97310a with the header data, and deprecate the old one. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: RFC on proposed patches to mr97310a.c for gspca and v4l
Hello Theodore kilg...@banach.math.auburn.edu wrote: Also, after the byte indicator for the compression algorithm there are some more bytes, and these almost definitely contain information which could be valuable while doing image processing on the output. If they are already kept and passed out of the module over to libv4lconvert, then it would be very easy to do something with those bytes if it is ever figured out precisely what they mean. But if it is not done now it would have to be done then and would cause even more trouble. I sent it already in private mail to you. Here is the observation I made for the PAC207 SOF some years ago: From usb snoop. FF FF 00 FF 96 64 xx 00 xx xx xx xx xx xx 00 00 1. xx: looks like random value 2. xx: changed from 0x03 to 0x0b 3. xx: changed from 0x06 to 0x49 4. xx: changed from 0x07 to 0x55 5. xx: static 0x96 6. xx: static 0x80 7. xx: static 0xa0 And I did play in Linux and could identify some fields :-) . In Linux the header looks like this: FF FF 00 FF 96 64 xx 00 xx xx xx xx xx xx F0 00 1. xx: don't know but value is changing between 0x00 to 0x07 2. xx: this is the actual pixel clock 3. xx: this is changing according light conditions from 0x03 (dark) to 0xfc (bright) (center) 4. xx: this is changing according light conditions from 0x03 (dark) to 0xfc (bright) (edge) 5. xx: set value "Digital Gain of Red" 6. xx: set value "Digital Gain of Green" 7. xx: set value "Digital Gain of Blue" Thomas -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: RFC on proposed patches to mr97310a.c for gspca and v4l
Thomas Kaiser wrote: Hello Theodore kilg...@banach.math.auburn.edu wrote: Also, after the byte indicator for the compression algorithm there are some more bytes, and these almost definitely contain information which could be valuable while doing image processing on the output. If they are already kept and passed out of the module over to libv4lconvert, then it would be very easy to do something with those bytes if it is ever figured out precisely what they mean. But if it is not done now it would have to be done then and would cause even more trouble. I sent it already in private mail to you. Here is the observation I made for the PAC207 SOF some years ago: From usb snoop. FF FF 00 FF 96 64 xx 00 xx xx xx xx xx xx 00 00 1. xx: looks like random value 2. xx: changed from 0x03 to 0x0b 3. xx: changed from 0x06 to 0x49 4. xx: changed from 0x07 to 0x55 5. xx: static 0x96 6. xx: static 0x80 7. xx: static 0xa0 And I did play in Linux and could identify some fields :-) . In Linux the header looks like this: FF FF 00 FF 96 64 xx 00 xx xx xx xx xx xx F0 00 1. xx: don't know but value is changing between 0x00 to 0x07 2. xx: this is the actual pixel clock 3. xx: this is changing according light conditions from 0x03 (dark) to 0xfc (bright) (center) 4. xx: this is changing according light conditions from 0x03 (dark) to 0xfc (bright) (edge) 5. xx: set value "Digital Gain of Red" 6. xx: set value "Digital Gain of Green" 7. xx: set value "Digital Gain of Blue" Thomas And I forgot to say that the center brightness sensor was used to do auto brightness control in the old gspca driver. Pixel clock was changed on the fly to get better brightness in dark light conditions. Thomas -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: RFC on proposed patches to mr97310a.c for gspca and v4l
On Wed, 4 Mar 2009, Hans de Goede wrote: Kyle Guinn wrote: On Tuesday 03 March 2009 18:12:33 kilg...@banach.math.auburn.edu wrote: Just a random thought, but maybe the pac207 driver can benefit from such a change as well? It could, but it is to late for that, the pac207 driver and corresponding libv4l functionality has been out there for 2 kernel releases now, so we cannot change that. Pretty much what I said. It would have been better so, but done is done. Which also makes me wonder about the same change for the mr97310a, is that cam already supported in a released kernel ? Someone else may know better than I do, but since it was only added quite recently, surely not? If not we MUST make sure we get this change in before 2.6.29 final, if it is I'm afraid we cannot make these changes. In that case if we ever need to header data we need to define a new PIXFMT for mr97310a with the header data, and deprecate the old one. I do not quite understand. Why not just do it right away. especially if this has not appeared yet in any kernel? Theodore Kilgore -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: RFC on proposed patches to mr97310a.c for gspca and v4l
On Wed, 4 Mar 2009, Thomas Kaiser wrote: Thomas Kaiser wrote: Hello Theodore kilg...@banach.math.auburn.edu wrote: Also, after the byte indicator for the compression algorithm there are some more bytes, and these almost definitely contain information which could be valuable while doing image processing on the output. If they are already kept and passed out of the module over to libv4lconvert, then it would be very easy to do something with those bytes if it is ever figured out precisely what they mean. But if it is not done now it would have to be done then and would cause even more trouble. I sent it already in private mail to you. Here is the observation I made for the PAC207 SOF some years ago: From usb snoop. FF FF 00 FF 96 64 xx 00 xx xx xx xx xx xx 00 00 1. xx: looks like random value 2. xx: changed from 0x03 to 0x0b 3. xx: changed from 0x06 to 0x49 4. xx: changed from 0x07 to 0x55 5. xx: static 0x96 6. xx: static 0x80 7. xx: static 0xa0 And I did play in Linux and could identify some fields :-) . In Linux the header looks like this: FF FF 00 FF 96 64 xx 00 xx xx xx xx xx xx F0 00 1. xx: don't know but value is changing between 0x00 to 0x07 2. xx: this is the actual pixel clock 3. xx: this is changing according light conditions from 0x03 (dark) to 0xfc (bright) (center) 4. xx: this is changing according light conditions from 0x03 (dark) to 0xfc (bright) (edge) 5. xx: set value "Digital Gain of Red" 6. xx: set value "Digital Gain of Green" 7. xx: set value "Digital Gain of Blue" Thomas And I forgot to say that the center brightness sensor was used to do auto brightness control in the old gspca driver. Pixel clock was changed on the fly to get better brightness in dark light conditions. Yes, you did send it. My point here was to preserve the header information, for future use. So this is a good accounting of some possible future uses. As to the actual contents of the header, as you describe things, 0. Do you have any idea how to account for the discrepancy between From usb snoop. FF FF 00 FF 96 64 xx 00 xx xx xx xx xx xx 00 00 and In Linux the header looks like this: FF FF 00 FF 96 64 xx 00 xx xx xx xx xx xx F0 00 (I am referring to the 00 00 as opposed to F0 00)? Or could this have happened somehow just because these were not two identical sessions? 1. xx: don't know but value is changing between 0x00 to 0x07 as I said, this signifies the image format, qua compression algorithm in use, or if 00 then no compression. 2. xx: this is the actual pixel clock So there is a control setting for this? 3. xx: this is changing according light conditions from 0x03 (dark) to 0xfc (bright) (center) 4. xx: this is changing according light conditions from 0x03 (dark) to 0xfc (bright) (edge) 5. xx: set value "Digital Gain of Red" 6. xx: set value "Digital Gain of Green" 7. xx: set value "Digital Gain of Blue" Does anyone have any idea of how actually to use this information/ for example, since a lot of cameras are reporting some kind of similar looking information, does anyone know if there are any kinds of standard scales for these entries? Just what are they supposed to signify, and how exactly is one supposed to use such values, if they have been presented? When I say "a lot of cameras," understand, I mean still cameras as well as webcams, and cameras with a lot of different chipsets in them, too. So this is a question whether there is any standard system for the presentation of such data, and how it might constructively be used in image processing. I have had questions about this kind of thing for a long time, and I do not know where to look for the answers. Theodore Kilgore -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: RFC on proposed patches to mr97310a.c for gspca and v4l
On Wednesday 04 March 2009 02:39:11 Hans de Goede wrote: > Which also makes me wonder about the same change for the mr97310a, is that > cam already supported in a released kernel ? > > If not we MUST make sure we get this change in before 2.6.29 final, if it > is I'm afraid we cannot make these changes. In that case if we ever need to > header data we need to define a new PIXFMT for mr97310a with the header > data, and deprecate the old one. > I don't believe the driver has made it to any kernel yet. Even if it has, the user would need to have an unreleased version of libv4l. I think this change would inconvenience me and Theodore at most. Let's change it now. -Kyle -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: RFC on proposed patches to mr97310a.c for gspca and v4l
On Tuesday 03 March 2009 18:12:33 kilg...@banach.math.auburn.edu wrote:
> contents of file mr97310a.patch follow, for gspca/mr97310a.c
>
> --- mr97310a.c.old2009-02-23 23:59:07.0 -0600
> +++ mr97310a.c.new2009-03-03 17:19:06.0 -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.
2. sd->header_read no longer seems necessary.
3. If the SOF marker is split over two transfers then everything falls apart.
I don't know if the camera will allow that to happen, but it's better to be
safe.
-Kyle
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: RFC on proposed patches to mr97310a.c for gspca and v4l
On Wed, 4 Mar 2009, Kyle Guinn wrote:
On Tuesday 03 March 2009 18:12:33 kilg...@banach.math.auburn.edu wrote:
contents of file mr97310a.patch follow, for gspca/mr97310a.c
--- mr97310a.c.old 2009-02-23 23:59:07.0 -0600
+++ mr97310a.c.new 2009-03-03 17:19:06.0 -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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: RFC on proposed patches to mr97310a.c for gspca and v4l
On Wednesday 04 March 2009 22:34:13 kilg...@banach.math.auburn.edu wrote:
> On Wed, 4 Mar 2009, Kyle Guinn wrote:
> > On Tuesday 03 March 2009 18:12:33 kilg...@banach.math.auburn.edu wrote:
> >> contents of file mr97310a.patch follow, for gspca/mr97310a.c
> >>
> >> --- mr97310a.c.old 2009-02-23 23:59:07.0 -0600
> >> +++ mr97310a.c.new 2009-03-03 17:19:06.0 -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?
>
Simple example: One transfer ends with FF FF 00 and the next begins with FF
96 64. pac_find_sof() returns a pointer to 64, n is set to 0, len stays the
same, data now points at 3 bytes _before_ the transfer buffer, and we will
most likely get undefined behavior when trying to copy the data out of the
transfer buffer. Not only that, but the FF FF 00 portion of the SOF won't
get copied to the frame buffer.
Since we know what the SOF looks like, we don't need to worry about losing the
FF FF 00 portion -- just copy sd->sof_read bytes from pac_sof_marker.
Unfortunately my brain is fried right now and I can't come up with a working
example.
> > 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);
>
Let's not overcomplicate things. I think inp += 12 with a comment is fine for
now since we haven't completely reverse engineered the header yet. Use one
function to parse through the header, then use a different one to handle the
frame decompression. The header parser will call the correct decompressor
function with the correct offset into the frame buffer.
> 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;
>
I don't think we know this for sure yet. Maybe the header length is variable
and is specified along with the compression code?
> 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 whic
Re: RFC on proposed patches to mr97310a.c for gspca and v4l
On Wed, 4 Mar 2009, Kyle Guinn wrote:
On Wednesday 04 March 2009 22:34:13 kilg...@banach.math.auburn.edu wrote:
On Wed, 4 Mar 2009, Kyle Guinn wrote:
On Tuesday 03 March 2009 18:12:33 kilg...@banach.math.auburn.edu wrote:
contents of file mr97310a.patch follow, for gspca/mr97310a.c
--- mr97310a.c.old 2009-02-23 23:59:07.0 -0600
+++ mr97310a.c.new 2009-03-03 17:19:06.0 -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.
True, and if you remove it then you can also delete some other things. Try
it and heed the compile warnings, and you will see.
3. If the SOF marker is split over two transfers then everything falls
apart.
Are you sure about that?
Simple example: One transfer ends with FF FF 00 and the next begins with FF
96 64. pac_find_sof() returns a pointer to 64, n is set to 0, len stays the
same, data now points at 3 bytes _before_ the transfer buffer, and we will
most likely get undefined behavior when trying to copy the data out of the
transfer buffer. Not only that, but the FF FF 00 portion of the SOF won't
get copied to the frame buffer.
Yes, you are right. I was chasing through all of it, and I found the same
thing. I will point out, though, that this danger is not a new one. The
code which you originally had there suffers the same defect. The problem
is not whether one decides to keep the SOF marker in the frame output.
Rather, the problem is that one must *detect* it. And to detect it one
must needs be able to read all of it at once. If you read only three bytes
from it, and that is the end of the packet, then that will be analysed as
still belonging to the same frame. Then the next packet will continue to
be in the same frame, too.
A mitigating factor here is that when the next packet is "in the same
frame" then what will happen in practice is that the decompressor will
run, fill up the current frame, and stop when it reaches the end of the
frame and will toss the rest of the data. So in other words the next frame
will just get tossed.
Since we know what the SOF looks like, we don't need to worry about losing the
FF FF 00 portion
Yes, you do. You know what the marker looks like, and I know. But the FF
FF 00 is the end of the packet. So a computer will not know. It will not
be detected as part of an SOF marker. Instead, it will just be tacked onto
the data from the current frame and any special meaning will be lost.
However, the consequence is that the decompression algorithm will use
enough data to finish the current frame, stop before it has to use the FF
FF 00, and, since no marker has been detected in the next packet, either,
all new data will just be ignored as junk until another SOF marker comes
up and is detected. Then and only then a new output frame will be
initiated.
-- just copy sd->sof_read bytes from pac_sof_marker.
Unfortunately my brain is fried right now and I can't come up with a working
example.
I don't know if the camera will allow that to happen, but it's better to
be safe.
Agreed.
Maybe not. Perhaps according to my analysis above it is actually no big
deal. My analysis of what will happen is based upon the way the
decomressor function works (uses data until it is finished with a frame,
and then quits). So if it occasionally happens that an SOF marker is split
in two across two packets, then it just causes a frame's data to be
skipped. I can't imagine any other ill effect. Of course, if this bad
thing happens for 350 frames in succession, that would be quite
interesting.
Therefore, what I think is that the attempt to code around the possibility
that an SOF marker is split across two frames would be quite likely to
cause more trouble than it is worth. What would be needed is to consider
two successive packets at a time. If there is no SOF marker which can
start inside the first one and end in the second one, then pu
Re: RFC on proposed patches to mr97310a.c for gspca and v4l
Kyle Guinn wrote:
On Wednesday 04 March 2009 22:34:13 kilg...@banach.math.auburn.edu wrote:
On Wed, 4 Mar 2009, Kyle Guinn wrote:
On Tuesday 03 March 2009 18:12:33 kilg...@banach.math.auburn.edu wrote:
contents of file mr97310a.patch follow, for gspca/mr97310a.c
--- mr97310a.c.old 2009-02-23 23:59:07.0 -0600
+++ mr97310a.c.new 2009-03-03 17:19:06.0 -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?
Simple example: One transfer ends with FF FF 00 and the next begins with FF
96 64. pac_find_sof() returns a pointer to 64, n is set to 0, len stays the
same, data now points at 3 bytes _before_ the transfer buffer, and we will
most likely get undefined behavior when trying to copy the data out of the
transfer buffer. Not only that, but the FF FF 00 portion of the SOF won't
get copied to the frame buffer.
Good point, since we will always pass frames to userspace which start with the
sof, maybe we should just only pass the variable part of the header to
userspace?
That sure feels like the easiest solution to me.
Regards,
Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: RFC on proposed patches to mr97310a.c for gspca and v4l
Kyle Guinn wrote: On Wednesday 04 March 2009 02:39:11 Hans de Goede wrote: Which also makes me wonder about the same change for the mr97310a, is that cam already supported in a released kernel ? If not we MUST make sure we get this change in before 2.6.29 final, if it is I'm afraid we cannot make these changes. In that case if we ever need to header data we need to define a new PIXFMT for mr97310a with the header data, and deprecate the old one. I don't believe the driver has made it to any kernel yet. Even if it has, the user would need to have an unreleased version of libv4l. I think this change would inconvenience me and Theodore at most. Let's change it now. +1 Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: RFC on proposed patches to mr97310a.c for gspca and v4l
Hello Theodore kilg...@banach.math.auburn.edu wrote: On Wed, 4 Mar 2009, Thomas Kaiser wrote: As to the actual contents of the header, as you describe things, 0. Do you have any idea how to account for the discrepancy between From usb snoop. FF FF 00 FF 96 64 xx 00 xx xx xx xx xx xx 00 00 and In Linux the header looks like this: FF FF 00 FF 96 64 xx 00 xx xx xx xx xx xx F0 00 (I am referring to the 00 00 as opposed to F0 00)? Or could this have happened somehow just because these were not two identical sessions? Doesn't remember what the differences was. The first is from Windoz (usbsnoop) and the second is from Linux. 1. xx: don't know but value is changing between 0x00 to 0x07 as I said, this signifies the image format, qua compression algorithm in use, or if 00 then no compression. On the PAC207, the compression can be controlled with a register called "Compression Balance size". So, I guess, depending on the value set in the register this value in the header will show what compression level is set. 2. xx: this is the actual pixel clock So there is a control setting for this? Yes, in the PAC207, register 2. (12 MHz divided by the value set). 3. xx: this is changing according light conditions from 0x03 (dark) to 0xfc (bright) (center) 4. xx: this is changing according light conditions from 0x03 (dark) to 0xfc (bright) (edge) 5. xx: set value "Digital Gain of Red" 6. xx: set value "Digital Gain of Green" 7. xx: set value "Digital Gain of Blue" Does anyone have any idea of how actually to use this information/ for example, since a lot of cameras are reporting some kind of similar looking information, does anyone know if there are any kinds of standard scales for these entries? Just what are they supposed to signify, and how exactly is one supposed to use such values, if they have been presented? When I say "a lot of cameras," understand, I mean still cameras as well as webcams, and cameras with a lot of different chipsets in them, too. So this is a question whether there is any standard system for the presentation of such data, and how it might constructively be used in image processing. I have had questions about this kind of thing for a long time, and I do not know where to look for the answers. For the brightness, I guess, 0 means dark and 0xff completely bright (sensor is in saturation)? Thomas -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: RFC on proposed patches to mr97310a.c for gspca and v4l
On Thu, 5 Mar 2009, Thomas Kaiser wrote: Hello Theodore kilg...@banach.math.auburn.edu wrote: On Wed, 4 Mar 2009, Thomas Kaiser wrote: As to the actual contents of the header, as you describe things, 0. Do you have any idea how to account for the discrepancy between From usb snoop. FF FF 00 FF 96 64 xx 00 xx xx xx xx xx xx 00 00 and In Linux the header looks like this: FF FF 00 FF 96 64 xx 00 xx xx xx xx xx xx F0 00 (I am referring to the 00 00 as opposed to F0 00)? Or could this have happened somehow just because these were not two identical sessions? Doesn't remember what the differences was. The first is from Windoz (usbsnoop) and the second is from Linux. 1. xx: don't know but value is changing between 0x00 to 0x07 as I said, this signifies the image format, qua compression algorithm in use, or if 00 then no compression. On the PAC207, the compression can be controlled with a register called "Compression Balance size". So, I guess, depending on the value set in the register this value in the header will show what compression level is set. 2. xx: this is the actual pixel clock So there is a control setting for this? Yes, in the PAC207, register 2. (12 MHz divided by the value set). 3. xx: this is changing according light conditions from 0x03 (dark) to 0xfc (bright) (center) 4. xx: this is changing according light conditions from 0x03 (dark) to 0xfc (bright) (edge) 5. xx: set value "Digital Gain of Red" 6. xx: set value "Digital Gain of Green" 7. xx: set value "Digital Gain of Blue" Does anyone have any idea of how actually to use this information/ for example, since a lot of cameras are reporting some kind of similar looking information, does anyone know if there are any kinds of standard scales for these entries? Just what are they supposed to signify, and how exactly is one supposed to use such values, if they have been presented? When I say "a lot of cameras," understand, I mean still cameras as well as webcams, and cameras with a lot of different chipsets in them, too. So this is a question whether there is any standard system for the presentation of such data, and how it might constructively be used in image processing. I have had questions about this kind of thing for a long time, and I do not know where to look for the answers. For the brightness, I guess, 0 means dark and 0xff completely bright (sensor is in saturation)? That of course is a guess. OTOH it could be on a scale of 0 to 0x80, or it could be that only the digits 0 through 9 are actually used, and the basis is then 100, or too many other variations to count. Also what is considered a "normal" or an "average" value? The trouble with your suggestion of a scale from 0 to 0xff is that it makes sense, and in a situation like this one obviously can not assume that. What I am suspecting is that these things have some kind of standard definitions, which are not necessarily done by logic but by convention, and there is a document out there somewhere which lays it all down. The document could have been produced by Microsoft, for example, which doubtless has its own problems reducing chaos to order in the industry, or by some kind of consortium of camera manufacturers, or something like that. I really do strongly suspect that the interpretation of all of this is written down somewhere. But I don't know where to look. Theodore Kilgore -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: RFC on proposed patches to mr97310a.c for gspca and v4l
On Thu, 5 Mar 2009, Hans de Goede wrote:
Kyle Guinn wrote:
On Wednesday 04 March 2009 22:34:13 kilg...@banach.math.auburn.edu wrote:
On Wed, 4 Mar 2009, Kyle Guinn wrote:
On Tuesday 03 March 2009 18:12:33 kilg...@banach.math.auburn.edu wrote:
contents of file mr97310a.patch follow, for gspca/mr97310a.c
--- mr97310a.c.old 2009-02-23 23:59:07.0 -0600
+++ mr97310a.c.new 2009-03-03 17:19:06.0 -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?
Simple example: One transfer ends with FF FF 00 and the next begins with
FF 96 64. pac_find_sof() returns a pointer to 64, n is set to 0, len stays
the same, data now points at 3 bytes _before_ the transfer buffer, and we
will most likely get undefined behavior when trying to copy the data out of
the transfer buffer. Not only that, but the FF FF 00 portion of the SOF
won't get copied to the frame buffer.
Good point, since we will always pass frames to userspace which start with
the
sof, maybe we should just only pass the variable part of the header to
userspace?
That sure feels like the easiest solution to me.
Regards,
Hans
Hans, that would not solve the problem. In fact, it appears to me that
this problem was already inherent in the driver code before I proposed any
patches at all. The problem is that one must _detect_ the SOF marker. The
further problem is (and was, and nothing yet has changed that) that if the
SOF marker is split across two packets it will fail to be detected.
Two possible courses of action:
1. Leave the matter alone. It could be maintained that one of the
following is the case:
(a) this never happens
(b) even if it does happen (rarely), it will do no great harm,
because the only effect will be that the output skips a frame, having not
detected its start of frame marker. For, the frames consist of compressed
data, and if there is too much compressed data for a given frame, then
that extra compressed data will simply be tossed. The next frame which is
actually seen and used will be the next frame for which an SOF marker is
actually detected.
So, the argument for number 1 would be that, yes, this is sort of a bug,
but it is insignificant, not serious, and could never cause a problem in
practice even if it occurs, because the only result would be for a frame
to get dropped. Another argument in favor of it would be that in any event
the camera is sending isochronous packets, and there is in any event no
guarantee of the validity and accuracy of any one single packet. Instead,
the reliance is on the high rate at which the packets get sent, received,
and processed.
2. If it is deemed that, yes, it can happen that an SOF marker gets split
between two packets, and, no, if that happens it is a problem which should
not be ignored, then there is an alternative course of action:
Keep a cache consisting of the last 4 bytes of each packet, and
see if, when the next packet comes down, a SOF marker is detected in those
bytes, plus the new bytes from the beginning of the new packet. Then a SOF
marker will not be missed, even if it occurs in the situation described
above.
Alternative number two is in fact not very much more difficult, but it
would involve putting an if-then-else or two into the code and also would
require one to have a place to put those last four bytes of each packet,
keep them around until the next packet is obtained, and then parse the
result. This might slow things down, but probably not by very much. So
which way to do this would obviously depend upon some evaluation of the
danger of doing nothing.
However, the discussion of whether or not to deal with an SOF marker which
is split across two packets has nothing at all to do with the question of
Re: RFC on proposed patches to mr97310a.c for gspca and v4l
kilg...@banach.math.auburn.edu wrote:
On Thu, 5 Mar 2009, Hans de Goede wrote:
Kyle Guinn wrote:
On Wednesday 04 March 2009 22:34:13 kilg...@banach.math.auburn.edu
wrote:
On Wed, 4 Mar 2009, Kyle Guinn wrote:
On Tuesday 03 March 2009 18:12:33 kilg...@banach.math.auburn.edu
wrote:
contents of file mr97310a.patch follow, for gspca/mr97310a.c
--- mr97310a.c.old2009-02-23 23:59:07.0 -0600
+++ mr97310a.c.new2009-03-03 17:19:06.0 -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?
Simple example: One transfer ends with FF FF 00 and the next begins
with FF 96 64. pac_find_sof() returns a pointer to 64, n is set to
0, len stays the same, data now points at 3 bytes _before_ the
transfer buffer, and we will most likely get undefined behavior when
trying to copy the data out of the transfer buffer. Not only that,
but the FF FF 00 portion of the SOF won't get copied to the frame
buffer.
Good point, since we will always pass frames to userspace which start
with the
sof, maybe we should just only pass the variable part of the header to
userspace?
That sure feels like the easiest solution to me.
Regards,
Hans
Hans, that would not solve the problem. In fact, it appears to me that
this problem was already inherent in the driver code before I proposed
any patches at all.
Erm, if I understood correctly (haven't looked yet) the driver is working
with the sof detection from pac_common, which does work with a SOF split
over multiple frames.
The problem with the new code is that it takes the return value of the sof
detection (which is a pointer into the current frame) and then
substracts the length of the sofmarker, however if only part of the sof was
in the current frame the resulting pointer (after substracting the sof length)
will point to before the current frame buffer.
Hence my proposal to fix this by simple only sending the variable part of the
header to userspace (and thus not do the substraction).
Anyways this is just what I understood from the former discussion I have *not*
looked at the actual code (-ENOTIME)
Regards,
Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: RFC on proposed patches to mr97310a.c for gspca and v4l
Hello Theodore kilg...@banach.math.auburn.edu wrote: For the brightness, I guess, 0 means dark and 0xff completely bright (sensor is in saturation)? That of course is a guess. OTOH it could be on a scale of 0 to 0x80, or it could be that only the digits 0 through 9 are actually used, and the basis is then 100, or too many other variations to count. Also what is considered a "normal" or an "average" value? The trouble with your suggestion of a scale from 0 to 0xff is that it makes sense, and in a situation like this one obviously can not assume that. I don't really understand what you try to tell with this sentence: "and in a situation like this one obviously can not assume that." The values changed from 0x03 (dark) to 0xfc (bright), for me does this mean that the scale goes from 0x00 to 0xff!? Or I am wrong? What I am suspecting is that these things have some kind of standard definitions, which are not necessarily done by logic but by convention, and there is a document out there somewhere which lays it all down. The document could have been produced by Microsoft, for example, which doubtless has its own problems reducing chaos to order in the industry, or by some kind of consortium of camera manufacturers, or something like that. I really do strongly suspect that the interpretation of all of this is written down somewhere. But I don't know where to look. I believe that this documents are exists, but not available for public:-( Just company confidential. Anyway most of the Linux webcam drivers were done by re-engineering the Windoz driver (usbsnoop). That said, all information about the cams is "a guess". For the brightness thing, I just was working with a light and studied what is changing in the header of the frame. At this time I did this, I was not aware that I could remove the lens of the webcam to be more sensible to light change and get more precise results. During the work I did for the PAC7311 Pixart chip I found out that removing the lens and put light directly to the sensor does help a lot to figure out how the cam is working. And with this idea in mind, we could even get further to guess the compression algo from a cam. Assuming that the sensor has a Bayer pattern. - remove lens. - put white light on the sensor - use color filter an put each spectrum (RGB) on the sensor - check the stream and find out what is changing in the stream according to the different light conditions. Looks like I get off topic, now ;-) Something else comes in my mind. Would it good to document all this what we are talking bout somewhere on a webpage? Thomas -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: RFC on proposed patches to mr97310a.c for gspca and v4l
On Thu, 5 Mar 2009, Thomas Kaiser wrote: Hello Theodore kilg...@banach.math.auburn.edu wrote: For the brightness, I guess, 0 means dark and 0xff completely bright (sensor is in saturation)? That of course is a guess. OTOH it could be on a scale of 0 to 0x80, or it could be that only the digits 0 through 9 are actually used, and the basis is then 100, or too many other variations to count. Also what is considered a "normal" or an "average" value? The trouble with your suggestion of a scale from 0 to 0xff is that it makes sense, and in a situation like this one obviously can not assume that. I don't really understand what you try to tell with this sentence: "and in a situation like this one obviously can not assume that." I mean, your interpretation of 0 to 0xff is a natural and sensible interpretation (for us). But what one can not assume is, it made sense to those who constructed the system. Perhaps those guys were setting it all up differently. The values changed from 0x03 (dark) to 0xfc (bright), for me does this mean that the scale goes from 0x00 to 0xff!? Or I am wrong? Well, if you have actual data to back up your impressions about this, then clearly you have evidence. So that is good, obviously. What I am suspecting is that these things have some kind of standard definitions, which are not necessarily done by logic but by convention, and there is a document out there somewhere which lays it all down. The document could have been produced by Microsoft, for example, which doubtless has its own problems reducing chaos to order in the industry, or by some kind of consortium of camera manufacturers, or something like that. I really do strongly suspect that the interpretation of all of this is written down somewhere. But I don't know where to look. I believe that this documents are exists, but not available for public:-( Just company confidential. That may be true. If so, then such documentation is indeed not available. But sometimes a document is published and available, and one just is not aware of the fact. Anyway most of the Linux webcam drivers were done by re-engineering the Windoz driver (usbsnoop). That said, all information about the cams is "a guess". Very true. Also true about the still cameras that I supported in libgphoto2. There are no secrets kept on the USB bus. But what is done inside the computer does not appear on the USB bus and there is no log of it. For the brightness thing, I just was working with a light and studied what is changing in the header of the frame. At this time I did this, I was not aware that I could remove the lens of the webcam to be more sensible to light change and get more precise results. During the work I did for the PAC7311 Pixart chip I found out that removing the lens and put light directly to the sensor does help a lot to figure out how the cam is working. And with this idea in mind, we could even get further to guess the compression algo from a cam. Assuming that the sensor has a Bayer pattern. - remove lens. - put white light on the sensor - use color filter an put each spectrum (RGB) on the sensor - check the stream and find out what is changing in the stream according to the different light conditions. Looks like I get off topic, now ;-) But it is very interesting nevertheless. Something else comes in my mind. Would it good to document all this what we are talking bout somewhere on a webpage? Thomas Perhaps so. Also a good idea to try to collect some people who have similar interests and are trying to work on similar problems. I have been trying to do that for a while, but with mixed and limited success. Theodore Kilgore -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: RFC on proposed patches to mr97310a.c for gspca and v4l
kilg...@banach.math.auburn.edu wrote: That of course is a guess. OTOH it could be on a scale of 0 to 0x80, or it could be that only the digits 0 through 9 are actually used, and the basis is then 100, or too many other variations to count. Also what is considered a "normal" or an "average" value? The trouble with your suggestion of a scale from 0 to 0xff is that it makes sense, and in a situation like this one obviously can not assume that. I don't really understand what you try to tell with this sentence: "and in a situation like this one obviously can not assume that." I mean, your interpretation of 0 to 0xff is a natural and sensible interpretation (for us). But what one can not assume is, it made sense to those who constructed the system. Perhaps those guys were setting it all up differently. You are right, we don't know what the developer were thinking, unfortunately, You have to turn yourself in a webcam developer and think how you would do it. When you find, by observing/testing the cam, that it looks similar as you thought about how to do it, the observation should be true! The values changed from 0x03 (dark) to 0xfc (bright), for me does this mean that the scale goes from 0x00 to 0xff!? Or I am wrong? Well, if you have actual data to back up your impressions about this, then clearly you have evidence. So that is good, obviously. I will do this again in the next couple of weeks (lens removed). What I am suspecting is that these things have some kind of standard definitions, which are not necessarily done by logic but by convention, and there is a document out there somewhere which lays it all down. The document could have been produced by Microsoft, for example, which doubtless has its own problems reducing chaos to order in the industry, or by some kind of consortium of camera manufacturers, or something like that. I really do strongly suspect that the interpretation of all of this is written down somewhere. But I don't know where to look. I believe that this documents are exists, but not available for public:-( Just company confidential. That may be true. If so, then such documentation is indeed not available. But sometimes a document is published and available, and one just is not aware of the fact. Anyway most of the Linux webcam drivers were done by re-engineering the Windoz driver (usbsnoop). That said, all information about the cams is "a guess". Very true. Also true about the still cameras that I supported in libgphoto2. There are no secrets kept on the USB bus. But what is done inside the computer does not appear on the USB bus and there is no log of it. For the brightness thing, I just was working with a light and studied what is changing in the header of the frame. At this time I did this, I was not aware that I could remove the lens of the webcam to be more sensible to light change and get more precise results. During the work I did for the PAC7311 Pixart chip I found out that removing the lens and put light directly to the sensor does help a lot to figure out how the cam is working. And with this idea in mind, we could even get further to guess the compression algo from a cam. Assuming that the sensor has a Bayer pattern. - remove lens. - put white light on the sensor - use color filter an put each spectrum (RGB) on the sensor - check the stream and find out what is changing in the stream according to the different light conditions. Looks like I get off topic, now ;-) But it is very interesting nevertheless. I think so, I didn't try with the color filter :-( Something else comes in my mind. Would it good to document all this what we are talking bout somewhere on a webpage? Thomas Perhaps so. Also a good idea to try to collect some people who have similar interests and are trying to work on similar problems. I have been trying to do that for a while, but with mixed and limited success. May be, some people read this and have the same felling. Let's see what happens. Thomas -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: RFC on proposed patches to mr97310a.c for gspca and v4l
On Thu, 5 Mar 2009, Hans de Goede wrote:
kilg...@banach.math.auburn.edu wrote:
On Thu, 5 Mar 2009, Hans de Goede wrote:
Kyle Guinn wrote:
On Wednesday 04 March 2009 22:34:13 kilg...@banach.math.auburn.edu wrote:
On Wed, 4 Mar 2009, Kyle Guinn wrote:
On Tuesday 03 March 2009 18:12:33 kilg...@banach.math.auburn.edu wrote:
contents of file mr97310a.patch follow, for gspca/mr97310a.c
--- mr97310a.c.old2009-02-23 23:59:07.0 -0600
+++ mr97310a.c.new2009-03-03 17:19:06.0 -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?
Simple example: One transfer ends with FF FF 00 and the next begins with
FF 96 64. pac_find_sof() returns a pointer to 64, n is set to 0, len
stays the same, data now points at 3 bytes _before_ the transfer buffer,
and we will most likely get undefined behavior when trying to copy the
data out of the transfer buffer. Not only that, but the FF FF 00 portion
of the SOF won't get copied to the frame buffer.
Good point, since we will always pass frames to userspace which start with
the
sof, maybe we should just only pass the variable part of the header to
userspace?
That sure feels like the easiest solution to me.
Regards,
Hans
Hans, that would not solve the problem. In fact, it appears to me that this
problem was already inherent in the driver code before I proposed any
patches at all.
Erm, if I understood correctly (haven't looked yet) the driver is working
with the sof detection from pac_common, which does work with a SOF split
over multiple frames.
That is not my impression of what the code in pac_common is doing. That
code, as I understand, is totally neutral about such things. What is does
is to parse some data and search for an SOF marker, and if it finds such a
thing then it declares the next byte after to be what it calls "sof".
Specifically, there is the function
static unsigned char *pac_find_sof(struct gspca_dev *gspca_dev,
unsigned char *m, int len)
and what it does is that it searches through unsigned char *m up to the
extent declared in int len, looking for an SOF marker. If it finds one,
then it returns the location of the next byte after the SOF marker has
been successfully read.
What this function does not address in any way whatsoever is where the
data which is called unsigned char *m came from. So, the problem is, if
unsigned char *m is a single packet, or if it is what remains after some
stuff from the head of the packet has previously been put away, then the
danger is very much present that we are discussing. Namely, if the first
part of an SOF marker is present at the end of the data being considered
and the rest of the SOF marker is in the next packet of data which is not
yet being considered, then this function from pac_common, if naively
applied, will miss the SOF marker completely. By its nature, this function
can not search data which was not yet presented to it. That is the
problem.
Therefore, if one must make sure the SOF marker is always detected, even
if it is split across two packets, then any application of this function
is buggy, which does not take into account the fact that one can run out
of data before detecting an SOF marker, even when part of it is there at
the very end of the data, or if an incomplete part of it is there at the
very beginning of the data it will equally be missed. This remark would
potentially apply to any camera driver which is using this function, not
just the mr97310a driver. Again, the pac_find_sof() function does not
deal with _this_ issue, at all. It is up to the user of it to code around
any potential problem of this sort.
The only way to avoid any possible occurrence of the problem is to
follow my suggestion number two. One must keep four bytes from the end of
a packet, and adjoin to that four bytes from the beginning of a new
packet, and search
Re: RFC on proposed patches to mr97310a.c for gspca and v4l
kilg...@banach.math.auburn.edu wrote:
On Thu, 5 Mar 2009, Hans de Goede wrote:
kilg...@banach.math.auburn.edu wrote:
On Thu, 5 Mar 2009, Hans de Goede wrote:
Kyle Guinn wrote:
On Wednesday 04 March 2009 22:34:13 kilg...@banach.math.auburn.edu
wrote:
On Wed, 4 Mar 2009, Kyle Guinn wrote:
On Tuesday 03 March 2009 18:12:33 kilg...@banach.math.auburn.edu
wrote:
contents of file mr97310a.patch follow, for gspca/mr97310a.c
--- mr97310a.c.old2009-02-23 23:59:07.0 -0600
+++ mr97310a.c.new2009-03-03 17:19:06.0 -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?
Simple example: One transfer ends with FF FF 00 and the next
begins with FF 96 64. pac_find_sof() returns a pointer to 64, n is
set to 0, len stays the same, data now points at 3 bytes _before_
the transfer buffer, and we will most likely get undefined behavior
when trying to copy the data out of the transfer buffer. Not only
that, but the FF FF 00 portion of the SOF won't get copied to the
frame buffer.
Good point, since we will always pass frames to userspace which
start with the
sof, maybe we should just only pass the variable part of the header
to userspace?
That sure feels like the easiest solution to me.
Regards,
Hans
Hans, that would not solve the problem. In fact, it appears to me
that this problem was already inherent in the driver code before I
proposed any patches at all.
Erm, if I understood correctly (haven't looked yet) the driver is working
with the sof detection from pac_common, which does work with a SOF split
over multiple frames.
That is not my impression of what the code in pac_common is doing. That
code, as I understand, is totally neutral about such things. What is
does is to parse some data and search for an SOF marker, and if it finds
such a thing then it declares the next byte after to be what it calls
"sof". Specifically, there is the function
static unsigned char *pac_find_sof(struct gspca_dev *gspca_dev,
unsigned char *m, int len)
and what it does is that it searches through unsigned char *m up to the
extent declared in int len, looking for an SOF marker. If it finds one,
then it returns the location of the next byte after the SOF marker has
been successfully read.
Check that function again, more carefully, if it fails, but finds a part of
the sof at the end of m it remembers how much of the sof it has already seen
and continues where it left of the next time it is called.
Regards,
Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: RFC on proposed patches to mr97310a.c for gspca and v4l
On Thu, 5 Mar 2009, Thomas Kaiser wrote: kilg...@banach.math.auburn.edu wrote: That of course is a guess. OTOH it could be on a scale of 0 to 0x80, or it could be that only the digits 0 through 9 are actually used, and the basis is then 100, or too many other variations to count. Also what is considered a "normal" or an "average" value? The trouble with your suggestion of a scale from 0 to 0xff is that it makes sense, and in a situation like this one obviously can not assume that. I don't really understand what you try to tell with this sentence: "and in a situation like this one obviously can not assume that." I mean, your interpretation of 0 to 0xff is a natural and sensible interpretation (for us). But what one can not assume is, it made sense to those who constructed the system. Perhaps those guys were setting it all up differently. You are right, we don't know what the developer were thinking, unfortunately, You have to turn yourself in a webcam developer and think how you would do it. When you find, by observing/testing the cam, that it looks similar as you thought about how to do it, the observation should be true! True enough. In this respect, there is not much difference between still cameras and webcams. I will do this again in the next couple of weeks (lens removed). I believe that this documents are exists, but not available for public:-( Just company confidential. That may be true. If so, then such documentation is indeed not available. But sometimes a document is published and available, and one just is not aware of the fact. I will add to this that a lot of documentation for a lot of things really is available to the public. Anyway most of the Linux webcam drivers were done by re-engineering the Windoz driver (usbsnoop). That said, all information about the cams is "a guess". Very true. Also true about the still cameras that I supported in libgphoto2. There are no secrets kept on the USB bus. But what is done inside the computer does not appear on the USB bus and there is no log of it. I will brag a little bit. Give me any cheap still camera that I don't know anything about, and a copy of the Windows driver. Provided only that the camera does not use an unknown, proprietary compression algorithm, I will promise you a working libgphoto2 driver for it within a week. Compression algorithms are the big obstacle there, and the only one. For the brightness thing, I just was working with a light and studied what is changing in the header of the frame. At this time I did this, I was not aware that I could remove the lens of the webcam to be more sensible to light change and get more precise results. During the work I did for the PAC7311 Pixart chip I found out that removing the lens and put light directly to the sensor does help a lot to figure out how the cam is working. And with this idea in mind, we could even get further to guess the compression algo from a cam. Assuming that the sensor has a Bayer pattern. - remove lens. - put white light on the sensor - use color filter an put each spectrum (RGB) on the sensor - check the stream and find out what is changing in the stream according to the different light conditions. I would very much like to see this in action. Looks like I get off topic, now ;-) But it is very interesting nevertheless. I think so, I didn't try with the color filter :-( Something else comes in my mind. Would it good to document all this what we are talking bout somewhere on a webpage? Thomas Perhaps so. Also a good idea to try to collect some people who have similar interests and are trying to work on similar problems. I have been trying to do that for a while, but with mixed and limited success. May be, some people read this and have the same felling. Let's see what happens. felling->feeling We are not chopping down trees. :) Theodore Kilgore -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: RFC on proposed patches to mr97310a.c for gspca and v4l
On Thu, 5 Mar 2009, Hans de Goede wrote:
kilg...@banach.math.auburn.edu wrote:
On Thu, 5 Mar 2009, Hans de Goede wrote:
kilg...@banach.math.auburn.edu wrote:
On Thu, 5 Mar 2009, Hans de Goede wrote:
Kyle Guinn wrote:
On Wednesday 04 March 2009 22:34:13 kilg...@banach.math.auburn.edu
wrote:
On Wed, 4 Mar 2009, Kyle Guinn wrote:
On Tuesday 03 March 2009 18:12:33 kilg...@banach.math.auburn.edu
wrote:
contents of file mr97310a.patch follow, for gspca/mr97310a.c
--- mr97310a.c.old2009-02-23 23:59:07.0 -0600
+++ mr97310a.c.new2009-03-03 17:19:06.0 -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?
Simple example: One transfer ends with FF FF 00 and the next begins
with FF 96 64. pac_find_sof() returns a pointer to 64, n is set to 0,
len stays the same, data now points at 3 bytes _before_ the transfer
buffer, and we will most likely get undefined behavior when trying to
copy the data out of the transfer buffer. Not only that, but the FF FF
00 portion of the SOF won't get copied to the frame buffer.
Good point, since we will always pass frames to userspace which start
with the
sof, maybe we should just only pass the variable part of the header to
userspace?
That sure feels like the easiest solution to me.
Regards,
Hans
Hans, that would not solve the problem. In fact, it appears to me that
this problem was already inherent in the driver code before I proposed
any patches at all.
Erm, if I understood correctly (haven't looked yet) the driver is working
with the sof detection from pac_common, which does work with a SOF split
over multiple frames.
That is not my impression of what the code in pac_common is doing. That
code, as I understand, is totally neutral about such things. What is does
is to parse some data and search for an SOF marker, and if it finds such a
thing then it declares the next byte after to be what it calls "sof".
Specifically, there is the function
static unsigned char *pac_find_sof(struct gspca_dev *gspca_dev,
unsigned char *m, int len)
and what it does is that it searches through unsigned char *m up to the
extent declared in int len, looking for an SOF marker. If it finds one,
then it returns the location of the next byte after the SOF marker has been
successfully read.
Check that function again, more carefully, if it fails, but finds a part of
the sof at the end of m it remembers how much of the sof it has already seen
and continues where it left of the next time it is called.
Well, here is the code in the function. I don't see. So can you explain?
Perhaps I am dense.
{
struct sd *sd = (struct sd *) gspca_dev;
int i;
/* Search for the SOF marker (fixed part) in the header */
for (i = 0; i < len; i++) {
if (m[i] == pac_sof_marker[sd->sof_read]) {
sd->sof_read++;
if (sd->sof_read == sizeof(pac_sof_marker)) {
PDEBUG(D_FRAM,
"SOF found, bytes to analyze: %u."
" Frame starts at byte #%u",
len, i + 1);
sd->sof_read = 0;
return m + i + 1;
}
} else {
sd->sof_read = 0;
}
}
return NULL;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: RFC on proposed patches to mr97310a.c for gspca and v4l
kilg...@banach.math.auburn.edu wrote: felling->feeling spelling/writing error (I meant feeling) Thomas -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: RFC on proposed patches to mr97310a.c for gspca and v4l
On Thursday 05 March 2009 14:58:54 kilg...@banach.math.auburn.edu wrote:
> Well, here is the code in the function. I don't see. So can you explain?
> Perhaps I am dense.
>
> {
> struct sd *sd = (struct sd *) gspca_dev;
> int i;
>
> /* Search for the SOF marker (fixed part) in the header */
> for (i = 0; i < len; i++) {
> if (m[i] == pac_sof_marker[sd->sof_read]) {
> sd->sof_read++;
> if (sd->sof_read == sizeof(pac_sof_marker)) {
> PDEBUG(D_FRAM,
> "SOF found, bytes to analyze: %u."
> " Frame starts at byte #%u",
> len, i + 1);
> sd->sof_read = 0;
> return m + i + 1;
> }
> } else {
> sd->sof_read = 0;
> }
> }
>
> return NULL;
> }
We send a chunk of data to this function, as pointed to by m. It could be the
entire transfer buffer or only a part of it, but that doesn't matter. If the
chunk of data ends with FF FF 00 then sd->sof_read will be set to 3 when the
function exits. On the next call it picks up where it left off and looks for
byte 4 of the SOF.
Way back when, I said to copy sd->sof_read bytes from pac_sof_marker if you
want the portion of the SOF that was in the previous transfer. There's no
need to buffer 4 bytes from the previous transfer because the SOF is
_constant_.
So, if it's constant, why do we need to copy it to userspace at all? If we
do, then every frame buffer begins with a constant, useless FF FF 00 FF 96.
The "reassurance" doesn't matter because the frame _must_ have started with
FF FF 00 FF 96 to get there in the first place. I agree with Hans that it
isn't necessary, and by not sending it to userspace we simplify the kernel
driver.
But what if it's not constant? Maybe the SOF is 4 bytes and the 5th byte is
some useful data that, 99.9% of the time, is set to 96? This is the only
reason I see for keeping the SOF.
-Kyle
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: RFC on proposed patches to mr97310a.c for gspca and v4l
On Thu, 5 Mar 2009, Kyle Guinn wrote:
On Thursday 05 March 2009 14:58:54 kilg...@banach.math.auburn.edu wrote:
Well, here is the code in the function. I don't see. So can you explain?
Perhaps I am dense.
{
struct sd *sd = (struct sd *) gspca_dev;
int i;
/* Search for the SOF marker (fixed part) in the header */
for (i = 0; i < len; i++) {
if (m[i] == pac_sof_marker[sd->sof_read]) {
sd->sof_read++;
if (sd->sof_read == sizeof(pac_sof_marker)) {
PDEBUG(D_FRAM,
"SOF found, bytes to analyze: %u."
" Frame starts at byte #%u",
len, i + 1);
sd->sof_read = 0;
return m + i + 1;
}
} else {
sd->sof_read = 0;
}
}
return NULL;
}
We send a chunk of data to this function, as pointed to by m. It could be the
entire transfer buffer or only a part of it, but that doesn't matter. If the
chunk of data ends with FF FF 00 then sd->sof_read will be set to 3 when the
function exits. On the next call it picks up where it left off and looks for
byte 4 of the SOF.
It took me a while to see this, but, yes. So I agree that something needed
to be fixed. It is fixed, now. There is a revised patch.
Way back when, I said to copy sd->sof_read bytes from pac_sof_marker if you
want the portion of the SOF that was in the previous transfer. There's no
need to buffer 4 bytes from the previous transfer because the SOF is
_constant_.
True. So this is the solution which is just now adopted.
So, if it's constant, why do we need to copy it to userspace at all? If we
do, then every frame buffer begins with a constant, useless FF FF 00 FF 96.
The "reassurance" doesn't matter because the frame _must_ have started with
FF FF 00 FF 96 to get there in the first place.
Unless it was a frame from some other camera. But it could have been a
frame dumped from some other camera, and then potentially useful
information for evaluating what it is, would have been lost.
I agree with Hans that it
isn't necessary, and by not sending it to userspace we simplify the kernel
driver.
My experience indicates otherwise. One of the reasons for doing this is,
if one has _all_ of this information it is easier to recognize where it
came from. What kind of camera. What kind of compression. That kind of
thing. It then becomes possible to do things such as to look
at a raw file in total isolation from the streaming app, six months later,
and to be able to know immediately what kind of camera it came from, and
all other information relevant for processing it on the spot with a
program which converts raw data into finished frames or images. If only it
were possible to embed the width and height, somehow, into the header,
then my happiness would be total.
But what if it's not constant? Maybe the SOF is 4 bytes and the 5th byte is
some useful data that, 99.9% of the time, is set to 96? This is the only
reason I see for keeping the SOF.
Very good point. It could happen, couldn't it? It already occurred to me,
actually. We do not know that it will not happen. For, in such a situation
we are at the mercy of some other guys who make cameras. So why paint
oneself into a corner and be sorry later on?
Theodore Kilgore
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: RFC on proposed patches to mr97310a.c for gspca and v4l (headers)
On Fri, 17 Apr 2009, Kyle Guinn wrote: On Friday 17 April 2009 12:50:51 Theodore Kilgore wrote: But I have never seen the 0x64 0xX0 bytes used to count the frames. Could you tell me how to repeat that? It certainly would knock down the validity of the above table wouldn't it? I've modified libv4l to print out the 12-byte header before it skips over it. Good idea, and an obvious one. Why did I not think of that? OK, below are some results for several cameras. They will agree, more or less, with what you get. Then when I fire up mplayer it prints out each header as each frame is received. The framerate is only about 5 fps so there isn't a ton of data to parse through. When I point the camera into a light I get this (at 640x480): ... ff ff 00 ff 96 64 d0 c1 5c c6 00 00 ff ff 00 ff 96 65 50 c1 5c c6 00 00 ff ff 00 ff 96 65 d0 c1 5c c6 00 00 ff ff 00 ff 96 66 50 c1 5c c6 00 00 ff ff 00 ff 96 66 d0 c1 5c c6 00 00 ff ff 00 ff 96 67 50 c1 5c c6 00 00 ff ff 00 ff 96 67 d0 c1 5c c6 00 00 ff ff 00 ff 96 64 50 c1 5c c6 00 00 ff ff 00 ff 96 64 d0 c1 5c c6 00 00 ff ff 00 ff 96 65 50 c1 5c c6 00 00 ... Which camera is this? Is it the Aiptek Pencam VGA+? If so, then I can try it, too, because I also have one of them. Yes, that's the one. Try your others if you can and let me know what happens. Some results follow now for various cameras. For some of them I have taken the trouble to give both 640x480 and 320x240 results. The one camera for which I have only given one result is a CIF camera for which we don't know how to do the decompression. Some headers from the Aiptek Pencam VGA+ (0x08ca: 0x0111) at 640x480 Header: ff ff 00 ff 96 64 d0 37 5a 27 48 91 Header: ff ff 00 ff 96 65 50 2c ce 1a 78 5d Header: ff ff 00 ff 96 65 d0 1b 22 02 1a 4e Header: ff ff 00 ff 96 66 50 0b b0 02 5c 01 Header: ff ff 00 ff 96 66 d0 0a 90 01 ec 09 Header: ff ff 00 ff 96 67 50 0b 81 02 7b fb Header: ff ff 00 ff 96 67 d0 0c 64 01 ec 00 Header: ff ff 00 ff 96 64 50 0c 4e 02 fb f7 Header: ff ff 00 ff 96 64 d0 0c a3 02 eb f2 Header: ff ff 00 ff 96 65 50 0e c5 01 db d5 Header: ff ff 00 ff 96 65 d0 0f b3 03 8b bc Header: ff ff 00 ff 96 66 50 10 03 03 ab bb Header: ff ff 00 ff 96 66 d0 10 28 03 6b c0 Header: ff ff 00 ff 96 67 50 10 9a 03 5b b2 Header: ff ff 00 ff 96 67 d0 11 2a 03 eb 96 Header: ff ff 00 ff 96 64 50 11 54 03 fb 90 Header: ff ff 00 ff 96 64 d0 11 36 03 fb 92 Header: ff ff 00 ff 96 65 50 11 3c 03 fb 8f Header: ff ff 00 ff 96 65 d0 11 41 04 4b 84 Header: ff ff 00 ff 96 66 50 11 5c 04 1b 84 Header: ff ff 00 ff 96 66 d0 11 69 04 3b 80 Header: ff ff 00 ff 96 67 50 11 75 03 fb 7e Header: ff ff 00 ff 96 67 d0 10 b9 03 5b 90 Header: ff ff 00 ff 96 64 50 10 83 03 3b 98 Header: ff ff 00 ff 96 64 d0 11 0e 03 1b 99 Header: ff ff 00 ff 96 65 50 11 70 03 7b 92 Header: ff ff 00 ff 96 65 d0 11 68 03 1b a9 Header: ff ff 00 ff 96 66 50 11 1d 03 9b b2 Header: ff ff 00 ff 96 66 d0 10 e4 03 8b ba Header: ff ff 00 ff 96 67 50 10 ad 03 2b cb Some headers from the Aiptek Pencam VGA+ (0x08ca: 0x0111) at 320x240 Header: ff ff 00 ff 96 64 d0 35 5f 2e 48 a9 Header: ff ff 00 ff 96 65 50 23 f4 11 e9 69 Header: ff ff 00 ff 96 65 d0 17 bf 0a 6b 1d Header: ff ff 00 ff 96 66 50 18 31 0a 5b 11 Header: ff ff 00 ff 96 66 d0 1c df 0d aa 87 Header: ff ff 00 ff 96 67 50 19 71 09 aa db Header: ff ff 00 ff 96 67 d0 12 6f 00 5b cf Header: ff ff 00 ff 96 64 50 0c 46 01 1c 41 Header: ff ff 00 ff 96 64 d0 0e 48 02 5c 09 Header: ff ff 00 ff 96 65 50 0e cf 02 6b fd Header: ff ff 00 ff 96 65 d0 0e 82 02 5c 05 Header: ff ff 00 ff 96 66 50 0e 45 02 5c 08 Header: ff ff 00 ff 96 66 d0 0e 94 02 6c 02 Header: ff ff 00 ff 96 67 50 0e 83 02 7b fd Header: ff ff 00 ff 96 67 d0 0e 6f 02 7c 00 Header: ff ff 00 ff 96 64 50 0e 6e 02 7c 03 Header: ff ff 00 ff 96 64 d0 0e 61 02 4c 04 Header: ff ff 00 ff 96 65 50 0e 86 02 4c 00 Header: ff ff 00 ff 96 65 d0 0e e3 02 8b f2 Header: ff ff 00 ff 96 66 50 0f 62 02 fb e9 Header: ff ff 00 ff 96 66 d0 0e c2 02 ab f6 Header: ff ff 00 ff 96 67 50 0e 76 02 3c 07 Some headers from the "Ion Digital Camera" 0x093a:0x010f, at 640x480 Header: ff ff 00 ff 96 64 d0 20 82 0c e9 af Header: ff ff 00 ff 96 65 50 17 bd 00 a9 c6 Header: ff ff 00 ff 96 65 d0 11 90 00 1c 0c Header: ff ff 00 ff 96 66 50 05 f7 00 7c 2b Header: ff ff 00 ff 96 66 d0 07 4e 01 5c 17 Header: ff ff 00 ff 96 67 50 07 b9 01 8b fb Header: ff ff 00 ff 96 67 d0 08 90 00 fc 05 Header: ff ff 00 ff 96 64 50 09 fc 00 db ef Header: ff ff 00 ff 96 64 d0 0c e6 00 2c 05 Header: ff ff 00 ff 96 65 50 13 10 01 db 98 Header: ff ff 00 ff 96 65 d0 13 54 02 0b 82 Header: ff ff 00 ff 96 66 50 10 d2 02 8b b3 Header: ff ff 00 ff 96 66 d0 0c 46 01 7b e7 Header: ff ff 00 ff 96 67 50 07 1a 00 0c 5d Header: ff ff 00 ff 96 67 d0 06 e4 00 0c 5f Header: ff ff 00 ff 96 64 50 07 8b 00 0c
Re: RFC on proposed patches to mr97310a.c for gspca and v4l (headers)
Replying to myself: Apologies that copying using the Cntrl-R option (read file) in pine made such a mess of the formatting. This was really a mess when I got a copy back again. Sorry, each header _was_ on a separate line :-/ Header: ff ff 00 ff 96 64 d0 37 5a 27 48 91 Header: ff ff 00 ff 96 65 50 2c ce 1a 78 5d Header: ff ff 00 ff 96 65 d0 1b 22 02 1a 4e Header: ff ff 00 ff 96 66 50 0b b0 02 5c 01 Header: ff ff 00 ff 96 66 d0 0a 90 01 ec 09 Header: ff ff 00 ff 96 67 50 0b 81 02 7b fb Header: ff ff 00 ff 96 67 d0 0c 64 01 ec 00 Header: ff ff 00 ff 96 64 50 0c 4e 02 fb f7 Header: ff ff 00 ff 96 64 d0 0c a3 02 eb f2 Header: ff ff 00 ff 96 65 50 0e c5 01 db d5 ... and so on... Theodore Kilgore -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
