Re: [PATCHv2 2/3] media: uvcvideo: add ROI auto controls

[Ricardo Ribalda Delgado]

On Wed, Mar 17, 2021 at 2:34 AM Sergey Senozhatsky
<sergey.senozhatsky.work@xxxxxxxxx> wrote:
>
> On (21/03/16 19:29), Ricardo Ribalda Delgado wrote:
> > > ROI control is a compound data type:
> > >   Control Selector     CT_REGION_OF_INTEREST_CONTROL
> > >   Mandatory Requests   SET_CUR, GET_CUR, GET_MIN, GET_MAX, GET_DEF
> > >   wLength 10
> > >   Offset   Field            Size
> > >   0        wROI_Top         2
> > >   2        wROI_Left        2
> > >   4        wROI_Bottom      2
> > >   6        wROI_Right       2
> > >   8        bmAutoControls   2       (Bitmap)
> > >
> > > uvc_control_mapping, however, can handle only s32 data type at the
> > > moment: ->get() returns s32 value, ->set() accepts s32 value; while
> > > v4l2_ctrl maximum/minimum/default_value can hold only s64 values.
> > >
> > > Hence ROI control handling is split into two patches:
> > > a) bmAutoControls is handled via uvc_control_mapping as V4L2_CTRL_TYPE_MENU
> > > b) ROI rectangle (SET_CUR, GET_CUR, GET_DEF) handling is implemented
> > >    separately, by the means of selection API.
> >
> > Maybe a reference to the uvc doc would be a good thing to add here.
>
> OK. What should be referenced there?

Nevermind, I thought there was a non-pdf version of the standard :(
https://www.usb.org/document-library/video-class-v15-document-set

>
> > > +    * - ``V4L2_CID_REGION_OF_INTEREST_AUTO_EXPOSURE``
> > > +      - Auto Exposure.
> > > +    * - ``V4L2_CID_REGION_OF_INTEREST_AUTO_IRIS``
> > > +      - Auto Iris.
> > > +    * - ``V4L2_CID_REGION_OF_INTEREST_AUTO_WHITE_BALANCE``
> > > +      - Auto White Balance.
> > > +    * - ``V4L2_CID_REGION_OF_INTEREST_AUTO_FOCUS``
> > > +      - Auto Focus.
> > > +    * - ``V4L2_CID_REGION_OF_INTEREST_AUTO_FACE_DETECT``
> > > +      - Auto Face Detect.
> > > +    * - ``V4L2_CID_REGION_OF_INTEREST_AUTO_DETECT_AND_TRACK``
> > > +      - Auto Detect and Track.
> > > +    * - ``V4L2_CID_REGION_OF_INTEREST_AUTO_IMAGE_STABILIXATION``
> > > +      - Image Stabilization.
> > > +    * - ``V4L2_CID_REGION_OF_INTEREST_AUTO_HIGHER_QUALITY``
> > > +      - Higher Quality.
> > > +
> > >
> > Nit: Same as before splitting doc and code.
>
> OK, I guess I can split.
>
> > >  static const struct uvc_menu_info power_line_frequency_controls[] = {
> > > @@ -753,6 +762,16 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
> > >                 .v4l2_type      = V4L2_CTRL_TYPE_BOOLEAN,
> > >                 .data_type      = UVC_CTRL_DATA_TYPE_BOOLEAN,
> > >         },
> > > +       {
> > > +               .id             = V4L2_CID_REGION_OF_INTEREST_AUTO,
> > > +               .name           = "Region of Interest (auto)",
> > > +               .entity         = UVC_GUID_UVC_CAMERA,
> > > +               .selector       = UVC_CT_REGION_OF_INTEREST_CONTROL,
> > > +               .size           = 16,
> > > +               .offset         = 64,
> > > +               .v4l2_type      = V4L2_CTRL_TYPE_BITMASK,
> > Are
>
> Are?

Aye!
You are not a good kernel reviewer if you do not talk pirate :P.

I wanted to make sure that V4L2_CTRL_TYPE_BITMASK was handled by the
uvc driver, and I think that it will work fine.
Sorry for the noise.

>
> > >  #define V4L2_CID_PAN_SPEED                     (V4L2_CID_CAMERA_CLASS_BASE+32)
> > >  #define V4L2_CID_TILT_SPEED                    (V4L2_CID_CAMERA_CLASS_BASE+33)
> > > +#define V4L2_CID_REGION_OF_INTEREST_AUTO       (V4L2_CID_CAMERA_CLASS_BASE+34)
> > > +#define V4L2_CID_REGION_OF_INTEREST_AUTO_EXPOSURE              (1 << 0)
> > > +#define V4L2_CID_REGION_OF_INTEREST_AUTO_IRIS                  (1 << 1)
> > > +#define V4L2_CID_REGION_OF_INTEREST_AUTO_WHITE_BALANCE         (1 << 2)
> > > +#define V4L2_CID_REGION_OF_INTEREST_AUTO_FOCUS                 (1 << 3)
> > > +#define V4L2_CID_REGION_OF_INTEREST_AUTO_FACE_DETECT           (1 << 4)
> > > +#define V4L2_CID_REGION_OF_INTEREST_AUTO_DETECT_AND_TRACK      (1 << 5)
> > > +#define V4L2_CID_REGION_OF_INTEREST_AUTO_IMAGE_STABILIXATION   (1 << 6)
> > > +#define V4L2_CID_REGION_OF_INTEREST_AUTO_HIGHER_QUALITY        (1 << 7)
> > >
> > >  #define V4L2_CID_CAMERA_ORIENTATION            (V4L2_CID_CAMERA_CLASS_BASE+34)
> > >  #define V4L2_CAMERA_ORIENTATION_FRONT          0
> > > --
> > > 2.30.0
> > >
> >
> > I think we have to add the CID to v4l2_ctrl_get_name()
>
> Sounds good. Only this one - V4L2_CID_REGION_OF_INTEREST_AUTO, right?

I think so :)

Thanks!



-- 
Ricardo Ribalda