Re: [PATCH v2] media: v4l2-ctrl: add control for long term reference.
From: dikshita
Date: Thu Aug 27 2020 - 02:59:29 EST
Hi Hans,
Thanks for your comments.
On 2020-08-25 15:34, Hans Verkuil wrote:
On 14/08/2020 07:29, Dikshita Agarwal wrote:
LTR (Long Term Reference) frames are the frames that are encoded
sometime in the past and stored in the DPB buffer list to be used
as reference to encode future frames.
This change adds controls to enable this feature.
Signed-off-by: Dikshita Agarwal <dikshita@xxxxxxxxxxxxxx>
---
.../userspace-api/media/v4l/ext-ctrls-codec.rst | 23
++++++++++++++++++++++
drivers/media/v4l2-core/v4l2-ctrls.c | 6 ++++++
include/uapi/linux/v4l2-controls.h | 4 ++++
3 files changed, 33 insertions(+)
diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
index d0d506a..6d1b005 100644
--- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
@@ -4272,3 +4272,26 @@ enum v4l2_mpeg_video_hevc_size_of_length_field
-
- Selecting this value specifies that HEVC slices are expected
to be prefixed by Annex B start codes. According to
:ref:`hevc`
valid start codes can be 3-bytes 0x000001 or 4-bytes
0x00000001.
+
+``V4L2_CID_MPEG_VIDEO_LTRCOUNT (enum)``
I prefer _LTR_COUNT (same for the other control defines).
I assume 'enum' is a mistake? This should be 'integer', right?
Right, I will correct it at all the places.
+ Specifies the number of Long Term Reference frames encoder needs to
+ generate or keep.
+ This control is used to query or configure the number of Long Term
+ Reference frames.
Add something like: "Applicable to the H264 and HEVC encoder."
Sure.
+
+``V4L2_CID_MPEG_VIDEO_MARKLTRFRAME (enum)``
+ This control is used to mark current frame as Long Term Reference
+ frame.
enum -> integer
_MARK_LTR_FRAME
How about renaming this to: "_FRAME_LTR_INDEX"?
You are suggesting to rename it to V4L2_CID_MPEG_VIDEO_MARK_LTR_FRAME or
V4L2_CID_MPEG_VIDEO_MARK_FRAME_LTR_INDEX ?
I would also suggest having the range as 0..LTR_COUNT where 0 means
that
this is not a LTR frame. An alternative is to have two controls: one
boolean
that determines if the frame is a LTR frame or not, and one control
containing
the LTR index.
Is the LTR index 0 or 1 based according to the standard? I think that
if it is
1 based you can use 0 to mean 'not an LTR frame'. If it is 0 based in
the standard,
then having two controls might be better.
A third alternative might be to use -1 as the value to indicate that it
is not
an LTR frame, but it feels hackish. I'm not sure yet.
Could you please help me to understand how this info will be helpful?
This control won't be set by client for every frame. So a frame for
which this control is not set
is not a LTR frame and a frame for which this control is set is a LTR
frame and will be marked with
LTR index ranging from 0 to LTR_COUNT-1 (range is according to standard)
+ this provides a Long Term Reference index that ranges from 0
+ to LTR count-1 and then the particular frame will be marked with
that
+ Long Term Reference index.
Add something like: "Applicable to the H264 and HEVC encoder."
sure, will add.
This only makes sense when used with requests, right? Otherwise you
cannot
reliably associate this control with a frame. That should be mentioned
here.
Sure, will add.
+
+``V4L2_CID_MPEG_VIDEO_USELTRFRAME (enum)``
enum -> bitmask
_USE_LTR_FRAMES
Will correct this in next patch.
+ Specifies the Long Term Reference frame(s) to be used for encoding
+ the current frame.
+ This provides a bitmask which consists of bits [0, 15]. A total of N
+ LSB bits of this field are valid, where N is the maximum number of
+ Long Term Reference frames supported.
+ All the other bits are invalid and should be rejected.
+ The LSB corresponds to the Long Term Reference index 0. Bit N-1 from
+ the LSB corresponds to the Long Term Reference index max LTR
count-1.
Add something like: "Applicable to the H264 and HEVC encoder."
This too only makes sense when using requests, correct? That should be
mentioned
here.
Sure, will do.
I assume that this must be set to 0 for LTR frames? Or at least this
control will
be ignored for LTR frames.
Yes, frames marked as LTR frames will not be encoded by using any other
LTR frame as a reference.
So this control won't be set for LTR frames.
This will be set only for the frames which needs to be encoded by using
an LTR frame as a reference.
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
b/drivers/media/v4l2-core/v4l2-ctrls.c
index 3f3fbcd..3138c72 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -991,6 +991,9 @@ const char *v4l2_ctrl_get_name(u32 id)
case V4L2_CID_MPEG_VIDEO_HEVC_SLICE_PARAMS: return "HEVC Slice
Parameters";
case V4L2_CID_MPEG_VIDEO_HEVC_DECODE_MODE: return "HEVC Decode
Mode";
case V4L2_CID_MPEG_VIDEO_HEVC_START_CODE: return "HEVC Start Code";
+ case V4L2_CID_MPEG_VIDEO_LTRCOUNT: return "LTR Count";
+ case V4L2_CID_MPEG_VIDEO_MARKLTRFRAME: return "Mark LTR";
+ case V4L2_CID_MPEG_VIDEO_USELTRFRAME: return "Use LTR";
"Use LTR Frames"
/* CAMERA controls */
/* Keep the order of the 'case's the same as in v4l2-controls.h! */
@@ -1224,6 +1227,9 @@ void v4l2_ctrl_fill(u32 id, const char **name,
enum v4l2_ctrl_type *type,
break;
case V4L2_CID_MPEG_VIDEO_MV_H_SEARCH_RANGE:
case V4L2_CID_MPEG_VIDEO_MV_V_SEARCH_RANGE:
+ case V4L2_CID_MPEG_VIDEO_LTRCOUNT:
+ case V4L2_CID_MPEG_VIDEO_MARKLTRFRAME:
+ case V4L2_CID_MPEG_VIDEO_USELTRFRAME:
*type = V4L2_CTRL_TYPE_INTEGER;
break;
case V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME:
diff --git a/include/uapi/linux/v4l2-controls.h
b/include/uapi/linux/v4l2-controls.h
index 6227141..f2daa86 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -742,6 +742,10 @@ enum
v4l2_cid_mpeg_video_hevc_size_of_length_field {
#define
V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L6_BR (V4L2_CID_MPEG_BASE + 642)
#define
V4L2_CID_MPEG_VIDEO_REF_NUMBER_FOR_PFRAMES (V4L2_CID_MPEG_BASE + 643)
#define V4L2_CID_MPEG_VIDEO_PREPEND_SPSPPS_TO_IDR (V4L2_CID_MPEG_BASE
+ 644)
+#define V4L2_CID_MPEG_VIDEO_LTRCOUNT (V4L2_CID_MPEG_BASE + 645)
+#define V4L2_CID_MPEG_VIDEO_MARKLTRFRAME (V4L2_CID_MPEG_BASE + 646)
+#define V4L2_CID_MPEG_VIDEO_USELTRFRAME (V4L2_CID_MPEG_BASE + 647)
+
/* MPEG-class control IDs specific to the CX2341x driver as defined
by V4L2 */
#define V4L2_CID_MPEG_CX2341X_BASE (V4L2_CTRL_CLASS_MPEG | 0x1000)
Regards,
Hans
Thanks,
Dikshita