linux-kernel - Re: Re: [PATCH v3] 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] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 27 Nov 2020 00:40:31 +0100
From:   Jernej Škrabec <jernej.skrabec@...l.net>
To:     mripard@...nel.org, paul.kocialkowski@...tlin.com,
        Ezequiel Garcia <ezequiel@...labora.com>
Cc:     mchehab@...nel.org, wens@...e.org, hverkuil@...all.nl,
        linux-kernel@...r.kernel.org, linux-media@...r.kernel.org,
        devel@...verdev.osuosl.org, linux-arm-kernel@...ts.infradead.org,
        linux-sunxi@...glegroups.com,
        Emmanuel Gil Peyrot <linkmauve@...kmauve.fr>
Subject: Re: Re: [PATCH v3] media: cedrus: Add support for VP8 decoding

Hi!

Dne petek, 27. november 2020 ob 00:21:11 CET je Ezequiel Garcia napisal(a):
> Hi Jernej, Emmanuel,
> 
> Thanks for the patch.
> 
> On Tue, 2020-11-10 at 23:35 +0100, Jernej Skrabec wrote:
> > 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
> > 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>
> > [addressed issues from reviewer]
> > Signed-off-by: Emmanuel Gil Peyrot <linkmauve@...kmauve.fr>
> > ---
> > Changes in v3:
> > - addressed comments from Ezequiel Garcia - new comments,
> >   using new macros from VP8 UAPI, new function for waiting
> >   on bit to be set
> > Changes in v2:
> > - rebased on top of current linux-media master branch
> > 
> > NOTE: This now depends on following patch:
> > https://patchwork.linuxtv.org/project/linux-media/patch/
20201108202021.4187-1-linkmauve@...kmauve.fr/
> > 
> 
> The patch looks fairly good, so let's wait and see
> what Hans, Paul and Maxime think about it.
> 
> FWIW, my humble Reviewed-by: Ezequiel Garcia <ezequiel@...labora.com>

Thanks!

> 
> It would be good to make sure this doesn't regress
> v4l2-compliance, or cause some regression in decoding.

I didn't include v4l2-compliance here, but it was in previous revisions. This 
revision has just cosmetics. Not sure how it could cause any regression since 
it's pretty standalone.

> 
> Not really a blocker to merge this, but I'm thinking
> that now that we have Fluster for conformance testing,
> we could add the VP8 vectors and use them against
> Cedrus and Hantro:
> 
> https://chromium.googlesource.com/webm/vp8-test-vectors/+/refs/heads/master

I tested VP8 test vectors with initial version of this decoder by hand and all 
videos were properly decoded as far as I can tell. But automated testing is 
always welcome.

Best regards,
Jernej

> 
> Thanks,
> Ezequiel
> 
> >  drivers/staging/media/sunxi/cedrus/Makefile   |   3 +-
> >  drivers/staging/media/sunxi/cedrus/cedrus.c   |   8 +
> >  drivers/staging/media/sunxi/cedrus/cedrus.h   |  24 +
> >  .../staging/media/sunxi/cedrus/cedrus_dec.c   |   5 +
> >  .../staging/media/sunxi/cedrus/cedrus_hw.c    |   2 +
> >  .../staging/media/sunxi/cedrus/cedrus_regs.h  |  80 ++
> >  .../staging/media/sunxi/cedrus/cedrus_video.c |   9 +
> >  .../staging/media/sunxi/cedrus/cedrus_vp8.c   | 907 ++++++++++++++++++
> >  8 files changed, 1037 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/staging/media/sunxi/cedrus/cedrus_vp8.c
> > 
> 
>