[RFCv2 PATCH] em28xx: fix bytesperline calculation in G/TRY_FMT

[RFCv2 PATCH] em28xx: fix bytesperline calculation in G/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.

Changes since v1:

- v1 still miscalculated the bytesperline and imagesize values (they were
  too large).
- G_FMT had the same calculation bug.

Tested with my em28xx.

Regards,

        Hans

The bytesperline calculation was incorrect: it used the old width instead of
the provided width in the case of TRY_FMT, and it miscalculated the bytesperline
value for the depth == 12 (planar YUV 4:1:1) case. For planar formats the
bytesperline value should be the bytesperline of the widest plane, which is
the Y plane which has 8 bits per pixel, not 12.

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

diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
index 2eabf2a..6ced426 100644
--- a/drivers/media/usb/em28xx/em28xx-video.c
+++ b/drivers/media/usb/em28xx/em28xx-video.c
@@ -837,8 +837,8 @@ static int vidioc_g_fmt_vid_cap(struct file *file, void *priv,
 	f->fmt.pix.width = dev->width;
 	f->fmt.pix.height = dev->height;
 	f->fmt.pix.pixelformat = dev->format->fourcc;
-	f->fmt.pix.bytesperline = (dev->width * dev->format->depth + 7) >> 3;
-	f->fmt.pix.sizeimage = f->fmt.pix.bytesperline  * dev->height;
+	f->fmt.pix.bytesperline = dev->width * (dev->format->depth >> 3);
+	f->fmt.pix.sizeimage = (dev->width * dev->height * dev->format->depth + 7) >> 3;
 	f->fmt.pix.colorspace = V4L2_COLORSPACE_SMPTE170M;
 
 	/* FIXME: TOP? NONE? BOTTOM? ALTENATE? */
@@ -906,8 +906,8 @@ 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.sizeimage = f->fmt.pix.bytesperline * height;
+	f->fmt.pix.bytesperline = width * (fmt->depth >> 3);
+	f->fmt.pix.sizeimage = (width * height * fmt->depth + 7) >> 3;
 	f->fmt.pix.colorspace = V4L2_COLORSPACE_SMPTE170M;
 	if (dev->progressive)
 		f->fmt.pix.field = V4L2_FIELD_NONE;
-- 
1.7.10.4

Re: [RFCv2 PATCH] em28xx: fix bytesperline calculation in G/TRY_FMT

[Mauro Carvalho Chehab]

Em Wed, 30 Jan 2013 09:01:22 +0100
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> 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.
> 
> Changes since v1:
> 
> - v1 still miscalculated the bytesperline and imagesize values (they were
>   too large).
> - G_FMT had the same calculation bug.
> 
> Tested with my em28xx.
> 
> Regards,
> 
>         Hans
> 
> The bytesperline calculation was incorrect: it used the old width instead of
> the provided width in the case of TRY_FMT, and it miscalculated the bytesperline
> value for the depth == 12 (planar YUV 4:1:1) case. For planar formats the
> bytesperline value should be the bytesperline of the widest plane, which is
> the Y plane which has 8 bits per pixel, not 12.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/media/usb/em28xx/em28xx-video.c |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
> index 2eabf2a..6ced426 100644
> --- a/drivers/media/usb/em28xx/em28xx-video.c
> +++ b/drivers/media/usb/em28xx/em28xx-video.c
> @@ -837,8 +837,8 @@ static int vidioc_g_fmt_vid_cap(struct file *file, void *priv,
>  	f->fmt.pix.width = dev->width;
>  	f->fmt.pix.height = dev->height;
>  	f->fmt.pix.pixelformat = dev->format->fourcc;
> -	f->fmt.pix.bytesperline = (dev->width * dev->format->depth + 7) >> 3;
> -	f->fmt.pix.sizeimage = f->fmt.pix.bytesperline  * dev->height;
> +	f->fmt.pix.bytesperline = dev->width * (dev->format->depth >> 3);

Why did you remove the round up here?

> +	f->fmt.pix.sizeimage = (dev->width * dev->height * dev->format->depth + 7) >> 3;
>  	f->fmt.pix.colorspace = V4L2_COLORSPACE_SMPTE170M;
>  
>  	/* FIXME: TOP? NONE? BOTTOM? ALTENATE? */
> @@ -906,8 +906,8 @@ 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.sizeimage = f->fmt.pix.bytesperline * height;
> +	f->fmt.pix.bytesperline = width * (fmt->depth >> 3);

Why did you remove the round up here?

> +	f->fmt.pix.sizeimage = (width * height * fmt->depth + 7) >> 3;
>  	f->fmt.pix.colorspace = V4L2_COLORSPACE_SMPTE170M;
>  	if (dev->progressive)
>  		f->fmt.pix.field = V4L2_FIELD_NONE;


Regards,
Mauro

Re: [RFCv2 PATCH] em28xx: fix bytesperline calculation in G/TRY_FMT

[Hans Verkuil]

On Wed 30 January 2013 10:40:30 Mauro Carvalho Chehab wrote:
> Em Wed, 30 Jan 2013 09:01:22 +0100
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> 
> > 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.
> > 
> > Changes since v1:
> > 
> > - v1 still miscalculated the bytesperline and imagesize values (they were
> >   too large).
> > - G_FMT had the same calculation bug.
> > 
> > Tested with my em28xx.
> > 
> > Regards,
> > 
> >         Hans
> > 
> > The bytesperline calculation was incorrect: it used the old width instead of
> > the provided width in the case of TRY_FMT, and it miscalculated the bytesperline
> > value for the depth == 12 (planar YUV 4:1:1) case. For planar formats the
> > bytesperline value should be the bytesperline of the widest plane, which is
> > the Y plane which has 8 bits per pixel, not 12.
> > 
> > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> > ---
> >  drivers/media/usb/em28xx/em28xx-video.c |    8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
> > index 2eabf2a..6ced426 100644
> > --- a/drivers/media/usb/em28xx/em28xx-video.c
> > +++ b/drivers/media/usb/em28xx/em28xx-video.c
> > @@ -837,8 +837,8 @@ static int vidioc_g_fmt_vid_cap(struct file *file, void *priv,
> >  	f->fmt.pix.width = dev->width;
> >  	f->fmt.pix.height = dev->height;
> >  	f->fmt.pix.pixelformat = dev->format->fourcc;
> > -	f->fmt.pix.bytesperline = (dev->width * dev->format->depth + 7) >> 3;
> > -	f->fmt.pix.sizeimage = f->fmt.pix.bytesperline  * dev->height;
> > +	f->fmt.pix.bytesperline = dev->width * (dev->format->depth >> 3);
> 
> Why did you remove the round up here?

Because that would give the wrong result. Depth can be 8, 12 or 16. The YUV 4:1:1
planar format is the one with depth 12. But for the purposes of the bytesperline
calculation only the depth of the largest plane counts, which is the luma plane
with a depth of 8. So for a width of 720 the value of bytesperline should be:

depth=8 -> bytesperline = 720
depth=12 -> bytesperline = 720
depth=16 -> bytesperline = 1440

By rounding the bytesperline for depth = 12 would become 1440, which is wrong.

> 
> > +	f->fmt.pix.sizeimage = (dev->width * dev->height * dev->format->depth + 7) >> 3;
> >  	f->fmt.pix.colorspace = V4L2_COLORSPACE_SMPTE170M;
> >  
> >  	/* FIXME: TOP? NONE? BOTTOM? ALTENATE? */
> > @@ -906,8 +906,8 @@ 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.sizeimage = f->fmt.pix.bytesperline * height;
> > +	f->fmt.pix.bytesperline = width * (fmt->depth >> 3);
> 
> Why did you remove the round up here?

See above.

Regards,

	Hans

> 
> > +	f->fmt.pix.sizeimage = (width * height * fmt->depth + 7) >> 3;
> >  	f->fmt.pix.colorspace = V4L2_COLORSPACE_SMPTE170M;
> >  	if (dev->progressive)
> >  		f->fmt.pix.field = V4L2_FIELD_NONE;
> 
> 
> Regards,
> Mauro
> 

Re: [RFCv2 PATCH] em28xx: fix bytesperline calculation in G/TRY_FMT

[Frank Schäfer]

Am 30.01.2013 10:49, schrieb Hans Verkuil:
> On Wed 30 January 2013 10:40:30 Mauro Carvalho Chehab wrote:
>> Em Wed, 30 Jan 2013 09:01:22 +0100
>> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
>>
>>> 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.
>>>
>>> Changes since v1:
>>>
>>> - v1 still miscalculated the bytesperline and imagesize values (they were
>>>   too large).
>>> - G_FMT had the same calculation bug.
>>>
>>> Tested with my em28xx.
>>>
>>> Regards,
>>>
>>>         Hans
>>>
>>> The bytesperline calculation was incorrect: it used the old width instead of
>>> the provided width in the case of TRY_FMT, and it miscalculated the bytesperline
>>> value for the depth == 12 (planar YUV 4:1:1) case. For planar formats the
>>> bytesperline value should be the bytesperline of the widest plane, which is
>>> the Y plane which has 8 bits per pixel, not 12.
>>>
>>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>>> ---
>>>  drivers/media/usb/em28xx/em28xx-video.c |    8 ++++----
>>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
>>> index 2eabf2a..6ced426 100644
>>> --- a/drivers/media/usb/em28xx/em28xx-video.c
>>> +++ b/drivers/media/usb/em28xx/em28xx-video.c
>>> @@ -837,8 +837,8 @@ static int vidioc_g_fmt_vid_cap(struct file *file, void *priv,
>>>  	f->fmt.pix.width = dev->width;
>>>  	f->fmt.pix.height = dev->height;
>>>  	f->fmt.pix.pixelformat = dev->format->fourcc;
>>> -	f->fmt.pix.bytesperline = (dev->width * dev->format->depth + 7) >> 3;
>>> -	f->fmt.pix.sizeimage = f->fmt.pix.bytesperline  * dev->height;
>>> +	f->fmt.pix.bytesperline = dev->width * (dev->format->depth >> 3);
>> Why did you remove the round up here?
> Because that would give the wrong result. Depth can be 8, 12 or 16. The YUV 4:1:1
> planar format is the one with depth 12. But for the purposes of the bytesperline
> calculation only the depth of the largest plane counts, which is the luma plane
> with a depth of 8. So for a width of 720 the value of bytesperline should be:
>
> depth=8 -> bytesperline = 720
> depth=12 -> bytesperline = 720
> depth=16 -> bytesperline = 1440
>
> By rounding the bytesperline for depth = 12 would become 1440, which is wrong.

Isn't the actual problem that the bytesperline value is calculated based
on the depth instead of size of the largest plane ?
While this formula gives us the right value for YUV411P (12bit), the
calculated value for YUV422P (same plane size but 16bit depth) is wrong.

I see two possibilities to solve this:
1) use switch/case(format_type) in vidioc_g_fmt_vid_cap() and
vidioc_try_fmt_vid_cap()
2) add plane size to format data struct em28xx_fmt (either as total or
relative value)

I prefer 2) to keep things together and to make adding new format easier.

Could you please also fix
static void em28xx_copy_video()

-        int bytesperline = dev->width << 1;
+        int bytesperline = (dev->width * dev->format->depth) >> 8

AFAICS, bytesperline here is supposed to describe the actual number of
bytes used per line (in opposition to g/try_fmt).

Regards,
Frank

>
>>> +	f->fmt.pix.sizeimage = (dev->width * dev->height * dev->format->depth + 7) >> 3;
>>>  	f->fmt.pix.colorspace = V4L2_COLORSPACE_SMPTE170M;
>>>  
>>>  	/* FIXME: TOP? NONE? BOTTOM? ALTENATE? */
>>> @@ -906,8 +906,8 @@ 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.sizeimage = f->fmt.pix.bytesperline * height;
>>> +	f->fmt.pix.bytesperline = width * (fmt->depth >> 3);
>> Why did you remove the round up here?
> See above.
>
> Regards,
>
> 	Hans
>
>>> +	f->fmt.pix.sizeimage = (width * height * fmt->depth + 7) >> 3;
>>>  	f->fmt.pix.colorspace = V4L2_COLORSPACE_SMPTE170M;
>>>  	if (dev->progressive)
>>>  		f->fmt.pix.field = V4L2_FIELD_NONE;
>>
>> Regards,
>> Mauro
>>

Re: [RFCv2 PATCH] em28xx: fix bytesperline calculation in G/TRY_FMT

[Hans Verkuil]

On Wed January 30 2013 17:32:38 Frank Schäfer wrote:
> Am 30.01.2013 10:49, schrieb Hans Verkuil:
> > On Wed 30 January 2013 10:40:30 Mauro Carvalho Chehab wrote:
> >> Em Wed, 30 Jan 2013 09:01:22 +0100
> >> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> >>
> >>> 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.
> >>>
> >>> Changes since v1:
> >>>
> >>> - v1 still miscalculated the bytesperline and imagesize values (they were
> >>>   too large).
> >>> - G_FMT had the same calculation bug.
> >>>
> >>> Tested with my em28xx.
> >>>
> >>> Regards,
> >>>
> >>>         Hans
> >>>
> >>> The bytesperline calculation was incorrect: it used the old width instead of
> >>> the provided width in the case of TRY_FMT, and it miscalculated the bytesperline
> >>> value for the depth == 12 (planar YUV 4:1:1) case. For planar formats the
> >>> bytesperline value should be the bytesperline of the widest plane, which is
> >>> the Y plane which has 8 bits per pixel, not 12.
> >>>
> >>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> >>> ---
> >>>  drivers/media/usb/em28xx/em28xx-video.c |    8 ++++----
> >>>  1 file changed, 4 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
> >>> index 2eabf2a..6ced426 100644
> >>> --- a/drivers/media/usb/em28xx/em28xx-video.c
> >>> +++ b/drivers/media/usb/em28xx/em28xx-video.c
> >>> @@ -837,8 +837,8 @@ static int vidioc_g_fmt_vid_cap(struct file *file, void *priv,
> >>>  	f->fmt.pix.width = dev->width;
> >>>  	f->fmt.pix.height = dev->height;
> >>>  	f->fmt.pix.pixelformat = dev->format->fourcc;
> >>> -	f->fmt.pix.bytesperline = (dev->width * dev->format->depth + 7) >> 3;
> >>> -	f->fmt.pix.sizeimage = f->fmt.pix.bytesperline  * dev->height;
> >>> +	f->fmt.pix.bytesperline = dev->width * (dev->format->depth >> 3);
> >> Why did you remove the round up here?
> > Because that would give the wrong result. Depth can be 8, 12 or 16. The YUV 4:1:1
> > planar format is the one with depth 12. But for the purposes of the bytesperline
> > calculation only the depth of the largest plane counts, which is the luma plane
> > with a depth of 8. So for a width of 720 the value of bytesperline should be:
> >
> > depth=8 -> bytesperline = 720
> > depth=12 -> bytesperline = 720
> > depth=16 -> bytesperline = 1440
> >
> > By rounding the bytesperline for depth = 12 would become 1440, which is wrong.
> 
> Isn't the actual problem that the bytesperline value is calculated based
> on the depth instead of size of the largest plane ?

Yes.

> While this formula gives us the right value for YUV411P (12bit), the
> calculated value for YUV422P (same plane size but 16bit depth) is wrong.

But em28xx doesn't support YUV422P :-)

Basically I just want to fix the bug without rewriting the whole thing...

> 
> I see two possibilities to solve this:
> 1) use switch/case(format_type) in vidioc_g_fmt_vid_cap() and
> vidioc_try_fmt_vid_cap()
> 2) add plane size to format data struct em28xx_fmt (either as total or
> relative value)
> 
> I prefer 2) to keep things together and to make adding new format easier.
> 
> Could you please also fix
> static void em28xx_copy_video()
> 
> -        int bytesperline = dev->width << 1;
> +        int bytesperline = (dev->width * dev->format->depth) >> 8

