[PATCHv2 0/2] v4l2-ioctl: cropcap improvements

[PATCHv2 0/2] v4l2-ioctl: cropcap improvements

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

The first patch just simplifies the logic in the code and makes no
functional changes.

The second patch improves the vidioc_cropcap handling with respect to
obtaining the pixel aspect ratio.

It was a bit buggy which I realized after reviewing the new rcar-vin driver.

Regards,

	Hans

Changes for v2:

- Added a sanity check for ops->vidioc_cropcap. While it shouldn't be NULL
  (determine_valid_ioctls() guards against that), it is not obvious from the
  code and since determine_valid_ioctls() is in a different source as well
  there is too much chance for someone to mess this up. I feel happier with
  a check in place.

Hans Verkuil (2):
  v4l2-ioctl: simplify code
  v4l2-ioctl: improve cropcap handling

 drivers/media/v4l2-core/v4l2-ioctl.c | 78 ++++++++++++++++++++++++------------
 1 file changed, 52 insertions(+), 26 deletions(-)

-- 
2.7.0

[PATCHv2 1/2] v4l2-ioctl: simplify code

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Instead of a big if at the beginning, just check if g_selection == NULL
and call the cropcap op immediately and return the result.

No functional changes in this patch.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/v4l2-core/v4l2-ioctl.c | 51 ++++++++++++++++++++----------------
 1 file changed, 29 insertions(+), 22 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 6bf5a3e..3cf8d3a 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -2160,33 +2160,40 @@ static int v4l_cropcap(const struct v4l2_ioctl_ops *ops,
 				struct file *file, void *fh, void *arg)
 {
 	struct v4l2_cropcap *p = arg;
+	struct v4l2_selection s = { .type = p->type };
+	int ret;
 
-	if (ops->vidioc_g_selection) {
-		struct v4l2_selection s = { .type = p->type };
-		int ret;
+	if (ops->vidioc_g_selection == NULL) {
+		/*
+		 * The determine_valid_ioctls() call already should ensure
+		 * that ops->vidioc_cropcap != NULL, but just in case...
+		 */
+		if (ops->vidioc_cropcap)
+			return ops->vidioc_cropcap(file, fh, p);
+		return -ENOTTY;
+	}
 
-		/* obtaining bounds */
-		if (V4L2_TYPE_IS_OUTPUT(p->type))
-			s.target = V4L2_SEL_TGT_COMPOSE_BOUNDS;
-		else
-			s.target = V4L2_SEL_TGT_CROP_BOUNDS;
+	/* obtaining bounds */
+	if (V4L2_TYPE_IS_OUTPUT(p->type))
+		s.target = V4L2_SEL_TGT_COMPOSE_BOUNDS;
+	else
+		s.target = V4L2_SEL_TGT_CROP_BOUNDS;
 
-		ret = ops->vidioc_g_selection(file, fh, &s);
-		if (ret)
-			return ret;
-		p->bounds = s.r;
+	ret = ops->vidioc_g_selection(file, fh, &s);
+	if (ret)
+		return ret;
+	p->bounds = s.r;
 
-		/* obtaining defrect */
-		if (V4L2_TYPE_IS_OUTPUT(p->type))
-			s.target = V4L2_SEL_TGT_COMPOSE_DEFAULT;
-		else
-			s.target = V4L2_SEL_TGT_CROP_DEFAULT;
+	/* obtaining defrect */
+	if (V4L2_TYPE_IS_OUTPUT(p->type))
+		s.target = V4L2_SEL_TGT_COMPOSE_DEFAULT;
+	else
+		s.target = V4L2_SEL_TGT_CROP_DEFAULT;
 
-		ret = ops->vidioc_g_selection(file, fh, &s);
-		if (ret)
-			return ret;
-		p->defrect = s.r;
-	}
+	ret = ops->vidioc_g_selection(file, fh, &s);
+	if (ret)
+		return ret;
+	p->defrect = s.r;
 
 	/* setting trivial pixelaspect */
 	p->pixelaspect.numerator = 1;
-- 
2.7.0

[PATCHv2 2/2] v4l2-ioctl: improve cropcap handling

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

If cropcap is implemented, then call it first to fill in the pixel
aspect ratio. Don't return if cropcap returns ENOTTY or ENOIOCTLCMD,
in that case just assume a 1:1 pixel aspect ratio.

After cropcap was called, then overwrite bounds and defrect with the
g_selection result because the g_selection() result always has priority
over anything that vidioc_cropcap returns.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/v4l2-core/v4l2-ioctl.c | 35 +++++++++++++++++++++++++++--------
 1 file changed, 27 insertions(+), 8 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 3cf8d3a..61eb810 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -2161,7 +2161,7 @@ static int v4l_cropcap(const struct v4l2_ioctl_ops *ops,
 {
 	struct v4l2_cropcap *p = arg;
 	struct v4l2_selection s = { .type = p->type };
-	int ret;
+	int ret = -ENOTTY;
 
 	if (ops->vidioc_g_selection == NULL) {
 		/*
@@ -2173,6 +2173,32 @@ static int v4l_cropcap(const struct v4l2_ioctl_ops *ops,
 		return -ENOTTY;
 	}
 
+	/*
+	 * Let cropcap fill in the pixelaspect if cropcap is available.
+	 * There is still no other way of obtaining this information.
+	 */
+	if (ops->vidioc_cropcap) {
+		ret = ops->vidioc_cropcap(file, fh, p);
+
+		/*
+		 * If cropcap reports that it isn't implemented,
+		 * then just keep going.
+		 */
+		if (ret && ret != -ENOTTY && ret != -ENOIOCTLCMD)
+			return ret;
+	}
+
+	if (ret) {
+		/*
+		 * cropcap wasn't implemented, so assume a 1:1 pixel
+		 * aspect ratio, otherwise keep what cropcap gave us.
+		 */
+		p->pixelaspect.numerator = 1;
+		p->pixelaspect.denominator = 1;
+	}
+
+	/* Use g_selection() to fill in the bounds and defrect rectangles */
+
 	/* obtaining bounds */
 	if (V4L2_TYPE_IS_OUTPUT(p->type))
 		s.target = V4L2_SEL_TGT_COMPOSE_BOUNDS;
@@ -2195,13 +2221,6 @@ static int v4l_cropcap(const struct v4l2_ioctl_ops *ops,
 		return ret;
 	p->defrect = s.r;
 
-	/* setting trivial pixelaspect */
-	p->pixelaspect.numerator = 1;
-	p->pixelaspect.denominator = 1;
-
-	if (ops->vidioc_cropcap)
-		return ops->vidioc_cropcap(file, fh, p);
-
 	return 0;
 }
 
-- 
2.7.0

Re: [PATCHv2 1/2] v4l2-ioctl: simplify code

[Mauro Carvalho Chehab]

Em Mon, 21 Mar 2016 09:47:59 +0100
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Instead of a big if at the beginning, just check if g_selection == NULL
> and call the cropcap op immediately and return the result.
> 
> No functional changes in this patch.

Hmm... not true. See below.

> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>

You forgot to add Miklas review here.

> ---
>  drivers/media/v4l2-core/v4l2-ioctl.c | 51 ++++++++++++++++++++----------------
>  1 file changed, 29 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index 6bf5a3e..3cf8d3a 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -2160,33 +2160,40 @@ static int v4l_cropcap(const struct v4l2_ioctl_ops *ops,
>  				struct file *file, void *fh, void *arg)
>  {
>  	struct v4l2_cropcap *p = arg;
> +	struct v4l2_selection s = { .type = p->type };
> +	int ret;
>  
> -	if (ops->vidioc_g_selection) {
> -		struct v4l2_selection s = { .type = p->type };
> -		int ret;
> +	if (ops->vidioc_g_selection == NULL) {
> +		/*
> +		 * The determine_valid_ioctls() call already should ensure
> +		 * that ops->vidioc_cropcap != NULL, but just in case...
> +		 */
> +		if (ops->vidioc_cropcap)
> +			return ops->vidioc_cropcap(file, fh, p);
> +		return -ENOTTY;

Actually, before this patch, the logic would be doing, instead:

        /* setting trivial pixelaspect */
        p->pixelaspect.numerator = 1;
        p->pixelaspect.denominator = 1;
 
        if (ops->vidioc_cropcap)
                return ops->vidioc_cropcap(file, fh, p);
 
        return 0;

With is not the same as what's there. So, there are actually two
differences:

1) it will now return -ENOTTY if !ops->vidioc_cropcap instead of 0.

(returning -ENOTTY is probably the best, but this is a fixup change,
and a functional change, so, should be in a separate patch, if needed)

2) pixel numerator/denominator is not initialized in the new code.

Ok, maybe all drivers would be setting numerator and denominator to 1
as default, so the code won't need to do it, but:

a) This is not a trivial assumption by looking only on this patch;

b) It sounds risky to not setup denominator, as, if a driver is not
initializing it, it could cause a division by zero at userspace.

So, I would prefer to keep initializing numerator/denominator here.

As this is just a cleanup patch, I'm skipping it for now.

Btw, IMHO, the best here would be to split the logic that it is called
if ops->vidioc_g_selection is not NULL on a separate function. That
would make the logic even clearer, and would help to show that it will
be handling the logic below "setting trivial pixelaspect" the same way.

Regards,
Mauro

> +	}
>  
> -		/* obtaining bounds */
> -		if (V4L2_TYPE_IS_OUTPUT(p->type))
> -			s.target = V4L2_SEL_TGT_COMPOSE_BOUNDS;
> -		else
> -			s.target = V4L2_SEL_TGT_CROP_BOUNDS;
> +	/* obtaining bounds */
> +	if (V4L2_TYPE_IS_OUTPUT(p->type))
> +		s.target = V4L2_SEL_TGT_COMPOSE_BOUNDS;
> +	else
> +		s.target = V4L2_SEL_TGT_CROP_BOUNDS;
>  
> -		ret = ops->vidioc_g_selection(file, fh, &s);
> -		if (ret)
> -			return ret;
> -		p->bounds = s.r;
> +	ret = ops->vidioc_g_selection(file, fh, &s);
> +	if (ret)
> +		return ret;
> +	p->bounds = s.r;
>  
> -		/* obtaining defrect */
> -		if (V4L2_TYPE_IS_OUTPUT(p->type))
> -			s.target = V4L2_SEL_TGT_COMPOSE_DEFAULT;
> -		else
> -			s.target = V4L2_SEL_TGT_CROP_DEFAULT;
> +	/* obtaining defrect */
> +	if (V4L2_TYPE_IS_OUTPUT(p->type))
> +		s.target = V4L2_SEL_TGT_COMPOSE_DEFAULT;
> +	else
> +		s.target = V4L2_SEL_TGT_CROP_DEFAULT;
>  
> -		ret = ops->vidioc_g_selection(file, fh, &s);
> -		if (ret)
> -			return ret;
> -		p->defrect = s.r;
> -	}
> +	ret = ops->vidioc_g_selection(file, fh, &s);
> +	if (ret)
> +		return ret;
> +	p->defrect = s.r;
>  
>  	/* setting trivial pixelaspect */
>  	p->pixelaspect.numerator = 1;


-- 
Thanks,
Mauro

Re: [PATCHv2 1/2] v4l2-ioctl: simplify code

[Hans Verkuil]

On 04/13/2016 09:57 PM, Mauro Carvalho Chehab wrote:
> Em Mon, 21 Mar 2016 09:47:59 +0100
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> 
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> Instead of a big if at the beginning, just check if g_selection == NULL
>> and call the cropcap op immediately and return the result.
>>
>> No functional changes in this patch.
> 
> Hmm... not true. See below.
> 
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> 
> You forgot to add Miklas review here.
> 
>> ---
>>  drivers/media/v4l2-core/v4l2-ioctl.c | 51 ++++++++++++++++++++----------------
>>  1 file changed, 29 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
>> index 6bf5a3e..3cf8d3a 100644
>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
>> @@ -2160,33 +2160,40 @@ static int v4l_cropcap(const struct v4l2_ioctl_ops *ops,
>>  				struct file *file, void *fh, void *arg)
>>  {
>>  	struct v4l2_cropcap *p = arg;
>> +	struct v4l2_selection s = { .type = p->type };
>> +	int ret;
>>  
>> -	if (ops->vidioc_g_selection) {
>> -		struct v4l2_selection s = { .type = p->type };
>> -		int ret;
>> +	if (ops->vidioc_g_selection == NULL) {
>> +		/*
>> +		 * The determine_valid_ioctls() call already should ensure
>> +		 * that ops->vidioc_cropcap != NULL, but just in case...
>> +		 */
>> +		if (ops->vidioc_cropcap)
>> +			return ops->vidioc_cropcap(file, fh, p);
>> +		return -ENOTTY;
> 
> Actually, before this patch, the logic would be doing, instead:
> 
>         /* setting trivial pixelaspect */
>         p->pixelaspect.numerator = 1;
>         p->pixelaspect.denominator = 1;
>  
>         if (ops->vidioc_cropcap)
>                 return ops->vidioc_cropcap(file, fh, p);
>  
>         return 0;
> 
> With is not the same as what's there. So, there are actually two
> differences:
> 
> 1) it will now return -ENOTTY if !ops->vidioc_cropcap instead of 0.
> 
> (returning -ENOTTY is probably the best, but this is a fixup change,
> and a functional change, so, should be in a separate patch, if needed)
> 
> 2) pixel numerator/denominator is not initialized in the new code.
> 
> Ok, maybe all drivers would be setting numerator and denominator to 1
> as default, so the code won't need to do it, but:
> 
> a) This is not a trivial assumption by looking only on this patch;
> 
> b) It sounds risky to not setup denominator, as, if a driver is not
> initializing it, it could cause a division by zero at userspace.
> 
> So, I would prefer to keep initializing numerator/denominator here.
> 
> As this is just a cleanup patch, I'm skipping it for now.

