Le mercredi 18 mai 2022 à 10:11 +0900, Yunke Cao a écrit :
> Thanks, I will add detailed behavior in the follow up version.
>
> On Tue, May 17, 2022 at 10:23 PM Nicolas Dufresne <nicolas@xxxxxxxxxxxx> wrote:
> >
> > Le lundi 16 mai 2022 à 23:04 +0900, Yunke Cao a écrit :
> > > Signed-off-by: Yunke Cao <yunkec@xxxxxxxxxx>
> > > ---
> > > .../media/v4l/ext-ctrls-camera.rst | 25 +++++++++++++++++++
> > > drivers/media/v4l2-core/v4l2-ctrls-defs.c | 2 ++
> > > include/uapi/linux/v4l2-controls.h | 9 +++++++
> > > 3 files changed, 36 insertions(+)
> > >
> > > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> > > index 86a1f09a8a1c..3da66e1e1fc7 100644
> > > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> > > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> > > @@ -665,3 +665,28 @@ enum v4l2_scene_mode -
> > > ``V4L2_CID_REGION_OF_INTEREST_RECT (struct)``
> > > This control determines the region of interest. Region of interest is an
> > > rectangular area represented by a struct v4l2_rect.
> > > +
> > > +``V4L2_CID_REGION_OF_INTEREST_AUTO (bitmask)``
> > > + This determines which, if any, on board features should track to the
> > > + Region of Interest.
> > > +
> > > +.. flat-table::
> > > + :header-rows: 0
> > > + :stub-columns: 0
> > > +
> > > + * - ``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_STABILIZATION``
> > > + - Image Stabilization.
> > > + * - ``V4L2_CID_REGION_OF_INTEREST_AUTO_HIGHER_QUALITY``
> > > + - Higher Quality.
> >
> > Now I see the usage, the control is missing cross-reference and behaviour.
> > Consider that someone may have to use or implement your feature on different HW
> > and in different context in the future. Right now you aren't writing a
> > specification, but barely listing things that are already encoded in the item
> > names. For each of this, add human readable prose that explain what is expected
> > behaviour when the bit is set. This way, future implementation can check their
> > behaviour and cross-over with the documentation to make sure it is a fit, or if
> > another bit need to be allocated.
> >
> > I still believe REGION_OF_INTEREST is too generic of a name for the purpose, as
> > in the context of the V4L2 API, we also support video encoders, and none of this
> > (perhaps except QUALITY, but with encoder you have to specify a delta for that).
> > The name really needs to be specialized to be implemented this way. Otherwise,
> > it create confusion, and makes the V4L2 uAPI poorer over time. Naming is hard,
> > but I need to make a suggestion, perhaps CAMERA_ROI ? We have classes, perhaps a
> > class for the CAMERA controls is needed ?
> >
> > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> > > index 95f39a2d2ad2..f28763bf95e9 100644
> > > --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> > > +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> > > @@ -1043,6 +1043,7 @@ const char *v4l2_ctrl_get_name(u32 id)
> > > case V4L2_CID_CAMERA_ORIENTATION: return "Camera Orientation";
> > > case V4L2_CID_CAMERA_SENSOR_ROTATION: return "Camera Sensor Rotation";
> > > case V4L2_CID_REGION_OF_INTEREST_RECT: return "Region Of Interest Rectangle";
> > > + case V4L2_CID_REGION_OF_INTEREST_AUTO: return "Region Of Interest Auto Controls";
> > >
> > > /* FM Radio Modulator controls */
> > > /* Keep the order of the 'case's the same as in v4l2-controls.h! */
> > > @@ -1415,6 +1416,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
> > > case V4L2_CID_JPEG_ACTIVE_MARKER:
> > > case V4L2_CID_3A_LOCK:
> > > case V4L2_CID_AUTO_FOCUS_STATUS:
> > > + case V4L2_CID_REGION_OF_INTEREST_AUTO:
> > > case V4L2_CID_DV_TX_HOTPLUG:
> > > case V4L2_CID_DV_TX_RXSENSE:
> > > case V4L2_CID_DV_TX_EDID_PRESENT:
> > > diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> > > index 499fcddb6254..f6938e4de129 100644
> > > --- a/include/uapi/linux/v4l2-controls.h
> > > +++ b/include/uapi/linux/v4l2-controls.h
> > > @@ -1009,6 +1009,15 @@ enum v4l2_auto_focus_range {
> > > #define V4L2_CID_CAMERA_SENSOR_ROTATION (V4L2_CID_CAMERA_CLASS_BASE+35)
> > >
> > > #define V4L2_CID_REGION_OF_INTEREST_RECT (V4L2_CID_CAMERA_CLASS_BASE+36)
> > > +#define V4L2_CID_REGION_OF_INTEREST_AUTO (V4L2_CID_CAMERA_CLASS_BASE+37)
>
> The ROI controls are in V4L2_CID_CAMERA_CLASS
> "(V4L2_CID_CAMERA_CLASS_BASE+36)" unless I miss anything.
> Correct me if I'm wrong but I don't see "CAMERA" included in the name
> of other camera controls.
Namespace should be extended when its obviously going to clash with other
similar concept. In this case it obviously clash with video encoder ROI.
regards,
Nicolas
>
> > > +#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_STABILIZATION (1 << 6)
> > > +#define V4L2_CID_REGION_OF_INTEREST_AUTO_HIGHER_QUALITY (1 << 7)
> > >
> > > /* FM Modulator class control IDs */
> > >
> >
