[PATCH 0/2] media: hantro/rkvdec handle unsupported H.264 bitstreams

[PATCH 0/2] media: hantro/rkvdec handle unsupported H.264 bitstreams

[Ezequiel Garcia]

Hi all,

Small patchset to add a check at TRY_EXT_CTRLS time,
via the H264 SPS control and reject unsupported bitstreams.

Properly refusing to decode unsupported bitstreams
allows applications to cleanly fallback to software
decoding.

Note that Rockchip VDEC hardware is capable of decoding High-10
and High-422 bitstreams. This needs more work, so for now
they are refused.

The same approach can be use on Cedrus, but since I'm not
very familiar there, I'll leave that to others.

Applies on top of media master.

Ezequiel Garcia (2):
  rkvdec: h264: Refuse to decode unsupported bitstream
  hantro: h264: Refuse to decode unsupported bitstream

 drivers/staging/media/hantro/hantro_drv.c | 29 ++++++++++++++++++++---
 drivers/staging/media/rkvdec/rkvdec.c     | 27 +++++++++++++++++++++
 2 files changed, 53 insertions(+), 3 deletions(-)

-- 
2.26.0.rc2

[PATCH 1/2] rkvdec: h264: Refuse to decode unsupported bitstream

[Ezequiel Garcia]

The hardware only supports 4:2:2, 4:2:0 or 4:0:0 (monochrome),
8-bit or 10-bit depth content.

Verify that the PPS refers to a supported bitstream, and refuse
unsupported bitstreams by failing at TRY_EXT_CTRLS time.

The driver is currently broken on 10-bit and 4:2:2
so disallow those as well.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/staging/media/rkvdec/rkvdec.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
index 225eeca73356..0f81b47792f6 100644
--- a/drivers/staging/media/rkvdec/rkvdec.c
+++ b/drivers/staging/media/rkvdec/rkvdec.c
@@ -27,6 +27,32 @@
 #include "rkvdec.h"
 #include "rkvdec-regs.h"
 
