[PATCH v5 0/3] media: rkvdec: Add a VP9 backend
Adrian Ratiu
adrian.ratiu at collabora.com
Tue Nov 10 19:11:30 EST 2020
On Tue, 10 Nov 2020, Ezequiel Garcia <ezequiel at collabora.com>
wrote:
> On Wed, 2020-11-11 at 00:28 +0200, Adrian Ratiu wrote:
>> Hi Ezequiel,
>>
>> On Tue, 10 Nov 2020, Ezequiel Garcia
>> <ezequiel at vanguardiasur.com.ar> wrote:
>> > On Mon, 2 Nov 2020 at 16:04, Adrian Ratiu
>> > <adrian.ratiu at collabora.com> wrote:
>> > > Dear all, This is v5 of the series adding VP9 profile 0
>> > > decoding to rkvdec. All feedback from v4 should be
>> > > addressed, there's just one thing I did not address:
>> > > ref_frame_sign_biases in the uAPI. The userspace tool I'm
>> > I believe that Hantro G2 VP9 needs ref_frame_sign_biases.
>> > I think that it's also needed for the MTK decoder. Might be
>> > worth checking that as well, if the code is publicly
>> > available somewhere.
>> I consulted the imx8m app ref manual for the Hantro G2 core
>> and indeed there's not one, but three fields at SWREG11 and 13
>> (last, gold, alt) to signify sign biases for ref
>> frames. Thanks for the hint!
>>
>> > Coming to think about it, I think we are really close to
>> > having this uAPI directly upstream. Let's take a step
>> > back on why we have these uAPIs in the staging area. Couple
>> > years ago, there were some doubts in the media community
>> > about these uAPIs, and we wanted to wait a bit for more
>> > users before moving to public land. The uAPIs were meant
>> > to be in staging until enough users appeared and we were
>> > confident enough to move to stable. For VP9, given the
>> > feedback received through the year was already addressed, I
>> > think all that's left is to check the interface and make
>> > sure it can support Rockchip (RK3399, RK3326, etc), Hantro
>> > G2 and Mediatek, We will be very close to having a public
>> > API, and we could even merge it directly there.
>> Thank you very much for this background. I understand that the
>> uAPI is independent from the driver implementations, so having
>> a good stable uAPI is beneficial when (for example) adding
>> support for VP9 on G2 in hantro or for upstream adoption of
>> these drivers. Given this rkvdec driver implementation is
>> also adding the VP9 uAPI and it's very close to stability
>> (maybe only missing ref frame sign bias, but who knows?) would
>> you like to block its submission until the uAPI is finalized
>> or would it make sense to treat the uAPI de-staging process
>> separately because the uAPI is independent from the driver?
>
> I don't mean to block it, quite the opposite, to make sure we
> take this opportunity to go through Rockchip, Hantro and
> Mediatek, double-check the uAPI is covering all the VP9 syntax,
> and then target for public API.
That makes sense. I'm just cautious to not directly botch the
public API, but that's what reviews are for, right? :) Thanks
again for helping with background & direction.
>
> Cheers,
> Ezequiel
>
>> Thanks,
>> Adrian
>>
>> > Thanks,
>> > Ezequiel
>> >
>> > > using [1] apparently doesn't need it or the default hwreg value for it
>> > > is capable of decoding the bitstreams I used on the driver, so I don't
>> > > really have a use-case to change and test that. :)
>> > >
>> > > Considering the uAPI is a work in progress and expected to be modified,
>> > > ref_frame_sign_biases can be added later with others which might be
>> > > required to enable more functionality (for eg profiles >= 1).
>> > >
>> > > Series tested on rk3399 and applies on next-20201030.
>> > >
>> > > [1] https://github.com/Kwiboo/FFmpeg/tree/v4l2-request-hwaccel-4.2.2-rkvdec
>> > >
>> > > Changelog
>> > > ---------
>> > >
>> > > v5:
>> > >
>> > > * Drop unnecessary OUTPUT buffer payload set in .buf_prepare.
>> > > * Drop obsolete .per_request ctrl flag
>> > > * Added new vp9 ctrls to v4l2_ctrl_ptr
>> > > * Fix pahole detected padding issues
>> > > * Send userspace an error if it tries to reconfigure decode resolution
>> > > as v4l2 or rkvdec-vp9 backend do not support dynamic res changes yet
>> > > * Allow frame ctx probability tables to be non-mandatory so users can
>> > > set them directly during frame decoding in cases where no defaults
>> > > have been set previously (eg. ffmpeg vp9 backend)
>> > > * Some comments and documentation clarifications
>> > > * Minor checkpatch fixes
>> > >
>> > > v4:
>> > >
>> > > * Drop color_space field from the VP9 interface.
>> > > V4L2 API should be used for it.
>> > > * Clarified Segment-ID comments.
>> > > * Moved motion vector probabilities to a separate
>> > > struct.
>> > >
>> > > v3:
>> > >
>> > > * Fix documentation issues found by Hans.
>> > > * Fix smatch detected issues as pointed out by Hans.
>> > > * Added patch to fix wrong bytesused set on .buf_prepare.
>> > >
>> > > v2:
>> > >
>> > > * Documentation style issues pointed out by Nicolas internally.
>> > > * s/VP9_PROFILE_MAX/V4L2_VP9_PROFILE_MAX/
>> > > * Fix wrong kfree(ctx).
>> > > * constify a couple structs on rkvdec-vp9.c
>> > >
>> > >
>> > > Boris Brezillon (2):
>> > > media: uapi: Add VP9 stateless decoder controls
>> > > media: rkvdec: Add the VP9 backend
>> > >
>> > > Ezequiel Garcia (1):
>> > > media: rkvdec: Fix .buf_prepare
>> > >
>> > > .../userspace-api/media/v4l/biblio.rst | 10 +
>> > > .../media/v4l/ext-ctrls-codec.rst | 550 ++++++
>> > > drivers/media/v4l2-core/v4l2-ctrls.c | 239 +++
>> > > drivers/media/v4l2-core/v4l2-ioctl.c | 1 +
>> > > drivers/staging/media/rkvdec/Makefile | 2 +-
>> > > drivers/staging/media/rkvdec/rkvdec-vp9.c | 1577 +++++++++++++++++
>> > > drivers/staging/media/rkvdec/rkvdec.c | 72 +-
>> > > drivers/staging/media/rkvdec/rkvdec.h | 6 +
>> > > include/media/v4l2-ctrls.h | 5 +
>> > > include/media/vp9-ctrls.h | 486 +++++
>> > > 10 files changed, 2942 insertions(+), 6 deletions(-)
>> > > create mode 100644 drivers/staging/media/rkvdec/rkvdec-vp9.c
>> > > create mode 100644 include/media/vp9-ctrls.h
>> > >
>> > > --
>> > > 2.29.0
>> > >
More information about the Linux-rockchip
mailing list