| Message ID | 20240218115954.4038-1-irui.wang@mediatek.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | media: mediatek: vcodec: Handle VP9 superframe bitstream with 8 sub-frames | expand |
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; > }
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 --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; }
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(-)