Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Wed, 30 Mar 2022 09:42:50 +0200
From:   Sebastian Fricke <sebastian.fricke@collabora.com>
To:     Nicolas Dufresne <nicolas.dufresne@collabora.com>
Cc:     Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>,
        Philipp Zabel <p.zabel@pengutronix.de>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        kernel@collabora.com, linux-media@vger.kernel.org,
        linux-rockchip@lists.infradead.org, linux-staging@lists.linux.dev,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v1 21/24] media: hantro: Stop using H.264 parameter
 pic_num
Message-ID: <20220330074250.jqyljbr53fgeci6q@basti-XPS-13-9310>
References: <20220328195936.82552-1-nicolas.dufresne@collabora.com>
 <20220328195936.82552-22-nicolas.dufresne@collabora.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20220328195936.82552-22-nicolas.dufresne@collabora.com>
Precedence: bulk

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

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

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

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.

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
>