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