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

[Nicolas Dufresne <nicolas.dufresne@collabora.com>]

Hi,

Le lundi 27 mars 2023 à 14:53 +0200, Andrzej Pietrasiewicz a écrit :
> 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@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?


Yes, I'd relax the range overall. And then, specifically for stateless, I'd
require it to match the codec spec specified range.

> 
> Regards,
> 
> Andrzej