diff mbox series

[v2,15/23] media: rkvdec: h264: Validate and use pic width and height in mbs

Message ID 20220331193726.289559-16-nicolas.dufresne@collabora.com (mailing list archive)
State New, archived
Headers show
Series None | expand

Commit Message

Nicolas Dufresne March 31, 2022, 7:37 p.m. UTC 
From: Jonas Karlman <jonas@kwiboo.se>

The width and height in macroblocks is currently configured based on OUTPUT
buffer resolution, this works for frame pictures but can cause issues for
field pictures.

When frame_mbs_only_flag is 0 the height in mbs should be height of
the field instead of height of frame.

Validate pic_width_in_mbs_minus1 and pic_height_in_map_units_minus1
against OUTPUT buffer resolution and use these values to configure HW.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Reviewed-by: Sebastian Fricke <sebastian.fricke@collabora.com>
---
 drivers/staging/media/rkvdec/rkvdec-h264.c |  4 ++--
 drivers/staging/media/rkvdec/rkvdec.c      | 10 ++++++++++
 2 files changed, 12 insertions(+), 2 deletions(-)

Comments

Ezequiel Garcia April 2, 2022, 11:32 a.m. UTC | #1 
Hi Nicolas,

On Thu, Mar 31, 2022 at 03:37:17PM -0400, Nicolas Dufresne wrote:
> From: Jonas Karlman <jonas@kwiboo.se>
> 
> The width and height in macroblocks is currently configured based on OUTPUT
> buffer resolution, this works for frame pictures but can cause issues for
> field pictures.
> 
> When frame_mbs_only_flag is 0 the height in mbs should be height of
> the field instead of height of frame.
> 
> Validate pic_width_in_mbs_minus1 and pic_height_in_map_units_minus1
> against OUTPUT buffer resolution and use these values to configure HW.
> 
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> Reviewed-by: Sebastian Fricke <sebastian.fricke@collabora.com>
> ---
>  drivers/staging/media/rkvdec/rkvdec-h264.c |  4 ++--
>  drivers/staging/media/rkvdec/rkvdec.c      | 10 ++++++++++
>  2 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
> index 8d44a884a52e..a42cf19bcc6d 100644
> --- a/drivers/staging/media/rkvdec/rkvdec-h264.c
> +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
> @@ -672,8 +672,8 @@ static void assemble_hw_pps(struct rkvdec_ctx *ctx,
>  		  LOG2_MAX_PIC_ORDER_CNT_LSB_MINUS4);
>  	WRITE_PPS(!!(sps->flags & V4L2_H264_SPS_FLAG_DELTA_PIC_ORDER_ALWAYS_ZERO),
>  		  DELTA_PIC_ORDER_ALWAYS_ZERO_FLAG);
> -	WRITE_PPS(DIV_ROUND_UP(ctx->coded_fmt.fmt.pix_mp.width, 16), PIC_WIDTH_IN_MBS);
> -	WRITE_PPS(DIV_ROUND_UP(ctx->coded_fmt.fmt.pix_mp.height, 16), PIC_HEIGHT_IN_MBS);

Please add a comment so we don't forget why we use the bitstream
fields here.

