Message-ID: <ee12e70c92bacf1080ceae82feafa736c0719a2e.camel@collabora.com>
Date:   Tue, 10 Nov 2020 19:39:38 -0300
From:   Ezequiel Garcia <ezequiel@...labora.com>
To:     Adrian Ratiu <adrian.ratiu@...labora.com>,
        Ezequiel Garcia <ezequiel@...guardiasur.com.ar>
Cc:     linux-media <linux-media@...r.kernel.org>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Boris Brezillon <boris.brezillon@...labora.com>,
        Hans Verkuil <hverkuil-cisco@...all.nl>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Jonas Karlman <jonas@...boo.se>,
        "open list:ARM/Rockchip SoC..." <linux-rockchip@...ts.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        kernel@...labora.com
Subject: Re: [PATCH v5 0/3] media: rkvdec: Add a VP9 backend

On Wed, 2020-11-11 at 00:28 +0200, Adrian Ratiu wrote:
> Hi Ezequiel,
>  
> On Tue, 10 Nov 2020, Ezequiel Garcia 
> <ezequiel@...guardiasur.com.ar> wrote:
> > On Mon, 2 Nov 2020 at 16:04, Adrian Ratiu 
> > <adrian.ratiu@...labora.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.

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

Powered by blists - more mailing lists