[RFC 1/2] media: uapi: Add VP8 stateless encoder controls

[Andrzej Pietrasiewicz]

Hi Hans,

W dniu 21.03.2023 o 14:39, Hans Verkuil pisze:
> Hi Andrzej,
> 
> On 09/03/2023 13:56, Andrzej Pietrasiewicz wrote:
>> Add uAPI for stateless VP8 encoders.
>>
>> Signed-off-by: Andrzej Pietrasiewicz <andrzej.p at collabora.com>
>> ---
>>   drivers/media/v4l2-core/v4l2-ctrls-core.c | 16 ++++
>>   drivers/media/v4l2-core/v4l2-ctrls-defs.c |  5 ++
>>   include/media/v4l2-ctrls.h                |  1 +
>>   include/uapi/linux/v4l2-controls.h        | 91 +++++++++++++++++++++++
>>   include/uapi/linux/videodev2.h            |  3 +
>>   5 files changed, 116 insertions(+)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c
>> index 29169170880a..5055e75d37bb 100644
>> --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c
>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c
>> @@ -335,6 +335,9 @@ void v4l2_ctrl_type_op_log(const struct v4l2_ctrl *ctrl)
>>   	case V4L2_CTRL_TYPE_VP9_FRAME:
>>   		pr_cont("VP9_FRAME");
>>   		break;
>> +	case V4L2_CTRL_TYPE_VP8_ENCODE_PARAMS:
>> +		pr_cont("VP8_ENCODE_PARAMS");
>> +		break;
>>   	case V4L2_CTRL_TYPE_HEVC_SPS:
>>   		pr_cont("HEVC_SPS");
>>   		break;
>> @@ -568,6 +571,7 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
>>   	struct v4l2_ctrl_hevc_pps *p_hevc_pps;
>>   	struct v4l2_ctrl_hdr10_mastering_display *p_hdr10_mastering;
>>   	struct v4l2_ctrl_hevc_decode_params *p_hevc_decode_params;
>> +	struct v4l2_ctrl_vp8_encode_params *p_vp8_encode_params;
>>   	struct v4l2_area *area;
>>   	void *p = ptr.p + idx * ctrl->elem_size;
>>   	unsigned int i;
>> @@ -918,6 +922,15 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
>>   			return -EINVAL;
>>   		break;
>>   
>> +	case V4L2_CTRL_TYPE_VP8_ENCODE_PARAMS:
>> +		p_vp8_encode_params = p;
>> +		if (p_vp8_encode_params->loop_filter_level > 63)
>> +			return -EINVAL;
>> +
>> +		if (p_vp8_encode_params->sharpness_level > 7)
>> +			return -EINVAL;
>> +		break;
>> +
>>   	default:
>>   		return -EINVAL;
>>   	}
>> @@ -1602,6 +1615,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
>>   	case V4L2_CTRL_TYPE_VP9_FRAME:
>>   		elem_size = sizeof(struct v4l2_ctrl_vp9_frame);
>>   		break;
>> +	case V4L2_CTRL_TYPE_VP8_ENCODE_PARAMS:
>> +		elem_size = sizeof(struct v4l2_ctrl_vp8_encode_params);
>> +		break;
>>   	case V4L2_CTRL_TYPE_AREA:
>>   		elem_size = sizeof(struct v4l2_area);
>>   		break;
>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>> index 564fedee2c88..935bd9a07bad 100644
>> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>> @@ -1182,6 +1182,8 @@ const char *v4l2_ctrl_get_name(u32 id)
>>   	case V4L2_CID_STATELESS_MPEG2_QUANTISATION:		return "MPEG-2 Quantisation Matrices";
>>   	case V4L2_CID_STATELESS_VP9_COMPRESSED_HDR:	return "VP9 Probabilities Updates";
>>   	case V4L2_CID_STATELESS_VP9_FRAME:			return "VP9 Frame Decode Parameters";
>> +	case V4L2_CID_STATELESS_VP8_ENCODE_PARAMS:		return "VP8 Encode Parameters";
>> +	case V4L2_CID_STATELESS_VP8_ENCODE_QP:			return "VP8 Encode QP";
>>   	case V4L2_CID_STATELESS_HEVC_SPS:			return "HEVC Sequence Parameter Set";
>>   	case V4L2_CID_STATELESS_HEVC_PPS:			return "HEVC Picture Parameter Set";
>>   	case V4L2_CID_STATELESS_HEVC_SLICE_PARAMS:		return "HEVC Slice Parameters";
>> @@ -1531,6 +1533,9 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>>   	case V4L2_CID_STATELESS_VP9_FRAME:
>>   		*type = V4L2_CTRL_TYPE_VP9_FRAME;
>>   		break;
>> +	case V4L2_CID_STATELESS_VP8_ENCODE_PARAMS:
>> +		*type = V4L2_CTRL_TYPE_VP8_ENCODE_PARAMS;
>> +		break;
> 
> Why isn't V4L2_CID_STATELESS_VP8_ENCODE_QP added here as well? I assume it is of
> type INTEGER?
> 

Thanks for pointing that.

And it is a simple integer, indeed.

> I also wonder if VP9 would have the same control, so that this could be called
> V4L2_CID_STATELESS_VPX_ENCODE_QP. On the other hand, that might be overkill.
> 

It seems to me that having a single kind of control for passing the
requested QP value for both VP8 and VP9 makes sense. In fact, perhaps not
restricting ourselves to VPX would make even more sense?

This touches the question of how we do rate control in general in stateless
encoders. If the uAPI is to be independent of underlying hardware, then the only
parameter userspace passes to the kernel is the required QP (which is determined
entirely by userspace using whatever means it considers appropriate, for example
judging by the last encoded frame(s) size(s)). Any other kinds of data would
probably be somehow hardware-specific. So, I'm wondering if maybe even a

V4L2_CID_STATELESS_ENCODE_QP

is what we want?

This, in turn, brings another question of the range and interpretation of values
that should be passed through this control. 0-255? 0-100? 0 = no quantization at
all (i.e. highest quality) or maybe 0 = lowest possible quality?

Regards,

Andrzej
Regards,

Andrzej