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?
> + 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."
> +
> +``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"?
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.
> + 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."
This only makes sense when used with requests, right? Otherwise you cannot
reliably associate this control with a frame. That should be mentioned here.
> +
> +``V4L2_CID_MPEG_VIDEO_USELTRFRAME (enum)``
enum -> bitmask
_USE_LTR_FRAMES
> + 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.
I assume that this must be set to 0 for LTR frames? Or at least this control will
be ignored for LTR frames.
> 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
