[PATCH v2] media: v4l2-ctrl: add control for long term reference.

[PATCH v2] media: v4l2-ctrl: add control for long term reference.

[Dikshita Agarwal]

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@codeaurora.org>
---
 .../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)``
+	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.
+
+``V4L2_CID_MPEG_VIDEO_MARKLTRFRAME (enum)``
+	This control is used to mark current frame as Long Term Reference
+	frame.
+	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.
+
+``V4L2_CID_MPEG_VIDEO_USELTRFRAME (enum)``
+	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.
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";
 
 	/* 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)
-- 
1.9.1

Re: [PATCH v2] media: v4l2-ctrl: add control for long term reference.

[Hans Verkuil]

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@codeaurora.org>
> ---
>  .../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

Re: [PATCH v2] media: v4l2-ctrl: add control for long term reference.

[dikshita]

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@codeaurora.org>
>> ---
>>  .../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

Re: [PATCH v2] media: v4l2-ctrl: add control for long term reference.

[Hans Verkuil]

On 27/08/2020 08:59, dikshita@codeaurora.org wrote:
> 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@codeaurora.org>
>>> ---
>>>  .../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 ?

V4L2_CID_MPEG_VIDEO_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 isn't how requests work. If a control is not set in a request, then it is assumed
that the control value is unchanged. So in this case, the previously set value for this
control will be used.

Regards,

	Hans

>>> +    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