Hi Hans On Mon, 9 Dec 2024 at 15:36, Hans de Goede <hdegoede@xxxxxxxxxx> wrote: > > Hi, > > On 14-Nov-24 8:10 PM, Ricardo Ribalda wrote: > > From: Yunke Cao <yunkec@xxxxxxxxxx> > > > > 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@xxxxxxxxxxxx> > > Reviewed-by: Sergey Senozhatsky <senozhatsky@xxxxxxxxxxxx> > > Signed-off-by: Yunke Cao <yunkec@xxxxxxxxxx> > > Signed-off-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx> > > ---> .../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@xxxxxxxxxxxxxxxxxxxxxxxxxx/ > > > + > > + > > +``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@xxxxxxxxx/ > > Regards, > > Hans > -- Ricardo Ribalda
