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

[ming qian]

Hi Nicolas,

On 4/5/24 02:14, Nicolas Dufresne wrote:
> Hi,
>
> Le vendredi 29 mars 2024 à 18:23 +0900, Ming Qian a écrit :
> > 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@xxxxxxx>
> > ---
> >   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.
>
> That seems ambiguous at best. What does it mean the "currently encoded frame" ?
> The OUTPUT and CAPTURE queue can be holding multiple frames. For "per frame"
> accurate reporting, I feel like we'd need something like Hans' read-only
> requests proposal [0]. Its basically a mechanism that let you attach request FD
> to capture buffer, so that supported controls can be saved per v4l2-buffer and
> read later on.
>
> https://patches.linaro.org/project/linux-media/patch/20210610113615.785359-12-hverkuil-cisco@xxxxxxxxx/
>
> If this isn't what you wanted, we'll need a better definition. It might be
> helpful to explain what this is used for.
>
> Nicolas

Yes, I want to report the qp value for every frame.
I thought the request FD is only used for stateless decoder, but I want
to add a read-only ctrl for the stateful encoder. So I checked the
defined read-only ctrls, I think it's similar with
V4L2_CID_MPEG_VIDEO_DEC_PTS.
(https://linuxtv.org/downloads/v4l-dvb-apis/userspace-api/v4l/ext-ctrls-codec.html?highlight=v4l2_cid_mpeg_video_dec_pts)

then back to your question about the "currently encoded frame", it's the
last dequeued capture buffer of the encoder, the capture queue can hold
multiple frames, each frame will have a qp value in this case, and this
ctrl only report the qp value of the last dequeued frame, when user has
dequeued an encoded frame from the capture queue, he can get the ctrl
value of V4L2_CID_MPEG_VIDEO_AVERAGE_QP immediately to get the qp value
of the currently dequeued frame. If user doesn't care about this
parameter, he doesn't need to do anything, it's just the same as before.
so I think this ctrl is backward compatible.

Maybe the request FD is a better and more intuitive way to suggest a
one-to-one correspondence between ctrl and frame. I'm just not sure if
it just applies to the stateless decoder. I did find any stateful
decoder or encoder to use them.
If we use the request FD for this stateful encoder, I'm not sure if it
will break the original flow.

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..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";
> >   
> >   	/* 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;
> > +		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)