Re: [PATCH v1 21/24] media: hantro: Stop using H.264 parameter pic_num

[Nicolas Dufresne <nicolas.dufresne@collabora.com>]

Le mercredi 30 mars 2022 à 09:42 +0200, Sebastian Fricke a écrit :
> Hey Nicolas,
> 
> The term pic_num is now only present in the following files:
> ```
> ❯ rg 'pic_num'
> staging/media/rkvdec/rkvdec-h264.c
> 766:	 * Assign an invalid pic_num if DPB entry at that position is inactive.
> 768:	 * reference picture with pic_num 0, triggering output picture

I should probably translate this one, since the HW uses frame_num, not pic_num.

> 
> media/platform/amphion/vpu_windsor.c
> 485:	u32 pic_num;

Amphion Windsor is a stateful driver, I cannot comment on the user of pic_num
for that type of driver.

> 
> media/platform/mediatek/vcodec/vdec/vdec_h264_req_if.c
> 97:	unsigned short pic_num;
> 346:		dst_entry->pic_num = src_entry->pic_num;

This is being sent to the firmware, so its a difficult change to make without
testing it first. I do have HW to test this, but would prefer doing so in a
seperate patchset. Note that MTK does not support field decoding, so pic_num ==
frame_num. So whatever it does here is likely correct.

> 
> media/v4l2-core/v4l2-h264.c
> 143:	 * but with frame_num (wrapped). As for frame the pic_num and frame_num
> 306:		/* this is pic_num for frame and frame_num (wrapped) for field,
> 307:		 * but for frame pic_num is equal to frame_num (wrapped).
> ```
> 
> In v4l2-h264 and rkvdec-h264 it is only present as comment and the term
> is not part of the specification.
> In vpu_windsor it is actually never used.
> And for the mediatek driver the same might apply.
> It might be worth it to get rid of that term all together while you are
> at it.

Amphion Windsor is a stateful driver, I'd leave it to the maintainer to cleanup
unused variables if there is. In general the term is not invalid, its just that
the value can be trivially deduced from frame_num and the value depends on the
current picture parity, which makes it an unstable identifier.

> 
> On 28.03.2022 15:59, Nicolas Dufresne wrote:
> > The hardware expects FrameNumWrap or long_term_frame_idx. Picture
> > numbers are per field, and are mostly used during the memory
> > management process, which is done in userland. This fixes two
> > ITU conformance tests:
> > 
> >  - MR6_BT_B
> >  - MR8_BT_B
> > 
> > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> Reviewed-by: Sebastian Fricke <sebastian.fricke@collabora.com>
> 
> Greetings,
> Sebastian
> > ---
> > drivers/staging/media/hantro/hantro_h264.c | 2 --
> > 1 file changed, 2 deletions(-)
> > 
> > diff --git a/drivers/staging/media/hantro/hantro_h264.c b/drivers/staging/media/hantro/hantro_h264.c
> > index 0b4d2491be3b..228629fb3cdf 100644
> > --- a/drivers/staging/media/hantro/hantro_h264.c
> > +++ b/drivers/staging/media/hantro/hantro_h264.c
> > @@ -354,8 +354,6 @@ u16 hantro_h264_get_ref_nbr(struct hantro_ctx *ctx, unsigned int dpb_idx)
> > 
> > 	if (!(dpb->flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE))
> > 		return 0;
> > -	if (dpb->flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM)
> > -		return dpb->pic_num;
> > 	return dpb->frame_num;
> > }
> > 
> > -- 
> > 2.34.1
> >