On Wed, Oct 13, 2021 at 9:40 PM Nicolas Dufresne <nicolas@xxxxxxxxxxxx> 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@xxxxxxxxxxxx> 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. 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(-) > > > > > > > > > > > >