I'm hesitant to do this since I don't understand that code well enough.
It seems bytesperline is set to the worst case bytesperline, and that seems
to work.

To be honest, I don't have the time to work on this much more. If you want
to take this further, then feel free.

Regards,

	Hans

> AFAICS, bytesperline here is supposed to describe the actual number of
> bytes used per line (in opposition to g/try_fmt).
> 
> Regards,
> Frank
> 
> >
> >>> +	f->fmt.pix.sizeimage = (dev->width * dev->height * dev->format->depth + 7) >> 3;
> >>>  	f->fmt.pix.colorspace = V4L2_COLORSPACE_SMPTE170M;
> >>>  
> >>>  	/* FIXME: TOP? NONE? BOTTOM? ALTENATE? */
> >>> @@ -906,8 +906,8 @@ 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.sizeimage = f->fmt.pix.bytesperline * height;
> >>> +	f->fmt.pix.bytesperline = width * (fmt->depth >> 3);
> >> Why did you remove the round up here?
> > See above.
> >
> > Regards,
> >
> > 	Hans
> >
> >>> +	f->fmt.pix.sizeimage = (width * height * fmt->depth + 7) >> 3;
> >>>  	f->fmt.pix.colorspace = V4L2_COLORSPACE_SMPTE170M;
> >>>  	if (dev->progressive)
> >>>  		f->fmt.pix.field = V4L2_FIELD_NONE;
> >>
> >> Regards,
> >> Mauro
> >>
> 

