diff mbox series

[v1,24/24] media: rkvdec-h264: Don't hardcode SPS/PPS parameters

Message ID 20220328195936.82552-25-nicolas.dufresne@collabora.com (mailing list archive)
State Superseded
Headers
Series [v1,01/24] media: h264: Increase reference lists size to 32 |

Commit Message

Nicolas Dufresne March 28, 2022, 7:59 p.m. UTC 
  From: Alex Bee <knaerzche@gmail.com>

Some SPS/PPS parameters are currently hardcoded in the driver
even though so do exist in the uapi which is stable by now.

Use them instead of hardcoding them.

Conformance tests have shown there is no difference, but it might
increase decoder performance.

Signed-off-by: Alex Bee <knaerzche@gmail.com>
---
 drivers/staging/media/rkvdec/rkvdec-h264.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

Comments

sebastian.fricke@collabora.com March 29, 2022, 7:22 a.m. UTC | #1 
Hey Nicolas,

The patch series doesn't seem to apply on the latest media tree master
branch. Looking at your tree I can see that you have commit: ba2c670a
"media: nxp: Restrict VIDEO_IMX_MIPI_CSIS to ARCH_MXC or COMPILE_TEST
Laurent Pinchart authored 1 week ago "

But the current head of the media tree is: 71e6d0608e4d
"media: platform: Remove unnecessary print function dev_err()
Yang Li authored 13 days ago"

On 28.03.2022 15:59, Nicolas Dufresne wrote:
>From: Alex Bee <knaerzche@gmail.com>
>
>Some SPS/PPS parameters are currently hardcoded in the driver
>even though so do exist in the uapi which is stable by now.

s/even though so/even though they/
>
>Use them instead of hardcoding them.
>
>Conformance tests have shown there is no difference, but it might
>increase decoder performance.

I think it would be great if we could add some performance metrics to
the commit description to have a metric that following patches could
compare themselves with.

Greetings,
Sebastian

