[RFC PATCH] em28xx: fix bytesperline calculation in TRY_FMT

[RFC PATCH] em28xx: fix bytesperline calculation in TRY_FMT

[Hans Verkuil]

This was part of my original em28xx patch series. That particular patch
combined two things: this fix and the change where TRY_FMT would no
longer return -EINVAL for unsupported pixelformats. The latter change was
rejected (correctly), but we all forgot about the second part of the patch
which fixed a real bug. I'm reposting just that fix.

Regards,

	Hans

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

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/usb/em28xx/em28xx-video.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
index 2eabf2a..070506d 100644
--- a/drivers/media/usb/em28xx/em28xx-video.c
+++ b/drivers/media/usb/em28xx/em28xx-video.c
@@ -906,7 +906,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)
-- 
1.7.10.4

Re: [RFC PATCH] em28xx: fix bytesperline calculation in TRY_FMT

[Devin Heitmueller]

On Tue, Jan 29, 2013 at 4:49 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> This was part of my original em28xx patch series. That particular patch
> combined two things: this fix and the change where TRY_FMT would no
> longer return -EINVAL for unsupported pixelformats. The latter change was
> rejected (correctly), but we all forgot about the second part of the patch
> which fixed a real bug. I'm reposting just that fix.
>
> Regards,
>
>         Hans
>
> The bytesperline calculation was incorrect: it used the old width instead
> of the provided width, and it miscalculated the bytesperline value for the
> depth == 12 case.
>
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/media/usb/em28xx/em28xx-video.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
> index 2eabf2a..070506d 100644
> --- a/drivers/media/usb/em28xx/em28xx-video.c
> +++ b/drivers/media/usb/em28xx/em28xx-video.c
> @@ -906,7 +906,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)

Reviewed-by: Devin Heitmueller <dheitmueller@kernellabs.com>

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [RFC PATCH] em28xx: fix bytesperline calculation in TRY_FMT

[Frank Schäfer]

Am 29.01.2013 10:49, schrieb Hans Verkuil:
> This was part of my original em28xx patch series. That particular patch
> combined two things: this fix and the change where TRY_FMT would no
> longer return -EINVAL for unsupported pixelformats. The latter change was
> rejected (correctly), but we all forgot about the second part of the patch
> which fixed a real bug. I'm reposting just that fix.
>
> Regards,
>
> 	Hans
>
> The bytesperline calculation was incorrect: it used the old width instead
> of the provided width, and it miscalculated the bytesperline value for the
> depth == 12 case.
>
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/media/usb/em28xx/em28xx-video.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
> index 2eabf2a..070506d 100644
> --- a/drivers/media/usb/em28xx/em28xx-video.c
> +++ b/drivers/media/usb/em28xx/em28xx-video.c
> @@ -906,7 +906,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)

Hmm... how are 12 bit pixels stored ? Are padding bits used so that 2
bytes per pixel are needed ?
I wonder if V4L2_PIX_FMT_YUV411P has ever been tested (libv4lconvert
doesn't support it)...

While we are at it, we should check and fix the other size calculations,
too.
For example, in em28xx-video.c we have in

vidioc_g_fmt_vid_cap():
f->fmt.pix.bytesperline = (dev->width * dev->format->depth + 7) >> 3;

queue_setup():
size = (dev->width * dev->height * dev->format->depth + 7) >> 3;

buffer_prepare():
size = (dev->width * dev->height * dev->format->depth + 7) >> 3;

em28xx_copy_video():
int bytesperline = dev->width << 1;

and there are probably more places...


Regards,
Frank

Re: [RFC PATCH] em28xx: fix bytesperline calculation in TRY_FMT

[Hans Verkuil]

On Tue January 29 2013 18:51:46 Frank Schäfer wrote:
> Am 29.01.2013 10:49, schrieb Hans Verkuil:
> > This was part of my original em28xx patch series. That particular patch
> > combined two things: this fix and the change where TRY_FMT would no
> > longer return -EINVAL for unsupported pixelformats. The latter change was
> > rejected (correctly), but we all forgot about the second part of the patch
> > which fixed a real bug. I'm reposting just that fix.
> >
> > Regards,
> >
> > 	Hans
> >
> > The bytesperline calculation was incorrect: it used the old width instead
> > of the provided width, and it miscalculated the bytesperline value for the
> > depth == 12 case.
> >
> > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> > ---
> >  drivers/media/usb/em28xx/em28xx-video.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
> > index 2eabf2a..070506d 100644
> > --- a/drivers/media/usb/em28xx/em28xx-video.c
> > +++ b/drivers/media/usb/em28xx/em28xx-video.c
> > @@ -906,7 +906,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)
> 
> Hmm... how are 12 bit pixels stored ? Are padding bits used so that 2
> bytes per pixel are needed ?

