[RFC] capture-example: allow V4L2_PIX_FMT_GREY with USERPTR

  There is an assumption that the format coming from the device
needs 2 bytes per pixel, which is not the case when the device
delivers e.g. V4L2_PIX_FMT_GREY. This doesn't manifest itself with
IO_METHOD_MMAP because init_mmap() (the default) doesn't take
sizeimage as an argument.

Signed-off-by: Michael Jones <michael.jones@matrix-vision.de>
---

This same issue would apply to other formats which have 1 byte per pixel,
this patch only fixes it for GREY.  Is this OK for now, or does somebody
have a better suggestion for supporting other formats as well?

 contrib/test/capture-example.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)
  

Em 28-06-2011 11:23, Michael Jones escreveu:
> There is an assumption that the format coming from the device
> needs 2 bytes per pixel, which is not the case when the device
> delivers e.g. V4L2_PIX_FMT_GREY. This doesn't manifest itself with
> IO_METHOD_MMAP because init_mmap() (the default) doesn't take
> sizeimage as an argument.
> 
> Signed-off-by: Michael Jones <michael.jones@matrix-vision.de>
> ---
> 
> This same issue would apply to other formats which have 1 byte per pixel,
> this patch only fixes it for GREY.  Is this OK for now, or does somebody
> have a better suggestion for supporting other formats as well?

Well, just rely on the bytesperline provided by the driver should be enough.
Devices should be returning it on a consistent way.

Regards,
Mauro

