| Message ID | 20240329092352.2648837-1-ming.qian@nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] media: v4l2-ctrls: Add average qp control | expand |
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@nxp.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. > > 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@xs4all.nl/ > > 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) >
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@nxp.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. 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
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@nxp.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 > > 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 >
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@nxp.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 >> >
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. + .. 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)
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@nxp.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(+)