Message-ID: <abf15705-01de-45f1-a869-49f96c9b82b9@oss.nxp.com>
Date: Wed, 30 Oct 2024 17:14:00 +0800
From: "Ming Qian(OSS)" <ming.qian@....nxp.com>
To: Hans Verkuil <hverkuil-cisco@...all.nl>, mchehab@...nel.org
Cc: yunkec@...gle.com, nicolas@...fresne.ca, s.hauer@...gutronix.de,
 kernel@...gutronix.de, festevam@...il.com, linux-imx@....com,
 xiahong.bao@....com, ming.zhou@....com, eagle.zhou@....com,
 tao.jiang_2@....com, ming.qian@....com, imx@...ts.linux.dev,
 linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org
Subject: Re: [RFC v4 4/4] media: v4l2-ctrls: Add video encoder ROI ctrls

Hi Hans,

On 2024/10/30 15:32, Hans Verkuil wrote:
> Hi Ming Qian,
> 
> Thank you for this, these descriptions are much better!
> 
> I still have a few comments, though. See below.
> 
> On 30/10/2024 03:21, ming.qian@....nxp.com wrote:
>> From: Ming Qian <ming.qian@....com>
>>
>> Add some ctrls to support the video encoder region-of-interest(ROI)
>> feature. The ROI QP Map and rectangular configuration are supported.
>>
>> Signed-off-by: Ming Qian <ming.qian@....com>
>> Signed-off-by: TaoJiang <tao.jiang_2@....com>
>> ---
>>   .../media/v4l/ext-ctrls-codec.rst             | 131 ++++++++++++++++++
>>   drivers/media/v4l2-core/v4l2-ctrls-defs.c     |  46 ++++++
>>   include/uapi/linux/v4l2-controls.h            |  16 +++
>>   3 files changed, 193 insertions(+)
>>
>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>> index 4a379bd9e3fb..7b4327c4a14e 100644
>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>> @@ -1667,6 +1667,137 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
>>       Codecs need to always use the specified range, rather then a HW custom range.
>>       Applicable to encoders
>>   
>> +``V4L2_CID_MPEG_VIDEO_ROI_MODE``
>> +    (enum)
>> +
>> +enum v4l2_mpeg_video_roi_mode -
>> +    Indicates Video Encoding region-of-interest (ROI) Mode, enable different
>> +    encoding strategies based on selected areas within a frame. Choosing a
>> +    specific mode activates corresponding control or combinations of controls
>> +    to configure the ROI, e.g. for rectangle delta QP mode, you need to provide
>> +    V4L2_CID_MPEG_VIDEO_ROI_RECT and V4L2_MPEG_VIDEO_ROI_MODE_RECT_DELTA_QP.
>> +    Applicable to encoders.
>> +
>> +    Possible values are:
>> +
>> +
>> +.. flat-table::
>> +    :header-rows:  0
>> +    :stub-columns: 0
>> +
>> +    * - ``V4L2_MPEG_VIDEO_ROI_MODE_NONE``
>> +      - No ROI in the MPEG stream. Default mode.
>> +    * - ``V4L2_MPEG_VIDEO_ROI_MODE_RECT_DELTA_QP``
>> +      - Rectangle ROI mode and specifies ROI delta QP.
>> +        When enabled, define one or more rectangular ROI areas
>> +        where QP delta is applied. The control
>> +        V4L2_CID_MPEG_VIDEO_ROI_RECT is used to set ROI areas
>> +        and V4L2_MPEG_VIDEO_ROI_MODE_RECT_DELTA_QP is used to
>> +        set QP delta value.
>> +    * - ``V4L2_MPEG_VIDEO_ROI_MODE_RECT_PRIORITY``
>> +      - Rectangle ROI mode and specifies ROI priority.
>> +        When enabled, define one or more rectangular ROI areas
>> +        where priority is applied. The control
>> +        V4L2_CID_MPEG_VIDEO_ROI_RECT is used to set ROI areas
>> +        and V4L2_MPEG_VIDEO_ROI_MODE_RECT_PRIORITY is used to
>> +        set priority.
>> +    * - ``V4L2_MPEG_VIDEO_ROI_MODE_MAP_DELTA_QP``
>> +      - Map ROI mode and specifies ROI delta QP.
>> +        When enabled, the whole frame is configured into a grid map
>> +        according to the ROI block region size in raster scan order,
>> +        and each region can have a QP delta applied.
>> +        The control V4L2_CID_MPEG_VIDEO_ROI_MAP_DELTA_QP
>> +        is used to configure the ROI Map with QP delta values.
>> +    * - ``V4L2_MPEG_VIDEO_ROI_MODE_MAP_ABSOLUTE_QP``
>> +      - Map ROI mode and specifies ROI absolute QP value.
>> +        When enabled, configure a grid ROI map where each region
>> +        has an absolute QP value assigned. The control
>> +        V4L2_CID_MPEG_VIDEO_ROI_MAP_ABSOLUTE_QP is used to
>> +        configure the ROI Map with absolute QP values.
>> +
>> +``V4L2_CID_MPEG_VIDEO_ROI_BLOCK_SIZE (struct)``
>> +    This read-only control returns the ROI block size in pixels. The struct
>> +    :c:type:`v4l2_area` provides the width and height in separate
>> +    fields. The resolution of the frame will be rounded up to be
>> +    aligned to this value when it's partitioned in blocks for QP
>> +    maps and the number of QP values in those maps will be the
>> +    number of blocks of these indicated pixel size that comprise
>> +    a full frame. This control depends on the encoding format,
>> +    and the detailed encoder hardware.
>> +    Applicable to encoders.
>> +
>> +``V4L2_CID_MPEG_VIDEO_ROI_RECT (struct)``
>> +    A struct :c:type:`v4l2_rect` provides the rectangular region
>> +    described by the position of its top-left corner, the width
>> +    and the height. The unit is in pixels. And it should be aligned
>> +    to the ROI block size, which can be queried from
>> +    ``V4L2_CID_MPEG_VIDEO_ROI_BLOCK_SIZE``. If it is not aligned to
>> +    the ROI block size, then X,Y gets rounded down, and width/height up.
>> +    The maximum number of rectangular regions depends on the hardware.
> 
> What happens with rectangles that are completely outside the frame?
> I assume those will be ignored?
> 
>> +    This control is a dynamically sized array.
>> +    This control is applicable when ``V4L2_CID_MPEG_VIDEO_ROI_MODE``
>> +    value is ``V4L2_MPEG_VIDEO_ROI_MODE_RECT_DELTA_QP`` or
>> +    ``V4L2_MPEG_VIDEO_ROI_MODE_RECT_PRIORITY``. For overlapping
>> +    regions, the value that is first in the ROI array will have priority.
>> +    Applicable to encoders.
>> +
>> +``V4L2_MPEG_VIDEO_ROI_MODE_RECT_DELTA_QP (integer)``
>> +    Specifies the ROI delta QP of a rectangular region. The delta QP
>> +    is the value that will be added on top of the frame level QP.
>> +    It can be positive (more distortion) or negative (less distortion)
>> +    values. This control is applicable when
>> +    ``V4L2_CID_MPEG_VIDEO_ROI_MODE`` value is
>> +    ``V4L2_MPEG_VIDEO_ROI_MODE_RECT_DELTA_QP``, and must be used
>> +    in combination with ``V4L2_CID_MPEG_VIDEO_ROI_RECT``.
>> +    This control is a dynamically sized array, and the array size
>> +    should match ``V4L2_CID_MPEG_VIDEO_ROI_RECT``. If not match,
>> +    the number of configured regions is the minimum value.
> 
> I'd rephrase this:
> 
> "If the array sizes do not match, then use the smallest size for
> both controls and ignore the remaining elements in the larger array."
> 

