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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200504210630.1489df6e@collabora.com>
Date:   Mon, 4 May 2020 21:06:30 +0200
From:   Boris Brezillon <boris.brezillon@...labora.com>
To:     Nicolas Dufresne <nicolas.dufresne@...labora.com>
Cc:     Ezequiel Garcia <ezequiel@...labora.com>,
        Hans Verkuil <hverkuil@...all.nl>, linux-media@...r.kernel.org,
        linux-rockchip@...ts.infradead.org, linux-kernel@...r.kernel.org,
        Tomasz Figa <tfiga@...omium.org>, kernel@...labora.com,
        Jonas Karlman <jonas@...boo.se>,
        Heiko Stuebner <heiko@...ech.de>,
        Alexandre Courbot <acourbot@...omium.org>,
        Jeffrey Kardatzke <jkardatzke@...omium.org>
Subject: Re: [PATCH v2 2/3] media: uapi: Add VP9 stateless decoder controls

On Mon, 04 May 2020 14:01:32 -0400
Nicolas Dufresne <nicolas.dufresne@...labora.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@...labora.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.