diff mbox series

media: mediatek: vcodec: Handle VP9 superframe bitstream with 8 sub-frames

Message ID 20240218115954.4038-1-irui.wang@mediatek.com (mailing list archive)
State Superseded
Delegated to: Sebastian Fricke
Headers
Series media: mediatek: vcodec: Handle VP9 superframe bitstream with 8 sub-frames |

Commit Message

Irui Wang Feb. 18, 2024, 11:59 a.m. UTC 
  The VP9 bitstream has 8 sub-frames into one superframe, the superframe
index validate failed when reach 8, modify the index checking so that the
last sub-frame can be decoded normally.

Signed-off-by: Irui Wang <irui.wang@mediatek.com>
---
 .../media/platform/mediatek/vcodec/decoder/vdec/vdec_vp9_if.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Nicolas Dufresne Feb. 19, 2024, 9:09 p.m. UTC | #1 
Hi,

Le dimanche 18 février 2024 à 19:59 +0800, Irui Wang a écrit :
> The VP9 bitstream has 8 sub-frames into one superframe, the superframe
> index validate failed when reach 8, modify the index checking so that the
> last sub-frame can be decoded normally.

When I first saw this patch I was concerned, since we don't allow bundling super
frame into the stateless API, but now I realize that this is for the stateful
decoder. Perhaps you can help me and possibly other reviewers with simply
stating that this is for stateful decoders.

