Re: [PATCH v2 2/3] media: uapi: Add VP9 stateless decoder controls

[Boris Brezillon <boris.brezillon@collabora.com>]

On Mon, 04 May 2020 14:01:32 -0400
Nicolas Dufresne <nicolas.dufresne@collabora.com> wrote:

> Le samedi 02 mai 2020 à 19:55 -0300, Ezequiel Garcia a écrit :
> > +Nicolas
> > 
> > On Sat, 2020-05-02 at 20:37 +0200, Boris Brezillon wrote:  
> > > On Fri, 01 May 2020 13:57:49 -0300
> > > Ezequiel Garcia <ezequiel@collabora.com> wrote:
> > >   
> > > > > > +
> > > > > > +.. tabularcolumns:: |p{1.5cm}|p{6.3cm}|p{9.4cm}|
> > > > > > +
> > > > > > +.. flat-table:: enum v4l2_vp9_reset_frame_context
> > > > > > +    :header-rows:  0
> > > > > > +    :stub-columns: 0
> > > > > > +    :widths:       1 2
> > > > > > +
> > > > > > +    * - ``V4L2_VP9_RESET_FRAME_CTX_NONE``
> > > > > > +      - Do not reset any frame context.
> > > > > > +    * - ``V4L2_VP9_RESET_FRAME_CTX_NONE_ALT``
> > > > > > +      - Do not reset any frame context. This is an alternative value for
> > > > > > +        V4L2_VP9_RESET_FRAME_CTX_NONE.    
> > > > > 
> > > > > Add `` around V4L2_VP9_RESET_FRAME_CTX_NONE.
> > > > >     
> > > > 
> > > > Hm, now that I look closer, what's the point
> > > > of having the NONE_ALT in our uAPI if it
> > > > has same meaning as NONE?
> > > > 
> > > > I think it can be removed.  
> > > 
> > > The intent was to match the spec so that one can pass the value
> > > extracted from the bitstream directly.  
> 
> reset_frame_contextspecifies whether the frame context should be reset
> to default values:
>   − 0 or 1 means do not reset any frame context
>   − 2 resets just the context specified in the frame header
>   − 3 resets all cont
> 
> But aren't we going too far by making this an emum ?

I like to not have to constantly go back to the spec when I read code,
and having constant values defined through enums definitely helps in
this regard, but maybe it's just me.

> In Microsfot DXVA,
> we pass that value without interpreting it in userspace. For the
> following RKVDEC, it is (suspiciously ?) ignored.

IIRC, the prob context has to be kept in userspace anyway (and reset
when needed), and the rkvdec engine does not have any hardware context.
That's probably why this value is ignored here.

> Maybe just passing
> over the value would make more sense, less work ?

I don't see how adding an enum adds more work, given the enum values
match the ones defined VP 9 spec, but maybe I'm missing something.