Yes, it's better, I'll apply it

>> +    Applicable to encoders.
>> +
>> +``V4L2_MPEG_VIDEO_ROI_MODE_RECT_PRIORITY (integer)``
>> +    Specifies the ROI priority of a rectangular region. it can
>> +    be positive (more important) or negative (less important)
>> +    values and is compared with non-ROI region (taken as value 0).
>> +    This control is applicable when ``V4L2_CID_MPEG_VIDEO_ROI_MODE``
>> +    value is ``V4L2_MPEG_VIDEO_ROI_MODE_RECT_PRIORITY``, and must
>> +    be used in combination with ``V4L2_CID_MPEG_VIDEO_ROI_RECT``.
>> +    This control is a dynamically sized array, and the array size
>> +    should match ``V4L2_CID_MPEG_VIDEO_ROI_RECT``. If not match,
>> +    the number of configured regions is the minimum value.
> 
> Same here.
> 
>> +    Applicable to encoders.
>> +
>> +``V4L2_CID_MPEG_VIDEO_ROI_MAP_DELTA_QP (integer)``
>> +    Configure a frame-wide grid map that divides the entire
>> +    frame into blocks of the ROI block size, where each
>> +    region will have a QP delta applied. The ROI map is
>> +    arranged in raster scan order, and it's configured
>> +    as an integer array. The block size can be got from
>> +    ``V4L2_CID_MPEG_VIDEO_ROI_BLOCK_SIZE``. This control is a
>> +    dynamically sized array, the array size can be calculated
>> +    from video resolution and the ROI block size, and the
>> +    width and height should be rounded up to be aligned to
>> +    the block size. This control is applicable when
>> +    ``V4L2_CID_MPEG_VIDEO_ROI_MODE`` value is
>> +    ``V4L2_CID_MPEG_VIDEO_ROI_MAP_DELTA_QP``.
>> +    If the array size is too small, the control should
>> +    return error. Applicable to encoders.
> 
> "If the array size is too small, the control should return error."
> 
> That doesn't really work. You can set the control, then change the
> video format causing the control array size to be too small, but
> that won't return an error.
> 
> I think you should either ignore the control completely in case
> of a mismatch, or assume that the remainder of the grid map will
> use a delta QP of 0. The latter would be my preference.
> 