I'm confused, you say you're skipping it, yet it is merged. But patch 2/2
(v4l2-ioctl: improve cropcap handling) isn't merged.

I refrain from commenting on the points you raised until I know what's going
on here.

Regards,

	Hans

> 
> Btw, IMHO, the best here would be to split the logic that it is called
> if ops->vidioc_g_selection is not NULL on a separate function. That
> would make the logic even clearer, and would help to show that it will
> be handling the logic below "setting trivial pixelaspect" the same way.
> 
> Regards,
> Mauro
> 
>> +	}
>>  
>> -		/* obtaining bounds */
>> -		if (V4L2_TYPE_IS_OUTPUT(p->type))
>> -			s.target = V4L2_SEL_TGT_COMPOSE_BOUNDS;
>> -		else
>> -			s.target = V4L2_SEL_TGT_CROP_BOUNDS;
>> +	/* obtaining bounds */
>> +	if (V4L2_TYPE_IS_OUTPUT(p->type))
>> +		s.target = V4L2_SEL_TGT_COMPOSE_BOUNDS;
>> +	else
>> +		s.target = V4L2_SEL_TGT_CROP_BOUNDS;
>>  
>> -		ret = ops->vidioc_g_selection(file, fh, &s);
>> -		if (ret)
>> -			return ret;
>> -		p->bounds = s.r;
>> +	ret = ops->vidioc_g_selection(file, fh, &s);
>> +	if (ret)
>> +		return ret;
>> +	p->bounds = s.r;
>>  
>> -		/* obtaining defrect */
>> -		if (V4L2_TYPE_IS_OUTPUT(p->type))
>> -			s.target = V4L2_SEL_TGT_COMPOSE_DEFAULT;
>> -		else
>> -			s.target = V4L2_SEL_TGT_CROP_DEFAULT;
>> +	/* obtaining defrect */
>> +	if (V4L2_TYPE_IS_OUTPUT(p->type))
>> +		s.target = V4L2_SEL_TGT_COMPOSE_DEFAULT;
>> +	else
>> +		s.target = V4L2_SEL_TGT_CROP_DEFAULT;
>>  
>> -		ret = ops->vidioc_g_selection(file, fh, &s);
>> -		if (ret)
>> -			return ret;
>> -		p->defrect = s.r;
>> -	}
>> +	ret = ops->vidioc_g_selection(file, fh, &s);
>> +	if (ret)
>> +		return ret;
>> +	p->defrect = s.r;
>>  
>>  	/* setting trivial pixelaspect */
>>  	p->pixelaspect.numerator = 1;
> 
> 