> 
> Signed-off-by: Irui Wang <irui.wang@mediatek.com>
> ---
>  .../media/platform/mediatek/vcodec/decoder/vdec/vdec_vp9_if.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_vp9_if.c b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_vp9_if.c
> index 55355fa70090..4a9ced7348ee 100644
> --- a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_vp9_if.c
> +++ b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_vp9_if.c
> @@ -526,7 +526,7 @@ static void vp9_swap_frm_bufs(struct vdec_vp9_inst *inst)
>  	/* if this super frame and it is not last sub-frame, get next fb for
>  	 * sub-frame decode
>  	 */
> -	if (vsi->sf_frm_cnt > 0 && vsi->sf_frm_idx != vsi->sf_frm_cnt - 1)
> +	if (vsi->sf_frm_cnt > 0 && vsi->sf_frm_idx != vsi->sf_frm_cnt)
>  		vsi->sf_next_ref_fb_idx = vp9_get_sf_ref_fb(inst);
>  }
>  
> @@ -735,7 +735,7 @@ static void get_free_fb(struct vdec_vp9_inst *inst, struct vdec_fb **out_fb)
>  
>  static int validate_vsi_array_indexes(struct vdec_vp9_inst *inst,
>  		struct vdec_vp9_vsi *vsi) {
> -	if (vsi->sf_frm_idx >= VP9_MAX_FRM_BUF_NUM - 1) {
> +	if (vsi->sf_frm_idx >= VP9_MAX_FRM_BUF_NUM) {

nit: I'd propose to define a new maximum (contractions allowed):

  #define VP9_MAX_NUM_SUPER_FRAMES 8

This way you can revisit bunch of `VP9_MAX_FRM_BUF_NUM-1`, and make the overall
code a bit more human readable. There is no relation between VP9_MAX_FRM_BUF_NUM
and this maximum. The limits simply comes from the fact
frames_in_superframe_minus_1 is expressed with 3 bits.

regards,
Nicolas

p.s. your change looks good otherwise.

>  		mtk_vdec_err(inst->ctx, "Invalid vsi->sf_frm_idx=%u.", vsi->sf_frm_idx);
>  		return -EIO;
>  	}
Irui Wang Feb. 22, 2024, 2:49 a.m. UTC | #2 
Dear Nicolas,

Thanks for your reviewing.
Yes, this patch is just for the VP9 stateful decoder process, so the
super frame is handled in stateful driver.

On Mon, 2024-02-19 at 16:09 -0500, Nicolas Dufresne wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  Hi,
> 
> Le dimanche 18 février 2024 à 19:59 +0800, Irui Wang a écrit :
> > The VP9 bitstream has 8 sub-frames into one superframe, the
> superframe
> > index validate failed when reach 8, modify the index checking so
> that the
> > last sub-frame can be decoded normally.
> 
> When I first saw this patch I was concerned, since we don't allow
> bundling super
> frame into the stateless API, but now I realize that this is for the
> stateful
> decoder. Perhaps you can help me and possibly other reviewers with
> simply
> stating that this is for stateful decoders.
> 
> > 
> > Signed-off-by: Irui Wang <irui.wang@mediatek.com>
> > ---
> >  .../media/platform/mediatek/vcodec/decoder/vdec/vdec_vp9_if.c | 4
> ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git
> a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_vp9_if.c
> b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_vp9_if.c
> > index 55355fa70090..4a9ced7348ee 100644
> > ---
> a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_vp9_if.c
> > +++
> b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_vp9_if.c
> > @@ -526,7 +526,7 @@ static void vp9_swap_frm_bufs(struct
> vdec_vp9_inst *inst)
> >  /* if this super frame and it is not last sub-frame, get next fb
> for
> >   * sub-frame decode
> >   */
> > -if (vsi->sf_frm_cnt > 0 && vsi->sf_frm_idx != vsi->sf_frm_cnt - 1)
> > +if (vsi->sf_frm_cnt > 0 && vsi->sf_frm_idx != vsi->sf_frm_cnt)
> >  vsi->sf_next_ref_fb_idx = vp9_get_sf_ref_fb(inst);
> >  }
> >  
> > @@ -735,7 +735,7 @@ static void get_free_fb(struct vdec_vp9_inst
> *inst, struct vdec_fb **out_fb)
> >  
> >  static int validate_vsi_array_indexes(struct vdec_vp9_inst *inst,
> >  struct vdec_vp9_vsi *vsi) {
> > -if (vsi->sf_frm_idx >= VP9_MAX_FRM_BUF_NUM - 1) {
> > +if (vsi->sf_frm_idx >= VP9_MAX_FRM_BUF_NUM) {
> 
> nit: I'd propose to define a new maximum (contractions allowed):
> 
>   #define VP9_MAX_NUM_SUPER_FRAMES 8
> 
> This way you can revisit bunch of `VP9_MAX_FRM_BUF_NUM-1`, and make
> the overall
> code a bit more human readable. There is no relation between
> VP9_MAX_FRM_BUF_NUM
> and this maximum. The limits simply comes from the fact
> frames_in_superframe_minus_1 is expressed with 3 bits.
> 
> regards,
> Nicolas
> 
Yes, define a new maximum makes code more readable, we will check it.

Thanks
Best Regards

> p.s. your change looks good otherwise.
> 
> >  mtk_vdec_err(inst->ctx, "Invalid vsi->sf_frm_idx=%u.", vsi-
> >sf_frm_idx);
> >  return -EIO;
> >  }
>
diff mbox series

Patch

diff --git a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_vp9_if.c b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_vp9_if.c
index 55355fa70090..4a9ced7348ee 100644
--- a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_vp9_if.c
+++ b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_vp9_if.c
@@ -526,7 +526,7 @@  static void vp9_swap_frm_bufs(struct vdec_vp9_inst *inst)
 	/* if this super frame and it is not last sub-frame, get next fb for
 	 * sub-frame decode
 	 */
-	if (vsi->sf_frm_cnt > 0 && vsi->sf_frm_idx != vsi->sf_frm_cnt - 1)
+	if (vsi->sf_frm_cnt > 0 && vsi->sf_frm_idx != vsi->sf_frm_cnt)
 		vsi->sf_next_ref_fb_idx = vp9_get_sf_ref_fb(inst);
 }
 
@@ -735,7 +735,7 @@  static void get_free_fb(struct vdec_vp9_inst *inst, struct vdec_fb **out_fb)
 
 static int validate_vsi_array_indexes(struct vdec_vp9_inst *inst,
 		struct vdec_vp9_vsi *vsi) {
-	if (vsi->sf_frm_idx >= VP9_MAX_FRM_BUF_NUM - 1) {
+	if (vsi->sf_frm_idx >= VP9_MAX_FRM_BUF_NUM) {
 		mtk_vdec_err(inst->ctx, "Invalid vsi->sf_frm_idx=%u.", vsi->sf_frm_idx);
 		return -EIO;
 	}