Re: [RFCv2 PATCH] em28xx: fix bytesperline calculation in G/TRY_FMT

[Mauro Carvalho Chehab]

Em Wed, 30 Jan 2013 10:49:25 +0100
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> On Wed 30 January 2013 10:40:30 Mauro Carvalho Chehab wrote:
> > Em Wed, 30 Jan 2013 09:01:22 +0100
> > Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> > 
> > > 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.
> > > 
> > > Changes since v1:
> > > 
> > > - v1 still miscalculated the bytesperline and imagesize values (they were
> > >   too large).
> > > - G_FMT had the same calculation bug.
> > > 
> > > Tested with my em28xx.
> > > 
> > > Regards,
> > > 
> > >         Hans
> > > 
> > > The bytesperline calculation was incorrect: it used the old width instead of
> > > the provided width in the case of TRY_FMT, and it miscalculated the bytesperline
> > > value for the depth == 12 (planar YUV 4:1:1) case. For planar formats the
> > > bytesperline value should be the bytesperline of the widest plane, which is
> > > the Y plane which has 8 bits per pixel, not 12.
> > > 
> > > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> > > ---
> > >  drivers/media/usb/em28xx/em28xx-video.c |    8 ++++----
> > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
> > > index 2eabf2a..6ced426 100644
> > > --- a/drivers/media/usb/em28xx/em28xx-video.c
> > > +++ b/drivers/media/usb/em28xx/em28xx-video.c
> > > @@ -837,8 +837,8 @@ static int vidioc_g_fmt_vid_cap(struct file *file, void *priv,
> > >  	f->fmt.pix.width = dev->width;
> > >  	f->fmt.pix.height = dev->height;
> > >  	f->fmt.pix.pixelformat = dev->format->fourcc;
> > > -	f->fmt.pix.bytesperline = (dev->width * dev->format->depth + 7) >> 3;
> > > -	f->fmt.pix.sizeimage = f->fmt.pix.bytesperline  * dev->height;
> > > +	f->fmt.pix.bytesperline = dev->width * (dev->format->depth >> 3);
> > 
> > Why did you remove the round up here?
> 
> Because that would give the wrong result. Depth can be 8, 12 or 16. The YUV 4:1:1
> planar format is the one with depth 12. But for the purposes of the bytesperline
> calculation only the depth of the largest plane counts, which is the luma plane
> with a depth of 8. So for a width of 720 the value of bytesperline should be:
> 
> depth=8 -> bytesperline = 720
> depth=12 -> bytesperline = 720

With depth=12, it should be, instead, 1080, as 2 pixels need 3 bytes.

> depth=16 -> bytesperline = 1440

Well,

depth=8 -> bytesperline =  (720 * 8) + 7) / 8 = 720
depth=12 -> bytesperline = (720 * 12) + 7) / 8 = 1080
depth=16 -> bytesperline = (720 * 16) + 7) / 8 = 1440

