[PATCH] gspca_ov534: Print only frame_rate actually used.

[PATCH] gspca_ov534: Print only frame_rate actually used.

[Antonio Ospite]

Print only frame_rate actually used.

It is better to avoid altogether misleading prints of to be discarded values.

Signed-off-by: Antonio Ospite <ospite@studenti.unina.it>

diff -r 8d178f462ba7 linux/drivers/media/video/gspca/ov534.c
--- a/linux/drivers/media/video/gspca/ov534.c	Mon Nov 24 10:38:21 2008 +0100
+++ b/linux/drivers/media/video/gspca/ov534.c	Tue Nov 25 23:41:10 2008 +0100
@@ -364,10 +364,9 @@
 	if (frame_rate > 0)
 		sd->frame_rate = frame_rate;
 
-	PDEBUG(D_PROBE, "frame_rate = %d", sd->frame_rate);
-
 	switch (sd->frame_rate) {
 	case 50:
+		PDEBUG(D_PROBE, "frame_rate = 50");
 		sccb_reg_write(gspca_dev->dev, 0x11, 0x01);
 		sccb_check_status(gspca_dev->dev);
 		sccb_reg_write(gspca_dev->dev, 0x0d, 0x41);
@@ -375,6 +374,7 @@
 		ov534_reg_verify_write(gspca_dev->dev, 0xe5, 0x02);
 		break;
 	case 40:
+		PDEBUG(D_PROBE, "frame_rate = 40");
 		sccb_reg_write(gspca_dev->dev, 0x11, 0x02);
 		sccb_check_status(gspca_dev->dev);
 		sccb_reg_write(gspca_dev->dev, 0x0d, 0xc1);
@@ -383,6 +383,7 @@
 		break;
 	case 30:
 	default:
+		PDEBUG(D_PROBE, "frame_rate = 30");
 		sccb_reg_write(gspca_dev->dev, 0x11, 0x04);
 		sccb_check_status(gspca_dev->dev);
 		sccb_reg_write(gspca_dev->dev, 0x0d, 0x81);
@@ -390,6 +391,7 @@
 		ov534_reg_verify_write(gspca_dev->dev, 0xe5, 0x02);
 		break;
 	case 15:
+		PDEBUG(D_PROBE, "frame_rate = 15");
 		sccb_reg_write(gspca_dev->dev, 0x11, 0x03);
 		sccb_check_status(gspca_dev->dev);
 		sccb_reg_write(gspca_dev->dev, 0x0d, 0x41);

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] gspca_ov534: Print only frame_rate actually used.

[Jean-Francois Moine]