I agree, But I prefer to ignore the control in case of mismatch.
When there is a mismatch in array size, it's not just a case of
missing configure data, the array size may be calculated from a
incorrect frame size. Since the array represents a grid map,
the defined area may not align with the intended target area.
So, I think it's better to ignore it completely and give some
tips log.

>> +
>> +``V4L2_CID_MPEG_VIDEO_ROI_MAP_ABSOLUTE_QP (integer)``
>> +    Configure a frame-wide grid map that divides the entire
>> +    frame into blocks of the ROI block size, where each
>> +    region will have an absolute QP applied. The ROI map is
>> +    arranged in raster scan order, and it's configured
>> +    as an integer array. The block size can be got from
>> +    ``V4L2_CID_MPEG_VIDEO_ROI_BLOCK_SIZE``. This control is a
>> +    dynamically sized array, the array size can be calculated
>> +    from video resolution and the ROI block size, and the
>> +    width and height should be rounded up to be aligned to
>> +    the block size. This control is applicable when
>> +    ``V4L2_CID_MPEG_VIDEO_ROI_MODE`` value is
>> +    ``V4L2_CID_MPEG_VIDEO_ROI_MAP_ABSOLUTE_QP``.
>> +    If the array size is too small, the control should
>> +    return error. Applicable to encoders.
> 
> You have the same problem here. Except that you can't use a QP
> value of 0 if the array is too short, but you can probably substitute
> the value from one of the existing codec-specific QP controls.
> 
> If that doesn't work, then ignoring arrays that are too short
> is probably the better approach.

I would like to ignore the control if the array size is mismatch.