It's a planar format where the luma plane is twice as big as the two chroma
planes combined. So that gives an effective 'depth' of 12 bits per pixel.
The bytesperline value should be that of the largest plane.

I now realize that that is still wrong in the calculation above. It should
be this instead:

	f->fmt.pix.bytesperline = width * (fmt->depth >> 3);
  	f->fmt.pix.sizeimage = (width * height * fmt->depth) >> 3;

> I wonder if V4L2_PIX_FMT_YUV411P has ever been tested (libv4lconvert
> doesn't support it)...
> 
> While we are at it, we should check and fix the other size calculations,
> too.
> For example, in em28xx-video.c we have in
> 
> vidioc_g_fmt_vid_cap():
> f->fmt.pix.bytesperline = (dev->width * dev->format->depth + 7) >> 3;
> 
> queue_setup():
> size = (dev->width * dev->height * dev->format->depth + 7) >> 3;
> 
> buffer_prepare():
> size = (dev->width * dev->height * dev->format->depth + 7) >> 3;
> 
> em28xx_copy_video():
> int bytesperline = dev->width << 1;

Hmm, I'll have to prepare a RFCv2.

Regards,

	Hans

> 
> and there are probably more places...
> 
> 
> Regards,
> Frank
> 
> 

Re: [RFC PATCH] em28xx: fix bytesperline calculation in TRY_FMT

[Mauro Carvalho Chehab]

Em Tue, 29 Jan 2013 19:21:50 +0100
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> On Tue January 29 2013 18:51:46 Frank Schäfer wrote:
> > Am 29.01.2013 10:49, schrieb Hans Verkuil:
> > > This was part of my original em28xx patch series. That particular patch
> > > combined two things: this fix and the change where TRY_FMT would no
> > > longer return -EINVAL for unsupported pixelformats. The latter change was
> > > rejected (correctly), but we all forgot about the second part of the patch
> > > which fixed a real bug. I'm reposting just that fix.
> > >
> > > Regards,
> > >
> > > 	Hans
> > >
> > > The bytesperline calculation was incorrect: it used the old width instead
> > > of the provided width, and it miscalculated the bytesperline value for the
> > > depth == 12 case.
> > >
> > > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> > > ---
> > >  drivers/media/usb/em28xx/em28xx-video.c |    2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
> > > index 2eabf2a..070506d 100644
> > > --- a/drivers/media/usb/em28xx/em28xx-video.c
> > > +++ b/drivers/media/usb/em28xx/em28xx-video.c
> > > @@ -906,7 +906,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)
> > 
> > Hmm... how are 12 bit pixels stored ? Are padding bits used so that 2
> > bytes per pixel are needed ?
> 
> It's a planar format where the luma plane is twice as big as the two chroma
> planes combined. So that gives an effective 'depth' of 12 bits per pixel.
> The bytesperline value should be that of the largest plane.
> 
> I now realize that that is still wrong in the calculation above. It should
> be this instead:
> 
> 	f->fmt.pix.bytesperline = width * (fmt->depth >> 3);
>   	f->fmt.pix.sizeimage = (width * height * fmt->depth) >> 3;

The original code adds a "+ 7" to be sure that it will truncate upper
before dividing by 8.

> 
> > I wonder if V4L2_PIX_FMT_YUV411P has ever been tested (libv4lconvert
> > doesn't support it)...
> > 
> > While we are at it, we should check and fix the other size calculations,
> > too.
> > For example, in em28xx-video.c we have in
> > 
> > vidioc_g_fmt_vid_cap():
> > f->fmt.pix.bytesperline = (dev->width * dev->format->depth + 7) >> 3;
> > 
> > queue_setup():
> > size = (dev->width * dev->height * dev->format->depth + 7) >> 3;
> > 
> > buffer_prepare():
> > size = (dev->width * dev->height * dev->format->depth + 7) >> 3;
> > 
> > em28xx_copy_video():
> > int bytesperline = dev->width << 1;
> 
> Hmm, I'll have to prepare a RFCv2.
> 
> Regards,
> 
> 	Hans
> 
> > 
> > and there are probably more places...
> > 
> > 
> > Regards,
> > Frank
> > 
> > 
> --
> 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


-- 

Cheers,
Mauro