[-- Attachment #1: Type: text/plain, Size: 722 bytes --]

On Tue, 2008-11-25 at 23:52 +0100, Antonio Ospite wrote:
> Print only frame_rate actually used.

Hello Antonio,

This may be simplified as in the attached patch (the frame_rate in the
sd structure was not used).

The patch also includes removing the bulk_size setting at streamon time:
the value is already used at this time, and also, there is only one
resolution.

I found a real problem: for USB read and write, you have a 16-bits
variable in/from which you read/write only one byte. This will fail with
big-endian machines. Anyway, it is safer to use the usb_buf from the
gspca structure.

Cheers.

-- 
Ken ar c'hentañ |             ** Breizh ha Linux atav! **
Jef             |               http://moinejf.free.fr/


[-- Attachment #2: ov534.patch --]
[-- Type: text/x-patch, Size: 2153 bytes --]

diff -r 3e0ba0a8e47f linux/drivers/media/video/gspca/ov534.c
--- a/linux/drivers/media/video/gspca/ov534.c	Wed Nov 26 20:17:13 2008 +0100
+++ b/linux/drivers/media/video/gspca/ov534.c	Thu Nov 27 10:15:08 2008 +0100
@@ -48,7 +48,6 @@
 /* specific webcam descriptor */
 struct sd {
 	struct gspca_dev gspca_dev;	/* !! must be the first item */
-	__u8 frame_rate;
 };
 
 /* V4L2 controls supported by the driver */
@@ -59,7 +58,7 @@
 	{640, 480, V4L2_PIX_FMT_YUYV, V4L2_FIELD_NONE,
 	 .bytesperline = 640 * 2,
 	 .sizeimage = 640 * 480 * 2,
-	 .colorspace = V4L2_COLORSPACE_JPEG,
+	 .colorspace = V4L2_COLORSPACE_SRGB,
 	 .priv = 0},
 };
 
@@ -359,14 +358,13 @@
 static int sd_init(struct gspca_dev *gspca_dev)
 {
 	struct sd *sd = (struct sd *)gspca_dev;
+	int fr;
+
 	ov534_setup(gspca_dev->dev);
 
-	if (frame_rate > 0)
-		sd->frame_rate = frame_rate;
+	fr = frame_rate;
 
-	PDEBUG(D_PROBE, "frame_rate = %d", sd->frame_rate);
-
-	switch (sd->frame_rate) {
+	switch (fr) {
 	case 50:
 		sccb_reg_write(gspca_dev->dev, 0x11, 0x01);
 		sccb_check_status(gspca_dev->dev);
@@ -381,8 +379,9 @@
 		sccb_check_status(gspca_dev->dev);
 		ov534_reg_verify_write(gspca_dev->dev, 0xe5, 0x04);
 		break;
-	case 30:
+/*	case 30: */
 	default:
+		fr = 30;
 		sccb_reg_write(gspca_dev->dev, 0x11, 0x04);
 		sccb_check_status(gspca_dev->dev);
 		sccb_reg_write(gspca_dev->dev, 0x0d, 0x81);
@@ -396,18 +395,15 @@
 		sccb_check_status(gspca_dev->dev);
 		ov534_reg_verify_write(gspca_dev->dev, 0xe5, 0x04);
 		break;
-	};
+	}
+
+	PDEBUG(D_PROBE, "frame_rate: %d", fr);
 
 	return 0;
 }
 
 static int sd_start(struct gspca_dev *gspca_dev)
 {
-	PDEBUG(D_PROBE, "width = %d, height = %d",
-	       gspca_dev->width, gspca_dev->height);
-
-	gspca_dev->cam.bulk_size = gspca_dev->width * gspca_dev->height * 2;
-
 	/* start streaming data */
 	ov534_set_led(gspca_dev->dev, 1);
 	ov534_reg_write(gspca_dev->dev, 0xe0, 0x00);
@@ -433,7 +429,6 @@
 	int framesize = gspca_dev->cam.bulk_size;
 
 	if (len == framesize - 4) {
-		frame =
 		    gspca_frame_add(gspca_dev, FIRST_PACKET, frame, data, len);
 		frame =
 		    gspca_frame_add(gspca_dev, LAST_PACKET, frame, last_pixel,

[-- Attachment #3: Type: text/plain, Size: 164 bytes --]

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] gspca_ov534: Print only frame_rate actually used.

[Antonio Ospite]

[-- Attachment #1.1: Type: text/plain, Size: 2688 bytes --]

On Thu, 27 Nov 2008 10:23:04 +0100
Jean-Francois Moine <moinejf@free.fr> wrote:

> On Tue, 2008-11-25 at 23:52 +0100, Antonio Ospite wrote:
> > Print only frame_rate actually used.
> 
> Hello Antonio,
> 
> This may be simplified as in the attached patch (the frame_rate in the
> sd structure was not used).
>

Some questions inlined. I am still learning.

> The patch also includes removing the bulk_size setting at streamon time:
> the value is already used at this time, and also, there is only one
> resolution.
>

We will add this again when we add other resolutions, OK.

> I found a real problem: for USB read and write, you have a 16-bits
> variable in/from which you read/write only one byte. This will fail with
> big-endian machines. Anyway, it is safer to use the usb_buf from the
> gspca structure.
>

Ah, you mean in control messages, yes, those always use size=1 so a u8
can be used there.
I'll give a look and will do some tests on a real PS3.

Thanks.

Regards,
   Antonio Ospite.

> Cheers.
> 
> -- 
> Ken ar c'hentañ |             ** Breizh ha Linux atav! **
> Jef             |               http://moinejf.free.fr/
> 
> 
> 
> [ov534.patch  text/x-patch (2,1KB)]
> diff -r 3e0ba0a8e47f linux/drivers/media/video/gspca/ov534.c
> --- a/linux/drivers/media/video/gspca/ov534.c	Wed Nov 26 20:17:13 2008 +0100
> +++ b/linux/drivers/media/video/gspca/ov534.c	Thu Nov 27 10:15:08 2008 +0100
> @@ -48,7 +48,6 @@
>  /* specific webcam descriptor */
>  struct sd {
>  	struct gspca_dev gspca_dev;	/* !! must be the first item */
> -	__u8 frame_rate;
>  };
>  
>  /* V4L2 controls supported by the driver */
> @@ -59,7 +58,7 @@
>  	{640, 480, V4L2_PIX_FMT_YUYV, V4L2_FIELD_NONE,
>  	 .bytesperline = 640 * 2,
>  	 .sizeimage = 640 * 480 * 2,
> -	 .colorspace = V4L2_COLORSPACE_JPEG,
> +	 .colorspace = V4L2_COLORSPACE_SRGB,
>  	 .priv = 0},
>  };
>

Can you explain this one, please?

[snip]
> @@ -433,7 +429,6 @@
>  	int framesize = gspca_dev->cam.bulk_size;
>  
>  	if (len == framesize - 4) {
> -		frame =
>  		    gspca_frame_add(gspca_dev, FIRST_PACKET, frame, data, len);

This change is just to follow the convention used by other drivers,
right? You could also adjust indentation on following line, then.

>  		frame =
>  		    gspca_frame_add(gspca_dev, LAST_PACKET, frame, last_pixel,
> 


-- 
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

  Web site: http://www.studenti.unina.it/~ospite
Public key: http://www.studenti.unina.it/~ospite/aopubkey.asc

[-- Attachment #1.2: Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 164 bytes --]

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] gspca_ov534: Print only frame_rate actually used.

[Jean-Francois Moine]

On Thu, 2008-11-27 at 12:05 +0100, Antonio Ospite wrote:
> > The patch also includes removing the bulk_size setting at streamon time:
> > the value is already used at this time, and also, there is only one
> > resolution.
> We will add this again when we add other resolutions, OK.

The bulk_size must be set at the max resolution because it is used for
buffer allocation before stream on.

	[snip]
> >  /* V4L2 controls supported by the driver */
> > @@ -59,7 +58,7 @@
> >  	{640, 480, V4L2_PIX_FMT_YUYV, V4L2_FIELD_NONE,
> >  	 .bytesperline = 640 * 2,
> >  	 .sizeimage = 640 * 480 * 2,
> > -	 .colorspace = V4L2_COLORSPACE_JPEG,
> > +	 .colorspace = V4L2_COLORSPACE_SRGB,
> >  	 .priv = 0},
> >  };
> >
> 
> Can you explain this one, please?

I think the JPEG images embed colorspace information, and here, it is
simple RGB.

> [snip]
> > @@ -433,7 +429,6 @@
> >  	int framesize = gspca_dev->cam.bulk_size;
> >  
> >  	if (len == framesize - 4) {
> > -		frame =
> >  		    gspca_frame_add(gspca_dev, FIRST_PACKET, frame, data, len);
> 
> This change is just to follow the convention used by other drivers,
> right? You could also adjust indentation on following line, then.

If you look carefully, the frame returned by gspca_frame_add() is
changed only when the packet type is LAST_PACKET. OK for the
indentation.

Cheers.

-- 
Ken ar c'hentañ |             ** Breizh ha Linux atav! **
Jef             |               http://moinejf.free.fr/


--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] gspca_ov534: Print only frame_rate actually used.

[Antonio Ospite]

[-- Attachment #1.1: Type: text/plain, Size: 2606 bytes --]

On Thu, 27 Nov 2008 13:22:33 +0100
Jean-Francois Moine <moinejf@free.fr> wrote:

> On Thu, 2008-11-27 at 12:05 +0100, Antonio Ospite wrote:
> > > The patch also includes removing the bulk_size setting at streamon time:
> > > the value is already used at this time, and also, there is only one
> > > resolution.
> > We will add this again when we add other resolutions, OK.
> 
> The bulk_size must be set at the max resolution because it is used for
> buffer allocation before stream on.
>

Well, isn't it only used in create_urbs()? AFAICS the latter uses it to
set urb->tranfer_buffer_length and is called at streamon time, so it
could still be worth to set bulk_size to exact value for the current
resolution to have more efficient transfers, what do you think?

> 	[snip]
> > >  /* V4L2 controls supported by the driver */
> > > @@ -59,7 +58,7 @@
> > >  	{640, 480, V4L2_PIX_FMT_YUYV, V4L2_FIELD_NONE,
> > >  	 .bytesperline = 640 * 2,
> > >  	 .sizeimage = 640 * 480 * 2,
> > > -	 .colorspace = V4L2_COLORSPACE_JPEG,
> > > +	 .colorspace = V4L2_COLORSPACE_SRGB,
> > >  	 .priv = 0},
> > >  };
> > >
> > 
> > Can you explain this one, please?
> 
> I think the JPEG images embed colorspace information, and here, it is
> simple RGB.
>

Also other drivers are setting this colorspace for raw YCbCr data, see
mt9m111.c:
COL_FMT("YCrYCb 16 bit", 16, V4L2_PIX_FMT_YUYV, V4L2_COLORSPACE_JPEG),

and tcm825x.c:
switch (pix->pixelformat) {
	case V4L2_PIX_FMT_UYVY:
	default:
		pix->colorspace = V4L2_COLORSPACE_JPEG;
		break;

and also the comment relative to V4L2_COLORSPACE_JPEG mentions YCbCr,
but honestly, I don't know how this colorspace info is used that's why I
am asking.

> > [snip]
> > > @@ -433,7 +429,6 @@
> > >  	int framesize = gspca_dev->cam.bulk_size;
> > >  
> > >  	if (len == framesize - 4) {
> > > -		frame =
> > >  		    gspca_frame_add(gspca_dev, FIRST_PACKET, frame, data, len);
> > 
> > This change is just to follow the convention used by other drivers,
> > right? You could also adjust indentation on following line, then.
> 
> If you look carefully, the frame returned by gspca_frame_add() is
> changed only when the packet type is LAST_PACKET. OK for the
> indentation.
>

OK, thanks.

Regards,
   Antonio Ospite

-- 
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

  Web site: http://www.studenti.unina.it/~ospite
Public key: http://www.studenti.unina.it/~ospite/aopubkey.asc

[-- Attachment #1.2: Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 164 bytes --]

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] gspca_ov534: Print only frame_rate actually used.

[Antonio Ospite]

[-- Attachment #1.1: Type: text/plain, Size: 3906 bytes --]

On Thu, 27 Nov 2008 14:52:33 +0100
Antonio Ospite <ospite@studenti.unina.it> wrote:

> On Thu, 27 Nov 2008 13:22:33 +0100
> Jean-Francois Moine <moinejf@free.fr> wrote:
> 
> > On Thu, 2008-11-27 at 12:05 +0100, Antonio Ospite wrote:
> > > > The patch also includes removing the bulk_size setting at streamon time:
> > > > the value is already used at this time, and also, there is only one
> > > > resolution.
> > > We will add this again when we add other resolutions, OK.
> > 
> > The bulk_size must be set at the max resolution because it is used for
> > buffer allocation before stream on.
> >
> 
> Well, isn't it only used in create_urbs()? AFAICS the latter uses it to
> set urb->tranfer_buffer_length and is called at streamon time, so it
> could still be worth to set bulk_size to exact value for the current
> resolution to have more efficient transfers, what do you think?
> 

Hi Jean-Francois,

it turned out that on PS3 the big current bulk_size doesn't work,
because here the hardware needs contiguous memory for transfers (if
the description is somewhat imprecise please correct me) and it is not
always possible to allocate such a big contiguous chunk, so we have to
use a smaller fixed size for bulk transfers and fill the frame in
several passes in pkt_scan.

I am preparing some patches, but I come across a problem.

Now I use this code in pkt_scan (I add a zero length FIRST_PACKET in
sd_start):

	if ((frame->data_end - frame->data + len) == (framesize - 4)) {
		PDEBUG(D_PACK, "  end of frame?");
		gspca_frame_add(gspca_dev, INTER_PACKET, frame, data, len);
		frame = gspca_frame_add(gspca_dev, LAST_PACKET, frame, last, 4);
		gspca_frame_add(gspca_dev, FIRST_PACKET, frame, data, 0);
	} else
		gspca_frame_add(gspca_dev, INTER_PACKET, frame, data, len);

When everything works ok I get this log messages:

  ov534: ** packet len = 16384, framesize = 614400
  ov534: ** frame->data_end - frame->data + len = 16384
  ov534: 
  ...
  ov534: ** packet len = 8188, framesize = 614400
  ov534: ** frame->data_end - frame->data + len = 614396
  ov534:    end of frame!
  ov534: 
  ov534: ** packet len = 16384, framesize = 614400
  ov534: ** frame->data_end - frame->data + len = 16384
  ov534: 
  ...

But sometime, at higher frame rates, it doesn't work,
frames are discarded because I get:

  ov534: ** packet len = 16384, framesize = 614400
  ov534: ** frame->data_end - frame->data + len = 16384
  ov534: 
  ...
  ov534: ** packet len = 8188, framesize = 614400
  ov534: ** frame->data_end - frame->data + len = 614396
  ov534:    end of frame!
  ov534: 
  ov534: ** packet len = 16384, framesize = 614400
  ov534: ** frame->data_end - frame->data + len = 630784
  ov534:                                          ^^^^^^
  ...                                             ^^^^^^

It looks like something is failing in gspca_frame_add(), it doesn't get
to the statement frame->data_end = frame->data; when adding the
FIRST_PACKET. That could be because the packet has been queued but not
added yet, AFAIU. How to deal with such case?

As a side note, if I use this check to detect the end of the frame:

	if (len < gspca_dev->cam.bulk_size) {
		...
	} else ...

I can recover from the previous error even if I get some frame
discarded from time to time. Is this check acceptable to you If I take
care that framesize is not a multiple of bulk_size?

The current quilt patchset is here:
http://shell.studenti.unina.it/~ospite/tmp/gspca_ov534_patches-2008-12-03.tar.gz

Regards,
   Antonio Ospite

-- 
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

  Web site: http://www.studenti.unina.it/~ospite
Public key: http://www.studenti.unina.it/~ospite/aopubkey.asc

[-- Attachment #1.2: Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 164 bytes --]

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] gspca_ov534: Print only frame_rate actually used.

[Jim Paris]

Hi Antonio,

Antonio Ospite wrote:
> it turned out that on PS3 the big current bulk_size doesn't work,
> because here the hardware needs contiguous memory for transfers (if
> the description is somewhat imprecise please correct me) and it is not
> always possible to allocate such a big contiguous chunk, so we have to
> use a smaller fixed size for bulk transfers and fill the frame in
> several passes in pkt_scan.

Such a large bulk_size is probably not a good idea on any system.
The PS3 in particular is already memory constrained so it's definitely
a good idea to break that up.

> I am preparing some patches, but I come across a problem.
> 
> Now I use this code in pkt_scan (I add a zero length FIRST_PACKET in
> sd_start):
> 
> 	if ((frame->data_end - frame->data + len) == (framesize - 4)) {
> 		PDEBUG(D_PACK, "  end of frame?");
> 		gspca_frame_add(gspca_dev, INTER_PACKET, frame, data, len);
> 		frame = gspca_frame_add(gspca_dev, LAST_PACKET, frame, last, 4);
> 		gspca_frame_add(gspca_dev, FIRST_PACKET, frame, data, 0);
> 	} else
> 		gspca_frame_add(gspca_dev, INTER_PACKET, frame, data, len);
...
> It looks like something is failing in gspca_frame_add(), it doesn't get
> to the statement frame->data_end = frame->data; when adding the
> FIRST_PACKET. That could be because the packet has been queued but not
> added yet, AFAIU. How to deal with such case?

If gspca decides to discard a frame with DISCARD_PACKET, I believe it
won't start working again until you send another FIRST_PACKET.  In
your code, if DISCARD_PACKET ever happens, the frame->data pointers
won't get updated so you'll never get to FIRST_PACKET?

> As a side note, if I use this check to detect the end of the frame:
> 
> 	if (len < gspca_dev->cam.bulk_size) {
> 		...
> 	} else ...
> 
> I can recover from the previous error even if I get some frame
> discarded from time to time. Is this check acceptable to you If I take
> care that framesize is not a multiple of bulk_size?

Hold off a bit on this work.

There's a problem with breaking up the transfers, because we're not
currently getting any header data from the bridge chip that lets us
know when we really hit the end of a frame, and it's easy to get out
of sync.  Using (len < bulk_size) is a good trick if they're not a
multiple, as you say, since the last payload will be shorter, but I
have a better solution -- I found how to get the camera to add a
UVC-format header on each payload.  I'm finishing up the patch and
will post it a bit later today once I iron out a few bugs.

-jim

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] gspca_ov534: Print only frame_rate actually used.

[Antonio Ospite]

[-- Attachment #1.1: Type: text/plain, Size: 2291 bytes --]

On Wed, 3 Dec 2008 13:01:28 -0500
Jim Paris <jim@jtan.com> wrote:

> Hi Antonio,
> 
> Antonio Ospite wrote:
> 
> If gspca decides to discard a frame with DISCARD_PACKET, I believe it
> won't start working again until you send another FIRST_PACKET.  In
> your code, if DISCARD_PACKET ever happens, the frame->data pointers
> won't get updated so you'll never get to FIRST_PACKET?
>

Yes, the code takes this path in gspca.c, gspca_frame_add(), line 277:

	if (packet_type == FIRST_PACKET) {
		if ((frame->v4l2_buf.flags & BUF_ALL_FLAGS)
						!= V4L2_BUF_FLAG_QUEUED) {
			gspca_dev->last_packet_type = DISCARD_PACKET;
			return frame;
		}

and then it keeps on adding only INTER_PACKETs because of the current
end of frame check. It is a timing issue, it happens only with high
frame_rates, maybe there is some code in gspca that needs to be
protected by locking?
Or would it be normal to loose some frames at high frame rates using
smaller bulk_size?

> > As a side note, if I use this check to detect the end of the frame:
> > 
> > 	if (len < gspca_dev->cam.bulk_size) {
> > 		...
> > 	} else ...
> > 
> > I can recover from the previous error even if I get some frame
> > discarded from time to time. Is this check acceptable to you If I take
> > care that framesize is not a multiple of bulk_size?
> 
> Hold off a bit on this work.
> 
> There's a problem with breaking up the transfers, because we're not
> currently getting any header data from the bridge chip that lets us
> know when we really hit the end of a frame, and it's easy to get out
> of sync.  Using (len < bulk_size) is a good trick if they're not a
> multiple, as you say, since the last payload will be shorter, but I
> have a better solution -- I found how to get the camera to add a
> UVC-format header on each payload.  I'm finishing up the patch and
> will post it a bit later today once I iron out a few bugs.
> 
> -jim

Ok, many thanks.

Regards,
   Antonio


-- 
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

  Web site: http://www.studenti.unina.it/~ospite
Public key: http://www.studenti.unina.it/~ospite/aopubkey.asc

[-- Attachment #1.2: Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 164 bytes --]

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] gspca_ov534: Print only frame_rate actually used.

[Jean-Francois Moine]

[-- Attachment #1: Type: text/plain, Size: 2536 bytes --]

On Wed, 2008-12-03 at 19:44 +0100, Antonio Ospite wrote:
> Yes, the code takes this path in gspca.c, gspca_frame_add(), line 277:
> 
> 	if (packet_type == FIRST_PACKET) {
> 		if ((frame->v4l2_buf.flags & BUF_ALL_FLAGS)
> 						!= V4L2_BUF_FLAG_QUEUED) {
> 			gspca_dev->last_packet_type = DISCARD_PACKET;
> 			return frame;
> 		}
> 
> and then it keeps on adding only INTER_PACKETs because of the current
> end of frame check. It is a timing issue, it happens only with high
> frame_rates, maybe there is some code in gspca that needs to be
> protected by locking?
> Or would it be normal to loose some frames at high frame rates using
> smaller bulk_size?
> 
> > > As a side note, if I use this check to detect the end of the frame:
> > > 
> > > 	if (len < gspca_dev->cam.bulk_size) {
> > > 		...
> > > 	} else ...
> > > 
> > > I can recover from the previous error even if I get some frame
> > > discarded from time to time. Is this check acceptable to you If I take
> > > care that framesize is not a multiple of bulk_size?
> > 
> > Hold off a bit on this work.
> > 
> > There's a problem with breaking up the transfers, because we're not
> > currently getting any header data from the bridge chip that lets us
> > know when we really hit the end of a frame, and it's easy to get out
> > of sync.  Using (len < bulk_size) is a good trick if they're not a
> > multiple, as you say, since the last payload will be shorter, but I
> > have a better solution -- I found how to get the camera to add a
> > UVC-format header on each payload.  I'm finishing up the patch and
> > will post it a bit later today once I iron out a few bugs.
> > 
> > -jim

Hi Antonio,

The problem comes from the availability of the application buffer. In
the bulk_irq, there is:

	frame = gspca_get_i_frame(gspca_dev);
	if (!frame) {
		gspca_dev->last_packet_type = DISCARD_PACKET;
	} else {
		.. pkt_scan(..);
	}

Then, you are not called and you cannot know how many bytes have been
really received.

As the buffer check exists in frame_add, I may call you each time with a
valid frame pointer (see patch). In this case, you cannot count the
image bytes with the data_end. You should have a counter in the sd
structure.

An other solution is to start and stop the transfer for each image as it
was in the original driver, but it asks for a kernel thread.

Anyway, if Jim may add a mark between the images, if will be the best...

Cheers.

-- 
Ken ar c'hentañ |             ** Breizh ha Linux atav! **
Jef             |               http://moinejf.free.fr/


[-- Attachment #2: gspca.patch --]
[-- Type: text/x-patch, Size: 1061 bytes --]

diff -r d23374509b5b linux/drivers/media/video/gspca/gspca.c
--- a/linux/drivers/media/video/gspca/gspca.c	Wed Dec 03 18:10:01 2008 +0100
+++ b/linux/drivers/media/video/gspca/gspca.c	Wed Dec 03 20:11:23 2008 +0100
@@ -211,6 +211,7 @@
 {
 	struct gspca_dev *gspca_dev = (struct gspca_dev *) urb->context;
 	struct gspca_frame *frame;
+	int i;
 	int st;
 
 	PDEBUG(D_PACK, "bulk irq");
@@ -231,16 +232,14 @@
 	}
 
 	/* check the availability of the frame buffer */
-	frame = gspca_get_i_frame(gspca_dev);
-	if (!frame) {
-		gspca_dev->last_packet_type = DISCARD_PACKET;
-	} else {
-		PDEBUG(D_PACK, "packet l:%d", urb->actual_length);
-		gspca_dev->sd_desc->pkt_scan(gspca_dev,
-					frame,
-					urb->transfer_buffer,
-					urb->actual_length);
-	}
+	i = gspca_dev->fr_i;
+	i = gspca_dev->fr_queue[i];
+	frame = &gspca_dev->frame[i];
+	PDEBUG(D_PACK, "packet l:%d", urb->actual_length);
+	gspca_dev->sd_desc->pkt_scan(gspca_dev,
+				frame,
+				urb->transfer_buffer,
+				urb->actual_length);
 
 	/* resubmit the URB */
 	if (gspca_dev->cam.bulk_nurbs != 0) {

[-- Attachment #3: Type: text/plain, Size: 164 bytes --]

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] gspca_ov534: Print only frame_rate actually used.

[Antonio Ospite]

[-- Attachment #1.1: Type: text/plain, Size: 2167 bytes --]

On Wed, 03 Dec 2008 20:17:20 +0100
Jean-Francois Moine <moinejf@free.fr> wrote:

> On Wed, 2008-12-03 at 19:44 +0100, Antonio Ospite wrote:
> > Yes, the code takes this path in gspca.c, gspca_frame_add(), line 277:
> > 
> > 	if (packet_type == FIRST_PACKET) {
> > 		if ((frame->v4l2_buf.flags & BUF_ALL_FLAGS)
> > 						!= V4L2_BUF_FLAG_QUEUED) {
> > 			gspca_dev->last_packet_type = DISCARD_PACKET;
> > 			return frame;
> > 		}
> > 
> > and then it keeps on adding only INTER_PACKETs because of the current
> > end of frame check. It is a timing issue, it happens only with high
> > frame_rates, maybe there is some code in gspca that needs to be
> > protected by locking?
> > Or would it be normal to loose some frames at high frame rates using
> > smaller bulk_size?
> > 
> 
> Hi Antonio,
> 
> The problem comes from the availability of the application buffer. In
> the bulk_irq, there is:
> 
> 	frame = gspca_get_i_frame(gspca_dev);
> 	if (!frame) {
> 		gspca_dev->last_packet_type = DISCARD_PACKET;
> 	} else {
> 		.. pkt_scan(..);
> 	}
> 
> Then, you are not called and you cannot know how many bytes have been
> really received.
>

And this happens because we couldn't have two separate events for
LAST_PACKET and FIRST_PACKET, right?

> As the buffer check exists in frame_add, I may call you each time with a
> valid frame pointer (see patch). In this case, you cannot count the
> image bytes with the data_end. You should have a counter in the sd
> structure.
> 
> An other solution is to start and stop the transfer for each image as it
> was in the original driver, but it asks for a kernel thread.
> 
> Anyway, if Jim may add a mark between the images, if will be the best...
>

Ok, we'll go with Jim changes. Thanks anyway for the patch, but I don't
think it is needed anymore.

Regards,
   Antonio

-- 
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

  Web site: http://www.studenti.unina.it/~ospite
Public key: http://www.studenti.unina.it/~ospite/aopubkey.asc

[-- Attachment #1.2: Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 164 bytes --]

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list