[{"id":17382,"web_url":"https://patchwork.libcamera.org/comment/17382/","msgid":"","date":"2021-06-02T10:10:27","subject":"Re: [libcamera-devel] [PATCH 1/2] ipa: raspberrypi: Add sensitivity\n\tfield to camera mode","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 work.\n\n\nOn Thu, 27 May 2021 at 09:45, David Plowman \nwrote:\n\n> Initialise it to the usual value of 1, until such time as we have a\n> mechanism that allows us to discover the correct value.\n>\n> The CamHelper class also gets a method to return this sensitivity\n> value. This method is virtual so that it can be overridden for\n> specific sensors. Once the correct value is obtainable elsewhere, this\n> can be removed.\n>\n> Signed-off-by: David Plowman \n> ---\n> src/ipa/raspberrypi/cam_helper.cpp | 5 +++++\n> src/ipa/raspberrypi/cam_helper.hpp | 3 +++\n> src/ipa/raspberrypi/controller/camera_mode.h | 2 ++\n> src/ipa/raspberrypi/raspberrypi.cpp | 12 ++++++++++++\n> 4 files changed, 22 insertions(+)\n>\n> diff --git a/src/ipa/raspberrypi/cam_helper.cpp\n> b/src/ipa/raspberrypi/cam_helper.cpp\n> index 09917f3c..71ea21ff 100644\n> --- a/src/ipa/raspberrypi/cam_helper.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper.cpp\n> @@ -128,6 +128,11 @@ bool CamHelper::SensorEmbeddedDataPresent() const\n> return false;\n> }\n>\n> +double CamHelper::Sensitivity() const\n> +{\n> + return mode_.sensitivity;\n> +}\n> +\n> unsigned int CamHelper::HideFramesStartup() const\n> {\n> /*\n> diff --git a/src/ipa/raspberrypi/cam_helper.hpp\n> b/src/ipa/raspberrypi/cam_helper.hpp\n> index a52f3f0b..14c4714c 100644\n> --- a/src/ipa/raspberrypi/cam_helper.hpp\n> +++ b/src/ipa/raspberrypi/cam_helper.hpp\n> @@ -39,6 +39,8 @@ namespace RPiController {\n> //\n> // A method to query if the sensor outputs embedded data that can be\n> parsed.\n> //\n> +// A method to return the sensitivity of the current camera mode.\n> +//\n> // A parser to parse the embedded data buffers provided by some sensors\n> (for\n> // example, the imx219 does; the ov5647 doesn't). This allows us to know\n> for\n> // sure the exposure and gain of the frame we're looking at. CamHelper\n> @@ -81,6 +83,7 @@ public:\n> virtual void GetDelays(int &exposure_delay, int &gain_delay,\n> int &vblank_delay) const;\n> virtual bool SensorEmbeddedDataPresent() const;\n> + virtual double Sensitivity() const;\n> virtual unsigned int HideFramesStartup() const;\n> virtual unsigned int HideFramesModeSwitch() const;\n> virtual unsigned int MistrustFramesStartup() const;\n> diff --git a/src/ipa/raspberrypi/controller/camera_mode.h\n> b/src/ipa/raspberrypi/controller/camera_mode.h\n> index 256438c9..326f4f20 100644\n> --- a/src/ipa/raspberrypi/controller/camera_mode.h\n> +++ b/src/ipa/raspberrypi/controller/camera_mode.h\n> @@ -39,6 +39,8 @@ struct CameraMode {\n> libcamera::Transform transform;\n> // minimum and maximum fame lengths in units of lines\n> uint32_t min_frame_length, max_frame_length;\n> + // sensitivity of this mode\n> + double sensitivity;\n> };\n>\n> #ifdef __cplusplus\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> index e5bb8159..da42d279 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -321,6 +321,12 @@ void IPARPi::setMode(const IPACameraSensorInfo\n> &sensorInfo)\n> */\n> mode_.min_frame_length = sensorInfo.minFrameLength;\n> mode_.max_frame_length = sensorInfo.maxFrameLength;\n> +\n> + /*\n> + * For now, initialise the mode sensitivity to the usual value of\n> 1.\n> + * \\todo Obtain the correct sensitivity number automatically.\n> + */\n> + mode_.sensitivity = 1.0;\n> }\n>\n> int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,\n> @@ -379,6 +385,12 @@ int IPARPi::configure(const IPACameraSensorInfo\n> &sensorInfo,\n> /* Pass the camera mode to the CamHelper to setup algorithms. */\n> helper_->SetCameraMode(mode_);\n>\n> + /*\n> + * For now, the CamHelper knows the sensitivity correctly.\n> + * \\todo This can be removed once the sensitivity is initialised\n> properly.\n> + */\n> + mode_.sensitivity = helper_->Sensitivity();\n> +\n>\n\nI'm afraid I don't quite follow the logic here. setMode sets the\nsensitivity in mode_,\nthen this gets updated via the helper to the same value. Obviously some of\nthis is\nplaceholder code right now, but could you explain how this may be expected\nto work\neventually?\n\nAnother related question, do we need to involve the CamHelper at all?\nCould we do\neverything in the setMode function?\n\nThanks,\nNaush\n\n\n> if (firstStart_) {\n> /* Supply initial values for frame durations. */\n> applyFrameDurations(defaultMinFrameDuration,\n> defaultMaxFrameDuration);\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 41396C3206\n\tfor ;\n\tWed, 2 Jun 2021 10:10:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B88926050E;\n\tWed, 2 Jun 2021 12:10:45 +0200 (CEST)","from mail-lf1-x134.google.com (mail-lf1-x134.google.com\n\t[IPv6:2a00:1450:4864:20::134])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 24F2C602A4\n\tfor ;\n\tWed, 2 Jun 2021 12:10:44 +0200 (CEST)","by mail-lf1-x134.google.com with SMTP id b18so2465527lfv.11\n\tfor ;\n\tWed, 02 Jun 2021 03:10:44 -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=\"TKrnFf36\"; 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=kYXV6pqzjSg4hLi7B2fiJjenoUzDrZyHKPVBnYCKMn4=;\n\tb=TKrnFf36qolcc7bsSTklje8p0D77v4ijIFpBn7MQ3CwrM+YZrSKA/dkOvP/X44sPhG\n\tlcEGJiD3dWooeIIGBKO0N2c18C58Zg/2IVVm18364g+TrlYPpSDYZJCyoNGLAkYQTW1t\n\tEzruAUfD/uIuRtWNJEP+9ICLg+juQjUz1OUfVUAkCniLPi4V7ab53aMoDsfHpTDMdhOx\n\trMGbKM4TQf6oDkPOyjT6B4XJbMYRVcnHV3il0c0UVydohX5kniTV31Sp6n2dkuaFBr8B\n\tXWXNT1DEDRNrLPJJwvN6FZK8BOIlDP2/kVs/DfqIMs1hDBCNFng+DGbE3NbaLoijxwpZ\n\tO4YA==","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=kYXV6pqzjSg4hLi7B2fiJjenoUzDrZyHKPVBnYCKMn4=;\n\tb=mExYQ8+nkTRZHCcp8wCLqoxf+ORI7liYswHQW5m8GwgRA3uEYkT/SQzLEKeThyLFOh\n\tJ7kaIJx3dBopxTAlIg14AWs+ZvjAUAJCrDwtz0h5OO5hr1kJVlbtt866TBQZUkQOl7Ck\n\tUPe5Wmn83/tZXJ9JbEHC3oX+RViAlH+POx/bRgNsOWpZ+4g4T5oZMSm/rb6AA4Y4YgN1\n\txYpEPxJr4yDF14+337O5dPvsbTh2sc6EKuuJURCILfzWM+FeXqn1AUxIwd6rF0xuqTiH\n\tOAVbULdwX9c2E/dP5YqeZcOr29EOIqvw25hHUzsqSan+nxz14t02agRy9zORkc9ddrJX\n\tkQ8w==","X-Gm-Message-State":"AOAM531J5vXTDBXukeE9JOnoLbIXvs1Y/gIETOvt8eQpSjBZghswjmA+\n\tw/5nwF6zzAP/RpqaG43XL1PmrX10ICe3D06qchYbzA==","X-Google-Smtp-Source":"ABdhPJy7t52HGjm5R9lK4x1cvhJMdlN2I1O7yKIQFZ0ZBODou2b7HcQg6UrwcizDhXk/CVV13+VR/HU8p8VxyCtrb5Q=","X-Received":"by 2002:a19:e05a:: with SMTP id g26mr982739lfj.210.1622628643544;\n\tWed, 02 Jun 2021 03:10:43 -0700 (PDT)","MIME-Version":"1.0","References":"<20210527084540.9983-1-david.plowman@raspberrypi.com>\n\t<20210527084540.9983-2-david.plowman@raspberrypi.com>","In-Reply-To":"<20210527084540.9983-2-david.plowman@raspberrypi.com>","From":"Naushir Patuck ","Date":"Wed, 2 Jun 2021 11:10:27 +0100","Message-ID":"","To":"David Plowman ","Content-Type":"multipart/alternative; boundary=\"000000000000e6b0d405c3c5ab24\"","Subject":"Re: [libcamera-devel] [PATCH 1/2] ipa: raspberrypi: Add sensitivity\n\tfield to camera mode","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":17384,"web_url":"https://patchwork.libcamera.org/comment/17384/","msgid":"","date":"2021-06-02T11:10:23","subject":"Re: [libcamera-devel] [PATCH 1/2] ipa: raspberrypi: Add sensitivity\n\tfield to camera mode","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 the reply!\n\nOn Wed, 2 Jun 2021 at 11:10, Naushir Patuck wrote:\n>\n> Hi David,\n>\n> Thank you for your work.\n>\n>\n> On Thu, 27 May 2021 at 09:45, David Plowman wrote:\n>>\n>> Initialise it to the usual value of 1, until such time as we have a\n>> mechanism that allows us to discover the correct value.\n>>\n>> The CamHelper class also gets a method to return this sensitivity\n>> value. This method is virtual so that it can be overridden for\n>> specific sensors. Once the correct value is obtainable elsewhere, this\n>> can be removed.\n>>\n>> Signed-off-by: David Plowman \n>> ---\n>> src/ipa/raspberrypi/cam_helper.cpp | 5 +++++\n>> src/ipa/raspberrypi/cam_helper.hpp | 3 +++\n>> src/ipa/raspberrypi/controller/camera_mode.h | 2 ++\n>> src/ipa/raspberrypi/raspberrypi.cpp | 12 ++++++++++++\n>> 4 files changed, 22 insertions(+)\n>>\n>> diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp\n>> index 09917f3c..71ea21ff 100644\n>> --- a/src/ipa/raspberrypi/cam_helper.cpp\n>> +++ b/src/ipa/raspberrypi/cam_helper.cpp\n>> @@ -128,6 +128,11 @@ bool CamHelper::SensorEmbeddedDataPresent() const\n>> return false;\n>> }\n>>\n>> +double CamHelper::Sensitivity() const\n>> +{\n>> + return mode_.sensitivity;\n>> +}\n>> +\n>> unsigned int CamHelper::HideFramesStartup() const\n>> {\n>> /*\n>> diff --git a/src/ipa/raspberrypi/cam_helper.hpp b/src/ipa/raspberrypi/cam_helper.hpp\n>> index a52f3f0b..14c4714c 100644\n>> --- a/src/ipa/raspberrypi/cam_helper.hpp\n>> +++ b/src/ipa/raspberrypi/cam_helper.hpp\n>> @@ -39,6 +39,8 @@ namespace RPiController {\n>> //\n>> // A method to query if the sensor outputs embedded data that can be parsed.\n>> //\n>> +// A method to return the sensitivity of the current camera mode.\n>> +//\n>> // A parser to parse the embedded data buffers provided by some sensors (for\n>> // example, the imx219 does; the ov5647 doesn't). This allows us to know for\n>> // sure the exposure and gain of the frame we're looking at. CamHelper\n>> @@ -81,6 +83,7 @@ public:\n>> virtual void GetDelays(int &exposure_delay, int &gain_delay,\n>> int &vblank_delay) const;\n>> virtual bool SensorEmbeddedDataPresent() const;\n>> + virtual double Sensitivity() const;\n>> virtual unsigned int HideFramesStartup() const;\n>> virtual unsigned int HideFramesModeSwitch() const;\n>> virtual unsigned int MistrustFramesStartup() const;\n>> diff --git a/src/ipa/raspberrypi/controller/camera_mode.h b/src/ipa/raspberrypi/controller/camera_mode.h\n>> index 256438c9..326f4f20 100644\n>> --- a/src/ipa/raspberrypi/controller/camera_mode.h\n>> +++ b/src/ipa/raspberrypi/controller/camera_mode.h\n>> @@ -39,6 +39,8 @@ struct CameraMode {\n>> libcamera::Transform transform;\n>> // minimum and maximum fame lengths in units of lines\n>> uint32_t min_frame_length, max_frame_length;\n>> + // sensitivity of this mode\n>> + double sensitivity;\n>> };\n>>\n>> #ifdef __cplusplus\n>> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n>> index e5bb8159..da42d279 100644\n>> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n>> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n>> @@ -321,6 +321,12 @@ void IPARPi::setMode(const IPACameraSensorInfo &sensorInfo)\n>> */\n>> mode_.min_frame_length = sensorInfo.minFrameLength;\n>> mode_.max_frame_length = sensorInfo.maxFrameLength;\n>> +\n>> + /*\n>> + * For now, initialise the mode sensitivity to the usual value of 1.\n>> + * \\todo Obtain the correct sensitivity number automatically.\n>> + */\n>> + mode_.sensitivity = 1.0;\n>> }\n>>\n>> int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,\n>> @@ -379,6 +385,12 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,\n>> /* Pass the camera mode to the CamHelper to setup algorithms. */\n>> helper_->SetCameraMode(mode_);\n>>\n>> + /*\n>> + * For now, the CamHelper knows the sensitivity correctly.\n>> + * \\todo This can be removed once the sensitivity is initialised properly.\n>> + */\n>> + mode_.sensitivity = helper_->Sensitivity();\n>> +\n>\n>\n> I'm afraid I don't quite follow the logic here. setMode sets the sensitivity in mode_,\n> then this gets updated via the helper to the same value. Obviously some of this is\n> placeholder code right now, but could you explain how this may be expected to work\n> eventually?\n\nSo the missing piece of the puzzle is that my under-development-camera\noverrides the default Sensitivity method with\n\ndouble CamHelperXXX::Sensitivity() const\n{\n return isBinned() ? 2.0 : 1.0;\n}\n\nThere's actually a slightly awkward chicken-and-egg thing going on\nhere. So where I initially set \"mode_.sensitivity = 1.0\", I'd rather\nhave\n\"mode_.sensitivity = helper_->Sensitivity()\".\nOnly the helper doesn't know the mode yet.\n\nPerhaps having a method which allowed us to write\n\"helper_->SetSensitivity(mode_)\" instead would be less weird? It would\nlook like:\n\nvoid CamHelperXXX:SetSensitivity(CameraMode &mode) const\n{\n mode.sensitivity = isBinned() ? 2.0 : 1.0;\n}\n\nWhat do you think?\n\n>\n> Another related question, do we need to involve the CamHelper at all? Could we do\n> everything in the setMode function?\n\nWasn't quite sure what you meant here... the CamHelper has to be\ninvolved somewhere, it's the only thing that knows that the binned\nmode for this sensor is \"different\", right?\n\nThanks again\nDavid\n\n>\n> Thanks,\n> Naush\n>\n>>\n>> if (firstStart_) {\n>> /* Supply initial values for frame durations. */\n>> applyFrameDurations(defaultMinFrameDuration, defaultMaxFrameDuration);\n>>\n>> --\n>> 2.20.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 5A08BC3208\n\tfor ;\n\tWed, 2 Jun 2021 11:10:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B90D06050E;\n\tWed, 2 Jun 2021 13:10:36 +0200 (CEST)","from mail-wm1-x332.google.com (mail-wm1-x332.google.com\n\t[IPv6:2a00:1450:4864:20::332])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D3397602A4\n\tfor ;\n\tWed, 2 Jun 2021 13:10:34 +0200 (CEST)","by mail-wm1-x332.google.com with SMTP id o127so1008549wmo.4\n\tfor ;\n\tWed, 02 Jun 2021 04:10:34 -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=\"g7nRjzm/\"; 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=Ck8IZbSWl/y5VU/f8EggmGWRlCenPeSsI8UXxbj/KrI=;\n\tb=g7nRjzm/2hSgE9nZcgayGxMQTcZpZNrnXNuQoDYBDFGqEjH59egVHPbNCgC8p0D4Rq\n\tKuI7CG//1tbdL4KlUv3syi4jH00qz3KuTtCUzyPCsmi6ssYqaRHCExfBDKaxuiMObQjy\n\tDkuCKiHvAmcL03xCM07IxzidQgFy17n8VTNveK0/C0WFgtDhFvd+8WiE65uupiXr1Wuw\n\tRVO/o0Rfg8qvVBeZZ8w6P26Gb4ZmKvlJHQaLeJDjODJqKSGIUR+UeBvjndLDRFCn1CR8\n\tK6IpZiqAWk9Ok5DmiobcyHJKoaCtw99PNh86SIMYRj+EwG4sBsbWxLQPXLi32TZkmSkQ\n\tZy5w==","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=Ck8IZbSWl/y5VU/f8EggmGWRlCenPeSsI8UXxbj/KrI=;\n\tb=mvN9mId8iW2gcESoT9yXi/3mv91rvAbzv2cgO/eCHLvi6w8qRFptmCi+X1r5odxApC\n\tbPafIK1c8VB8pISYOQUElUHcbZqlSymrIe3oXyYyl/Jv5t++3VbfhMyfXTlDGk/f9iFu\n\tVDpUiwt95cu4y13j9AJW8mahfpDVxXt//m/ab/MA0LO2oMaSluhY6imJOg8hPLEv4G/a\n\tKheFxVZzkNNQF001iVH94Od2mai0URL5D/FGphU6IqxvcB18Bwvr4wcGRoFleN2ugkRe\n\tjPt5Acr4mYiUvNtBBQuxc9wPUpITZNsJo5NNMRlJo1mccuwSTEfK+yulMKNV29qxZY+c\n\tsCMg==","X-Gm-Message-State":"AOAM531YWsSN+DUgFa1XdT+Y8vI6ebc1Jor3kg+N0HP4qRa+to1YeejP\n\txL4r0unQj6/bjAyOr1N7W3WTK8XroBqNXzpIXplK32t1/hA38A==","X-Google-Smtp-Source":"ABdhPJwK17U8I7myrTeqfeAz6bNyYoZZLIU0RQvmjvh3FN7ouyb3NDtJwKKiyIKNghRSt/P4Dgrk/Lh1V6TTIgA5EsA=","X-Received":"by 2002:a1c:7912:: with SMTP id\n\tl18mr4158349wme.135.1622632234394; \n\tWed, 02 Jun 2021 04:10:34 -0700 (PDT)","MIME-Version":"1.0","References":"<20210527084540.9983-1-david.plowman@raspberrypi.com>\n\t<20210527084540.9983-2-david.plowman@raspberrypi.com>\n\t","In-Reply-To":"","From":"David Plowman ","Date":"Wed, 2 Jun 2021 12:10:23 +0100","Message-ID":"","To":"Naushir Patuck ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 1/2] ipa: raspberrypi: Add sensitivity\n\tfield to camera mode","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":17387,"web_url":"https://patchwork.libcamera.org/comment/17387/","msgid":"","date":"2021-06-02T12:04:55","subject":"Re: [libcamera-devel] [PATCH 1/2] ipa: raspberrypi: Add sensitivity\n\tfield to camera mode","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi David,\n\nOn Wed, 2 Jun 2021 at 12:10, David Plowman \nwrote:\n\n> Hi Naush\n>\n> Thanks for the reply!\n>\n> On Wed, 2 Jun 2021 at 11:10, Naushir Patuck wrote:\n> >\n> > Hi David,\n> >\n> > Thank you for your work.\n> >\n> >\n> > On Thu, 27 May 2021 at 09:45, David Plowman <\n> david.plowman@raspberrypi.com> wrote:\n> >>\n> >> Initialise it to the usual value of 1, until such time as we have a\n> >> mechanism that allows us to discover the correct value.\n> >>\n> >> The CamHelper class also gets a method to return this sensitivity\n> >> value. This method is virtual so that it can be overridden for\n> >> specific sensors. Once the correct value is obtainable elsewhere, this\n> >> can be removed.\n> >>\n> >> Signed-off-by: David Plowman \n> >> ---\n> >> src/ipa/raspberrypi/cam_helper.cpp | 5 +++++\n> >> src/ipa/raspberrypi/cam_helper.hpp | 3 +++\n> >> src/ipa/raspberrypi/controller/camera_mode.h | 2 ++\n> >> src/ipa/raspberrypi/raspberrypi.cpp | 12 ++++++++++++\n> >> 4 files changed, 22 insertions(+)\n> >>\n> >> diff --git a/src/ipa/raspberrypi/cam_helper.cpp\n> b/src/ipa/raspberrypi/cam_helper.cpp\n> >> index 09917f3c..71ea21ff 100644\n> >> --- a/src/ipa/raspberrypi/cam_helper.cpp\n> >> +++ b/src/ipa/raspberrypi/cam_helper.cpp\n> >> @@ -128,6 +128,11 @@ bool CamHelper::SensorEmbeddedDataPresent() const\n> >> return false;\n> >> }\n> >>\n> >> +double CamHelper::Sensitivity() const\n> >> +{\n> >> + return mode_.sensitivity;\n> >> +}\n> >> +\n> >> unsigned int CamHelper::HideFramesStartup() const\n> >> {\n> >> /*\n> >> diff --git a/src/ipa/raspberrypi/cam_helper.hpp\n> b/src/ipa/raspberrypi/cam_helper.hpp\n> >> index a52f3f0b..14c4714c 100644\n> >> --- a/src/ipa/raspberrypi/cam_helper.hpp\n> >> +++ b/src/ipa/raspberrypi/cam_helper.hpp\n> >> @@ -39,6 +39,8 @@ namespace RPiController {\n> >> //\n> >> // A method to query if the sensor outputs embedded data that can be\n> parsed.\n> >> //\n> >> +// A method to return the sensitivity of the current camera mode.\n> >> +//\n> >> // A parser to parse the embedded data buffers provided by some\n> sensors (for\n> >> // example, the imx219 does; the ov5647 doesn't). This allows us to\n> know for\n> >> // sure the exposure and gain of the frame we're looking at. CamHelper\n> >> @@ -81,6 +83,7 @@ public:\n> >> virtual void GetDelays(int &exposure_delay, int &gain_delay,\n> >> int &vblank_delay) const;\n> >> virtual bool SensorEmbeddedDataPresent() const;\n> >> + virtual double Sensitivity() const;\n> >> virtual unsigned int HideFramesStartup() const;\n> >> virtual unsigned int HideFramesModeSwitch() const;\n> >> virtual unsigned int MistrustFramesStartup() const;\n> >> diff --git a/src/ipa/raspberrypi/controller/camera_mode.h\n> b/src/ipa/raspberrypi/controller/camera_mode.h\n> >> index 256438c9..326f4f20 100644\n> >> --- a/src/ipa/raspberrypi/controller/camera_mode.h\n> >> +++ b/src/ipa/raspberrypi/controller/camera_mode.h\n> >> @@ -39,6 +39,8 @@ struct CameraMode {\n> >> libcamera::Transform transform;\n> >> // minimum and maximum fame lengths in units of lines\n> >> uint32_t min_frame_length, max_frame_length;\n> >> + // sensitivity of this mode\n> >> + double sensitivity;\n> >> };\n> >>\n> >> #ifdef __cplusplus\n> >> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> >> index e5bb8159..da42d279 100644\n> >> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> >> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> >> @@ -321,6 +321,12 @@ void IPARPi::setMode(const IPACameraSensorInfo\n> &sensorInfo)\n> >> */\n> >> mode_.min_frame_length = sensorInfo.minFrameLength;\n> >> mode_.max_frame_length = sensorInfo.maxFrameLength;\n> >> +\n> >> + /*\n> >> + * For now, initialise the mode sensitivity to the usual value\n> of 1.\n> >> + * \\todo Obtain the correct sensitivity number automatically.\n> >> + */\n> >> + mode_.sensitivity = 1.0;\n> >> }\n> >>\n> >> int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,\n> >> @@ -379,6 +385,12 @@ int IPARPi::configure(const IPACameraSensorInfo\n> &sensorInfo,\n> >> /* Pass the camera mode to the CamHelper to setup algorithms. */\n> >> helper_->SetCameraMode(mode_);\n> >>\n> >> + /*\n> >> + * For now, the CamHelper knows the sensitivity correctly.\n> >> + * \\todo This can be removed once the sensitivity is\n> initialised properly.\n> >> + */\n> >> + mode_.sensitivity = helper_->Sensitivity();\n> >> +\n> >\n> >\n> > I'm afraid I don't quite follow the logic here. setMode sets the\n> sensitivity in mode_,\n> > then this gets updated via the helper to the same value. Obviously some\n> of this is\n> > placeholder code right now, but could you explain how this may be\n> expected to work\n> > eventually?\n>\n> So the missing piece of the puzzle is that my under-development-camera\n> overrides the default Sensitivity method with\n>\n> double CamHelperXXX::Sensitivity() const\n> {\n> return isBinned() ? 2.0 : 1.0;\n> }\n\n\nRight, I get it now.\n\n\n>\n> There's actually a slightly awkward chicken-and-egg thing going on\n> here. So where I initially set \"mode_.sensitivity = 1.0\", I'd rather\n> have\n> \"mode_.sensitivity = helper_->Sensitivity()\".\n> Only the helper doesn't know the mode yet.\n>\n> Perhaps having a method which allowed us to write\n> \"helper_->SetSensitivity(mode_)\" instead would be less weird? It would\n> look like:\n>\n> void CamHelperXXX:SetSensitivity(CameraMode &mode) const\n> {\n> mode.sensitivity = isBinned() ? 2.0 : 1.0;\n> }\n>\n> What do you think?\n>\n\nYes, I see the awkwardness here. You can get around the chicken-and-egg\nproblem\nby perhaps not storing the sensitivity in the mode structure, but having an\nAPI in\nAgcAlgorithm to set sensitivity directly? Not sure I mind any approach.\n\n\n\n>\n> >\n> > Another related question, do we need to involve the CamHelper at all?\n> Could we do\n> > everything in the setMode function?\n>\n> Wasn't quite sure what you meant here... the CamHelper has to be\n> involved somewhere, it's the only thing that knows that the binned\n> mode for this sensor is \"different\", right?\n>\n\nIgnore this comment, with your above explanation this does not make sense\n:-)\n\nThanks,\nNaush\n\n\n\n>\n> Thanks again\n> David\n>\n> >\n> > Thanks,\n> > Naush\n> >\n> >>\n> >> if (firstStart_) {\n> >> /* Supply initial values for frame durations. */\n> >> applyFrameDurations(defaultMinFrameDuration,\n> defaultMaxFrameDuration);\n> >>\n> >> --\n> >> 2.20.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 A9264C3206\n\tfor ;\n\tWed, 2 Jun 2021 12:05:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 00791602AF;\n\tWed, 2 Jun 2021 14:05:14 +0200 (CEST)","from mail-lf1-x12c.google.com (mail-lf1-x12c.google.com\n\t[IPv6:2a00:1450:4864:20::12c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 68D73602A4\n\tfor ;\n\tWed, 2 Jun 2021 14:05:13 +0200 (CEST)","by mail-lf1-x12c.google.com with SMTP id v8so2973953lft.8\n\tfor ;\n\tWed, 02 Jun 2021 05:05:13 -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=\"WNZ6epMw\"; 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=+jlEk0If64cH0qCMQw+kvq/Lu+J/e+EHe+k3BSj0vRY=;\n\tb=WNZ6epMwNxrSPf+3611AJVST1s3KNMLjwrA4DSyNip6qECowqXESjvX8PlvjX7CD05\n\t3oRiyB+OvhYKoPSRNRYbC2Fu5rRR9XGT7Su4bKuOjP7hf1QSEEKu8YF33k+BxJwt8woF\n\t/k1rXF7iUMa6SgMlChfxb2IbSPz3kfa/lnMWv08NjLQs4n9zXbLK+lJVXB42czj7KK3S\n\t/H9vGpJYurdJTmQgvGgsIOrbUHyW95axtqpZq+9hfC6PHSwTvT3bbLdf8WRukZR2Cnh/\n\tf/WfuRrx09Uh2W0boHee5JRkT7utPpt4G6H2OW2SxA4bevVeT+L51B4lA90NcS/cpWaB\n\tX79g==","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=+jlEk0If64cH0qCMQw+kvq/Lu+J/e+EHe+k3BSj0vRY=;\n\tb=tVM3R1rSlXYOgqPMsm8+VLpAr6jGrffycUX/hA6nNKPGMSgJ5DZUotbLMVnlWM/5zW\n\tQLL5RTmRUUwHdsMbi86fA3eCdkOz1NDUzVahF2oN+L0IE0xQETTtQUskAHEVGGDVlpp7\n\thhgixpbCQkrOYb1ZlicymR3/tjTYzAxztO7T2HfiW3yV9P/5wzbS9MTJkmkMICnx+oP9\n\tre8b3opMlDTJilAF9KhS6808ducEio++/Elg/jUS7/ellCGoAjpq91Dk5gu4yLdVvSNd\n\t2O5M8unpafjKIa0cQsJcDtElE7mOy5sRCLWN7fdInHWDBcelCr4WLJ093CC54UearuQs\n\tq1Hw==","X-Gm-Message-State":"AOAM5310D3x/jCn5ozkqjgLrxbH/PPDbd9G4xSsaotQlAY1r3nJB+24i\n\thHx0OoZW0nXfeLRq6UtGFq0PzFcBPCh28j3ezj3E5tuVW18=","X-Google-Smtp-Source":"ABdhPJyxIYZEjjvVRZ4TwEGOwX+If5mgAtcCSphrYa/Hxz491b6wQS5JvFhoDP7GZQURKK02QyMbDygzf+ZKUSUVB3o=","X-Received":"by 2002:a05:6512:3f2:: with SMTP id\n\tn18mr13297915lfq.617.1622635511204; \n\tWed, 02 Jun 2021 05:05:11 -0700 (PDT)","MIME-Version":"1.0","References":"<20210527084540.9983-1-david.plowman@raspberrypi.com>\n\t<20210527084540.9983-2-david.plowman@raspberrypi.com>\n\t\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Wed, 2 Jun 2021 13:04:55 +0100","Message-ID":"","To":"David Plowman ","Content-Type":"multipart/alternative; boundary=\"0000000000003edcc805c3c74580\"","Subject":"Re: [libcamera-devel] [PATCH 1/2] ipa: raspberrypi: Add sensitivity\n\tfield to camera mode","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\" "}}]