Re: [PATCH 0/2] media: rkvdec: Align decoder behavior with Hantro and Cedrus

From: Nicolas Dufresne <nicolas@ndufresne.ca>
To: Chen-Yu Tsai <wenst@chromium.org>,
	Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>,
	Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: linux-media@vger.kernel.org, linux-rockchip@lists.infradead.org,
	linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Andrzej Pietrasiewicz <andrzej.p@collabora.com>
Subject: Re: [PATCH 0/2] media: rkvdec: Align decoder behavior with Hantro and Cedrus
Date: Fri, 08 Oct 2021 11:42:01 -0400	[thread overview]
Message-ID: <f108f23dadc846222c63c88af826dae9c5082d83.camel@ndufresne.ca> (raw)
In-Reply-To: <20211008100423.739462-1-wenst@chromium.org>

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.

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.

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(-)
> 