[{"id":18267,"web_url":"https://patchwork.libcamera.org/comment/18267/","msgid":"<20210722140259.mp7bpgmur4qnyndm@uno.localdomain>","date":"2021-07-22T14:02:59","subject":"Re: [libcamera-devel] [PATCH v6 5/9] android: Add infrastructure\n\tfor determining capabilities and hardware level","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Paul,\n\nOn Tue, Jul 20, 2021 at 07:13:03PM +0900, Paul Elder wrote:\n> Add the infrastructure for checking and reporting capabilities. Use\n> these capabilities to determine the hardware level as well.\n>\n> Bug: https://bugs.libcamera.org/show_bug.cgi?id=55\n> Signed-off-by: Paul Elder \n>\n> ---\n> I'm thinking that we should hardcode the validate* functions to return\n> false for now until we add all the required capabilities. They cannot\n> return true anyway until that happens.\n\nAs you said, they will return false anyhow, what would we gain in\nhardocding it ?\n\n>\n> Changes in v6:\n> - make all args const\n> - convert the caps list from set to vector\n>\n> Changes in v5:\n> - change the whole thing to turn on capabilities after they are all\n> confirmed, instead of disabling them as conditions are not met\n>\n> Changes in v4:\n> - rebase on camera capabilities refactoring\n> - switch to std::set from std::map\n> - make hwlevel similar to capabilities\n>\n> Changes in v3:\n> - fix some compiler errors\n> - go ahead and initialize the capabilities to true, update the commit\n> message accordingly\n>\n> Changes in v2:\n> - add a flag for FULL, since there are a few requirements that are not\n> obtained from capabilities alone\n> - add burst capture capability, since that is required for FULL as well\n> ---\n> src/android/camera_capabilities.cpp | 174 +++++++++++++++++++++++++---\n> src/android/camera_capabilities.h | 12 ++\n> 2 files changed, 171 insertions(+), 15 deletions(-)\n>\n> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp\n> index 9e2714f1..c97a17e6 100644\n> --- a/src/android/camera_capabilities.cpp\n> +++ b/src/android/camera_capabilities.cpp\n> @@ -7,8 +7,10 @@\n>\n> #include \"camera_capabilities.h\"\n>\n> +#include \n> #include \n> #include \n> +#include \n>\n> #include \n>\n> @@ -114,8 +116,153 @@ const std::map camera3FormatsMap = {\n> \t},\n> };\n>\n> +const std::map\n> +hwLevelStrings = {\n> +\t{ ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED, \"LIMITED\" },\n> +\t{ ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_FULL, \"FULL\" },\n> +\t{ ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LEGACY, \"LEGACY\" },\n> +\t{ ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_3, \"LEVEL_3\" },\n> +\t{ ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_EXTERNAL, \"EXTERNAL\" },\n> +};\n> +\n> } /* namespace */\n>\n> +bool CameraCapabilities::validateManualSensorCapability(const CameraMetadata *staticMetadata)\n> +{\n> +\tcamera_metadata_ro_entry_t entry;\n> +\tbool found;\n> +\n> +\tconst char *noMode = \"Manual sensor capability unavailable: \";\n> +\n> +\tif (!(found = staticMetadata->entryContains(ANDROID_CONTROL_AE_AVAILABLE_MODES,\n> +\t\t\t\t\t\t\t ANDROID_CONTROL_AE_MODE_OFF))) {\n> +\t\tLOG(HAL, Info)\n> +\t\t\t<< noMode << \"missing AE mode off: \"\n> +\t\t\t<< (found ? \"not supported\" : \"not found\");\n\nProbably a leftover, if found==true you won't get here, so you can\nremove the ternary operator.\n\n> +\t\treturn false;\n> +\t}\n> +\n> +\tfound = staticMetadata->getEntry(ANDROID_CONTROL_AE_LOCK_AVAILABLE,\n> +\t\t\t\t\t &entry);\n> +\tif (!found || !(*entry.data.u8 == ANDROID_CONTROL_AE_LOCK_AVAILABLE_TRUE)) {\n\nI'm sorry if I repeat the same question as in v5, but can't this just\nbe\n staticMetadata->entryContains(ANDROID_CONTROL_AE_LOCK_AVAILABLE,\n ANDROID_CONTROL_AE_LOCK_AVAILABLE_TRUE);\n\nI re-read your answer, but I don't get why this won't work. It would\ntranslate in:\n\ntemplate bool entryContains(uint32_t tag, T value) const\n{\n camera_metadata_ro_entry_t entry;\n if (!getEntry(tag, &entry))\n return false;\n\n return entryContainsOne(entry, value);\n}\n\ntemplate<>\nbool CameraMetadata::entryContainsOne(const camera_metadata_ro_entry_t &entry,\n uint8_t value) const\n{\n for (unsigned int i = 0; i < entry.count; i++) {\n if (entry.data.u8[i] == value)\n return true;\n }\n\n return false;\n}\n\n\nThe first part does what you do with 'found'\nThe specialization checks for entry.data.u8[0] to be\nLOCK_AVAILABLE_TRUE.\n\nWhy is it different than open-coding it ?\n\n\n> +\t\tLOG(HAL, Info) << noMode << \"missing AE lock\";\n\nHere the ternary operation might make sense\n\n> +\t\treturn false;\n> +\t}\n> +\n> +\treturn true;\n> +}\n> +\n> +bool CameraCapabilities::validateManualPostProcessingCapability(const CameraMetadata *staticMetadata)\n> +{\n> +\tcamera_metadata_ro_entry_t entry;\n> +\tbool found;\n> +\n> +\tconst char *noMode = \"Manual post processing capability unavailable: \";\n> +\n> +\tif (!staticMetadata->entryContains(ANDROID_CONTROL_AWB_AVAILABLE_MODES,\n> +\t\t\t\t\t\t ANDROID_CONTROL_AWB_MODE_OFF)) {\n> +\t\tLOG(HAL, Info) << noMode << \"missing AWB mode off\";\n> +\t\treturn false;\n> +\t}\n> +\n> +\tfound = staticMetadata->getEntry(ANDROID_CONTROL_AWB_LOCK_AVAILABLE,\n> +\t\t\t\t\t &entry);\n> +\tif (!found || !(*entry.data.u8 == ANDROID_CONTROL_AWB_LOCK_AVAILABLE_TRUE)) {\n> +\t\tLOG(HAL, Info) << noMode << \"missing AWB lock\";\n> +\t\treturn false;\n> +\t}\n> +\n> +\treturn true;\n> +}\n> +\n> +bool CameraCapabilities::validateBurstCaptureCapability(const CameraMetadata *staticMetadata)\n> +{\n> +\tcamera_metadata_ro_entry_t entry;\n> +\tbool found;\n> +\n> +\tconst char *noMode = \"Burst capture capability unavailable: \";\n> +\n> +\tfound = staticMetadata->getEntry(ANDROID_CONTROL_AE_LOCK_AVAILABLE,\n> +\t\t\t\t\t &entry);\n> +\tif (!found || !(*entry.data.u8 == ANDROID_CONTROL_AE_LOCK_AVAILABLE_TRUE)) {\n> +\t\tLOG(HAL, Info) << noMode << \"missing AE lock\";\n> +\t\treturn false;\n> +\t}\n> +\n> +\tfound = staticMetadata->getEntry(ANDROID_CONTROL_AWB_LOCK_AVAILABLE,\n> +\t\t\t\t\t &entry);\n> +\tif (!found || !(*entry.data.u8 == ANDROID_CONTROL_AWB_LOCK_AVAILABLE_TRUE)) {\n> +\t\tLOG(HAL, Info) << noMode << \"missing AWB lock\";\n> +\t\treturn false;\n> +\t}\n> +\n> +\tfound = staticMetadata->getEntry(ANDROID_SYNC_MAX_LATENCY, &entry);\n> +\tif (!found || *entry.data.i32 < 0 || 4 < *entry.data.i32) {\n> +\t\tLOG(HAL, Info)\n> +\t\t\t<< noMode << \"max sync latency is \"\n> +\t\t\t<< (found ? std::to_string(*entry.data.i32) : \"not present\");\n> +\t\treturn false;\n> +\t}\n> +\n> +\treturn true;\n> +}\n> +\n> +std::vector\n> +CameraCapabilities::computeCapabilities(const CameraMetadata *staticMetadata)\n> +{\n> +\tstd::vector\n> +\t\tcapabilities;\n> +\n> +\tcapabilities.push_back(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BACKWARD_COMPATIBLE);\n> +\n> +\tif (validateManualSensorCapability(staticMetadata))\n> +\t\tcapabilities.push_back(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR);\n> +\n> +\tif (validateManualPostProcessingCapability(staticMetadata))\n> +\t\tcapabilities.push_back(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_POST_PROCESSING);\n> +\n> +\tif (validateBurstCaptureCapability(staticMetadata))\n> +\t\tcapabilities.push_back(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE);\n> +\n> +\tif (rawStreamAvailable_)\n> +\t\tcapabilities.push_back(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_RAW);\n> +\n> +\treturn capabilities;\n> +}\n> +\n> +camera_metadata_enum_android_info_supported_hardware_level\n> +CameraCapabilities::computeHwLevel(const CameraMetadata *staticMetadata,\n> +\tconst std::vector &caps)\n> +{\n> +\tcamera_metadata_ro_entry_t entry;\n> +\tbool found;\n> +\tcamera_metadata_enum_android_info_supported_hardware_level\n> +\t\thwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_FULL;\n> +\n> +\tif (std::find(caps.begin(), caps.end(),\n> +\t\t ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR) == caps.end()) {\n> +\t\thwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> +\t}\n> +\n> +\tif (std::find(caps.begin(), caps.end(),\n> +\t\t ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_POST_PROCESSING) == caps.end()) {\n> +\t\thwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> +\t}\n> +\n> +\tif (std::find(caps.begin(), caps.end(),\n> +\t\t ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE) == caps.end()) {\n> +\t\thwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> +\t}\n> +\n> +\tfound = staticMetadata->getEntry(ANDROID_SYNC_MAX_LATENCY, &entry);\n> +\tif (!found || *entry.data.i32 != 0)\n> +\t\thwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> +\n> +\thwLevel_ = hwLevel;\n> +\n> +\treturn hwLevel;\n> +}\n> +\n> int CameraCapabilities::initialize(std::shared_ptr camera,\n> \t\t\t\t int orientation, int facing)\n> {\n> @@ -840,11 +987,6 @@ int CameraCapabilities::initializeStaticMetadata()\n> \tuint8_t croppingType = ANDROID_SCALER_CROPPING_TYPE_CENTER_ONLY;\n> \tstaticMetadata_->addEntry(ANDROID_SCALER_CROPPING_TYPE, croppingType);\n>\n> -\t/* Info static metadata. */\n> -\tuint8_t supportedHWLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> -\tstaticMetadata_->addEntry(ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL,\n> -\t\t\t\t supportedHWLevel);\n> -\n> \t/* Request static metadata. */\n> \tint32_t partialResultCount = 1;\n> \tstaticMetadata_->addEntry(ANDROID_REQUEST_PARTIAL_RESULT_COUNT,\n> @@ -865,21 +1007,23 @@ int CameraCapabilities::initializeStaticMetadata()\n> \tstaticMetadata_->addEntry(ANDROID_REQUEST_MAX_NUM_INPUT_STREAMS,\n> \t\t\t\t maxNumInputStreams);\n>\n> -\tstd::vector availableCapabilities = {\n> -\t\tANDROID_REQUEST_AVAILABLE_CAPABILITIES_BACKWARD_COMPATIBLE,\n> -\t};\n> -\n> -\t/* Report if camera supports RAW. */\n> -\tif (rawStreamAvailable_)\n> -\t\t\tavailableCapabilities.push_back(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_RAW);\n> -\n> \t/* Number of { RAW, YUV, JPEG } supported output streams */\n> \tint32_t numOutStreams[] = { rawStreamAvailable_, 2, 1 };\n> \tstaticMetadata_->addEntry(ANDROID_REQUEST_MAX_NUM_OUTPUT_STREAMS,\n> \t\t\t\t numOutStreams);\n>\n> -\tstaticMetadata_->addEntry(ANDROID_REQUEST_AVAILABLE_CAPABILITIES,\n> -\t\t\t\t availableCapabilities);\n> +\t/* Check capabilities */\n> +\tstd::vector\n> +\t\tcapabilities = computeCapabilities(staticMetadata_.get());\n> +\tstaticMetadata_->addEntry(ANDROID_REQUEST_AVAILABLE_CAPABILITIES, capabilities);\n> +\n> +\tcamera_metadata_enum_android_info_supported_hardware_level hwLevel =\n> +\t\tcomputeHwLevel(staticMetadata_.get(), capabilities);\n> +\tstaticMetadata_->addEntry(ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL, hwLevel);\n> +\n> +\tLOG(HAL, Info)\n> +\t\t<< \"Hardware level: \"\n> +\t\t<< hwLevelStrings.find((camera_metadata_enum_android_info_supported_hardware_level)hwLevel)->second;\n>\n> \tstd::vector availableCharacteristicsKeys = {\n> \t\tANDROID_COLOR_CORRECTION_AVAILABLE_ABERRATION_MODES,\n> diff --git a/src/android/camera_capabilities.h b/src/android/camera_capabilities.h\n> index 42a976d3..6ef81714 100644\n> --- a/src/android/camera_capabilities.h\n> +++ b/src/android/camera_capabilities.h\n> @@ -42,6 +42,17 @@ private:\n> \t\tint androidFormat;\n> \t};\n>\n> +\tbool validateManualSensorCapability(const CameraMetadata *staticMetadata);\n> +\tbool validateManualPostProcessingCapability(const CameraMetadata *staticMetadata);\n> +\tbool validateBurstCaptureCapability(const CameraMetadata *staticMetadata);\n> +\n> +\tstd::vector\n> +\t\tcomputeCapabilities(const CameraMetadata *staticMetadata);\n> +\n> +\tcamera_metadata_enum_android_info_supported_hardware_level\n> +\t\tcomputeHwLevel(const CameraMetadata *staticMetadata,\n> +\t\t\tconst std::vector &caps);\n> +\n> \tstd::vector\n> \tinitializeYUVResolutions(const libcamera::PixelFormat &pixelFormat,\n> \t\t\t\t const std::vector &resolutions);\n> @@ -56,6 +67,7 @@ private:\n> \tint facing_;\n> \tint orientation_;\n> \tbool rawStreamAvailable_;\n> +\tcamera_metadata_enum_android_info_supported_hardware_level hwLevel_;\n>\n> \tstd::vector streamConfigurations_;\n> \tstd::map formatsMap_;\n> --\n> 2.27.0\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 72C6BC322B\n\tfor ;\n\tThu, 22 Jul 2021 14:02:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EE7E768779;\n\tThu, 22 Jul 2021 16:02:14 +0200 (CEST)","from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B24EA68778\n\tfor ;\n\tThu, 22 Jul 2021 16:02:12 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay11.mail.gandi.net (Postfix) with ESMTPSA id 15FDA100003;\n\tThu, 22 Jul 2021 14:02:11 +0000 (UTC)"],"Date":"Thu, 22 Jul 2021 16:02:59 +0200","From":"Jacopo Mondi ","To":"Paul Elder ","Message-ID":"<20210722140259.mp7bpgmur4qnyndm@uno.localdomain>","References":"<20210720101307.26010-1-paul.elder@ideasonboard.com>\n\t<20210720101307.26010-6-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210720101307.26010-6-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v6 5/9] android: Add infrastructure\n\tfor determining capabilities and hardware level","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":18279,"web_url":"https://patchwork.libcamera.org/comment/18279/","msgid":"<20210723101532.GE63622@pyrite.rasen.tech>","date":"2021-07-23T10:15:32","subject":"Re: [libcamera-devel] [PATCH v6 5/9] android: Add infrastructure\n\tfor determining capabilities and hardware level","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Thu, Jul 22, 2021 at 04:02:59PM +0200, Jacopo Mondi wrote:\n> Hi Paul,\n> \n> On Tue, Jul 20, 2021 at 07:13:03PM +0900, Paul Elder wrote:\n> > Add the infrastructure for checking and reporting capabilities. Use\n> > these capabilities to determine the hardware level as well.\n> >\n> > Bug: https://bugs.libcamera.org/show_bug.cgi?id=55\n> > Signed-off-by: Paul Elder \n> >\n> > ---\n> > I'm thinking that we should hardcode the validate* functions to return\n> > false for now until we add all the required capabilities. They cannot\n> > return true anyway until that happens.\n> \n> As you said, they will return false anyhow, what would we gain in\n> hardocding it ?\n\nNot all of the checks are implemented yet, so if the ones that are here\npass then it'll return true.\n\n> \n> >\n> > Changes in v6:\n> > - make all args const\n> > - convert the caps list from set to vector\n> >\n> > Changes in v5:\n> > - change the whole thing to turn on capabilities after they are all\n> > confirmed, instead of disabling them as conditions are not met\n> >\n> > Changes in v4:\n> > - rebase on camera capabilities refactoring\n> > - switch to std::set from std::map\n> > - make hwlevel similar to capabilities\n> >\n> > Changes in v3:\n> > - fix some compiler errors\n> > - go ahead and initialize the capabilities to true, update the commit\n> > message accordingly\n> >\n> > Changes in v2:\n> > - add a flag for FULL, since there are a few requirements that are not\n> > obtained from capabilities alone\n> > - add burst capture capability, since that is required for FULL as well\n> > ---\n> > src/android/camera_capabilities.cpp | 174 +++++++++++++++++++++++++---\n> > src/android/camera_capabilities.h | 12 ++\n> > 2 files changed, 171 insertions(+), 15 deletions(-)\n> >\n> > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp\n> > index 9e2714f1..c97a17e6 100644\n> > --- a/src/android/camera_capabilities.cpp\n> > +++ b/src/android/camera_capabilities.cpp\n> > @@ -7,8 +7,10 @@\n> >\n> > #include \"camera_capabilities.h\"\n> >\n> > +#include \n> > #include \n> > #include \n> > +#include \n> >\n> > #include \n> >\n> > @@ -114,8 +116,153 @@ const std::map camera3FormatsMap = {\n> > \t},\n> > };\n> >\n> > +const std::map\n> > +hwLevelStrings = {\n> > +\t{ ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED, \"LIMITED\" },\n> > +\t{ ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_FULL, \"FULL\" },\n> > +\t{ ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LEGACY, \"LEGACY\" },\n> > +\t{ ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_3, \"LEVEL_3\" },\n> > +\t{ ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_EXTERNAL, \"EXTERNAL\" },\n> > +};\n> > +\n> > } /* namespace */\n> >\n> > +bool CameraCapabilities::validateManualSensorCapability(const CameraMetadata *staticMetadata)\n> > +{\n> > +\tcamera_metadata_ro_entry_t entry;\n> > +\tbool found;\n> > +\n> > +\tconst char *noMode = \"Manual sensor capability unavailable: \";\n> > +\n> > +\tif (!(found = staticMetadata->entryContains(ANDROID_CONTROL_AE_AVAILABLE_MODES,\n> > +\t\t\t\t\t\t\t ANDROID_CONTROL_AE_MODE_OFF))) {\n> > +\t\tLOG(HAL, Info)\n> > +\t\t\t<< noMode << \"missing AE mode off: \"\n> > +\t\t\t<< (found ? \"not supported\" : \"not found\");\n> \n> Probably a leftover, if found==true you won't get here, so you can\n> remove the ternary operator.\n\nOh yeah you're right.\n\n> \n> > +\t\treturn false;\n> > +\t}\n> > +\n> > +\tfound = staticMetadata->getEntry(ANDROID_CONTROL_AE_LOCK_AVAILABLE,\n> > +\t\t\t\t\t &entry);\n> > +\tif (!found || !(*entry.data.u8 == ANDROID_CONTROL_AE_LOCK_AVAILABLE_TRUE)) {\n> \n> I'm sorry if I repeat the same question as in v5, but can't this just\n> be\n> staticMetadata->entryContains(ANDROID_CONTROL_AE_LOCK_AVAILABLE,\n> ANDROID_CONTROL_AE_LOCK_AVAILABLE_TRUE);\n> \n> I re-read your answer, but I don't get why this won't work. It would\n> translate in:\n> \n> template bool entryContains(uint32_t tag, T value) const\n> {\n> camera_metadata_ro_entry_t entry;\n> if (!getEntry(tag, &entry))\n> return false;\n> \n> return entryContainsOne(entry, value);\n> }\n> \n> template<>\n> bool CameraMetadata::entryContainsOne(const camera_metadata_ro_entry_t &entry,\n> uint8_t value) const\n> {\n> for (unsigned int i = 0; i < entry.count; i++) {\n> if (entry.data.u8[i] == value)\n> return true;\n> }\n> \n> return false;\n> }\n> \n> \n> The first part does what you do with 'found'\n> The specialization checks for entry.data.u8[0] to be\n> LOCK_AVAILABLE_TRUE.\n> \n> Why is it different than open-coding it ?\n\nOh, I see what you mean. I guess it should work then.\n\n\nThanks,\n\nPaul\n\n> \n> \n> > +\t\tLOG(HAL, Info) << noMode << \"missing AE lock\";\n> \n> Here the ternary operation might make sense\n> \n> > +\t\treturn false;\n> > +\t}\n> > +\n> > +\treturn true;\n> > +}\n> > +\n> > +bool CameraCapabilities::validateManualPostProcessingCapability(const CameraMetadata *staticMetadata)\n> > +{\n> > +\tcamera_metadata_ro_entry_t entry;\n> > +\tbool found;\n> > +\n> > +\tconst char *noMode = \"Manual post processing capability unavailable: \";\n> > +\n> > +\tif (!staticMetadata->entryContains(ANDROID_CONTROL_AWB_AVAILABLE_MODES,\n> > +\t\t\t\t\t\t ANDROID_CONTROL_AWB_MODE_OFF)) {\n> > +\t\tLOG(HAL, Info) << noMode << \"missing AWB mode off\";\n> > +\t\treturn false;\n> > +\t}\n> > +\n> > +\tfound = staticMetadata->getEntry(ANDROID_CONTROL_AWB_LOCK_AVAILABLE,\n> > +\t\t\t\t\t &entry);\n> > +\tif (!found || !(*entry.data.u8 == ANDROID_CONTROL_AWB_LOCK_AVAILABLE_TRUE)) {\n> > +\t\tLOG(HAL, Info) << noMode << \"missing AWB lock\";\n> > +\t\treturn false;\n> > +\t}\n> > +\n> > +\treturn true;\n> > +}\n> > +\n> > +bool CameraCapabilities::validateBurstCaptureCapability(const CameraMetadata *staticMetadata)\n> > +{\n> > +\tcamera_metadata_ro_entry_t entry;\n> > +\tbool found;\n> > +\n> > +\tconst char *noMode = \"Burst capture capability unavailable: \";\n> > +\n> > +\tfound = staticMetadata->getEntry(ANDROID_CONTROL_AE_LOCK_AVAILABLE,\n> > +\t\t\t\t\t &entry);\n> > +\tif (!found || !(*entry.data.u8 == ANDROID_CONTROL_AE_LOCK_AVAILABLE_TRUE)) {\n> > +\t\tLOG(HAL, Info) << noMode << \"missing AE lock\";\n> > +\t\treturn false;\n> > +\t}\n> > +\n> > +\tfound = staticMetadata->getEntry(ANDROID_CONTROL_AWB_LOCK_AVAILABLE,\n> > +\t\t\t\t\t &entry);\n> > +\tif (!found || !(*entry.data.u8 == ANDROID_CONTROL_AWB_LOCK_AVAILABLE_TRUE)) {\n> > +\t\tLOG(HAL, Info) << noMode << \"missing AWB lock\";\n> > +\t\treturn false;\n> > +\t}\n> > +\n> > +\tfound = staticMetadata->getEntry(ANDROID_SYNC_MAX_LATENCY, &entry);\n> > +\tif (!found || *entry.data.i32 < 0 || 4 < *entry.data.i32) {\n> > +\t\tLOG(HAL, Info)\n> > +\t\t\t<< noMode << \"max sync latency is \"\n> > +\t\t\t<< (found ? std::to_string(*entry.data.i32) : \"not present\");\n> > +\t\treturn false;\n> > +\t}\n> > +\n> > +\treturn true;\n> > +}\n> > +\n> > +std::vector\n> > +CameraCapabilities::computeCapabilities(const CameraMetadata *staticMetadata)\n> > +{\n> > +\tstd::vector\n> > +\t\tcapabilities;\n> > +\n> > +\tcapabilities.push_back(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BACKWARD_COMPATIBLE);\n> > +\n> > +\tif (validateManualSensorCapability(staticMetadata))\n> > +\t\tcapabilities.push_back(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR);\n> > +\n> > +\tif (validateManualPostProcessingCapability(staticMetadata))\n> > +\t\tcapabilities.push_back(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_POST_PROCESSING);\n> > +\n> > +\tif (validateBurstCaptureCapability(staticMetadata))\n> > +\t\tcapabilities.push_back(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE);\n> > +\n> > +\tif (rawStreamAvailable_)\n> > +\t\tcapabilities.push_back(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_RAW);\n> > +\n> > +\treturn capabilities;\n> > +}\n> > +\n> > +camera_metadata_enum_android_info_supported_hardware_level\n> > +CameraCapabilities::computeHwLevel(const CameraMetadata *staticMetadata,\n> > +\tconst std::vector &caps)\n> > +{\n> > +\tcamera_metadata_ro_entry_t entry;\n> > +\tbool found;\n> > +\tcamera_metadata_enum_android_info_supported_hardware_level\n> > +\t\thwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_FULL;\n> > +\n> > +\tif (std::find(caps.begin(), caps.end(),\n> > +\t\t ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR) == caps.end()) {\n> > +\t\thwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> > +\t}\n> > +\n> > +\tif (std::find(caps.begin(), caps.end(),\n> > +\t\t ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_POST_PROCESSING) == caps.end()) {\n> > +\t\thwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> > +\t}\n> > +\n> > +\tif (std::find(caps.begin(), caps.end(),\n> > +\t\t ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE) == caps.end()) {\n> > +\t\thwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> > +\t}\n> > +\n> > +\tfound = staticMetadata->getEntry(ANDROID_SYNC_MAX_LATENCY, &entry);\n> > +\tif (!found || *entry.data.i32 != 0)\n> > +\t\thwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> > +\n> > +\thwLevel_ = hwLevel;\n> > +\n> > +\treturn hwLevel;\n> > +}\n> > +\n> > int CameraCapabilities::initialize(std::shared_ptr camera,\n> > \t\t\t\t int orientation, int facing)\n> > {\n> > @@ -840,11 +987,6 @@ int CameraCapabilities::initializeStaticMetadata()\n> > \tuint8_t croppingType = ANDROID_SCALER_CROPPING_TYPE_CENTER_ONLY;\n> > \tstaticMetadata_->addEntry(ANDROID_SCALER_CROPPING_TYPE, croppingType);\n> >\n> > -\t/* Info static metadata. */\n> > -\tuint8_t supportedHWLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> > -\tstaticMetadata_->addEntry(ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL,\n> > -\t\t\t\t supportedHWLevel);\n> > -\n> > \t/* Request static metadata. */\n> > \tint32_t partialResultCount = 1;\n> > \tstaticMetadata_->addEntry(ANDROID_REQUEST_PARTIAL_RESULT_COUNT,\n> > @@ -865,21 +1007,23 @@ int CameraCapabilities::initializeStaticMetadata()\n> > \tstaticMetadata_->addEntry(ANDROID_REQUEST_MAX_NUM_INPUT_STREAMS,\n> > \t\t\t\t maxNumInputStreams);\n> >\n> > -\tstd::vector availableCapabilities = {\n> > -\t\tANDROID_REQUEST_AVAILABLE_CAPABILITIES_BACKWARD_COMPATIBLE,\n> > -\t};\n> > -\n> > -\t/* Report if camera supports RAW. */\n> > -\tif (rawStreamAvailable_)\n> > -\t\t\tavailableCapabilities.push_back(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_RAW);\n> > -\n> > \t/* Number of { RAW, YUV, JPEG } supported output streams */\n> > \tint32_t numOutStreams[] = { rawStreamAvailable_, 2, 1 };\n> > \tstaticMetadata_->addEntry(ANDROID_REQUEST_MAX_NUM_OUTPUT_STREAMS,\n> > \t\t\t\t numOutStreams);\n> >\n> > -\tstaticMetadata_->addEntry(ANDROID_REQUEST_AVAILABLE_CAPABILITIES,\n> > -\t\t\t\t availableCapabilities);\n> > +\t/* Check capabilities */\n> > +\tstd::vector\n> > +\t\tcapabilities = computeCapabilities(staticMetadata_.get());\n> > +\tstaticMetadata_->addEntry(ANDROID_REQUEST_AVAILABLE_CAPABILITIES, capabilities);\n> > +\n> > +\tcamera_metadata_enum_android_info_supported_hardware_level hwLevel =\n> > +\t\tcomputeHwLevel(staticMetadata_.get(), capabilities);\n> > +\tstaticMetadata_->addEntry(ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL, hwLevel);\n> > +\n> > +\tLOG(HAL, Info)\n> > +\t\t<< \"Hardware level: \"\n> > +\t\t<< hwLevelStrings.find((camera_metadata_enum_android_info_supported_hardware_level)hwLevel)->second;\n> >\n> > \tstd::vector availableCharacteristicsKeys = {\n> > \t\tANDROID_COLOR_CORRECTION_AVAILABLE_ABERRATION_MODES,\n> > diff --git a/src/android/camera_capabilities.h b/src/android/camera_capabilities.h\n> > index 42a976d3..6ef81714 100644\n> > --- a/src/android/camera_capabilities.h\n> > +++ b/src/android/camera_capabilities.h\n> > @@ -42,6 +42,17 @@ private:\n> > \t\tint androidFormat;\n> > \t};\n> >\n> > +\tbool validateManualSensorCapability(const CameraMetadata *staticMetadata);\n> > +\tbool validateManualPostProcessingCapability(const CameraMetadata *staticMetadata);\n> > +\tbool validateBurstCaptureCapability(const CameraMetadata *staticMetadata);\n> > +\n> > +\tstd::vector\n> > +\t\tcomputeCapabilities(const CameraMetadata *staticMetadata);\n> > +\n> > +\tcamera_metadata_enum_android_info_supported_hardware_level\n> > +\t\tcomputeHwLevel(const CameraMetadata *staticMetadata,\n> > +\t\t\tconst std::vector &caps);\n> > +\n> > \tstd::vector\n> > \tinitializeYUVResolutions(const libcamera::PixelFormat &pixelFormat,\n> > \t\t\t\t const std::vector &resolutions);\n> > @@ -56,6 +67,7 @@ private:\n> > \tint facing_;\n> > \tint orientation_;\n> > \tbool rawStreamAvailable_;\n> > +\tcamera_metadata_enum_android_info_supported_hardware_level hwLevel_;\n> >\n> > \tstd::vector streamConfigurations_;\n> > \tstd::map formatsMap_;\n> > --\n> > 2.27.0\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 5DB62C0109\n\tfor ;\n\tFri, 23 Jul 2021 10:15:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C8BC768544;\n\tFri, 23 Jul 2021 12:15:42 +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 6451D68536\n\tfor ;\n\tFri, 23 Jul 2021 12:15:41 +0200 (CEST)","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 D6D8F3F2;\n\tFri, 23 Jul 2021 12:15:39 +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=\"euJX1HRA\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627035341;\n\tbh=pe67ePmKrr3o99PUYwnWbaI4n32QGnh84UBjjRPOGYc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=euJX1HRAwv/lAoQetQV+NlNCNgXC5x4XLcTRvbekVdHqzSs92kbSXlRza5U9DokrW\n\t4aes/BVzPjvOkV6NwRXoyJNh1awSi6I3ovz5DO7nmc76+xtem/Kkse255Ew4EQx+W8\n\tAs0yuqbLvIQc9/2Jr7/sMnidtMZYaK0dXWhVKMy4=","Date":"Fri, 23 Jul 2021 19:15:32 +0900","From":"paul.elder@ideasonboard.com","To":"Jacopo Mondi ","Message-ID":"<20210723101532.GE63622@pyrite.rasen.tech>","References":"<20210720101307.26010-1-paul.elder@ideasonboard.com>\n\t<20210720101307.26010-6-paul.elder@ideasonboard.com>\n\t<20210722140259.mp7bpgmur4qnyndm@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20210722140259.mp7bpgmur4qnyndm@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v6 5/9] android: Add infrastructure\n\tfor determining capabilities and hardware level","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":18328,"web_url":"https://patchwork.libcamera.org/comment/18328/","msgid":"","date":"2021-07-25T00:55:55","subject":"Re: [libcamera-devel] [PATCH v6 5/9] android: Add infrastructure\n\tfor determining capabilities and hardware level","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Fri, Jul 23, 2021 at 07:15:32PM +0900, paul.elder@ideasonboard.com wrote:\n> On Thu, Jul 22, 2021 at 04:02:59PM +0200, Jacopo Mondi wrote:\n> > On Tue, Jul 20, 2021 at 07:13:03PM +0900, Paul Elder wrote:\n> > > Add the infrastructure for checking and reporting capabilities. Use\n> > > these capabilities to determine the hardware level as well.\n> > >\n> > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=55\n> > > Signed-off-by: Paul Elder \n> > >\n> > > ---\n> > > I'm thinking that we should hardcode the validate* functions to return\n> > > false for now until we add all the required capabilities. They cannot\n> > > return true anyway until that happens.\n> > \n> > As you said, they will return false anyhow, what would we gain in\n> > hardocding it ?\n> \n> Not all of the checks are implemented yet, so if the ones that are here\n> pass then it'll return true.\n\nI don't think it's necessary, but I don't mind. I'd document what checks\nare missing with a comment just before the return at the end of the\nfunction though.\n\n> > > Changes in v6:\n> > > - make all args const\n> > > - convert the caps list from set to vector\n> > >\n> > > Changes in v5:\n> > > - change the whole thing to turn on capabilities after they are all\n> > > confirmed, instead of disabling them as conditions are not met\n> > >\n> > > Changes in v4:\n> > > - rebase on camera capabilities refactoring\n> > > - switch to std::set from std::map\n> > > - make hwlevel similar to capabilities\n> > >\n> > > Changes in v3:\n> > > - fix some compiler errors\n> > > - go ahead and initialize the capabilities to true, update the commit\n> > > message accordingly\n> > >\n> > > Changes in v2:\n> > > - add a flag for FULL, since there are a few requirements that are not\n> > > obtained from capabilities alone\n> > > - add burst capture capability, since that is required for FULL as well\n> > > ---\n> > > src/android/camera_capabilities.cpp | 174 +++++++++++++++++++++++++---\n> > > src/android/camera_capabilities.h | 12 ++\n> > > 2 files changed, 171 insertions(+), 15 deletions(-)\n> > >\n> > > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp\n> > > index 9e2714f1..c97a17e6 100644\n> > > --- a/src/android/camera_capabilities.cpp\n> > > +++ b/src/android/camera_capabilities.cpp\n> > > @@ -7,8 +7,10 @@\n> > >\n> > > #include \"camera_capabilities.h\"\n> > >\n> > > +#include \n> > > #include \n> > > #include \n> > > +#include \n> > >\n> > > #include \n> > >\n> > > @@ -114,8 +116,153 @@ const std::map camera3FormatsMap = {\n> > > \t},\n> > > };\n> > >\n> > > +const std::map\n> > > +hwLevelStrings = {\n> > > +\t{ ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED, \"LIMITED\" },\n> > > +\t{ ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_FULL, \"FULL\" },\n> > > +\t{ ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LEGACY, \"LEGACY\" },\n> > > +\t{ ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_3, \"LEVEL_3\" },\n> > > +\t{ ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_EXTERNAL, \"EXTERNAL\" },\n> > > +};\n> > > +\n> > > } /* namespace */\n> > >\n> > > +bool CameraCapabilities::validateManualSensorCapability(const CameraMetadata *staticMetadata)\n> > > +{\n> > > +\tcamera_metadata_ro_entry_t entry;\n> > > +\tbool found;\n> > > +\n> > > +\tconst char *noMode = \"Manual sensor capability unavailable: \";\n\nYou can repeat this below, and the compiler should de-duplicate strings\nautomatically. The code would be a bit more readable I think.\n\n> > > +\n> > > +\tif (!(found = staticMetadata->entryContains(ANDROID_CONTROL_AE_AVAILABLE_MODES,\n> > > +\t\t\t\t\t\t\t ANDROID_CONTROL_AE_MODE_OFF))) {\n> > > +\t\tLOG(HAL, Info)\n> > > +\t\t\t<< noMode << \"missing AE mode off: \"\n> > > +\t\t\t<< (found ? \"not supported\" : \"not found\");\n> > \n> > Probably a leftover, if found==true you won't get here, so you can\n> > remove the ternary operator.\n> \n> Oh yeah you're right.\n> \n> > > +\t\treturn false;\n> > > +\t}\n> > > +\n> > > +\tfound = staticMetadata->getEntry(ANDROID_CONTROL_AE_LOCK_AVAILABLE,\n> > > +\t\t\t\t\t &entry);\n> > > +\tif (!found || !(*entry.data.u8 == ANDROID_CONTROL_AE_LOCK_AVAILABLE_TRUE)) {\n> > \n> > I'm sorry if I repeat the same question as in v5, but can't this just\n> > be\n> > staticMetadata->entryContains(ANDROID_CONTROL_AE_LOCK_AVAILABLE,\n> > ANDROID_CONTROL_AE_LOCK_AVAILABLE_TRUE);\n> > \n> > I re-read your answer, but I don't get why this won't work. It would\n> > translate in:\n> > \n> > template bool entryContains(uint32_t tag, T value) const\n> > {\n> > camera_metadata_ro_entry_t entry;\n> > if (!getEntry(tag, &entry))\n> > return false;\n> > \n> > return entryContainsOne(entry, value);\n> > }\n> > \n> > template<>\n> > bool CameraMetadata::entryContainsOne(const camera_metadata_ro_entry_t &entry,\n> > uint8_t value) const\n> > {\n> > for (unsigned int i = 0; i < entry.count; i++) {\n> > if (entry.data.u8[i] == value)\n> > return true;\n> > }\n> > \n> > return false;\n> > }\n> > \n> > \n> > The first part does what you do with 'found'\n> > The specialization checks for entry.data.u8[0] to be\n> > LOCK_AVAILABLE_TRUE.\n> > \n> > Why is it different than open-coding it ?\n> \n> Oh, I see what you mean. I guess it should work then.\n> \n> > > +\t\tLOG(HAL, Info) << noMode << \"missing AE lock\";\n> > \n> > Here the ternary operation might make sense\n> > \n> > > +\t\treturn false;\n> > > +\t}\n> > > +\n> > > +\treturn true;\n> > > +}\n> > > +\n> > > +bool CameraCapabilities::validateManualPostProcessingCapability(const CameraMetadata *staticMetadata)\n> > > +{\n> > > +\tcamera_metadata_ro_entry_t entry;\n> > > +\tbool found;\n> > > +\n> > > +\tconst char *noMode = \"Manual post processing capability unavailable: \";\n> > > +\n> > > +\tif (!staticMetadata->entryContains(ANDROID_CONTROL_AWB_AVAILABLE_MODES,\n> > > +\t\t\t\t\t\t ANDROID_CONTROL_AWB_MODE_OFF)) {\n> > > +\t\tLOG(HAL, Info) << noMode << \"missing AWB mode off\";\n> > > +\t\treturn false;\n> > > +\t}\n> > > +\n> > > +\tfound = staticMetadata->getEntry(ANDROID_CONTROL_AWB_LOCK_AVAILABLE,\n> > > +\t\t\t\t\t &entry);\n> > > +\tif (!found || !(*entry.data.u8 == ANDROID_CONTROL_AWB_LOCK_AVAILABLE_TRUE)) {\n> > > +\t\tLOG(HAL, Info) << noMode << \"missing AWB lock\";\n> > > +\t\treturn false;\n> > > +\t}\n> > > +\n> > > +\treturn true;\n> > > +}\n> > > +\n> > > +bool CameraCapabilities::validateBurstCaptureCapability(const CameraMetadata *staticMetadata)\n> > > +{\n> > > +\tcamera_metadata_ro_entry_t entry;\n> > > +\tbool found;\n> > > +\n> > > +\tconst char *noMode = \"Burst capture capability unavailable: \";\n> > > +\n> > > +\tfound = staticMetadata->getEntry(ANDROID_CONTROL_AE_LOCK_AVAILABLE,\n> > > +\t\t\t\t\t &entry);\n> > > +\tif (!found || !(*entry.data.u8 == ANDROID_CONTROL_AE_LOCK_AVAILABLE_TRUE)) {\n> > > +\t\tLOG(HAL, Info) << noMode << \"missing AE lock\";\n> > > +\t\treturn false;\n> > > +\t}\n> > > +\n> > > +\tfound = staticMetadata->getEntry(ANDROID_CONTROL_AWB_LOCK_AVAILABLE,\n> > > +\t\t\t\t\t &entry);\n> > > +\tif (!found || !(*entry.data.u8 == ANDROID_CONTROL_AWB_LOCK_AVAILABLE_TRUE)) {\n> > > +\t\tLOG(HAL, Info) << noMode << \"missing AWB lock\";\n> > > +\t\treturn false;\n> > > +\t}\n> > > +\n> > > +\tfound = staticMetadata->getEntry(ANDROID_SYNC_MAX_LATENCY, &entry);\n> > > +\tif (!found || *entry.data.i32 < 0 || 4 < *entry.data.i32) {\n\nAnything wrong with *entry.data.i32 > 4 ?\n\n> > > +\t\tLOG(HAL, Info)\n> > > +\t\t\t<< noMode << \"max sync latency is \"\n> > > +\t\t\t<< (found ? std::to_string(*entry.data.i32) : \"not present\");\n> > > +\t\treturn false;\n> > > +\t}\n> > > +\n> > > +\treturn true;\n> > > +}\n> > > +\n> > > +std::vector\n> > > +CameraCapabilities::computeCapabilities(const CameraMetadata *staticMetadata)\n\nI wonder, would it make sense for this function to return a set ? It\nwould make the find operations easier, and nicer to write:\n\n\tif (std::find(caps.begin(), caps.end(),\n\t\t ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR) == caps.end()) {\n\t\thwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n\t}\n\ncould become\n\n\tif (!caps.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR))\n\t\thwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n\n(I really wish the contains() function was part of C++17, not C++20\n:-().\n\nIt would create a problem when calling CameraMetadata::addEntry()\nthough, but that's a single call, so we could convert to a vector there.\n\nPerformance-wise none of this is critical, I'd favour readability if I\nhad to pick a single criteria.\n\n> > > +{\n> > > +\tstd::vector\n> > > +\t\tcapabilities;\n> > > +\n> > > +\tcapabilities.push_back(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BACKWARD_COMPATIBLE);\n> > > +\n> > > +\tif (validateManualSensorCapability(staticMetadata))\n> > > +\t\tcapabilities.push_back(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR);\n> > > +\n> > > +\tif (validateManualPostProcessingCapability(staticMetadata))\n> > > +\t\tcapabilities.push_back(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_POST_PROCESSING);\n> > > +\n> > > +\tif (validateBurstCaptureCapability(staticMetadata))\n> > > +\t\tcapabilities.push_back(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE);\n> > > +\n> > > +\tif (rawStreamAvailable_)\n> > > +\t\tcapabilities.push_back(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_RAW);\n> > > +\n> > > +\treturn capabilities;\n> > > +}\n> > > +\n> > > +camera_metadata_enum_android_info_supported_hardware_level\n> > > +CameraCapabilities::computeHwLevel(const CameraMetadata *staticMetadata,\n> > > +\tconst std::vector &caps)\n> > > +{\n> > > +\tcamera_metadata_ro_entry_t entry;\n> > > +\tbool found;\n> > > +\tcamera_metadata_enum_android_info_supported_hardware_level\n> > > +\t\thwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_FULL;\n> > > +\n> > > +\tif (std::find(caps.begin(), caps.end(),\n> > > +\t\t ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR) == caps.end()) {\n> > > +\t\thwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> > > +\t}\n> > > +\n> > > +\tif (std::find(caps.begin(), caps.end(),\n> > > +\t\t ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_POST_PROCESSING) == caps.end()) {\n> > > +\t\thwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> > > +\t}\n> > > +\n> > > +\tif (std::find(caps.begin(), caps.end(),\n> > > +\t\t ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE) == caps.end()) {\n> > > +\t\thwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> > > +\t}\n> > > +\n> > > +\tfound = staticMetadata->getEntry(ANDROID_SYNC_MAX_LATENCY, &entry);\n> > > +\tif (!found || *entry.data.i32 != 0)\n> > > +\t\thwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> > > +\n> > > +\thwLevel_ = hwLevel;\n> > > +\n> > > +\treturn hwLevel;\n\nI'd make this a void function, and use hwLevel_ in\nCameraCapabilities::initializeStaticMetadata().\n\n> > > +}\n> > > +\n> > > int CameraCapabilities::initialize(std::shared_ptr camera,\n> > > \t\t\t\t int orientation, int facing)\n> > > {\n> > > @@ -840,11 +987,6 @@ int CameraCapabilities::initializeStaticMetadata()\n> > > \tuint8_t croppingType = ANDROID_SCALER_CROPPING_TYPE_CENTER_ONLY;\n> > > \tstaticMetadata_->addEntry(ANDROID_SCALER_CROPPING_TYPE, croppingType);\n> > >\n> > > -\t/* Info static metadata. */\n> > > -\tuint8_t supportedHWLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> > > -\tstaticMetadata_->addEntry(ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL,\n> > > -\t\t\t\t supportedHWLevel);\n> > > -\n> > > \t/* Request static metadata. */\n> > > \tint32_t partialResultCount = 1;\n> > > \tstaticMetadata_->addEntry(ANDROID_REQUEST_PARTIAL_RESULT_COUNT,\n> > > @@ -865,21 +1007,23 @@ int CameraCapabilities::initializeStaticMetadata()\n> > > \tstaticMetadata_->addEntry(ANDROID_REQUEST_MAX_NUM_INPUT_STREAMS,\n> > > \t\t\t\t maxNumInputStreams);\n> > >\n> > > -\tstd::vector availableCapabilities = {\n> > > -\t\tANDROID_REQUEST_AVAILABLE_CAPABILITIES_BACKWARD_COMPATIBLE,\n> > > -\t};\n> > > -\n> > > -\t/* Report if camera supports RAW. */\n> > > -\tif (rawStreamAvailable_)\n> > > -\t\t\tavailableCapabilities.push_back(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_RAW);\n> > > -\n> > > \t/* Number of { RAW, YUV, JPEG } supported output streams */\n> > > \tint32_t numOutStreams[] = { rawStreamAvailable_, 2, 1 };\n> > > \tstaticMetadata_->addEntry(ANDROID_REQUEST_MAX_NUM_OUTPUT_STREAMS,\n> > > \t\t\t\t numOutStreams);\n> > >\n> > > -\tstaticMetadata_->addEntry(ANDROID_REQUEST_AVAILABLE_CAPABILITIES,\n> > > -\t\t\t\t availableCapabilities);\n> > > +\t/* Check capabilities */\n> > > +\tstd::vector\n> > > +\t\tcapabilities = computeCapabilities(staticMetadata_.get());\n> > > +\tstaticMetadata_->addEntry(ANDROID_REQUEST_AVAILABLE_CAPABILITIES, capabilities);\n> > > +\n> > > +\tcamera_metadata_enum_android_info_supported_hardware_level hwLevel =\n> > > +\t\tcomputeHwLevel(staticMetadata_.get(), capabilities);\n> > > +\tstaticMetadata_->addEntry(ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL, hwLevel);\n> > > +\n> > > +\tLOG(HAL, Info)\n> > > +\t\t<< \"Hardware level: \"\n> > > +\t\t<< hwLevelStrings.find((camera_metadata_enum_android_info_supported_hardware_level)hwLevel)->second;\n\nCan't you skip the cast, as hwLevel is already a\ncamera_metadata_enum_android_info_supported_hardware_level ? (What a\nhorrible type name...)\n\n> > >\n> > > \tstd::vector availableCharacteristicsKeys = {\n> > > \t\tANDROID_COLOR_CORRECTION_AVAILABLE_ABERRATION_MODES,\n> > > diff --git a/src/android/camera_capabilities.h b/src/android/camera_capabilities.h\n> > > index 42a976d3..6ef81714 100644\n> > > --- a/src/android/camera_capabilities.h\n> > > +++ b/src/android/camera_capabilities.h\n> > > @@ -42,6 +42,17 @@ private:\n> > > \t\tint androidFormat;\n> > > \t};\n> > >\n> > > +\tbool validateManualSensorCapability(const CameraMetadata *staticMetadata);\n> > > +\tbool validateManualPostProcessingCapability(const CameraMetadata *staticMetadata);\n> > > +\tbool validateBurstCaptureCapability(const CameraMetadata *staticMetadata);\n> > > +\n> > > +\tstd::vector\n> > > +\t\tcomputeCapabilities(const CameraMetadata *staticMetadata);\n> > > +\n> > > +\tcamera_metadata_enum_android_info_supported_hardware_level\n> > > +\t\tcomputeHwLevel(const CameraMetadata *staticMetadata,\n> > > +\t\t\tconst std::vector &caps);\n\nAs all these are non-static member functions, do you need to pass the\nstaticMetadata argument ? It seems you could use staticMetadata_\ninternally.\n\n> > > +\n> > > \tstd::vector\n> > > \tinitializeYUVResolutions(const libcamera::PixelFormat &pixelFormat,\n> > > \t\t\t\t const std::vector &resolutions);\n> > > @@ -56,6 +67,7 @@ private:\n> > > \tint facing_;\n> > > \tint orientation_;\n> > > \tbool rawStreamAvailable_;\n> > > +\tcamera_metadata_enum_android_info_supported_hardware_level hwLevel_;\n> > >\n> > > \tstd::vector streamConfigurations_;\n> > > \tstd::map formatsMap_;","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 02B44C322C\n\tfor ;\n\tSun, 25 Jul 2021 00:56:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6F3B9687B5;\n\tSun, 25 Jul 2021 02:56:01 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2B0E260274\n\tfor ;\n\tSun, 25 Jul 2021 02:55:59 +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 89745255;\n\tSun, 25 Jul 2021 02:55:58 +0200 (CEST)"],"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=\"mV+ceYCF\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627174558;\n\tbh=jRlLJs4g+KvZq1YM/5qWSiYoRKnkc4qLw7+qTYl2uh8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=mV+ceYCFMxzyniDcsfhbeNGpa1EQMdYMWaIv9r+ElXB5Nyl7pLBFQWvNvN4HHkjDW\n\tLzrW9YU9AHU+orBZZkjXZMirM1NSZ9uJdAtzu64byVLtlm8Nbs4cBMIUKfokgzsY77\n\tpU1idwOoEl/dgSGrniw054/0p2lF5EHG4lcvJdOg=","Date":"Sun, 25 Jul 2021 03:55:55 +0300","From":"Laurent Pinchart ","To":"paul.elder@ideasonboard.com","Message-ID":"","References":"<20210720101307.26010-1-paul.elder@ideasonboard.com>\n\t<20210720101307.26010-6-paul.elder@ideasonboard.com>\n\t<20210722140259.mp7bpgmur4qnyndm@uno.localdomain>\n\t<20210723101532.GE63622@pyrite.rasen.tech>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210723101532.GE63622@pyrite.rasen.tech>","Subject":"Re: [libcamera-devel] [PATCH v6 5/9] android: Add infrastructure\n\tfor determining capabilities and hardware level","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":18443,"web_url":"https://patchwork.libcamera.org/comment/18443/","msgid":"<20210730093518.GM2167@pyrite.rasen.tech>","date":"2021-07-30T09:35:18","subject":"Re: [libcamera-devel] [PATCH v6 5/9] android: Add infrastructure\n\tfor determining capabilities and hardware level","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Laurent,\n\nThank you for the review.\n\nOn Sun, Jul 25, 2021 at 03:55:55AM +0300, Laurent Pinchart wrote:\n> Hi Paul,\n> \n> Thank you for the patch.\n> \n> On Fri, Jul 23, 2021 at 07:15:32PM +0900, paul.elder@ideasonboard.com wrote:\n> > On Thu, Jul 22, 2021 at 04:02:59PM +0200, Jacopo Mondi wrote:\n> > > On Tue, Jul 20, 2021 at 07:13:03PM +0900, Paul Elder wrote:\n> > > > Add the infrastructure for checking and reporting capabilities. Use\n> > > > these capabilities to determine the hardware level as well.\n> > > >\n> > > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=55\n> > > > Signed-off-by: Paul Elder \n> > > >\n> > > > ---\n> > > > I'm thinking that we should hardcode the validate* functions to return\n> > > > false for now until we add all the required capabilities. They cannot\n> > > > return true anyway until that happens.\n> > > \n> > > As you said, they will return false anyhow, what would we gain in\n> > > hardocding it ?\n> > \n> > Not all of the checks are implemented yet, so if the ones that are here\n> > pass then it'll return true.\n> \n> I don't think it's necessary, but I don't mind. I'd document what checks\n> are missing with a comment just before the return at the end of the\n> function though.\n> \n> > > > Changes in v6:\n> > > > - make all args const\n> > > > - convert the caps list from set to vector\n> > > >\n> > > > Changes in v5:\n> > > > - change the whole thing to turn on capabilities after they are all\n> > > > confirmed, instead of disabling them as conditions are not met\n> > > >\n> > > > Changes in v4:\n> > > > - rebase on camera capabilities refactoring\n> > > > - switch to std::set from std::map\n> > > > - make hwlevel similar to capabilities\n> > > >\n> > > > Changes in v3:\n> > > > - fix some compiler errors\n> > > > - go ahead and initialize the capabilities to true, update the commit\n> > > > message accordingly\n> > > >\n> > > > Changes in v2:\n> > > > - add a flag for FULL, since there are a few requirements that are not\n> > > > obtained from capabilities alone\n> > > > - add burst capture capability, since that is required for FULL as well\n> > > > ---\n> > > > src/android/camera_capabilities.cpp | 174 +++++++++++++++++++++++++---\n> > > > src/android/camera_capabilities.h | 12 ++\n> > > > 2 files changed, 171 insertions(+), 15 deletions(-)\n> > > >\n> > > > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp\n> > > > index 9e2714f1..c97a17e6 100644\n> > > > --- a/src/android/camera_capabilities.cpp\n> > > > +++ b/src/android/camera_capabilities.cpp\n> > > > @@ -7,8 +7,10 @@\n> > > >\n> > > > #include \"camera_capabilities.h\"\n> > > >\n> > > > +#include \n> > > > #include \n> > > > #include \n> > > > +#include \n> > > >\n> > > > #include \n> > > >\n> > > > @@ -114,8 +116,153 @@ const std::map camera3FormatsMap = {\n> > > > \t},\n> > > > };\n> > > >\n> > > > +const std::map\n> > > > +hwLevelStrings = {\n> > > > +\t{ ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED, \"LIMITED\" },\n> > > > +\t{ ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_FULL, \"FULL\" },\n> > > > +\t{ ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LEGACY, \"LEGACY\" },\n> > > > +\t{ ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_3, \"LEVEL_3\" },\n> > > > +\t{ ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_EXTERNAL, \"EXTERNAL\" },\n> > > > +};\n> > > > +\n> > > > } /* namespace */\n> > > >\n> > > > +bool CameraCapabilities::validateManualSensorCapability(const CameraMetadata *staticMetadata)\n> > > > +{\n> > > > +\tcamera_metadata_ro_entry_t entry;\n> > > > +\tbool found;\n> > > > +\n> > > > +\tconst char *noMode = \"Manual sensor capability unavailable: \";\n> \n> You can repeat this below, and the compiler should de-duplicate strings\n> automatically. The code would be a bit more readable I think.\n\nI think this way is more readable. Otherwise we have the exact same\nstring copy and pasted multiple times in here, making lines longer, and\nif we change it then it's more changes.\n\n> \n> > > > +\n> > > > +\tif (!(found = staticMetadata->entryContains(ANDROID_CONTROL_AE_AVAILABLE_MODES,\n> > > > +\t\t\t\t\t\t\t ANDROID_CONTROL_AE_MODE_OFF))) {\n> > > > +\t\tLOG(HAL, Info)\n> > > > +\t\t\t<< noMode << \"missing AE mode off: \"\n> > > > +\t\t\t<< (found ? \"not supported\" : \"not found\");\n> > > \n> > > Probably a leftover, if found==true you won't get here, so you can\n> > > remove the ternary operator.\n> > \n> > Oh yeah you're right.\n> > \n> > > > +\t\treturn false;\n> > > > +\t}\n> > > > +\n> > > > +\tfound = staticMetadata->getEntry(ANDROID_CONTROL_AE_LOCK_AVAILABLE,\n> > > > +\t\t\t\t\t &entry);\n> > > > +\tif (!found || !(*entry.data.u8 == ANDROID_CONTROL_AE_LOCK_AVAILABLE_TRUE)) {\n> > > \n> > > I'm sorry if I repeat the same question as in v5, but can't this just\n> > > be\n> > > staticMetadata->entryContains(ANDROID_CONTROL_AE_LOCK_AVAILABLE,\n> > > ANDROID_CONTROL_AE_LOCK_AVAILABLE_TRUE);\n> > > \n> > > I re-read your answer, but I don't get why this won't work. It would\n> > > translate in:\n> > > \n> > > template bool entryContains(uint32_t tag, T value) const\n> > > {\n> > > camera_metadata_ro_entry_t entry;\n> > > if (!getEntry(tag, &entry))\n> > > return false;\n> > > \n> > > return entryContainsOne(entry, value);\n> > > }\n> > > \n> > > template<>\n> > > bool CameraMetadata::entryContainsOne(const camera_metadata_ro_entry_t &entry,\n> > > uint8_t value) const\n> > > {\n> > > for (unsigned int i = 0; i < entry.count; i++) {\n> > > if (entry.data.u8[i] == value)\n> > > return true;\n> > > }\n> > > \n> > > return false;\n> > > }\n> > > \n> > > \n> > > The first part does what you do with 'found'\n> > > The specialization checks for entry.data.u8[0] to be\n> > > LOCK_AVAILABLE_TRUE.\n> > > \n> > > Why is it different than open-coding it ?\n> > \n> > Oh, I see what you mean. I guess it should work then.\n> > \n> > > > +\t\tLOG(HAL, Info) << noMode << \"missing AE lock\";\n> > > \n> > > Here the ternary operation might make sense\n> > > \n> > > > +\t\treturn false;\n> > > > +\t}\n> > > > +\n> > > > +\treturn true;\n> > > > +}\n> > > > +\n> > > > +bool CameraCapabilities::validateManualPostProcessingCapability(const CameraMetadata *staticMetadata)\n> > > > +{\n> > > > +\tcamera_metadata_ro_entry_t entry;\n> > > > +\tbool found;\n> > > > +\n> > > > +\tconst char *noMode = \"Manual post processing capability unavailable: \";\n> > > > +\n> > > > +\tif (!staticMetadata->entryContains(ANDROID_CONTROL_AWB_AVAILABLE_MODES,\n> > > > +\t\t\t\t\t\t ANDROID_CONTROL_AWB_MODE_OFF)) {\n> > > > +\t\tLOG(HAL, Info) << noMode << \"missing AWB mode off\";\n> > > > +\t\treturn false;\n> > > > +\t}\n> > > > +\n> > > > +\tfound = staticMetadata->getEntry(ANDROID_CONTROL_AWB_LOCK_AVAILABLE,\n> > > > +\t\t\t\t\t &entry);\n> > > > +\tif (!found || !(*entry.data.u8 == ANDROID_CONTROL_AWB_LOCK_AVAILABLE_TRUE)) {\n> > > > +\t\tLOG(HAL, Info) << noMode << \"missing AWB lock\";\n> > > > +\t\treturn false;\n> > > > +\t}\n> > > > +\n> > > > +\treturn true;\n> > > > +}\n> > > > +\n> > > > +bool CameraCapabilities::validateBurstCaptureCapability(const CameraMetadata *staticMetadata)\n> > > > +{\n> > > > +\tcamera_metadata_ro_entry_t entry;\n> > > > +\tbool found;\n> > > > +\n> > > > +\tconst char *noMode = \"Burst capture capability unavailable: \";\n> > > > +\n> > > > +\tfound = staticMetadata->getEntry(ANDROID_CONTROL_AE_LOCK_AVAILABLE,\n> > > > +\t\t\t\t\t &entry);\n> > > > +\tif (!found || !(*entry.data.u8 == ANDROID_CONTROL_AE_LOCK_AVAILABLE_TRUE)) {\n> > > > +\t\tLOG(HAL, Info) << noMode << \"missing AE lock\";\n> > > > +\t\treturn false;\n> > > > +\t}\n> > > > +\n> > > > +\tfound = staticMetadata->getEntry(ANDROID_CONTROL_AWB_LOCK_AVAILABLE,\n> > > > +\t\t\t\t\t &entry);\n> > > > +\tif (!found || !(*entry.data.u8 == ANDROID_CONTROL_AWB_LOCK_AVAILABLE_TRUE)) {\n> > > > +\t\tLOG(HAL, Info) << noMode << \"missing AWB lock\";\n> > > > +\t\treturn false;\n> > > > +\t}\n> > > > +\n> > > > +\tfound = staticMetadata->getEntry(ANDROID_SYNC_MAX_LATENCY, &entry);\n> > > > +\tif (!found || *entry.data.i32 < 0 || 4 < *entry.data.i32) {\n> \n> Anything wrong with *entry.data.i32 > 4 ?\n\nI think it's better to line up the conditions in order of small ->\nlarge, since it's the same order that we read things. This way it's\nclear that the condition is )0,4(.\n\n> \n> > > > +\t\tLOG(HAL, Info)\n> > > > +\t\t\t<< noMode << \"max sync latency is \"\n> > > > +\t\t\t<< (found ? std::to_string(*entry.data.i32) : \"not present\");\n> > > > +\t\treturn false;\n> > > > +\t}\n> > > > +\n> > > > +\treturn true;\n> > > > +}\n> > > > +\n> > > > +std::vector\n> > > > +CameraCapabilities::computeCapabilities(const CameraMetadata *staticMetadata)\n> \n> I wonder, would it make sense for this function to return a set ? It\n\nThat's what I had first but Jacopo said \"would it make sense for this\nfunction to return a vector?\" :D\n\n> would make the find operations easier, and nicer to write:\n> \n> \tif (std::find(caps.begin(), caps.end(),\n> \t\t ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR) == caps.end()) {\n> \t\thwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> \t}\n> \n> could become\n> \n> \tif (!caps.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR))\n> \t\thwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n\nThis is why I thought set would be better.\n\n> \n> (I really wish the contains() function was part of C++17, not C++20\n> :-().\n> \n> It would create a problem when calling CameraMetadata::addEntry()\n> though, but that's a single call, so we could convert to a vector there.\n\nAnd this is why Jacopo said that vector would be better.\n\n> \n> Performance-wise none of this is critical, I'd favour readability if I\n> had to pick a single criteria.\n> \n> > > > +{\n> > > > +\tstd::vector\n> > > > +\t\tcapabilities;\n> > > > +\n> > > > +\tcapabilities.push_back(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BACKWARD_COMPATIBLE);\n> > > > +\n> > > > +\tif (validateManualSensorCapability(staticMetadata))\n> > > > +\t\tcapabilities.push_back(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR);\n> > > > +\n> > > > +\tif (validateManualPostProcessingCapability(staticMetadata))\n> > > > +\t\tcapabilities.push_back(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_POST_PROCESSING);\n> > > > +\n> > > > +\tif (validateBurstCaptureCapability(staticMetadata))\n> > > > +\t\tcapabilities.push_back(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE);\n> > > > +\n> > > > +\tif (rawStreamAvailable_)\n> > > > +\t\tcapabilities.push_back(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_RAW);\n> > > > +\n> > > > +\treturn capabilities;\n> > > > +}\n> > > > +\n> > > > +camera_metadata_enum_android_info_supported_hardware_level\n> > > > +CameraCapabilities::computeHwLevel(const CameraMetadata *staticMetadata,\n> > > > +\tconst std::vector &caps)\n> > > > +{\n> > > > +\tcamera_metadata_ro_entry_t entry;\n> > > > +\tbool found;\n> > > > +\tcamera_metadata_enum_android_info_supported_hardware_level\n> > > > +\t\thwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_FULL;\n> > > > +\n> > > > +\tif (std::find(caps.begin(), caps.end(),\n> > > > +\t\t ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR) == caps.end()) {\n> > > > +\t\thwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> > > > +\t}\n> > > > +\n> > > > +\tif (std::find(caps.begin(), caps.end(),\n> > > > +\t\t ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_POST_PROCESSING) == caps.end()) {\n> > > > +\t\thwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> > > > +\t}\n> > > > +\n> > > > +\tif (std::find(caps.begin(), caps.end(),\n> > > > +\t\t ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE) == caps.end()) {\n> > > > +\t\thwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> > > > +\t}\n> > > > +\n> > > > +\tfound = staticMetadata->getEntry(ANDROID_SYNC_MAX_LATENCY, &entry);\n> > > > +\tif (!found || *entry.data.i32 != 0)\n> > > > +\t\thwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> > > > +\n> > > > +\thwLevel_ = hwLevel;\n> > > > +\n> > > > +\treturn hwLevel;\n> \n> I'd make this a void function, and use hwLevel_ in\n> CameraCapabilities::initializeStaticMetadata().\n\nAh okay.\n\n> \n> > > > +}\n> > > > +\n> > > > int CameraCapabilities::initialize(std::shared_ptr camera,\n> > > > \t\t\t\t int orientation, int facing)\n> > > > {\n> > > > @@ -840,11 +987,6 @@ int CameraCapabilities::initializeStaticMetadata()\n> > > > \tuint8_t croppingType = ANDROID_SCALER_CROPPING_TYPE_CENTER_ONLY;\n> > > > \tstaticMetadata_->addEntry(ANDROID_SCALER_CROPPING_TYPE, croppingType);\n> > > >\n> > > > -\t/* Info static metadata. */\n> > > > -\tuint8_t supportedHWLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> > > > -\tstaticMetadata_->addEntry(ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL,\n> > > > -\t\t\t\t supportedHWLevel);\n> > > > -\n> > > > \t/* Request static metadata. */\n> > > > \tint32_t partialResultCount = 1;\n> > > > \tstaticMetadata_->addEntry(ANDROID_REQUEST_PARTIAL_RESULT_COUNT,\n> > > > @@ -865,21 +1007,23 @@ int CameraCapabilities::initializeStaticMetadata()\n> > > > \tstaticMetadata_->addEntry(ANDROID_REQUEST_MAX_NUM_INPUT_STREAMS,\n> > > > \t\t\t\t maxNumInputStreams);\n> > > >\n> > > > -\tstd::vector availableCapabilities = {\n> > > > -\t\tANDROID_REQUEST_AVAILABLE_CAPABILITIES_BACKWARD_COMPATIBLE,\n> > > > -\t};\n> > > > -\n> > > > -\t/* Report if camera supports RAW. */\n> > > > -\tif (rawStreamAvailable_)\n> > > > -\t\t\tavailableCapabilities.push_back(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_RAW);\n> > > > -\n> > > > \t/* Number of { RAW, YUV, JPEG } supported output streams */\n> > > > \tint32_t numOutStreams[] = { rawStreamAvailable_, 2, 1 };\n> > > > \tstaticMetadata_->addEntry(ANDROID_REQUEST_MAX_NUM_OUTPUT_STREAMS,\n> > > > \t\t\t\t numOutStreams);\n> > > >\n> > > > -\tstaticMetadata_->addEntry(ANDROID_REQUEST_AVAILABLE_CAPABILITIES,\n> > > > -\t\t\t\t availableCapabilities);\n> > > > +\t/* Check capabilities */\n> > > > +\tstd::vector\n> > > > +\t\tcapabilities = computeCapabilities(staticMetadata_.get());\n> > > > +\tstaticMetadata_->addEntry(ANDROID_REQUEST_AVAILABLE_CAPABILITIES, capabilities);\n> > > > +\n> > > > +\tcamera_metadata_enum_android_info_supported_hardware_level hwLevel =\n> > > > +\t\tcomputeHwLevel(staticMetadata_.get(), capabilities);\n> > > > +\tstaticMetadata_->addEntry(ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL, hwLevel);\n> > > > +\n> > > > +\tLOG(HAL, Info)\n> > > > +\t\t<< \"Hardware level: \"\n> > > > +\t\t<< hwLevelStrings.find((camera_metadata_enum_android_info_supported_hardware_level)hwLevel)->second;\n> \n> Can't you skip the cast, as hwLevel is already a\n> camera_metadata_enum_android_info_supported_hardware_level ? (What a\n> horrible type name...)\n\nOh maybe this was leftover from when I had int or something.\n\n> \n> > > >\n> > > > \tstd::vector availableCharacteristicsKeys = {\n> > > > \t\tANDROID_COLOR_CORRECTION_AVAILABLE_ABERRATION_MODES,\n> > > > diff --git a/src/android/camera_capabilities.h b/src/android/camera_capabilities.h\n> > > > index 42a976d3..6ef81714 100644\n> > > > --- a/src/android/camera_capabilities.h\n> > > > +++ b/src/android/camera_capabilities.h\n> > > > @@ -42,6 +42,17 @@ private:\n> > > > \t\tint androidFormat;\n> > > > \t};\n> > > >\n> > > > +\tbool validateManualSensorCapability(const CameraMetadata *staticMetadata);\n> > > > +\tbool validateManualPostProcessingCapability(const CameraMetadata *staticMetadata);\n> > > > +\tbool validateBurstCaptureCapability(const CameraMetadata *staticMetadata);\n> > > > +\n> > > > +\tstd::vector\n> > > > +\t\tcomputeCapabilities(const CameraMetadata *staticMetadata);\n> > > > +\n> > > > +\tcamera_metadata_enum_android_info_supported_hardware_level\n> > > > +\t\tcomputeHwLevel(const CameraMetadata *staticMetadata,\n> > > > +\t\t\tconst std::vector &caps);\n> \n> As all these are non-static member functions, do you need to pass the\n> staticMetadata argument ? It seems you could use staticMetadata_\n> internally.\n\nI guess I could.\n\n\nThanks,\n\nPaul\n\n> \n> > > > +\n> > > > \tstd::vector\n> > > > \tinitializeYUVResolutions(const libcamera::PixelFormat &pixelFormat,\n> > > > \t\t\t\t const std::vector &resolutions);\n> > > > @@ -56,6 +67,7 @@ private:\n> > > > \tint facing_;\n> > > > \tint orientation_;\n> > > > \tbool rawStreamAvailable_;\n> > > > +\tcamera_metadata_enum_android_info_supported_hardware_level hwLevel_;\n> > > >\n> > > > \tstd::vector streamConfigurations_;\n> > > > \tstd::map formatsMap_;","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 F1930C3230\n\tfor ;\n\tFri, 30 Jul 2021 09:35:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4E6EA687C5;\n\tFri, 30 Jul 2021 11:35:28 +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 BA30A687B1\n\tfor ;\n\tFri, 30 Jul 2021 11:35:26 +0200 (CEST)","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 B39A09FB;\n\tFri, 30 Jul 2021 11:35:24 +0200 (CEST)"],"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=\"qdtjCWdX\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627637726;\n\tbh=E0ol8A1U5E2TI1bqHaSCXLRbYlnJABQrWOsPCkgG8dI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=qdtjCWdXwFTgWEMCAPghp+FRcY5PdZK9EXly7kXKjLxYZD7eU5gfEsvLAAXNqks6U\n\twlBZm2VdpqU1WM+7EAF9P89NIaiUNbIf7SDUhPCmvWYF9az8Qx/rGd3UIgsffK9ldg\n\t7BikGOAgbqhVfk1et5OjC2zzOBpCsvjMIDO1oEhU=","Date":"Fri, 30 Jul 2021 18:35:18 +0900","From":"paul.elder@ideasonboard.com","To":"Laurent Pinchart ","Message-ID":"<20210730093518.GM2167@pyrite.rasen.tech>","References":"<20210720101307.26010-1-paul.elder@ideasonboard.com>\n\t<20210720101307.26010-6-paul.elder@ideasonboard.com>\n\t<20210722140259.mp7bpgmur4qnyndm@uno.localdomain>\n\t<20210723101532.GE63622@pyrite.rasen.tech>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v6 5/9] android: Add infrastructure\n\tfor determining capabilities and hardware level","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":18448,"web_url":"https://patchwork.libcamera.org/comment/18448/","msgid":"","date":"2021-07-31T02:18:08","subject":"Re: [libcamera-devel] [PATCH v6 5/9] android: Add infrastructure\n\tfor determining capabilities and hardware level","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nOn Fri, Jul 30, 2021 at 06:35:18PM +0900, paul.elder@ideasonboard.com wrote:\n> On Sun, Jul 25, 2021 at 03:55:55AM +0300, Laurent Pinchart wrote:\n> > On Fri, Jul 23, 2021 at 07:15:32PM +0900, paul.elder@ideasonboard.com wrote:\n> > > On Thu, Jul 22, 2021 at 04:02:59PM +0200, Jacopo Mondi wrote:\n> > > > On Tue, Jul 20, 2021 at 07:13:03PM +0900, Paul Elder wrote:\n> > > > > Add the infrastructure for checking and reporting capabilities. Use\n> > > > > these capabilities to determine the hardware level as well.\n> > > > >\n> > > > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=55\n> > > > > Signed-off-by: Paul Elder \n> > > > >\n> > > > > ---\n> > > > > I'm thinking that we should hardcode the validate* functions to return\n> > > > > false for now until we add all the required capabilities. They cannot\n> > > > > return true anyway until that happens.\n> > > > \n> > > > As you said, they will return false anyhow, what would we gain in\n> > > > hardocding it ?\n> > > \n> > > Not all of the checks are implemented yet, so if the ones that are here\n> > > pass then it'll return true.\n> > \n> > I don't think it's necessary, but I don't mind. I'd document what checks\n> > are missing with a comment just before the return at the end of the\n> > function though.\n> > \n> > > > > Changes in v6:\n> > > > > - make all args const\n> > > > > - convert the caps list from set to vector\n> > > > >\n> > > > > Changes in v5:\n> > > > > - change the whole thing to turn on capabilities after they are all\n> > > > > confirmed, instead of disabling them as conditions are not met\n> > > > >\n> > > > > Changes in v4:\n> > > > > - rebase on camera capabilities refactoring\n> > > > > - switch to std::set from std::map\n> > > > > - make hwlevel similar to capabilities\n> > > > >\n> > > > > Changes in v3:\n> > > > > - fix some compiler errors\n> > > > > - go ahead and initialize the capabilities to true, update the commit\n> > > > > message accordingly\n> > > > >\n> > > > > Changes in v2:\n> > > > > - add a flag for FULL, since there are a few requirements that are not\n> > > > > obtained from capabilities alone\n> > > > > - add burst capture capability, since that is required for FULL as well\n> > > > > ---\n> > > > > src/android/camera_capabilities.cpp | 174 +++++++++++++++++++++++++---\n> > > > > src/android/camera_capabilities.h | 12 ++\n> > > > > 2 files changed, 171 insertions(+), 15 deletions(-)\n> > > > >\n> > > > > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp\n> > > > > index 9e2714f1..c97a17e6 100644\n> > > > > --- a/src/android/camera_capabilities.cpp\n> > > > > +++ b/src/android/camera_capabilities.cpp\n> > > > > @@ -7,8 +7,10 @@\n> > > > >\n> > > > > #include \"camera_capabilities.h\"\n> > > > >\n> > > > > +#include \n> > > > > #include \n> > > > > #include \n> > > > > +#include \n> > > > >\n> > > > > #include \n> > > > >\n> > > > > @@ -114,8 +116,153 @@ const std::map camera3FormatsMap = {\n> > > > > \t},\n> > > > > };\n> > > > >\n> > > > > +const std::map\n> > > > > +hwLevelStrings = {\n> > > > > +\t{ ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED, \"LIMITED\" },\n> > > > > +\t{ ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_FULL, \"FULL\" },\n> > > > > +\t{ ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LEGACY, \"LEGACY\" },\n> > > > > +\t{ ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_3, \"LEVEL_3\" },\n> > > > > +\t{ ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_EXTERNAL, \"EXTERNAL\" },\n> > > > > +};\n> > > > > +\n> > > > > } /* namespace */\n> > > > >\n> > > > > +bool CameraCapabilities::validateManualSensorCapability(const CameraMetadata *staticMetadata)\n> > > > > +{\n> > > > > +\tcamera_metadata_ro_entry_t entry;\n> > > > > +\tbool found;\n> > > > > +\n> > > > > +\tconst char *noMode = \"Manual sensor capability unavailable: \";\n> > \n> > You can repeat this below, and the compiler should de-duplicate strings\n> > automatically. The code would be a bit more readable I think.\n> \n> I think this way is more readable. Otherwise we have the exact same\n> string copy and pasted multiple times in here, making lines longer, and\n> if we change it then it's more changes.\n> \n> > > > > +\n> > > > > +\tif (!(found = staticMetadata->entryContains(ANDROID_CONTROL_AE_AVAILABLE_MODES,\n> > > > > +\t\t\t\t\t\t\t ANDROID_CONTROL_AE_MODE_OFF))) {\n> > > > > +\t\tLOG(HAL, Info)\n> > > > > +\t\t\t<< noMode << \"missing AE mode off: \"\n> > > > > +\t\t\t<< (found ? \"not supported\" : \"not found\");\n> > > > \n> > > > Probably a leftover, if found==true you won't get here, so you can\n> > > > remove the ternary operator.\n> > > \n> > > Oh yeah you're right.\n> > > \n> > > > > +\t\treturn false;\n> > > > > +\t}\n> > > > > +\n> > > > > +\tfound = staticMetadata->getEntry(ANDROID_CONTROL_AE_LOCK_AVAILABLE,\n> > > > > +\t\t\t\t\t &entry);\n> > > > > +\tif (!found || !(*entry.data.u8 == ANDROID_CONTROL_AE_LOCK_AVAILABLE_TRUE)) {\n> > > > \n> > > > I'm sorry if I repeat the same question as in v5, but can't this just\n> > > > be\n> > > > staticMetadata->entryContains(ANDROID_CONTROL_AE_LOCK_AVAILABLE,\n> > > > ANDROID_CONTROL_AE_LOCK_AVAILABLE_TRUE);\n> > > > \n> > > > I re-read your answer, but I don't get why this won't work. It would\n> > > > translate in:\n> > > > \n> > > > template bool entryContains(uint32_t tag, T value) const\n> > > > {\n> > > > camera_metadata_ro_entry_t entry;\n> > > > if (!getEntry(tag, &entry))\n> > > > return false;\n> > > > \n> > > > return entryContainsOne(entry, value);\n> > > > }\n> > > > \n> > > > template<>\n> > > > bool CameraMetadata::entryContainsOne(const camera_metadata_ro_entry_t &entry,\n> > > > uint8_t value) const\n> > > > {\n> > > > for (unsigned int i = 0; i < entry.count; i++) {\n> > > > if (entry.data.u8[i] == value)\n> > > > return true;\n> > > > }\n> > > > \n> > > > return false;\n> > > > }\n> > > > \n> > > > \n> > > > The first part does what you do with 'found'\n> > > > The specialization checks for entry.data.u8[0] to be\n> > > > LOCK_AVAILABLE_TRUE.\n> > > > \n> > > > Why is it different than open-coding it ?\n> > > \n> > > Oh, I see what you mean. I guess it should work then.\n> > > \n> > > > > +\t\tLOG(HAL, Info) << noMode << \"missing AE lock\";\n> > > > \n> > > > Here the ternary operation might make sense\n> > > > \n> > > > > +\t\treturn false;\n> > > > > +\t}\n> > > > > +\n> > > > > +\treturn true;\n> > > > > +}\n> > > > > +\n> > > > > +bool CameraCapabilities::validateManualPostProcessingCapability(const CameraMetadata *staticMetadata)\n> > > > > +{\n> > > > > +\tcamera_metadata_ro_entry_t entry;\n> > > > > +\tbool found;\n> > > > > +\n> > > > > +\tconst char *noMode = \"Manual post processing capability unavailable: \";\n> > > > > +\n> > > > > +\tif (!staticMetadata->entryContains(ANDROID_CONTROL_AWB_AVAILABLE_MODES,\n> > > > > +\t\t\t\t\t\t ANDROID_CONTROL_AWB_MODE_OFF)) {\n> > > > > +\t\tLOG(HAL, Info) << noMode << \"missing AWB mode off\";\n> > > > > +\t\treturn false;\n> > > > > +\t}\n> > > > > +\n> > > > > +\tfound = staticMetadata->getEntry(ANDROID_CONTROL_AWB_LOCK_AVAILABLE,\n> > > > > +\t\t\t\t\t &entry);\n> > > > > +\tif (!found || !(*entry.data.u8 == ANDROID_CONTROL_AWB_LOCK_AVAILABLE_TRUE)) {\n> > > > > +\t\tLOG(HAL, Info) << noMode << \"missing AWB lock\";\n> > > > > +\t\treturn false;\n> > > > > +\t}\n> > > > > +\n> > > > > +\treturn true;\n> > > > > +}\n> > > > > +\n> > > > > +bool CameraCapabilities::validateBurstCaptureCapability(const CameraMetadata *staticMetadata)\n> > > > > +{\n> > > > > +\tcamera_metadata_ro_entry_t entry;\n> > > > > +\tbool found;\n> > > > > +\n> > > > > +\tconst char *noMode = \"Burst capture capability unavailable: \";\n> > > > > +\n> > > > > +\tfound = staticMetadata->getEntry(ANDROID_CONTROL_AE_LOCK_AVAILABLE,\n> > > > > +\t\t\t\t\t &entry);\n> > > > > +\tif (!found || !(*entry.data.u8 == ANDROID_CONTROL_AE_LOCK_AVAILABLE_TRUE)) {\n> > > > > +\t\tLOG(HAL, Info) << noMode << \"missing AE lock\";\n> > > > > +\t\treturn false;\n> > > > > +\t}\n> > > > > +\n> > > > > +\tfound = staticMetadata->getEntry(ANDROID_CONTROL_AWB_LOCK_AVAILABLE,\n> > > > > +\t\t\t\t\t &entry);\n> > > > > +\tif (!found || !(*entry.data.u8 == ANDROID_CONTROL_AWB_LOCK_AVAILABLE_TRUE)) {\n> > > > > +\t\tLOG(HAL, Info) << noMode << \"missing AWB lock\";\n> > > > > +\t\treturn false;\n> > > > > +\t}\n> > > > > +\n> > > > > +\tfound = staticMetadata->getEntry(ANDROID_SYNC_MAX_LATENCY, &entry);\n> > > > > +\tif (!found || *entry.data.i32 < 0 || 4 < *entry.data.i32) {\n> > \n> > Anything wrong with *entry.data.i32 > 4 ?\n> \n> I think it's better to line up the conditions in order of small ->\n> large, since it's the same order that we read things. This way it's\n> clear that the condition is )0,4(.\n\nDo you say, in English, \"if the value is smaller than zero or larger\nthan four\", or \"if the value is smaller than zero or four is smaller\nthan the value\" ? :-) \"4 < *entry.data.i32\" requires me to pause and\nreally focus on the code to read it.\n\n> > > > > +\t\tLOG(HAL, Info)\n> > > > > +\t\t\t<< noMode << \"max sync latency is \"\n> > > > > +\t\t\t<< (found ? std::to_string(*entry.data.i32) : \"not present\");\n> > > > > +\t\treturn false;\n> > > > > +\t}\n> > > > > +\n> > > > > +\treturn true;\n> > > > > +}\n> > > > > +\n> > > > > +std::vector\n> > > > > +CameraCapabilities::computeCapabilities(const CameraMetadata *staticMetadata)\n> > \n> > I wonder, would it make sense for this function to return a set ? It\n> \n> That's what I had first but Jacopo said \"would it make sense for this\n> function to return a vector?\" :D\n> \n> > would make the find operations easier, and nicer to write:\n> > \n> > \tif (std::find(caps.begin(), caps.end(),\n> > \t\t ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR) == caps.end()) {\n> > \t\thwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> > \t}\n> > \n> > could become\n> > \n> > \tif (!caps.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR))\n> > \t\thwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> \n> This is why I thought set would be better.\n> \n> > (I really wish the contains() function was part of C++17, not C++20\n> > :-().\n> > \n> > It would create a problem when calling CameraMetadata::addEntry()\n> > though, but that's a single call, so we could convert to a vector there.\n> \n> And this is why Jacopo said that vector would be better.\n\nPros and cons. I find the ability to write caps.count() worth the need\nto convert to a vector, but that's just me. You can pick one :-)\n\n> > Performance-wise none of this is critical, I'd favour readability if I\n> > had to pick a single criteria.\n> > \n> > > > > +{\n> > > > > +\tstd::vector\n> > > > > +\t\tcapabilities;\n> > > > > +\n> > > > > +\tcapabilities.push_back(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BACKWARD_COMPATIBLE);\n> > > > > +\n> > > > > +\tif (validateManualSensorCapability(staticMetadata))\n> > > > > +\t\tcapabilities.push_back(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR);\n> > > > > +\n> > > > > +\tif (validateManualPostProcessingCapability(staticMetadata))\n> > > > > +\t\tcapabilities.push_back(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_POST_PROCESSING);\n> > > > > +\n> > > > > +\tif (validateBurstCaptureCapability(staticMetadata))\n> > > > > +\t\tcapabilities.push_back(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE);\n> > > > > +\n> > > > > +\tif (rawStreamAvailable_)\n> > > > > +\t\tcapabilities.push_back(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_RAW);\n> > > > > +\n> > > > > +\treturn capabilities;\n> > > > > +}\n> > > > > +\n> > > > > +camera_metadata_enum_android_info_supported_hardware_level\n> > > > > +CameraCapabilities::computeHwLevel(const CameraMetadata *staticMetadata,\n> > > > > +\tconst std::vector &caps)\n> > > > > +{\n> > > > > +\tcamera_metadata_ro_entry_t entry;\n> > > > > +\tbool found;\n> > > > > +\tcamera_metadata_enum_android_info_supported_hardware_level\n> > > > > +\t\thwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_FULL;\n> > > > > +\n> > > > > +\tif (std::find(caps.begin(), caps.end(),\n> > > > > +\t\t ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR) == caps.end()) {\n> > > > > +\t\thwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> > > > > +\t}\n> > > > > +\n> > > > > +\tif (std::find(caps.begin(), caps.end(),\n> > > > > +\t\t ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_POST_PROCESSING) == caps.end()) {\n> > > > > +\t\thwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> > > > > +\t}\n> > > > > +\n> > > > > +\tif (std::find(caps.begin(), caps.end(),\n> > > > > +\t\t ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE) == caps.end()) {\n> > > > > +\t\thwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> > > > > +\t}\n> > > > > +\n> > > > > +\tfound = staticMetadata->getEntry(ANDROID_SYNC_MAX_LATENCY, &entry);\n> > > > > +\tif (!found || *entry.data.i32 != 0)\n> > > > > +\t\thwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> > > > > +\n> > > > > +\thwLevel_ = hwLevel;\n> > > > > +\n> > > > > +\treturn hwLevel;\n> > \n> > I'd make this a void function, and use hwLevel_ in\n> > CameraCapabilities::initializeStaticMetadata().\n> \n> Ah okay.\n> \n> > > > > +}\n> > > > > +\n> > > > > int CameraCapabilities::initialize(std::shared_ptr camera,\n> > > > > \t\t\t\t int orientation, int facing)\n> > > > > {\n> > > > > @@ -840,11 +987,6 @@ int CameraCapabilities::initializeStaticMetadata()\n> > > > > \tuint8_t croppingType = ANDROID_SCALER_CROPPING_TYPE_CENTER_ONLY;\n> > > > > \tstaticMetadata_->addEntry(ANDROID_SCALER_CROPPING_TYPE, croppingType);\n> > > > >\n> > > > > -\t/* Info static metadata. */\n> > > > > -\tuint8_t supportedHWLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> > > > > -\tstaticMetadata_->addEntry(ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL,\n> > > > > -\t\t\t\t supportedHWLevel);\n> > > > > -\n> > > > > \t/* Request static metadata. */\n> > > > > \tint32_t partialResultCount = 1;\n> > > > > \tstaticMetadata_->addEntry(ANDROID_REQUEST_PARTIAL_RESULT_COUNT,\n> > > > > @@ -865,21 +1007,23 @@ int CameraCapabilities::initializeStaticMetadata()\n> > > > > \tstaticMetadata_->addEntry(ANDROID_REQUEST_MAX_NUM_INPUT_STREAMS,\n> > > > > \t\t\t\t maxNumInputStreams);\n> > > > >\n> > > > > -\tstd::vector availableCapabilities = {\n> > > > > -\t\tANDROID_REQUEST_AVAILABLE_CAPABILITIES_BACKWARD_COMPATIBLE,\n> > > > > -\t};\n> > > > > -\n> > > > > -\t/* Report if camera supports RAW. */\n> > > > > -\tif (rawStreamAvailable_)\n> > > > > -\t\t\tavailableCapabilities.push_back(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_RAW);\n> > > > > -\n> > > > > \t/* Number of { RAW, YUV, JPEG } supported output streams */\n> > > > > \tint32_t numOutStreams[] = { rawStreamAvailable_, 2, 1 };\n> > > > > \tstaticMetadata_->addEntry(ANDROID_REQUEST_MAX_NUM_OUTPUT_STREAMS,\n> > > > > \t\t\t\t numOutStreams);\n> > > > >\n> > > > > -\tstaticMetadata_->addEntry(ANDROID_REQUEST_AVAILABLE_CAPABILITIES,\n> > > > > -\t\t\t\t availableCapabilities);\n> > > > > +\t/* Check capabilities */\n> > > > > +\tstd::vector\n> > > > > +\t\tcapabilities = computeCapabilities(staticMetadata_.get());\n> > > > > +\tstaticMetadata_->addEntry(ANDROID_REQUEST_AVAILABLE_CAPABILITIES, capabilities);\n> > > > > +\n> > > > > +\tcamera_metadata_enum_android_info_supported_hardware_level hwLevel =\n> > > > > +\t\tcomputeHwLevel(staticMetadata_.get(), capabilities);\n> > > > > +\tstaticMetadata_->addEntry(ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL, hwLevel);\n> > > > > +\n> > > > > +\tLOG(HAL, Info)\n> > > > > +\t\t<< \"Hardware level: \"\n> > > > > +\t\t<< hwLevelStrings.find((camera_metadata_enum_android_info_supported_hardware_level)hwLevel)->second;\n> > \n> > Can't you skip the cast, as hwLevel is already a\n> > camera_metadata_enum_android_info_supported_hardware_level ? (What a\n> > horrible type name...)\n> \n> Oh maybe this was leftover from when I had int or something.\n> \n> > > > >\n> > > > > \tstd::vector availableCharacteristicsKeys = {\n> > > > > \t\tANDROID_COLOR_CORRECTION_AVAILABLE_ABERRATION_MODES,\n> > > > > diff --git a/src/android/camera_capabilities.h b/src/android/camera_capabilities.h\n> > > > > index 42a976d3..6ef81714 100644\n> > > > > --- a/src/android/camera_capabilities.h\n> > > > > +++ b/src/android/camera_capabilities.h\n> > > > > @@ -42,6 +42,17 @@ private:\n> > > > > \t\tint androidFormat;\n> > > > > \t};\n> > > > >\n> > > > > +\tbool validateManualSensorCapability(const CameraMetadata *staticMetadata);\n> > > > > +\tbool validateManualPostProcessingCapability(const CameraMetadata *staticMetadata);\n> > > > > +\tbool validateBurstCaptureCapability(const CameraMetadata *staticMetadata);\n> > > > > +\n> > > > > +\tstd::vector\n> > > > > +\t\tcomputeCapabilities(const CameraMetadata *staticMetadata);\n> > > > > +\n> > > > > +\tcamera_metadata_enum_android_info_supported_hardware_level\n> > > > > +\t\tcomputeHwLevel(const CameraMetadata *staticMetadata,\n> > > > > +\t\t\tconst std::vector &caps);\n> > \n> > As all these are non-static member functions, do you need to pass the\n> > staticMetadata argument ? It seems you could use staticMetadata_\n> > internally.\n> \n> I guess I could.\n> \n> > > > > +\n> > > > > \tstd::vector\n> > > > > \tinitializeYUVResolutions(const libcamera::PixelFormat &pixelFormat,\n> > > > > \t\t\t\t const std::vector &resolutions);\n> > > > > @@ -56,6 +67,7 @@ private:\n> > > > > \tint facing_;\n> > > > > \tint orientation_;\n> > > > > \tbool rawStreamAvailable_;\n> > > > > +\tcamera_metadata_enum_android_info_supported_hardware_level hwLevel_;\n> > > > >\n> > > > > \tstd::vector streamConfigurations_;\n> > > > > \tstd::map formatsMap_;","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 B26CFC3232\n\tfor ;\n\tSat, 31 Jul 2021 02:18:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2C412687C5;\n\tSat, 31 Jul 2021 04:18:19 +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 BE035687BA\n\tfor ;\n\tSat, 31 Jul 2021 04:18:17 +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 1D2852A3;\n\tSat, 31 Jul 2021 04:18:17 +0200 (CEST)"],"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=\"LIJxWqe5\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627697897;\n\tbh=6r78fNvkYzUuBJ/ZRHXMjeLZbzik/XVCqtYm6HBMhRw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=LIJxWqe5GadTCfCQm/HF9lLUjlXWqRvwLYRt5/13ZOyurJIHIJFrfbwhzh0B4W9R5\n\tBQJ2QhXvRtO/ML7ighz5Mf4ozxG/JANf+G2755ow/knQ3Gid0UNXK95FkAjmArOe3m\n\tPNpHrcbP3dsR2qzuFhhxSo+5Bwv7D3dhez3SoTBk=","Date":"Sat, 31 Jul 2021 05:18:08 +0300","From":"Laurent Pinchart ","To":"paul.elder@ideasonboard.com","Message-ID":"","References":"<20210720101307.26010-1-paul.elder@ideasonboard.com>\n\t<20210720101307.26010-6-paul.elder@ideasonboard.com>\n\t<20210722140259.mp7bpgmur4qnyndm@uno.localdomain>\n\t<20210723101532.GE63622@pyrite.rasen.tech>\n\t\n\t<20210730093518.GM2167@pyrite.rasen.tech>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210730093518.GM2167@pyrite.rasen.tech>","Subject":"Re: [libcamera-devel] [PATCH v6 5/9] android: Add infrastructure\n\tfor determining capabilities and hardware level","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\" "}}]