From: ming qian <ming.qian@oss.nxp.com>
To: Andrzej Pietrasiewicz <andrzej.p@collabora.com>,
Ming Qian <ming.qian@nxp.com>,
mchehab@kernel.org, hverkuil-cisco@xs4all.nl
Cc: shawnguo@kernel.org, robh+dt@kernel.org, s.hauer@pengutronix.de,
kernel@pengutronix.de, festevam@gmail.com, linux-imx@nxp.com,
xiahong.bao@nxp.com, eagle.zhou@nxp.com, tao.jiang_2@nxp.com,
imx@lists.linux.dev, linux-media@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v2 1/3] media: v4l2-ctrls: Add average qp control
Date: Tue, 30 Apr 2024 16:43:39 +0800 [thread overview]
Message-ID: <c1580fc5-457d-465b-b533-37bda8c69181@oss.nxp.com> (raw)
In-Reply-To: <a839b292-6776-4ecb-8d8a-a25e58816787@collabora.com>
Hi Andrzej,
> Hi Ming,
>
> W dniu 30.04.2024 o 04:20, ming qian pisze:
>>
>> Hi Andrzej,
>>
>>> Hi Ming Qian,
>>>
>>> W dniu 25.04.2024 o 08:50, Ming Qian pisze:
>>>> Add a control V4L2_CID_MPEG_VIDEO_AVERAGE_QP to report the average qp
>>>> value of current encoded frame. the value applies to the last dequeued
>>>> capture buffer.
>>>>
>>>> Signed-off-by: Ming Qian <ming.qian@nxp.com>
>>>> ---
>>>> v2
>>>> - improve document description according Hans's comments
>>>> - drop volatile flag
>>>>
>>>> Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 5 +++++
>>>> drivers/media/v4l2-core/v4l2-ctrls-defs.c | 5 +++++
>>>> include/uapi/linux/v4l2-controls.h | 2 ++
>>>> 3 files changed, 12 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..7d82ab14b8ba 100644
>>>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>>> @@ -1653,6 +1653,11 @@ 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. The value applies to the last dequeued capture
>>>> buffer
>>>> + (VIDIOC_DQBUF). Applicable to encoders.
>>>> +
>>>
>>> What is the intended range of the values? And why? Is your intention
>>> to make it
>>> a hardware-agnostic control or a hardware-specific control?
>>>
>>> Regrds,
>>>
>>> Andrzej
>>>
>>
>> The value range depends on the format,
>> For H264, it's [V4L2_CID_MPEG_VIDEO_H264_MIN_QP,
>> V4L2_CID_MPEG_VIDEO_H264_MIN_QP],
>> usually this is [0, 51].
>> For HEVC, it's [V4L2_CID_MPEG_VIDEO_HEVC_MIN_QP,
>> V4L2_CID_MPEG_VIDEO_HEVC_MAX_QP],
>> usually it's from 0 to 51 for 8 bit and from 0 to 63 for 10 bit.
>> For H263 and MPEG4, it may be from 1 to 31.
>> For VPX, it may be from 1 to 128.
>>
>> I think the driver that supports this ctrl can set an appropriate value
>> range based on the format it supports.
>>
>> Users also need to read the value of this ctrl according to the format
>> they set.
>
> What happens if the user does not set the format or until they set it?
>
> This looks like a contextual control, where the context is a cross
> product between setting or not setting the format and which particular
> format is actually set.
>
> I don't want to voice strong opinions about whether this should or
> should not be accepted, but this kind of behavior should definitely
> be documented so that driver writers know how to implement this control.
> Right now reading the documentation in the *.rst file as a driver writer
> I would be puzzled. So, from a driver writer's perspective I'd expect
> these additions to the doc:
>
> - the range of returned values
> - the expected behavior when format has not been set
> - the expected behavior when no frames have been processed yet (so
> there's no QP
> to be read back from the hardware)
>
> Regards,
>
> Andrzej
>
The ctrl value only applies to the last dequeued capture buffer. It
requires that the format has been set and the streaming has been
started. Before that, the value is meaningless, and the driver can set a
default value by itself.
I agree that it's better to add the value range description in the
document, but as discussed above, it's not a fixed value, maybe I can
add some description according to format.
Best regards,
Ming
>>
>> Best regards,
>> Ming
>>
>>>> .. 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..1ea52011247a 100644
>>>> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>>>> @@ -970,6 +970,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>>>> case V4L2_CID_MPEG_VIDEO_LTR_COUNT: return "LTR
>>>> Count";
>>>> 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_FWHT_I_FRAME_QP: return "FWHT
>>>> I-Frame QP Value";
>>>> case V4L2_CID_FWHT_P_FRAME_QP: return "FWHT
>>>> P-Frame QP Value";
>>>> @@ -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_READ_ONLY;
>>>> + 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)
>>>
>
prev parent reply other threads:[~2024-04-30 8:37 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-25 6:50 [PATCH v2 1/3] media: v4l2-ctrls: Add average qp control Ming Qian
2024-04-25 6:50 ` [PATCH v2 2/3] media: amphion: Report the average QP of current encoded frame Ming Qian
2024-04-25 6:50 ` [PATCH v2 3/3] media: amphion: Remove lock in s_ctrl callback Ming Qian
2024-04-29 17:44 ` Andrzej Pietrasiewicz
2024-04-30 2:32 ` ming qian
2024-04-30 8:00 ` Andrzej Pietrasiewicz
2024-04-30 8:55 ` ming qian
2024-04-29 17:38 ` [PATCH v2 1/3] media: v4l2-ctrls: Add average qp control Andrzej Pietrasiewicz
2024-04-30 2:20 ` ming qian
2024-04-30 7:56 ` Andrzej Pietrasiewicz
2024-04-30 8:43 ` ming qian [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=c1580fc5-457d-465b-b533-37bda8c69181@oss.nxp.com \
--to=ming.qian@oss.nxp.com \
--cc=andrzej.p@collabora.com \
--cc=eagle.zhou@nxp.com \
--cc=festevam@gmail.com \
--cc=hverkuil-cisco@xs4all.nl \
--cc=imx@lists.linux.dev \
--cc=kernel@pengutronix.de \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-imx@nxp.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=mchehab@kernel.org \
--cc=ming.qian@nxp.com \
--cc=robh+dt@kernel.org \
--cc=s.hauer@pengutronix.de \
--cc=shawnguo@kernel.org \
--cc=tao.jiang_2@nxp.com \
--cc=xiahong.bao@nxp.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).