[{"id":20999,"web_url":"https://patchwork.libcamera.org/comment/20999/","msgid":"<14d14215-7111-c32a-13ff-f29e03e1cac5@ideasonboard.com>","date":"2021-11-18T11:10:58","subject":"Re: [libcamera-devel] [PATCH v2] android: Check exposure time range\n\tfor manual sensor capability","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"Hi Paul,\n\nThanks for the patch !\n\nOn 18/11/2021 10:48, Paul Elder wrote:\n> In the manual sensor capability validator, add a check for the exposure\n> time range. While at it, since the minimum exposure time is not limited\n> to the manual sensor capability, add a check for it when initializing\n> static metadata.\n> \n> Signed-off-by: Paul Elder \n> Reviewed-by: Laurent Pinchart \n> \n> ---\n> Changes in v2:\n> - fix comparator order (cosmetic)\n> - change comparators and comments to \"equal or\", as that is what is\n> specificied in the hal docs\n> - add check for minimum exposure time when initializing static metadata\n> - this only prints error, should we return -EINVAL instead?\n> ---\n> src/android/camera_capabilities.cpp | 30 +++++++++++++++++++++++++++++\n> 1 file changed, 30 insertions(+)\n> \n> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp\n> index f357902e..6e46f163 100644\n> --- a/src/android/camera_capabilities.cpp\n> +++ b/src/android/camera_capabilities.cpp\n> @@ -217,6 +217,8 @@ std::vector setMetadata(CameraMetadata *metadata, uint32_t tag,\n> \n> bool CameraCapabilities::validateManualSensorCapability()\n> {\n> +\tcamera_metadata_ro_entry_t entry;\n> +\n> \tconst char *noMode = \"Manual sensor capability unavailable: \";\n> \n> \tif (!staticMetadata_->entryContains(ANDROID_CONTROL_AE_AVAILABLE_MODES,\n> @@ -231,6 +233,26 @@ bool CameraCapabilities::validateManualSensorCapability()\n> \t\treturn false;\n> \t}\n> \n> +\tif (!staticMetadata_->hasEntry(ANDROID_SENSOR_INFO_EXPOSURE_TIME_RANGE)) {\n> +\t\tLOG(HAL, Info) << noMode << \"missing exposure time range\";\n> +\t\treturn false;\n> +\t}\n> +\n> +\tstaticMetadata_->getEntry(ANDROID_SENSOR_INFO_EXPOSURE_TIME_RANGE, &entry);\n> +\tif (entry.data.i32[0] >= 100000) {\n> +\t\tLOG(HAL, Info)\n> +\t\t\t<< noMode\n> +\t\t\t<< \"exposure time range minimum must not be equal nor larger than 100us\";\n> +\t\treturn false;\n> +\t}\n> +\n> +\tif (entry.data.i32[1] <= 100000000) {\n> +\t\tLOG(HAL, Info)\n> +\t\t\t<< noMode\n> +\t\t\t<< \"exposure time range maximum must not be equal nor smaller than 100ms\";\n> +\t\treturn false;\n> +\t}\n> +\n> \t/*\n> \t * \\todo Return true here after we satisfy all the requirements:\n> \t * https://developer.android.com/reference/android/hardware/camera2/CameraMetadata#REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR\n> @@ -1074,6 +1096,14 @@ int CameraCapabilities::initializeStaticMetadata()\n> \t\t\texposureInfo->second.min().get() * 1000LL,\n> \t\t\texposureInfo->second.max().get() * 1000LL,\n> \t\t};\n> +\n> +\t\tif (exposureTimeRange[0] >= 100000) {\n> +\t\t\tLOG(HAL, Error)\n> +\t\t\t\t<< \"Minimum exposure time \"\n> +\t\t\t\t<< exposureTimeRange[0]\n> +\t\t\t\t<< \"ns is too big (should be smaller than 100us)\";\n> +\t\t}\n\nI read the doc (better now than never :-)) in \nhttps://developer.android.com/reference/android/hardware/camera2/CameraCharacteristics#SENSOR_INFO_EXPOSURE_TIME_RANGE\n\nInterestingly, the ExposureTimeRange is in nanoseconds. Our \nControls::ExposureTime metadata is in microseconds. Should it be changed ?\n\nReviewed-by: Jean-Michel Hautbois \n\n> +\n> \t\tstaticMetadata_->addEntry(ANDROID_SENSOR_INFO_EXPOSURE_TIME_RANGE,\n> \t\t\t\t\t exposureTimeRange, 2);\n> \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 C869FBF415\n\tfor ;\n\tThu, 18 Nov 2021 11:11:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 15D9060231;\n\tThu, 18 Nov 2021 12:11:03 +0100 (CET)","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 CCC2A60231\n\tfor ;\n\tThu, 18 Nov 2021 12:11:01 +0100 (CET)","from [IPV6:2a01:e0a:169:7140:b12c:8f66:c7d9:e3b7] (unknown\n\t[IPv6:2a01:e0a:169:7140:b12c:8f66:c7d9:e3b7])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 62BB893;\n\tThu, 18 Nov 2021 12:11:01 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"c3+WOZnW\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637233861;\n\tbh=234MolfeF6P+pYXUW+dXCqPOBljX2cBzBLxwT3aRYo4=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=c3+WOZnW3sKHBDbSnngVvl7o7jaDOaCpD2iB1lrgK89ff9U10Z2Z/wnSSiuPN3Wub\n\tnlLJlFdm+Bw5/S/u6m+X7JibYtzNCd5bgq39AnJjDsIndKdJMRddKU7nRGexdM1HS+\n\tdp4UElXEBEfGk9fC7erItGHKTVRyIGKpj8P7hwpU=","Message-ID":"<14d14215-7111-c32a-13ff-f29e03e1cac5@ideasonboard.com>","Date":"Thu, 18 Nov 2021 12:10:58 +0100","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.2.1","Content-Language":"en-US","To":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20211118094823.344496-1-paul.elder@ideasonboard.com>","From":"Jean-Michel Hautbois ","In-Reply-To":"<20211118094823.344496-1-paul.elder@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v2] android: Check exposure time range\n\tfor manual sensor capability","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":21000,"web_url":"https://patchwork.libcamera.org/comment/21000/","msgid":"","date":"2021-11-18T11:56:47","subject":"Re: [libcamera-devel] [PATCH v2] android: Check exposure time range\n\tfor manual sensor capability","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Thu, Nov 18, 2021 at 12:10:58PM +0100, Jean-Michel Hautbois wrote:\n> Hi Paul,\n> \n> Thanks for the patch !\n> \n> On 18/11/2021 10:48, Paul Elder wrote:\n> > In the manual sensor capability validator, add a check for the exposure\n> > time range. While at it, since the minimum exposure time is not limited\n> > to the manual sensor capability, add a check for it when initializing\n> > static metadata.\n> > \n> > Signed-off-by: Paul Elder \n> > Reviewed-by: Laurent Pinchart \n> > \n> > ---\n> > Changes in v2:\n> > - fix comparator order (cosmetic)\n> > - change comparators and comments to \"equal or\", as that is what is\n> > specificied in the hal docs\n> > - add check for minimum exposure time when initializing static metadata\n> > - this only prints error, should we return -EINVAL instead?\n> > ---\n> > src/android/camera_capabilities.cpp | 30 +++++++++++++++++++++++++++++\n> > 1 file changed, 30 insertions(+)\n> > \n> > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp\n> > index f357902e..6e46f163 100644\n> > --- a/src/android/camera_capabilities.cpp\n> > +++ b/src/android/camera_capabilities.cpp\n> > @@ -217,6 +217,8 @@ std::vector setMetadata(CameraMetadata *metadata, uint32_t tag,\n> > \n> > bool CameraCapabilities::validateManualSensorCapability()\n> > {\n> > +\tcamera_metadata_ro_entry_t entry;\n> > +\n> > \tconst char *noMode = \"Manual sensor capability unavailable: \";\n> > \n> > \tif (!staticMetadata_->entryContains(ANDROID_CONTROL_AE_AVAILABLE_MODES,\n> > @@ -231,6 +233,26 @@ bool CameraCapabilities::validateManualSensorCapability()\n> > \t\treturn false;\n> > \t}\n> > \n> > +\tif (!staticMetadata_->hasEntry(ANDROID_SENSOR_INFO_EXPOSURE_TIME_RANGE)) {\n> > +\t\tLOG(HAL, Info) << noMode << \"missing exposure time range\";\n> > +\t\treturn false;\n> > +\t}\n> > +\n> > +\tstaticMetadata_->getEntry(ANDROID_SENSOR_INFO_EXPOSURE_TIME_RANGE, &entry);\n> > +\tif (entry.data.i32[0] >= 100000) {\n> > +\t\tLOG(HAL, Info)\n> > +\t\t\t<< noMode\n> > +\t\t\t<< \"exposure time range minimum must not be equal nor larger than 100us\";\n\nMaybe \"must be smaller than 100us\" ?\n\n> > +\t\treturn false;\n> > +\t}\n> > +\n> > +\tif (entry.data.i32[1] <= 100000000) {\n> > +\t\tLOG(HAL, Info)\n> > +\t\t\t<< noMode\n> > +\t\t\t<< \"exposure time range maximum must not be equal nor smaller than 100ms\";\n\nSame here, \"must be larger than 100ms\".\n\n> > +\t\treturn false;\n> > +\t}\n> > +\n> > \t/*\n> > \t * \\todo Return true here after we satisfy all the requirements:\n> > \t * https://developer.android.com/reference/android/hardware/camera2/CameraMetadata#REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR\n> > @@ -1074,6 +1096,14 @@ int CameraCapabilities::initializeStaticMetadata()\n> > \t\t\texposureInfo->second.min().get() * 1000LL,\n> > \t\t\texposureInfo->second.max().get() * 1000LL,\n> > \t\t};\n> > +\n> > +\t\tif (exposureTimeRange[0] >= 100000) {\n> > +\t\t\tLOG(HAL, Error)\n> > +\t\t\t\t<< \"Minimum exposure time \"\n> > +\t\t\t\t<< exposureTimeRange[0]\n> > +\t\t\t\t<< \"ns is too big (should be smaller than 100us)\";\n\nDo we need to handle the error somehow ?\n\n> > +\t\t}\n> \n> I read the doc (better now than never :-)) in \n> https://developer.android.com/reference/android/hardware/camera2/CameraCharacteristics#SENSOR_INFO_EXPOSURE_TIME_RANGE\n> \n> Interestingly, the ExposureTimeRange is in nanoseconds. Our \n> Controls::ExposureTime metadata is in microseconds. Should it be changed ?\n\nI think we should standardize all times in nanoseconds in libcamera at\nsome point, unless there's a good technical reason not to do so.\n\n> Reviewed-by: Jean-Michel Hautbois \n> \n> > +\n> > \t\tstaticMetadata_->addEntry(ANDROID_SENSOR_INFO_EXPOSURE_TIME_RANGE,\n> > \t\t\t\t\t exposureTimeRange, 2);\n> > \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 D7ACABDB1C\n\tfor ;\n\tThu, 18 Nov 2021 11:57:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 262AA60371;\n\tThu, 18 Nov 2021 12:57:11 +0100 (CET)","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 2262C60231\n\tfor ;\n\tThu, 18 Nov 2021 12:57:10 +0100 (CET)","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 90B0E513;\n\tThu, 18 Nov 2021 12:57:09 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"HPQInNcm\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637236629;\n\tbh=9BLZ4eFrbq3oi40qfBz5kADsFPrhnwWfkfFB4qB8MCs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=HPQInNcmozwo23X3fZxINVCOVMKIY4R+9IW08J5xFTGyxk2jpINq5UW1AD0kgDmGM\n\tNwjyH/uWGIi0z6ZFR+wHZWsOX3MFmACWbVNw832R5vQLWAW1wi4Y1p5PGhdFEC5zu0\n\taaZDqUzd28LAaLL2S2hcigmf1yu4cI3SnB71AfcQ=","Date":"Thu, 18 Nov 2021 13:56:47 +0200","From":"Laurent Pinchart ","To":"Jean-Michel Hautbois ","Message-ID":"","References":"<20211118094823.344496-1-paul.elder@ideasonboard.com>\n\t<14d14215-7111-c32a-13ff-f29e03e1cac5@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<14d14215-7111-c32a-13ff-f29e03e1cac5@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2] android: Check exposure time range\n\tfor manual sensor capability","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@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21001,"web_url":"https://patchwork.libcamera.org/comment/21001/","msgid":"<163723676290.752913.4632402298462694005@Monstersaurus>","date":"2021-11-18T11:59:22","subject":"Re: [libcamera-devel] [PATCH v2] android: Check exposure time range\n\tfor manual sensor capability","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Jean-Michel Hautbois (2021-11-18 11:10:58)\n> Hi Paul,\n> \n> Thanks for the patch !\n> \n> On 18/11/2021 10:48, Paul Elder wrote:\n> > In the manual sensor capability validator, add a check for the exposure\n> > time range. While at it, since the minimum exposure time is not limited\n> > to the manual sensor capability, add a check for it when initializing\n> > static metadata.\n> > \n> > Signed-off-by: Paul Elder \n> > Reviewed-by: Laurent Pinchart \n> > \n> > ---\n> > Changes in v2:\n> > - fix comparator order (cosmetic)\n> > - change comparators and comments to \"equal or\", as that is what is\n> > specificied in the hal docs\n> > - add check for minimum exposure time when initializing static metadata\n> > - this only prints error, should we return -EINVAL instead?\n\nIs the issue caught by\nCameraCapabilities::validateManualSensorCapability() sufficiently?\n\n\n> > ---\n> > src/android/camera_capabilities.cpp | 30 +++++++++++++++++++++++++++++\n> > 1 file changed, 30 insertions(+)\n> > \n> > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp\n> > index f357902e..6e46f163 100644\n> > --- a/src/android/camera_capabilities.cpp\n> > +++ b/src/android/camera_capabilities.cpp\n> > @@ -217,6 +217,8 @@ std::vector setMetadata(CameraMetadata *metadata, uint32_t tag,\n> > \n> > bool CameraCapabilities::validateManualSensorCapability()\n> > {\n> > + camera_metadata_ro_entry_t entry;\n> > +\n> > const char *noMode = \"Manual sensor capability unavailable: \";\n> > \n> > if (!staticMetadata_->entryContains(ANDROID_CONTROL_AE_AVAILABLE_MODES,\n> > @@ -231,6 +233,26 @@ bool CameraCapabilities::validateManualSensorCapability()\n> > return false;\n> > }\n> > \n> > + if (!staticMetadata_->hasEntry(ANDROID_SENSOR_INFO_EXPOSURE_TIME_RANGE)) {\n> > + LOG(HAL, Info) << noMode << \"missing exposure time range\";\n> > + return false;\n> > + }\n> > +\n> > + staticMetadata_->getEntry(ANDROID_SENSOR_INFO_EXPOSURE_TIME_RANGE, &entry);\n> > + if (entry.data.i32[0] >= 100000) {\n> > + LOG(HAL, Info)\n> > + << noMode\n> > + << \"exposure time range minimum must not be equal nor larger than 100us\";\n> > + return false;\n> > + }\n> > +\n> > + if (entry.data.i32[1] <= 100000000) {\n> > + LOG(HAL, Info)\n> > + << noMode\n> > + << \"exposure time range maximum must not be equal nor smaller than 100ms\";\n> > + return false;\n> > + }\n> > +\n> > /*\n> > * \\todo Return true here after we satisfy all the requirements:\n> > * https://developer.android.com/reference/android/hardware/camera2/CameraMetadata#REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR\n> > @@ -1074,6 +1096,14 @@ int CameraCapabilities::initializeStaticMetadata()\n> > exposureInfo->second.min().get() * 1000LL,\n> > exposureInfo->second.max().get() * 1000LL,\n> > };\n> > +\n> > + if (exposureTimeRange[0] >= 100000) {\n> > + LOG(HAL, Error)\n> > + << \"Minimum exposure time \"\n> > + << exposureTimeRange[0]\n> > + << \"ns is too big (should be smaller than 100us)\";\n> > + }\n> \n> I read the doc (better now than never :-)) in \n> https://developer.android.com/reference/android/hardware/camera2/CameraCharacteristics#SENSOR_INFO_EXPOSURE_TIME_RANGE\n> \n> Interestingly, the ExposureTimeRange is in nanoseconds. Our \n> Controls::ExposureTime metadata is in microseconds. Should it be changed ?\n\nI can see some *1000LL above which I presume are converting the\nmicroseconds to nanoseconds ...\n\nMakes me wonder if we could use std::chrono more in the android layer\ntoo...\n\nWhy do we only report an error in initializeStaticMetadata() on the\nminimum, and not check the maximum?\n\nDo we need to check the minimum there at all if it's already validated\nin validateManualSensorCapability() ?\n\n\n> Reviewed-by: Jean-Michel Hautbois \n> \n> > +\n> > staticMetadata_->addEntry(ANDROID_SENSOR_INFO_EXPOSURE_TIME_RANGE,\n> > exposureTimeRange, 2);\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 74BD1BF415\n\tfor ;\n\tThu, 18 Nov 2021 11:59:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B713260371;\n\tThu, 18 Nov 2021 12:59:26 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A789260231\n\tfor ;\n\tThu, 18 Nov 2021 12:59:25 +0100 (CET)","from pendragon.ideasonboard.com\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 529C6E7;\n\tThu, 18 Nov 2021 12:59:25 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"hu6b8uKJ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637236765;\n\tbh=JMEErNZ94fQ8A6g/VxlQO+C/qvLJT3nQt0W+IQq11r8=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=hu6b8uKJI6GFPUAl1VH/p8oNsu/lozCMtseCLoQL/qN+sB8XvdV0z+0RcY563aiYd\n\tCepUeqGy5log3U7Xh2sbyzoUVQYlnYW1iu3sEVyvFcW7bGq62OstXOpfHhKmlaXyhd\n\tikIlPMtEMB7pVM0H2CErxKWvidsB6k+ep9/VrAFk=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<14d14215-7111-c32a-13ff-f29e03e1cac5@ideasonboard.com>","References":"<20211118094823.344496-1-paul.elder@ideasonboard.com>\n\t<14d14215-7111-c32a-13ff-f29e03e1cac5@ideasonboard.com>","From":"Kieran Bingham ","To":"Jean-Michel Hautbois ,\n\tPaul Elder ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 18 Nov 2021 11:59:22 +0000","Message-ID":"<163723676290.752913.4632402298462694005@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v2] android: Check exposure time range\n\tfor manual sensor capability","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":21002,"web_url":"https://patchwork.libcamera.org/comment/21002/","msgid":"<163723708404.752913.16018078276989970353@Monstersaurus>","date":"2021-11-18T12:04:44","subject":"Re: [libcamera-devel] [PATCH v2] android: Check exposure time range\n\tfor manual sensor capability","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Kieran Bingham (2021-11-18 11:59:22)\n> Quoting Jean-Michel Hautbois (2021-11-18 11:10:58)\n> > Hi Paul,\n> > \n> > Thanks for the patch !\n> > \n> > On 18/11/2021 10:48, Paul Elder wrote:\n> > > In the manual sensor capability validator, add a check for the exposure\n> > > time range. While at it, since the minimum exposure time is not limited\n> > > to the manual sensor capability, add a check for it when initializing\n> > > static metadata.\n> > > \n> > > Signed-off-by: Paul Elder \n> > > Reviewed-by: Laurent Pinchart \n> > > \n> > > ---\n> > > Changes in v2:\n> > > - fix comparator order (cosmetic)\n> > > - change comparators and comments to \"equal or\", as that is what is\n> > > specificied in the hal docs\n> > > - add check for minimum exposure time when initializing static metadata\n> > > - this only prints error, should we return -EINVAL instead?\n> \n> Is the issue caught by\n> CameraCapabilities::validateManualSensorCapability() sufficiently?\n> \n> \n> > > ---\n> > > src/android/camera_capabilities.cpp | 30 +++++++++++++++++++++++++++++\n> > > 1 file changed, 30 insertions(+)\n> > > \n> > > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp\n> > > index f357902e..6e46f163 100644\n> > > --- a/src/android/camera_capabilities.cpp\n> > > +++ b/src/android/camera_capabilities.cpp\n> > > @@ -217,6 +217,8 @@ std::vector setMetadata(CameraMetadata *metadata, uint32_t tag,\n> > > \n> > > bool CameraCapabilities::validateManualSensorCapability()\n> > > {\n> > > + camera_metadata_ro_entry_t entry;\n> > > +\n> > > const char *noMode = \"Manual sensor capability unavailable: \";\n> > > \n> > > if (!staticMetadata_->entryContains(ANDROID_CONTROL_AE_AVAILABLE_MODES,\n> > > @@ -231,6 +233,26 @@ bool CameraCapabilities::validateManualSensorCapability()\n> > > return false;\n> > > }\n> > > \n> > > + if (!staticMetadata_->hasEntry(ANDROID_SENSOR_INFO_EXPOSURE_TIME_RANGE)) {\n> > > + LOG(HAL, Info) << noMode << \"missing exposure time range\";\n> > > + return false;\n> > > + }\n> > > +\n> > > + staticMetadata_->getEntry(ANDROID_SENSOR_INFO_EXPOSURE_TIME_RANGE, &entry);\n> > > + if (entry.data.i32[0] >= 100000) {\n> > > + LOG(HAL, Info)\n> > > + << noMode\n> > > + << \"exposure time range minimum must not be equal nor larger than 100us\";\n> > > + return false;\n> > > + }\n> > > +\n> > > + if (entry.data.i32[1] <= 100000000) {\n> > > + LOG(HAL, Info)\n> > > + << noMode\n> > > + << \"exposure time range maximum must not be equal nor smaller than 100ms\";\n> > > + return false;\n> > > + }\n> > > +\n> > > /*\n> > > * \\todo Return true here after we satisfy all the requirements:\n> > > * https://developer.android.com/reference/android/hardware/camera2/CameraMetadata#REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR\n> > > @@ -1074,6 +1096,14 @@ int CameraCapabilities::initializeStaticMetadata()\n> > > exposureInfo->second.min().get() * 1000LL,\n> > > exposureInfo->second.max().get() * 1000LL,\n> > > };\n> > > +\n> > > + if (exposureTimeRange[0] >= 100000) {\n> > > + LOG(HAL, Error)\n> > > + << \"Minimum exposure time \"\n> > > + << exposureTimeRange[0]\n> > > + << \"ns is too big (should be smaller than 100us)\";\n> > > + }\n> > \n> > I read the doc (better now than never :-)) in \n> > https://developer.android.com/reference/android/hardware/camera2/CameraCharacteristics#SENSOR_INFO_EXPOSURE_TIME_RANGE\n> > \n> > Interestingly, the ExposureTimeRange is in nanoseconds. Our \n> > Controls::ExposureTime metadata is in microseconds. Should it be changed ?\n> \n> I can see some *1000LL above which I presume are converting the\n> microseconds to nanoseconds ...\n> \n> Makes me wonder if we could use std::chrono more in the android layer\n> too...\n> \n> Why do we only report an error in initializeStaticMetadata() on the\n> minimum, and not check the maximum?\n> \n> Do we need to check the minimum there at all if it's already validated\n> in validateManualSensorCapability() ?\n\nOk, I re-read the commit message. I'm not sure I understand yet what the\ndifference is but that explains the duplication.\n\n\nReviewed-by: Kieran Bingham \n\n> > Reviewed-by: Jean-Michel Hautbois \n> > \n> > > +\n> > > staticMetadata_->addEntry(ANDROID_SENSOR_INFO_EXPOSURE_TIME_RANGE,\n> > > exposureTimeRange, 2);\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 96DABBDB1C\n\tfor ;\n\tThu, 18 Nov 2021 12:04:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D74E360371;\n\tThu, 18 Nov 2021 13:04:48 +0100 (CET)","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 4F90C60231\n\tfor ;\n\tThu, 18 Nov 2021 13:04:47 +0100 (CET)","from pendragon.ideasonboard.com\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 CF899E7;\n\tThu, 18 Nov 2021 13:04:46 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"a3v0/Qpk\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637237086;\n\tbh=2pLTA9GYDl18Lwr3BZtr7LwSVV/ckDYKNOgMnecHHoU=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=a3v0/QpkTCXAFcTkUVWViorHisnSFKKzmUkVIpFAIHqkg7wHwayrDEZ0xx4tuID9I\n\tDf8Lug7XT02CVQOkX3IK5eA3Uh3QHWEUrvHxtgjA9tjHyGI4eNBKQAPRH4BgV7RxtP\n\t+/Z/p7LBGShjXcRp4pCZHqP1shAFgH5kpoAqAeq4=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<163723676290.752913.4632402298462694005@Monstersaurus>","References":"<20211118094823.344496-1-paul.elder@ideasonboard.com>\n\t<14d14215-7111-c32a-13ff-f29e03e1cac5@ideasonboard.com>\n\t<163723676290.752913.4632402298462694005@Monstersaurus>","From":"Kieran Bingham ","To":"Jean-Michel Hautbois ,\n\tPaul Elder ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 18 Nov 2021 12:04:44 +0000","Message-ID":"<163723708404.752913.16018078276989970353@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v2] android: Check exposure time range\n\tfor manual sensor capability","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":21018,"web_url":"https://patchwork.libcamera.org/comment/21018/","msgid":"<20211119090453.GH4088@pyrite.rasen.tech>","date":"2021-11-19T09:04:53","subject":"Re: [libcamera-devel] [PATCH v2] android: Check exposure time range\n\tfor manual sensor capability","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Laurent,\n\nOn Thu, Nov 18, 2021 at 01:56:47PM +0200, Laurent Pinchart wrote:\n> On Thu, Nov 18, 2021 at 12:10:58PM +0100, Jean-Michel Hautbois wrote:\n> > Hi Paul,\n> > \n> > Thanks for the patch !\n> > \n> > On 18/11/2021 10:48, Paul Elder wrote:\n> > > In the manual sensor capability validator, add a check for the exposure\n> > > time range. While at it, since the minimum exposure time is not limited\n> > > to the manual sensor capability, add a check for it when initializing\n> > > static metadata.\n> > > \n> > > Signed-off-by: Paul Elder \n> > > Reviewed-by: Laurent Pinchart \n> > > \n> > > ---\n> > > Changes in v2:\n> > > - fix comparator order (cosmetic)\n> > > - change comparators and comments to \"equal or\", as that is what is\n> > > specificied in the hal docs\n> > > - add check for minimum exposure time when initializing static metadata\n> > > - this only prints error, should we return -EINVAL instead?\n> > > ---\n> > > src/android/camera_capabilities.cpp | 30 +++++++++++++++++++++++++++++\n> > > 1 file changed, 30 insertions(+)\n> > > \n> > > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp\n> > > index f357902e..6e46f163 100644\n> > > --- a/src/android/camera_capabilities.cpp\n> > > +++ b/src/android/camera_capabilities.cpp\n> > > @@ -217,6 +217,8 @@ std::vector setMetadata(CameraMetadata *metadata, uint32_t tag,\n> > > \n> > > bool CameraCapabilities::validateManualSensorCapability()\n> > > {\n> > > +\tcamera_metadata_ro_entry_t entry;\n> > > +\n> > > \tconst char *noMode = \"Manual sensor capability unavailable: \";\n> > > \n> > > \tif (!staticMetadata_->entryContains(ANDROID_CONTROL_AE_AVAILABLE_MODES,\n> > > @@ -231,6 +233,26 @@ bool CameraCapabilities::validateManualSensorCapability()\n> > > \t\treturn false;\n> > > \t}\n> > > \n> > > +\tif (!staticMetadata_->hasEntry(ANDROID_SENSOR_INFO_EXPOSURE_TIME_RANGE)) {\n> > > +\t\tLOG(HAL, Info) << noMode << \"missing exposure time range\";\n> > > +\t\treturn false;\n> > > +\t}\n> > > +\n> > > +\tstaticMetadata_->getEntry(ANDROID_SENSOR_INFO_EXPOSURE_TIME_RANGE, &entry);\n> > > +\tif (entry.data.i32[0] >= 100000) {\n> > > +\t\tLOG(HAL, Info)\n> > > +\t\t\t<< noMode\n> > > +\t\t\t<< \"exposure time range minimum must not be equal nor larger than 100us\";\n> \n> Maybe \"must be smaller than 100us\" ?\n> \n> > > +\t\treturn false;\n> > > +\t}\n> > > +\n> > > +\tif (entry.data.i32[1] <= 100000000) {\n> > > +\t\tLOG(HAL, Info)\n> > > +\t\t\t<< noMode\n> > > +\t\t\t<< \"exposure time range maximum must not be equal nor smaller than 100ms\";\n> \n> Same here, \"must be larger than 100ms\".\n> \n> > > +\t\treturn false;\n> > > +\t}\n> > > +\n> > > \t/*\n> > > \t * \\todo Return true here after we satisfy all the requirements:\n> > > \t * https://developer.android.com/reference/android/hardware/camera2/CameraMetadata#REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR\n> > > @@ -1074,6 +1096,14 @@ int CameraCapabilities::initializeStaticMetadata()\n> > > \t\t\texposureInfo->second.min().get() * 1000LL,\n> > > \t\t\texposureInfo->second.max().get() * 1000LL,\n> > > \t\t};\n> > > +\n> > > +\t\tif (exposureTimeRange[0] >= 100000) {\n> > > +\t\t\tLOG(HAL, Error)\n> > > +\t\t\t\t<< \"Minimum exposure time \"\n> > > +\t\t\t\t<< exposureTimeRange[0]\n> > > +\t\t\t\t<< \"ns is too big (should be smaller than 100us)\";\n> \n> Do we need to handle the error somehow ?\n\nOkay, reading the docs again carefully, perhaps we have an answer.\n\n- This key (SENSOR_INFO_EXPOSURE_TIME_RANGE) is only required on FULL\n level devices\n- The minimum value for this key must always be less than 100us\n- The maximum value for this key, if hardware level is FULL, must be\n greater than 100ms\n\nSince this key is optional on non-FULL, if the minimum value is not\nsatisfied, then we'll just not report the key, and the manual sensor\nmode (and therefore FULL mode) will not be reported as available.\n\n...here comes a v3.\n\n\nPaul\n\n> \n> > > +\t\t}\n> > \n> > I read the doc (better now than never :-)) in \n> > https://developer.android.com/reference/android/hardware/camera2/CameraCharacteristics#SENSOR_INFO_EXPOSURE_TIME_RANGE\n> > \n> > Interestingly, the ExposureTimeRange is in nanoseconds. Our \n> > Controls::ExposureTime metadata is in microseconds. Should it be changed ?\n> \n> I think we should standardize all times in nanoseconds in libcamera at\n> some point, unless there's a good technical reason not to do so.\n> \n> > Reviewed-by: Jean-Michel Hautbois \n> > \n> > > +\n> > > \t\tstaticMetadata_->addEntry(ANDROID_SENSOR_INFO_EXPOSURE_TIME_RANGE,\n> > > \t\t\t\t\t exposureTimeRange, 2);\n> > > \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 4760ABDB1C\n\tfor ;\n\tFri, 19 Nov 2021 09:05:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 721AC60233;\n\tFri, 19 Nov 2021 10:05:03 +0100 (CET)","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 323046022F\n\tfor ;\n\tFri, 19 Nov 2021 10:05:02 +0100 (CET)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4C6DD5A7;\n\tFri, 19 Nov 2021 10:05:00 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Ahy5O/JP\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637312701;\n\tbh=bl9GPlkFXKRTfGhpru1u26ZycndVv9z51aX3t2dcxns=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Ahy5O/JPn90GrD9OvH/6syfAhswfnWuQf8r1kbyrhzUJ65kEKurw4IPiaoat1L5wO\n\tzhh0VyLx+Xj6yG8IZloHp6/QmZ1r3EmDrgcTNEe5eUUHR5VUGEvK66q8czAMRdVUun\n\tRKMyW+xQWYIeQPLHRxvYXxstkp0t/80/xiYW3Ook=","Date":"Fri, 19 Nov 2021 18:04:53 +0900","From":"paul.elder@ideasonboard.com","To":"Laurent Pinchart ","Message-ID":"<20211119090453.GH4088@pyrite.rasen.tech>","References":"<20211118094823.344496-1-paul.elder@ideasonboard.com>\n\t<14d14215-7111-c32a-13ff-f29e03e1cac5@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2] android: Check exposure time range\n\tfor manual sensor capability","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@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]