Message ID | 20220405204426.259074-18-nicolas.dufresne@collabora.com (mailing list archive) |
---|---|
State | Changes Requested |
Delegated to: | Hans Verkuil |
Headers |
Received: from vger.kernel.org ([23.128.96.18]) by www.linuxtv.org with esmtp (Exim 4.92) (envelope-from <linux-media-owner@vger.kernel.org>) id 1nbsz5-005D0f-LA; Tue, 05 Apr 2022 23:53:15 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1573798AbiDEWxN (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Tue, 5 Apr 2022 18:53:13 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:43736 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1444225AbiDEWVB (ORCPT <rfc822;linux-media@vger.kernel.org>); Tue, 5 Apr 2022 18:21:01 -0400 Received: from bhuna.collabora.co.uk (bhuna.collabora.co.uk [46.235.227.227]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id C998B8E1BB; Tue, 5 Apr 2022 13:45:11 -0700 (PDT) Received: from [127.0.0.1] (localhost [127.0.0.1]) (Authenticated sender: nicolas) with ESMTPSA id 47B301F448A1 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com; s=mail; t=1649191510; bh=t6w6zh2urP7UBHlWH5GWukchWArbLK9mbtqizbdraAY=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=TkK+aVQN462OFzg3kXJMyyDzO9T5YZP4amtnyD+IC682s9lJt9HfFWJIqL5686Ljz /JF9E3x+CLnwgJ8byoIc1gLKjhAYVf5LLJvnskfXyFbE0nNpttYv4gxOg1s9CsRTNs AwdTGwxXdcxGIx/YlfnIu7+3Za8p8RI0he0Yu3Pa62uJ73r6KAHWnd+2NyYoKq+xfR vULVmlkefvGmBpcWiLzWObKsI1+YnxZxTF0muf+/GqNSWumbYdHjzwdySXF38cSXGe eWSRYf7IRpZi0MWDeZfFszhSmt7gZWD2MD7j0ZuG/oZyTGQAwwt26oDn+Fis6PS2GV ylea4nyVQ09Pw== From: Nicolas Dufresne <nicolas.dufresne@collabora.com> To: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>, Mauro Carvalho Chehab <mchehab@kernel.org>, Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: kernel@collabora.com, linux-kernel@vger.kernel.org, Jonas Karlman <jonas@kwiboo.se>, Ezequiel Garcia <ezequiel@collabora.com>, Sebastian Fricke <sebastian.fricke@collabora.com>, linux-media@vger.kernel.org, linux-rockchip@lists.infradead.org, linux-staging@lists.linux.dev Subject: [PATCH v3 17/24] media: rkvdec: h264: Fix reference frame_num wrap for second field Date: Tue, 5 Apr 2022 16:44:18 -0400 Message-Id: <20220405204426.259074-18-nicolas.dufresne@collabora.com> X-Mailer: git-send-email 2.34.1 In-Reply-To: <20220405204426.259074-1-nicolas.dufresne@collabora.com> References: <20220405204426.259074-1-nicolas.dufresne@collabora.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,SPF_HELO_PASS,SPF_PASS, T_SCC_BODY_TEXT_LINE,UNPARSEABLE_RELAY autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org X-LSpam-Score: -2.5 (--) X-LSpam-Report: No, score=-2.5 required=5.0 tests=BAYES_00=-1.9,DKIM_SIGNED=0.1,DKIM_VALID=-0.1,DKIM_VALID_AU=-0.1,HEADER_FROM_DIFFERENT_DOMAINS=0.5,MAILING_LIST_MULTI=-1,UNPARSEABLE_RELAY=0.001 autolearn=ham autolearn_force=no |
Series |
[v3,01/24] media: doc: Document dual use of H.264 pic_num/frame_num
|
|
Commit Message
Nicolas Dufresne
April 5, 2022, 8:44 p.m. UTC
From: Jonas Karlman <jonas@kwiboo.se> When decoding the second field in a complementary field pair the second field is sharing the same frame_num with the first field. Currently the frame_num for the first field is wrapped when it matches the field being decoded, this cause issues to decode the second field in a complementary field pair. Fix this by using inclusive comparison, less than or equal. Signed-off-by: Jonas Karlman <jonas@kwiboo.se> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com> Reviewed-by: Sebastian Fricke <sebastian.fricke@collabora.com> --- drivers/staging/media/rkvdec/rkvdec-h264.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Comments
On 05/04/2022 22:44, Nicolas Dufresne wrote: > From: Jonas Karlman <jonas@kwiboo.se> > > When decoding the second field in a complementary field pair the second > field is sharing the same frame_num with the first field. > > Currently the frame_num for the first field is wrapped when it matches the > field being decoded, this cause issues to decode the second field in a cause issues to decode -> caused issues decoding > complementary field pair. > > Fix this by using inclusive comparison, less than or equal. I would change this last sentence to: Fix this by using inclusive comparison: 'less than or equal'. It makes it a bit easier to parse. > > Signed-off-by: Jonas Karlman <jonas@kwiboo.se> > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> > Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com> > Reviewed-by: Sebastian Fricke <sebastian.fricke@collabora.com> > --- > drivers/staging/media/rkvdec/rkvdec-h264.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c > index f081b476340f..60eaf06b6e25 100644 > --- a/drivers/staging/media/rkvdec/rkvdec-h264.c > +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c > @@ -781,7 +781,7 @@ static void assemble_hw_rps(struct rkvdec_ctx *ctx, > continue; > > if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM || > - dpb[i].frame_num < dec_params->frame_num) { > + dpb[i].frame_num <= dec_params->frame_num) { I wonder if a comment should be added here, explaining the reason for '<='. It doesn't seem obvious to me. Up to you, though. > p[i] = dpb[i].frame_num; > continue; > } Regards, Hans
Le vendredi 22 avril 2022 à 09:43 +0200, Hans Verkuil a écrit : > On 05/04/2022 22:44, Nicolas Dufresne wrote: > > From: Jonas Karlman <jonas@kwiboo.se> > > > > When decoding the second field in a complementary field pair the second > > field is sharing the same frame_num with the first field. > > > > Currently the frame_num for the first field is wrapped when it matches the > > field being decoded, this cause issues to decode the second field in a > > cause issues to decode -> caused issues decoding > > > complementary field pair. > > > > Fix this by using inclusive comparison, less than or equal. > > I would change this last sentence to: > > Fix this by using inclusive comparison: 'less than or equal'. > > It makes it a bit easier to parse. > > > > > Signed-off-by: Jonas Karlman <jonas@kwiboo.se> > > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> > > Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com> > > Reviewed-by: Sebastian Fricke <sebastian.fricke@collabora.com> > > --- > > drivers/staging/media/rkvdec/rkvdec-h264.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c > > index f081b476340f..60eaf06b6e25 100644 > > --- a/drivers/staging/media/rkvdec/rkvdec-h264.c > > +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c > > @@ -781,7 +781,7 @@ static void assemble_hw_rps(struct rkvdec_ctx *ctx, > > continue; > > > > if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM || > > - dpb[i].frame_num < dec_params->frame_num) { > > + dpb[i].frame_num <= dec_params->frame_num) { > > I wonder if a comment should be added here, explaining the reason for '<='. > > It doesn't seem obvious to me. Up to you, though. I guess I could, the algo for wrapping in the spec is (formula 8-27): if( FrameNum > frame_num ) FrameNumWrap = FrameNum − MaxFrameNum else FrameNumWrap = FrameNum Our implementation has the branch condition flip over, and the flipped version of that is: if( FrameNum <= frame_num ) FrameNumWrap = FrameNum else FrameNumWrap = FrameNum − MaxFrameNum There is no deeper rationale since we simply follow the recipe described in the spec. This is done so that we can share that condition with that long term reference handling. > > > p[i] = dpb[i].frame_num; > > continue; > > } > > Regards, > > Hans
Le lundi 25 avril 2022 à 14:55 -0400, Nicolas Dufresne a écrit : > Le vendredi 22 avril 2022 à 09:43 +0200, Hans Verkuil a écrit : > > On 05/04/2022 22:44, Nicolas Dufresne wrote: > > > From: Jonas Karlman <jonas@kwiboo.se> > > > > > > When decoding the second field in a complementary field pair the second > > > field is sharing the same frame_num with the first field. > > > > > > Currently the frame_num for the first field is wrapped when it matches the > > > field being decoded, this cause issues to decode the second field in a > > > > cause issues to decode -> caused issues decoding > > > > > complementary field pair. > > > > > > Fix this by using inclusive comparison, less than or equal. > > > > I would change this last sentence to: > > > > Fix this by using inclusive comparison: 'less than or equal'. > > > > It makes it a bit easier to parse. > > > > > > > > Signed-off-by: Jonas Karlman <jonas@kwiboo.se> > > > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> > > > Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com> > > > Reviewed-by: Sebastian Fricke <sebastian.fricke@collabora.com> > > > --- > > > drivers/staging/media/rkvdec/rkvdec-h264.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c > > > index f081b476340f..60eaf06b6e25 100644 > > > --- a/drivers/staging/media/rkvdec/rkvdec-h264.c > > > +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c > > > @@ -781,7 +781,7 @@ static void assemble_hw_rps(struct rkvdec_ctx *ctx, > > > continue; > > > > > > if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM || > > > - dpb[i].frame_num < dec_params->frame_num) { > > > + dpb[i].frame_num <= dec_params->frame_num) { > > > > I wonder if a comment should be added here, explaining the reason for '<='. > > > > It doesn't seem obvious to me. Up to you, though. > > I guess I could, the algo for wrapping in the spec is (formula 8-27): > > if( FrameNum > frame_num ) > FrameNumWrap = FrameNum − MaxFrameNum > else > FrameNumWrap = FrameNum > > Our implementation has the branch condition flip over, and the flipped version of that is: > > if( FrameNum <= frame_num ) > FrameNumWrap = FrameNum > else > FrameNumWrap = FrameNum − MaxFrameNum > > There is no deeper rationale since we simply follow the recipe described in the > spec. This is done so that we can share that condition with that long term > reference handling. Now I come to realize that in patch "[v3,19/24] media: rkvdec-h264: Add field decoding support" all this code is removed. This is because the wrapping is already done by the ref-builder, so while enabling field decoding, I now use the wrapped value from the ref builder. I'm mostly keeping the patch so that this fix is well documented. I will leave it like this then. > > > > > > p[i] = dpb[i].frame_num; > > > continue; > > > } > > > > Regards, > > > > Hans >
diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c index f081b476340f..60eaf06b6e25 100644 --- a/drivers/staging/media/rkvdec/rkvdec-h264.c +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c @@ -781,7 +781,7 @@ static void assemble_hw_rps(struct rkvdec_ctx *ctx, continue; if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM || - dpb[i].frame_num < dec_params->frame_num) { + dpb[i].frame_num <= dec_params->frame_num) { p[i] = dpb[i].frame_num; continue; }