linux-kernel - Re: [PATCH v1 07/24] media: h264: Sort p/b reflist using frame

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220329133439.qt6bze35bppx3mne@basti-XPS-13-9310>
Date:   Tue, 29 Mar 2022 15:34:39 +0200
From:   Sebastian Fricke <sebastian.fricke@...labora.com>
To:     Nicolas Dufresne <nicolas.dufresne@...labora.com>
Cc:     Mauro Carvalho Chehab <mchehab@...nel.org>, kernel@...labora.com,
        linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 07/24] media: h264: Sort p/b reflist using frame_num

Hey Nicolas,

On 28.03.2022 15:59, Nicolas Dufresne wrote:
>In the reference list builder, frame_num referers to FrameNumWrap

s/referers/refers/

>in the spec, which is the same as the pic_num for frame decoding.
>The same applied for long_tern_pic_num and long_ter_frame_idx.

s/same applied/same applies/
s/long_ter_frame_idx/long_term_frame_idx/

>
>Sort all type of references by frame_num so the sort can be reused

s/frame_num so the sort/frame_num, so the order/

>for field reflist were the sorting is done using frame_num instead.

s/field/the field/
s/reflist were the sorting is done using frame_num instead/
   reflist, which is sorted by the frame_num instead/

>In short, pic_num is never actually used for building reference
>lists.
>
>Signed-off-by: Nicolas Dufresne <nicolas.dufresne@...labora.com>
Reviewed-by: Sebastian Fricke <sebastian.fricke@...labora.com>

>---
> drivers/media/v4l2-core/v4l2-h264.c | 22 ++++++++++++----------
> include/media/v4l2-h264.h           |  2 --
> 2 files changed, 12 insertions(+), 12 deletions(-)
>
>diff --git a/drivers/media/v4l2-core/v4l2-h264.c b/drivers/media/v4l2-core/v4l2-h264.c
>index c9e18fd51d78..c3fad382882d 100644
>--- a/drivers/media/v4l2-core/v4l2-h264.c
>+++ b/drivers/media/v4l2-core/v4l2-h264.c
>@@ -50,7 +50,6 @@ v4l2_h264_init_reflist_builder(struct v4l2_h264_reflist_builder *b,
> 		if (!(dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE))
> 			continue;
>
>-		b->refs[i].pic_num = dpb[i].pic_num;
> 		if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM)
> 			b->refs[i].longterm = true;
>
>@@ -139,15 +138,18 @@ static int v4l2_h264_p_ref_list_cmp(const void *ptra, const void *ptrb,
> 	}
>
> 	/*
>-	 * Short term pics in descending pic num order, long term ones in
>-	 * ascending order.
>+	 * For frames, short term pics are in descending pic num order and long
>+	 * term ones in ascending order. For field this is the same direction

s/For field this is the same direction/For fields the same direction is used/

>+	 * but with frame_num (wrapped). As for frame the pic_num and frame_num
>+	 * are the same (see formula (8-28) and (8-29)) use the frame_num and share

s/As for frame the pic_num and frame_num are the same/
   The fields pic_num and frame_num are the same for frames/

>+	 * this funciton between frame and field reflist.

s/funciton/function/

Greetings,
Sebastian

> 	 */
> 	if (!builder->refs[idxa].longterm)
> 		return builder->refs[idxb].frame_num <
> 		       builder->refs[idxa].frame_num ?
> 		       -1 : 1;
>
>-	return builder->refs[idxa].pic_num < builder->refs[idxb].pic_num ?
>+	return builder->refs[idxa].frame_num < builder->refs[idxb].frame_num ?
> 	       -1 : 1;
> }
>
>@@ -173,10 +175,10 @@ static int v4l2_h264_b0_ref_list_cmp(const void *ptra, const void *ptrb,
> 			return 1;
> 	}
>
>-	/* Long term pics in ascending pic num order. */
>+	/* Long term pics in ascending frame num order. */
> 	if (builder->refs[idxa].longterm)
>-		return builder->refs[idxa].pic_num <
>-		       builder->refs[idxb].pic_num ?
>+		return builder->refs[idxa].frame_num <
>+		       builder->refs[idxb].frame_num ?
> 		       -1 : 1;
>
> 	poca = v4l2_h264_get_poc(builder, ptra);
>@@ -218,10 +220,10 @@ static int v4l2_h264_b1_ref_list_cmp(const void *ptra, const void *ptrb,
> 			return 1;
> 	}
>
>-	/* Long term pics in ascending pic num order. */
>+	/* Long term pics in ascending frame num order. */
> 	if (builder->refs[idxa].longterm)
>-		return builder->refs[idxa].pic_num <
>-		       builder->refs[idxb].pic_num ?
>+		return builder->refs[idxa].frame_num <
>+		       builder->refs[idxb].frame_num ?
> 		       -1 : 1;
>
> 	poca = v4l2_h264_get_poc(builder, ptra);
>diff --git a/include/media/v4l2-h264.h b/include/media/v4l2-h264.h
>index 4cef717b3f18..0d9eaa956123 100644
>--- a/include/media/v4l2-h264.h
>+++ b/include/media/v4l2-h264.h
>@@ -18,7 +18,6 @@
>  * @refs.top_field_order_cnt: top field order count
>  * @refs.bottom_field_order_cnt: bottom field order count
>  * @refs.frame_num: reference frame number
>- * @refs.pic_num: reference picture number
>  * @refs.longterm: set to true for a long term reference
>  * @refs: array of references
>  * @cur_pic_order_count: picture order count of the frame being decoded
>@@ -36,7 +35,6 @@ struct v4l2_h264_reflist_builder {
> 		s32 top_field_order_cnt;
> 		s32 bottom_field_order_cnt;
> 		int frame_num;
>-		u32 pic_num;
> 		u16 longterm : 1;
> 	} refs[V4L2_H264_NUM_DPB_ENTRIES];
>
>-- 
>2.34.1
>