Re: [PATCH v14 11/11] media: uvcvideo: document UVC v1.5 ROI

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Yunke,

On Tue, Dec 12, 2023 at 01:45:00PM +0900, Yunke Cao wrote:
> On Sat, Dec 9, 2023 at 1:00 AM Laurent Pinchart wrote:
> > On Fri, Dec 01, 2023 at 04:19:02PM +0900, Yunke Cao wrote:
> > > Added documentation of V4L2_CID_UVC_REGION_OF_INTEREST_RECT and
> > > V4L2_CID_UVC_REGION_OF_INTEREST_AUTO.
> >
> > It would be useful to point here to the userspace code that uses those
> > controls. A link to the appropriate Chrome OS git tree would be useful,
> > to showcase a real user.
> 
> Does https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/hal/usb/
> work?

I would pin it to a branch, as it may disappear from the main branch
later:

https://chromium.googlesource.com/chromiumos/platform2/+/refs/heads/release-R121-15699.B/camera/hal/usb/

> Do you think we need the link in both the commit message and this .rst file?

Sounds good to me.

> > > Reviewed-by: Ricardo Ribalda <ribalda@chromium.org>
> > > Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> > > Signed-off-by: Yunke Cao <yunkec@google.com>
> > > ---
> > > Changelog since v11:
> > > - No change.
> > > Changelog since v10:
> > > - Added Reviewed-by from Sergey.
> > > Changelog since v9:
> > > - No change.
> > > Changelog since v8:
> > > - No change.
> > > Changelog since v7:
> > > - Fix documentation for automatic exposure based on comment in v7.
> > >
> > >  .../userspace-api/media/drivers/uvcvideo.rst  | 62 +++++++++++++++++++
> > >  1 file changed, 62 insertions(+)
> > >
> > > diff --git a/Documentation/userspace-api/media/drivers/uvcvideo.rst b/Documentation/userspace-api/media/drivers/uvcvideo.rst
> > > index aab4304e6bb5..3dc062221f8b 100644
> > > --- a/Documentation/userspace-api/media/drivers/uvcvideo.rst
> > > +++ b/Documentation/userspace-api/media/drivers/uvcvideo.rst
> > > @@ -181,6 +181,7 @@ Argument: struct uvc_xu_control_mapping
> > >       UVC_CTRL_DATA_TYPE_BOOLEAN      Boolean
> > >       UVC_CTRL_DATA_TYPE_ENUM         Enumeration
> > >       UVC_CTRL_DATA_TYPE_BITMASK      Bitmask
> > > +     UVC_CTRL_DATA_TYPE_RECT         Rectangular area
> > >
> > >
> > >  UVCIOC_CTRL_QUERY - Query a UVC XU control
> > > @@ -255,3 +256,64 @@ Argument: struct uvc_xu_control_query
> > >       __u8    query           Request code to send to the device
> > >       __u16   size            Control data size (in bytes)
> > >       __u8    *data           Control value
> > > +
> > > +
> > > +Driver-specific V4L2 controls
> > > +-----------------------------
> > > +
> > > +The uvcvideo driver implements the following UVC-specific controls:
> > > +
> > > +``V4L2_CID_UVC_REGION_OF_INTEREST_RECT (struct)``
> > > +     This control determines the region of interest (ROI). ROI is a
> > > +     rectangular area represented by a struct :c:type:`v4l2_rect`. The
> > > +     rectangle is in global sensor coordinates and pixel units. It is
> > > +     independent of the field of view, not impacted by any cropping or
> > > +     scaling.
> > > +
> > > +     Use ``V4L2_CTRL_WHICH_MIN_VAL`` and ``V4L2_CTRL_WHICH_MAX_VAL`` to query
> > > +     the range of rectangle sizes. The left/top coordinates of a minimum or
> > > +     maximum rectangle are always 0. For example, a device can have a minimum
> > > +     ROI rectangle of 1x1@0x0 and a maximum of 640x480@0x0.
> >
> > Is that actually true ? The UVC 1.5 specification states
> >
> >     GET_MAX shall return the current Window as specified by
> >     CT_DIGITAL_WINDOW_CONTROL.
> >
> > And the window can have non-zero left and right coordinates.
> 
> Ah, you are right. I think I will just remove "The left/top
> coordinates of a minimum or maximum rectangle are always 0."

The rectangle would still be relative to CT_DIGITAL_WINDOW_CONTROL,
which isn't exposed by the driver to userspace. Isn't that a problem ?

> > > +
> > > +     Setting a ROI allows the camera to optimize the capture for the region.
> > > +     The value of ``V4L2_CID_REGION_OF_INTEREST_AUTO`` control determines
> > > +     the detailed behavior.
> > > +
> > > +
> > > +``V4L2_CID_UVC_REGION_OF_INTEREST_AUTO (bitmask)``
> > > +     This determines which, if any, on board features should track to the
> >
> > s/on board/on-board/
> >
> > > +     Region of Interest specified by the current value of
> > > +     ``V4L2_CID_UVD__REGION_OF_INTEREST_RECT``.
> > > +
> > > +     Max value is a mask indicating all supported Auto Controls.
> > > +
> > > +.. flat-table::
> > > +    :header-rows:  0
> > > +    :stub-columns: 0
> > > +
> > > +    * - ``V4L2_UVC_REGION_OF_INTEREST_AUTO_EXPOSURE``
> > > +      - Setting this to true causes automatic exposure to track the region of
> >
> > Maybe "Setting this bit causes..." as those values are bit flags, not
> > booleans. Same below.
> 
> Sounds good.
> 
> > > +     interest instead of the whole image.
> > > +    * - ``V4L2_UVC_REGION_OF_INTEREST_AUTO_IRIS``
> > > +      - Setting this to true causes automatic iris to track the region of
> > > +     interest instead of the whole image.
> > > +    * - ``V4L2_UVC_REGION_OF_INTEREST_AUTO_WHITE_BALANCE``
> > > +      - Setting this to true causes automatic white balance to track the region
> > > +     of interest instead of the whole image.
> > > +    * - ``V4L2_UVC_REGION_OF_INTEREST_AUTO_FOCUS``
> > > +      - Setting this to true causes automatic focus adjustment to track the
> > > +     region of interest instead of the whole image.
> > > +    * - ``V4L2_UVC_REGION_OF_INTEREST_AUTO_FACE_DETECT``
> > > +      - Setting this to true causes automatic face detection to track the
> > > +     region of interest instead of the whole image.
> > > +    * - ``V4L2_UVC_REGION_OF_INTEREST_AUTO_DETECT_AND_TRACK``
> > > +      - Setting this to true enables automatic face detection and tracking. The
> > > +     current value of ``V4L2_CID_REGION_OF_INTEREST_RECT`` may be updated by
> > > +     the driver.
> > > +    * - ``V4L2_UVC_REGION_OF_INTEREST_AUTO_IMAGE_STABILIZATION``
> > > +      - Setting this to true enables automatic image stabilization. The
> > > +     current value of ``V4L2_CID_REGION_OF_INTEREST_RECT`` may be updated by
> > > +     the driver.
> > > +    * - ``V4L2_UVC_REGION_OF_INTEREST_AUTO_HIGHER_QUALITY``
> > > +      - Setting this to true enables automatically capture the specified region
> >
> > I wonder what this means, the UVC 1.5 specification doesn't document
> > this bit clearly :-(
> 
> I have no idea :-( . I haven't seen any camera module in Chrome OS
> attempt to implement this bit, either.
> 
> > > +     with higher quality if possible.

-- 
Regards,

Laurent Pinchart