Re: [PATCH v5 0/3] media: rkvdec: Add a VP9 backend

[Adrian Ratiu]

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