Re: [RFC 0/2] VP8 stateless V4L2 encoding uAPI + driver

From: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
To: Nicolas Frattaroli <frattaroli.nicolas@gmail.com>,
	linux-media@vger.kernel.org, linux-rockchip@lists.infradead.org
Cc: linux-arm-kernel@lists.infradead.org,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>,
	Philipp Zabel <p.zabel@pengutronix.de>,
	Heiko Stuebner <heiko@sntech.de>,
	kernel@collabora.com,
	Benjamin Gaignard <benjamin.gaignard@collabora.com>
Subject: Re: [RFC 0/2] VP8 stateless V4L2 encoding uAPI + driver
Date: Mon, 20 Mar 2023 11:07:19 +0100	[thread overview]
Message-ID: <b6468072-834b-c948-3a27-e34fc80d203f@collabora.com> (raw)
In-Reply-To: <4586871.LvFx2qVVIh@archbox>

Hi Nicolas,

+Cc Benjamin

W dniu 18.03.2023 o 10:20, Nicolas Frattaroli pisze:
> On Thursday, 9 March 2023 13:56:49 CET Andrzej Pietrasiewicz wrote:
>> Dear All,
>>
>> This two-patch series adds uAPI for stateless VP8 encoding
>> and an accompanying driver using it.
>>
>> It has been tested on an rk3399 board and there exists
>> a gstreamer user:
>>
>> https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/3736
>>
>> example pipeline:
>>
>> gst-launch-1.0 videotestsrc num-buffers=500 !
>> video/x-raw,width=640,height=480 \ ! v4l2slvp8enc ! queue ! matroskamux !
>> filesink location=test_vp8.mkv
>>
>> I kindly ask for comments.
>>
>> Notably, the documentation for the added uAPI is missing,
>> that is to be addressed when sending a patch series proper (not RFC).
>>
>> For the RFC I also did not care to replace a BUG_ON() in the boolean
>> encoder.
>>
>> Rebased onto v6.2.
>>
>> Regards,
>>
>> Andrzej
> 
> Hello,
> 
> I can't offer much in terms on technical comments on the implementation,
> but thank you for your work on this. A more general question: Is the
> rate control done by the userspace component or the kernel or even the
> hardware?
> 
> I tried this patchset (and the gstreamer merge request) out last night
> and ran into quite noticable i-frame pulsing, and am wondering who the
> culprit of that is. Looking at the vp8 encode params in the uAPI, it
> looks like it'll be userspace in charge of rate control?
> 

Yes, rate control is entirely on userspace.

Modern codec specs (not just vp8) only mandate what constitutes a valid
bitstream and how do decode it. No word about encoding, which means that
actually many different strategies can be applied to produce a valid
bitstream. As such, these strategies look a lot like policies which do not
belong to the kernel and I'd rather provide a tool than a policy.

As far as I know the rate control mechanism used in the gst element is
nothing sophisticated, not even a (full) PID algorithm. Which, I think,
was exactly intended to get the thing running in the first place. I would
assume that the expected algorithm would be PID proper in this case.
Specifically, PID being PID will not prevent the encoding stack from
overrunning the set bandwidth for short periods of time, but exactly
because rate control belongs to userspace anyone is welcome to develop
a solution which provides hard bandwidth guarantees.

> On a related side note, since I let this run all night with different
> parameters I can happily report that it seems to be quite stable, no
> problems encountered at all.

Thank you for reporting. In the (expected) case this turns into a
patchset proper I would kindly ask for your Tested-by then.

Can you share what you used for the nightly tests, both in terms of
testing harness and unencoded video material?

Regards,

Andrzej