> 
>  contrib/test/capture-example.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/contrib/test/capture-example.c b/contrib/test/capture-example.c
> index 3852c58..0eb5235 100644
> --- a/contrib/test/capture-example.c
> +++ b/contrib/test/capture-example.c
> @@ -416,6 +416,7 @@ static void init_device(void)
>  	struct v4l2_crop crop;
>  	struct v4l2_format fmt;
>  	unsigned int min;
> +	unsigned int bytes_per_pixel;
>  
>  	if (-1 == xioctl(fd, VIDIOC_QUERYCAP, &cap)) {
>  		if (EINVAL == errno) {
> @@ -519,7 +520,8 @@ static void init_device(void)
>  	}
>  
>  	/* Buggy driver paranoia. */
> -	min = fmt.fmt.pix.width * 2;
> +	bytes_per_pixel = fmt.fmt.pix.pixelformat == V4L2_PIX_FMT_GREY ? 1 : 2;
> +	min = fmt.fmt.pix.width * bytes_per_pixel;
>  	if (fmt.fmt.pix.bytesperline < min)
>  		fmt.fmt.pix.bytesperline = min;
>  	min = fmt.fmt.pix.bytesperline * fmt.fmt.pix.height;

--
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
  

Hi Mauro,

On 07/14/2011 12:35 AM, Mauro Carvalho Chehab wrote:
> Em 28-06-2011 11:23, Michael Jones escreveu:
>> There is an assumption that the format coming from the device
>> needs 2 bytes per pixel, which is not the case when the device
>> delivers e.g. V4L2_PIX_FMT_GREY. This doesn't manifest itself with
>> IO_METHOD_MMAP because init_mmap() (the default) doesn't take
>> sizeimage as an argument.
>>
>> Signed-off-by: Michael Jones <michael.jones@matrix-vision.de>
>> ---
>>
>> This same issue would apply to other formats which have 1 byte per pixel,
>> this patch only fixes it for GREY.  Is this OK for now, or does somebody
>> have a better suggestion for supporting other formats as well?
> 
> Well, just rely on the bytesperline provided by the driver should be enough.
> Devices should be returning it on a consistent way.
> 
> Regards,
> Mauro

So you would rather remove the "Buggy driver paranoia" altogether and
just trust the bytesperline from the driver? That's fine with me, but I
presumed the paranoia was there for a reason. Would you accept a patch
then that just removes the 7 lines which fiddle with bytesperline?

-Michael

> 
>>
>>  contrib/test/capture-example.c |    4 +++-
>>  1 files changed, 3 insertions(+), 1 deletions(-)
>>
>> diff --git a/contrib/test/capture-example.c b/contrib/test/capture-example.c
>> index 3852c58..0eb5235 100644
>> --- a/contrib/test/capture-example.c
>> +++ b/contrib/test/capture-example.c
>> @@ -416,6 +416,7 @@ static void init_device(void)
>>  	struct v4l2_crop crop;
>>  	struct v4l2_format fmt;
>>  	unsigned int min;
>> +	unsigned int bytes_per_pixel;
>>  
>>  	if (-1 == xioctl(fd, VIDIOC_QUERYCAP, &cap)) {
>>  		if (EINVAL == errno) {
>> @@ -519,7 +520,8 @@ static void init_device(void)
>>  	}
>>  
>>  	/* Buggy driver paranoia. */
>> -	min = fmt.fmt.pix.width * 2;
>> +	bytes_per_pixel = fmt.fmt.pix.pixelformat == V4L2_PIX_FMT_GREY ? 1 : 2;
>> +	min = fmt.fmt.pix.width * bytes_per_pixel;
>>  	if (fmt.fmt.pix.bytesperline < min)
>>  		fmt.fmt.pix.bytesperline = min;
>>  	min = fmt.fmt.pix.bytesperline * fmt.fmt.pix.height;
> 


MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler
Registergericht: Amtsgericht Stuttgart, HRB 271090
Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner, Erhard Meier
--
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
  

Em 14-07-2011 04:03, Michael Jones escreveu:
> Hi Mauro,
> 
> On 07/14/2011 12:35 AM, Mauro Carvalho Chehab wrote:
>> Em 28-06-2011 11:23, Michael Jones escreveu:
>>> There is an assumption that the format coming from the device
>>> needs 2 bytes per pixel, which is not the case when the device
>>> delivers e.g. V4L2_PIX_FMT_GREY. This doesn't manifest itself with
>>> IO_METHOD_MMAP because init_mmap() (the default) doesn't take
>>> sizeimage as an argument.
>>>
>>> Signed-off-by: Michael Jones <michael.jones@matrix-vision.de>
>>> ---
>>>
>>> This same issue would apply to other formats which have 1 byte per pixel,
>>> this patch only fixes it for GREY.  Is this OK for now, or does somebody
>>> have a better suggestion for supporting other formats as well?
>>
>> Well, just rely on the bytesperline provided by the driver should be enough.
>> Devices should be returning it on a consistent way.
>>
>> Regards,
>> Mauro
> 
> So you would rather remove the "Buggy driver paranoia" altogether and
> just trust the bytesperline from the driver?

In fact, to rely on sizeimage. The code there is:

	/* Buggy driver paranoia. */
	min = fmt.fmt.pix.width * 2;
	if (fmt.fmt.pix.bytesperline < min)
		fmt.fmt.pix.bytesperline = min;
	min = fmt.fmt.pix.bytesperline * fmt.fmt.pix.height;
	if (fmt.fmt.pix.sizeimage < min)
		fmt.fmt.pix.sizeimage = min;

See: the code there is just over-estimating the sizeimage. Provided that the drivers
are returning reliable sizeimages, the above code doesn't need to exist.

> That's fine with me, but I
> presumed the paranoia was there for a reason.

Several webcam drivers used to have bugs on the value reported by sizeimage,
or were just filling sizeimage with 0 or a small value and bytesperline with 0.
This were fixed already.

Hmm... there are afew drivers that fill bytesperline with 0, but they
fill sizeimage properly. For example:

drivers/media/video/cx18/cx18-ioctl.c:            pixfmt->sizeimage = 128 * 1024;
drivers/media/video/cx18/cx18-ioctl.c-            pixfmt->bytesperline = 0;
drivers/media/video/ivtv/ivtv-ioctl.c:            pixfmt->sizeimage = 128 * 1024;
drivers/media/video/ivtv/ivtv-ioctl.c-            pixfmt->bytesperline = 0;

If you want to be pedantic, is is probably worth to take a deeper look at the
drivers bellow, but, at least on a quick check, all of them seems to be properly
filling sizeimage:

$ git grep "bytesperline = 0" drivers/media/video/
drivers/media/video/cpia2/cpia2_v4l.c:    f->fmt.pix.bytesperline = 0;
drivers/media/video/cpia2/cpia2_v4l.c:    f->fmt.pix.bytesperline = 0;
drivers/media/video/cx18/cx18-ioctl.c:            pixfmt->bytesperline = 0;
drivers/media/video/cx231xx/cx231xx-417.c:        f->fmt.pix.bytesperline = 0;
drivers/media/video/cx231xx/cx231xx-417.c:        f->fmt.pix.bytesperline = 0;
drivers/media/video/cx23885/cx23885-417.c:        f->fmt.pix.bytesperline = 0;
drivers/media/video/cx23885/cx23885-417.c:        f->fmt.pix.bytesperline = 0;
drivers/media/video/cx23885/cx23885-417.c:        f->fmt.pix.bytesperline = 0;
drivers/media/video/cx88/cx88-blackbird.c:        f->fmt.pix.bytesperline = 0;
drivers/media/video/cx88/cx88-blackbird.c:        f->fmt.pix.bytesperline = 0;
drivers/media/video/cx88/cx88-blackbird.c:        f->fmt.pix.bytesperline = 0;
drivers/media/video/gspca/se401.c:                        sd->fmts[i].bytesperline = 0;
drivers/media/video/ivtv/ivtv-ioctl.c:            pixfmt->bytesperline = 0;
drivers/media/video/ivtv/ivtv-ioctl.c:            pixfmt->bytesperline = 0;
drivers/media/video/saa7164/saa7164-encoder.c:    f->fmt.pix.bytesperline = 0;
drivers/media/video/saa7164/saa7164-encoder.c:    f->fmt.pix.bytesperline = 0;
drivers/media/video/saa7164/saa7164-encoder.c:    f->fmt.pix.bytesperline = 0;
drivers/media/video/saa7164/saa7164-vbi.c:        f->fmt.pix.bytesperline = 0;
drivers/media/video/saa7164/saa7164-vbi.c:        f->fmt.pix.bytesperline = 0;
drivers/media/video/saa7164/saa7164-vbi.c:        f->fmt.pix.bytesperline = 0;
drivers/media/video/soc_camera.c: pix->bytesperline = 0;
drivers/media/video/zoran/zoran_card.c:   zr->vbuf_bytesperline = 0;
drivers/media/video/zoran/zoran_driver.c: fmt->fmt.pix.bytesperline = 0;
drivers/media/video/zoran/zoran_driver.c: fmt->fmt.pix.bytesperline = 0;
drivers/media/video/zoran/zoran_driver.c: fmt->fmt.pix.bytesperline = 0;

> Would you accept a patch
> then that just removes the 7 lines which fiddle with bytesperline?

Just removing the bytesperline seems appropriate for me.

The usage of bytesperline for buffer allocation is wrong. Drivers that use compression
often do things like:

drivers/media/video/gspca/conex.c-                .bytesperline = 640,
drivers/media/video/gspca/conex.c:                .sizeimage = 640 * 480 * 3 / 8 + 590,

The sizeimage considers the maximum size of the headers, and the worse case of the
compress algorithm. Userspace should not expect anything bigger than sizeimage, as
the driver internal buffers were allocated with sizeimage. So, in the unlikely case
that a frame got more than sizeimage, that frame will be dropped or corrupted (or
the driver will OOPS, if there's an internal bug). But nothing bigger than sizeimage
will be returned to userspace.

Thanks,
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