[PATCH v2] media: vim2m: better handle cap/out buffers with different sizes

[PATCH v2] media: vim2m: better handle cap/out buffers with different sizes

[Mauro Carvalho Chehab]

The vim2m driver doesn't enforce that the capture and output
buffers would have the same size. Do the right thing if the
buffers are different, zeroing the buffer before writing,
ensuring that lines will be aligned and it won't write past
the buffer area.

Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
---
 drivers/media/platform/vim2m.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/drivers/media/platform/vim2m.c b/drivers/media/platform/vim2m.c
index 89384f324e25..46e3e096123e 100644
--- a/drivers/media/platform/vim2m.c
+++ b/drivers/media/platform/vim2m.c
@@ -481,7 +481,9 @@ static int device_process(struct vim2m_ctx *ctx,
 	struct vim2m_dev *dev = ctx->dev;
 	struct vim2m_q_data *q_data_in, *q_data_out;
 	u8 *p_in, *p, *p_out;
-	int width, height, bytesperline, x, y, y_out, start, end, step;
+	unsigned int width, height, bytesperline, bytesperline_out;
+	unsigned int x, y, y_out;
+	int start, end, step;
 	struct vim2m_fmt *in, *out;
 
 	q_data_in = get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_OUTPUT);
@@ -491,8 +493,15 @@ static int device_process(struct vim2m_ctx *ctx,
 	bytesperline = (q_data_in->width * q_data_in->fmt->depth) >> 3;
 
 	q_data_out = get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE);
+	bytesperline_out = (q_data_out->width * q_data_out->fmt->depth) >> 3;
 	out = q_data_out->fmt;
 
