[{"id":18046,"web_url":"https://patchwork.libcamera.org/comment/18046/","msgid":"","date":"2021-07-09T13:00:45","subject":"Re: [libcamera-devel] [PATCH v5 2/8] ipa: raspberrypi: Add a\n\tconstructor struct DeviceStatus","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Naush,\n\nOn 09/07/2021 10:56, Naushir Patuck wrote:\n> The constructor sets all fields to 0. This replaces the memset(0) and default\n> value initialisation usage in the agc and lux controllers respectively.\n\nI guess this makes everything a bit more explicit.\n\nIs it to fix coverity warnings? or just a general improvement?\n\n> Signed-off-by: Naushir Patuck \n> Reviewed-by: David Plowman \n> ---\n> src/ipa/raspberrypi/controller/device_status.h | 6 ++++++\n> src/ipa/raspberrypi/controller/rpi/agc.cpp | 1 -\n> src/ipa/raspberrypi/controller/rpi/lux.cpp | 9 +--------\n> 3 files changed, 7 insertions(+), 9 deletions(-)\n> \n> diff --git a/src/ipa/raspberrypi/controller/device_status.h b/src/ipa/raspberrypi/controller/device_status.h\n> index 733378dbfa27..73df7ce228dd 100644\n> --- a/src/ipa/raspberrypi/controller/device_status.h\n> +++ b/src/ipa/raspberrypi/controller/device_status.h\n> @@ -14,6 +14,12 @@\n> */\n> \n> struct DeviceStatus {\n> +\tDeviceStatus()\n> +\t\t: shutter_speed(std::chrono::seconds(0)), analogue_gain(0.0),\n> +\t\t lens_position(0.0), aperture(0.0), flash_intensity(0.0)\n> +\t{\n> +\t}\n> +\n> \t/* time shutter is open */\n> \tlibcamera::utils::Duration shutter_speed;\n> \tdouble analogue_gain;\n> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> index 6c3a4eb2a04d..f57783f821ec 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> @@ -172,7 +172,6 @@ Agc::Agc(Controller *controller)\n> \t// it's not been calculated yet (i.e. Process hasn't yet run).\n> \tmemset(&status_, 0, sizeof(status_));\n> \tstatus_.ev = ev_;\n> -\tmemset(&last_device_status_, 0, sizeof(last_device_status_));\n> }\n> \n> char const *Agc::Name() const\n> diff --git a/src/ipa/raspberrypi/controller/rpi/lux.cpp b/src/ipa/raspberrypi/controller/rpi/lux.cpp\n> index f58d69397e1c..6367b17dc7f4 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/lux.cpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/lux.cpp\n> @@ -60,14 +60,7 @@ void Lux::Prepare(Metadata *image_metadata)\n> \n> void Lux::Process(StatisticsPtr &stats, Metadata *image_metadata)\n> {\n> -\t// set some initial values to shut the compiler up\n> -\tDeviceStatus device_status = {\n> -\t\t.shutter_speed = 1.0ms,\n> -\t\t.analogue_gain = 1.0,\n\nThese are now going to be set to 0, I assume that's not an issue if\npreviously they're just default values to satisfy the compiler...\n\nReviewed-by: Kieran Bingham \n\n> -\t\t.lens_position = 0.0,\n> -\t\t.aperture = 0.0,\n> -\t\t.flash_intensity = 0.0\n> -\t};\n> +\tDeviceStatus device_status;\n> \tif (image_metadata->Get(\"device.status\", device_status) == 0) {\n> \t\tdouble current_gain = device_status.analogue_gain;\n> \t\tdouble current_aperture = device_status.aperture;\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 27A8BC3224\n\tfor ;\n\tFri, 9 Jul 2021 13:00:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 884A36851B;\n\tFri, 9 Jul 2021 15:00:49 +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 808A6605AC\n\tfor ;\n\tFri, 9 Jul 2021 15:00:48 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0F44AE7;\n\tFri, 9 Jul 2021 15:00:48 +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=\"fK3TBN9u\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1625835648;\n\tbh=lUuhJqGjvwIuMSCloWlVuvcKVwbTXTebKOptACRD8ZI=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=fK3TBN9uQl272732FmFeIUZO5XEypm0MIXYZE8Daua4y2IszbsXfa0dHScbUaFM4Y\n\tGm/EDS808LbkdyFcFFm8G5SlKp2NTQnuIbAsx+7SqxVe4iLtO8Vzp+7ccl/EOEa45+\n\t8BDebPbTCgzwcwiINb0eKeUz+8bk4euKFYu0Zhi8=","To":"Naushir Patuck ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210709095638.2801713-1-naush@raspberrypi.com>\n\t<20210709095638.2801713-2-naush@raspberrypi.com>","From":"Kieran Bingham ","Message-ID":"","Date":"Fri, 9 Jul 2021 14:00:45 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"<20210709095638.2801713-2-naush@raspberrypi.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v5 2/8] ipa: raspberrypi: Add a\n\tconstructor struct DeviceStatus","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\" "}},{"id":18048,"web_url":"https://patchwork.libcamera.org/comment/18048/","msgid":"","date":"2021-07-09T13:14:14","subject":"Re: [libcamera-devel] [PATCH v5 2/8] ipa: raspberrypi: Add a\n\tconstructor struct DeviceStatus","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Kieran,\n\nThank you for your review feedback.\n\nOn Fri, 9 Jul 2021 at 14:00, Kieran Bingham \nwrote:\n\n> Hi Naush,\n>\n> On 09/07/2021 10:56, Naushir Patuck wrote:\n> > The constructor sets all fields to 0. This replaces the memset(0) and\n> default\n> > value initialisation usage in the agc and lux controllers respectively.\n>\n> I guess this makes everything a bit more explicit.\n>\n> Is it to fix coverity warnings? or just a general improvement?\n>\n\nIt's just general improvements, mostly to allow the fixups below.\n\n\n\n> > Signed-off-by: Naushir Patuck \n> > Reviewed-by: David Plowman \n> > ---\n> > src/ipa/raspberrypi/controller/device_status.h | 6 ++++++\n> > src/ipa/raspberrypi/controller/rpi/agc.cpp | 1 -\n> > src/ipa/raspberrypi/controller/rpi/lux.cpp | 9 +--------\n> > 3 files changed, 7 insertions(+), 9 deletions(-)\n> >\n> > diff --git a/src/ipa/raspberrypi/controller/device_status.h\n> b/src/ipa/raspberrypi/controller/device_status.h\n> > index 733378dbfa27..73df7ce228dd 100644\n> > --- a/src/ipa/raspberrypi/controller/device_status.h\n> > +++ b/src/ipa/raspberrypi/controller/device_status.h\n> > @@ -14,6 +14,12 @@\n> > */\n> >\n> > struct DeviceStatus {\n> > + DeviceStatus()\n> > + : shutter_speed(std::chrono::seconds(0)),\n> analogue_gain(0.0),\n> > + lens_position(0.0), aperture(0.0), flash_intensity(0.0)\n> > + {\n> > + }\n> > +\n> > /* time shutter is open */\n> > libcamera::utils::Duration shutter_speed;\n> > double analogue_gain;\n> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > index 6c3a4eb2a04d..f57783f821ec 100644\n> > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > @@ -172,7 +172,6 @@ Agc::Agc(Controller *controller)\n> > // it's not been calculated yet (i.e. Process hasn't yet run).\n> > memset(&status_, 0, sizeof(status_));\n> > status_.ev = ev_;\n> > - memset(&last_device_status_, 0, sizeof(last_device_status_));\n> > }\n> >\n> > char const *Agc::Name() const\n> > diff --git a/src/ipa/raspberrypi/controller/rpi/lux.cpp\n> b/src/ipa/raspberrypi/controller/rpi/lux.cpp\n> > index f58d69397e1c..6367b17dc7f4 100644\n> > --- a/src/ipa/raspberrypi/controller/rpi/lux.cpp\n> > +++ b/src/ipa/raspberrypi/controller/rpi/lux.cpp\n> > @@ -60,14 +60,7 @@ void Lux::Prepare(Metadata *image_metadata)\n> >\n> > void Lux::Process(StatisticsPtr &stats, Metadata *image_metadata)\n> > {\n> > - // set some initial values to shut the compiler up\n> > - DeviceStatus device_status = {\n> > - .shutter_speed = 1.0ms,\n> > - .analogue_gain = 1.0,\n>\n> These are now going to be set to 0, I assume that's not an issue if\n> previously they're just default values to satisfy the compiler...\n>\n\nCorrect, we now have the explicit constructor so no need for this\nugliness!\n\n\n> Reviewed-by: Kieran Bingham \n>\n> > - .lens_position = 0.0,\n> > - .aperture = 0.0,\n> > - .flash_intensity = 0.0\n> > - };\n> > + DeviceStatus device_status;\n> > if (image_metadata->Get(\"device.status\", device_status) == 0) {\n> > double current_gain = device_status.analogue_gain;\n> > double current_aperture = device_status.aperture;\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 9217AC3224\n\tfor ;\n\tFri, 9 Jul 2021 13:14:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4A5566851D;\n\tFri, 9 Jul 2021 15:14:33 +0200 (CEST)","from mail-lj1-x231.google.com (mail-lj1-x231.google.com\n\t[IPv6:2a00:1450:4864:20::231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 11AAD605AC\n\tfor ;\n\tFri, 9 Jul 2021 15:14:31 +0200 (CEST)","by mail-lj1-x231.google.com with SMTP id t30so7963770ljo.5\n\tfor ;\n\tFri, 09 Jul 2021 06:14:31 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"RUAbjfFN\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=0AHhOTi92gp/FglpHHO2OY4dFEOsbhHM+c8qGZdpwJo=;\n\tb=RUAbjfFNJY+HvQEZheiEK3j0Za5Nj9TBzg9WwvN5/oHfxy9MRSY/I1gvuGJk7+46D9\n\t41X9V8RB58M23PNqzRC5HNxj56N+5IdBS2U2M/mfG/v/Qrfxf9Ho83aqn6HLn9OkVRKK\n\tg+gDXgYI1RqW5zfDRaKQS/HFuvWcwpFxrgejnE/bx1272mCf+BZF73fa3B6mcwghnDz5\n\tGeEC2k63/8Ww7ICkTCz4cyKbE6D8JzYr1wqMIJ3vfpXxPwKhbJzdp2xhR9P6ucVGnXQ9\n\tck+Y04h7JNKNPIVAQU/xgQqBLlx81p5HluQKCWLW1Rr6/IoZEYxIgFo0AQvsl19TOwGu\n\tQXgA==","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=0AHhOTi92gp/FglpHHO2OY4dFEOsbhHM+c8qGZdpwJo=;\n\tb=BSxD3RbHs7RoPuONNpgm6FxeshplZDkYARzdkRhUU+mPKSZYAFOe24KTqAORWemR46\n\tBhKOZsGpX/3IVbcuvruZY/yMx6A4yu5ZnKgBnxwWeF2iv37ADdufHUWUu5mhHAV5lJkW\n\t8roFgVHdDtVXDOLk2xVlX/hEVMpYnp/MR8hdgYw7p204wQ6vKGdfct+5qLc7rPP6pqZv\n\tEk3hvV+DXteQhKgq4L7VKRyFXRVI88WSivPErkS9Efmg5e9bpIaV66gBadDx2TULEo3A\n\t5WIJ/7jylLVK0GMSTC5MFcHvVoKkDrwVOZXqCiwQixvJ72JRoiDjk5rJ2TQCi/L0EskG\n\tEMyQ==","X-Gm-Message-State":"AOAM530sFb53BnG0JnAdHq7zygeswEPy49u/hlbIRpfnCWYA2o0U+ZNy\n\tathQ6Xs4FBLoug5AiV0tZJsXmbofWEtLOvUbHiokWA==","X-Google-Smtp-Source":"ABdhPJyjcTcpEU8WPLYxpDd8iB0AOWmWaVwNUlvVvvhpu1q7AzYQA26W0jXKihHuap8/+595Qf4asmHj/D18xX2vHRA=","X-Received":"by 2002:a2e:9910:: with SMTP id\n\tv16mr10624458lji.499.1625836470399; \n\tFri, 09 Jul 2021 06:14:30 -0700 (PDT)","MIME-Version":"1.0","References":"<20210709095638.2801713-1-naush@raspberrypi.com>\n\t<20210709095638.2801713-2-naush@raspberrypi.com>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Fri, 9 Jul 2021 14:14:14 +0100","Message-ID":"","To":"Kieran Bingham ","Content-Type":"multipart/alternative; boundary=\"000000000000480ce705c6b08d3a\"","Subject":"Re: [libcamera-devel] [PATCH v5 2/8] ipa: raspberrypi: Add a\n\tconstructor struct DeviceStatus","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\" "}}]