+static int rkvdec_try_ctrl(struct v4l2_ctrl *ctrl)
+{
+	if (ctrl->id == V4L2_CID_MPEG_VIDEO_H264_SPS) {
+		const struct v4l2_ctrl_h264_sps *sps = ctrl->p_cur.p;
+		/*
+		 * TODO: The hardware supports 10-bit and 4:2:2 profiles,
+		 * but it's currently broken in the driver.
+		 * Reject them for now, until it's fixed.
+		 */
+		if (sps->chroma_format_idc > 1)
+			/* Only 4:0:0 and 4:2:0 are supported */
+			return -EINVAL;
+		if (sps->bit_depth_luma_minus8 != sps->bit_depth_chroma_minus8)
+			/* Luma and chroma bit depth mismatch */
+			return -EINVAL;
+		if (sps->bit_depth_luma_minus8 != 0)
+			/* Only 8-bit is supported */
+			return -EINVAL;
+	}
+	return 0;
+}
+
+static const struct v4l2_ctrl_ops rkvdec_ctrl_ops = {
+	.try_ctrl = rkvdec_try_ctrl,
+};
+
 static const struct rkvdec_ctrl_desc rkvdec_h264_ctrl_descs[] = {
 	{
 		.per_request = true,
@@ -42,6 +68,7 @@ static const struct rkvdec_ctrl_desc rkvdec_h264_ctrl_descs[] = {
 		.per_request = true,
 		.mandatory = true,
 		.cfg.id = V4L2_CID_MPEG_VIDEO_H264_SPS,
+		.cfg.ops = &rkvdec_ctrl_ops,
 	},
 	{
 		.per_request = true,
-- 
2.26.0.rc2

[PATCH 2/2] hantro: h264: Refuse to decode unsupported bitstream

[Ezequiel Garcia]

The hardware only supports 4:2:0 or 4:0:0 (monochrome),
8-bit depth content.

Verify that the PPS refers to a supported bitstream, and refuse
unsupported bitstreams by failing at TRY_EXT_CTRLS time.

Given the JPEG compression level control is the only one
that needs setting, a specific ops is provided.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/staging/media/hantro/hantro_drv.c | 29 ++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
index 0db8ad455160..361ffaa821ef 100644
--- a/drivers/staging/media/hantro/hantro_drv.c
+++ b/drivers/staging/media/hantro/hantro_drv.c
@@ -261,7 +261,25 @@ queue_init(void *priv, struct vb2_queue *src_vq, struct vb2_queue *dst_vq)
 	return vb2_queue_init(dst_vq);
 }
 
-static int hantro_s_ctrl(struct v4l2_ctrl *ctrl)
+static int hantro_try_ctrl(struct v4l2_ctrl *ctrl)
+{
+	if (ctrl->id == V4L2_CID_MPEG_VIDEO_H264_SPS) {
+		const struct v4l2_ctrl_h264_sps *sps = ctrl->p_cur.p;
+
+		if (sps->chroma_format_idc > 1)
+			/* Only 4:0:0 and 4:2:0 are supported */
+			return -EINVAL;
+		if (sps->bit_depth_luma_minus8 != sps->bit_depth_chroma_minus8)
+			/* Luma and chroma bit depth mismatch */
+			return -EINVAL;
+		if (sps->bit_depth_luma_minus8 != 0)
+			/* Only 8-bit is supported */
+			return -EINVAL;
+	}
+	return 0;
+}
+
+static int hantro_jpeg_s_ctrl(struct v4l2_ctrl *ctrl)
 {
 	struct hantro_ctx *ctx;
 
@@ -282,7 +300,11 @@ static int hantro_s_ctrl(struct v4l2_ctrl *ctrl)
 }
 
 static const struct v4l2_ctrl_ops hantro_ctrl_ops = {
-	.s_ctrl = hantro_s_ctrl,
+	.try_ctrl = hantro_try_ctrl,
+};
+
+static const struct v4l2_ctrl_ops hantro_jpeg_ctrl_ops = {
+	.s_ctrl = hantro_jpeg_s_ctrl,
 };
 
 static const struct hantro_ctrl controls[] = {
@@ -294,7 +316,7 @@ static const struct hantro_ctrl controls[] = {
 			.max = 100,
 			.step = 1,
 			.def = 50,
-			.ops = &hantro_ctrl_ops,
+			.ops = &hantro_jpeg_ctrl_ops,
 		},
 	}, {
 		.codec = HANTRO_MPEG2_DECODER,
@@ -325,6 +347,7 @@ static const struct hantro_ctrl controls[] = {
 		.codec = HANTRO_H264_DECODER,
 		.cfg = {
 			.id = V4L2_CID_MPEG_VIDEO_H264_SPS,
+			.ops = &hantro_ctrl_ops,
 		},
 	}, {
 		.codec = HANTRO_H264_DECODER,
-- 
2.26.0.rc2

Re: [PATCH 2/2] hantro: h264: Refuse to decode unsupported bitstream

[Philipp Zabel]

On Fri, 2020-06-26 at 14:11 -0300, Ezequiel Garcia wrote:
> The hardware only supports 4:2:0 or 4:0:0 (monochrome),
> 8-bit depth content.
> 
> Verify that the PPS refers to a supported bitstream, and refuse
> unsupported bitstreams by failing at TRY_EXT_CTRLS time.
> 
> Given the JPEG compression level control is the only one
> that needs setting, a specific ops is provided.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp

Re: [PATCH 1/2] rkvdec: h264: Refuse to decode unsupported bitstream

[Jonas Karlman]

On 2020-06-26 19:11, Ezequiel Garcia wrote:
> The hardware only supports 4:2:2, 4:2:0 or 4:0:0 (monochrome),
> 8-bit or 10-bit depth content.
> 
> Verify that the PPS refers to a supported bitstream, and refuse

This should be SPS not PPS, same for hantro patch.

> unsupported bitstreams by failing at TRY_EXT_CTRLS time.
> 
> The driver is currently broken on 10-bit and 4:2:2
> so disallow those as well.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> ---
>  drivers/staging/media/rkvdec/rkvdec.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
> index 225eeca73356..0f81b47792f6 100644
> --- a/drivers/staging/media/rkvdec/rkvdec.c
> +++ b/drivers/staging/media/rkvdec/rkvdec.c
> @@ -27,6 +27,32 @@
>  #include "rkvdec.h"
>  #include "rkvdec-regs.h"
>  
> +static int rkvdec_try_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	if (ctrl->id == V4L2_CID_MPEG_VIDEO_H264_SPS) {
> +		const struct v4l2_ctrl_h264_sps *sps = ctrl->p_cur.p;

This should be p_new and not p_cur to validate the new ctrl value, same for hantro patch.

With both fixed this and the hantro patch is,

Reviewed-by: Jonas Karlman <jonas@kwiboo.se>

Regards,
Jonas

> +		/*
> +		 * TODO: The hardware supports 10-bit and 4:2:2 profiles,
> +		 * but it's currently broken in the driver.
> +		 * Reject them for now, until it's fixed.
> +		 */
> +		if (sps->chroma_format_idc > 1)
> +			/* Only 4:0:0 and 4:2:0 are supported */
> +			return -EINVAL;
> +		if (sps->bit_depth_luma_minus8 != sps->bit_depth_chroma_minus8)
> +			/* Luma and chroma bit depth mismatch */
> +			return -EINVAL;
> +		if (sps->bit_depth_luma_minus8 != 0)
> +			/* Only 8-bit is supported */
> +			return -EINVAL;
> +	}
> +	return 0;
> +}
> +
> +static const struct v4l2_ctrl_ops rkvdec_ctrl_ops = {
> +	.try_ctrl = rkvdec_try_ctrl,
> +};
> +
>  static const struct rkvdec_ctrl_desc rkvdec_h264_ctrl_descs[] = {
>  	{
>  		.per_request = true,
> @@ -42,6 +68,7 @@ static const struct rkvdec_ctrl_desc rkvdec_h264_ctrl_descs[] = {
>  		.per_request = true,
>  		.mandatory = true,
>  		.cfg.id = V4L2_CID_MPEG_VIDEO_H264_SPS,
> +		.cfg.ops = &rkvdec_ctrl_ops,
>  	},
>  	{
>  		.per_request = true,
> 

Re: [PATCH 1/2] rkvdec: h264: Refuse to decode unsupported bitstream

[Ezequiel Garcia]

On Mon, 2020-07-06 at 09:45 +0000, Jonas Karlman wrote:
> On 2020-06-26 19:11, Ezequiel Garcia wrote:
> > The hardware only supports 4:2:2, 4:2:0 or 4:0:0 (monochrome),
> > 8-bit or 10-bit depth content.
> > 
> > Verify that the PPS refers to a supported bitstream, and refuse
> 
> This should be SPS not PPS, same for hantro patch.
> 

Yup.

> > unsupported bitstreams by failing at TRY_EXT_CTRLS time.
> > 
> > The driver is currently broken on 10-bit and 4:2:2
> > so disallow those as well.
> > 
> > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > ---
> >  drivers/staging/media/rkvdec/rkvdec.c | 27 +++++++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> > 
> > diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
> > index 225eeca73356..0f81b47792f6 100644
> > --- a/drivers/staging/media/rkvdec/rkvdec.c
> > +++ b/drivers/staging/media/rkvdec/rkvdec.c
> > @@ -27,6 +27,32 @@
> >  #include "rkvdec.h"
> >  #include "rkvdec-regs.h"
> >  
> > +static int rkvdec_try_ctrl(struct v4l2_ctrl *ctrl)
> > +{
> > +	if (ctrl->id == V4L2_CID_MPEG_VIDEO_H264_SPS) {
> > +		const struct v4l2_ctrl_h264_sps *sps = ctrl->p_cur.p;
> 
> This should be p_new and not p_cur to validate the new ctrl value, same for hantro patch.
> 
> With both fixed this and the hantro patch is,
> 
> Reviewed-by: Jonas Karlman <jonas@kwiboo.se>
> 

Ah, nice catch. Will fix.

Thanks,
Ezequiel

> Regards,
> Jonas
> 
> > +		/*
> > +		 * TODO: The hardware supports 10-bit and 4:2:2 profiles,
> > +		 * but it's currently broken in the driver.
> > +		 * Reject them for now, until it's fixed.
> > +		 */
> > +		if (sps->chroma_format_idc > 1)
> > +			/* Only 4:0:0 and 4:2:0 are supported */
> > +			return -EINVAL;
> > +		if (sps->bit_depth_luma_minus8 != sps->bit_depth_chroma_minus8)
> > +			/* Luma and chroma bit depth mismatch */
> > +			return -EINVAL;
> > +		if (sps->bit_depth_luma_minus8 != 0)
> > +			/* Only 8-bit is supported */
> > +			return -EINVAL;
> > +	}
> > +	return 0;
> > +}
> > +
> > +static const struct v4l2_ctrl_ops rkvdec_ctrl_ops = {
> > +	.try_ctrl = rkvdec_try_ctrl,
> > +};
> > +
> >  static const struct rkvdec_ctrl_desc rkvdec_h264_ctrl_descs[] = {
> >  	{
> >  		.per_request = true,
> > @@ -42,6 +68,7 @@ static const struct rkvdec_ctrl_desc rkvdec_h264_ctrl_descs[] = {
> >  		.per_request = true,
> >  		.mandatory = true,
> >  		.cfg.id = V4L2_CID_MPEG_VIDEO_H264_SPS,
> > +		.cfg.ops = &rkvdec_ctrl_ops,
> >  	},
> >  	{
> >  		.per_request = true,
> >