On 04/13/2016 09:57 PM, Mauro Carvalho Chehab wrote:
> Em Mon, 21 Mar 2016 09:47:59 +0100
> Hans Verkuil <hverkuil@xxxxxxxxx> escreveu:
>
>> From: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>>
>> 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@xxxxxxxxx>
>
> 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;
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