So, this sounds perfectly OK on my eyes:
	f->fmt.pix.bytesperline = (dev->width * dev->format->depth + 7) >> 3;

Regards,
Mauro

Re: [RFCv2 PATCH] em28xx: fix bytesperline calculation in G/TRY_FMT

[Hans Verkuil]

On Wed January 30 2013 20:07:29 Mauro Carvalho Chehab wrote:
> Em Wed, 30 Jan 2013 10:49:25 +0100
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> 
> > On Wed 30 January 2013 10:40:30 Mauro Carvalho Chehab wrote:
> > > Em Wed, 30 Jan 2013 09:01:22 +0100
> > > Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> > > 
> > > > 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.
> > > > 
> > > > Changes since v1:
> > > > 
> > > > - v1 still miscalculated the bytesperline and imagesize values (they were
> > > >   too large).
> > > > - G_FMT had the same calculation bug.
> > > > 
> > > > Tested with my em28xx.
> > > > 
> > > > Regards,
> > > > 
> > > >         Hans
> > > > 
> > > > The bytesperline calculation was incorrect: it used the old width instead of
> > > > the provided width in the case of TRY_FMT, and it miscalculated the bytesperline
> > > > value for the depth == 12 (planar YUV 4:1:1) case. For planar formats the
> > > > bytesperline value should be the bytesperline of the widest plane, which is
> > > > the Y plane which has 8 bits per pixel, not 12.
> > > > 
> > > > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> > > > ---
> > > >  drivers/media/usb/em28xx/em28xx-video.c |    8 ++++----
> > > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
> > > > index 2eabf2a..6ced426 100644
> > > > --- a/drivers/media/usb/em28xx/em28xx-video.c
> > > > +++ b/drivers/media/usb/em28xx/em28xx-video.c
> > > > @@ -837,8 +837,8 @@ static int vidioc_g_fmt_vid_cap(struct file *file, void *priv,
> > > >  	f->fmt.pix.width = dev->width;
> > > >  	f->fmt.pix.height = dev->height;
> > > >  	f->fmt.pix.pixelformat = dev->format->fourcc;
> > > > -	f->fmt.pix.bytesperline = (dev->width * dev->format->depth + 7) >> 3;
> > > > -	f->fmt.pix.sizeimage = f->fmt.pix.bytesperline  * dev->height;
> > > > +	f->fmt.pix.bytesperline = dev->width * (dev->format->depth >> 3);
> > > 
> > > Why did you remove the round up here?
> > 
> > Because that would give the wrong result. Depth can be 8, 12 or 16. The YUV 4:1:1
> > planar format is the one with depth 12. But for the purposes of the bytesperline
> > calculation only the depth of the largest plane counts, which is the luma plane
> > with a depth of 8. So for a width of 720 the value of bytesperline should be:
> > 
> > depth=8 -> bytesperline = 720
> > depth=12 -> bytesperline = 720
> 
> With depth=12, it should be, instead, 1080, as 2 pixels need 3 bytes.

No, it's not. It's a *planar* format: first the Y plane, then the two smaller
chroma planes. The spec says that bytesperline for planar formats refers to
the largest plane.

For this format the luma plane is one byte per pixel. Each of the two chroma
planes have effectively two bits per pixel (actually one byte per four pixels),
so you end up with 8+2+2=12 bits per pixel.

Hence bytesperline should be 720 for this particular format.

Regards,

	Hans

> 
> > depth=16 -> bytesperline = 1440
> 
> Well,
> 
> depth=8 -> bytesperline =  (720 * 8) + 7) / 8 = 720
> depth=12 -> bytesperline = (720 * 12) + 7) / 8 = 1080
> depth=16 -> bytesperline = (720 * 16) + 7) / 8 = 1440
> 
> So, this sounds perfectly OK on my eyes:
> 	f->fmt.pix.bytesperline = (dev->width * dev->format->depth + 7) >> 3;
> 
> Regards,
> Mauro
> 