>
>Signed-off-by: Alex Bee <knaerzche@gmail.com>
>---
> drivers/staging/media/rkvdec/rkvdec-h264.c | 13 +++++++------
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
>diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
>index 891c48bf6a51..91f65d78e453 100644
>--- a/drivers/staging/media/rkvdec/rkvdec-h264.c
>+++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
>@@ -655,13 +655,14 @@ static void assemble_hw_pps(struct rkvdec_ctx *ctx,
>
> #define WRITE_PPS(value, field) set_ps_field(hw_ps->info, field, value)
> 	/* write sps */
>-	WRITE_PPS(0xf, SEQ_PARAMETER_SET_ID);
>-	WRITE_PPS(0xff, PROFILE_IDC);
>-	WRITE_PPS(1, CONSTRAINT_SET3_FLAG);
>+	WRITE_PPS(sps->seq_parameter_set_id, SEQ_PARAMETER_SET_ID);
>+	WRITE_PPS(sps->profile_idc, PROFILE_IDC);
>+	WRITE_PPS((sps->constraint_set_flags & 1 << 3) ? 1 : 0, CONSTRAINT_SET3_FLAG);
> 	WRITE_PPS(sps->chroma_format_idc, CHROMA_FORMAT_IDC);
> 	WRITE_PPS(sps->bit_depth_luma_minus8, BIT_DEPTH_LUMA);
> 	WRITE_PPS(sps->bit_depth_chroma_minus8, BIT_DEPTH_CHROMA);
>-	WRITE_PPS(0, QPPRIME_Y_ZERO_TRANSFORM_BYPASS_FLAG);
>+	WRITE_PPS(!!(sps->flags & V4L2_H264_SPS_FLAG_QPPRIME_Y_ZERO_TRANSFORM_BYPASS),
>+		  QPPRIME_Y_ZERO_TRANSFORM_BYPASS_FLAG);
> 	WRITE_PPS(sps->log2_max_frame_num_minus4, LOG2_MAX_FRAME_NUM_MINUS4);
> 	WRITE_PPS(sps->max_num_ref_frames, MAX_NUM_REF_FRAMES);
> 	WRITE_PPS(sps->pic_order_cnt_type, PIC_ORDER_CNT_TYPE);
>@@ -679,8 +680,8 @@ static void assemble_hw_pps(struct rkvdec_ctx *ctx,
> 		  DIRECT_8X8_INFERENCE_FLAG);
>
> 	/* write pps */
>-	WRITE_PPS(0xff, PIC_PARAMETER_SET_ID);
>-	WRITE_PPS(0x1f, PPS_SEQ_PARAMETER_SET_ID);
>+	WRITE_PPS(pps->pic_parameter_set_id, PIC_PARAMETER_SET_ID);
>+	WRITE_PPS(pps->seq_parameter_set_id, PPS_SEQ_PARAMETER_SET_ID);
> 	WRITE_PPS(!!(pps->flags & V4L2_H264_PPS_FLAG_ENTROPY_CODING_MODE),
> 		  ENTROPY_CODING_MODE_FLAG);
> 	WRITE_PPS(!!(pps->flags & V4L2_H264_PPS_FLAG_BOTTOM_FIELD_PIC_ORDER_IN_FRAME_PRESENT),
>-- 
>2.34.1
>
Nicolas Dufresne March 30, 2022, 3:27 p.m. UTC | #2 
Le mardi 29 mars 2022 à 09:22 +0200, Sebastian Fricke a écrit :
> Hey Nicolas,
> 
> The patch series doesn't seem to apply on the latest media tree master
> branch. Looking at your tree I can see that you have commit: ba2c670a
> "media: nxp: Restrict VIDEO_IMX_MIPI_CSIS to ARCH_MXC or COMPILE_TEST
> Laurent Pinchart authored 1 week ago "
> 
> But the current head of the media tree is: 71e6d0608e4d
> "media: platform: Remove unnecessary print function dev_err()
> Yang Li authored 13 days ago"
> 
> On 28.03.2022 15:59, Nicolas Dufresne wrote:
> > From: Alex Bee <knaerzche@gmail.com>
> > 
> > Some SPS/PPS parameters are currently hardcoded in the driver
> > even though so do exist in the uapi which is stable by now.
> 
> s/even though so/even though they/
> > 
> > Use them instead of hardcoding them.
> > 
> > Conformance tests have shown there is no difference, but it might
> > increase decoder performance.
> 
> I think it would be great if we could add some performance metrics to
> the commit description to have a metric that following patches could
> compare themselves with.

Alex, can you extend on this one? I'm not sure how this can impact performance,
so I doubt any mitric will be significant. Can I just drop that part of the
comment ?
> 
> Greetings,
> Sebastian
> 
> > 
> > Signed-off-by: Alex Bee <knaerzche@gmail.com>
> > ---
> > drivers/staging/media/rkvdec/rkvdec-h264.c | 13 +++++++------
> > 1 file changed, 7 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
> > index 891c48bf6a51..91f65d78e453 100644
> > --- a/drivers/staging/media/rkvdec/rkvdec-h264.c
> > +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
> > @@ -655,13 +655,14 @@ static void assemble_hw_pps(struct rkvdec_ctx *ctx,
> > 
> > #define WRITE_PPS(value, field) set_ps_field(hw_ps->info, field, value)
> > 	/* write sps */
> > -	WRITE_PPS(0xf, SEQ_PARAMETER_SET_ID);
> > -	WRITE_PPS(0xff, PROFILE_IDC);
> > -	WRITE_PPS(1, CONSTRAINT_SET3_FLAG);
> > +	WRITE_PPS(sps->seq_parameter_set_id, SEQ_PARAMETER_SET_ID);
> > +	WRITE_PPS(sps->profile_idc, PROFILE_IDC);
> > +	WRITE_PPS((sps->constraint_set_flags & 1 << 3) ? 1 : 0, CONSTRAINT_SET3_FLAG);
> > 	WRITE_PPS(sps->chroma_format_idc, CHROMA_FORMAT_IDC);
> > 	WRITE_PPS(sps->bit_depth_luma_minus8, BIT_DEPTH_LUMA);
> > 	WRITE_PPS(sps->bit_depth_chroma_minus8, BIT_DEPTH_CHROMA);
> > -	WRITE_PPS(0, QPPRIME_Y_ZERO_TRANSFORM_BYPASS_FLAG);
> > +	WRITE_PPS(!!(sps->flags & V4L2_H264_SPS_FLAG_QPPRIME_Y_ZERO_TRANSFORM_BYPASS),
> > +		  QPPRIME_Y_ZERO_TRANSFORM_BYPASS_FLAG);
> > 	WRITE_PPS(sps->log2_max_frame_num_minus4, LOG2_MAX_FRAME_NUM_MINUS4);
> > 	WRITE_PPS(sps->max_num_ref_frames, MAX_NUM_REF_FRAMES);
> > 	WRITE_PPS(sps->pic_order_cnt_type, PIC_ORDER_CNT_TYPE);
> > @@ -679,8 +680,8 @@ static void assemble_hw_pps(struct rkvdec_ctx *ctx,
> > 		  DIRECT_8X8_INFERENCE_FLAG);
> > 
> > 	/* write pps */
> > -	WRITE_PPS(0xff, PIC_PARAMETER_SET_ID);
> > -	WRITE_PPS(0x1f, PPS_SEQ_PARAMETER_SET_ID);
> > +	WRITE_PPS(pps->pic_parameter_set_id, PIC_PARAMETER_SET_ID);
> > +	WRITE_PPS(pps->seq_parameter_set_id, PPS_SEQ_PARAMETER_SET_ID);
> > 	WRITE_PPS(!!(pps->flags & V4L2_H264_PPS_FLAG_ENTROPY_CODING_MODE),
> > 		  ENTROPY_CODING_MODE_FLAG);
> > 	WRITE_PPS(!!(pps->flags & V4L2_H264_PPS_FLAG_BOTTOM_FIELD_PIC_ORDER_IN_FRAME_PRESENT),
> > -- 
> > 2.34.1
> >
diff mbox series

Patch

diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
index 891c48bf6a51..91f65d78e453 100644
--- a/drivers/staging/media/rkvdec/rkvdec-h264.c
+++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
@@ -655,13 +655,14 @@  static void assemble_hw_pps(struct rkvdec_ctx *ctx,
 
 #define WRITE_PPS(value, field) set_ps_field(hw_ps->info, field, value)
 	/* write sps */
-	WRITE_PPS(0xf, SEQ_PARAMETER_SET_ID);
-	WRITE_PPS(0xff, PROFILE_IDC);
-	WRITE_PPS(1, CONSTRAINT_SET3_FLAG);
+	WRITE_PPS(sps->seq_parameter_set_id, SEQ_PARAMETER_SET_ID);
+	WRITE_PPS(sps->profile_idc, PROFILE_IDC);
+	WRITE_PPS((sps->constraint_set_flags & 1 << 3) ? 1 : 0, CONSTRAINT_SET3_FLAG);
 	WRITE_PPS(sps->chroma_format_idc, CHROMA_FORMAT_IDC);
 	WRITE_PPS(sps->bit_depth_luma_minus8, BIT_DEPTH_LUMA);
 	WRITE_PPS(sps->bit_depth_chroma_minus8, BIT_DEPTH_CHROMA);
-	WRITE_PPS(0, QPPRIME_Y_ZERO_TRANSFORM_BYPASS_FLAG);
+	WRITE_PPS(!!(sps->flags & V4L2_H264_SPS_FLAG_QPPRIME_Y_ZERO_TRANSFORM_BYPASS),
+		  QPPRIME_Y_ZERO_TRANSFORM_BYPASS_FLAG);
 	WRITE_PPS(sps->log2_max_frame_num_minus4, LOG2_MAX_FRAME_NUM_MINUS4);
 	WRITE_PPS(sps->max_num_ref_frames, MAX_NUM_REF_FRAMES);
 	WRITE_PPS(sps->pic_order_cnt_type, PIC_ORDER_CNT_TYPE);
@@ -679,8 +680,8 @@  static void assemble_hw_pps(struct rkvdec_ctx *ctx,
 		  DIRECT_8X8_INFERENCE_FLAG);
 
 	/* write pps */
-	WRITE_PPS(0xff, PIC_PARAMETER_SET_ID);
-	WRITE_PPS(0x1f, PPS_SEQ_PARAMETER_SET_ID);
+	WRITE_PPS(pps->pic_parameter_set_id, PIC_PARAMETER_SET_ID);
+	WRITE_PPS(pps->seq_parameter_set_id, PPS_SEQ_PARAMETER_SET_ID);
 	WRITE_PPS(!!(pps->flags & V4L2_H264_PPS_FLAG_ENTROPY_CODING_MODE),
 		  ENTROPY_CODING_MODE_FLAG);
 	WRITE_PPS(!!(pps->flags & V4L2_H264_PPS_FLAG_BOTTOM_FIELD_PIC_ORDER_IN_FRAME_PRESENT),