Re: [PATCHv2 1/2] v4l2-ioctl: simplify code

[Mauro Carvalho Chehab]

Em Wed, 13 Apr 2016 23:51:45 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> On 04/13/2016 09:57 PM, Mauro Carvalho Chehab wrote:
> > Em Mon, 21 Mar 2016 09:47:59 +0100
> > Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> >   
> >> From: Hans Verkuil <hans.verkuil@cisco.com>
> >>
> >> Instead of a big if at the beginning, just check if g_selection == NULL
> >> and call the cropcap op immediately and return the result.
> >>
> >> No functional changes in this patch.  
> > 
> > Hmm... not true. See below.
> >   
> >>
> >> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>  
> > 
> > You forgot to add Miklas review here.
> >   
> >> ---
> >>  drivers/media/v4l2-core/v4l2-ioctl.c | 51 ++++++++++++++++++++----------------
> >>  1 file changed, 29 insertions(+), 22 deletions(-)
> >>
> >> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> >> index 6bf5a3e..3cf8d3a 100644
> >> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> >> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> >> @@ -2160,33 +2160,40 @@ static int v4l_cropcap(const struct v4l2_ioctl_ops *ops,
> >>  				struct file *file, void *fh, void *arg)
> >>  {
> >>  	struct v4l2_cropcap *p = arg;
> >> +	struct v4l2_selection s = { .type = p->type };
> >> +	int ret;
> >>  
> >> -	if (ops->vidioc_g_selection) {
> >> -		struct v4l2_selection s = { .type = p->type };
> >> -		int ret;
> >> +	if (ops->vidioc_g_selection == NULL) {
> >> +		/*
> >> +		 * The determine_valid_ioctls() call already should ensure
> >> +		 * that ops->vidioc_cropcap != NULL, but just in case...
> >> +		 */
> >> +		if (ops->vidioc_cropcap)
> >> +			return ops->vidioc_cropcap(file, fh, p);
> >> +		return -ENOTTY;  
> > 
> > Actually, before this patch, the logic would be doing, instead:
> > 
> >         /* setting trivial pixelaspect */
> >         p->pixelaspect.numerator = 1;
> >         p->pixelaspect.denominator = 1;
> >  
> >         if (ops->vidioc_cropcap)
> >                 return ops->vidioc_cropcap(file, fh, p);
> >  
> >         return 0;
> > 
> > With is not the same as what's there. So, there are actually two
> > differences:
> > 
> > 1) it will now return -ENOTTY if !ops->vidioc_cropcap instead of 0.
> > 
> > (returning -ENOTTY is probably the best, but this is a fixup change,
> > and a functional change, so, should be in a separate patch, if needed)
> > 
> > 2) pixel numerator/denominator is not initialized in the new code.
> > 
> > Ok, maybe all drivers would be setting numerator and denominator to 1
> > as default, so the code won't need to do it, but:
> > 
> > a) This is not a trivial assumption by looking only on this patch;
> > 
> > b) It sounds risky to not setup denominator, as, if a driver is not
> > initializing it, it could cause a division by zero at userspace.
> > 
> > So, I would prefer to keep initializing numerator/denominator here.
> > 
> > As this is just a cleanup patch, I'm skipping it for now.  
> 
> I'm confused, you say you're skipping it, yet it is merged. But patch 2/2
> (v4l2-ioctl: improve cropcap handling) isn't merged.
> 
> I refrain from commenting on the points you raised until I know what's going
> on here.

