linux-kernel - Re: [PATCH v15 19/19] media: uvcvideo: document UVC v1.5 ROI

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CANiDSCsphbdkHePXManvtR_i4iSUmHkFXQLzZGSc7BJ900c1Hw@mail.gmail.com>
Date: Mon, 9 Dec 2024 16:22:14 +0100
From: Ricardo Ribalda <ribalda@...omium.org>
To: Hans de Goede <hdegoede@...hat.com>
Cc: Laurent Pinchart <laurent.pinchart@...asonboard.com>, 
	Mauro Carvalho Chehab <mchehab@...nel.org>, Ricardo Ribalda <ribalda@...nel.org>, 
	Sakari Ailus <sakari.ailus@...ux.intel.com>, Hans Verkuil <hverkuil@...all.nl>, 
	Yunke Cao <yunkec@...omium.org>, linux-media@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Yunke Cao <yunkec@...gle.com>, 
	Sergey Senozhatsky <senozhatsky@...omium.org>
Subject: Re: [PATCH v15 19/19] media: uvcvideo: document UVC v1.5 ROI

Hi Hans

On Mon, 9 Dec 2024 at 15:36, Hans de Goede <hdegoede@...hat.com> wrote:
>
> Hi,
>
> On 14-Nov-24 8:10 PM, Ricardo Ribalda wrote:
> > From: Yunke Cao <yunkec@...gle.com>
> >
> > Added documentation of V4L2_CID_UVC_REGION_OF_INTEREST_RECT and
> > V4L2_CID_UVC_REGION_OF_INTEREST_AUTO.
> >
> > An example of a userspace implementing this feature can be found at:
> > https://chromium.googlesource.com/chromiumos/platform2/+/refs/heads/release-R121-15699.B/camera/hal/usb/
> >
> > Reviewed-by: Ricardo Ribalda <ribalda@...omium.org>
> > Reviewed-by: Sergey Senozhatsky <senozhatsky@...omium.org>
> > Signed-off-by: Yunke Cao <yunkec@...gle.com>
> > Signed-off-by: Ricardo Ribalda <ribalda@...omium.org>
> > --->  .../userspace-api/media/drivers/uvcvideo.rst       | 64 ++++++++++++++++++++++
> >  1 file changed, 64 insertions(+)
> >
> > diff --git a/Documentation/userspace-api/media/drivers/uvcvideo.rst b/Documentation/userspace-api/media/drivers/uvcvideo.rst
> > index a290f9fadae9..1cdcd45907a3 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,66 @@ 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
>
> Maybe: "The rectangle is in global sensor coordinates using pixel units" ?
>
> being "in pixel units" sounds a bit weird and had me confused for a moment.
>
> > +     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.
> > +
> > +     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.
> > +
> > +     An example of use of this control, can be found in the:
> > +     `Chrome OS USB camera HAL.
> > +     <https://chromium.googlesource.com/chromiumos/platform2/+/refs/heads/release-R121-15699.B/camera/hal/usb/>`
>
> Hmm, not sure we want this in the API documentation. OTOH why not ? Anyone else
> have an opinion on this ?

Laurent requested this:
https://lore.kernel.org/linux-media/20231218034413.GN5290@pendragon.ideasonboard.com/

>
> > +
> > +
> > +``V4L2_CID_UVC_REGION_OF_INTEREST_AUTO (bitmask)``
> > +     This determines which, if any, on-board features should track to the
> > +     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 bit causes automatic exposure to track the region of
> > +     interest instead of the whole image.
> > +    * - ``V4L2_UVC_REGION_OF_INTEREST_AUTO_IRIS``
> > +      - Setting this bit causes automatic iris to track the region of interest
> > +        instead of the whole image.
> > +    * - ``V4L2_UVC_REGION_OF_INTEREST_AUTO_WHITE_BALANCE``
> > +      - Setting this bit causes automatic white balance to track the region
> > +     of interest instead of the whole image.
> > +    * - ``V4L2_UVC_REGION_OF_INTEREST_AUTO_FOCUS``
> > +      - Setting this bit 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 bit 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 bit 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 bit enables automatic image stabilization. The
> > +     current value of ``V4L2_CID_REGION_OF_INTEREST_RECT`` may be updated by
> > +     the driver.
>
> This one I do not understand. Since the ROI is not a crop, I don't see how
> this interacts with image-stabilization. Typically digital image-stabilization
> uses a moving slightly smaller crop of the full sensor rectangle which it moves
> around in realtime to compensate for camera movements.
>
> So I wonder what this is expected to do. Does this set the ROI to the image
> stabilization crop ? I guess that combined with reading back the ROI that might be
> somewhat useful to follow what the image stabilization code is doing.
>
> OTOH this does not seem useful for using as region for AEC / AWB ?

Unfortunately, the standard is not very verbose about this:
https://ibb.co/VppnQ43

What about:

- ``Image Stabilization`` bit from the UVC's bmAutoControls Region of
Interest Control.

?


>
>
> > +    * - ``V4L2_UVC_REGION_OF_INTEREST_AUTO_HIGHER_QUALITY``
> > +      - Setting this bit enables automatically capture the specified region
> > +        with higher quality if possible.
> >
>
> Otherwise this looks good to me. But I would still like to see
> a discussion about using UVC custom ctrls instead of something standardized
> for this. Although I guess maybe that already happened before I got involved ?

Seems like both Hans V and Laurent preferred uvc custom controls:

https://lore.kernel.org/linux-media/a0fe2b49-12b7-8eaf-c3ef-7af1a247e595@xs4all.nl/

>
> Regards,
>
> Hans
>


-- 
Ricardo Ribalda