2021-10-08 10:08:37

by Chen-Yu Tsai

Subject: [PATCH 1/2] media: rkvdec: Do not override sizeimage for output format

The rkvdec H.264 decoder currently overrides sizeimage for the output
format. This causes issues when userspace requires and requests a larger
buffer, but ends up with one of insufficient size.

Instead, only provide a default size if none was requested. This fixes
the video_decode_accelerator_tests from Chromium failing on the first
frame due to insufficient buffer space. It also aligns the behavior
of the rkvdec driver with the Hantro and Cedrus drivers.

Fixes: cd33c830448b ("media: rkvdec: Add the rkvdec driver")
Cc: <[email protected]>
Signed-off-by: Chen-Yu Tsai <[email protected]>
---
drivers/staging/media/rkvdec/rkvdec-h264.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
index 76e97cbe2512..951e19231da2 100644
--- a/drivers/staging/media/rkvdec/rkvdec-h264.c
+++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
@@ -1015,8 +1015,9 @@ static int rkvdec_h264_adjust_fmt(struct rkvdec_ctx *ctx,
struct v4l2_pix_format_mplane *fmt = &f->fmt.pix_mp;

fmt->num_planes = 1;
- fmt->plane_fmt[0].sizeimage = fmt->width * fmt->height *
- RKVDEC_H264_MAX_DEPTH_IN_BYTES;
+ if (!fmt->plane_fmt[0].sizeimage)
+ fmt->plane_fmt[0].sizeimage = fmt->width * fmt->height *
+ RKVDEC_H264_MAX_DEPTH_IN_BYTES;
return 0;
}

--
2.33.0.882.g93a45727a2-goog

2021-10-08 15:51:00

by Nicolas Dufresne

Subject: Re: [PATCH 1/2] media: rkvdec: Do not override sizeimage for output format

Le vendredi 08 octobre 2021 à 18:04 +0800, Chen-Yu Tsai a écrit :
> The rkvdec H.264 decoder currently overrides sizeimage for the output
> format. This causes issues when userspace requires and requests a larger
> buffer, but ends up with one of insufficient size.
>
> Instead, only provide a default size if none was requested. This fixes
> the video_decode_accelerator_tests from Chromium failing on the first
> frame due to insufficient buffer space. It also aligns the behavior
> of the rkvdec driver with the Hantro and Cedrus drivers.
>
> Fixes: cd33c830448b ("media: rkvdec: Add the rkvdec driver")
> Cc: <[email protected]>
> Signed-off-by: Chen-Yu Tsai <[email protected]>
> ---
> drivers/staging/media/rkvdec/rkvdec-h264.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
> index 76e97cbe2512..951e19231da2 100644
> --- a/drivers/staging/media/rkvdec/rkvdec-h264.c
> +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
> @@ -1015,8 +1015,9 @@ static int rkvdec_h264_adjust_fmt(struct rkvdec_ctx *ctx,
> struct v4l2_pix_format_mplane *fmt = &f->fmt.pix_mp;
>
> fmt->num_planes = 1;
> - fmt->plane_fmt[0].sizeimage = fmt->width * fmt->height *
> - RKVDEC_H264_MAX_DEPTH_IN_BYTES;
> + if (!fmt->plane_fmt[0].sizeimage)
> + fmt->plane_fmt[0].sizeimage = fmt->width * fmt->height *
> + RKVDEC_H264_MAX_DEPTH_IN_BYTES;

Note that the test is more strict then the spec, since this behaviour is within
spec. But in general, the application may have more information about the
incoming stream, the maximum encoded frame size would even be encoded in the
container (which is parsed in userspace). So I agree it will be more flexible to
accept userspace desired size. If that size is too small, userspace will fail at
filling it in the first place, and decoding won't be possible, that's all.

Perhaps we could make a recommendation in that sense in the spec ?

Reviewed-by: Nicolas Dufresne <[email protected]>

> return 0;
> }
>

2021-10-13 14:25:14

by Ezequiel Garcia

Subject: Re: [PATCH 1/2] media: rkvdec: Do not override sizeimage for output format

Hi Chen-Yu,

On Fri, Oct 08, 2021 at 06:04:22PM +0800, Chen-Yu Tsai wrote:
> The rkvdec H.264 decoder currently overrides sizeimage for the output
> format. This causes issues when userspace requires and requests a larger
> buffer, but ends up with one of insufficient size.
>
> Instead, only provide a default size if none was requested. This fixes
> the video_decode_accelerator_tests from Chromium failing on the first
> frame due to insufficient buffer space. It also aligns the behavior
> of the rkvdec driver with the Hantro and Cedrus drivers.
>
> Fixes: cd33c830448b ("media: rkvdec: Add the rkvdec driver")
> Cc: <[email protected]>
> Signed-off-by: Chen-Yu Tsai <[email protected]>

Reviewed-by: Ezequiel Garcia <[email protected]>

Thanks for taking care of this!

Ezequiel

> ---
> drivers/staging/media/rkvdec/rkvdec-h264.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
> index 76e97cbe2512..951e19231da2 100644
> --- a/drivers/staging/media/rkvdec/rkvdec-h264.c
> +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
> @@ -1015,8 +1015,9 @@ static int rkvdec_h264_adjust_fmt(struct rkvdec_ctx *ctx,
> struct v4l2_pix_format_mplane *fmt = &f->fmt.pix_mp;
>
> fmt->num_planes = 1;
> - fmt->plane_fmt[0].sizeimage = fmt->width * fmt->height *
> - RKVDEC_H264_MAX_DEPTH_IN_BYTES;
> + if (!fmt->plane_fmt[0].sizeimage)
> + fmt->plane_fmt[0].sizeimage = fmt->width * fmt->height *
> + RKVDEC_H264_MAX_DEPTH_IN_BYTES;
> return 0;
> }
>
> --
> 2.33.0.882.g93a45727a2-goog
>