[PATCH] v4l2-ctrl: add control for thumnails

[PATCH] v4l2-ctrl: add control for thumnails

[Stanimir Varbanov]

Add v4l2 control for decoder thumbnail.

Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
---
 Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 7 +++++++
 drivers/media/v4l2-core/v4l2-ctrls.c                      | 2 ++
 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 d0d506a444b1..e838e410651b 100644
--- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
@@ -3726,6 +3726,13 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
     disables generating SPS and PPS at every IDR. Setting it to one enables
     generating SPS and PPS at every IDR.
 
+``V4L2_CID_MPEG_VIDEO_DECODER_THUMBNAIL (button)``
+    Instructs the decoder to produce immediate output. The decoder should
+    consume first input buffer for progressive stream (or first two buffers
+    for interlace). Decoder should not allocate more output buffers that it
+    is required to consume one input frame. Usually the decoder input
+    buffers will contain only I/IDR frames but it is not mandatory.
+
 .. _v4l2-mpeg-hevc:
 
 ``V4L2_CID_MPEG_VIDEO_HEVC_SPS (struct)``
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index b188577db40f..cb2554404c63 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -991,6 +991,7 @@ 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_DECODER_THUMBNAIL:		return "Thumbnail generation";
 
 	/* CAMERA controls */
 	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
@@ -1234,6 +1235,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 	case V4L2_CID_AUTO_FOCUS_START:
 	case V4L2_CID_AUTO_FOCUS_STOP:
 	case V4L2_CID_DO_WHITE_BALANCE:
+	case V4L2_CID_MPEG_VIDEO_DECODER_THUMBNAIL:
 		*type = V4L2_CTRL_TYPE_BUTTON;
 		*flags |= V4L2_CTRL_FLAG_WRITE_ONLY |
 			  V4L2_CTRL_FLAG_EXECUTE_ON_WRITE;
diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
index 62271418c1be..7e44a2779863 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -743,6 +743,8 @@ enum v4l2_cid_mpeg_video_hevc_size_of_length_field {
 #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_DECODER_THUMBNAIL		(V4L2_CID_MPEG_BASE + 645)
+
 /*  MPEG-class control IDs specific to the CX2341x driver as defined by V4L2 */
 #define V4L2_CID_MPEG_CX2341X_BASE				(V4L2_CTRL_CLASS_MPEG | 0x1000)
 #define V4L2_CID_MPEG_CX2341X_VIDEO_SPATIAL_FILTER_MODE		(V4L2_CID_MPEG_CX2341X_BASE+0)
-- 
2.17.1

Re: [PATCH] v4l2-ctrl: add control for thumnails

[Hans Verkuil]

On 26/05/2020 10:54, Stanimir Varbanov wrote:
> Add v4l2 control for decoder thumbnail.
> 
> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> ---
>  Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 7 +++++++
>  drivers/media/v4l2-core/v4l2-ctrls.c                      | 2 ++
>  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 d0d506a444b1..e838e410651b 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> @@ -3726,6 +3726,13 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
>      disables generating SPS and PPS at every IDR. Setting it to one enables
>      generating SPS and PPS at every IDR.
>  
> +``V4L2_CID_MPEG_VIDEO_DECODER_THUMBNAIL (button)``
> +    Instructs the decoder to produce immediate output. The decoder should
> +    consume first input buffer for progressive stream (or first two buffers
> +    for interlace). Decoder should not allocate more output buffers that it
> +    is required to consume one input frame. Usually the decoder input
> +    buffers will contain only I/IDR frames but it is not mandatory.

This is very vague. It doesn't explain why the control is called 'THUMBNAIL',
but more importantly it doesn't explain how this relates to normal decoding.

I.e. if you are decoding and 'press' this control, what happens then?

What exactly is the use-case?

Regards,

	Hans

> +
>  .. _v4l2-mpeg-hevc:
>  
>  ``V4L2_CID_MPEG_VIDEO_HEVC_SPS (struct)``
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index b188577db40f..cb2554404c63 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -991,6 +991,7 @@ 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_DECODER_THUMBNAIL:		return "Thumbnail generation";
>  
>  	/* CAMERA controls */
>  	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
> @@ -1234,6 +1235,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>  	case V4L2_CID_AUTO_FOCUS_START:
>  	case V4L2_CID_AUTO_FOCUS_STOP:
>  	case V4L2_CID_DO_WHITE_BALANCE:
> +	case V4L2_CID_MPEG_VIDEO_DECODER_THUMBNAIL:
>  		*type = V4L2_CTRL_TYPE_BUTTON;
>  		*flags |= V4L2_CTRL_FLAG_WRITE_ONLY |
>  			  V4L2_CTRL_FLAG_EXECUTE_ON_WRITE;
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index 62271418c1be..7e44a2779863 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -743,6 +743,8 @@ enum v4l2_cid_mpeg_video_hevc_size_of_length_field {
>  #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_DECODER_THUMBNAIL		(V4L2_CID_MPEG_BASE + 645)
> +
>  /*  MPEG-class control IDs specific to the CX2341x driver as defined by V4L2 */
>  #define V4L2_CID_MPEG_CX2341X_BASE				(V4L2_CTRL_CLASS_MPEG | 0x1000)
>  #define V4L2_CID_MPEG_CX2341X_VIDEO_SPATIAL_FILTER_MODE		(V4L2_CID_MPEG_CX2341X_BASE+0)
> 

Re: [PATCH] v4l2-ctrl: add control for thumnails

[Stanimir Varbanov]

Hi Hans,

On 5/26/20 3:04 PM, Hans Verkuil wrote:
> On 26/05/2020 10:54, Stanimir Varbanov wrote:
>> Add v4l2 control for decoder thumbnail.
>>
>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
>> ---
>>  Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 7 +++++++
>>  drivers/media/v4l2-core/v4l2-ctrls.c                      | 2 ++
>>  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 d0d506a444b1..e838e410651b 100644
>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>> @@ -3726,6 +3726,13 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
>>      disables generating SPS and PPS at every IDR. Setting it to one enables
>>      generating SPS and PPS at every IDR.
>>  
>> +``V4L2_CID_MPEG_VIDEO_DECODER_THUMBNAIL (button)``
>> +    Instructs the decoder to produce immediate output. The decoder should
>> +    consume first input buffer for progressive stream (or first two buffers
>> +    for interlace). Decoder should not allocate more output buffers that it
>> +    is required to consume one input frame. Usually the decoder input
>> +    buffers will contain only I/IDR frames but it is not mandatory.
> 
> This is very vague. It doesn't explain why the control is called 'THUMBNAIL',
> but more importantly it doesn't explain how this relates to normal decoding.

If in the normal decode the capture queue buffers are 5, in the
thumbnail mode the number of buffers will be only 1 (if the bitstream is
progressive) and this will guarantee low memory usage. The other
difference is that the decoder will produce decoded frames (without
errors) only for I/IDR (sync frames).

> 
> I.e. if you are decoding and 'press' this control, what happens then?

Might be the button type wasn't great idea. In fact the control should
be set before streamon so that the driver returns min_capture_bufs 1.

> 
> What exactly is the use-case?

It could be used to generate thumbnails of all video clips in a folder
or when you open a Gallery application on your phone.

> 
> Regards,
> 
> 	Hans
> 
>> +
>>  .. _v4l2-mpeg-hevc:
>>  
>>  ``V4L2_CID_MPEG_VIDEO_HEVC_SPS (struct)``
>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
>> index b188577db40f..cb2554404c63 100644
>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
>> @@ -991,6 +991,7 @@ 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_DECODER_THUMBNAIL:		return "Thumbnail generation";
>>  
>>  	/* CAMERA controls */
>>  	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
>> @@ -1234,6 +1235,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>>  	case V4L2_CID_AUTO_FOCUS_START:
>>  	case V4L2_CID_AUTO_FOCUS_STOP:
>>  	case V4L2_CID_DO_WHITE_BALANCE:
>> +	case V4L2_CID_MPEG_VIDEO_DECODER_THUMBNAIL:
>>  		*type = V4L2_CTRL_TYPE_BUTTON;
>>  		*flags |= V4L2_CTRL_FLAG_WRITE_ONLY |
>>  			  V4L2_CTRL_FLAG_EXECUTE_ON_WRITE;
>> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
>> index 62271418c1be..7e44a2779863 100644
>> --- a/include/uapi/linux/v4l2-controls.h
>> +++ b/include/uapi/linux/v4l2-controls.h
>> @@ -743,6 +743,8 @@ enum v4l2_cid_mpeg_video_hevc_size_of_length_field {
>>  #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_DECODER_THUMBNAIL		(V4L2_CID_MPEG_BASE + 645)
>> +
>>  /*  MPEG-class control IDs specific to the CX2341x driver as defined by V4L2 */
>>  #define V4L2_CID_MPEG_CX2341X_BASE				(V4L2_CTRL_CLASS_MPEG | 0x1000)
>>  #define V4L2_CID_MPEG_CX2341X_VIDEO_SPATIAL_FILTER_MODE		(V4L2_CID_MPEG_CX2341X_BASE+0)
>>
> 

-- 
regards,
Stan

Re: [PATCH] v4l2-ctrl: add control for thumnails

[Stanimir Varbanov]

Hi Hans,

On 5/27/20 12:53 AM, Stanimir Varbanov wrote:
> Hi Hans,
> 
> On 5/26/20 3:04 PM, Hans Verkuil wrote:
>> On 26/05/2020 10:54, Stanimir Varbanov wrote:
>>> Add v4l2 control for decoder thumbnail.
>>>
>>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
>>> ---
>>>  Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 7 +++++++
>>>  drivers/media/v4l2-core/v4l2-ctrls.c                      | 2 ++
>>>  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 d0d506a444b1..e838e410651b 100644
>>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>> @@ -3726,6 +3726,13 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
>>>      disables generating SPS and PPS at every IDR. Setting it to one enables
>>>      generating SPS and PPS at every IDR.
>>>  
>>> +``V4L2_CID_MPEG_VIDEO_DECODER_THUMBNAIL (button)``
>>> +    Instructs the decoder to produce immediate output. The decoder should
>>> +    consume first input buffer for progressive stream (or first two buffers
>>> +    for interlace). Decoder should not allocate more output buffers that it
>>> +    is required to consume one input frame. Usually the decoder input
>>> +    buffers will contain only I/IDR frames but it is not mandatory.
>>
>> This is very vague. It doesn't explain why the control is called 'THUMBNAIL',
>> but more importantly it doesn't explain how this relates to normal decoding.
> 
> If in the normal decode the capture queue buffers are 5, in the
> thumbnail mode the number of buffers will be only 1 (if the bitstream is
> progressive) and this will guarantee low memory usage. The other
> difference is that the decoder will produce decoded frames (without
> errors) only for I/IDR (sync frames).
> 
>>
>> I.e. if you are decoding and 'press' this control, what happens then?
> 
> Might be the button type wasn't great idea. In fact the control should
> be set before streamon so that the driver returns min_capture_bufs 1.
> 
>>
>> What exactly is the use-case?
> 
> It could be used to generate thumbnails of all video clips in a folder
> or when you open a Gallery application on your phone.
> 

What is your opinion on that control? I could consider to make it Venus
custom control but from the use-case it looks other drivers also can
benefit of it.

I tried to make more generic one [1] but it looks it will be too difficult.

-- 
regards,
Stan

[1] https://www.spinics.net/lists/linux-media/msg167794.html

Re: [PATCH] v4l2-ctrl: add control for thumnails

[Hans Verkuil]

On 04/06/2020 11:02, Stanimir Varbanov wrote:
> Hi Hans,
> 
> On 5/27/20 12:53 AM, Stanimir Varbanov wrote:
>> Hi Hans,
>>
>> On 5/26/20 3:04 PM, Hans Verkuil wrote:
>>> On 26/05/2020 10:54, Stanimir Varbanov wrote:
>>>> Add v4l2 control for decoder thumbnail.
>>>>
>>>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
>>>> ---
>>>>  Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 7 +++++++
>>>>  drivers/media/v4l2-core/v4l2-ctrls.c                      | 2 ++
>>>>  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 d0d506a444b1..e838e410651b 100644
>>>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>>> @@ -3726,6 +3726,13 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
>>>>      disables generating SPS and PPS at every IDR. Setting it to one enables
>>>>      generating SPS and PPS at every IDR.
>>>>  
>>>> +``V4L2_CID_MPEG_VIDEO_DECODER_THUMBNAIL (button)``
>>>> +    Instructs the decoder to produce immediate output. The decoder should
>>>> +    consume first input buffer for progressive stream (or first two buffers
>>>> +    for interlace). Decoder should not allocate more output buffers that it
>>>> +    is required to consume one input frame. Usually the decoder input
>>>> +    buffers will contain only I/IDR frames but it is not mandatory.
>>>
>>> This is very vague. It doesn't explain why the control is called 'THUMBNAIL',
>>> but more importantly it doesn't explain how this relates to normal decoding.
>>
>> If in the normal decode the capture queue buffers are 5, in the
>> thumbnail mode the number of buffers will be only 1 (if the bitstream is
>> progressive) and this will guarantee low memory usage. The other
>> difference is that the decoder will produce decoded frames (without
>> errors) only for I/IDR (sync frames).

Isn't this really a "DECODE_SYNC_FRAMES_ONLY" control? That's what it does,
right? Skip any B/P frames and only decode sync frames.

That this is useful for creating thumbnails is just a specific use-case.

Regards,

	Hans

>>
>>>
>>> I.e. if you are decoding and 'press' this control, what happens then?
>>
>> Might be the button type wasn't great idea. In fact the control should
>> be set before streamon so that the driver returns min_capture_bufs 1.
>>
>>>
>>> What exactly is the use-case?
>>
>> It could be used to generate thumbnails of all video clips in a folder
>> or when you open a Gallery application on your phone.
>>
> 
> What is your opinion on that control? I could consider to make it Venus
> custom control but from the use-case it looks other drivers also can
> benefit of it.
> 
> I tried to make more generic one [1] but it looks it will be too difficult.
> 

Re: [PATCH] v4l2-ctrl: add control for thumnails

[Stanimir Varbanov]

Hi Hans,

On 6/4/20 12:08 PM, Hans Verkuil wrote:
> On 04/06/2020 11:02, Stanimir Varbanov wrote:
>> Hi Hans,
>>
>> On 5/27/20 12:53 AM, Stanimir Varbanov wrote:
>>> Hi Hans,
>>>
>>> On 5/26/20 3:04 PM, Hans Verkuil wrote:
>>>> On 26/05/2020 10:54, Stanimir Varbanov wrote:
>>>>> Add v4l2 control for decoder thumbnail.
>>>>>
>>>>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
>>>>> ---
>>>>>  Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 7 +++++++
>>>>>  drivers/media/v4l2-core/v4l2-ctrls.c                      | 2 ++
>>>>>  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 d0d506a444b1..e838e410651b 100644
>>>>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>>>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>>>> @@ -3726,6 +3726,13 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
>>>>>      disables generating SPS and PPS at every IDR. Setting it to one enables
>>>>>      generating SPS and PPS at every IDR.
>>>>>  
>>>>> +``V4L2_CID_MPEG_VIDEO_DECODER_THUMBNAIL (button)``
>>>>> +    Instructs the decoder to produce immediate output. The decoder should
>>>>> +    consume first input buffer for progressive stream (or first two buffers
>>>>> +    for interlace). Decoder should not allocate more output buffers that it
>>>>> +    is required to consume one input frame. Usually the decoder input
>>>>> +    buffers will contain only I/IDR frames but it is not mandatory.
>>>>
>>>> This is very vague. It doesn't explain why the control is called 'THUMBNAIL',
>>>> but more importantly it doesn't explain how this relates to normal decoding.
>>>
>>> If in the normal decode the capture queue buffers are 5, in the
>>> thumbnail mode the number of buffers will be only 1 (if the bitstream is
>>> progressive) and this will guarantee low memory usage. The other
>>> difference is that the decoder will produce decoded frames (without
>>> errors) only for I/IDR (sync frames).
> 
> Isn't this really a "DECODE_SYNC_FRAMES_ONLY" control? That's what it does,
> right? Skip any B/P frames and only decode sync frames.

Yes, it is.
To me V4L2_CID_MPEG_VIDEO_DECODE_SYNC_FRAMES sounds better. If you are
fine I can send a new patch.

The definition of "sync frames" is a bit difficult for codec-agnostic
controls. Is it sound better "INTRA", DECODE_INTRA_FRAMES (ONLY)?

> 
> That this is useful for creating thumbnails is just a specific use-case.
> 
> Regards,
> 
> 	Hans
> 
>>>
>>>>
>>>> I.e. if you are decoding and 'press' this control, what happens then?
>>>
>>> Might be the button type wasn't great idea. In fact the control should
>>> be set before streamon so that the driver returns min_capture_bufs 1.
>>>
>>>>
>>>> What exactly is the use-case?
>>>
>>> It could be used to generate thumbnails of all video clips in a folder
>>> or when you open a Gallery application on your phone.
>>>
>>
>> What is your opinion on that control? I could consider to make it Venus
>> custom control but from the use-case it looks other drivers also can
>> benefit of it.
>>
>> I tried to make more generic one [1] but it looks it will be too difficult.
>>
> 

-- 
regards,
Stan

Re: [PATCH] v4l2-ctrl: add control for thumnails

[Hans Verkuil]

On 04/06/2020 14:34, Stanimir Varbanov wrote:
> Hi Hans,
> 
> On 6/4/20 12:08 PM, Hans Verkuil wrote:
>> On 04/06/2020 11:02, Stanimir Varbanov wrote:
>>> Hi Hans,
>>>
>>> On 5/27/20 12:53 AM, Stanimir Varbanov wrote:
>>>> Hi Hans,
>>>>
>>>> On 5/26/20 3:04 PM, Hans Verkuil wrote:
>>>>> On 26/05/2020 10:54, Stanimir Varbanov wrote:
>>>>>> Add v4l2 control for decoder thumbnail.
>>>>>>
>>>>>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
>>>>>> ---
>>>>>>  Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 7 +++++++
>>>>>>  drivers/media/v4l2-core/v4l2-ctrls.c                      | 2 ++
>>>>>>  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 d0d506a444b1..e838e410651b 100644
>>>>>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>>>>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>>>>> @@ -3726,6 +3726,13 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
>>>>>>      disables generating SPS and PPS at every IDR. Setting it to one enables
>>>>>>      generating SPS and PPS at every IDR.
>>>>>>  
>>>>>> +``V4L2_CID_MPEG_VIDEO_DECODER_THUMBNAIL (button)``
>>>>>> +    Instructs the decoder to produce immediate output. The decoder should
>>>>>> +    consume first input buffer for progressive stream (or first two buffers
>>>>>> +    for interlace). Decoder should not allocate more output buffers that it
>>>>>> +    is required to consume one input frame. Usually the decoder input
>>>>>> +    buffers will contain only I/IDR frames but it is not mandatory.
>>>>>
>>>>> This is very vague. It doesn't explain why the control is called 'THUMBNAIL',
>>>>> but more importantly it doesn't explain how this relates to normal decoding.
>>>>
>>>> If in the normal decode the capture queue buffers are 5, in the
>>>> thumbnail mode the number of buffers will be only 1 (if the bitstream is
>>>> progressive) and this will guarantee low memory usage. The other
>>>> difference is that the decoder will produce decoded frames (without
>>>> errors) only for I/IDR (sync frames).
>>
>> Isn't this really a "DECODE_SYNC_FRAMES_ONLY" control? That's what it does,
>> right? Skip any B/P frames and only decode sync frames.
> 
> Yes, it is.
> To me V4L2_CID_MPEG_VIDEO_DECODE_SYNC_FRAMES sounds better. If you are
> fine I can send a new patch.
> 
> The definition of "sync frames" is a bit difficult for codec-agnostic
> controls. Is it sound better "INTRA", DECODE_INTRA_FRAMES (ONLY)?

INTRA is better. DECODE_INTRA_FRAMES_ONLY is a good name, I think.

Thumbnail creation can be given as an example in the description of the
control, but that's just a use-case.

Regards,

	Hans

> 
>>
>> That this is useful for creating thumbnails is just a specific use-case.
>>
>> Regards,
>>
>> 	Hans
>>
>>>>
>>>>>
>>>>> I.e. if you are decoding and 'press' this control, what happens then?
>>>>
>>>> Might be the button type wasn't great idea. In fact the control should
>>>> be set before streamon so that the driver returns min_capture_bufs 1.
>>>>
>>>>>
>>>>> What exactly is the use-case?
>>>>
>>>> It could be used to generate thumbnails of all video clips in a folder
>>>> or when you open a Gallery application on your phone.
>>>>
>>>
>>> What is your opinion on that control? I could consider to make it Venus
>>> custom control but from the use-case it looks other drivers also can
>>> benefit of it.
>>>
>>> I tried to make more generic one [1] but it looks it will be too difficult.
>>>
>>
> 

Re: [PATCH] v4l2-ctrl: add control for thumnails

[Tomasz Figa]

On Thu, Jun 4, 2020 at 2:56 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>
> On 04/06/2020 14:34, Stanimir Varbanov wrote:
> > Hi Hans,
> >
> > On 6/4/20 12:08 PM, Hans Verkuil wrote:
> >> On 04/06/2020 11:02, Stanimir Varbanov wrote:
> >>> Hi Hans,
> >>>
> >>> On 5/27/20 12:53 AM, Stanimir Varbanov wrote:
> >>>> Hi Hans,
> >>>>
> >>>> On 5/26/20 3:04 PM, Hans Verkuil wrote:
> >>>>> On 26/05/2020 10:54, Stanimir Varbanov wrote:
> >>>>>> Add v4l2 control for decoder thumbnail.
> >>>>>>
> >>>>>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> >>>>>> ---
> >>>>>>  Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 7 +++++++
> >>>>>>  drivers/media/v4l2-core/v4l2-ctrls.c                      | 2 ++
> >>>>>>  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 d0d506a444b1..e838e410651b 100644
> >>>>>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> >>>>>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> >>>>>> @@ -3726,6 +3726,13 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
> >>>>>>      disables generating SPS and PPS at every IDR. Setting it to one enables
> >>>>>>      generating SPS and PPS at every IDR.
> >>>>>>
> >>>>>> +``V4L2_CID_MPEG_VIDEO_DECODER_THUMBNAIL (button)``
> >>>>>> +    Instructs the decoder to produce immediate output. The decoder should
> >>>>>> +    consume first input buffer for progressive stream (or first two buffers
> >>>>>> +    for interlace). Decoder should not allocate more output buffers that it
> >>>>>> +    is required to consume one input frame. Usually the decoder input
> >>>>>> +    buffers will contain only I/IDR frames but it is not mandatory.
> >>>>>
> >>>>> This is very vague. It doesn't explain why the control is called 'THUMBNAIL',
> >>>>> but more importantly it doesn't explain how this relates to normal decoding.
> >>>>
> >>>> If in the normal decode the capture queue buffers are 5, in the
> >>>> thumbnail mode the number of buffers will be only 1 (if the bitstream is
> >>>> progressive) and this will guarantee low memory usage. The other
> >>>> difference is that the decoder will produce decoded frames (without
> >>>> errors) only for I/IDR (sync frames).
> >>
> >> Isn't this really a "DECODE_SYNC_FRAMES_ONLY" control? That's what it does,
> >> right? Skip any B/P frames and only decode sync frames.
> >
> > Yes, it is.
> > To me V4L2_CID_MPEG_VIDEO_DECODE_SYNC_FRAMES sounds better. If you are
> > fine I can send a new patch.
> >
> > The definition of "sync frames" is a bit difficult for codec-agnostic
> > controls. Is it sound better "INTRA", DECODE_INTRA_FRAMES (ONLY)?
>
> INTRA is better. DECODE_INTRA_FRAMES_ONLY is a good name, I think.
>
> Thumbnail creation can be given as an example in the description of the
> control, but that's just a use-case.

How about the other aspect of the behavior?

"Decoder should not allocate more output buffers that it
is required to consume one input frame."

Best regards,
Tomasz

>
> Regards,
>
>         Hans
>
> >
> >>
> >> That this is useful for creating thumbnails is just a specific use-case.
> >>
> >> Regards,
> >>
> >>      Hans
> >>
> >>>>
> >>>>>
> >>>>> I.e. if you are decoding and 'press' this control, what happens then?
> >>>>
> >>>> Might be the button type wasn't great idea. In fact the control should
> >>>> be set before streamon so that the driver returns min_capture_bufs 1.
> >>>>
> >>>>>
> >>>>> What exactly is the use-case?
> >>>>
> >>>> It could be used to generate thumbnails of all video clips in a folder
> >>>> or when you open a Gallery application on your phone.
> >>>>
> >>>
> >>> What is your opinion on that control? I could consider to make it Venus
> >>> custom control but from the use-case it looks other drivers also can
> >>> benefit of it.
> >>>
> >>> I tried to make more generic one [1] but it looks it will be too difficult.
> >>>
> >>
> >
>

Re: [PATCH] v4l2-ctrl: add control for thumnails

[Stanimir Varbanov]

Hi,

On 6/4/20 3:57 PM, Tomasz Figa wrote:
> On Thu, Jun 4, 2020 at 2:56 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>>
>> On 04/06/2020 14:34, Stanimir Varbanov wrote:
>>> Hi Hans,
>>>
>>> On 6/4/20 12:08 PM, Hans Verkuil wrote:
>>>> On 04/06/2020 11:02, Stanimir Varbanov wrote:
>>>>> Hi Hans,
>>>>>
>>>>> On 5/27/20 12:53 AM, Stanimir Varbanov wrote:
>>>>>> Hi Hans,
>>>>>>
>>>>>> On 5/26/20 3:04 PM, Hans Verkuil wrote:
>>>>>>> On 26/05/2020 10:54, Stanimir Varbanov wrote:
>>>>>>>> Add v4l2 control for decoder thumbnail.
>>>>>>>>
>>>>>>>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
>>>>>>>> ---
>>>>>>>>  Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 7 +++++++
>>>>>>>>  drivers/media/v4l2-core/v4l2-ctrls.c                      | 2 ++
>>>>>>>>  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 d0d506a444b1..e838e410651b 100644
>>>>>>>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>>>>>>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>>>>>>> @@ -3726,6 +3726,13 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
>>>>>>>>      disables generating SPS and PPS at every IDR. Setting it to one enables
>>>>>>>>      generating SPS and PPS at every IDR.
>>>>>>>>
>>>>>>>> +``V4L2_CID_MPEG_VIDEO_DECODER_THUMBNAIL (button)``
>>>>>>>> +    Instructs the decoder to produce immediate output. The decoder should
>>>>>>>> +    consume first input buffer for progressive stream (or first two buffers
>>>>>>>> +    for interlace). Decoder should not allocate more output buffers that it
>>>>>>>> +    is required to consume one input frame. Usually the decoder input
>>>>>>>> +    buffers will contain only I/IDR frames but it is not mandatory.
>>>>>>>
>>>>>>> This is very vague. It doesn't explain why the control is called 'THUMBNAIL',
>>>>>>> but more importantly it doesn't explain how this relates to normal decoding.
>>>>>>
>>>>>> If in the normal decode the capture queue buffers are 5, in the
>>>>>> thumbnail mode the number of buffers will be only 1 (if the bitstream is
>>>>>> progressive) and this will guarantee low memory usage. The other
>>>>>> difference is that the decoder will produce decoded frames (without
>>>>>> errors) only for I/IDR (sync frames).
>>>>
>>>> Isn't this really a "DECODE_SYNC_FRAMES_ONLY" control? That's what it does,
>>>> right? Skip any B/P frames and only decode sync frames.
>>>
>>> Yes, it is.
>>> To me V4L2_CID_MPEG_VIDEO_DECODE_SYNC_FRAMES sounds better. If you are
>>> fine I can send a new patch.
>>>
>>> The definition of "sync frames" is a bit difficult for codec-agnostic
>>> controls. Is it sound better "INTRA", DECODE_INTRA_FRAMES (ONLY)?
>>
>> INTRA is better. DECODE_INTRA_FRAMES_ONLY is a good name, I think.
>>
>> Thumbnail creation can be given as an example in the description of the
>> control, but that's just a use-case.
> 
> How about the other aspect of the behavior?
> 
> "Decoder should not allocate more output buffers that it
> is required to consume one input frame."
> 

In fact I have to refine this; It looks like that picture type decode vs
thumbnail are two different modes.

Thumbnail mode - first frame decode without additional memory (one input
buffer and one output buffer). The first frame can be even non-intra
frame as well. Also no matter frame parser is sending, the decoder will
try to produce output for thumbnail generation.

> Best regards,
> Tomasz
> 
>>
>> Regards,
>>
>>         Hans
>>
>>>
>>>>
>>>> That this is useful for creating thumbnails is just a specific use-case.
>>>>
>>>> Regards,
>>>>
>>>>      Hans
>>>>
>>>>>>
>>>>>>>
>>>>>>> I.e. if you are decoding and 'press' this control, what happens then?
>>>>>>
>>>>>> Might be the button type wasn't great idea. In fact the control should
>>>>>> be set before streamon so that the driver returns min_capture_bufs 1.
>>>>>>
>>>>>>>
>>>>>>> What exactly is the use-case?
>>>>>>
>>>>>> It could be used to generate thumbnails of all video clips in a folder
>>>>>> or when you open a Gallery application on your phone.
>>>>>>
>>>>>
>>>>> What is your opinion on that control? I could consider to make it Venus
>>>>> custom control but from the use-case it looks other drivers also can
>>>>> benefit of it.
>>>>>
>>>>> I tried to make more generic one [1] but it looks it will be too difficult.
>>>>>
>>>>
>>>
>>

-- 
regards,
Stan

Re: [PATCH] v4l2-ctrl: add control for thumnails

[Tomasz Figa]

On Tue, Oct 13, 2020 at 2:52 PM Stanimir Varbanov
<stanimir.varbanov@linaro.org> wrote:
>
> Hi,
>
> On 6/4/20 3:57 PM, Tomasz Figa wrote:
> > On Thu, Jun 4, 2020 at 2:56 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
> >>
> >> On 04/06/2020 14:34, Stanimir Varbanov wrote:
> >>> Hi Hans,
> >>>
> >>> On 6/4/20 12:08 PM, Hans Verkuil wrote:
> >>>> On 04/06/2020 11:02, Stanimir Varbanov wrote:
> >>>>> Hi Hans,
> >>>>>
> >>>>> On 5/27/20 12:53 AM, Stanimir Varbanov wrote:
> >>>>>> Hi Hans,
> >>>>>>
> >>>>>> On 5/26/20 3:04 PM, Hans Verkuil wrote:
> >>>>>>> On 26/05/2020 10:54, Stanimir Varbanov wrote:
> >>>>>>>> Add v4l2 control for decoder thumbnail.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> >>>>>>>> ---
> >>>>>>>>  Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 7 +++++++
> >>>>>>>>  drivers/media/v4l2-core/v4l2-ctrls.c                      | 2 ++
> >>>>>>>>  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 d0d506a444b1..e838e410651b 100644
> >>>>>>>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> >>>>>>>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> >>>>>>>> @@ -3726,6 +3726,13 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
> >>>>>>>>      disables generating SPS and PPS at every IDR. Setting it to one enables
> >>>>>>>>      generating SPS and PPS at every IDR.
> >>>>>>>>
> >>>>>>>> +``V4L2_CID_MPEG_VIDEO_DECODER_THUMBNAIL (button)``
> >>>>>>>> +    Instructs the decoder to produce immediate output. The decoder should
> >>>>>>>> +    consume first input buffer for progressive stream (or first two buffers
> >>>>>>>> +    for interlace). Decoder should not allocate more output buffers that it
> >>>>>>>> +    is required to consume one input frame. Usually the decoder input
> >>>>>>>> +    buffers will contain only I/IDR frames but it is not mandatory.
> >>>>>>>
> >>>>>>> This is very vague. It doesn't explain why the control is called 'THUMBNAIL',
> >>>>>>> but more importantly it doesn't explain how this relates to normal decoding.
> >>>>>>
> >>>>>> If in the normal decode the capture queue buffers are 5, in the
> >>>>>> thumbnail mode the number of buffers will be only 1 (if the bitstream is
> >>>>>> progressive) and this will guarantee low memory usage. The other
> >>>>>> difference is that the decoder will produce decoded frames (without
> >>>>>> errors) only for I/IDR (sync frames).
> >>>>
> >>>> Isn't this really a "DECODE_SYNC_FRAMES_ONLY" control? That's what it does,
> >>>> right? Skip any B/P frames and only decode sync frames.
> >>>
> >>> Yes, it is.
> >>> To me V4L2_CID_MPEG_VIDEO_DECODE_SYNC_FRAMES sounds better. If you are
> >>> fine I can send a new patch.
> >>>
> >>> The definition of "sync frames" is a bit difficult for codec-agnostic
> >>> controls. Is it sound better "INTRA", DECODE_INTRA_FRAMES (ONLY)?
> >>
> >> INTRA is better. DECODE_INTRA_FRAMES_ONLY is a good name, I think.
> >>
> >> Thumbnail creation can be given as an example in the description of the
> >> control, but that's just a use-case.
> >
> > How about the other aspect of the behavior?
> >
> > "Decoder should not allocate more output buffers that it
> > is required to consume one input frame."
> >
>
> In fact I have to refine this; It looks like that picture type decode vs
> thumbnail are two different modes.
>
> Thumbnail mode - first frame decode without additional memory (one input
> buffer and one output buffer). The first frame can be even non-intra
> frame as well.

How comes it can be decoded without additional memory to store the
reference frames then?

> Also no matter frame parser is sending, the decoder will
> try to produce output for thumbnail generation.

Well, thinking of it now, actually DECODE_INTRA_FRAMES_ONLY makes
sense here - if it is set, only 1 CAPTURE buffer could be allowed
indeed, because no reference frames are needed for decoding.

If inter frames are needed, I believe full DPB needs to be allocated,
because it all depends on the stream how the references are set, so
this is equivalent to normal decoding.

best regards,
Tomasz

>
> > Best regards,
> > Tomasz
> >
> >>
> >> Regards,
> >>
> >>         Hans
> >>
> >>>
> >>>>
> >>>> That this is useful for creating thumbnails is just a specific use-case.
> >>>>
> >>>> Regards,
> >>>>
> >>>>      Hans
> >>>>
> >>>>>>
> >>>>>>>
> >>>>>>> I.e. if you are decoding and 'press' this control, what happens then?
> >>>>>>
> >>>>>> Might be the button type wasn't great idea. In fact the control should
> >>>>>> be set before streamon so that the driver returns min_capture_bufs 1.
> >>>>>>
> >>>>>>>
> >>>>>>> What exactly is the use-case?
> >>>>>>
> >>>>>> It could be used to generate thumbnails of all video clips in a folder
> >>>>>> or when you open a Gallery application on your phone.
> >>>>>>
> >>>>>
> >>>>> What is your opinion on that control? I could consider to make it Venus
> >>>>> custom control but from the use-case it looks other drivers also can
> >>>>> benefit of it.
> >>>>>
> >>>>> I tried to make more generic one [1] but it looks it will be too difficult.
> >>>>>
> >>>>
> >>>
> >>
>
> --
> regards,
> Stan