> +	WRITE_PPS(sps->pic_width_in_mbs_minus1 + 1, PIC_WIDTH_IN_MBS);
> +	WRITE_PPS(sps->pic_height_in_map_units_minus1 + 1, PIC_HEIGHT_IN_MBS);
>  	WRITE_PPS(!!(sps->flags & V4L2_H264_SPS_FLAG_FRAME_MBS_ONLY),
>  		  FRAME_MBS_ONLY_FLAG);
>  	WRITE_PPS(!!(sps->flags & V4L2_H264_SPS_FLAG_MB_ADAPTIVE_FRAME_FIELD),
> diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
> index 2df8cf4883e2..1b805710e195 100644
> --- a/drivers/staging/media/rkvdec/rkvdec.c
> +++ b/drivers/staging/media/rkvdec/rkvdec.c
> @@ -29,8 +29,11 @@
>  
>  static int rkvdec_try_ctrl(struct v4l2_ctrl *ctrl)
>  {
> +	struct rkvdec_ctx *ctx = container_of(ctrl->handler, struct rkvdec_ctx, ctrl_hdl);
> +
>  	if (ctrl->id == V4L2_CID_STATELESS_H264_SPS) {
>  		const struct v4l2_ctrl_h264_sps *sps = ctrl->p_new.p_h264_sps;
> +		unsigned int width, height;
>  		/*
>  		 * TODO: The hardware supports 10-bit and 4:2:2 profiles,
>  		 * but it's currently broken in the driver.
> @@ -45,6 +48,13 @@ static int rkvdec_try_ctrl(struct v4l2_ctrl *ctrl)
>  		if (sps->bit_depth_luma_minus8 != 0)
>  			/* Only 8-bit is supported */
>  			return -EINVAL;
> +
> +		width = (sps->pic_width_in_mbs_minus1 + 1) * 16;
> +		height = (sps->pic_height_in_map_units_minus1 + 1) * 16;
> +

Let's please add a comment here, clarifying it's legal to check
the coded format (OUTPUT queue format) at .try_ctrl time,
because the stateless decoder specification [1] mandates
S_FMT on the OUTPUT queue, before passing the SPS/PPS controls.

[1] https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/dev-stateless-decoder.html

> +		if (width > ctx->coded_fmt.fmt.pix_mp.width ||
> +		    height > ctx->coded_fmt.fmt.pix_mp.height)

Can you add a debug message or error message?
These silent errors tend to get super hard to track.

With these changes:

Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>

Thanks,
Ezequiel
Nicolas Dufresne April 5, 2022, 3:44 p.m. UTC | #2 
Le samedi 02 avril 2022 à 08:32 -0300, Ezequiel Garcia a écrit :
> Hi Nicolas,
> 
> On Thu, Mar 31, 2022 at 03:37:17PM -0400, Nicolas Dufresne wrote:
> > From: Jonas Karlman <jonas@kwiboo.se>
> > 
> > The width and height in macroblocks is currently configured based on OUTPUT
> > buffer resolution, this works for frame pictures but can cause issues for
> > field pictures.
> > 
> > When frame_mbs_only_flag is 0 the height in mbs should be height of
> > the field instead of height of frame.
> > 
> > Validate pic_width_in_mbs_minus1 and pic_height_in_map_units_minus1
> > against OUTPUT buffer resolution and use these values to configure HW.
> > 
> > Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > Reviewed-by: Sebastian Fricke <sebastian.fricke@collabora.com>
> > ---
> >  drivers/staging/media/rkvdec/rkvdec-h264.c |  4 ++--
> >  drivers/staging/media/rkvdec/rkvdec.c      | 10 ++++++++++
> >  2 files changed, 12 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
> > index 8d44a884a52e..a42cf19bcc6d 100644
> > --- a/drivers/staging/media/rkvdec/rkvdec-h264.c
> > +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
> > @@ -672,8 +672,8 @@ static void assemble_hw_pps(struct rkvdec_ctx *ctx,
> >  		  LOG2_MAX_PIC_ORDER_CNT_LSB_MINUS4);
> >  	WRITE_PPS(!!(sps->flags & V4L2_H264_SPS_FLAG_DELTA_PIC_ORDER_ALWAYS_ZERO),
> >  		  DELTA_PIC_ORDER_ALWAYS_ZERO_FLAG);
> > -	WRITE_PPS(DIV_ROUND_UP(ctx->coded_fmt.fmt.pix_mp.width, 16), PIC_WIDTH_IN_MBS);
> > -	WRITE_PPS(DIV_ROUND_UP(ctx->coded_fmt.fmt.pix_mp.height, 16), PIC_HEIGHT_IN_MBS);
> 
> Please add a comment so we don't forget why we use the bitstream
> fields here.

And perhaps I should clarify that only the height will vary. It remains nice if
we can decode smaller images into larger image/format, that will be needed to
handle the sub-layers in SVC (these are not to be displayed, so we don't care
much about the output stride and all). So that is also an improvement.

> 
> > +	WRITE_PPS(sps->pic_width_in_mbs_minus1 + 1, PIC_WIDTH_IN_MBS);
> > +	WRITE_PPS(sps->pic_height_in_map_units_minus1 + 1, PIC_HEIGHT_IN_MBS);
> >  	WRITE_PPS(!!(sps->flags & V4L2_H264_SPS_FLAG_FRAME_MBS_ONLY),
> >  		  FRAME_MBS_ONLY_FLAG);
> >  	WRITE_PPS(!!(sps->flags & V4L2_H264_SPS_FLAG_MB_ADAPTIVE_FRAME_FIELD),
> > diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
> > index 2df8cf4883e2..1b805710e195 100644
> > --- a/drivers/staging/media/rkvdec/rkvdec.c
> > +++ b/drivers/staging/media/rkvdec/rkvdec.c
> > @@ -29,8 +29,11 @@
> >  
> >  static int rkvdec_try_ctrl(struct v4l2_ctrl *ctrl)
> >  {
> > +	struct rkvdec_ctx *ctx = container_of(ctrl->handler, struct rkvdec_ctx, ctrl_hdl);
> > +
> >  	if (ctrl->id == V4L2_CID_STATELESS_H264_SPS) {
> >  		const struct v4l2_ctrl_h264_sps *sps = ctrl->p_new.p_h264_sps;
> > +		unsigned int width, height;
> >  		/*
> >  		 * TODO: The hardware supports 10-bit and 4:2:2 profiles,
> >  		 * but it's currently broken in the driver.
> > @@ -45,6 +48,13 @@ static int rkvdec_try_ctrl(struct v4l2_ctrl *ctrl)
> >  		if (sps->bit_depth_luma_minus8 != 0)
> >  			/* Only 8-bit is supported */
> >  			return -EINVAL;
> > +
> > +		width = (sps->pic_width_in_mbs_minus1 + 1) * 16;
> > +		height = (sps->pic_height_in_map_units_minus1 + 1) * 16;
> > +
> 
> Let's please add a comment here, clarifying it's legal to check
> the coded format (OUTPUT queue format) at .try_ctrl time,
> because the stateless decoder specification [1] mandates
> S_FMT on the OUTPUT queue, before passing the SPS/PPS controls.

Indeed, though I come to see some flaw in the validation. First, the height
formula shall be base on formula 7-18 from the spec:

  FrameHeightInMbs = ( 2 − frame_mbs_only_flag ) * PicHeightInMapUnits

So would be

+		height = (sps->pic_height_in_map_units_minus1 + 1) * 16;
+		if (sps->flags & V4L2_H264_SPS_FLAG_FRAME_MBS_ONLY)
+			height *= 2;

As this driver do interleaved interlaced buffer, not alternate. Finally, we
should validate this again at STREAMON, since userland may have simply omitted
to set the SPS at all. I'll try to improve this and drop the review tag to
ensure this get fully reviewed again.

> 
> [1] https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/dev-stateless-decoder.html
> 
> > +		if (width > ctx->coded_fmt.fmt.pix_mp.width ||
> > +		    height > ctx->coded_fmt.fmt.pix_mp.height)
> 
> Can you add a debug message or error message?
> These silent errors tend to get super hard to track.
> 
> With these changes:
> 
> Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> 
> Thanks,
> Ezequiel
Nicolas Dufresne April 5, 2022, 4:04 p.m. UTC | #3 
Le mardi 05 avril 2022 à 11:44 -0400, Nicolas Dufresne a écrit :
> Le samedi 02 avril 2022 à 08:32 -0300, Ezequiel Garcia a écrit :
> > Hi Nicolas,
> > 
> > On Thu, Mar 31, 2022 at 03:37:17PM -0400, Nicolas Dufresne wrote:
> > > From: Jonas Karlman <jonas@kwiboo.se>
> > > 
> > > The width and height in macroblocks is currently configured based on OUTPUT
> > > buffer resolution, this works for frame pictures but can cause issues for
> > > field pictures.
> > > 
> > > When frame_mbs_only_flag is 0 the height in mbs should be height of
> > > the field instead of height of frame.
> > > 
> > > Validate pic_width_in_mbs_minus1 and pic_height_in_map_units_minus1
> > > against OUTPUT buffer resolution and use these values to configure HW.
> > > 
> > > Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> > > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > > Reviewed-by: Sebastian Fricke <sebastian.fricke@collabora.com>
> > > ---
> > >  drivers/staging/media/rkvdec/rkvdec-h264.c |  4 ++--
> > >  drivers/staging/media/rkvdec/rkvdec.c      | 10 ++++++++++
> > >  2 files changed, 12 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
> > > index 8d44a884a52e..a42cf19bcc6d 100644
> > > --- a/drivers/staging/media/rkvdec/rkvdec-h264.c
> > > +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
> > > @@ -672,8 +672,8 @@ static void assemble_hw_pps(struct rkvdec_ctx *ctx,
> > >  		  LOG2_MAX_PIC_ORDER_CNT_LSB_MINUS4);
> > >  	WRITE_PPS(!!(sps->flags & V4L2_H264_SPS_FLAG_DELTA_PIC_ORDER_ALWAYS_ZERO),
> > >  		  DELTA_PIC_ORDER_ALWAYS_ZERO_FLAG);
> > > -	WRITE_PPS(DIV_ROUND_UP(ctx->coded_fmt.fmt.pix_mp.width, 16), PIC_WIDTH_IN_MBS);
> > > -	WRITE_PPS(DIV_ROUND_UP(ctx->coded_fmt.fmt.pix_mp.height, 16), PIC_HEIGHT_IN_MBS);
> > 
> > Please add a comment so we don't forget why we use the bitstream
> > fields here.
> 
> And perhaps I should clarify that only the height will vary. It remains nice if
> we can decode smaller images into larger image/format, that will be needed to
> handle the sub-layers in SVC (these are not to be displayed, so we don't care
> much about the output stride and all). So that is also an improvement.
> 
> > 
> > > +	WRITE_PPS(sps->pic_width_in_mbs_minus1 + 1, PIC_WIDTH_IN_MBS);
> > > +	WRITE_PPS(sps->pic_height_in_map_units_minus1 + 1, PIC_HEIGHT_IN_MBS);
> > >  	WRITE_PPS(!!(sps->flags & V4L2_H264_SPS_FLAG_FRAME_MBS_ONLY),
> > >  		  FRAME_MBS_ONLY_FLAG);
> > >  	WRITE_PPS(!!(sps->flags & V4L2_H264_SPS_FLAG_MB_ADAPTIVE_FRAME_FIELD),
> > > diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
> > > index 2df8cf4883e2..1b805710e195 100644
> > > --- a/drivers/staging/media/rkvdec/rkvdec.c
> > > +++ b/drivers/staging/media/rkvdec/rkvdec.c
> > > @@ -29,8 +29,11 @@
> > >  
> > >  static int rkvdec_try_ctrl(struct v4l2_ctrl *ctrl)
> > >  {
> > > +	struct rkvdec_ctx *ctx = container_of(ctrl->handler, struct rkvdec_ctx, ctrl_hdl);
> > > +
> > >  	if (ctrl->id == V4L2_CID_STATELESS_H264_SPS) {
> > >  		const struct v4l2_ctrl_h264_sps *sps = ctrl->p_new.p_h264_sps;
> > > +		unsigned int width, height;
> > >  		/*
> > >  		 * TODO: The hardware supports 10-bit and 4:2:2 profiles,
> > >  		 * but it's currently broken in the driver.
> > > @@ -45,6 +48,13 @@ static int rkvdec_try_ctrl(struct v4l2_ctrl *ctrl)
> > >  		if (sps->bit_depth_luma_minus8 != 0)
> > >  			/* Only 8-bit is supported */
> > >  			return -EINVAL;
> > > +
> > > +		width = (sps->pic_width_in_mbs_minus1 + 1) * 16;
> > > +		height = (sps->pic_height_in_map_units_minus1 + 1) * 16;
> > > +
> > 
> > Let's please add a comment here, clarifying it's legal to check
> > the coded format (OUTPUT queue format) at .try_ctrl time,
> > because the stateless decoder specification [1] mandates
> > S_FMT on the OUTPUT queue, before passing the SPS/PPS controls.
> 
> Indeed, though I come to see some flaw in the validation. First, the height
> formula shall be base on formula 7-18 from the spec:
> 
>   FrameHeightInMbs = ( 2 − frame_mbs_only_flag ) * PicHeightInMapUnits
> 
> So would be
> 
> +		height = (sps->pic_height_in_map_units_minus1 + 1) * 16;
> +		if (sps->flags & V4L2_H264_SPS_FLAG_FRAME_MBS_ONLY)
> +			height *= 2;

Oops, I meant:


+		if (!(sps->flags & V4L2_H264_SPS_FLAG_FRAME_MBS_ONLY))
+			height *= 2;


> 
> As this driver do interleaved interlaced buffer, not alternate. Finally, we
> should validate this again at STREAMON, since userland may have simply omitted
> to set the SPS at all. I'll try to improve this and drop the review tag to
> ensure this get fully reviewed again.
> 
> > 
> > [1] https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/dev-stateless-decoder.html
> > 
> > > +		if (width > ctx->coded_fmt.fmt.pix_mp.width ||
> > > +		    height > ctx->coded_fmt.fmt.pix_mp.height)
> > 
> > Can you add a debug message or error message?
> > These silent errors tend to get super hard to track.
> > 
> > With these changes:
> > 
> > Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> > 
> > Thanks,
> > Ezequiel
>
diff mbox series

Patch

diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
index 8d44a884a52e..a42cf19bcc6d 100644
--- a/drivers/staging/media/rkvdec/rkvdec-h264.c
+++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
@@ -672,8 +672,8 @@  static void assemble_hw_pps(struct rkvdec_ctx *ctx,
 		  LOG2_MAX_PIC_ORDER_CNT_LSB_MINUS4);
 	WRITE_PPS(!!(sps->flags & V4L2_H264_SPS_FLAG_DELTA_PIC_ORDER_ALWAYS_ZERO),
 		  DELTA_PIC_ORDER_ALWAYS_ZERO_FLAG);
-	WRITE_PPS(DIV_ROUND_UP(ctx->coded_fmt.fmt.pix_mp.width, 16), PIC_WIDTH_IN_MBS);
-	WRITE_PPS(DIV_ROUND_UP(ctx->coded_fmt.fmt.pix_mp.height, 16), PIC_HEIGHT_IN_MBS);
+	WRITE_PPS(sps->pic_width_in_mbs_minus1 + 1, PIC_WIDTH_IN_MBS);
+	WRITE_PPS(sps->pic_height_in_map_units_minus1 + 1, PIC_HEIGHT_IN_MBS);
 	WRITE_PPS(!!(sps->flags & V4L2_H264_SPS_FLAG_FRAME_MBS_ONLY),
 		  FRAME_MBS_ONLY_FLAG);
 	WRITE_PPS(!!(sps->flags & V4L2_H264_SPS_FLAG_MB_ADAPTIVE_FRAME_FIELD),
diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
index 2df8cf4883e2..1b805710e195 100644
--- a/drivers/staging/media/rkvdec/rkvdec.c
+++ b/drivers/staging/media/rkvdec/rkvdec.c
@@ -29,8 +29,11 @@ 
 
 static int rkvdec_try_ctrl(struct v4l2_ctrl *ctrl)
 {
+	struct rkvdec_ctx *ctx = container_of(ctrl->handler, struct rkvdec_ctx, ctrl_hdl);
+
 	if (ctrl->id == V4L2_CID_STATELESS_H264_SPS) {
 		const struct v4l2_ctrl_h264_sps *sps = ctrl->p_new.p_h264_sps;
+		unsigned int width, height;
 		/*
 		 * TODO: The hardware supports 10-bit and 4:2:2 profiles,
 		 * but it's currently broken in the driver.
@@ -45,6 +48,13 @@  static int rkvdec_try_ctrl(struct v4l2_ctrl *ctrl)
 		if (sps->bit_depth_luma_minus8 != 0)
 			/* Only 8-bit is supported */
 			return -EINVAL;
+
+		width = (sps->pic_width_in_mbs_minus1 + 1) * 16;
+		height = (sps->pic_height_in_map_units_minus1 + 1) * 16;
+
+		if (width > ctx->coded_fmt.fmt.pix_mp.width ||
+		    height > ctx->coded_fmt.fmt.pix_mp.height)
+			return -EINVAL;
 	}
 	return 0;
 }