Le mercredi 18 mai 2022 à 14:19 +0900, Tomasz Figa a écrit :
> Hi Nicolas,
>
> On Tue, May 17, 2022 at 4:02 AM Nicolas Dufresne <nicolas@xxxxxxxxxxxx> wrote:
> >
> > Hi,
> >
> > thanks for working on this, see my comments below ...
> >
> > Le lundi 16 mai 2022 à 23:04 +0900, Yunke Cao a écrit :
> > > Including:
> > > 1. Add a control ID.
> > > 2. Add p_rect to struct v4l2_ext_control with basic support in
> > > v4l2-ctrls.
> > >
> > > Signed-off-by: Yunke Cao <yunkec@xxxxxxxxxx>
> > > ---
> > > .../media/v4l/ext-ctrls-camera.rst | 4 ++++
> > > .../media/v4l/vidioc-g-ext-ctrls.rst | 4 ++++
> > > .../media/videodev2.h.rst.exceptions | 1 +
> > > drivers/media/v4l2-core/v4l2-ctrls-core.c | 20 +++++++++++++++++++
> > > drivers/media/v4l2-core/v4l2-ctrls-defs.c | 4 ++++
> > > include/media/v4l2-ctrls.h | 2 ++
> > > include/uapi/linux/v4l2-controls.h | 2 ++
> > > include/uapi/linux/videodev2.h | 2 ++
> > > 8 files changed, 39 insertions(+)
> > >
> > > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> > > index 4c5061aa9cd4..86a1f09a8a1c 100644
> > > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> > > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> > > @@ -661,3 +661,7 @@ enum v4l2_scene_mode -
> > > .. [#f1]
> > > This control may be changed to a menu control in the future, if more
> > > options are required.
> > > +
> > > +``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.
> >
> > This control documentation is missing some important information. Notably, what
> > will happen if this rectangle is set ? Is there a value to unset it ?
> >
>
> Thanks for the review!
>
> In V4L2 all the controls are always set to something. There is no way
> to unset a control. Since controls have default values, perhaps the
> way to "unset" (or rather reset it to the initial configuration) is to
> set it to the default value?
That's why a lot of similar controls comes with a companion boolean control to
enable/disable the feature, or an enum that specify what else is to be used,
could be an "automatic" default or something.
>
> > The name is very generic and I would expect that to be usable in general. But it
> > won't work for encoders, as you only allow 1 rectangle and it would be missing
> > some QP delta parameter. I think I would prefer if we specialize this type of
> > control a bit more. In your case, I'm guessing you only care about 1 ROI when
> > taking a picture, and this ROI will be used for automatic focus. If my guess is
> > right, perhaps a FOCUS_AERA could be a better name ?
>
> I wonder if an array control is what we need here to make it flexible
> enough? For a UVC camera obviously we would only need 1 element, but
> other devices could accept more.
See Hans and Laurent's comment on V3. They don't believe camera region of
interest is likely to exists outside of UVC, and suggested making all these
control entirely UVC specific, which also allow making it 1:1 with UVC without
any headache.
And this way we don't need to think about encoder ROI just yet.
>
> Best regards,
> Tomasz
>
> >
> > regards,
> > Nicolas
> >
> > > diff --git a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> > > index 29971a45a2d4..f4e205ead0a2 100644
> > > --- a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> > > +++ b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> > > @@ -189,6 +189,10 @@ still cause this situation.
> > > - ``p_area``
> > > - A pointer to a struct :c:type:`v4l2_area`. Valid if this control is
> > > of type ``V4L2_CTRL_TYPE_AREA``.
> > > + * - struct :c:type:`v4l2_rect` *
> > > + - ``p_area``
> > > + - A pointer to a struct :c:type:`v4l2_rect`. Valid if this control is
> > > + of type ``V4L2_CTRL_TYPE_RECT``.
> > > * - struct :c:type:`v4l2_ctrl_h264_sps` *
> > > - ``p_h264_sps``
> > > - A pointer to a struct :c:type:`v4l2_ctrl_h264_sps`. Valid if this control is
> > > diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> > > index 9cbb7a0c354a..7b423475281d 100644
> > > --- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> > > +++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> > > @@ -147,6 +147,7 @@ replace symbol V4L2_CTRL_TYPE_HEVC_SPS :c:type:`v4l2_ctrl_type`
> > > replace symbol V4L2_CTRL_TYPE_HEVC_PPS :c:type:`v4l2_ctrl_type`
> > > replace symbol V4L2_CTRL_TYPE_HEVC_SLICE_PARAMS :c:type:`v4l2_ctrl_type`
> > > replace symbol V4L2_CTRL_TYPE_AREA :c:type:`v4l2_ctrl_type`
> > > +replace symbol V4L2_CTRL_TYPE_RECT :c:type:`v4l2_ctrl_type`
> > > replace symbol V4L2_CTRL_TYPE_FWHT_PARAMS :c:type:`v4l2_ctrl_type`
> > > replace symbol V4L2_CTRL_TYPE_VP8_FRAME :c:type:`v4l2_ctrl_type`
> > > replace symbol V4L2_CTRL_TYPE_VP9_COMPRESSED_HDR :c:type:`v4l2_ctrl_type`
> > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> > > index 8968cec8454e..dcde405c2713 100644
> > > --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c
> > > +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> > > @@ -84,6 +84,11 @@ static bool std_equal(const struct v4l2_ctrl *ctrl, u32 idx,
> > > return ptr1.p_u16[idx] == ptr2.p_u16[idx];
> > > case V4L2_CTRL_TYPE_U32:
> > > return ptr1.p_u32[idx] == ptr2.p_u32[idx];
> > > + case V4L2_CTRL_TYPE_RECT:
> > > + return ptr1.p_rect->top == ptr2.p_rect->top &&
> > > + ptr1.p_rect->left == ptr2.p_rect->left &&
> > > + ptr1.p_rect->height == ptr2.p_rect->height &&
> > > + ptr1.p_rect->width == ptr2.p_rect->width;
> > > default:
> > > if (ctrl->is_int)
> > > return ptr1.p_s32[idx] == ptr2.p_s32[idx];
> > > @@ -307,6 +312,11 @@ static void std_log(const struct v4l2_ctrl *ctrl)
> > > case V4L2_CTRL_TYPE_VP9_FRAME:
> > > pr_cont("VP9_FRAME");
> > > break;
> > > + case V4L2_CTRL_TYPE_RECT:
> > > + pr_cont("l: %d, t: %d, w: %u, h: %u",
> > > + ptr.p_rect->left, ptr.p_rect->top,
> > > + ptr.p_rect->width, ptr.p_rect->height);
> > > + break;
> > > default:
> > > pr_cont("unknown type %d", ctrl->type);
> > > break;
> > > @@ -525,6 +535,7 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
> > > struct v4l2_ctrl_hdr10_mastering_display *p_hdr10_mastering;
> > > struct v4l2_ctrl_hevc_decode_params *p_hevc_decode_params;
> > > struct v4l2_area *area;
> > > + struct v4l2_rect *rect;
> > > void *p = ptr.p + idx * ctrl->elem_size;
> > > unsigned int i;
> > >
> > > @@ -888,6 +899,12 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
> > > return -EINVAL;
> > > break;
> > >
> > > + case V4L2_CTRL_TYPE_RECT:
> > > + rect = p;
> > > + if (!rect->width || !rect->height)
> > > + return -EINVAL;
> > > + break;
> > > +
> > > default:
> > > return -EINVAL;
> > > }
> > > @@ -1456,6 +1473,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
> > > case V4L2_CTRL_TYPE_AREA:
> > > elem_size = sizeof(struct v4l2_area);
> > > break;
> > > + case V4L2_CTRL_TYPE_RECT:
> > > + elem_size = sizeof(struct v4l2_rect);
> > > + break;
> > > default:
> > > if (type < V4L2_CTRL_COMPOUND_TYPES)
> > > elem_size = sizeof(s32);
> > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> > > index 54ca4e6b820b..95f39a2d2ad2 100644
> > > --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> > > +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> > > @@ -1042,6 +1042,7 @@ const char *v4l2_ctrl_get_name(u32 id)
> > > case V4L2_CID_UNIT_CELL_SIZE: return "Unit Cell Size";
> > > 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";
> > >
> > > /* FM Radio Modulator controls */
> > > /* Keep the order of the 'case's the same as in v4l2-controls.h! */
> > > @@ -1524,6 +1525,9 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
> > > case V4L2_CID_COLORIMETRY_HDR10_MASTERING_DISPLAY:
> > > *type = V4L2_CTRL_TYPE_HDR10_MASTERING_DISPLAY;
> > > break;
> > > + case V4L2_CID_REGION_OF_INTEREST_RECT:
> > > + *type = V4L2_CTRL_TYPE_RECT;
> > > + break;
> > > default:
> > > *type = V4L2_CTRL_TYPE_INTEGER;
> > > break;
> > > diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> > > index b3ce438f1329..919e104de50b 100644
> > > --- a/include/media/v4l2-ctrls.h
> > > +++ b/include/media/v4l2-ctrls.h
> > > @@ -58,6 +58,7 @@ struct video_device;
> > > * @p_hdr10_cll: Pointer to an HDR10 Content Light Level structure.
> > > * @p_hdr10_mastering: Pointer to an HDR10 Mastering Display structure.
> > > * @p_area: Pointer to an area.
> > > + * @p_rect: Pointer to a rectangle.
> > > * @p: Pointer to a compound value.
> > > * @p_const: Pointer to a constant compound value.
> > > */
> > > @@ -87,6 +88,7 @@ union v4l2_ctrl_ptr {
> > > struct v4l2_ctrl_hdr10_cll_info *p_hdr10_cll;
> > > struct v4l2_ctrl_hdr10_mastering_display *p_hdr10_mastering;
> > > struct v4l2_area *p_area;
> > > + struct v4l2_rect *p_rect;
> > > void *p;
> > > const void *p_const;
> > > };
> > > diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> > > index bb40129446d4..499fcddb6254 100644
> > > --- a/include/uapi/linux/v4l2-controls.h
> > > +++ b/include/uapi/linux/v4l2-controls.h
> > > @@ -1008,6 +1008,8 @@ 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)
> > > +
> > > /* FM Modulator class control IDs */
> > >
> > > #define V4L2_CID_FM_TX_CLASS_BASE (V4L2_CTRL_CLASS_FM_TX | 0x900)
> > > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> > > index 3768a0a80830..b712412cf763 100644
> > > --- a/include/uapi/linux/videodev2.h
> > > +++ b/include/uapi/linux/videodev2.h
> > > @@ -1751,6 +1751,7 @@ struct v4l2_ext_control {
> > > __u16 __user *p_u16;
> > > __u32 __user *p_u32;
> > > struct v4l2_area __user *p_area;
> > > + struct v4l2_rect __user *p_rect;
> > > struct v4l2_ctrl_h264_sps __user *p_h264_sps;
> > > struct v4l2_ctrl_h264_pps *p_h264_pps;
> > > struct v4l2_ctrl_h264_scaling_matrix __user *p_h264_scaling_matrix;
> > > @@ -1810,6 +1811,7 @@ enum v4l2_ctrl_type {
> > > V4L2_CTRL_TYPE_U16 = 0x0101,
> > > V4L2_CTRL_TYPE_U32 = 0x0102,
> > > V4L2_CTRL_TYPE_AREA = 0x0106,
> > > + V4L2_CTRL_TYPE_RECT = 0x0107,
> > >
> > > V4L2_CTRL_TYPE_HDR10_CLL_INFO = 0x0110,
> > > V4L2_CTRL_TYPE_HDR10_MASTERING_DISPLAY = 0x0111,
> >