Sorry, my mistake. I'll revert it. Please send patch 2/2 after
we fix the issues on this one.

> 
> Regards,
> 
> 	Hans
> 
> > 
> > Btw, IMHO, the best here would be to split the logic that it is called
> > if ops->vidioc_g_selection is not NULL on a separate function. That
> > would make the logic even clearer, and would help to show that it will
> > be handling the logic below "setting trivial pixelaspect" the same way.
> > 
> > Regards,
> > Mauro
> >   
> >> +	}
> >>  
> >> -		/* obtaining bounds */
> >> -		if (V4L2_TYPE_IS_OUTPUT(p->type))
> >> -			s.target = V4L2_SEL_TGT_COMPOSE_BOUNDS;
> >> -		else
> >> -			s.target = V4L2_SEL_TGT_CROP_BOUNDS;
> >> +	/* obtaining bounds */
> >> +	if (V4L2_TYPE_IS_OUTPUT(p->type))
> >> +		s.target = V4L2_SEL_TGT_COMPOSE_BOUNDS;
> >> +	else
> >> +		s.target = V4L2_SEL_TGT_CROP_BOUNDS;
> >>  
> >> -		ret = ops->vidioc_g_selection(file, fh, &s);
> >> -		if (ret)
> >> -			return ret;
> >> -		p->bounds = s.r;
> >> +	ret = ops->vidioc_g_selection(file, fh, &s);
> >> +	if (ret)
> >> +		return ret;
> >> +	p->bounds = s.r;
> >>  
> >> -		/* obtaining defrect */
> >> -		if (V4L2_TYPE_IS_OUTPUT(p->type))
> >> -			s.target = V4L2_SEL_TGT_COMPOSE_DEFAULT;
> >> -		else
> >> -			s.target = V4L2_SEL_TGT_CROP_DEFAULT;
> >> +	/* obtaining defrect */
> >> +	if (V4L2_TYPE_IS_OUTPUT(p->type))
> >> +		s.target = V4L2_SEL_TGT_COMPOSE_DEFAULT;
> >> +	else
> >> +		s.target = V4L2_SEL_TGT_CROP_DEFAULT;
> >>  
> >> -		ret = ops->vidioc_g_selection(file, fh, &s);
> >> -		if (ret)
> >> -			return ret;
> >> -		p->defrect = s.r;
> >> -	}
> >> +	ret = ops->vidioc_g_selection(file, fh, &s);
> >> +	if (ret)
> >> +		return ret;
> >> +	p->defrect = s.r;
> >>  
> >>  	/* setting trivial pixelaspect */
> >>  	p->pixelaspect.numerator = 1;  
> > 
> >   
> 


-- 
Thanks,
Mauro