[{"id":17954,"web_url":"https://patchwork.libcamera.org/comment/17954/","msgid":"","date":"2021-07-02T15:44:50","subject":"Re: [libcamera-devel] [PATCH v3 2/8] ipa: raspberrypi: Add a\n\tconstructor struct DeviceStatus","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Naush\n\nThanks for tidying this up!\n\nOn Fri, 2 Jul 2021 at 16:09, Naushir Patuck wrote:\n>\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>\n> Signed-off-by: Naushir Patuck \n> ---\n> src/ipa/raspberrypi/controller/device_status.h | 6 ++++++\n> src/ipa/raspberrypi/controller/rpi/agc.cpp | 2 +-\n> src/ipa/raspberrypi/controller/rpi/lux.cpp | 8 +-------\n> 3 files changed, 8 insertions(+), 8 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> + DeviceStatus()\n> + : shutter_speed(std::chrono::seconds(0)), 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 b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> index 6c3a4eb2a04d..393cfacea025 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> @@ -172,7 +172,7 @@ 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> + last_device_status_ = {};\n\nI only wondered slightly whether this could be left out and we rely on\nthe new constructor iniitialising last_device_stats_ automatically?\n\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..a3ae633b867a 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/lux.cpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/lux.cpp\n> @@ -61,13 +61,7 @@ void Lux::Prepare(Metadata *image_metadata)\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> - .lens_position = 0.0,\n> - .aperture = 0.0,\n> - .flash_intensity = 0.0\n> - };\n> + DeviceStatus device_status;\n\nMaybe we can remove my slightly off-hand remark about shutting up the\ncompiler too?\n\nNotwithstanding those very minor things:\n\nReviewed-by: David Plowman \n\nThanks\nDavid\n\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> 2.25.1\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 96D50BD794\n\tfor ;\n\tFri, 2 Jul 2021 15:45:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 04A6268500;\n\tFri, 2 Jul 2021 17:45:04 +0200 (CEST)","from mail-wr1-x42d.google.com (mail-wr1-x42d.google.com\n\t[IPv6:2a00:1450:4864:20::42d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6789E684E6\n\tfor ;\n\tFri, 2 Jul 2021 17:45:02 +0200 (CEST)","by mail-wr1-x42d.google.com with SMTP id t6so3091048wrm.9\n\tfor ;\n\tFri, 02 Jul 2021 08:45:02 -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=\"m/lm+TPI\"; 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=+B1KWKaj3mYILYv15JOnr7tU87eTLMQ7FrGZWev8PuA=;\n\tb=m/lm+TPI+olIQmRRhoHF9DFWXe2nZEVA7kAWevBK3sUemq/cdmZXMKtYd5nYEJAqTj\n\tN54nUb6GHCS0MBtBAhc2Up4dWUEb6NeMuHZmbNnai49ly4lIZM6becWZwFPjr9gQrY4R\n\tdsh5BGtevoWu7bwPbZyr8BA6JwE1gue3wiO6nTCMcJNhm3yTV0Rgn/fim6wQQ5CMogxq\n\tXcUnz/0Ht9mCmq44jiL1anHI7FdnRCV3k/bvKdNpXNfmBewu9syQGrSqNQozYQFW4q0i\n\tpSwosXGPyRVSzHYNNfkYX2uHqUzyDBt0GP8lqgY7UmlAyueURd4vahyfM8+J2yBVVg46\n\ty9ww==","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=+B1KWKaj3mYILYv15JOnr7tU87eTLMQ7FrGZWev8PuA=;\n\tb=bQVusBuR97OuBk1NFhtkhETCLV3X+5f2cBjnEKXNwp8tUtbjz31nLJeHUEDf5d5dhN\n\tuOZlENef0iPVtD1eTGmWCiQ0qSzQi5nNCSds6qo+GgtMQ/ydGjRq8ZQ9EUo3XtYk40e+\n\tbzFa0BJ5tO/o0lawOP2CDgAP8CxUZsQ2Q+1DXG2dBPi7dUozyuZjeKayNH63tM+E+LV4\n\tcGaftLFspZoRZnHP8ue1yMnYvCCB+iD/1aW/spGh0jyvgT1Bog/BMwVkRCLDsv8+3VPP\n\t5ZdCIJXnmdBtMB2YNJGYh2ApIc/iFUN7mOhZiPuGyh3gy4Pscl1Tw/VOTprh8QBrXewd\n\t+7Kg==","X-Gm-Message-State":"AOAM530GTaC9uVYYTNhyXwlhDxlCNNBrb9hVg+Dha3qOn2ubKxHVETcj\n\tP9GKEmPorGSX+k6jtWbpByxpxQVh5wJT7mNFlxTNGQ==","X-Google-Smtp-Source":"ABdhPJwk4GN1e7j8Ip809chMC2TCe42gphQHqr9PIj7mC1Z+4Ns7Bceo8Z6cQqr5oyjPJ6z8D0fGhg+8dPKkf+pEO9E=","X-Received":"by 2002:a5d:4bca:: with SMTP id l10mr318484wrt.236.1625240702135;\n\tFri, 02 Jul 2021 08:45:02 -0700 (PDT)","MIME-Version":"1.0","References":"<20210702150940.226941-1-naush@raspberrypi.com>\n\t<20210702150940.226941-3-naush@raspberrypi.com>","In-Reply-To":"<20210702150940.226941-3-naush@raspberrypi.com>","From":"David Plowman ","Date":"Fri, 2 Jul 2021 16:44:50 +0100","Message-ID":"","To":"Naushir Patuck ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v3 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\" "}},{"id":17956,"web_url":"https://patchwork.libcamera.org/comment/17956/","msgid":"","date":"2021-07-02T15:59:53","subject":"Re: [libcamera-devel] [PATCH v3 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 David,\n\nThank you for your review feedback.\n\nOn Fri, 2 Jul 2021 at 16:45, David Plowman \nwrote:\n\n> Hi Naush\n>\n> Thanks for tidying this up!\n>\n> On Fri, 2 Jul 2021 at 16:09, Naushir Patuck wrote:\n> >\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> > Signed-off-by: Naushir Patuck \n> > ---\n> > src/ipa/raspberrypi/controller/device_status.h | 6 ++++++\n> > src/ipa/raspberrypi/controller/rpi/agc.cpp | 2 +-\n> > src/ipa/raspberrypi/controller/rpi/lux.cpp | 8 +-------\n> > 3 files changed, 8 insertions(+), 8 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..393cfacea025 100644\n> > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > @@ -172,7 +172,7 @@ 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> > + last_device_status_ = {};\n>\n> I only wondered slightly whether this could be left out and we rely on\n> the new constructor iniitialising last_device_stats_ automatically?\n>\n\nOops, yes we can!\n\n\n>\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..a3ae633b867a 100644\n> > --- a/src/ipa/raspberrypi/controller/rpi/lux.cpp\n> > +++ b/src/ipa/raspberrypi/controller/rpi/lux.cpp\n> > @@ -61,13 +61,7 @@ void Lux::Prepare(Metadata *image_metadata)\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> > - .lens_position = 0.0,\n> > - .aperture = 0.0,\n> > - .flash_intensity = 0.0\n> > - };\n> > + DeviceStatus device_status;\n>\n> Maybe we can remove my slightly off-hand remark about shutting up the\n> compiler too?\n>\n\nWill remove the comment in the next revision.\n\nRegards,\nNaush\n\n\n>\n> Notwithstanding those very minor things:\n>\n> Reviewed-by: David Plowman \n>\n> Thanks\n> David\n>\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> > 2.25.1\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 6E2D8BD794\n\tfor ;\n\tFri, 2 Jul 2021 16:00:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D54B268505;\n\tFri, 2 Jul 2021 18:00:12 +0200 (CEST)","from mail-lj1-x232.google.com (mail-lj1-x232.google.com\n\t[IPv6:2a00:1450:4864:20::232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 63DE1684E6\n\tfor ;\n\tFri, 2 Jul 2021 18:00:11 +0200 (CEST)","by mail-lj1-x232.google.com with SMTP id k8so13920507lja.4\n\tfor ;\n\tFri, 02 Jul 2021 09:00:11 -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=\"Wm0MsSvI\"; 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=GGunLZzJedkNE4aOPQgARmvCm4HeORzGjccLQ99HR6s=;\n\tb=Wm0MsSvIjOc59Jowid+bDrFM2fMNiDSX2Zt7NbZa1aKqn2DGpseOCPu7jiUiozYg3w\n\tNb5GNSw/+CUrm+zm11HfcHDhFsOQHRqxBLo7JQRDsmwsWibM4tF7w+gQQBOamrvzFd2t\n\tuVytyQAQ3ElFsOiXfiOaFj6eBkIJfkkpb1k7xy3O9bL1kX8DRn0SGEfi6nTHpSq2fmmz\n\td6LltLL3Kx63Vhzj9wggvDyAC4m+jvUpVUfcV62+gk11amXhQf54ddOlPpeFQC8q1p1x\n\tN3Bhjag1S0VVyeOBq6T5rHxg9nfzIxPg6Mj4kvLwBvFuRxDvoRh/LWWxKalT5nrHbrjX\n\tSNeg==","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=GGunLZzJedkNE4aOPQgARmvCm4HeORzGjccLQ99HR6s=;\n\tb=NO64N8TbaWBGcretXD0hW9OTeJ1hrHzspSDhP4n26vTkXTQRviMqLMTo0EzbbNczMS\n\tdHnBzne1JWrlCD7xcvbVPLUT666E9/TaXZ0mIaqgiEJ75K5R60cesQpwDUEjjVNl+MR3\n\tr/yeNvGI3RUA2t/Ti4ghnHrmqxkEBWzN+qdlxXOS07plQsPkVwc7iaW/9UNJ947GIhxY\n\tH9X7x8076O5SLfJWBEvkMF6ZjuYG2FAtUEcQjzOw0zw35AnndDsnfvAD/kMtlN3/8R6x\n\tqk8zYMVnY5TmakDAfidbUi+iwi3pOhcqSJf58v+IGwlWLiWHk7gh8KC+rG0mLfBSMOB3\n\tpz1Q==","X-Gm-Message-State":"AOAM532wrTmsqeXle/iLF/EhAN0wW5Zpt3LsXXw8+EJb1vsvPjHKplhY\n\t3BWokCz+TXeEz9o28YBrLR8FI6xwOxTawzP1rxYaqw==","X-Google-Smtp-Source":"ABdhPJzrPMnaM+uGRdeOSVglGzQTB66jamplhRXGDqlNYE8TVH2Qo6JRWU8KHf25kRlvYbkfb1RApQSQAYmM0ZhWWsw=","X-Received":"by 2002:a2e:9f11:: with SMTP id u17mr154660ljk.48.1625241610102; \n\tFri, 02 Jul 2021 09:00:10 -0700 (PDT)","MIME-Version":"1.0","References":"<20210702150940.226941-1-naush@raspberrypi.com>\n\t<20210702150940.226941-3-naush@raspberrypi.com>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Fri, 2 Jul 2021 16:59:53 +0100","Message-ID":"","To":"David Plowman ","Content-Type":"multipart/alternative; boundary=\"000000000000d838bb05c6260c3e\"","Subject":"Re: [libcamera-devel] [PATCH v3 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\" "}}]