[RFC 1/2] media: uapi: Add VP8 stateless encoder controls
Andrzej Pietrasiewicz
andrzej.p at collabora.com
Mon Mar 27 05:53:21 PDT 2023
Hi,
W dniu 24.03.2023 o 19:49, Nicolas Dufresne pisze:
> Le mercredi 22 mars 2023 à 11:06 +0100, Andrzej Pietrasiewicz a écrit :
>> 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?
>
> We already have V4L2_CID_MPEG_VIDEO_CONSTANT_QUALITY which is bound to
> V4L2_MPEG_VIDEO_BITRATE_MODE_CQ,
Nice catch. Both are used only by Venus. We could reuse it. But then there's
the allowed range - which you do discuss below.
which seems what we should expect form a
> stateless encoder. In fact, adding the entire BITRATE_MODE would enable later
> encoder that has firmware driven rate control to be able to add it easily
> (similar to what we have in GPUs).
>
> We don't need per frame type QP, as stateless encoder have requests, so we can
> set the QP for each frame separately anyway.
>
>>
>> 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?
>
> It seems V4L2_CID_MPEG_VIDEO_CONSTANT_QUALITY has decided to go 0-100 regardless
> of the CODEC. The API is not very inconsistent, like VPX_IN_QP does not even
> document a range, and says its for VP8 only. Perhaps we could open it up, and
> allow per codec range so we can match 1:1 with the CODEC specs ? We could only
> allow that for stateless if we beleive abstracting it to 0-100 make is better in
> general. Just that in stateless, we expect that number to be written in the
> bitstream verbatim.
>
Do you mean to relax the 0-100 range of V4L2_CID_MPEG_VIDEO_CONSTANT_QUALITY
and then use this control instead of adding a new one for stateless codecs,
or to specifically add a new one, modeled after
V4L2_CID_MPEG_VIDEO_CONSTANT_QUALITY?
Regards,
Andrzej
More information about the linux-arm-kernel
mailing list