Re: [RFCv2 PATCH] em28xx: fix bytesperline calculation in G/TRY_FMT

[Mauro Carvalho Chehab]

Em Thu, 31 Jan 2013 08:16:39 +0100
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> On Wed January 30 2013 20:07:29 Mauro Carvalho Chehab wrote:
> > Em Wed, 30 Jan 2013 10:49:25 +0100
> > Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> > 
> > > On Wed 30 January 2013 10:40:30 Mauro Carvalho Chehab wrote:
> > > > Em Wed, 30 Jan 2013 09:01:22 +0100
> > > > Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> > > > 
> > > > > 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.
> > > > > 
> > > > > Changes since v1:
> > > > > 
> > > > > - v1 still miscalculated the bytesperline and imagesize values (they were
> > > > >   too large).
> > > > > - G_FMT had the same calculation bug.
> > > > > 
> > > > > Tested with my em28xx.
> > > > > 
> > > > > Regards,
> > > > > 
> > > > >         Hans
> > > > > 
> > > > > The bytesperline calculation was incorrect: it used the old width instead of
> > > > > the provided width in the case of TRY_FMT, and it miscalculated the bytesperline
> > > > > value for the depth == 12 (planar YUV 4:1:1) case. For planar formats the
> > > > > bytesperline value should be the bytesperline of the widest plane, which is
> > > > > the Y plane which has 8 bits per pixel, not 12.
> > > > > 
> > > > > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> > > > > ---
> > > > >  drivers/media/usb/em28xx/em28xx-video.c |    8 ++++----
> > > > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
> > > > > index 2eabf2a..6ced426 100644
> > > > > --- a/drivers/media/usb/em28xx/em28xx-video.c
> > > > > +++ b/drivers/media/usb/em28xx/em28xx-video.c
> > > > > @@ -837,8 +837,8 @@ static int vidioc_g_fmt_vid_cap(struct file *file, void *priv,
> > > > >  	f->fmt.pix.width = dev->width;
> > > > >  	f->fmt.pix.height = dev->height;
> > > > >  	f->fmt.pix.pixelformat = dev->format->fourcc;
> > > > > -	f->fmt.pix.bytesperline = (dev->width * dev->format->depth + 7) >> 3;
> > > > > -	f->fmt.pix.sizeimage = f->fmt.pix.bytesperline  * dev->height;
> > > > > +	f->fmt.pix.bytesperline = dev->width * (dev->format->depth >> 3);
> > > > 
> > > > Why did you remove the round up here?
> > > 
> > > Because that would give the wrong result. Depth can be 8, 12 or 16. The YUV 4:1:1
> > > planar format is the one with depth 12. But for the purposes of the bytesperline
> > > calculation only the depth of the largest plane counts, which is the luma plane
> > > with a depth of 8. So for a width of 720 the value of bytesperline should be:
> > > 
> > > depth=8 -> bytesperline = 720
> > > depth=12 -> bytesperline = 720
> > 
> > With depth=12, it should be, instead, 1080, as 2 pixels need 3 bytes.
> 
> No, it's not. It's a *planar* format: first the Y plane, then the two smaller
> chroma planes. The spec says that bytesperline for planar formats refers to
> the largest plane.
> 
> For this format the luma plane is one byte per pixel. Each of the two chroma
> planes have effectively two bits per pixel (actually one byte per four pixels),
> so you end up with 8+2+2=12 bits per pixel.
> 
> Hence bytesperline should be 720 for this particular format.

If I understood what you just said, you're talking that the only format marked
as depth=12 is actually depth=8, right? Then the fix would be to change depth
in the table, and not here.

Yet, I'm not sure if this is the proper fix.

The only used I saw on userspace apps for this field is to allocate size for
the memory buffer. Some userspace applications use to get bytesperline and
multiply by the image height and get the image size, instead of relying
on sizeimage, as some drivers didn't use to fill sizeimage properly.

By using bytesperline equal to 1080 in this case warrants that the buffers
on userspace will have enough space.

Regards,
Mauro

Re: [RFCv2 PATCH] em28xx: fix bytesperline calculation in G/TRY_FMT

[Hans Verkuil]

On Thu 31 January 2013 11:08:07 Mauro Carvalho Chehab wrote:
> Em Thu, 31 Jan 2013 08:16:39 +0100
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> 
> > On Wed January 30 2013 20:07:29 Mauro Carvalho Chehab wrote:
> > > Em Wed, 30 Jan 2013 10:49:25 +0100
> > > Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> > > 
> > > > On Wed 30 January 2013 10:40:30 Mauro Carvalho Chehab wrote:
> > > > > Em Wed, 30 Jan 2013 09:01:22 +0100
> > > > > Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> > > > > 
> > > > > > 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.
> > > > > > 
> > > > > > Changes since v1:
> > > > > > 
> > > > > > - v1 still miscalculated the bytesperline and imagesize values (they were
> > > > > >   too large).
> > > > > > - G_FMT had the same calculation bug.
> > > > > > 
> > > > > > Tested with my em28xx.
> > > > > > 
> > > > > > Regards,
> > > > > > 
> > > > > >         Hans
> > > > > > 
> > > > > > The bytesperline calculation was incorrect: it used the old width instead of
> > > > > > the provided width in the case of TRY_FMT, and it miscalculated the bytesperline
> > > > > > value for the depth == 12 (planar YUV 4:1:1) case. For planar formats the
> > > > > > bytesperline value should be the bytesperline of the widest plane, which is
> > > > > > the Y plane which has 8 bits per pixel, not 12.
> > > > > > 
> > > > > > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> > > > > > ---
> > > > > >  drivers/media/usb/em28xx/em28xx-video.c |    8 ++++----
> > > > > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
> > > > > > index 2eabf2a..6ced426 100644
> > > > > > --- a/drivers/media/usb/em28xx/em28xx-video.c
> > > > > > +++ b/drivers/media/usb/em28xx/em28xx-video.c
> > > > > > @@ -837,8 +837,8 @@ static int vidioc_g_fmt_vid_cap(struct file *file, void *priv,
> > > > > >  	f->fmt.pix.width = dev->width;
> > > > > >  	f->fmt.pix.height = dev->height;
> > > > > >  	f->fmt.pix.pixelformat = dev->format->fourcc;
> > > > > > -	f->fmt.pix.bytesperline = (dev->width * dev->format->depth + 7) >> 3;
> > > > > > -	f->fmt.pix.sizeimage = f->fmt.pix.bytesperline  * dev->height;
> > > > > > +	f->fmt.pix.bytesperline = dev->width * (dev->format->depth >> 3);
> > > > > 
> > > > > Why did you remove the round up here?
> > > > 
> > > > Because that would give the wrong result. Depth can be 8, 12 or 16. The YUV 4:1:1
> > > > planar format is the one with depth 12. But for the purposes of the bytesperline
> > > > calculation only the depth of the largest plane counts, which is the luma plane
> > > > with a depth of 8. So for a width of 720 the value of bytesperline should be:
> > > > 
> > > > depth=8 -> bytesperline = 720
> > > > depth=12 -> bytesperline = 720
> > > 
> > > With depth=12, it should be, instead, 1080, as 2 pixels need 3 bytes.
> > 
> > No, it's not. It's a *planar* format: first the Y plane, then the two smaller
> > chroma planes. The spec says that bytesperline for planar formats refers to
> > the largest plane.
> > 
> > For this format the luma plane is one byte per pixel. Each of the two chroma
> > planes have effectively two bits per pixel (actually one byte per four pixels),
> > so you end up with 8+2+2=12 bits per pixel.
> > 
> > Hence bytesperline should be 720 for this particular format.
> 
> If I understood what you just said, you're talking that the only format marked
> as depth=12 is actually depth=8, right? Then the fix would be to change depth
> in the table, and not here.
> 
> Yet, I'm not sure if this is the proper fix.
> 
> The only used I saw on userspace apps for this field is to allocate size for
> the memory buffer. Some userspace applications use to get bytesperline and
> multiply by the image height and get the image size, instead of relying
> on sizeimage, as some drivers didn't use to fill sizeimage properly.
> 
> By using bytesperline equal to 1080 in this case warrants that the buffers
> on userspace will have enough space.

I did some research into planar formats in our drivers and (of course) it's a big
mess.

I looked at drivers that support V4L2_PIX_FMT_YUV422P, which is fairly common:

s5p-fimc:	follows the spec
arv:		follows the spec
vpif_capture:	follows the spec
vpif_display:	follows the spec
pxa_camera:	no idea, I can't figure this out
s3c-camif:	follows the spec
exynos-gsc:	uses depth
s2255:		uses depth
usbvision:	uses depth
saa7146:	uses depth
saa7134:	uses depth
bttv:		follows the spec

I think we should follow the spec here. In practice, nobody uses planar formats
for consumer-type hardware as it is a very awkward format, and libv4l doesn't
support it either. Since there is no clear common practice in our drivers, I'd
say we stick to the spec.

Regards,

	Hans

Re: [RFCv2 PATCH] em28xx: fix bytesperline calculation in G/TRY_FMT

[Frank Schäfer]

Am 31.01.2013 11:08, schrieb Mauro Carvalho Chehab:
> Em Thu, 31 Jan 2013 08:16:39 +0100
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
>
>> On Wed January 30 2013 20:07:29 Mauro Carvalho Chehab wrote:
>>> Em Wed, 30 Jan 2013 10:49:25 +0100
>>> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
>>>
>>>> On Wed 30 January 2013 10:40:30 Mauro Carvalho Chehab wrote:
>>>>> Em Wed, 30 Jan 2013 09:01:22 +0100
>>>>> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
>>>>>
>>>>>> 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.
>>>>>>
>>>>>> Changes since v1:
>>>>>>
>>>>>> - v1 still miscalculated the bytesperline and imagesize values (they were
>>>>>>   too large).
>>>>>> - G_FMT had the same calculation bug.
>>>>>>
>>>>>> Tested with my em28xx.
>>>>>>
>>>>>> Regards,
>>>>>>
>>>>>>         Hans
>>>>>>
>>>>>> The bytesperline calculation was incorrect: it used the old width instead of
>>>>>> the provided width in the case of TRY_FMT, and it miscalculated the bytesperline
>>>>>> value for the depth == 12 (planar YUV 4:1:1) case. For planar formats the
>>>>>> bytesperline value should be the bytesperline of the widest plane, which is
>>>>>> the Y plane which has 8 bits per pixel, not 12.
>>>>>>
>>>>>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>>>>>> ---
>>>>>>  drivers/media/usb/em28xx/em28xx-video.c |    8 ++++----
>>>>>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
>>>>>> index 2eabf2a..6ced426 100644
>>>>>> --- a/drivers/media/usb/em28xx/em28xx-video.c
>>>>>> +++ b/drivers/media/usb/em28xx/em28xx-video.c
>>>>>> @@ -837,8 +837,8 @@ static int vidioc_g_fmt_vid_cap(struct file *file, void *priv,
>>>>>>  	f->fmt.pix.width = dev->width;
>>>>>>  	f->fmt.pix.height = dev->height;
>>>>>>  	f->fmt.pix.pixelformat = dev->format->fourcc;
>>>>>> -	f->fmt.pix.bytesperline = (dev->width * dev->format->depth + 7) >> 3;
>>>>>> -	f->fmt.pix.sizeimage = f->fmt.pix.bytesperline  * dev->height;
>>>>>> +	f->fmt.pix.bytesperline = dev->width * (dev->format->depth >> 3);
>>>>> Why did you remove the round up here?
>>>> Because that would give the wrong result. Depth can be 8, 12 or 16. The YUV 4:1:1
>>>> planar format is the one with depth 12. But for the purposes of the bytesperline
>>>> calculation only the depth of the largest plane counts, which is the luma plane
>>>> with a depth of 8. So for a width of 720 the value of bytesperline should be:
>>>>
>>>> depth=8 -> bytesperline = 720
>>>> depth=12 -> bytesperline = 720
>>> With depth=12, it should be, instead, 1080, as 2 pixels need 3 bytes.
>> No, it's not. It's a *planar* format: first the Y plane, then the two smaller
>> chroma planes. The spec says that bytesperline for planar formats refers to
>> the largest plane.
>>
>> For this format the luma plane is one byte per pixel. Each of the two chroma
>> planes have effectively two bits per pixel (actually one byte per four pixels),
>> so you end up with 8+2+2=12 bits per pixel.
>>
>> Hence bytesperline should be 720 for this particular format.
> If I understood what you just said, you're talking that the only format marked
> as depth=12 is actually depth=8, right? Then the fix would be to change depth
> in the table, and not here.

No, the depth is correct. and it is needed for image size calculation.

> Yet, I'm not sure if this is the proper fix.

The problem here which causes confusion is, that bytesperline actually
isn't bytesperline for planar formats (and hence size != bytesperline *
height) when we follow the current spec.
I don't understand the reason for handling this different for planar and
non-planar formats, but I'm sure there is a good one. Hans ?

Given that we can't and/or don't want to change the spec, the correct
fix would be to consider the plane size in the calculation.
This info can be derived either from the format type (implicit) or we
can add it to the format struct (explicit).

Example (using the ratio between the size of the largest plane and the
size of all planes together):

lp_ratio = 1    (for non-planar formats)
lp_ratio = 4/(4+1+1) = 4/6 = 2/3    (for YUV411P)
lp_ratio = 4/(4+2+2) = 4/8 = 1/2    (for YUV422P)
...

=> bytesperline = (width * depth * lp_ratio + 7) >> 3  


Regards,
Frank

> The only used I saw on userspace apps for this field is to allocate size for
> the memory buffer. Some userspace applications use to get bytesperline and
> multiply by the image height and get the image size, instead of relying
> on sizeimage, as some drivers didn't use to fill sizeimage properly.
>
> By using bytesperline equal to 1080 in this case warrants that the buffers
> on userspace will have enough space.
>
> Regards,
> Mauro