+	/* Crop to the limits of the destination image */
+	if (width > q_data_out->width)
+		width = q_data_out->width;
+	if (height > q_data_out->height)
+		height = q_data_out->height;
+
 	p_in = vb2_plane_vaddr(&in_vb->vb2_buf, 0);
 	p_out = vb2_plane_vaddr(&out_vb->vb2_buf, 0);
 	if (!p_in || !p_out) {
@@ -501,6 +510,10 @@ static int device_process(struct vim2m_ctx *ctx,
 		return -EFAULT;
 	}
 
+	/* Image size is different. Zero buffer first */
+	if (q_data_in->width  != q_data_out->width ||
+	    q_data_in->height != q_data_out->height)
+		memset(p_out, 0, q_data_out->sizeimage);
 	out_vb->sequence = get_q_data(ctx,
 				      V4L2_BUF_TYPE_VIDEO_CAPTURE)->sequence++;
 	in_vb->sequence = q_data_in->sequence++;
@@ -524,6 +537,11 @@ static int device_process(struct vim2m_ctx *ctx,
 		for (x = 0; x < width >> 1; x++)
 			copy_two_pixels(in, out, &p, &p_out, y_out,
 					ctx->mode & MEM2MEM_HFLIP);
+
+		/* Go to the next line at the out buffer*/
+		if (width < q_data_out->width)
+			p_out += ((q_data_out->width - width)
+				  * q_data_out->fmt->depth) >> 3;
 	}
 
 	return 0;
@@ -977,12 +995,6 @@ static int vim2m_buf_prepare(struct vb2_buffer *vb)
 	dprintk(ctx->dev, 2, "type: %s\n", type_name(vb->vb2_queue->type));
 
 	q_data = get_q_data(ctx, vb->vb2_queue->type);
-	if (vb2_plane_size(vb, 0) < q_data->sizeimage) {
-		dprintk(ctx->dev, "%s data will not fit into plane (%lu < %lu)\n",
-				__func__, vb2_plane_size(vb, 0), (long)q_data->sizeimage);
-		return -EINVAL;
-	}
-
 	vb2_set_plane_payload(vb, 0, q_data->sizeimage);
 
 	return 0;
-- 
2.20.1

Re: [PATCH v2] media: vim2m: better handle cap/out buffers with different sizes

[Hans Verkuil]

On 2/26/19 6:36 PM, Mauro Carvalho Chehab wrote:
> The vim2m driver doesn't enforce that the capture and output
> buffers would have the same size. Do the right thing if the
> buffers are different, zeroing the buffer before writing,
> ensuring that lines will be aligned and it won't write past
> the buffer area.

I don't really like this. Since the vimc driver doesn't scale it shouldn't
automatically crop either. If you want to crop/compose, then the
selection API should be implemented.

That would be the right approach to allowing capture and output
formats (we're talking formats here, not buffer sizes) with
different resolutions.

> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
> ---
>  drivers/media/platform/vim2m.c | 26 +++++++++++++++++++-------
>  1 file changed, 19 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/platform/vim2m.c b/drivers/media/platform/vim2m.c
> index 89384f324e25..46e3e096123e 100644
> --- a/drivers/media/platform/vim2m.c
> +++ b/drivers/media/platform/vim2m.c
> @@ -481,7 +481,9 @@ static int device_process(struct vim2m_ctx *ctx,
>  	struct vim2m_dev *dev = ctx->dev;
>  	struct vim2m_q_data *q_data_in, *q_data_out;
>  	u8 *p_in, *p, *p_out;
> -	int width, height, bytesperline, x, y, y_out, start, end, step;
> +	unsigned int width, height, bytesperline, bytesperline_out;
> +	unsigned int x, y, y_out;
> +	int start, end, step;
>  	struct vim2m_fmt *in, *out;
>  
>  	q_data_in = get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_OUTPUT);
> @@ -491,8 +493,15 @@ static int device_process(struct vim2m_ctx *ctx,
>  	bytesperline = (q_data_in->width * q_data_in->fmt->depth) >> 3;
>  
>  	q_data_out = get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE);
> +	bytesperline_out = (q_data_out->width * q_data_out->fmt->depth) >> 3;
>  	out = q_data_out->fmt;
>  
> +	/* Crop to the limits of the destination image */
> +	if (width > q_data_out->width)
> +		width = q_data_out->width;
> +	if (height > q_data_out->height)
> +		height = q_data_out->height;
> +
>  	p_in = vb2_plane_vaddr(&in_vb->vb2_buf, 0);
>  	p_out = vb2_plane_vaddr(&out_vb->vb2_buf, 0);
>  	if (!p_in || !p_out) {
> @@ -501,6 +510,10 @@ static int device_process(struct vim2m_ctx *ctx,
>  		return -EFAULT;
>  	}
>  
> +	/* Image size is different. Zero buffer first */
> +	if (q_data_in->width  != q_data_out->width ||
> +	    q_data_in->height != q_data_out->height)
> +		memset(p_out, 0, q_data_out->sizeimage);
>  	out_vb->sequence = get_q_data(ctx,
>  				      V4L2_BUF_TYPE_VIDEO_CAPTURE)->sequence++;
>  	in_vb->sequence = q_data_in->sequence++;
> @@ -524,6 +537,11 @@ static int device_process(struct vim2m_ctx *ctx,
>  		for (x = 0; x < width >> 1; x++)
>  			copy_two_pixels(in, out, &p, &p_out, y_out,
>  					ctx->mode & MEM2MEM_HFLIP);
> +
> +		/* Go to the next line at the out buffer*/

Add space after 'buffer'.

> +		if (width < q_data_out->width)
> +			p_out += ((q_data_out->width - width)
> +				  * q_data_out->fmt->depth) >> 3;
>  	}
>  
>  	return 0;
> @@ -977,12 +995,6 @@ static int vim2m_buf_prepare(struct vb2_buffer *vb)
>  	dprintk(ctx->dev, 2, "type: %s\n", type_name(vb->vb2_queue->type));
>  
>  	q_data = get_q_data(ctx, vb->vb2_queue->type);
> -	if (vb2_plane_size(vb, 0) < q_data->sizeimage) {
> -		dprintk(ctx->dev, "%s data will not fit into plane (%lu < %lu)\n",
> -				__func__, vb2_plane_size(vb, 0), (long)q_data->sizeimage);
> -		return -EINVAL;
> -	}
> -

As discussed on irc, this can't be removed. It checks if the provided buffer
is large enough for the current format.

Regards,

	Hans

>  	vb2_set_plane_payload(vb, 0, q_data->sizeimage);
>  
>  	return 0;
> 

Re: [PATCH v2] media: vim2m: better handle cap/out buffers with different sizes

[Mauro Carvalho Chehab]

Em Thu, 28 Feb 2019 13:30:49 +0100
Hans Verkuil <hverkuil-cisco@xs4all.nl> escreveu:

> On 2/26/19 6:36 PM, Mauro Carvalho Chehab wrote:
> > The vim2m driver doesn't enforce that the capture and output
> > buffers would have the same size. Do the right thing if the
> > buffers are different, zeroing the buffer before writing,
> > ensuring that lines will be aligned and it won't write past
> > the buffer area.  
> 
> I don't really like this. Since the vimc driver doesn't scale it shouldn't
> automatically crop either. If you want to crop/compose, then the
> selection API should be implemented.
> 
> That would be the right approach to allowing capture and output
> formats (we're talking formats here, not buffer sizes) with
> different resolutions.

The original vim2m implementation assumes that this driver would
"scale" and do format conversions (except that it didn't do neither).

While I fixed the format conversion on a past patchset, vim2m
still allows a "free" image size on both sides of the pipeline.

I agree with you that the best would be to implement a scaler
(and maybe crop/compose), but for now, we need to solve an issue that
vim2m is doing a very poor job to confine the image at the destination
buffer's resolution.

Also, as far as I remember, the first M2M devices have scalers, so
existing apps likely assume that such devices will do scaling.

So, a non-scaling M2M device is something that, in thesis, we don't
support[1].

[1] Very likely we have several ones that don't do scaling, but the
needed API bits for apps to detect if scaling is supported or not
aren't implemented.

The problem of enforcing the resolution to be identical on both capture
and output buffers is that the V4L2 API doesn't really have
a way for userspace apps to identify that it won't scale until
too late.

How do you imagine an application negotiating the image resolution?

I mean, application A may set first the capture buffer to, let's say,
320x200 and then try to set the output buffer. 

Application B may do the reverse, e. g. set first the output buffer
to 320x200 and then try to set the capture buffer.

Application C could try to initialize with some default for both 
capture and output buffers and only later decide what resolution
it wants and try to set both sides.

Application D could have setup both sides, started streaming at 
320x200. Then, it stopped streaming and changed the capture 
resolution to, let's say 640x480, without changing the resolution
of the output buffer.

For all the above scenarios, the app may either first set both
sides and then request the buffer for both, or do S_FMT/REQBUFS
for one side and then to the other side.

What I mean is that, wit just use the existing ioctls and flags, I 
can't see any way for all the above scenarios work on devices that
don't scale.

One solution would be to filter the output of ENUM_FMT, TRY_FMT,
G_FMT and S_FMT when one of the sides of the M2M buffer is set,
but that would break some possible real usecases.

I suspect that the option that it would work best is to have a
flag to indicate that a M2M device has scalers.

In any case, this should be discussed and properly documented
before we would be able to implement a non-scaling M2M device.

-

Without a clear way for the API to report that the device can't scale,
an application like, for example, the GStreamer plugin, won't be able to 
detect that the resolutions should be identical until too late (at
STREAMON).

IMO, this is something that we should address, but it is out of the
scope of this fixup patch.

That's why I prefer to keep vim2m working supporting different
resolutions on each side of the M2M device.

-

That's said, I may end working on a very simple scaler patch for vim2m. 

I suspect that a simple decimation filtering like:

#define x_scale xout_max / xin_max
#define y_scale yout_max / yin_max

	out_pixel(x, y) = in_pixel(x * x_scale, y * y_scale)

would be simple enough to implement at the current image copy
thread.

Regards,
Mauro

> 
> > 
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
> > ---
> >  drivers/media/platform/vim2m.c | 26 +++++++++++++++++++-------
> >  1 file changed, 19 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/media/platform/vim2m.c b/drivers/media/platform/vim2m.c
> > index 89384f324e25..46e3e096123e 100644
> > --- a/drivers/media/platform/vim2m.c
> > +++ b/drivers/media/platform/vim2m.c
> > @@ -481,7 +481,9 @@ static int device_process(struct vim2m_ctx *ctx,
> >  	struct vim2m_dev *dev = ctx->dev;
> >  	struct vim2m_q_data *q_data_in, *q_data_out;
> >  	u8 *p_in, *p, *p_out;
> > -	int width, height, bytesperline, x, y, y_out, start, end, step;
> > +	unsigned int width, height, bytesperline, bytesperline_out;
> > +	unsigned int x, y, y_out;
> > +	int start, end, step;
> >  	struct vim2m_fmt *in, *out;
> >  
> >  	q_data_in = get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_OUTPUT);
> > @@ -491,8 +493,15 @@ static int device_process(struct vim2m_ctx *ctx,
> >  	bytesperline = (q_data_in->width * q_data_in->fmt->depth) >> 3;
> >  
> >  	q_data_out = get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE);
> > +	bytesperline_out = (q_data_out->width * q_data_out->fmt->depth) >> 3;
> >  	out = q_data_out->fmt;
> >  
> > +	/* Crop to the limits of the destination image */
> > +	if (width > q_data_out->width)
> > +		width = q_data_out->width;
> > +	if (height > q_data_out->height)
> > +		height = q_data_out->height;
> > +
> >  	p_in = vb2_plane_vaddr(&in_vb->vb2_buf, 0);
> >  	p_out = vb2_plane_vaddr(&out_vb->vb2_buf, 0);
> >  	if (!p_in || !p_out) {
> > @@ -501,6 +510,10 @@ static int device_process(struct vim2m_ctx *ctx,
> >  		return -EFAULT;
> >  	}
> >  
> > +	/* Image size is different. Zero buffer first */
> > +	if (q_data_in->width  != q_data_out->width ||
> > +	    q_data_in->height != q_data_out->height)
> > +		memset(p_out, 0, q_data_out->sizeimage);
> >  	out_vb->sequence = get_q_data(ctx,
> >  				      V4L2_BUF_TYPE_VIDEO_CAPTURE)->sequence++;
> >  	in_vb->sequence = q_data_in->sequence++;
> > @@ -524,6 +537,11 @@ static int device_process(struct vim2m_ctx *ctx,
> >  		for (x = 0; x < width >> 1; x++)
> >  			copy_two_pixels(in, out, &p, &p_out, y_out,
> >  					ctx->mode & MEM2MEM_HFLIP);
> > +
> > +		/* Go to the next line at the out buffer*/  
> 
> Add space after 'buffer'.
> 
> > +		if (width < q_data_out->width)
> > +			p_out += ((q_data_out->width - width)
> > +				  * q_data_out->fmt->depth) >> 3;
> >  	}
> >  
> >  	return 0;
> > @@ -977,12 +995,6 @@ static int vim2m_buf_prepare(struct vb2_buffer *vb)
> >  	dprintk(ctx->dev, 2, "type: %s\n", type_name(vb->vb2_queue->type));
> >  
> >  	q_data = get_q_data(ctx, vb->vb2_queue->type);
> > -	if (vb2_plane_size(vb, 0) < q_data->sizeimage) {
> > -		dprintk(ctx->dev, "%s data will not fit into plane (%lu < %lu)\n",
> > -				__func__, vb2_plane_size(vb, 0), (long)q_data->sizeimage);
> > -		return -EINVAL;
> > -	}
> > -  
> 
> As discussed on irc, this can't be removed. It checks if the provided buffer
> is large enough for the current format.
> 
> Regards,
> 
> 	Hans
> 
> >  	vb2_set_plane_payload(vb, 0, q_data->sizeimage);
> >  
> >  	return 0;
> >   
> 



Thanks,
Mauro

Re: [PATCH v2] media: vim2m: better handle cap/out buffers with different sizes

[Hans Verkuil]

On 2/28/19 3:09 PM, Mauro Carvalho Chehab wrote:
> Em Thu, 28 Feb 2019 13:30:49 +0100
> Hans Verkuil <hverkuil-cisco@xs4all.nl> escreveu:
> 
>> On 2/26/19 6:36 PM, Mauro Carvalho Chehab wrote:
>>> The vim2m driver doesn't enforce that the capture and output
>>> buffers would have the same size. Do the right thing if the
>>> buffers are different, zeroing the buffer before writing,
>>> ensuring that lines will be aligned and it won't write past
>>> the buffer area.  
>>
>> I don't really like this. Since the vimc driver doesn't scale it shouldn't
>> automatically crop either. If you want to crop/compose, then the
>> selection API should be implemented.
>>
>> That would be the right approach to allowing capture and output
>> formats (we're talking formats here, not buffer sizes) with
>> different resolutions.
> 
> The original vim2m implementation assumes that this driver would
> "scale" and do format conversions (except that it didn't do neither).

I'm not sure we should assume anything about the original implementation.
It had lots of issues. I rather do this right then keep supporting hacks.

> While I fixed the format conversion on a past patchset, vim2m
> still allows a "free" image size on both sides of the pipeline.
> 
> I agree with you that the best would be to implement a scaler
> (and maybe crop/compose), but for now, we need to solve an issue that
> vim2m is doing a very poor job to confine the image at the destination
> buffer's resolution.
> 
> Also, as far as I remember, the first M2M devices have scalers, so
> existing apps likely assume that such devices will do scaling.

Most m2m devices are codecs and codecs do not do scaling (at least,
I'm not aware of any).

I don't think there are many m2m devices that do scaling: most do
format conversion of one type or another (codecs, deinterlacers,
yuv-rgb converters). Scalers tend to be integrated into a video
pipeline. The only m2m drivers I found that also scale are
exynos-gsc, ti-vpe, mtk-mdp and possibly sti/bdisp.

> 
> So, a non-scaling M2M device is something that, in thesis, we don't
> support[1].
> 
> [1] Very likely we have several ones that don't do scaling, but the
> needed API bits for apps to detect if scaling is supported or not
> aren't implemented.
> 
> The problem of enforcing the resolution to be identical on both capture
> and output buffers is that the V4L2 API doesn't really have
> a way for userspace apps to identify that it won't scale until
> too late.
> 
> How do you imagine an application negotiating the image resolution?
> 
> I mean, application A may set first the capture buffer to, let's say,
> 320x200 and then try to set the output buffer. 
> 
> Application B may do the reverse, e. g. set first the output buffer
> to 320x200 and then try to set the capture buffer.
> 
> Application C could try to initialize with some default for both 
> capture and output buffers and only later decide what resolution
> it wants and try to set both sides.
> 
> Application D could have setup both sides, started streaming at 
> 320x200. Then, it stopped streaming and changed the capture 
> resolution to, let's say 640x480, without changing the resolution
> of the output buffer.
> 
> For all the above scenarios, the app may either first set both
> sides and then request the buffer for both, or do S_FMT/REQBUFS
> for one side and then to the other side.
> 
> What I mean is that, wit just use the existing ioctls and flags, I 
> can't see any way for all the above scenarios work on devices that
> don't scale.

If the device cannot scale, then setting the format on either capture
or output will modify the format on the other side as well.

If the device also support cropping and composing, then it becomes
more complicated, but the basics remain the same.

It would certainly be nice if there was a scaling capability. I suspect
one reason that nobody requested this is that you usually know what
you are doing when you use an m2m device.

> One solution would be to filter the output of ENUM_FMT, TRY_FMT,
> G_FMT and S_FMT when one of the sides of the M2M buffer is set,
> but that would break some possible real usecases.

Not sure what you mean here.

> 
> I suspect that the option that it would work best is to have a
> flag to indicate that a M2M device has scalers.
> 
> In any case, this should be discussed and properly documented
> before we would be able to implement a non-scaling M2M device.

I don't know where you get the idea that most m2m devices scale.
The reverse is true, very few m2m devices have a scaler.

> 
> -
> 
> Without a clear way for the API to report that the device can't scale,
> an application like, for example, the GStreamer plugin, won't be able to 
> detect that the resolutions should be identical until too late (at
> STREAMON).
> 
> IMO, this is something that we should address, but it is out of the
> scope of this fixup patch.
> 
> That's why I prefer to keep vim2m working supporting different
> resolutions on each side of the M2M device.

I suspect it was always just a bug in vim2m that it allowed different
resolutions for capture and output.

> 
> -
> 
> That's said, I may end working on a very simple scaler patch for vim2m. 

v4l2-tpg-core.c uses Coarse Bresenham scaling to implement the scaler.

Regards,

	Hans

> 
> I suspect that a simple decimation filtering like:
> 
> #define x_scale xout_max / xin_max
> #define y_scale yout_max / yin_max
> 
> 	out_pixel(x, y) = in_pixel(x * x_scale, y * y_scale)
> 
> would be simple enough to implement at the current image copy
> thread.
> 
> Regards,
> Mauro
> 
>>
>>>
>>> Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
>>> ---
>>>  drivers/media/platform/vim2m.c | 26 +++++++++++++++++++-------
>>>  1 file changed, 19 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/vim2m.c b/drivers/media/platform/vim2m.c
>>> index 89384f324e25..46e3e096123e 100644
>>> --- a/drivers/media/platform/vim2m.c
>>> +++ b/drivers/media/platform/vim2m.c
>>> @@ -481,7 +481,9 @@ static int device_process(struct vim2m_ctx *ctx,
>>>  	struct vim2m_dev *dev = ctx->dev;
>>>  	struct vim2m_q_data *q_data_in, *q_data_out;
>>>  	u8 *p_in, *p, *p_out;
>>> -	int width, height, bytesperline, x, y, y_out, start, end, step;
>>> +	unsigned int width, height, bytesperline, bytesperline_out;
>>> +	unsigned int x, y, y_out;
>>> +	int start, end, step;
>>>  	struct vim2m_fmt *in, *out;
>>>  
>>>  	q_data_in = get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_OUTPUT);
>>> @@ -491,8 +493,15 @@ static int device_process(struct vim2m_ctx *ctx,
>>>  	bytesperline = (q_data_in->width * q_data_in->fmt->depth) >> 3;
>>>  
>>>  	q_data_out = get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE);
>>> +	bytesperline_out = (q_data_out->width * q_data_out->fmt->depth) >> 3;
>>>  	out = q_data_out->fmt;
>>>  
>>> +	/* Crop to the limits of the destination image */
>>> +	if (width > q_data_out->width)
>>> +		width = q_data_out->width;
>>> +	if (height > q_data_out->height)
>>> +		height = q_data_out->height;
>>> +
>>>  	p_in = vb2_plane_vaddr(&in_vb->vb2_buf, 0);
>>>  	p_out = vb2_plane_vaddr(&out_vb->vb2_buf, 0);
>>>  	if (!p_in || !p_out) {
>>> @@ -501,6 +510,10 @@ static int device_process(struct vim2m_ctx *ctx,
>>>  		return -EFAULT;
>>>  	}
>>>  
>>> +	/* Image size is different. Zero buffer first */
>>> +	if (q_data_in->width  != q_data_out->width ||
>>> +	    q_data_in->height != q_data_out->height)
>>> +		memset(p_out, 0, q_data_out->sizeimage);
>>>  	out_vb->sequence = get_q_data(ctx,
>>>  				      V4L2_BUF_TYPE_VIDEO_CAPTURE)->sequence++;
>>>  	in_vb->sequence = q_data_in->sequence++;
>>> @@ -524,6 +537,11 @@ static int device_process(struct vim2m_ctx *ctx,
>>>  		for (x = 0; x < width >> 1; x++)
>>>  			copy_two_pixels(in, out, &p, &p_out, y_out,
>>>  					ctx->mode & MEM2MEM_HFLIP);
>>> +
>>> +		/* Go to the next line at the out buffer*/  
>>
>> Add space after 'buffer'.
>>
>>> +		if (width < q_data_out->width)
>>> +			p_out += ((q_data_out->width - width)
>>> +				  * q_data_out->fmt->depth) >> 3;
>>>  	}
>>>  
>>>  	return 0;
>>> @@ -977,12 +995,6 @@ static int vim2m_buf_prepare(struct vb2_buffer *vb)
>>>  	dprintk(ctx->dev, 2, "type: %s\n", type_name(vb->vb2_queue->type));
>>>  
>>>  	q_data = get_q_data(ctx, vb->vb2_queue->type);
>>> -	if (vb2_plane_size(vb, 0) < q_data->sizeimage) {
>>> -		dprintk(ctx->dev, "%s data will not fit into plane (%lu < %lu)\n",
>>> -				__func__, vb2_plane_size(vb, 0), (long)q_data->sizeimage);
>>> -		return -EINVAL;
>>> -	}
>>> -  
>>
>> As discussed on irc, this can't be removed. It checks if the provided buffer
>> is large enough for the current format.
>>
>> Regards,
>>
>> 	Hans
>>
>>>  	vb2_set_plane_payload(vb, 0, q_data->sizeimage);
>>>  
>>>  	return 0;
>>>   
>>
> 
> 
> 
> Thanks,
> Mauro
> 

Re: [PATCH v2] media: vim2m: better handle cap/out buffers with different sizes

[Mauro Carvalho Chehab]

Em Thu, 28 Feb 2019 15:35:07 +0100
Hans Verkuil <hverkuil-cisco@xs4all.nl> escreveu:

> On 2/28/19 3:09 PM, Mauro Carvalho Chehab wrote:
> > Em Thu, 28 Feb 2019 13:30:49 +0100
> > Hans Verkuil <hverkuil-cisco@xs4all.nl> escreveu:
> >   
> >> On 2/26/19 6:36 PM, Mauro Carvalho Chehab wrote:  
> >>> The vim2m driver doesn't enforce that the capture and output
> >>> buffers would have the same size. Do the right thing if the
> >>> buffers are different, zeroing the buffer before writing,
> >>> ensuring that lines will be aligned and it won't write past
> >>> the buffer area.    
> >>
> >> I don't really like this. Since the vimc driver doesn't scale it shouldn't
> >> automatically crop either. If you want to crop/compose, then the
> >> selection API should be implemented.
> >>
> >> That would be the right approach to allowing capture and output
> >> formats (we're talking formats here, not buffer sizes) with
> >> different resolutions.  
> > 
> > The original vim2m implementation assumes that this driver would
> > "scale" and do format conversions (except that it didn't do neither).  
> 
> I'm not sure we should assume anything about the original implementation.
> It had lots of issues. I rather do this right then keep supporting hacks.

True, but we are too close to the merge window. That's why I opted on
solving the bug, and not changing the behavior.

> 
> > While I fixed the format conversion on a past patchset, vim2m
> > still allows a "free" image size on both sides of the pipeline.
> > 
> > I agree with you that the best would be to implement a scaler
> > (and maybe crop/compose), but for now, we need to solve an issue that
> > vim2m is doing a very poor job to confine the image at the destination
> > buffer's resolution.
> > 
> > Also, as far as I remember, the first M2M devices have scalers, so
> > existing apps likely assume that such devices will do scaling.  
> 
> Most m2m devices are codecs and codecs do not do scaling (at least,
> I'm not aware of any).

At least on GStreamer, codecs are implemented on a separate logic.
GStreamer checks it using the FOURCC types. If one of the sides is
compressed, it is either an encoder or a decoder. Otherwise, it is
a transform device.

From what I understood from the code (and from my tests), it
assumes that scaler is supported for transform devices.

> I don't think there are many m2m devices that do scaling: most do
> format conversion of one type or another (codecs, deinterlacers,
> yuv-rgb converters). Scalers tend to be integrated into a video
> pipeline. The only m2m drivers I found that also scale are
> exynos-gsc, ti-vpe, mtk-mdp and possibly sti/bdisp.

Well, the M2M API was added with Exynos. 

> > 
> > So, a non-scaling M2M device is something that, in thesis, we don't
> > support[1].
> > 
> > [1] Very likely we have several ones that don't do scaling, but the
> > needed API bits for apps to detect if scaling is supported or not
> > aren't implemented.
> > 
> > The problem of enforcing the resolution to be identical on both capture
> > and output buffers is that the V4L2 API doesn't really have
> > a way for userspace apps to identify that it won't scale until
> > too late.
> > 
> > How do you imagine an application negotiating the image resolution?
> > 
> > I mean, application A may set first the capture buffer to, let's say,
> > 320x200 and then try to set the output buffer. 
> > 
> > Application B may do the reverse, e. g. set first the output buffer
> > to 320x200 and then try to set the capture buffer.
> > 
> > Application C could try to initialize with some default for both 
> > capture and output buffers and only later decide what resolution
> > it wants and try to set both sides.
> > 
> > Application D could have setup both sides, started streaming at 
> > 320x200. Then, it stopped streaming and changed the capture 
> > resolution to, let's say 640x480, without changing the resolution
> > of the output buffer.
> > 
> > For all the above scenarios, the app may either first set both
> > sides and then request the buffer for both, or do S_FMT/REQBUFS
> > for one side and then to the other side.
> > 
> > What I mean is that, wit just use the existing ioctls and flags, I 
> > can't see any way for all the above scenarios work on devices that
> > don't scale.  
> 
> If the device cannot scale, then setting the format on either capture
> or output will modify the format on the other side as well.

That's one way to handle it, but what happens if buffers were already
allocated at one side? If the buffer is USERPTR, this is even worse,
as the size may not fit the image anymore.

Also, changing drivers to this new behavior could break some stuff.

(with this particular matter, changing vim2m code doesn't count as a
change, as this driver should not be used in production - but if any
other driver is doing something different, then we're limited to do
such change)

> 
> If the device also support cropping and composing, then it becomes
> more complicated, but the basics remain the same.
> 

I suspect that the above are just assumptions (and perhaps the current
implementation on most drivers). At least, I was unable to find any mention
about the M2M chapter at the V4L2 specs.

> It would certainly be nice if there was a scaling capability. I suspect
> one reason that nobody requested this is that you usually know what
> you are doing when you use an m2m device.

And that's a bad assumption, as it prevents having generic apps
using it. The expected behavior for having and not having scaler
should be described, and we need to be able to cope with legacy stuff.

> 
> > One solution would be to filter the output of ENUM_FMT, TRY_FMT,
> > G_FMT and S_FMT when one of the sides of the M2M buffer is set,
> > but that would break some possible real usecases.  
> 
> Not sure what you mean here.

I mean that one possible solution would be that, if one side sets 
resolution, the answer for the ioctls on the other side would be
different. IMHO, that's a bad idea.

> > 
> > I suspect that the option that it would work best is to have a
> > flag to indicate that a M2M device has scalers.
> > 
> > In any case, this should be discussed and properly documented
> > before we would be able to implement a non-scaling M2M device.  
> 
> I don't know where you get the idea that most m2m devices scale.
> The reverse is true, very few m2m devices have a scaler.

No, I didn't say that most m2m devices scale. I said that the initial
M2M implementations scale, and that's probably one of the reasons why 
this is the behavior that Gstreamer expects scales on transform devices.

> 
> > 
> > -
> > 
> > Without a clear way for the API to report that the device can't scale,
> > an application like, for example, the GStreamer plugin, won't be able to 
> > detect that the resolutions should be identical until too late (at
> > STREAMON).
> > 
> > IMO, this is something that we should address, but it is out of the
> > scope of this fixup patch.
> > 
> > That's why I prefer to keep vim2m working supporting different
> > resolutions on each side of the M2M device.  
> 
> I suspect it was always just a bug in vim2m that it allowed different
> resolutions for capture and output.
> 
> > 
> > -
> > 
> > That's said, I may end working on a very simple scaler patch for vim2m.   
> 
> v4l2-tpg-core.c uses Coarse Bresenham scaling to implement the scaler.

Yeah, we could reuse part of the logic there, but the challenge here is
to do that at the same logic we already do HFLIP/VFLIP and image transform.

I'll seek for some time to do that.

> 
> Regards,
> 
> 	Hans
> 
> > 
> > I suspect that a simple decimation filtering like:
> > 
> > #define x_scale xout_max / xin_max
> > #define y_scale yout_max / yin_max
> > 
> > 	out_pixel(x, y) = in_pixel(x * x_scale, y * y_scale)
> > 
> > would be simple enough to implement at the current image copy
> > thread.
> > 
> > Regards,
> > Mauro
> >   
> >>  
> >>>
> >>> Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
> >>> ---
> >>>  drivers/media/platform/vim2m.c | 26 +++++++++++++++++++-------
> >>>  1 file changed, 19 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/drivers/media/platform/vim2m.c b/drivers/media/platform/vim2m.c
> >>> index 89384f324e25..46e3e096123e 100644
> >>> --- a/drivers/media/platform/vim2m.c
> >>> +++ b/drivers/media/platform/vim2m.c
> >>> @@ -481,7 +481,9 @@ static int device_process(struct vim2m_ctx *ctx,
> >>>  	struct vim2m_dev *dev = ctx->dev;
> >>>  	struct vim2m_q_data *q_data_in, *q_data_out;
> >>>  	u8 *p_in, *p, *p_out;
> >>> -	int width, height, bytesperline, x, y, y_out, start, end, step;
> >>> +	unsigned int width, height, bytesperline, bytesperline_out;
> >>> +	unsigned int x, y, y_out;
> >>> +	int start, end, step;
> >>>  	struct vim2m_fmt *in, *out;
> >>>  
> >>>  	q_data_in = get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_OUTPUT);
> >>> @@ -491,8 +493,15 @@ static int device_process(struct vim2m_ctx *ctx,
> >>>  	bytesperline = (q_data_in->width * q_data_in->fmt->depth) >> 3;
> >>>  
> >>>  	q_data_out = get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE);
> >>> +	bytesperline_out = (q_data_out->width * q_data_out->fmt->depth) >> 3;
> >>>  	out = q_data_out->fmt;
> >>>  
> >>> +	/* Crop to the limits of the destination image */
> >>> +	if (width > q_data_out->width)
> >>> +		width = q_data_out->width;
> >>> +	if (height > q_data_out->height)
> >>> +		height = q_data_out->height;
> >>> +
> >>>  	p_in = vb2_plane_vaddr(&in_vb->vb2_buf, 0);
> >>>  	p_out = vb2_plane_vaddr(&out_vb->vb2_buf, 0);
> >>>  	if (!p_in || !p_out) {
> >>> @@ -501,6 +510,10 @@ static int device_process(struct vim2m_ctx *ctx,
> >>>  		return -EFAULT;
> >>>  	}
> >>>  
> >>> +	/* Image size is different. Zero buffer first */
> >>> +	if (q_data_in->width  != q_data_out->width ||
> >>> +	    q_data_in->height != q_data_out->height)
> >>> +		memset(p_out, 0, q_data_out->sizeimage);
> >>>  	out_vb->sequence = get_q_data(ctx,
> >>>  				      V4L2_BUF_TYPE_VIDEO_CAPTURE)->sequence++;
> >>>  	in_vb->sequence = q_data_in->sequence++;
> >>> @@ -524,6 +537,11 @@ static int device_process(struct vim2m_ctx *ctx,
> >>>  		for (x = 0; x < width >> 1; x++)
> >>>  			copy_two_pixels(in, out, &p, &p_out, y_out,
> >>>  					ctx->mode & MEM2MEM_HFLIP);
> >>> +
> >>> +		/* Go to the next line at the out buffer*/    
> >>
> >> Add space after 'buffer'.
> >>  
> >>> +		if (width < q_data_out->width)
> >>> +			p_out += ((q_data_out->width - width)
> >>> +				  * q_data_out->fmt->depth) >> 3;
> >>>  	}
> >>>  
> >>>  	return 0;
> >>> @@ -977,12 +995,6 @@ static int vim2m_buf_prepare(struct vb2_buffer *vb)
> >>>  	dprintk(ctx->dev, 2, "type: %s\n", type_name(vb->vb2_queue->type));
> >>>  
> >>>  	q_data = get_q_data(ctx, vb->vb2_queue->type);
> >>> -	if (vb2_plane_size(vb, 0) < q_data->sizeimage) {
> >>> -		dprintk(ctx->dev, "%s data will not fit into plane (%lu < %lu)\n",
> >>> -				__func__, vb2_plane_size(vb, 0), (long)q_data->sizeimage);
> >>> -		return -EINVAL;
> >>> -	}
> >>> -    
> >>
> >> As discussed on irc, this can't be removed. It checks if the provided buffer
> >> is large enough for the current format.
> >>
> >> Regards,
> >>
> >> 	Hans
> >>  
> >>>  	vb2_set_plane_payload(vb, 0, q_data->sizeimage);
> >>>  
> >>>  	return 0;
> >>>     
> >>  
> > 
> > 
> > 
> > Thanks,
> > Mauro
> >   
> 



Thanks,
Mauro

Re: [PATCH v2] media: vim2m: better handle cap/out buffers with different sizes

[Hans Verkuil]

On 2/28/19 4:21 PM, Mauro Carvalho Chehab wrote:
> Em Thu, 28 Feb 2019 15:35:07 +0100
> Hans Verkuil <hverkuil-cisco@xs4all.nl> escreveu:
> 
>> On 2/28/19 3:09 PM, Mauro Carvalho Chehab wrote:
>>> Em Thu, 28 Feb 2019 13:30:49 +0100
>>> Hans Verkuil <hverkuil-cisco@xs4all.nl> escreveu:
>>>   
>>>> On 2/26/19 6:36 PM, Mauro Carvalho Chehab wrote:  
>>>>> The vim2m driver doesn't enforce that the capture and output
>>>>> buffers would have the same size. Do the right thing if the
>>>>> buffers are different, zeroing the buffer before writing,
>>>>> ensuring that lines will be aligned and it won't write past
>>>>> the buffer area.    
>>>>
>>>> I don't really like this. Since the vimc driver doesn't scale it shouldn't
>>>> automatically crop either. If you want to crop/compose, then the
>>>> selection API should be implemented.
>>>>
>>>> That would be the right approach to allowing capture and output
>>>> formats (we're talking formats here, not buffer sizes) with
>>>> different resolutions.  
>>>
>>> The original vim2m implementation assumes that this driver would
>>> "scale" and do format conversions (except that it didn't do neither).  
>>
>> I'm not sure we should assume anything about the original implementation.
>> It had lots of issues. I rather do this right then keep supporting hacks.
> 
> True, but we are too close to the merge window. That's why I opted on
> solving the bug, and not changing the behavior.

Then that should be documented in the commit log: i.e. it is a temporary
fix until we have a better solution. I'm fine with that.

> 
>>
>>> While I fixed the format conversion on a past patchset, vim2m
>>> still allows a "free" image size on both sides of the pipeline.
>>>
>>> I agree with you that the best would be to implement a scaler
>>> (and maybe crop/compose), but for now, we need to solve an issue that
>>> vim2m is doing a very poor job to confine the image at the destination
>>> buffer's resolution.
>>>
>>> Also, as far as I remember, the first M2M devices have scalers, so
>>> existing apps likely assume that such devices will do scaling.  
>>
>> Most m2m devices are codecs and codecs do not do scaling (at least,
>> I'm not aware of any).
> 
> At least on GStreamer, codecs are implemented on a separate logic.
> GStreamer checks it using the FOURCC types. If one of the sides is
> compressed, it is either an encoder or a decoder. Otherwise, it is
> a transform device.
> 
> From what I understood from the code (and from my tests), it
> assumes that scaler is supported for transform devices.

I find it hard to believe that it assumes a transform device can always
scale. Nicolas?

> 
>> I don't think there are many m2m devices that do scaling: most do
>> format conversion of one type or another (codecs, deinterlacers,
>> yuv-rgb converters). Scalers tend to be integrated into a video
>> pipeline. The only m2m drivers I found that also scale are
>> exynos-gsc, ti-vpe, mtk-mdp and possibly sti/bdisp.
> 
> Well, the M2M API was added with Exynos. 
> 
>>>
>>> So, a non-scaling M2M device is something that, in thesis, we don't
>>> support[1].
>>>
>>> [1] Very likely we have several ones that don't do scaling, but the
>>> needed API bits for apps to detect if scaling is supported or not
>>> aren't implemented.
>>>
>>> The problem of enforcing the resolution to be identical on both capture
>>> and output buffers is that the V4L2 API doesn't really have
>>> a way for userspace apps to identify that it won't scale until
>>> too late.
>>>
>>> How do you imagine an application negotiating the image resolution?
>>>
>>> I mean, application A may set first the capture buffer to, let's say,
>>> 320x200 and then try to set the output buffer. 
>>>
>>> Application B may do the reverse, e. g. set first the output buffer
>>> to 320x200 and then try to set the capture buffer.
>>>
>>> Application C could try to initialize with some default for both 
>>> capture and output buffers and only later decide what resolution
>>> it wants and try to set both sides.
>>>
>>> Application D could have setup both sides, started streaming at 
>>> 320x200. Then, it stopped streaming and changed the capture 
>>> resolution to, let's say 640x480, without changing the resolution
>>> of the output buffer.
>>>
>>> For all the above scenarios, the app may either first set both
>>> sides and then request the buffer for both, or do S_FMT/REQBUFS
>>> for one side and then to the other side.
>>>
>>> What I mean is that, wit just use the existing ioctls and flags, I 
>>> can't see any way for all the above scenarios work on devices that
>>> don't scale.  
>>
>> If the device cannot scale, then setting the format on either capture
>> or output will modify the format on the other side as well.
> 
> That's one way to handle it, but what happens if buffers were already
> allocated at one side?

Then trying to change the format on the other side will simply return
the current format.

 If the buffer is USERPTR, this is even worse,
> as the size may not fit the image anymore.

That's why you shouldn't remove the check in buf_prepare :-)

> 
> Also, changing drivers to this new behavior could break some stuff.

I'm not at all certain if all m2m drivers are safe when in comes to this.
The v4l2-compliance testing for m2m devices has always lagged behind the
testing for non-m2m devices, so we don't do in-depth tests. Otherwise we'd
found this vim2m issue a long time ago.

And I don't think it is new behavior. When it comes to m2m devices
they just behave according to the standard v4l2 behavior: the driver
always tries to fulfill the last ioctl to the best of its ability.

That said, this has never been made explicit in the spec.

> 
> (with this particular matter, changing vim2m code doesn't count as a
> change, as this driver should not be used in production - but if any
> other driver is doing something different, then we're limited to do
> such change)
> 
>>
>> If the device also support cropping and composing, then it becomes
>> more complicated, but the basics remain the same.
>>
> 
> I suspect that the above are just assumptions (and perhaps the current
> implementation on most drivers). At least, I was unable to find any mention
> about the M2M chapter at the V4L2 specs.
> 
>> It would certainly be nice if there was a scaling capability. I suspect
>> one reason that nobody requested this is that you usually know what
>> you are doing when you use an m2m device.
> 
> And that's a bad assumption, as it prevents having generic apps
> using it. The expected behavior for having and not having scaler
> should be described, and we need to be able to cope with legacy stuff.

We should likely write up something similar as what we are doing for
the codec documentation, but then for m2m transform devices.

> 
>>
>>> One solution would be to filter the output of ENUM_FMT, TRY_FMT,
>>> G_FMT and S_FMT when one of the sides of the M2M buffer is set,
>>> but that would break some possible real usecases.  
>>
>> Not sure what you mean here.
> 
> I mean that one possible solution would be that, if one side sets 
> resolution, the answer for the ioctls on the other side would be
> different. IMHO, that's a bad idea.
> 
>>>
>>> I suspect that the option that it would work best is to have a
>>> flag to indicate that a M2M device has scalers.
>>>
>>> In any case, this should be discussed and properly documented
>>> before we would be able to implement a non-scaling M2M device.  
>>
>> I don't know where you get the idea that most m2m devices scale.
>> The reverse is true, very few m2m devices have a scaler.
> 
> No, I didn't say that most m2m devices scale. I said that the initial
> M2M implementations scale, and that's probably one of the reasons why 
> this is the behavior that Gstreamer expects scales on transform devices.

Well, it doesn't really matter. The fact is that gstreamer apparently
makes assumptions that are not in general valid.

And all this is made worse by insufficiently detailed documentation and
insufficient compliance testing.

Patches are welcome.

Regarding this vim2m patch: I'm fine with this patch if it is clear that
it is a temporary fix and that it should really implement a real scaler
instead of cropping.

Regards,

	Hans

> 
>>
>>>
>>> -
>>>
>>> Without a clear way for the API to report that the device can't scale,
>>> an application like, for example, the GStreamer plugin, won't be able to 
>>> detect that the resolutions should be identical until too late (at
>>> STREAMON).
>>>
>>> IMO, this is something that we should address, but it is out of the
>>> scope of this fixup patch.
>>>
>>> That's why I prefer to keep vim2m working supporting different
>>> resolutions on each side of the M2M device.  
>>
>> I suspect it was always just a bug in vim2m that it allowed different
>> resolutions for capture and output.
>>
>>>
>>> -
>>>
>>> That's said, I may end working on a very simple scaler patch for vim2m.   
>>
>> v4l2-tpg-core.c uses Coarse Bresenham scaling to implement the scaler.
> 
> Yeah, we could reuse part of the logic there, but the challenge here is
> to do that at the same logic we already do HFLIP/VFLIP and image transform.
> 
> I'll seek for some time to do that.
> 
>>
>> Regards,
>>
>> 	Hans
>>
>>>
>>> I suspect that a simple decimation filtering like:
>>>
>>> #define x_scale xout_max / xin_max
>>> #define y_scale yout_max / yin_max
>>>
>>> 	out_pixel(x, y) = in_pixel(x * x_scale, y * y_scale)
>>>
>>> would be simple enough to implement at the current image copy
>>> thread.
>>>
>>> Regards,
>>> Mauro
>>>   
>>>>  
>>>>>
>>>>> Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
>>>>> ---
>>>>>  drivers/media/platform/vim2m.c | 26 +++++++++++++++++++-------
>>>>>  1 file changed, 19 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/drivers/media/platform/vim2m.c b/drivers/media/platform/vim2m.c
>>>>> index 89384f324e25..46e3e096123e 100644
>>>>> --- a/drivers/media/platform/vim2m.c
>>>>> +++ b/drivers/media/platform/vim2m.c
>>>>> @@ -481,7 +481,9 @@ static int device_process(struct vim2m_ctx *ctx,
>>>>>  	struct vim2m_dev *dev = ctx->dev;
>>>>>  	struct vim2m_q_data *q_data_in, *q_data_out;
>>>>>  	u8 *p_in, *p, *p_out;
>>>>> -	int width, height, bytesperline, x, y, y_out, start, end, step;
>>>>> +	unsigned int width, height, bytesperline, bytesperline_out;
>>>>> +	unsigned int x, y, y_out;
>>>>> +	int start, end, step;
>>>>>  	struct vim2m_fmt *in, *out;
>>>>>  
>>>>>  	q_data_in = get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_OUTPUT);
>>>>> @@ -491,8 +493,15 @@ static int device_process(struct vim2m_ctx *ctx,
>>>>>  	bytesperline = (q_data_in->width * q_data_in->fmt->depth) >> 3;
>>>>>  
>>>>>  	q_data_out = get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE);
>>>>> +	bytesperline_out = (q_data_out->width * q_data_out->fmt->depth) >> 3;
>>>>>  	out = q_data_out->fmt;
>>>>>  
>>>>> +	/* Crop to the limits of the destination image */
>>>>> +	if (width > q_data_out->width)
>>>>> +		width = q_data_out->width;
>>>>> +	if (height > q_data_out->height)
>>>>> +		height = q_data_out->height;
>>>>> +
>>>>>  	p_in = vb2_plane_vaddr(&in_vb->vb2_buf, 0);
>>>>>  	p_out = vb2_plane_vaddr(&out_vb->vb2_buf, 0);
>>>>>  	if (!p_in || !p_out) {
>>>>> @@ -501,6 +510,10 @@ static int device_process(struct vim2m_ctx *ctx,
>>>>>  		return -EFAULT;
>>>>>  	}
>>>>>  
>>>>> +	/* Image size is different. Zero buffer first */
>>>>> +	if (q_data_in->width  != q_data_out->width ||
>>>>> +	    q_data_in->height != q_data_out->height)
>>>>> +		memset(p_out, 0, q_data_out->sizeimage);
>>>>>  	out_vb->sequence = get_q_data(ctx,
>>>>>  				      V4L2_BUF_TYPE_VIDEO_CAPTURE)->sequence++;
>>>>>  	in_vb->sequence = q_data_in->sequence++;
>>>>> @@ -524,6 +537,11 @@ static int device_process(struct vim2m_ctx *ctx,
>>>>>  		for (x = 0; x < width >> 1; x++)
>>>>>  			copy_two_pixels(in, out, &p, &p_out, y_out,
>>>>>  					ctx->mode & MEM2MEM_HFLIP);
>>>>> +
>>>>> +		/* Go to the next line at the out buffer*/    
>>>>
>>>> Add space after 'buffer'.
>>>>  
>>>>> +		if (width < q_data_out->width)
>>>>> +			p_out += ((q_data_out->width - width)
>>>>> +				  * q_data_out->fmt->depth) >> 3;
>>>>>  	}
>>>>>  
>>>>>  	return 0;
>>>>> @@ -977,12 +995,6 @@ static int vim2m_buf_prepare(struct vb2_buffer *vb)
>>>>>  	dprintk(ctx->dev, 2, "type: %s\n", type_name(vb->vb2_queue->type));
>>>>>  
>>>>>  	q_data = get_q_data(ctx, vb->vb2_queue->type);
>>>>> -	if (vb2_plane_size(vb, 0) < q_data->sizeimage) {
>>>>> -		dprintk(ctx->dev, "%s data will not fit into plane (%lu < %lu)\n",
>>>>> -				__func__, vb2_plane_size(vb, 0), (long)q_data->sizeimage);
>>>>> -		return -EINVAL;
>>>>> -	}
>>>>> -    
>>>>
>>>> As discussed on irc, this can't be removed. It checks if the provided buffer
>>>> is large enough for the current format.
>>>>
>>>> Regards,
>>>>
>>>> 	Hans
>>>>  
>>>>>  	vb2_set_plane_payload(vb, 0, q_data->sizeimage);
>>>>>  
>>>>>  	return 0;
>>>>>     
>>>>  
>>>
>>>
>>>
>>> Thanks,
>>> Mauro
>>>   
>>
> 
> 
> 
> Thanks,
> Mauro
> 

Re: [PATCH v2] media: vim2m: better handle cap/out buffers with different sizes

[Mauro Carvalho Chehab]

Em Thu, 28 Feb 2019 16:42:28 +0100
Hans Verkuil <hverkuil-cisco@xs4all.nl> escreveu:

> On 2/28/19 4:21 PM, Mauro Carvalho Chehab wrote:
> > Em Thu, 28 Feb 2019 15:35:07 +0100
> > Hans Verkuil <hverkuil-cisco@xs4all.nl> escreveu:
> >   
> >> On 2/28/19 3:09 PM, Mauro Carvalho Chehab wrote:  
> >>> Em Thu, 28 Feb 2019 13:30:49 +0100
> >>> Hans Verkuil <hverkuil-cisco@xs4all.nl> escreveu:
> >>>     
> >>>> On 2/26/19 6:36 PM, Mauro Carvalho Chehab wrote:    
> >>>>> The vim2m driver doesn't enforce that the capture and output
> >>>>> buffers would have the same size. Do the right thing if the
> >>>>> buffers are different, zeroing the buffer before writing,
> >>>>> ensuring that lines will be aligned and it won't write past
> >>>>> the buffer area.      
> >>>>
> >>>> I don't really like this. Since the vimc driver doesn't scale it shouldn't
> >>>> automatically crop either. If you want to crop/compose, then the
> >>>> selection API should be implemented.
> >>>>
> >>>> That would be the right approach to allowing capture and output
> >>>> formats (we're talking formats here, not buffer sizes) with
> >>>> different resolutions.    
> >>>
> >>> The original vim2m implementation assumes that this driver would
> >>> "scale" and do format conversions (except that it didn't do neither).    
> >>
> >> I'm not sure we should assume anything about the original implementation.
> >> It had lots of issues. I rather do this right then keep supporting hacks.  
> > 
> > True, but we are too close to the merge window. That's why I opted on
> > solving the bug, and not changing the behavior.  
> 
> Then that should be documented in the commit log: i.e. it is a temporary
> fix until we have a better solution. I'm fine with that.

Ok, I'll add it at version 3.

> 
> >   
> >>  
> >>> While I fixed the format conversion on a past patchset, vim2m
> >>> still allows a "free" image size on both sides of the pipeline.
> >>>
> >>> I agree with you that the best would be to implement a scaler
> >>> (and maybe crop/compose), but for now, we need to solve an issue that
> >>> vim2m is doing a very poor job to confine the image at the destination
> >>> buffer's resolution.
> >>>
> >>> Also, as far as I remember, the first M2M devices have scalers, so
> >>> existing apps likely assume that such devices will do scaling.    
> >>
> >> Most m2m devices are codecs and codecs do not do scaling (at least,
> >> I'm not aware of any).  
> > 
> > At least on GStreamer, codecs are implemented on a separate logic.
> > GStreamer checks it using the FOURCC types. If one of the sides is
> > compressed, it is either an encoder or a decoder. Otherwise, it is
> > a transform device.
> > 
> > From what I understood from the code (and from my tests), it
> > assumes that scaler is supported for transform devices.  
> 
> I find it hard to believe that it assumes a transform device can always
> scale. Nicolas?

Provided that you have gst-plugins-good compiled with --enable-v4l2-probe
(needed in order to enable M2M support), you can easily test it with:

$ gst-launch-1.0 videotestsrc num-buffers=120 ! video/x-raw,format=RGB,width=322,height=200 ! v4l2convert disable-passthrough=1 ! video/x-raw,width=428,height=400 ! videoconvert ! xvimagesink

At dmesg, you'll see:

[246685.031750] vim2m vim2m.0: vidioc_s_fmt: Format for type Output: 322x200 (24 bpp), fmt: RGB3
[246685.032300] vim2m vim2m.0: vidioc_s_fmt: Format for type Capture: 428x400 (24 bpp), fmt: RGB3

Note: that's the Gstreamer 1.5.x syntax. "v4l2convert" has a different
name on version 1.4.x.. There, you should use "v4l2video0convert" instead:

$ gst-launch-1.0 videotestsrc num-buffers=120 ! video/x-raw,format=RGB,width=322,height=200 ! v4l2video0convert disable-passthrough=1 ! video/x-raw,width=428,height=400 ! videoconvert ! xvimagesink

> 
> >   
> >> I don't think there are many m2m devices that do scaling: most do
> >> format conversion of one type or another (codecs, deinterlacers,
> >> yuv-rgb converters). Scalers tend to be integrated into a video
> >> pipeline. The only m2m drivers I found that also scale are
> >> exynos-gsc, ti-vpe, mtk-mdp and possibly sti/bdisp.  
> > 
> > Well, the M2M API was added with Exynos. 
> >   
> >>>
> >>> So, a non-scaling M2M device is something that, in thesis, we don't
> >>> support[1].
> >>>
> >>> [1] Very likely we have several ones that don't do scaling, but the
> >>> needed API bits for apps to detect if scaling is supported or not
> >>> aren't implemented.
> >>>
> >>> The problem of enforcing the resolution to be identical on both capture
> >>> and output buffers is that the V4L2 API doesn't really have
> >>> a way for userspace apps to identify that it won't scale until
> >>> too late.
> >>>
> >>> How do you imagine an application negotiating the image resolution?
> >>>
> >>> I mean, application A may set first the capture buffer to, let's say,
> >>> 320x200 and then try to set the output buffer. 
> >>>
> >>> Application B may do the reverse, e. g. set first the output buffer
> >>> to 320x200 and then try to set the capture buffer.
> >>>
> >>> Application C could try to initialize with some default for both 
> >>> capture and output buffers and only later decide what resolution
> >>> it wants and try to set both sides.
> >>>
> >>> Application D could have setup both sides, started streaming at 
> >>> 320x200. Then, it stopped streaming and changed the capture 
> >>> resolution to, let's say 640x480, without changing the resolution
> >>> of the output buffer.
> >>>
> >>> For all the above scenarios, the app may either first set both
> >>> sides and then request the buffer for both, or do S_FMT/REQBUFS
> >>> for one side and then to the other side.
> >>>
> >>> What I mean is that, wit just use the existing ioctls and flags, I 
> >>> can't see any way for all the above scenarios work on devices that
> >>> don't scale.    
> >>
> >> If the device cannot scale, then setting the format on either capture
> >> or output will modify the format on the other side as well.  
> > 
> > That's one way to handle it, but what happens if buffers were already
> > allocated at one side?  
> 
> Then trying to change the format on the other side will simply return
> the current format.

That's one way to handle that, but it somewhat violates the V4L2 API,
as this kind of thing was never enforced.

> 
>  If the buffer is USERPTR, this is even worse,
> > as the size may not fit the image anymore.  
> 
> That's why you shouldn't remove the check in buf_prepare :-)

Yeah, sure.

> 
> > 
> > Also, changing drivers to this new behavior could break some stuff.  
> 
> I'm not at all certain if all m2m drivers are safe when in comes to this.
> The v4l2-compliance testing for m2m devices has always lagged behind the
> testing for non-m2m devices, so we don't do in-depth tests. Otherwise we'd
> found this vim2m issue a long time ago.

Yes. Btw, this kind of test is a good candidate for v4l2-compliance,
once we define the proper behavior.

> 
> And I don't think it is new behavior. When it comes to m2m devices
> they just behave according to the standard v4l2 behavior: the driver
> always tries to fulfill the last ioctl to the best of its ability.

Yes, but in this case, it violates a basic concept: it is overriding
the resolution that was set before for a separate part of the pipeline.

I doubt any application would expect this kind of behavior and re-check.

I'm not saying I'm against it, but it doesn't look right to do that on
my eyes. Yet, not sure what other options we would have.

> That said, this has never been made explicit in the spec.

Well, we need to make it clear, and a way for userspace to know if
it will scale or just override the last setting.

> > (with this particular matter, changing vim2m code doesn't count as a
> > change, as this driver should not be used in production - but if any
> > other driver is doing something different, then we're limited to do
> > such change)
> >   
> >>
> >> If the device also support cropping and composing, then it becomes
> >> more complicated, but the basics remain the same.
> >>  
> > 
> > I suspect that the above are just assumptions (and perhaps the current
> > implementation on most drivers). At least, I was unable to find any mention
> > about the M2M chapter at the V4L2 specs.
> >   
> >> It would certainly be nice if there was a scaling capability. I suspect
> >> one reason that nobody requested this is that you usually know what
> >> you are doing when you use an m2m device.  
> > 
> > And that's a bad assumption, as it prevents having generic apps
> > using it. The expected behavior for having and not having scaler
> > should be described, and we need to be able to cope with legacy stuff.  
> 
> We should likely write up something similar as what we are doing for
> the codec documentation, but then for m2m transform devices.

Yes.

> 
> >   
> >>  
> >>> One solution would be to filter the output of ENUM_FMT, TRY_FMT,
> >>> G_FMT and S_FMT when one of the sides of the M2M buffer is set,
> >>> but that would break some possible real usecases.    
> >>
> >> Not sure what you mean here.  
> > 
> > I mean that one possible solution would be that, if one side sets 
> > resolution, the answer for the ioctls on the other side would be
> > different. IMHO, that's a bad idea.
> >   
> >>>
> >>> I suspect that the option that it would work best is to have a
> >>> flag to indicate that a M2M device has scalers.
> >>>
> >>> In any case, this should be discussed and properly documented
> >>> before we would be able to implement a non-scaling M2M device.    
> >>
> >> I don't know where you get the idea that most m2m devices scale.
> >> The reverse is true, very few m2m devices have a scaler.  
> > 
> > No, I didn't say that most m2m devices scale. I said that the initial
> > M2M implementations scale, and that's probably one of the reasons why 
> > this is the behavior that Gstreamer expects scales on transform devices.  
> 
> Well, it doesn't really matter. The fact is that gstreamer apparently
> makes assumptions that are not in general valid.
> 
> And all this is made worse by insufficiently detailed documentation and
> insufficient compliance testing.

Well, any change now would break an existing userspace app, and this is
something we shouldn't do. Worse than that, I suspect that Gstreamer is
actually used on several custom platform-dependent media applications.

> Patches are welcome.

I know. Need to think a little bit more about what would be the best
solution for it though. We have very few space at VIDIOC_QUERYCAP, and
I'm not sure if a "HAVE_SCALER" flag would belong there.

> Regarding this vim2m patch: I'm fine with this patch if it is clear that
> it is a temporary fix and that it should really implement a real scaler
> instead of cropping.

OK!

Btw, I found another problem that version 3 will also fix: the 
"fast track" copy (used when no conversion is needed) is also broken
if resolutions are different. While it could likely be solved, for
now, I'll just default to the slower copy logic.

Such change will also make easier once we add a scaler there.

Thanks,
Mauro

Re: [PATCH v2] media: vim2m: better handle cap/out buffers with different sizes

[Hans Verkuil]

On 2/28/19 6:31 PM, Mauro Carvalho Chehab wrote:
> Em Thu, 28 Feb 2019 16:42:28 +0100
> Hans Verkuil <hverkuil-cisco@xs4all.nl> escreveu:
> 
>> On 2/28/19 4:21 PM, Mauro Carvalho Chehab wrote:
>>> Em Thu, 28 Feb 2019 15:35:07 +0100
>>> Hans Verkuil <hverkuil-cisco@xs4all.nl> escreveu:
>>>   
>>>> On 2/28/19 3:09 PM, Mauro Carvalho Chehab wrote:  
>>>>> Em Thu, 28 Feb 2019 13:30:49 +0100
>>>>> Hans Verkuil <hverkuil-cisco@xs4all.nl> escreveu:
>>>>>     
>>>>>> On 2/26/19 6:36 PM, Mauro Carvalho Chehab wrote:    
>>>>>>> The vim2m driver doesn't enforce that the capture and output
>>>>>>> buffers would have the same size. Do the right thing if the
>>>>>>> buffers are different, zeroing the buffer before writing,
>>>>>>> ensuring that lines will be aligned and it won't write past
>>>>>>> the buffer area.      
>>>>>>
>>>>>> I don't really like this. Since the vimc driver doesn't scale it shouldn't
>>>>>> automatically crop either. If you want to crop/compose, then the
>>>>>> selection API should be implemented.
>>>>>>
>>>>>> That would be the right approach to allowing capture and output
>>>>>> formats (we're talking formats here, not buffer sizes) with
>>>>>> different resolutions.    
>>>>>
>>>>> The original vim2m implementation assumes that this driver would
>>>>> "scale" and do format conversions (except that it didn't do neither).    
>>>>
>>>> I'm not sure we should assume anything about the original implementation.
>>>> It had lots of issues. I rather do this right then keep supporting hacks.  
>>>
>>> True, but we are too close to the merge window. That's why I opted on
>>> solving the bug, and not changing the behavior.  
>>
>> Then that should be documented in the commit log: i.e. it is a temporary
>> fix until we have a better solution. I'm fine with that.
> 
> Ok, I'll add it at version 3.
> 
>>
>>>   
>>>>  
>>>>> While I fixed the format conversion on a past patchset, vim2m
>>>>> still allows a "free" image size on both sides of the pipeline.
>>>>>
>>>>> I agree with you that the best would be to implement a scaler
>>>>> (and maybe crop/compose), but for now, we need to solve an issue that
>>>>> vim2m is doing a very poor job to confine the image at the destination
>>>>> buffer's resolution.
>>>>>
>>>>> Also, as far as I remember, the first M2M devices have scalers, so
>>>>> existing apps likely assume that such devices will do scaling.    
>>>>
>>>> Most m2m devices are codecs and codecs do not do scaling (at least,
>>>> I'm not aware of any).  
>>>
>>> At least on GStreamer, codecs are implemented on a separate logic.
>>> GStreamer checks it using the FOURCC types. If one of the sides is
>>> compressed, it is either an encoder or a decoder. Otherwise, it is
>>> a transform device.
>>>
>>> From what I understood from the code (and from my tests), it
>>> assumes that scaler is supported for transform devices.  
>>
>> I find it hard to believe that it assumes a transform device can always
>> scale. Nicolas?
> 
> Provided that you have gst-plugins-good compiled with --enable-v4l2-probe
> (needed in order to enable M2M support), you can easily test it with:
> 
> $ gst-launch-1.0 videotestsrc num-buffers=120 ! video/x-raw,format=RGB,width=322,height=200 ! v4l2convert disable-passthrough=1 ! video/x-raw,width=428,height=400 ! videoconvert ! xvimagesink
> 
> At dmesg, you'll see:
> 
> [246685.031750] vim2m vim2m.0: vidioc_s_fmt: Format for type Output: 322x200 (24 bpp), fmt: RGB3
> [246685.032300] vim2m vim2m.0: vidioc_s_fmt: Format for type Capture: 428x400 (24 bpp), fmt: RGB3
> 
> Note: that's the Gstreamer 1.5.x syntax. "v4l2convert" has a different
> name on version 1.4.x.. There, you should use "v4l2video0convert" instead:
> 
> $ gst-launch-1.0 videotestsrc num-buffers=120 ! video/x-raw,format=RGB,width=322,height=200 ! v4l2video0convert disable-passthrough=1 ! video/x-raw,width=428,height=400 ! videoconvert ! xvimagesink

But you explicitly set different resolutions for source and sink.
What happens in gstreamer if the driver doesn't support scaling? Will
gstreamer continue with wrong values? Give an error?

> 
>>
>>>   
>>>> I don't think there are many m2m devices that do scaling: most do
>>>> format conversion of one type or another (codecs, deinterlacers,
>>>> yuv-rgb converters). Scalers tend to be integrated into a video
>>>> pipeline. The only m2m drivers I found that also scale are
>>>> exynos-gsc, ti-vpe, mtk-mdp and possibly sti/bdisp.  
>>>
>>> Well, the M2M API was added with Exynos. 
>>>   
>>>>>
>>>>> So, a non-scaling M2M device is something that, in thesis, we don't
>>>>> support[1].
>>>>>
>>>>> [1] Very likely we have several ones that don't do scaling, but the
>>>>> needed API bits for apps to detect if scaling is supported or not
>>>>> aren't implemented.
>>>>>
>>>>> The problem of enforcing the resolution to be identical on both capture
>>>>> and output buffers is that the V4L2 API doesn't really have
>>>>> a way for userspace apps to identify that it won't scale until
>>>>> too late.
>>>>>
>>>>> How do you imagine an application negotiating the image resolution?
>>>>>
>>>>> I mean, application A may set first the capture buffer to, let's say,
>>>>> 320x200 and then try to set the output buffer. 
>>>>>
>>>>> Application B may do the reverse, e. g. set first the output buffer
>>>>> to 320x200 and then try to set the capture buffer.
>>>>>
>>>>> Application C could try to initialize with some default for both 
>>>>> capture and output buffers and only later decide what resolution
>>>>> it wants and try to set both sides.
>>>>>
>>>>> Application D could have setup both sides, started streaming at 
>>>>> 320x200. Then, it stopped streaming and changed the capture 
>>>>> resolution to, let's say 640x480, without changing the resolution
>>>>> of the output buffer.
>>>>>
>>>>> For all the above scenarios, the app may either first set both
>>>>> sides and then request the buffer for both, or do S_FMT/REQBUFS
>>>>> for one side and then to the other side.
>>>>>
>>>>> What I mean is that, wit just use the existing ioctls and flags, I 
>>>>> can't see any way for all the above scenarios work on devices that
>>>>> don't scale.    
>>>>
>>>> If the device cannot scale, then setting the format on either capture
>>>> or output will modify the format on the other side as well.  
>>>
>>> That's one way to handle it, but what happens if buffers were already
>>> allocated at one side?  
>>
>> Then trying to change the format on the other side will simply return
>> the current format.
> 
> That's one way to handle that, but it somewhat violates the V4L2 API,
> as this kind of thing was never enforced.

I don't see how this violates the spec. There is never any guarantee that
the format that you set is also the format that it returned.

> 
>>
>>  If the buffer is USERPTR, this is even worse,
>>> as the size may not fit the image anymore.  
>>
>> That's why you shouldn't remove the check in buf_prepare :-)
> 
> Yeah, sure.
> 
>>
>>>
>>> Also, changing drivers to this new behavior could break some stuff.  
>>
>> I'm not at all certain if all m2m drivers are safe when in comes to this.
>> The v4l2-compliance testing for m2m devices has always lagged behind the
>> testing for non-m2m devices, so we don't do in-depth tests. Otherwise we'd
>> found this vim2m issue a long time ago.
> 
> Yes. Btw, this kind of test is a good candidate for v4l2-compliance,
> once we define the proper behavior.
> 
>>
>> And I don't think it is new behavior. When it comes to m2m devices
>> they just behave according to the standard v4l2 behavior: the driver
>> always tries to fulfill the last ioctl to the best of its ability.
> 
> Yes, but in this case, it violates a basic concept: it is overriding
> the resolution that was set before for a separate part of the pipeline.
> 
> I doubt any application would expect this kind of behavior and re-check.
> 
> I'm not saying I'm against it, but it doesn't look right to do that on
> my eyes. Yet, not sure what other options we would have.

I went through the spec to see where we need to improve the documentation.

There are several levels of ioctls that can influence others:

S_INPUT/OUTPUT: these should come first since changing input or output
can reset everything else.

This is clearly stated for ioctls.

Next are S_STD and S_DV_TIMINGS: these can change the format and any
cropping/composing rectangles. Neither makes any mention of this.

This should be fixed.

Note that the Data Formats section (1.22) makes implicit mention of this
("A video standard change ... can invalidate the selected image format.").
But that isn't quite right: it doesn't invalidate it, it only changes it.
As an aside, there are more issues with this section since it says that
setting the format 'assigns a logical stream exclusively to one file
descriptor', but that's wrong. It is when buffers are allocated that this
happens.

S_FMT and S_SELECTION (and S_CROP): sadly only S_CROP makes any mention
that changing the crop rectangle can also change the format.

In practice changing for the format can change any existing crop/compose
rectangles and vice versa. This is so dependent on hardware constraints
that it is hard to say anything specific.

The general rule is that the driver will do its best to satisfy the
request. It's mentioned in section 1.22.1: "Negotiation means the
application asks for a particular format and the driver selects and
reports the best the hardware can do to satisfy the request."

Unfortunately it is not clear from the text that crop/compose is part
of the negotiation and follows the same rule.

> 
>> That said, this has never been made explicit in the spec.
> 
> Well, we need to make it clear, and a way for userspace to know if
> it will scale or just override the last setting.
> 
>>> (with this particular matter, changing vim2m code doesn't count as a
>>> change, as this driver should not be used in production - but if any
>>> other driver is doing something different, then we're limited to do
>>> such change)
>>>   
>>>>
>>>> If the device also support cropping and composing, then it becomes
>>>> more complicated, but the basics remain the same.
>>>>  
>>>
>>> I suspect that the above are just assumptions (and perhaps the current
>>> implementation on most drivers). At least, I was unable to find any mention
>>> about the M2M chapter at the V4L2 specs.
>>>   
>>>> It would certainly be nice if there was a scaling capability. I suspect
>>>> one reason that nobody requested this is that you usually know what
>>>> you are doing when you use an m2m device.  
>>>
>>> And that's a bad assumption, as it prevents having generic apps
>>> using it. The expected behavior for having and not having scaler
>>> should be described, and we need to be able to cope with legacy stuff.  
>>
>> We should likely write up something similar as what we are doing for
>> the codec documentation, but then for m2m transform devices.
> 
> Yes.
> 
>>
>>>   
>>>>  
>>>>> One solution would be to filter the output of ENUM_FMT, TRY_FMT,
>>>>> G_FMT and S_FMT when one of the sides of the M2M buffer is set,
>>>>> but that would break some possible real usecases.    
>>>>
>>>> Not sure what you mean here.  
>>>
>>> I mean that one possible solution would be that, if one side sets 
>>> resolution, the answer for the ioctls on the other side would be
>>> different. IMHO, that's a bad idea.
>>>   
>>>>>
>>>>> I suspect that the option that it would work best is to have a
>>>>> flag to indicate that a M2M device has scalers.
>>>>>
>>>>> In any case, this should be discussed and properly documented
>>>>> before we would be able to implement a non-scaling M2M device.    
>>>>
>>>> I don't know where you get the idea that most m2m devices scale.
>>>> The reverse is true, very few m2m devices have a scaler.  
>>>
>>> No, I didn't say that most m2m devices scale. I said that the initial
>>> M2M implementations scale, and that's probably one of the reasons why 
>>> this is the behavior that Gstreamer expects scales on transform devices.  
>>
>> Well, it doesn't really matter. The fact is that gstreamer apparently
>> makes assumptions that are not in general valid.
>>
>> And all this is made worse by insufficiently detailed documentation and
>> insufficient compliance testing.
> 
> Well, any change now would break an existing userspace app, and this is
> something we shouldn't do. Worse than that, I suspect that Gstreamer is
> actually used on several custom platform-dependent media applications.
> 
>> Patches are welcome.
> 
> I know. Need to think a little bit more about what would be the best
> solution for it though. We have very few space at VIDIOC_QUERYCAP, and
> I'm not sure if a "HAVE_SCALER" flag would belong there.

We have a u32 reserved[3] for QUERYCAP, so we have room for many more
capabilities.

What I would like to see are these flags:

V4L2_CAP_HAS_DEVICE_CAPS2: indicate that there is a device_caps2 capability
field.

And add these defines for the device_caps2 field:

V4L2_CAP2_VIDEO_CAPTURE_HAS_CROP
V4L2_CAP2_VIDEO_CAPTURE_HAS_COMPOSE
V4L2_CAP2_VIDEO_CAPTURE_HAS_SCALER
V4L2_CAP2_VIDEO_OUTPUT_HAS_CROP
V4L2_CAP2_VIDEO_OUTPUT_HAS_COMPOSE
V4L2_CAP2_VIDEO_OUTPUT_HAS_SCALER
V4L2_CAP2_VIDEO_M2M_HAS_SCALER

The main problem is what to do with drivers that do not explicitly set
these capabilities. Applications need to know whether the driver never
sets the capabilities, or that it is not capable of these features.

Perhaps the best way is to add these caps:

V4L2_CAP2_VIDEO_CAPTURE_FIXED
V4L2_CAP2_VIDEO_OUTPUT_FIXED
V4L2_CAP2_VIDEO_M2M_FIXED

that are set when the driver is incapable of cropping/composing/scaling.

If all bits are 0, then you don't know and you have to test for these
features (i.e. the current situation). You want to have a nice macro
to test this.

v4l2-compliance can easily use these caps to verify if the driver isn't
lying to the user, so that is a nice bonus.

What do you think about this?

I can make an RFC PATCH if you think this is the right direction.

Regards,

	Hans

> 
>> Regarding this vim2m patch: I'm fine with this patch if it is clear that
>> it is a temporary fix and that it should really implement a real scaler
>> instead of cropping.
> 
> OK!
> 
> Btw, I found another problem that version 3 will also fix: the 
> "fast track" copy (used when no conversion is needed) is also broken
> if resolutions are different. While it could likely be solved, for
> now, I'll just default to the slower copy logic.
> 
> Such change will also make easier once we add a scaler there.
> 
> Thanks,
> Mauro
> 

Re: [PATCH v2] media: vim2m: better handle cap/out buffers with different sizes

[Nicolas Dufresne]

Le vendredi 01 mars 2019 à 11:19 +0100, Hans Verkuil a écrit :
> > $ gst-launch-1.0 videotestsrc num-buffers=120 ! video/x-raw,format=RGB,width=322,height=200 ! v4l2video0convert disable-passthrough=1 ! video/x-raw,width=428,height=400 ! videoconvert ! xvimagesink
> 
> But you explicitly set different resolutions for source and sink.
> What happens in gstreamer if the driver doesn't support scaling? Will
> gstreamer continue with wrong values? Give an error?

It will post an error or type NOT_NEGOTIATED after failing to set the
OUTPUT or CAPTURE format. It does assume your converter driver do
scaling though, the caps negotiation is written in a way that it will
first try to avoid it. So if it's unavoidable, and your driver does not
support scaling, it's also unavoidable that negotiation will fail.

In the case of this test pipeline, you cannot test scaling without
forcing the width/height on both side of it. Because videotestsrc can
produce frames at any raw video format mapped into GStreamer. Then
xvimagesink does scaling, so it would accept any given videotestsrc
resolution without the need for scaling.

Nicolas

Re: [PATCH v2] media: vim2m: better handle cap/out buffers with different sizes

[Mauro Carvalho Chehab]

Em Fri, 1 Mar 2019 11:19:06 +0100
Hans Verkuil <hverkuil-cisco@xs4all.nl> escreveu:

> >>> At least on GStreamer, codecs are implemented on a separate logic.
> >>> GStreamer checks it using the FOURCC types. If one of the sides is
> >>> compressed, it is either an encoder or a decoder. Otherwise, it is
> >>> a transform device.
> >>>
> >>> From what I understood from the code (and from my tests), it
> >>> assumes that scaler is supported for transform devices.    
> >>
> >> I find it hard to believe that it assumes a transform device can always
> >> scale. Nicolas?  
> > 
> > Provided that you have gst-plugins-good compiled with --enable-v4l2-probe
> > (needed in order to enable M2M support), you can easily test it with:
> > 
> > $ gst-launch-1.0 videotestsrc num-buffers=120 ! video/x-raw,format=RGB,width=322,height=200 ! v4l2convert disable-passthrough=1 ! video/x-raw,width=428,height=400 ! videoconvert ! xvimagesink
> > 
> > At dmesg, you'll see:
> > 
> > [246685.031750] vim2m vim2m.0: vidioc_s_fmt: Format for type Output: 322x200 (24 bpp), fmt: RGB3
> > [246685.032300] vim2m vim2m.0: vidioc_s_fmt: Format for type Capture: 428x400 (24 bpp), fmt: RGB3
> > 
> > Note: that's the Gstreamer 1.5.x syntax. "v4l2convert" has a different
> > name on version 1.4.x.. There, you should use "v4l2video0convert" instead:
> > 
> > $ gst-launch-1.0 videotestsrc num-buffers=120 ! video/x-raw,format=RGB,width=322,height=200 ! v4l2video0convert disable-passthrough=1 ! video/x-raw,width=428,height=400 ! videoconvert ! xvimagesink  
> 
> But you explicitly set different resolutions for source and sink.

Yes. That's the Gstreamer's way to select the resolution.

> What happens in gstreamer if the driver doesn't support scaling? Will
> gstreamer continue with wrong values? Give an error?

See Nicolas e-mail.

> 
> >   
> >>  
> >>>     
> >>>> I don't think there are many m2m devices that do scaling: most do
> >>>> format conversion of one type or another (codecs, deinterlacers,
> >>>> yuv-rgb converters). Scalers tend to be integrated into a video
> >>>> pipeline. The only m2m drivers I found that also scale are
> >>>> exynos-gsc, ti-vpe, mtk-mdp and possibly sti/bdisp.    
> >>>
> >>> Well, the M2M API was added with Exynos. 
> >>>     
> >>>>>
> >>>>> So, a non-scaling M2M device is something that, in thesis, we don't
> >>>>> support[1].
> >>>>>
> >>>>> [1] Very likely we have several ones that don't do scaling, but the
> >>>>> needed API bits for apps to detect if scaling is supported or not
> >>>>> aren't implemented.
> >>>>>
> >>>>> The problem of enforcing the resolution to be identical on both capture
> >>>>> and output buffers is that the V4L2 API doesn't really have
> >>>>> a way for userspace apps to identify that it won't scale until
> >>>>> too late.
> >>>>>
> >>>>> How do you imagine an application negotiating the image resolution?
> >>>>>
> >>>>> I mean, application A may set first the capture buffer to, let's say,
> >>>>> 320x200 and then try to set the output buffer. 
> >>>>>
> >>>>> Application B may do the reverse, e. g. set first the output buffer
> >>>>> to 320x200 and then try to set the capture buffer.
> >>>>>
> >>>>> Application C could try to initialize with some default for both 
> >>>>> capture and output buffers and only later decide what resolution
> >>>>> it wants and try to set both sides.
> >>>>>
> >>>>> Application D could have setup both sides, started streaming at 
> >>>>> 320x200. Then, it stopped streaming and changed the capture 
> >>>>> resolution to, let's say 640x480, without changing the resolution
> >>>>> of the output buffer.
> >>>>>
> >>>>> For all the above scenarios, the app may either first set both
> >>>>> sides and then request the buffer for both, or do S_FMT/REQBUFS
> >>>>> for one side and then to the other side.
> >>>>>
> >>>>> What I mean is that, wit just use the existing ioctls and flags, I 
> >>>>> can't see any way for all the above scenarios work on devices that
> >>>>> don't scale.      
> >>>>
> >>>> If the device cannot scale, then setting the format on either capture
> >>>> or output will modify the format on the other side as well.    
> >>>
> >>> That's one way to handle it, but what happens if buffers were already
> >>> allocated at one side?    
> >>
> >> Then trying to change the format on the other side will simply return
> >> the current format.  
> > 
> > That's one way to handle that, but it somewhat violates the V4L2 API,
> > as this kind of thing was never enforced.  
> 
> I don't see how this violates the spec. There is never any guarantee that
> the format that you set is also the format that it returned.

No, but there was always a guarantee that the format returned by S_FMT
would be the one the driver will use. In other words, setting the
capture's format should be independent of setting the output format.

So, if one sets capture to RGB and then sets output to YUYV, a set
to the output should not change the capture to RGB. While this is
not really explicit at the API, it is a contra-sense to let a S_FMT
to one side to silently affect the other side's resolution.

> >   
> >>
> >>  If the buffer is USERPTR, this is even worse,  
> >>> as the size may not fit the image anymore.    
> >>
> >> That's why you shouldn't remove the check in buf_prepare :-)  
> > 
> > Yeah, sure.
> >   
> >>  
> >>>
> >>> Also, changing drivers to this new behavior could break some stuff.    
> >>
> >> I'm not at all certain if all m2m drivers are safe when in comes to this.
> >> The v4l2-compliance testing for m2m devices has always lagged behind the
> >> testing for non-m2m devices, so we don't do in-depth tests. Otherwise we'd
> >> found this vim2m issue a long time ago.  
> > 
> > Yes. Btw, this kind of test is a good candidate for v4l2-compliance,
> > once we define the proper behavior.
> >   
> >>
> >> And I don't think it is new behavior. When it comes to m2m devices
> >> they just behave according to the standard v4l2 behavior: the driver
> >> always tries to fulfill the last ioctl to the best of its ability.  
> > 
> > Yes, but in this case, it violates a basic concept: it is overriding
> > the resolution that was set before for a separate part of the pipeline.
> > 
> > I doubt any application would expect this kind of behavior and re-check.
> > 
> > I'm not saying I'm against it, but it doesn't look right to do that on
> > my eyes. Yet, not sure what other options we would have.  
> 
> I went through the spec to see where we need to improve the documentation.
> 
> There are several levels of ioctls that can influence others:
> 
> S_INPUT/OUTPUT: these should come first since changing input or output
> can reset everything else.
> 
> This is clearly stated for ioctls.
> 
> Next are S_STD and S_DV_TIMINGS: these can change the format and any
> cropping/composing rectangles. Neither makes any mention of this.
> 
> This should be fixed.

I think that we should have a chapter mentioning the driver's expected
order. Yet, we should take care to avoid breaking existing apps.

> 
> Note that the Data Formats section (1.22) makes implicit mention of this
> ("A video standard change ... can invalidate the selected image format.").
> But that isn't quite right: it doesn't invalidate it, it only changes it.
> As an aside, there are more issues with this section since it says that
> setting the format 'assigns a logical stream exclusively to one file
> descriptor', but that's wrong. It is when buffers are allocated that this
> happens.
> 
> S_FMT and S_SELECTION (and S_CROP): sadly only S_CROP makes any mention
> that changing the crop rectangle can also change the format.

What are you meaning by format? fourcc or resolution.

IMO, it doesn't make any sense to change fourcc after crop.

Are there any drivers doing that after S_CROP/S_SELECTION?
What driver? Why?

Are out there any drivers that allow crop only on certain fourcc?

-

Reducing the resolution after CROP makes sense, though. As far as I
remember, CROP was added before USB drivers. On that time, its main goal
were to adjust the visible area to remove the VBI lines and any black
lines/columns.

So, IMO, it makes sense to explicitly mention that S_CROP/S_SELECTION
should guarantee that fourcc won't be touched. Yet, applications 
should call G_FMT after S_CROP/S_SELECTION, as the resolution could
be affected.

> In practice changing for the format can change any existing crop/compose
> rectangles and vice versa. This is so dependent on hardware constraints
> that it is hard to say anything specific.

There are two separate issues here:

1) I don't see why this would depend on the hardware. I mean, if changing
one register (with resolution or crop) would reset the other one affecting
fourcc, it should be up to the driver to rewrite the affected register
to a value that would ensure that the previous set would be honored.

2) IMO, S_FMT should always reset the crop/selection. If app decided
for a change, it very likely expect that crop/selection would return
to its default.

We should make both explicit at the documentation.

> The general rule is that the driver will do its best to satisfy the
> request. It's mentioned in section 1.22.1: "Negotiation means the
> application asks for a particular format and the driver selects and
> reports the best the hardware can do to satisfy the request."

For me, that applies only for the ioctls that are designed to change
the format (mainly S_FMT, but S_STD and S_DV_TIMINGS may also do that).
S_CROP (and its new variant S_SELECTION) are designed to affect
the captured area (and resolution). It should preserve the fourcc
or return an error if the driver cannot work with the previously
selected fourcc.

> Unfortunately it is not clear from the text that crop/compose is part
> of the negotiation and follows the same rule.

We should explicitly mention what ioctls may affect the resolution
and would require a G_FMT.

> 
> >   
> >> That said, this has never been made explicit in the spec.  
> > 
> > Well, we need to make it clear, and a way for userspace to know if
> > it will scale or just override the last setting.
> >   
> >>> (with this particular matter, changing vim2m code doesn't count as a
> >>> change, as this driver should not be used in production - but if any
> >>> other driver is doing something different, then we're limited to do
> >>> such change)
> >>>     
> >>>>
> >>>> If the device also support cropping and composing, then it becomes
> >>>> more complicated, but the basics remain the same.
> >>>>    
> >>>
> >>> I suspect that the above are just assumptions (and perhaps the current
> >>> implementation on most drivers). At least, I was unable to find any mention
> >>> about the M2M chapter at the V4L2 specs.
> >>>     
> >>>> It would certainly be nice if there was a scaling capability. I suspect
> >>>> one reason that nobody requested this is that you usually know what
> >>>> you are doing when you use an m2m device.    
> >>>
> >>> And that's a bad assumption, as it prevents having generic apps
> >>> using it. The expected behavior for having and not having scaler
> >>> should be described, and we need to be able to cope with legacy stuff.    
> >>
> >> We should likely write up something similar as what we are doing for
> >> the codec documentation, but then for m2m transform devices.  
> > 
> > Yes.
> >   
> >>  
> >>>     
> >>>>    
> >>>>> One solution would be to filter the output of ENUM_FMT, TRY_FMT,
> >>>>> G_FMT and S_FMT when one of the sides of the M2M buffer is set,
> >>>>> but that would break some possible real usecases.      
> >>>>
> >>>> Not sure what you mean here.    
> >>>
> >>> I mean that one possible solution would be that, if one side sets 
> >>> resolution, the answer for the ioctls on the other side would be
> >>> different. IMHO, that's a bad idea.
> >>>     
> >>>>>
> >>>>> I suspect that the option that it would work best is to have a
> >>>>> flag to indicate that a M2M device has scalers.
> >>>>>
> >>>>> In any case, this should be discussed and properly documented
> >>>>> before we would be able to implement a non-scaling M2M device.      
> >>>>
> >>>> I don't know where you get the idea that most m2m devices scale.
> >>>> The reverse is true, very few m2m devices have a scaler.    
> >>>
> >>> No, I didn't say that most m2m devices scale. I said that the initial
> >>> M2M implementations scale, and that's probably one of the reasons why 
> >>> this is the behavior that Gstreamer expects scales on transform devices.    
> >>
> >> Well, it doesn't really matter. The fact is that gstreamer apparently
> >> makes assumptions that are not in general valid.
> >>
> >> And all this is made worse by insufficiently detailed documentation and
> >> insufficient compliance testing.  
> > 
> > Well, any change now would break an existing userspace app, and this is
> > something we shouldn't do. Worse than that, I suspect that Gstreamer is
> > actually used on several custom platform-dependent media applications.
> >   
> >> Patches are welcome.  
> > 
> > I know. Need to think a little bit more about what would be the best
> > solution for it though. We have very few space at VIDIOC_QUERYCAP, and
> > I'm not sure if a "HAVE_SCALER" flag would belong there.  
> 
> We have a u32 reserved[3] for QUERYCAP, so we have room for many more
> capabilities.
> 
> What I would like to see are these flags:
> 
> V4L2_CAP_HAS_DEVICE_CAPS2: indicate that there is a device_caps2 capability
> field.

makes sense, except that _CAPS2 is a bad name ;-) EXTENDED_CAPS?

Or if we end by adding such caps per format (see below) name it as
V4L2_CAP_HAS_PER_FMT_CAPS.

> 
> And add these defines for the device_caps2 field:
> 
> V4L2_CAP2_VIDEO_CAPTURE_HAS_CROP
> V4L2_CAP2_VIDEO_CAPTURE_HAS_COMPOSE
> V4L2_CAP2_VIDEO_CAPTURE_HAS_SCALER

Makes sense.

> V4L2_CAP2_VIDEO_OUTPUT_HAS_CROP
> V4L2_CAP2_VIDEO_OUTPUT_HAS_COMPOSE
> V4L2_CAP2_VIDEO_OUTPUT_HAS_SCALER

Not so so sure about that. At the moment we start needing to put
multiple caps, it sounds that this flag should be per interface.
See below.

> V4L2_CAP2_VIDEO_M2M_HAS_SCALER

IMHO, We don't need this. As far as I can see, on a M2M driver, scaler
affects the capture buffer.

Btw, a M2M device with crop/composite in thesis could have it
either on capture or output (or even on both).

I'm stating to think that we should have this flag returned instead
when the app is adjusting the capture/output format.

In other words, a device with dozens of interfaces may need such flag
per interface (and, if M2M, on each side of the pipeline). 

So, IMO, the best would be to return such kind of flags at
S_FMT, G_FMT and TRY_FMT return. ON other words, this should be
added into v4l2_format.

So, the new v4l2_format.flags would be something like:

	V4L2_FMT_HAS_CROP
	V4L2_FMT_HAS_COMPOSE
	V4L2_FMT_HAS_SCALER

I suspect we have enough space for it already at v4l2_format, as it
reserves 200 bytes for format-specific structs, but, if I counted
well, the biggest one is 52 bytes (struct v4l2_pix_format_mplane).

So, we could define it, for example, as:

struct v4l2_format {
	__u32	 type;
	union {
		struct v4l2_pix_format		pix;     /* V4L2_BUF_TYPE_VIDEO_CAPTURE */
		struct v4l2_pix_format_mplane	pix_mp;  /* V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE */
		struct v4l2_window		win;     /* V4L2_BUF_TYPE_VIDEO_OVERLAY */
		struct v4l2_vbi_format		vbi;     /* V4L2_BUF_TYPE_VBI_CAPTURE */
		struct v4l2_sliced_vbi_format	sliced;  /* V4L2_BUF_TYPE_SLICED_VBI_CAPTURE */
		struct v4l2_sdr_format		sdr;     /* V4L2_BUF_TYPE_SDR_CAPTURE */
		struct v4l2_meta_format		meta;    /* V4L2_BUF_TYPE_META_CAPTURE */
		__u8	raw_data[188];                   /* reserve space for other structs */
	} fmt;
	__u32	flags;		/* This should be 64-bit aligned */
	__u8	reserved[8];
};

> The main problem is what to do with drivers that do not explicitly set
> these capabilities.

If the driver doesn't set those new capabilities, just don't return
V4L2_CAP_HAS_PER_FMT_CAPS (or whatever name we give for it).

I would make an effort tough to ensure that all drivers would be using
those flags, trying to touch all of them for the same Kernel version.

> Applications need to know whether the driver never
> sets the capabilities, or that it is not capable of these features.
> 
> Perhaps the best way is to add these caps:
> 
> V4L2_CAP2_VIDEO_CAPTURE_FIXED
> V4L2_CAP2_VIDEO_OUTPUT_FIXED
> V4L2_CAP2_VIDEO_M2M_FIXED

That doesn't make any sense for me. I mean, all drivers that return
V4L2_CAP_HAS_PER_FMT_CAPS to QUERYCAP should properly fill the new
v4l2_format.flags field, telling if it supports or not scaler/crop/compose.

> 
> that are set when the driver is incapable of cropping/composing/scaling.
> 
> If all bits are 0, then you don't know and you have to test for these
> features (i.e. the current situation). You want to have a nice macro
> to test this.

If V4L2_CAP_HAS_PER_FMT_CAPS is not set, apps should probe.

> 
> v4l2-compliance can easily use these caps to verify if the driver isn't
> lying to the user, so that is a nice bonus.
> 
> What do you think about this?
> 
> I can make an RFC PATCH if you think this is the right direction.

Yeah, a RFC is welcomed.

> 
> Regards,
> 
> 	Hans

Thanks,
Mauro