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

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

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

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

-- 
2.7.0

[PATCH 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 | 44 ++++++++++++++++++------------------
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 86c4c19..67dbb03 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -2157,33 +2157,33 @@ 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)
+		return ops->vidioc_cropcap(file, fh, p);
 
-		/* 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

[PATCH 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 67dbb03..a3db569 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -2158,11 +2158,37 @@ 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)
 		return ops->vidioc_cropcap(file, fh, p);
 
+	/*
+	 * 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;
@@ -2185,13 +2211,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: [PATCH 1/2] v4l2-ioctl: simplify code

[Niklas Söderlund]

Hi Hans,

On 2016-02-29 11:16:39 +0100, Hans Verkuil wrote:
> 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 | 44 ++++++++++++++++++------------------
>  1 file changed, 22 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index 86c4c19..67dbb03 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -2157,33 +2157,33 @@ 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)
> +		return ops->vidioc_cropcap(file, fh, p);

I might be missing something but is there a guarantee 
ops->vidioc_cropcap is not NULL here?

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

-- 
Regards,
Niklas Söderlund

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

[Hans Verkuil]

On 03/14/2016 01:42 PM, Niklas Söderlund wrote:
> Hi Hans,
> 
> On 2016-02-29 11:16:39 +0100, Hans Verkuil wrote:
>> 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 | 44 ++++++++++++++++++------------------
>>  1 file changed, 22 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
>> index 86c4c19..67dbb03 100644
>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
>> @@ -2157,33 +2157,33 @@ 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)
>> +		return ops->vidioc_cropcap(file, fh, p);
> 
> I might be missing something but is there a guarantee 
> ops->vidioc_cropcap is not NULL here?

There is, either vidioc_g_selection or vidioc_cropcap will always be
non-NULL. Since g_selection == NULL it follows that cropcap != NULL.

But I admit that it isn't exactly obvious since the test that ensures
this is in determine_valid_ioctls() in v4l2-dev.c.

Regards,

	Hans

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

[Niklas Söderlund]

On 2016-03-14 13:52:17 +0100, Hans Verkuil wrote:
> On 03/14/2016 01:42 PM, Niklas Söderlund wrote:
> > Hi Hans,
> > 
> > On 2016-02-29 11:16:39 +0100, Hans Verkuil wrote:
> >> 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 | 44 ++++++++++++++++++------------------
> >>  1 file changed, 22 insertions(+), 22 deletions(-)
> >>
> >> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> >> index 86c4c19..67dbb03 100644
> >> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> >> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> >> @@ -2157,33 +2157,33 @@ 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)
> >> +		return ops->vidioc_cropcap(file, fh, p);
> > 
> > I might be missing something but is there a guarantee 
> > ops->vidioc_cropcap is not NULL here?
> 
> There is, either vidioc_g_selection or vidioc_cropcap will always be
> non-NULL. Since g_selection == NULL it follows that cropcap != NULL.
> 
> But I admit that it isn't exactly obvious since the test that ensures
> this is in determine_valid_ioctls() in v4l2-dev.c.

Nice, thanks for clarifying.

Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

-- 
Regards,
Niklas Söderlund

Re: [PATCH 2/2] v4l2-ioctl: improve cropcap handling

[Niklas Söderlund]

Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

On 2016-02-29 11:16:40 +0100, Hans Verkuil wrote:
> 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 67dbb03..a3db569 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -2158,11 +2158,37 @@ 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)
>  		return ops->vidioc_cropcap(file, fh, p);
>  
> +	/*
> +	 * 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;
> @@ -2185,13 +2211,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
> 

-- 
Regards,
Niklas Söderlund