[10/15] em28xx: fix broken TRY_FMT.

Message ID 1357333186-8466-11-git-send-email-dheitmueller@kernellabs.com (mailing list archive)
State Rejected, archived
Headers

Commit Message

Devin Heitmueller Jan. 4, 2013, 8:59 p.m. UTC
  TRY_FMT should not return an error if a pixelformat is unsupported. Instead just
pick a common pixelformat.

Also the bytesperline calculation was incorrect: it used the old width instead of
the provided with, and it miscalculated the bytesperline value for the depth == 12
case.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Signed-off-by: Devin Heitmueller <dheitmueller@kernellabs.com>
---
 drivers/media/usb/em28xx/em28xx-video.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
  

Comments

Mauro Carvalho Chehab Jan. 5, 2013, 2:54 a.m. UTC | #1
Hans/Devin,

Em Fri,  4 Jan 2013 15:59:40 -0500
Devin Heitmueller <dheitmueller@kernellabs.com> escreveu:

> TRY_FMT should not return an error if a pixelformat is unsupported. Instead just
> pick a common pixelformat.
> 
> Also the bytesperline calculation was incorrect: it used the old width instead of
> the provided with, and it miscalculated the bytesperline value for the depth == 12
> case.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> Signed-off-by: Devin Heitmueller <dheitmueller@kernellabs.com>
> ---
>  drivers/media/usb/em28xx/em28xx-video.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
> index a91a248..7c09b55 100644
> --- a/drivers/media/usb/em28xx/em28xx-video.c
> +++ b/drivers/media/usb/em28xx/em28xx-video.c
> @@ -821,7 +821,7 @@ static int vidioc_try_fmt_vid_cap(struct file *file, void *priv,
>  	if (!fmt) {
>  		em28xx_videodbg("Fourcc format (%08x) invalid.\n",
>  				f->fmt.pix.pixelformat);
> -		return -EINVAL;
> +		fmt = format_by_fourcc(V4L2_PIX_FMT_YUYV);

This change has the potential of causing userspace regressions, so,
for now, I won't apply such change.

We need to discuss it better, before risk breaking things, and likely fix
applications first.

Regards,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  
Hans Verkuil Jan. 5, 2013, 1:34 p.m. UTC | #2
On Sat January 5 2013 03:54:44 Mauro Carvalho Chehab wrote:
> Hans/Devin,
> 
> Em Fri,  4 Jan 2013 15:59:40 -0500
> Devin Heitmueller <dheitmueller@kernellabs.com> escreveu:
> 
> > TRY_FMT should not return an error if a pixelformat is unsupported. Instead just
> > pick a common pixelformat.
> > 
> > Also the bytesperline calculation was incorrect: it used the old width instead of
> > the provided with, and it miscalculated the bytesperline value for the depth == 12
> > case.
> > 
> > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> > Signed-off-by: Devin Heitmueller <dheitmueller@kernellabs.com>
> > ---
> >  drivers/media/usb/em28xx/em28xx-video.c |    4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
> > index a91a248..7c09b55 100644
> > --- a/drivers/media/usb/em28xx/em28xx-video.c
> > +++ b/drivers/media/usb/em28xx/em28xx-video.c
> > @@ -821,7 +821,7 @@ static int vidioc_try_fmt_vid_cap(struct file *file, void *priv,
> >  	if (!fmt) {
> >  		em28xx_videodbg("Fourcc format (%08x) invalid.\n",
> >  				f->fmt.pix.pixelformat);
> > -		return -EINVAL;
> > +		fmt = format_by_fourcc(V4L2_PIX_FMT_YUYV);
> 
> This change has the potential of causing userspace regressions, so,
> for now, I won't apply such change.

Good!

> We need to discuss it better, before risk breaking things, and likely fix
> applications first.

Absolutely. I also want to change this test in v4l2-compliance from 'failure'
to 'warning' for the time being.

Regards,

	Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  
Mauro Carvalho Chehab Jan. 5, 2013, 3:36 p.m. UTC | #3
Em Sat, 5 Jan 2013 14:34:04 +0100
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> On Sat January 5 2013 03:54:44 Mauro Carvalho Chehab wrote:
> > Hans/Devin,
> > 
> > Em Fri,  4 Jan 2013 15:59:40 -0500
> > Devin Heitmueller <dheitmueller@kernellabs.com> escreveu:
> > 
> > > TRY_FMT should not return an error if a pixelformat is unsupported. Instead just
> > > pick a common pixelformat.
> > > 
> > > Also the bytesperline calculation was incorrect: it used the old width instead of
> > > the provided with, and it miscalculated the bytesperline value for the depth == 12
> > > case.
> > > 
> > > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> > > Signed-off-by: Devin Heitmueller <dheitmueller@kernellabs.com>
> > > ---
> > >  drivers/media/usb/em28xx/em28xx-video.c |    4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
> > > index a91a248..7c09b55 100644
> > > --- a/drivers/media/usb/em28xx/em28xx-video.c
> > > +++ b/drivers/media/usb/em28xx/em28xx-video.c
> > > @@ -821,7 +821,7 @@ static int vidioc_try_fmt_vid_cap(struct file *file, void *priv,
> > >  	if (!fmt) {
> > >  		em28xx_videodbg("Fourcc format (%08x) invalid.\n",
> > >  				f->fmt.pix.pixelformat);
> > > -		return -EINVAL;
> > > +		fmt = format_by_fourcc(V4L2_PIX_FMT_YUYV);
> > 
> > This change has the potential of causing userspace regressions, so,
> > for now, I won't apply such change.
> 
> Good!
> 
> > We need to discuss it better, before risk breaking things, and likely fix
> > applications first.
> 
> Absolutely. I also want to change this test in v4l2-compliance from 'failure'
> to 'warning' for the time being.

Sounds reasonable for me.

Regards,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

Patch

diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
index a91a248..7c09b55 100644
--- a/drivers/media/usb/em28xx/em28xx-video.c
+++ b/drivers/media/usb/em28xx/em28xx-video.c
@@ -821,7 +821,7 @@  static int vidioc_try_fmt_vid_cap(struct file *file, void *priv,
 	if (!fmt) {
 		em28xx_videodbg("Fourcc format (%08x) invalid.\n",
 				f->fmt.pix.pixelformat);
-		return -EINVAL;
+		fmt = format_by_fourcc(V4L2_PIX_FMT_YUYV);
 	}
 
 	if (dev->board.is_em2800) {
@@ -847,7 +847,7 @@  static int vidioc_try_fmt_vid_cap(struct file *file, void *priv,
 	f->fmt.pix.width = width;
 	f->fmt.pix.height = height;
 	f->fmt.pix.pixelformat = fmt->fourcc;
-	f->fmt.pix.bytesperline = (dev->width * fmt->depth + 7) >> 3;
+	f->fmt.pix.bytesperline = width * ((fmt->depth + 7) >> 3);
 	f->fmt.pix.sizeimage = f->fmt.pix.bytesperline * height;
 	f->fmt.pix.colorspace = V4L2_COLORSPACE_SMPTE170M;
 	if (dev->progressive)