Re: [PATCHv3 5/6] media: uvcvideo: add UVC 1.5 ROI control

From: Tomasz Figa <tfiga@chromium.org>
To: Sergey Senozhatsky <senozhatsky@chromium.org>
Cc: Ricardo Ribalda <ribalda@chromium.org>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	Hans Verkuil <hverkuil-cisco@xs4all.nl>,
	Linux Media Mailing List <linux-media@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCHv3 5/6] media: uvcvideo: add UVC 1.5 ROI control
Date: Wed, 24 Mar 2021 11:14:27 +0900	[thread overview]
Message-ID: <CAAFQd5DaDZA8==HPrL1v1M=1a5g3DgY58nuq7KnA8USQ6UuiTQ@mail.gmail.com> (raw)
In-Reply-To: <YFqdaHCQak5ZM0Sf@google.com>

On Wed, Mar 24, 2021 at 11:01 AM Sergey Senozhatsky
<senozhatsky@chromium.org> wrote:
>
> On (21/03/23 17:16), Ricardo Ribalda wrote:
> [..]
> > > +static bool validate_roi_bounds(struct uvc_streaming *stream,
> > > +                               struct v4l2_selection *sel)
> > > +{
> > > +       if (sel->r.left > USHRT_MAX ||
> > > +           sel->r.top > USHRT_MAX ||
> > > +           (sel->r.width + sel->r.left) > USHRT_MAX ||
> > > +           (sel->r.height + sel->r.top) > USHRT_MAX ||
> > > +           !sel->r.width || !sel->r.height)
> > > +               return false;
> > > +
> > > +       if (sel->flags > V4L2_SEL_FLAG_ROI_AUTO_HIGHER_QUALITY)
> > > +               return false;
> >
> > Is it not allowed V4L2_SEL_FLAG_ROI_AUTO_IRIS |
> > V4L2_SEL_FLAG_ROI_AUTO_HIGHER_QUALITY   ?
>
> Good question.
>
> I don't know. Depends on what HIGHER_QUALITY can stand for (UVC doesn't
> specify). But overall it seems like features there are mutually
> exclusive. E.g. AUTO_FACE_DETECT and AUTO_DETECT_AND_TRACK.
>
>
> I think it'll be better to replace this with
>
>         if (sel->flags > USHRT_MAX)
>                 return false;
>
> so that we don't let overflow happen and accidentally enable/disable
> some of the features.
>
> > > +
> > > +       return true;
> > > +}
> > > +
> > > +static int uvc_ioctl_s_roi(struct file *file, void *fh,
> > > +                          struct v4l2_selection *sel)
> > > +{
> > > +       struct uvc_fh *handle = fh;
> > > +       struct uvc_streaming *stream = handle->stream;
> > > +       struct uvc_roi_rect *roi;
> > > +       int ret;
> > > +
> > > +       if (!validate_roi_bounds(stream, sel))
> > > +               return -E2BIG;
> >
> > Not sure if this is the correct approach or if we should convert the
> > value to the closest valid...
>
> Well, at this point we know that ROI rectangle dimensions are out of
> sane value range. I'd rather tell user-space about integer overflow.

Adjusting the rectangle to something supported by the hardware is
mentioned explicitly in the V4L2 API documentation and is what drivers
have to implement. Returning an error on invalid value is not a
correct behavior here (and similarly for many other operations, e.g.
S_FMT).

https://www.kernel.org/doc/html/v4.8/media/uapi/v4l/vidioc-g-selection.html

>
> Looking for the closest ROI rectangle that suffice can be rather
> tricky. It may sounds like we can just use BOUNDARIES_MAX, but this
> is what Firmware D returns for GET_MAX
>
> ioctl(V4L2_SEL_TGT_ROI_BOUNDS_MAX)
>
>         0, 0, 65535, 65535

Perhaps the frame size would be the correct bounds?

Best regards,
Tomasz