linux-kernel - Re: [PATCH 1/2] media: v4l2-ctrls: Add average qp control

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date: Mon, 6 May 2024 10:28:01 +0800
From: ming qian <ming.qian@....nxp.com>
To: Nicolas Dufresne <nicolas@...fresne.ca>,
 Hans Verkuil <hverkuil-cisco@...all.nl>, Ming Qian <ming.qian@....com>,
 mchehab@...nel.org
Cc: shawnguo@...nel.org, robh+dt@...nel.org, s.hauer@...gutronix.de,
 kernel@...gutronix.de, festevam@...il.com, linux-imx@....com,
 xiahong.bao@....com, eagle.zhou@....com, tao.jiang_2@....com,
 imx@...ts.linux.dev, linux-media@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 1/2] media: v4l2-ctrls: Add average qp control

Hi Nicolas,

> Le mercredi 24 avril 2024 à 12:03 +0200, Hans Verkuil a écrit :
>> On 29/03/2024 10:23, Ming Qian wrote:
>>> Add a control V4L2_CID_MPEG_VIDEO_AVERAGE_QP to report the average qp
>>> value of current encoded frame.
>>>
>>> Signed-off-by: Ming Qian <ming.qian@....com>
>>> ---
>>>   Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 4 ++++
>>>   drivers/media/v4l2-core/v4l2-ctrls-defs.c                 | 5 +++++
>>>   include/uapi/linux/v4l2-controls.h                        | 2 ++
>>>   3 files changed, 11 insertions(+)
>>>
>>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>> index 2a165ae063fb..cef20b3f54ca 100644
>>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>> @@ -1653,6 +1653,10 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
>>>       Quantization parameter for a P frame for FWHT. Valid range: from 1
>>>       to 31.
>>>   
>>> +``V4L2_CID_MPEG_VIDEO_AVERAGE_QP (integer)``
>>> +    This read-only control returns the average qp value of the currently
>>> +    encoded frame. Applicable to the H264 and HEVC encoders.
>>
>> qp -> QP
>>
>> Why is this applicable to H264/HEVC only? I think it is fine for any codec.
> 
> Same question, though we should document the range, which will differ per-
> codecs. We should also document to always use the specified range, rather then a
> HW custom range. This way we don't endup with issues we have hit with other ill-
> defined controls.
> 
> Nicolas

Thanks for the tip, I will add a description of the value range in V3
patch.


> 
>>
>> This needs to document that the value applies to the last dequeued buffer
>> (VIDIOC_DQBUF).
>>
>>> +
>>>   .. raw:: latex
>>>   
>>>       \normalsize
>>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>>> index 8696eb1cdd61..88e86e4e539d 100644
>>> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>>> @@ -972,6 +972,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>>>   	case V4L2_CID_MPEG_VIDEO_USE_LTR_FRAMES:		return "Use LTR Frames";
>>>   	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";
>>> +	case V4L2_CID_MPEG_VIDEO_AVERAGE_QP:			return "Average QP value";
>>
>> value -> Value
>>
>> Also move it up two lines so that it follows V4L2_CID_MPEG_VIDEO_USE_LTR_FRAMES
>> rather than FWHT controls.
>>
>>>   
>>>   	/* VPX controls */
>>>   	case V4L2_CID_MPEG_VIDEO_VPX_NUM_PARTITIONS:		return "VPX Number of Partitions";
>>> @@ -1507,6 +1508,10 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>>>   		*max = 0xffffffffffffLL;
>>>   		*step = 1;
>>>   		break;
>>> +	case V4L2_CID_MPEG_VIDEO_AVERAGE_QP:
>>> +		*type = V4L2_CTRL_TYPE_INTEGER;
>>> +		*flags |= V4L2_CTRL_FLAG_VOLATILE | V4L2_CTRL_FLAG_READ_ONLY;
>>
>> Drop the volatile flag, this isn't a volatile value.
>>
>>> +		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 99c3f5e99da7..974fd254e573 100644
>>> --- a/include/uapi/linux/v4l2-controls.h
>>> +++ b/include/uapi/linux/v4l2-controls.h
>>> @@ -898,6 +898,8 @@ enum v4l2_mpeg_video_av1_level {
>>>   	V4L2_MPEG_VIDEO_AV1_LEVEL_7_3 = 23
>>>   };
>>>   
>>> +#define V4L2_CID_MPEG_VIDEO_AVERAGE_QP  (V4L2_CID_CODEC_BASE + 657)
>>> +
>>>   /*  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)
>>
>> Regards,
>>
>> 	Hans
>>
>