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@xxxxxxxxxx>
>> ---
>>  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