Date:   Thu, 14 Oct 2021 16:57:20 +0800
From:   Chen-Yu Tsai <wenst@...omium.org>
To:     Ezequiel Garcia <ezequiel@...guardiasur.com.ar>
Cc:     Nicolas Dufresne <nicolas@...fresne.ca>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Linux Media Mailing List <linux-media@...r.kernel.org>,
        "open list:ARM/Rockchip SoC..." <linux-rockchip@...ts.infradead.org>,
        "open list:STAGING SUBSYSTEM" <linux-staging@...ts.linux.dev>,
        LKML <linux-kernel@...r.kernel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Andrzej Pietrasiewicz <andrzej.p@...labora.com>,
        Jernej Skrabec <jernej.skrabec@...il.com>
Subject: Re: [PATCH 0/2] media: rkvdec: Align decoder behavior with Hantro and Cedrus

On Thu, Oct 14, 2021 at 4:46 PM Ezequiel Garcia
<ezequiel@...guardiasur.com.ar> wrote:
>
> On Thu, 14 Oct 2021 at 04:31, Chen-Yu Tsai <wenst@...omium.org> wrote:
> >
> > On Wed, Oct 13, 2021 at 9:40 PM Nicolas Dufresne <nicolas@...fresne.ca> wrote:
> > >
> > > Le mercredi 13 octobre 2021 à 15:05 +0800, Chen-Yu Tsai a écrit :
> > > > Hi,
> > > >
> > > > On Fri, Oct 8, 2021 at 11:42 PM Nicolas Dufresne <nicolas@...fresne.ca> wrote:
> > > > >
> > > > > Hi Chen-Yu,
> > > > >
> > > > > thanks for looking into this.
> > > > >
> > > > > Le vendredi 08 octobre 2021 à 18:04 +0800, Chen-Yu Tsai a écrit :
> > > > > > Hi everyone,
> > > > > >
> > > > > > While working on the rkvdec H.264 decoder for ChromeOS, I noticed some
> > > > > > behavioral differences compared to Hantro and Cedrus:
> > > > > >
> > > > > > 1. The driver always overrides the sizeimage setting given by userspace
> > > > > >    for the output format. This results in insufficient buffer space when
> > > > > >    running the ChromeOS video_decode_accelerator_tests test program,
> > > > > >    likely due to a small initial resolution followed by dynamic
> > > > > >    resolution change.
> > > > > >
> > > > > > 2. Doesn't support dynamic resolution change.
> > > > > >
> > > > > > This small series fixes both and aligns the behavior with the other two
> > > > > > stateless decoder drivers. This was tested on the downstream ChromeOS
> > > > > > 5.10 kernel with ChromeOS. Also compiled tested on mainline but I don't
> > > > > > have any other RK3399 devices set up to test video stuff, so testing
> > > > > > would be very much appreciated.
> > > > > >
> > > > > > Also, I'm not sure if user applications are required to check the value
> > > > > > of sizeimage upon S_FMT return. If the value is different or too small,
> > > > > > what can the application do besides fail? AFAICT it can't split the
> > > > > > data of one frame (or slice) between different buffers.
> > > > >
> > > > > While most software out there just assumes that driver will do it right and
> > > > > crash when it's not the case, application that do map the buffer to CPU must
> > > > > read back the fmt structure as the drivers are all fail-safe and will modify
> > > > > that structure to a set of valid value s for the context.
> > > >
> > > > I believe what is happening in Chromium is that the decoder is opened with
> > > > some default settings, including the smallest viable resolution for the
> > > > output side, and the buffers allocated accordingly. When dynamic resolution
> > > > change happens, the decoder does not check if the current buffers are
> > > > sufficiently sized; it just assumes that they are. And when it starts
> > > > pushing data into the buffers, it realizes they are too small and fails.
> > > >
> > > > The spec also says:
> > > >
> > > >     Clients are allowed to set the sizeimage field for variable length
> > > >     compressed data flagged with V4L2_FMT_FLAG_COMPRESSED at ioctl
> > > >     VIDIOC_ENUM_FMT, but the driver may ignore it and set the value itself,
> > > >     or it may modify the provided value based on alignment requirements or
> > > >     minimum/maximum size requirements.
> > > >
> > > > The spec only guarantees that the buffers are of sufficient size for the
> > > > resolution configured at the time they were allocated/requested.
> > > >
> > > > So I think my first patch is a workaround for a somewhat broken userspace.
> > > > But it seems the other stateless drivers are providing similar behavior,
> > > > as I previously mentioned.
> > >
> > > That's what I mean, this is not a driver bug strictly speaking (assuming it does
> > > guaranty the buffer size is sufficient) but it is without your change
> > > inconvenient, as userspace may be aware of the largest resolution it will
> > > decode, and may want to allocate larger buffer upfront.
> >
> > Thinking about this more, I think a few follow up fixes for each driver
> > are in order. The spec implies that the driver should override the value
> > should userspace give some unrealistic value, such as asking for a 256 byte
> > buffer for a 4K frame size.
> >
>
> Where is the spec implying that?

In Documentation/userspace-api/media/v4l/pixfmt-v4l2-mplane.rst:

    Clients are allowed to set the sizeimage field for variable length
    compressed data flagged with ``V4L2_FMT_FLAG_COMPRESSED`` at
    :ref:`VIDIOC_ENUM_FMT`, but the driver may ignore it and set the
    value itself, or it may modify the provided value based on
    alignment requirements or minimum/maximum size requirements.

I guess I read "minimum/maximum size requirements" a bit liberally.
Maybe it refers to how much buffer space the hardware can address or
program for each request?

> This is encoded content, so I'm really inclined to avoid this path.
> Having the driver decide what is "unrealistic" would mean some
> heuristics in the drivers for something that should really come from userspace.

And if the driver refuses to give adequate buffer space, then it's a bug?


Regards
ChenYu


> Thanks,
> Ezequiel
>
> > Cedrus (CCing Jernej) comes close, but a 1K buffer might not be enough for
> > really large frames, even though it's slice based?
> >
> > ChenYu
> >
> >
> > > As per Chromium bug, this is being addressed already. Thanks for this driver
> > > improvement.
> > >
> > > >
> > > > > As for opposite direction (output vs capture) format being changed, this should
> > > > > be documented in the spec, if you find it too unclear or missing for sateless
> > > > > codec (I know it's there for stateful but can't remember, would have to re-read,
> > > > > for stateless) let us know.
> > > >
> > > > AFAICT the capture side is working OK and to spec.
> > > >
> > > >
> > > > Regards
> > > > ChenYu
> > > >
> > > > > regards,
> > > > > Nicolas
> > > > >
> > > > > >
> > > > > > Andrzej, I believe the second patch would conflict with your VP9 series.
> > > > > >
> > > > > >
> > > > > > Regards
> > > > > > ChenYu
> > > > > >
> > > > > > Chen-Yu Tsai (2):
> > > > > >   media: rkvdec: Do not override sizeimage for output format
> > > > > >   media: rkvdec: Support dynamic resolution changes
> > > > > >
> > > > > >  drivers/staging/media/rkvdec/rkvdec-h264.c |  5 +--
> > > > > >  drivers/staging/media/rkvdec/rkvdec.c      | 40 +++++++++++-----------
> > > > > >  2 files changed, 23 insertions(+), 22 deletions(-)
> > > > > >
> > > > >
> > > > >
> > >
> > >

Powered by blists - more mailing lists