[{"id":17351,"web_url":"https://patchwork.libcamera.org/comment/17351/","msgid":"","date":"2021-05-31T04:50:12","subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: pipeline: ipu3: Fix\n\tincorrect bdsHeight calculation","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent, thank you for the patch.\n\nOn Mon, May 31, 2021 at 11:08 AM Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> When compiling with optimization, gcc 9 and newer throw an unitialized\n> variable warning:\n>\n> ../../src/libcamera/pipeline/ipu3/imgu.cpp: In function ‘void\n> libcamera::{anonymous}::calculateBDSHeight(libcamera::ImgUDevice::Pipe*,\n> const libcamera::Size&, const libcamera::Size&, unsigned int, float)’:\n> ../../src/libcamera/pipeline/ipu3/imgu.cpp:172:17: error: ‘bdsHeight’ may\n> be used uninitialized in this function [-Werror=maybe-uninitialized]\n> 172 | unsigned int bdsIntHeight = static_cast int>(bdsHeight);\n>\n> Neither clang not gcc versions older than 9 complain. This seems to be\n> a false positive.\n>\n> However, there's an obvious error in the code. The second while () loop\n> in the first part of calculateBDSHeight() modifies the bdsHeight\n> variable set by the first loop even if the second loop doesn't find a\n> suitable height. This can result in an incorrect bdsHeight value. Fix\n> this, which also gets rid of the compiler warning.\n>\n> Signed-off-by: Laurent Pinchart \n>\n\nThe fix looks correct to me.\nReviewed-by: Hirokazu Honda \n\n\n> ---\n> src/libcamera/pipeline/ipu3/imgu.cpp | 14 ++++++++------\n> 1 file changed, 8 insertions(+), 6 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp\n> b/src/libcamera/pipeline/ipu3/imgu.cpp\n> index 3e517ac67962..4eb3f7b730a9 100644\n> --- a/src/libcamera/pipeline/ipu3/imgu.cpp\n> +++ b/src/libcamera/pipeline/ipu3/imgu.cpp\n> @@ -138,12 +138,13 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe,\n> const Size &iif, const Size &gdc\n> while (ifHeight >= minIFHeight && ifHeight <= iif.height &&\n> ifHeight / bdsSF >= minBDSHeight) {\n>\n> - bdsHeight = ifHeight / bdsSF;\n> - if (std::fmod(bdsHeight, 1.0) == 0) {\n> - unsigned int bdsIntHeight =\n> static_cast(bdsHeight);\n> + float height = ifHeight / bdsSF;\n> + if (std::fmod(height, 1.0) == 0) {\n> + unsigned int bdsIntHeight =\n> static_cast(height);\n>\n> if (!(bdsIntHeight % BDS_ALIGN_H)) {\n> foundIfHeight = ifHeight;\n> + bdsHeight = height;\n> break;\n> }\n> }\n> @@ -155,12 +156,13 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe,\n> const Size &iif, const Size &gdc\n> while (ifHeight >= minIFHeight && ifHeight <= iif.height &&\n> ifHeight / bdsSF >= minBDSHeight) {\n>\n> - bdsHeight = ifHeight / bdsSF;\n> - if (std::fmod(bdsHeight, 1.0) == 0) {\n> - unsigned int bdsIntHeight =\n> static_cast(bdsHeight);\n> + float height = ifHeight / bdsSF;\n> + if (std::fmod(height, 1.0) == 0) {\n> + unsigned int bdsIntHeight =\n> static_cast(height);\n>\n> if (!(bdsIntHeight % BDS_ALIGN_H)) {\n> foundIfHeight = ifHeight;\n> + bdsHeight = height;\n> break;\n> }\n> }\n> --\n> Regards,\n>\n> Laurent Pinchart\n>\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 04889C3206\n\tfor ;\n\tMon, 31 May 2021 04:50:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4CD4268922;\n\tMon, 31 May 2021 06:50:24 +0200 (CEST)","from mail-ed1-x529.google.com (mail-ed1-x529.google.com\n\t[IPv6:2a00:1450:4864:20::529])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E07A9602A5\n\tfor ;\n\tMon, 31 May 2021 06:50:22 +0200 (CEST)","by mail-ed1-x529.google.com with SMTP id u24so2450532edy.11\n\tfor ;\n\tSun, 30 May 2021 21:50:22 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"Buf6jGat\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=w5yn2yZwlFaKvM1qYqOMD3aZPYtnE4ztzWVXZ19U24Q=;\n\tb=Buf6jGat7suSKqWYEHFZREzap+wziHgux6oPcDDYjblMNhqo5BVy0gWKLbyNxxmm5x\n\tvOPG2gzc7/sHNign1qfSq0mbLlJzvd2ESJZS8nb+8Q0JaGR/NNX18tgx91Fu2oS3m3pK\n\tN+i52z/ZqC2n816MJw0CcDCRoa3hLpE53LO5w=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=w5yn2yZwlFaKvM1qYqOMD3aZPYtnE4ztzWVXZ19U24Q=;\n\tb=nSf4dW+DbmKkET5ZDCo7JPWQfvuN8nGetwxRpBLDqZTB3pL5jzI98CX+ojZvpRKtBv\n\tqITmQK/B41WDhQl7/ZE1hcQKfOhcpTR0LaS2aFUEZADt1kOOljzAOUEBVjE0kHKdeYfO\n\tpKOtPopk3gHDopmOLKSll9RbSpRXw316GkcR/QLRB8MJZOi29l9Hm9nrWh5y/zGxeheB\n\tDDfeetCMOgMgoMBiTeov1CirfnpNFIs4KQfil9bB1zlNpKBMcpCH9mHBV404OoBHvhWr\n\tLrl078xbWb0zdSEZg8xK2EoyeZLDTtsVfMr9hRnYfl7XEVsEIrHOHZ7s8CHN3kknZyD4\n\thUlg==","X-Gm-Message-State":"AOAM532A+MCb/lhz90rDy4jsEY+eVFLdJ1/EiQnbcY/r6cWYqkeoeIPg\n\tsLImc9MOr2uUMJUgCQRSRnW/Q7f2Ih8Pt/tKuW4jyoPtuEU=","X-Google-Smtp-Source":"ABdhPJyY6B/7BIbnS2DQNc5eOw7dM1VzxM4yvxejVIIGHgneONVKCR99YcaJ613kZzKiTFBf960GE4U77tVZ0RX6sT0=","X-Received":"by 2002:a05:6402:2547:: with SMTP id\n\tl7mr23687647edb.73.1622436622468; \n\tSun, 30 May 2021 21:50:22 -0700 (PDT)","MIME-Version":"1.0","References":"<20210531020745.13815-1-laurent.pinchart@ideasonboard.com>\n\t<20210531020745.13815-3-laurent.pinchart@ideasonboard.com>","In-Reply-To":"<20210531020745.13815-3-laurent.pinchart@ideasonboard.com>","From":"Hirokazu Honda ","Date":"Mon, 31 May 2021 13:50:12 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"0000000000008d9c9b05c398f65c\"","Subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: pipeline: ipu3: Fix\n\tincorrect bdsHeight calculation","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17352,"web_url":"https://patchwork.libcamera.org/comment/17352/","msgid":"","date":"2021-05-31T08:37:07","subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: pipeline: ipu3: Fix\n\tincorrect bdsHeight calculation","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nOn Mon, May 31, 2021 at 01:50:12PM +0900, Hirokazu Honda wrote:\n> On Mon, May 31, 2021 at 11:08 AM Laurent Pinchart wrote:\n> \n> > When compiling with optimization, gcc 9 and newer throw an unitialized\n> > variable warning:\n> >\n> > ../../src/libcamera/pipeline/ipu3/imgu.cpp: In function ‘void\n> > libcamera::{anonymous}::calculateBDSHeight(libcamera::ImgUDevice::Pipe*,\n> > const libcamera::Size&, const libcamera::Size&, unsigned int, float)’:\n> > ../../src/libcamera/pipeline/ipu3/imgu.cpp:172:17: error: ‘bdsHeight’ may\n> > be used uninitialized in this function [-Werror=maybe-uninitialized]\n> > 172 | unsigned int bdsIntHeight = static_cast > int>(bdsHeight);\n> >\n> > Neither clang not gcc versions older than 9 complain. This seems to be\n> > a false positive.\n> >\n> > However, there's an obvious error in the code. The second while () loop\n> > in the first part of calculateBDSHeight() modifies the bdsHeight\n> > variable set by the first loop even if the second loop doesn't find a\n> > suitable height. This can result in an incorrect bdsHeight value. Fix\n> > this, which also gets rid of the compiler warning.\n> >\n> > Signed-off-by: Laurent Pinchart \n> \n> The fix looks correct to me.\n> Reviewed-by: Hirokazu Honda \n\nAs mentioned in the cover letter, which of 1/2 and 2/2 do you think we\nshould merge ?\n\n> > ---\n> > src/libcamera/pipeline/ipu3/imgu.cpp | 14 ++++++++------\n> > 1 file changed, 8 insertions(+), 6 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp\n> > b/src/libcamera/pipeline/ipu3/imgu.cpp\n> > index 3e517ac67962..4eb3f7b730a9 100644\n> > --- a/src/libcamera/pipeline/ipu3/imgu.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp\n> > @@ -138,12 +138,13 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc\n> > while (ifHeight >= minIFHeight && ifHeight <= iif.height &&\n> > ifHeight / bdsSF >= minBDSHeight) {\n> >\n> > - bdsHeight = ifHeight / bdsSF;\n> > - if (std::fmod(bdsHeight, 1.0) == 0) {\n> > - unsigned int bdsIntHeight = static_cast(bdsHeight);\n> > + float height = ifHeight / bdsSF;\n> > + if (std::fmod(height, 1.0) == 0) {\n> > + unsigned int bdsIntHeight = static_cast(height);\n> >\n> > if (!(bdsIntHeight % BDS_ALIGN_H)) {\n> > foundIfHeight = ifHeight;\n> > + bdsHeight = height;\n> > break;\n> > }\n> > }\n> > @@ -155,12 +156,13 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc\n> > while (ifHeight >= minIFHeight && ifHeight <= iif.height &&\n> > ifHeight / bdsSF >= minBDSHeight) {\n> >\n> > - bdsHeight = ifHeight / bdsSF;\n> > - if (std::fmod(bdsHeight, 1.0) == 0) {\n> > - unsigned int bdsIntHeight = static_cast(bdsHeight);\n> > + float height = ifHeight / bdsSF;\n> > + if (std::fmod(height, 1.0) == 0) {\n> > + unsigned int bdsIntHeight = static_cast(height);\n> >\n> > if (!(bdsIntHeight % BDS_ALIGN_H)) {\n> > foundIfHeight = ifHeight;\n> > + bdsHeight = height;\n> > break;\n> > }\n> > }","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id A766AC3205\n\tfor ;\n\tMon, 31 May 2021 08:37:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1BD3868923;\n\tMon, 31 May 2021 10:37:20 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 182166891F\n\tfor ;\n\tMon, 31 May 2021 10:37:18 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7F4DF1178;\n\tMon, 31 May 2021 10:37:17 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Pk5A2pQO\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1622450237;\n\tbh=uojOHjoryIFyys9QtD+S+/OmhfPan/HZkZUfnisdKyw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Pk5A2pQOtnPB1KMJc512klQmTZNpd5kZFogEro6KLn/AlYbzhsy3VXqMTYbP+s6/g\n\tco99Lo8AlLusLfENRmjCnlQsGo4fmAvpj1MoDTs82JYAXuSG0TFWLGZceOjOEGjkWp\n\tIxnlLkrf9XQYbFW0jULd8kwZ0l3H35QTCXzU0ugE=","Date":"Mon, 31 May 2021 11:37:07 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210531020745.13815-1-laurent.pinchart@ideasonboard.com>\n\t<20210531020745.13815-3-laurent.pinchart@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: pipeline: ipu3: Fix\n\tincorrect bdsHeight calculation","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17353,"web_url":"https://patchwork.libcamera.org/comment/17353/","msgid":"","date":"2021-05-31T08:39:07","subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: pipeline: ipu3: Fix\n\tincorrect bdsHeight calculation","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent,\n\nOn Mon, May 31, 2021 at 5:37 PM Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Hiro,\n>\n> On Mon, May 31, 2021 at 01:50:12PM +0900, Hirokazu Honda wrote:\n> > On Mon, May 31, 2021 at 11:08 AM Laurent Pinchart wrote:\n> >\n> > > When compiling with optimization, gcc 9 and newer throw an unitialized\n> > > variable warning:\n> > >\n> > > ../../src/libcamera/pipeline/ipu3/imgu.cpp: In function ‘void\n> > >\n> libcamera::{anonymous}::calculateBDSHeight(libcamera::ImgUDevice::Pipe*,\n> > > const libcamera::Size&, const libcamera::Size&, unsigned int, float)’:\n> > > ../../src/libcamera/pipeline/ipu3/imgu.cpp:172:17: error: ‘bdsHeight’\n> may\n> > > be used uninitialized in this function [-Werror=maybe-uninitialized]\n> > > 172 | unsigned int bdsIntHeight = static_cast > > int>(bdsHeight);\n> > >\n> > > Neither clang not gcc versions older than 9 complain. This seems to be\n> > > a false positive.\n> > >\n> > > However, there's an obvious error in the code. The second while () loop\n> > > in the first part of calculateBDSHeight() modifies the bdsHeight\n> > > variable set by the first loop even if the second loop doesn't find a\n> > > suitable height. This can result in an incorrect bdsHeight value. Fix\n> > > this, which also gets rid of the compiler warning.\n> > >\n> > > Signed-off-by: Laurent Pinchart \n> >\n> > The fix looks correct to me.\n> > Reviewed-by: Hirokazu Honda \n>\n> As mentioned in the cover letter, which of 1/2 and 2/2 do you think we\n> should merge ?\n>\n\nI prefer 2/2.\n\n-Hiro\n\n>\n> > > ---\n> > > src/libcamera/pipeline/ipu3/imgu.cpp | 14 ++++++++------\n> > > 1 file changed, 8 insertions(+), 6 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp\n> > > b/src/libcamera/pipeline/ipu3/imgu.cpp\n> > > index 3e517ac67962..4eb3f7b730a9 100644\n> > > --- a/src/libcamera/pipeline/ipu3/imgu.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp\n> > > @@ -138,12 +138,13 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe,\n> const Size &iif, const Size &gdc\n> > > while (ifHeight >= minIFHeight && ifHeight <=\n> iif.height &&\n> > > ifHeight / bdsSF >= minBDSHeight) {\n> > >\n> > > - bdsHeight = ifHeight / bdsSF;\n> > > - if (std::fmod(bdsHeight, 1.0) == 0) {\n> > > - unsigned int bdsIntHeight =\n> static_cast(bdsHeight);\n> > > + float height = ifHeight / bdsSF;\n> > > + if (std::fmod(height, 1.0) == 0) {\n> > > + unsigned int bdsIntHeight =\n> static_cast(height);\n> > >\n> > > if (!(bdsIntHeight % BDS_ALIGN_H)) {\n> > > foundIfHeight = ifHeight;\n> > > + bdsHeight = height;\n> > > break;\n> > > }\n> > > }\n> > > @@ -155,12 +156,13 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe,\n> const Size &iif, const Size &gdc\n> > > while (ifHeight >= minIFHeight && ifHeight <=\n> iif.height &&\n> > > ifHeight / bdsSF >= minBDSHeight) {\n> > >\n> > > - bdsHeight = ifHeight / bdsSF;\n> > > - if (std::fmod(bdsHeight, 1.0) == 0) {\n> > > - unsigned int bdsIntHeight =\n> static_cast(bdsHeight);\n> > > + float height = ifHeight / bdsSF;\n> > > + if (std::fmod(height, 1.0) == 0) {\n> > > + unsigned int bdsIntHeight =\n> static_cast(height);\n> > >\n> > > if (!(bdsIntHeight % BDS_ALIGN_H)) {\n> > > foundIfHeight = ifHeight;\n> > > + bdsHeight = height;\n> > > break;\n> > > }\n> > > }\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 6F5C0C3205\n\tfor ;\n\tMon, 31 May 2021 08:39:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C0DE468923;\n\tMon, 31 May 2021 10:39:19 +0200 (CEST)","from mail-ej1-x633.google.com (mail-ej1-x633.google.com\n\t[IPv6:2a00:1450:4864:20::633])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9E7D26891F\n\tfor ;\n\tMon, 31 May 2021 10:39:18 +0200 (CEST)","by mail-ej1-x633.google.com with SMTP id b9so15436131ejc.13\n\tfor ;\n\tMon, 31 May 2021 01:39:18 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"RI812S8v\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=DoDfrKLaNTtNwy22lFO1i7XGpcoQ+cN1MmcWCcn8JnY=;\n\tb=RI812S8vME/oFe+BvzgDE5tgVuWTHVeEc4HevV1X4N7lE9s/KoIcaFU99fSVDJmu1+\n\t4XcV4oR23FCqd4eJaNaCq9QcgPyUp61VvZOV87njBN12HOZJ6lz+agy7Nr8qu+NNUsrn\n\tIJC+L1tqWLuiYy7xQt9UycHjNZgGIG3EfIJ18=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=DoDfrKLaNTtNwy22lFO1i7XGpcoQ+cN1MmcWCcn8JnY=;\n\tb=MgoAu0Lg8Jcibg1hlpfvzeeUWiH7WaFe5VYOUsffENkA4t/pZPaAWB+drHuuT4anzF\n\tbVCFUJ6wkoLGzB6iDhpQ1r8mg6e4tntFpPOuDUo/wrw4SLaipoOXED2XVpcKIYmRK6TJ\n\t2HB4Dh9RuBlQvHtdlPyE49x3/9006KpQ3KGwin6zXpHRzrG/ptUeuN1dAfHqXrIX6vqu\n\tAmqk85iSx1BoER9WlwvTVcmjmgICyuLZJ3SVOzdbT3fMt4QQ92kp36F2JB58hRZmlkQt\n\tw7tA5p0E6DG+ssEERKE+U86yFXH2QaJ4U1shFuTHlQ66Qm+ZV6ynO4+Z0mWe4Zb02Mze\n\tfbjA==","X-Gm-Message-State":"AOAM531VYdeoRt3Pq1rte0Vfc+XRFZCApNgleDS5bL7TnkX6jyzuMgnY\n\tS5zRCO9JNSELUagsTAw9azYVI1KY/9gInEMk955d8iQmcY4=","X-Google-Smtp-Source":"ABdhPJztCJSkr7ui7M7aoYETRUBHR/CZAX2qbXOLzPbdKGwTGI+irZNSH/ygkKTbPza+otdapiyOpAscse5Wdt02uac=","X-Received":"by 2002:a17:907:1b06:: with SMTP id\n\tmp6mr21881072ejc.292.1622450358100; \n\tMon, 31 May 2021 01:39:18 -0700 (PDT)","MIME-Version":"1.0","References":"<20210531020745.13815-1-laurent.pinchart@ideasonboard.com>\n\t<20210531020745.13815-3-laurent.pinchart@ideasonboard.com>\n\t\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Mon, 31 May 2021 17:39:07 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"00000000000042bf8a05c39c2909\"","Subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: pipeline: ipu3: Fix\n\tincorrect bdsHeight calculation","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17354,"web_url":"https://patchwork.libcamera.org/comment/17354/","msgid":"","date":"2021-05-31T08:43:48","subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: pipeline: ipu3: Fix\n\tincorrect bdsHeight calculation","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nOn Mon, May 31, 2021 at 05:39:07PM +0900, Hirokazu Honda wrote:\n> On Mon, May 31, 2021 at 5:37 PM Laurent Pinchart wrote:\n> > On Mon, May 31, 2021 at 01:50:12PM +0900, Hirokazu Honda wrote:\n> > > On Mon, May 31, 2021 at 11:08 AM Laurent Pinchart wrote:\n> > >\n> > > > When compiling with optimization, gcc 9 and newer throw an unitialized\n> > > > variable warning:\n> > > >\n> > > > ../../src/libcamera/pipeline/ipu3/imgu.cpp: In function ‘void\n> > > >\n> > libcamera::{anonymous}::calculateBDSHeight(libcamera::ImgUDevice::Pipe*,\n> > > > const libcamera::Size&, const libcamera::Size&, unsigned int, float)’:\n> > > > ../../src/libcamera/pipeline/ipu3/imgu.cpp:172:17: error: ‘bdsHeight’\n> > may\n> > > > be used uninitialized in this function [-Werror=maybe-uninitialized]\n> > > > 172 | unsigned int bdsIntHeight = static_cast > > > int>(bdsHeight);\n> > > >\n> > > > Neither clang not gcc versions older than 9 complain. This seems to be\n> > > > a false positive.\n> > > >\n> > > > However, there's an obvious error in the code. The second while () loop\n> > > > in the first part of calculateBDSHeight() modifies the bdsHeight\n> > > > variable set by the first loop even if the second loop doesn't find a\n> > > > suitable height. This can result in an incorrect bdsHeight value. Fix\n> > > > this, which also gets rid of the compiler warning.\n> > > >\n> > > > Signed-off-by: Laurent Pinchart \n> > >\n> > > The fix looks correct to me.\n> > > Reviewed-by: Hirokazu Honda \n> >\n> > As mentioned in the cover letter, which of 1/2 and 2/2 do you think we\n> > should merge ?\n> \n> I prefer 2/2.\n\nOK, I'll run a full CTS on that to check for regressions.\n\n> > > > ---\n> > > > src/libcamera/pipeline/ipu3/imgu.cpp | 14 ++++++++------\n> > > > 1 file changed, 8 insertions(+), 6 deletions(-)\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp\n> > > > b/src/libcamera/pipeline/ipu3/imgu.cpp\n> > > > index 3e517ac67962..4eb3f7b730a9 100644\n> > > > --- a/src/libcamera/pipeline/ipu3/imgu.cpp\n> > > > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp\n> > > > @@ -138,12 +138,13 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc\n> > > > while (ifHeight >= minIFHeight && ifHeight <= iif.height &&\n> > > > ifHeight / bdsSF >= minBDSHeight) {\n> > > >\n> > > > - bdsHeight = ifHeight / bdsSF;\n> > > > - if (std::fmod(bdsHeight, 1.0) == 0) {\n> > > > - unsigned int bdsIntHeight = static_cast(bdsHeight);\n> > > > + float height = ifHeight / bdsSF;\n> > > > + if (std::fmod(height, 1.0) == 0) {\n> > > > + unsigned int bdsIntHeight = static_cast(height);\n> > > >\n> > > > if (!(bdsIntHeight % BDS_ALIGN_H)) {\n> > > > foundIfHeight = ifHeight;\n> > > > + bdsHeight = height;\n> > > > break;\n> > > > }\n> > > > }\n> > > > @@ -155,12 +156,13 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc\n> > > > while (ifHeight >= minIFHeight && ifHeight <= iif.height &&\n> > > > ifHeight / bdsSF >= minBDSHeight) {\n> > > >\n> > > > - bdsHeight = ifHeight / bdsSF;\n> > > > - if (std::fmod(bdsHeight, 1.0) == 0) {\n> > > > - unsigned int bdsIntHeight = static_cast(bdsHeight);\n> > > > + float height = ifHeight / bdsSF;\n> > > > + if (std::fmod(height, 1.0) == 0) {\n> > > > + unsigned int bdsIntHeight = static_cast(height);\n> > > >\n> > > > if (!(bdsIntHeight % BDS_ALIGN_H)) {\n> > > > foundIfHeight = ifHeight;\n> > > > + bdsHeight = height;\n> > > > break;\n> > > > }\n> > > > }","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 5E707C3206\n\tfor ;\n\tMon, 31 May 2021 08:44:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BDEA868925;\n\tMon, 31 May 2021 10:44:00 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A69426891F\n\tfor ;\n\tMon, 31 May 2021 10:43:58 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1398A1178;\n\tMon, 31 May 2021 10:43:58 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"V8PmZh52\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1622450638;\n\tbh=gOQhAjwN064QPCesUnAUZiCQcj4B81us434lz3NQ3IY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=V8PmZh520kl6gIcProOwrctHoYTJIy7nWaajP9PgTQO2qPJGP1Vokgb88/ElM7RZc\n\tkKXH27vo+/ptv7gQUE6nZx/m/nrLO6OM0wGm/24bH3ZREXDBptB40mRiB7/Aqoil0d\n\tDquZ++ysLAQ6VtQ8Jfdd8KTPnyl59VLKbQa7Cfpo=","Date":"Mon, 31 May 2021 11:43:48 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210531020745.13815-1-laurent.pinchart@ideasonboard.com>\n\t<20210531020745.13815-3-laurent.pinchart@ideasonboard.com>\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: pipeline: ipu3: Fix\n\tincorrect bdsHeight calculation","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17359,"web_url":"https://patchwork.libcamera.org/comment/17359/","msgid":"","date":"2021-05-31T18:28:13","subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: pipeline: ipu3: Fix\n\tincorrect bdsHeight calculation","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 31/05/2021 04:07, Laurent Pinchart wrote:\n> When compiling with optimization, gcc 9 and newer throw an unitialized\n> variable warning:\n> \n> ../../src/libcamera/pipeline/ipu3/imgu.cpp: In function ‘void libcamera::{anonymous}::calculateBDSHeight(libcamera::ImgUDevice::Pipe*, const libcamera::Size&, const libcamera::Size&, unsigned int, float)’:\n> ../../src/libcamera/pipeline/ipu3/imgu.cpp:172:17: error: ‘bdsHeight’ may be used uninitialized in this function [-Werror=maybe-uninitialized]\n> 172 | unsigned int bdsIntHeight = static_cast(bdsHeight);\n> \n> Neither clang not gcc versions older than 9 complain. This seems to be\n> a false positive.\n> \n> However, there's an obvious error in the code. The second while () loop\n> in the first part of calculateBDSHeight() modifies the bdsHeight\n> variable set by the first loop even if the second loop doesn't find a\n> suitable height. This can result in an incorrect bdsHeight value. Fix\n> this, which also gets rid of the compiler warning.\n> \n> Signed-off-by: Laurent Pinchart \nTested-by: Jean-Michel Hautbois \nReviewed-by: Jean-Michel Hautbois \n> ---\n> src/libcamera/pipeline/ipu3/imgu.cpp | 14 ++++++++------\n> 1 file changed, 8 insertions(+), 6 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp\n> index 3e517ac67962..4eb3f7b730a9 100644\n> --- a/src/libcamera/pipeline/ipu3/imgu.cpp\n> +++ b/src/libcamera/pipeline/ipu3/imgu.cpp\n> @@ -138,12 +138,13 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc\n> \t\twhile (ifHeight >= minIFHeight && ifHeight <= iif.height &&\n> \t\t ifHeight / bdsSF >= minBDSHeight) {\n> \n> -\t\t\tbdsHeight = ifHeight / bdsSF;\n> -\t\t\tif (std::fmod(bdsHeight, 1.0) == 0) {\n> -\t\t\t\tunsigned int bdsIntHeight = static_cast(bdsHeight);\n> +\t\t\tfloat height = ifHeight / bdsSF;\n> +\t\t\tif (std::fmod(height, 1.0) == 0) {\n> +\t\t\t\tunsigned int bdsIntHeight = static_cast(height);\n> \n> \t\t\t\tif (!(bdsIntHeight % BDS_ALIGN_H)) {\n> \t\t\t\t\tfoundIfHeight = ifHeight;\n> +\t\t\t\t\tbdsHeight = height;\n> \t\t\t\t\tbreak;\n> \t\t\t\t}\n> \t\t\t}\n> @@ -155,12 +156,13 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc\n> \t\twhile (ifHeight >= minIFHeight && ifHeight <= iif.height &&\n> \t\t ifHeight / bdsSF >= minBDSHeight) {\n> \n> -\t\t\tbdsHeight = ifHeight / bdsSF;\n> -\t\t\tif (std::fmod(bdsHeight, 1.0) == 0) {\n> -\t\t\t\tunsigned int bdsIntHeight = static_cast(bdsHeight);\n> +\t\t\tfloat height = ifHeight / bdsSF;\n> +\t\t\tif (std::fmod(height, 1.0) == 0) {\n> +\t\t\t\tunsigned int bdsIntHeight = static_cast(height);\n> \n> \t\t\t\tif (!(bdsIntHeight % BDS_ALIGN_H)) {\n> \t\t\t\t\tfoundIfHeight = ifHeight;\n> +\t\t\t\t\tbdsHeight = height;\n> \t\t\t\t\tbreak;\n> \t\t\t\t}\n> \t\t\t}\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id B3D72C3205\n\tfor ;\n\tMon, 31 May 2021 18:28:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7758E602A8;\n\tMon, 31 May 2021 20:28:15 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 46581602A8\n\tfor ;\n\tMon, 31 May 2021 20:28:14 +0200 (CEST)","from tatooine.ideasonboard.com (unknown\n\t[IPv6:2a01:e0a:169:7140:c372:462d:cfcc:b7d1])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EAF996EF;\n\tMon, 31 May 2021 20:28:13 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"D3a8b2E2\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1622485694;\n\tbh=XHKsE6T4xASJRzNOMc0W9uqWq578Fn223+SISLYpwPg=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=D3a8b2E2TPGhanhZAfDdU4MwhwiGD2AWvXcmGSTJOQ9Eq71gJvRO6jF92POgy+n3n\n\tyexUeqQzWWp9wiVspc4cAdsOKG+PzUgxyKaSnq9fuea9GjQ1pFOlRi6idQfgvxZ74t\n\tmI6POn+p40LgKKOIlxEVrW2mwVq3H/GtnAR8WkE0=","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210531020745.13815-1-laurent.pinchart@ideasonboard.com>\n\t<20210531020745.13815-3-laurent.pinchart@ideasonboard.com>","From":"Jean-Michel Hautbois ","Message-ID":"","Date":"Mon, 31 May 2021 20:28:13 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.8.1","MIME-Version":"1.0","In-Reply-To":"<20210531020745.13815-3-laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: pipeline: ipu3: Fix\n\tincorrect bdsHeight calculation","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]