linux-kernel - Re: [PATCH] media: cedrus: Add support for VP8 decoding

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] [day] [month] [year] [list]

Message-ID: <2875977.BS6FNRR2HQ@jernej-laptop>
Date:   Thu, 21 May 2020 00:30:56 +0200
From:   Jernej Škrabec <jernej.skrabec@...l.net>
To:     mripard@...nel.org, paul.kocialkowski@...tlin.com,
        Nicolas Dufresne <nicolas@...fresne.ca>
Cc:     mchehab@...nel.org, gregkh@...uxfoundation.org, wens@...e.org,
        hverkuil-cisco@...all.nl, linux-kernel@...r.kernel.org,
        linux-media@...r.kernel.org, devel@...verdev.osuosl.org,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH] media: cedrus: Add support for VP8 decoding

Dne sreda, 20. maj 2020 ob 23:43:40 CEST je Nicolas Dufresne napisal(a):
> Le mercredi 20 mai 2020 à 23:01 +0200, Jernej Skrabec a écrit :
> > VP8 in Cedrus shares same engine as H264.
> > 
> > Note that it seems necessary to call bitstream parsing functions,
> > to parse frame header, otherwise decoded image is garbage. This is
> > contrary to what is driver supposed to do. However, values are not
> > really used, so this might be acceptable. It's possible that bitstream
> 
> Have you verified that all values passed through controls are not used
> ? To remain a stateless driver, there is no requirement for parsed data
> to be used, the only requirement is that the reference are used.
> Otherwise doing parallel decoding of two stream of different stream
> would be broken. Have you verified that parallel decoding is working as
> expected ?

I'm not sure if you understand what I meant. Although userspace app parses 
frame header and fills all data in VP8 control, driver parses frame header 
again, using HW bitstream parsing functionality in cedrus_read_header(). 
Without that second header parsing in HW, decoded image is garbage. Note that 
cedrus_read_header() discards all parsed values and relies on those provided 
in controls.

This parsing doesn't cause any problems with parallel decoding or anything. 
It's done during frame decoding job, so it doesn't affect any state. It's just 
that we shouldn't need to parse header in driver because all data is already 
provided in controls. It seems that Cedrus core was never tested without that 
HW frame header parsing. I found out that HEVC and H264 frames can sometimes 
also be wrongly decoded if no bitstream parsing function is triggered in HW 
before final decoding.

I spend a lot of time trying to avoid that header parsing, but I couldn't find 
any way around it.

In another words, Cedrus VPU provides two functionalities - HW bitstream 
parsing (to speed up header parsing) and video decoding. One would thought 
that video decoding can be used independently, if all data from header is 
already known, but it can't be.

Best regards,
Jernej

> 
> > parsing functions set some internal VPU state, which is later necessary
> > for proper decoding. Biggest suspect is "VP8 probs update" trigger.
> > 
> > Signed-off-by: Jernej Skrabec <jernej.skrabec@...l.net>
> > ---
> > 
> >  drivers/staging/media/sunxi/cedrus/Makefile   |   3 +-
> >  drivers/staging/media/sunxi/cedrus/cedrus.c   |   8 +
> >  drivers/staging/media/sunxi/cedrus/cedrus.h   |  15 +
> >  .../staging/media/sunxi/cedrus/cedrus_dec.c   |   5 +
> >  .../staging/media/sunxi/cedrus/cedrus_hw.c    |   1 +
> >  .../staging/media/sunxi/cedrus/cedrus_regs.h  |  80 ++
> >  .../staging/media/sunxi/cedrus/cedrus_video.c |   9 +
> >  .../staging/media/sunxi/cedrus/cedrus_vp8.c   | 699 ++++++++++++++++++
> >  8 files changed, 819 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/staging/media/sunxi/cedrus/cedrus_vp8.c