> 
> Regards,
> 
> 	Hans
> 
>> +
>>   .. raw:: latex
>>   
>>       \normalsize
>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>> index 1ea52011247a..4d89309bf8d0 100644
>> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>> @@ -612,6 +612,15 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>>   		NULL,
>>   	};
>>   
>> +	static const char * const mpeg_video_roi_mode[] = {
>> +		"None",
>> +		"Rectangle Delta QP",
>> +		"Rectangle Priority",
>> +		"Map Delta QP",
>> +		"Map Absolute QP",
>> +		NULL,
>> +	};
>> +
>>   	switch (id) {
>>   	case V4L2_CID_MPEG_AUDIO_SAMPLING_FREQ:
>>   		return mpeg_audio_sampling_freq;
>> @@ -750,6 +759,8 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>>   		return camera_orientation;
>>   	case V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_PERIOD_TYPE:
>>   		return intra_refresh_period_type;
>> +	case V4L2_CID_MPEG_VIDEO_ROI_MODE:
>> +		return mpeg_video_roi_mode;
>>   	default:
>>   		return NULL;
>>   	}
>> @@ -971,6 +982,13 @@ const char *v4l2_ctrl_get_name(u32 id)
>>   	case V4L2_CID_MPEG_VIDEO_FRAME_LTR_INDEX:		return "Frame LTR Index";
>>   	case V4L2_CID_MPEG_VIDEO_USE_LTR_FRAMES:		return "Use LTR Frames";
>>   	case V4L2_CID_MPEG_VIDEO_AVERAGE_QP:			return "Average QP Value";
>> +	case V4L2_CID_MPEG_VIDEO_ROI_MODE:		return "Video ROI Mode";
>> +	case V4L2_CID_MPEG_VIDEO_ROI_BLOCK_SIZE:	return "Video ROI Block Size";
>> +	case V4L2_CID_MPEG_VIDEO_ROI_RECT:		return "Video ROI Rectangle Region";
>> +	case V4L2_CID_MPEG_VIDEO_ROI_RECT_DELTA_QP:	return "Video ROI Rectangle Delta QP";
>> +	case V4L2_CID_MPEG_VIDEO_ROI_RECT_PRIORITY:	return "Video ROI Rectangle Priority";
>> +	case V4L2_CID_MPEG_VIDEO_ROI_MAP_DELTA_QP:	return "Video ROI Delta QP Map";
>> +	case V4L2_CID_MPEG_VIDEO_ROI_MAP_ABSOLUTE_QP:	return "Video ROI Absolute QP Map";
>>   	case V4L2_CID_FWHT_I_FRAME_QP:				return "FWHT I-Frame QP Value";
>>   	case V4L2_CID_FWHT_P_FRAME_QP:				return "FWHT P-Frame QP Value";
>>   
>> @@ -1512,6 +1530,34 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>>   		*type = V4L2_CTRL_TYPE_INTEGER;
>>   		*flags |= V4L2_CTRL_FLAG_READ_ONLY;
>>   		break;
>> +	case V4L2_CID_MPEG_VIDEO_ROI_MODE:
>> +		*type = V4L2_CTRL_TYPE_MENU;
>> +		*flags |= V4L2_CTRL_FLAG_UPDATE;
>> +		break;
>> +	case V4L2_CID_MPEG_VIDEO_ROI_BLOCK_SIZE:
>> +		*type = V4L2_CTRL_TYPE_AREA;
>> +		*flags |= V4L2_CTRL_FLAG_READ_ONLY;
>> +		break;
>> +	case V4L2_CID_MPEG_VIDEO_ROI_RECT:
>> +		*type = V4L2_CTRL_TYPE_RECT;
>> +		*flags |= V4L2_CTRL_FLAG_DYNAMIC_ARRAY | V4L2_CTRL_FLAG_HAS_WHICH_MIN_MAX;
>> +		break;
>> +	case V4L2_CID_MPEG_VIDEO_ROI_RECT_DELTA_QP:
>> +		*type = V4L2_CTRL_TYPE_INTEGER;
>> +		*flags |= V4L2_CTRL_FLAG_DYNAMIC_ARRAY | V4L2_CTRL_FLAG_HAS_WHICH_MIN_MAX;
>> +		break;
>> +	case V4L2_CID_MPEG_VIDEO_ROI_RECT_PRIORITY:
>> +		*type = V4L2_CTRL_TYPE_INTEGER;
>> +		*flags |= V4L2_CTRL_FLAG_DYNAMIC_ARRAY | V4L2_CTRL_FLAG_HAS_WHICH_MIN_MAX;
>> +		break;
>> +	case V4L2_CID_MPEG_VIDEO_ROI_MAP_DELTA_QP:
>> +		*type = V4L2_CTRL_TYPE_INTEGER;
>> +		*flags |= V4L2_CTRL_FLAG_DYNAMIC_ARRAY | V4L2_CTRL_FLAG_HAS_WHICH_MIN_MAX;
>> +		break;
>> +	case V4L2_CID_MPEG_VIDEO_ROI_MAP_ABSOLUTE_QP:
>> +		*type = V4L2_CTRL_TYPE_INTEGER;
>> +		*flags |= V4L2_CTRL_FLAG_DYNAMIC_ARRAY | V4L2_CTRL_FLAG_HAS_WHICH_MIN_MAX;
>> +		break;
>>   	case V4L2_CID_PIXEL_RATE:
>>   		*type = V4L2_CTRL_TYPE_INTEGER64;
>>   		*flags |= V4L2_CTRL_FLAG_READ_ONLY;
>> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
>> index 974fd254e573..bf67d53af737 100644
>> --- a/include/uapi/linux/v4l2-controls.h
>> +++ b/include/uapi/linux/v4l2-controls.h
>> @@ -900,6 +900,22 @@ enum v4l2_mpeg_video_av1_level {
>>   
>>   #define V4L2_CID_MPEG_VIDEO_AVERAGE_QP  (V4L2_CID_CODEC_BASE + 657)
>>   
>> +enum v4l2_mpeg_video_roi_mode {
>> +	V4L2_MPEG_VIDEO_ROI_MODE_NONE,
>> +	V4L2_MPEG_VIDEO_ROI_MODE_RECT_DELTA_QP,
>> +	V4L2_MPEG_VIDEO_ROI_MODE_RECT_PRIORITY,
>> +	V4L2_MPEG_VIDEO_ROI_MODE_MAP_DELTA_QP,
>> +	V4L2_MPEG_VIDEO_ROI_MODE_MAP_ABSOLUTE_QP
>> +};
>> +
>> +#define V4L2_CID_MPEG_VIDEO_ROI_MODE			(V4L2_CID_CODEC_BASE + 658)
>> +#define V4L2_CID_MPEG_VIDEO_ROI_BLOCK_SIZE		(V4L2_CID_CODEC_BASE + 659)
>> +#define V4L2_CID_MPEG_VIDEO_ROI_RECT			(V4L2_CID_CODEC_BASE + 660)
>> +#define V4L2_CID_MPEG_VIDEO_ROI_RECT_DELTA_QP		(V4L2_CID_CODEC_BASE + 661)
>> +#define V4L2_CID_MPEG_VIDEO_ROI_RECT_PRIORITY		(V4L2_CID_CODEC_BASE + 662)
>> +#define V4L2_CID_MPEG_VIDEO_ROI_MAP_DELTA_QP		(V4L2_CID_CODEC_BASE + 663)
>> +#define V4L2_CID_MPEG_VIDEO_ROI_MAP_ABSOLUTE_QP		(V4L2_CID_CODEC_BASE + 664)
>> +
>>   /*  MPEG-class control IDs specific to the CX2341x driver as defined by V4L2 */
>>   #define V4L2_CID_CODEC_CX2341X_BASE				(V4L2_CTRL_CLASS_CODEC | 0x1000)
>>   #define V4L2_CID_MPEG_CX2341X_VIDEO_SPATIAL_FILTER_MODE		(V4L2_CID_CODEC_CX2341X_BASE+0)
> 

Powered by blists - more mailing lists