diff mbox

[resend] usbvision: disable scaling for Nogatech MicroCam

Message ID 201107222200.55834.linux@rainbow-software.org (mailing list archive)
State Changes Requested
Headers show

Commit Message

Ondrej Zary July 22, 2011, 8 p.m. UTC 
Scaling causes bad artifacts (horizontal lines) with compression at least
with Nogatech MicroCam so disable it (for this HW).

This also fixes messed up image with some programs (Cheese with 160x120,
Adobe Flash). HW seems to support only image widths that are multiple of 64
but the driver does not account that in vidioc_try_fmt_vid_cap(). Cheese
calls try_fmt with 160x120, succeeds and then assumes that it really gets
data in that resolution - but it gets 128x120 instead. Don't know if this
affects other usbvision devices, it would be great if someone could test it.

Signed-off-by: Ondrej Zary <linux@rainbow-software.org>

Comments

Devin Heitmueller July 22, 2011, 8:06 p.m. UTC | #1 
On Fri, Jul 22, 2011 at 4:00 PM, Ondrej Zary <linux@rainbow-software.org> wrote:
> Scaling causes bad artifacts (horizontal lines) with compression at least
> with Nogatech MicroCam so disable it (for this HW).
>
> This also fixes messed up image with some programs (Cheese with 160x120,
> Adobe Flash). HW seems to support only image widths that are multiple of 64
> but the driver does not account that in vidioc_try_fmt_vid_cap(). Cheese
> calls try_fmt with 160x120, succeeds and then assumes that it really gets
> data in that resolution - but it gets 128x120 instead. Don't know if this
> affects other usbvision devices, it would be great if someone could test it.
>
> Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
>
> diff -urp linux-2.6.39-rc2-/drivers/media/video/usbvision//usbvision-video.c linux-2.6.39-rc2/drivers/media/video/usbvision/usbvision-video.c
> --- linux-2.6.39-rc2-/drivers/media/video/usbvision//usbvision-video.c  2011-07-16 16:42:35.000000000 +0200
> +++ linux-2.6.39-rc2/drivers/media/video/usbvision/usbvision-video.c    2011-07-16 16:36:43.000000000 +0200
> @@ -924,6 +924,11 @@ static int vidioc_try_fmt_vid_cap(struct
>        RESTRICT_TO_RANGE(vf->fmt.pix.width, MIN_FRAME_WIDTH, MAX_FRAME_WIDTH);
>        RESTRICT_TO_RANGE(vf->fmt.pix.height, MIN_FRAME_HEIGHT, MAX_FRAME_HEIGHT);
>
> +       if (usbvision_device_data[usbvision->dev_model].codec == CODEC_WEBCAM) {
> +               vf->fmt.pix.width = MAX_FRAME_WIDTH;
> +               vf->fmt.pix.height = MAX_FRAME_HEIGHT;
> +       }
> +
>        vf->fmt.pix.bytesperline = vf->fmt.pix.width*
>                usbvision->palette.bytes_per_pixel;
>        vf->fmt.pix.sizeimage = vf->fmt.pix.bytesperline*vf->fmt.pix.height;
> @@ -952,6 +957,11 @@ static int vidioc_s_fmt_vid_cap(struct f
>
>        usbvision->cur_frame = NULL;
>
> +       if (usbvision_device_data[usbvision->dev_model].codec == CODEC_WEBCAM) {
> +               vf->fmt.pix.width = MAX_FRAME_WIDTH;
> +               vf->fmt.pix.height = MAX_FRAME_HEIGHT;
> +       }
> +
>        /* by now we are committed to the new data... */
>        usbvision_set_output(usbvision, vf->fmt.pix.width, vf->fmt.pix.height);
>
>
> --
> Ondrej Zary

Hello Ondrej,

Drivers are permitted to return a different resolution than what the
application provided in the S_FMT call.  It is the responsibility of
the application to look at the struct after the ioctl() call and if
the values are not what it expects to then accommodate the change.

In other words, this sounds like a bug in Cheese.

Devin
Ondrej Zary July 22, 2011, 9:22 p.m. UTC | #2 
On Friday 22 July 2011 22:06:59 Devin Heitmueller wrote:
> On Fri, Jul 22, 2011 at 4:00 PM, Ondrej Zary <linux@rainbow-software.org> wrote:
> > Scaling causes bad artifacts (horizontal lines) with compression at least
> > with Nogatech MicroCam so disable it (for this HW).
> >
> > This also fixes messed up image with some programs (Cheese with 160x120,
> > Adobe Flash). HW seems to support only image widths that are multiple of 64
> > but the driver does not account that in vidioc_try_fmt_vid_cap(). Cheese
> > calls try_fmt with 160x120, succeeds and then assumes that it really gets
> > data in that resolution - but it gets 128x120 instead. Don't know if this
> > affects other usbvision devices, it would be great if someone could test it.
> >
> > Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
> >
> > diff -urp linux-2.6.39-rc2-/drivers/media/video/usbvision//usbvision-video.c linux-2.6.39-rc2/drivers/media/video/usbvision/usbvision-video.c
> > --- linux-2.6.39-rc2-/drivers/media/video/usbvision//usbvision-video.c  2011-07-16 16:42:35.000000000 +0200
> > +++ linux-2.6.39-rc2/drivers/media/video/usbvision/usbvision-video.c    2011-07-16 16:36:43.000000000 +0200
> > @@ -924,6 +924,11 @@ static int vidioc_try_fmt_vid_cap(struct
> >        RESTRICT_TO_RANGE(vf->fmt.pix.width, MIN_FRAME_WIDTH, MAX_FRAME_WIDTH);
> >        RESTRICT_TO_RANGE(vf->fmt.pix.height, MIN_FRAME_HEIGHT, MAX_FRAME_HEIGHT);
> >
> > +       if (usbvision_device_data[usbvision->dev_model].codec == CODEC_WEBCAM) {
> > +               vf->fmt.pix.width = MAX_FRAME_WIDTH;
> > +               vf->fmt.pix.height = MAX_FRAME_HEIGHT;
> > +       }
> > +
> >        vf->fmt.pix.bytesperline = vf->fmt.pix.width*
> >                usbvision->palette.bytes_per_pixel;
> >        vf->fmt.pix.sizeimage = vf->fmt.pix.bytesperline*vf->fmt.pix.height;
> > @@ -952,6 +957,11 @@ static int vidioc_s_fmt_vid_cap(struct f
> >
> >        usbvision->cur_frame = NULL;
> >
> > +       if (usbvision_device_data[usbvision->dev_model].codec == CODEC_WEBCAM) {
> > +               vf->fmt.pix.width = MAX_FRAME_WIDTH;
> > +               vf->fmt.pix.height = MAX_FRAME_HEIGHT;
> > +       }
> > +
> >        /* by now we are committed to the new data... */
> >        usbvision_set_output(usbvision, vf->fmt.pix.width, vf->fmt.pix.height);
> >
> >
> > --
> > Ondrej Zary
> 
> Hello Ondrej,
> 
> Drivers are permitted to return a different resolution than what the
> application provided in the S_FMT call.  It is the responsibility of
> the application to look at the struct after the ioctl() call and if
> the values are not what it expects to then accommodate the change.
> 
> In other words, this sounds like a bug in Cheese.

Seems that this bug is widespread - the same problem appears also in guvcview
and adobe flash. I think that the driver is broken too - it should return
corrected resolution in TRY_FMT.

> Devin
>
Devin Heitmueller July 22, 2011, 9:31 p.m. UTC | #3 
On Fri, Jul 22, 2011 at 5:22 PM, Ondrej Zary <linux@rainbow-software.org> wrote:
> Seems that this bug is widespread - the same problem appears also in guvcview
> and adobe flash. I think that the driver is broken too - it should return
> corrected resolution in TRY_FMT.

Well, if the driver does not return the resolution it is actually
capturing in, then that indeed is a driver bug.  That's a different
issue though than what your patch proposed.

You should make the TRY_FMT call specifying an invalid resolution and
see if it returns the real resolution the device will capture at.  If
it doesn't, then fix *that* bug.

The application needs to know the capturing resolution, so it's
possible that it does properly handle the driver passing back the real
resolution and the driver is at fault, in which case no change is
needed to the app at all.

Devin
Ondrej Zary July 22, 2011, 9:44 p.m. UTC | #4 
On Friday 22 July 2011 23:31:46 Devin Heitmueller wrote:
> On Fri, Jul 22, 2011 at 5:22 PM, Ondrej Zary <linux@rainbow-software.org> wrote:
> > Seems that this bug is widespread - the same problem appears also in guvcview
> > and adobe flash. I think that the driver is broken too - it should return
> > corrected resolution in TRY_FMT.
> 
> Well, if the driver does not return the resolution it is actually
> capturing in, then that indeed is a driver bug.  That's a different
> issue though than what your patch proposed.
> 
> You should make the TRY_FMT call specifying an invalid resolution and
> see if it returns the real resolution the device will capture at.  If
> it doesn't, then fix *that* bug.

It does not, there's no code that would do that. I actually fixed that only
to find out that the scaling is unusable at least with the MicroCam because
of black horizontal lines that appear in the image (amount of the lines
depend on the scaling factor). So I just disabled the scaling completely for
MicroCam.

I also don't know if the multiple-of-64 width limit is valid for all
usbvision devices - that's why I haven't posted patch to fix this.

> The application needs to know the capturing resolution, so it's
> possible that it does properly handle the driver passing back the real
> resolution and the driver is at fault, in which case no change is
> needed to the app at all.
> 
> Devin
>
diff mbox

Patch

diff -urp linux-2.6.39-rc2-/drivers/media/video/usbvision//usbvision-video.c linux-2.6.39-rc2/drivers/media/video/usbvision/usbvision-video.c
--- linux-2.6.39-rc2-/drivers/media/video/usbvision//usbvision-video.c	2011-07-16 16:42:35.000000000 +0200
+++ linux-2.6.39-rc2/drivers/media/video/usbvision/usbvision-video.c	2011-07-16 16:36:43.000000000 +0200
@@ -924,6 +924,11 @@  static int vidioc_try_fmt_vid_cap(struct
 	RESTRICT_TO_RANGE(vf->fmt.pix.width, MIN_FRAME_WIDTH, MAX_FRAME_WIDTH);
 	RESTRICT_TO_RANGE(vf->fmt.pix.height, MIN_FRAME_HEIGHT, MAX_FRAME_HEIGHT);
 
+	if (usbvision_device_data[usbvision->dev_model].codec == CODEC_WEBCAM) {
+		vf->fmt.pix.width = MAX_FRAME_WIDTH;
+		vf->fmt.pix.height = MAX_FRAME_HEIGHT;
+	}
+
 	vf->fmt.pix.bytesperline = vf->fmt.pix.width*
 		usbvision->palette.bytes_per_pixel;
 	vf->fmt.pix.sizeimage = vf->fmt.pix.bytesperline*vf->fmt.pix.height;
@@ -952,6 +957,11 @@  static int vidioc_s_fmt_vid_cap(struct f
 
 	usbvision->cur_frame = NULL;
 
+	if (usbvision_device_data[usbvision->dev_model].codec == CODEC_WEBCAM) {
+		vf->fmt.pix.width = MAX_FRAME_WIDTH;
+		vf->fmt.pix.height = MAX_FRAME_HEIGHT;
+	}
+
 	/* by now we are committed to the new data... */
 	usbvision_set_output(usbvision, vf->fmt.pix.width, vf->fmt.pix.height);