linux-kernel - Re: [PATCH 0/9] media: cedrus: Add H264 decoding support

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]

Date:   Thu, 14 Jun 2018 18:37:17 +0200
From:   Maxime Ripard <maxime.ripard@...tlin.com>
To:     Tomasz Figa <tfiga@...omium.org>
Cc:     Hans Verkuil <hans.verkuil@...co.com>,
        Alexandre Courbot <acourbot@...omium.org>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Pawel Osciak <posciak@...omium.org>,
        Paul Kocialkowski <paul.kocialkowski@...tlin.com>,
        Chen-Yu Tsai <wens@...e.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "list@....net:IOMMU DRIVERS <iommu@...ts.linux-foundation.org>, Joerg
        Roedel <joro@...tes.org>," <linux-arm-kernel@...ts.infradead.org>,
        Linux Media Mailing List <linux-media@...r.kernel.org>,
        nicolas.dufresne@...labora.com, jenskuske@...il.com,
        linux-sunxi@...glegroups.com, thomas.petazzoni@...tlin.com
Subject: Re: [PATCH 0/9] media: cedrus: Add H264 decoding support

Hi Tomasz,

On Thu, Jun 14, 2018 at 10:00:43PM +0900, Tomasz Figa wrote:
> Hi Maxime,
> 
> On Wed, Jun 13, 2018 at 11:07 PM Maxime Ripard
> <maxime.ripard@...tlin.com> wrote:
> >
> > Hi,
> >
> > Here is a preliminary version of the H264 decoding support in the
> > cedrus driver.
> 
> Thanks for the series! Let me reply inline to some of the points raised here.
> 
> > As you might already know, the cedrus driver relies on the Request
> > API, and is a reverse engineered driver for the video decoding engine
> > found on the Allwinner SoCs.
> >
> > This work has been possible thanks to the work done by the people
> > behind libvdpau-sunxi found here:
> > https://github.com/linux-sunxi/libvdpau-sunxi/
> >
> > This driver is based on the last version of the cedrus driver sent by
> > Paul, based on Request API v13 sent by Hans:
> > https://lkml.org/lkml/2018/5/7/316
> 
> Just FYI, there is v15 already. :)

Yeah, we know, Paul is currently working on rebasing to that version :)

> > This driver has been tested only with baseline profile videos, and is
> > missing a few key features to decode videos with higher profiles.
> > This has been tested using our cedrus-frame-test tool, which should be
> > a quite generic v4l2-to-drm decoder using the request API to
> > demonstrate the video decoding:
> > https://github.com/free-electrons/cedrus-frame-test/, branch h264
> >
> > However, sending this preliminary version, I'd really like to start a
> > discussion and get some feedback on the user-space API for the H264
> > controls exposed through the request API.
> >
> > I've been using the controls currently integrated into ChromeOS that
> > have a working version of this particular setup. However, these
> > controls have a number of shortcomings and inconsistencies with other
> > decoding API. I've worked with libva so far, but I've noticed already
> > that:
> 
> Note that these controls are supposed to be defined exactly like the
> bitstream headers deserialized into C structs in memory. I believe
> Pawel (on CC) defined them based on the actual H264 specification.
> 
> >   - The kernel UAPI expects to have the nal_ref_idc variable, while
> >     libva only exposes whether that frame is a reference frame or
> >     not. I've looked at the rockchip driver in the ChromeOS tree, and
> >     our own driver, and they both need only the information about
> >     whether the frame is a reference one or not, so maybe we should
> >     change this?
> 
> The fact that 2 drivers only need partial information doesn't mean
> that we should ignore the data being already in the bitstream. IMHO
> this API should to provide all the metadata available in the stream to
> the kernel driver, as a replacement for bitstream parsing in firmware
> (or in kernel... yuck).

The point is more that libva will only pass the result of (nal_ref_idc
!= 0). So in the libva plugin, you won't be able to fill the proper
value to the kernel, since you don't have access to it.

> >   - The H264 bitstream exposes the picture default reference list (for
> >     both list 0 and list 1), the slice reference list and an override
> >     flag. The libva will only pass the reference list to be used (so
> >     either the picture default's or the slice's) depending on the
> >     override flag. The kernel UAPI wants the picture default reference
> >     list and the slice reference list, but doesn't expose the override
> >     flag, which prevents us from configuring properly the
> >     hardware. Our video decoding engine needs the three information,
> >     but we can easily adapt to having only one. However, having two
> >     doesn't really work for us.
> 
> Where does the override flag come from? If it's in the bitstream, then
> I guess it was just missed when creating the structures.

It's in the bitstream yeah. I'll add it then.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com