[{"id":18043,"web_url":"https://patchwork.libcamera.org/comment/18043/","msgid":"<20210709113446.GC5937@pyrite.rasen.tech>","date":"2021-07-09T11:34:46","subject":"Re: [libcamera-devel] [RFC PATCH] wip: android: capabilities:\n\tCapability detection by population","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hello everyone,\n\nOn Fri, Jul 09, 2021 at 08:28:14PM +0900, Paul Elder wrote:\n> Implement capability and hardware level detection based on the static\n> metadata that has been set, instead of disabling them as requirements\n> are not met. This results in cleaner code where the static metadata is\n> set.\n> \n> Signed-off-by: Paul Elder \n> \n> ---\n> This currently does not work as the ControlInfo constructor for Spans is\n> broken. This is more of an RFC for Jacopo to check that this is the\n> direction that he wants.\n> \n> Clearly this is not meant to be merged, as the actual capability\n> requirements are not fully specified yet. Plus they belong in separate\n> patches anyway.\n> \n> This patch does not apply either, as it depends on many unreleased\n> patches.\n> ---\n> src/android/camera_capabilities.cpp | 219 ++++++++++++++++++++--------\n> 1 file changed, 157 insertions(+), 62 deletions(-)\n> \n> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp\n> index 83c7f0d0..ceb5cfe8 100644\n> --- a/src/android/camera_capabilities.cpp\n> +++ b/src/android/camera_capabilities.cpp\n> @@ -262,6 +262,149 @@ std::vector setMetadata(CameraMetadata *metadata, uint32_t tag,\n> \treturn ret;\n> }\n> \n> +\n> +template\n> +bool metadataContains(camera_metadata_ro_entry_t &entry, T value);\n> +\n> +template<>\n> +bool metadataContains(camera_metadata_ro_entry_t &entry, uint8_t value)\n> +{\n> +\tfor (unsigned int i = 0; i < entry.count; i++)\n> +\t\tif (entry.data.u8[i] == value)\n> +\t\t\treturn true;\n> +\n> +\treturn false;\n> +}\n> +\n> +bool validateManualSensorCapability(CameraMetadata *staticMetadata)\n> +{\n> +\tcamera_metadata_ro_entry_t entry;\n> +\tbool found;\n> +\n> +\tfound = staticMetadata->getEntry(ANDROID_CONTROL_AE_AVAILABLE_MODES,\n> +\t\t\t\t\t &entry);\n> +\tif (!found || !metadataContains(entry, ANDROID_CONTROL_AE_MODE_OFF)) {\n> +\t\tLOG(HAL, Info)\n> +\t\t\t<< \"Missing AE mode off: \"\n> +\t\t\t<< (found ? \"not supported\" : \"not found\");\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> +\t\tLOG(HAL, Info) << \"Missing AE lock\";\n> +\t\treturn false;\n> +\t}\n> +\n> +\tfound = staticMetadata->getEntry(ANDROID_EDGE_AVAILABLE_EDGE_MODES,\n> +\t\t\t\t\t &entry);\n> +\tif (!found) {\n> +\t\tLOG(HAL, Info) << \"Missing edge modes\";\n> +\t\treturn false;\n> +\t}\n> +\n> +\treturn true;\n> +}\n> +\n> +bool validateManualPostProcessingCapability(CameraMetadata *staticMetadata)\n> +{\n> +\tcamera_metadata_ro_entry_t entry;\n> +\tbool found;\n> +\n> +\tfound = staticMetadata->getEntry(ANDROID_CONTROL_AWB_AVAILABLE_MODES,\n> +\t\t\t\t\t &entry);\n> +\tif (!found || !metadataContains(entry, ANDROID_CONTROL_AWB_MODE_OFF)) {\n> +\t\tLOG(HAL, Info) << \"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) << \"Missing AWB lock\";\n> +\t\treturn false;\n> +\t}\n> +\n> +\treturn true;\n> +}\n> +\n> +bool validateBurstCaptureCapability(CameraMetadata *staticMetadata)\n> +{\n> +\tcamera_metadata_ro_entry_t entry;\n> +\tbool found;\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) << \"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) << \"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<< \"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::set\n> +computeCapabilities(CameraMetadata *staticMetadata,\n> +\t\t std::set &existingCaps)\n> +{\n> +\tstd::set\n> +\tcapabilities = existingCaps;\n> +\n> +\tcapabilities.insert(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BACKWARD_COMPATIBLE);\n> +\n> +\tif (validateManualSensorCapability(staticMetadata))\n> +\t\tcapabilities.insert(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR);\n> +\n> +\tif (validateManualPostProcessingCapability(staticMetadata))\n> +\t\tcapabilities.insert(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_POST_PROCESSING);\n> +\n> +\tif (validateBurstCaptureCapability(staticMetadata))\n> +\t\tcapabilities.insert(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE);\n> +\n> +\treturn capabilities;\n> +}\n> +\n> +camera_metadata_enum_android_info_supported_hardware_level\n> +computeHwLevel(CameraMetadata *staticMetadata,\n> +\t std::set capabilities)\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 (!capabilities.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR))\n> +\t\thwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> +\n> +\tif (!capabilities.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_POST_PROCESSING))\n> +\t\thwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> +\n> +\tif (!capabilities.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE))\n> +\t\thwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\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> +\treturn hwLevel;\n> +}\n> +\n> } /* namespace */\n> \n> int CameraCapabilities::initialize(std::shared_ptr camera,\n> @@ -655,17 +798,7 @@ int CameraCapabilities::initializeStaticMetadata()\n> \t};\n> \n> \tstd::set\n> -\tcapabilities = {\n> -\t\tANDROID_REQUEST_AVAILABLE_CAPABILITIES_BACKWARD_COMPATIBLE,\n> -\t\tANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR,\n> -\t\tANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_POST_PROCESSING,\n> -\t\tANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE,\n> -\t};\n> -\n> -\tstd::set\n> -\thwLevels = {\n> -\t\tANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_FULL,\n> -\t};\n> +\tcapabilities = {};\n> \n> \t/* Color correction static metadata. */\n> \t{\n> @@ -692,19 +825,12 @@ int CameraCapabilities::initializeStaticMetadata()\n> \tstaticMetadata_->addEntry(ANDROID_CONTROL_AE_AVAILABLE_ANTIBANDING_MODES,\n> \t\t\t\t aeAvailableAntiBandingModes);\n> \n> -\tstd::vector aeModes = setMetadata(\n> +\tsetMetadata(\n> \t\tstaticMetadata_.get(),\n> \t\tANDROID_CONTROL_AE_AVAILABLE_MODES,\n> \t\tcontrolsInfo, &controls::AeEnable,\n> \t\t{ ANDROID_CONTROL_AE_MODE_ON });\n> \n> -\tif (std::find(aeModes.begin(), aeModes.end(),\n> -\t\t ANDROID_CONTROL_AE_MODE_OFF) == aeModes.end()) {\n> -\t\tLOG(HAL, Info) << \"AE cannot be turned off\";\n> -\t\thwLevels.erase(ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_FULL);\n> -\t\tcapabilities.erase(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR);\n> -\t}\n> -\n> \tint64_t minFrameDurationNsec = -1;\n> \tint64_t maxFrameDurationNsec = -1;\n> \tconst auto frameDurationsInfo = controlsInfo.find(&controls::FrameDurationLimits);\n> @@ -791,17 +917,11 @@ int CameraCapabilities::initializeStaticMetadata()\n> \tstaticMetadata_->addEntry(ANDROID_CONTROL_AVAILABLE_VIDEO_STABILIZATION_MODES,\n> \t\t\t\t availableStabilizationModes);\n> \n> -\tstd::vector awbModes = setMetadata(\n> +\tsetMetadata(\n> \t\tstaticMetadata_.get(),\n> \t\tANDROID_CONTROL_AWB_AVAILABLE_MODES,\n> \t\tcontrolsInfo, &controls::AwbMode,\n> \t\t{ ANDROID_CONTROL_AWB_MODE_AUTO });\n> -\tif (std::find(awbModes.begin(), awbModes.end(),\n> -\t\t ANDROID_CONTROL_AWB_MODE_OFF) == awbModes.end()) {\n> -\t\tLOG(HAL, Info) << \"AWB cannot be turned off\";\n> -\t\thwLevels.erase(ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_FULL);\n> -\t\tcapabilities.erase(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_POST_PROCESSING);\n> -\t}\n> \n> \tstd::vector availableMaxRegions = {\n> \t\t0, 0, 0,\n> @@ -817,29 +937,19 @@ int CameraCapabilities::initializeStaticMetadata()\n> \tstaticMetadata_->addEntry(ANDROID_CONTROL_SCENE_MODE_OVERRIDES,\n> \t\t\t\t sceneModesOverride);\n> \n> -\tuint8_t aeLockAvailable = setMetadata(\n> +\tsetMetadata(\n> \t\tstaticMetadata_.get(),\n> \t\tANDROID_CONTROL_AE_LOCK_AVAILABLE,\n> \t\tcontrolsInfo, &controls::AeLock,\n> \t\tControlRange::Max,\n> \t\tANDROID_CONTROL_AE_LOCK_AVAILABLE_FALSE);\n> -\tif (aeLockAvailable != ANDROID_CONTROL_AE_LOCK_AVAILABLE_TRUE) {\n> -\t\tLOG(HAL, Info) << \"AE lock is unavailable\";\n> -\t\tcapabilities.erase(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE);\n> -\t\tcapabilities.erase(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR);\n> -\t}\n> \n> -\tuint8_t awbLockAvailable = setMetadata(\n> +\tsetMetadata(\n> \t\tstaticMetadata_.get(),\n> \t\tANDROID_CONTROL_AWB_LOCK_AVAILABLE,\n> \t\tcontrolsInfo, &controls::AwbLock,\n> \t\tControlRange::Max,\n> \t\tANDROID_CONTROL_AWB_LOCK_AVAILABLE_FALSE);\n> -\tif (awbLockAvailable != ANDROID_CONTROL_AWB_LOCK_AVAILABLE_TRUE) {\n> -\t\tLOG(HAL, Info) << \"AWB lock is unavailable\";\n> -\t\tcapabilities.erase(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE);\n> -\t\tcapabilities.erase(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_POST_PROCESSING);\n> -\t}\n> \n> \tchar availableControlModes = ANDROID_CONTROL_MODE_AUTO;\n> \tstaticMetadata_->addEntry(ANDROID_CONTROL_AVAILABLE_MODES,\n> @@ -859,10 +969,6 @@ int CameraCapabilities::initializeStaticMetadata()\n> \t\tavailableCharacteristicsKeys_.insert(ANDROID_EDGE_AVAILABLE_EDGE_MODES);\n> \t\tavailableRequestKeys_.insert(ANDROID_EDGE_MODE);\n> \t\tavailableResultKeys_.insert(ANDROID_EDGE_MODE);\n> -\t} else {\n> -\t\tLOG(HAL, Info) << \"Edge mode unavailable\";\n> -\t\thwLevels.erase(ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_FULL);\n> -\t\tcapabilities.erase(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR);\n> \t}\n> \n> \t/* JPEG static metadata. */\n> @@ -1046,17 +1152,11 @@ int CameraCapabilities::initializeStaticMetadata()\n> \t}\n> \n> \t/* Sync static metadata. */\n> -\tint32_t maxLatency = setMetadata(\n> +\tsetMetadata(\n> \t\tstaticMetadata_.get(), ANDROID_SYNC_MAX_LATENCY,\n> \t\tcontrolsInfo, &controls::draft::MaxLatency,\n> \t\tControlRange::Def,\n> \t\tANDROID_SYNC_MAX_LATENCY_UNKNOWN);\n> -\tLOG(HAL, Info) << \"Max sync latency is \" << maxLatency;\n> -\t/* CTS allows a sync latency of up to 4 for burst capture capability */\n> -\tif (maxLatency < 0 || 4 < maxLatency)\n> -\t\tcapabilities.erase(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE);\n> -\tif (maxLatency != 0)\n> -\t\thwLevels.erase(ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_FULL);\n> \n> \t/* Flash static metadata. */\n> \tchar flashAvailable = ANDROID_FLASH_INFO_AVAILABLE_FALSE;\n> @@ -1220,19 +1320,14 @@ int CameraCapabilities::initializeStaticMetadata()\n> \t\t\t\t numOutStreams);\n> \n> \t/* Check capabilities */\n> -\tstd::vector availableCapabilities(capabilities.begin(),\n> -\t\t\t\t\t\t capabilities.end());\n> -\tstaticMetadata_->addEntry(ANDROID_REQUEST_AVAILABLE_CAPABILITIES,\n> -\t\t\t\t availableCapabilities);\n> -\n> -\tuint8_t hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> -\tif (capabilities.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR) &&\n> -\t capabilities.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_POST_PROCESSING) &&\n> -\t capabilities.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE) &&\n> -\t hwLevels.count(ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_FULL))\n> -\t\thwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_FULL;\n> -\tstaticMetadata_->addEntry(ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL,\n> -\t\t\t\t hwLevel);\n> +\tcapabilities = computeCapabilities(staticMetadata_.get(), capabilities);\n\nJust to clarify, this is here because above this we have:\n\n\t/* Report if camera supports RAW. */\n\tbool rawStreamAvailable = false;\n\tstd::unique_ptr cameraConfig =\n\t\tcamera_->generateConfiguration({ StreamRole::Raw });\n\tif (cameraConfig && !cameraConfig->empty()) {\n\t\tconst PixelFormatInfo &info =\n\t\t\tPixelFormatInfo::info(cameraConfig->at(0).pixelFormat);\n\t\t/* Only advertise RAW support if RAW16 is possible. */\n\t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW &&\n\t\t info.bitsPerPixel == 16) {\n\t\t\trawStreamAvailable = true;\n\t\t\tcapabilities.insert(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_RAW);\n\t\t}\n\t}\n\nThere's no way to capture this information in the static metadata\notherwise, and I don't think it's a good idea to feed arbitrary\nvariables into the capabilities gatherer, since at that point we might\nas well keep the method that I had earlier (which has the benefit of not\nhaving to walk the static metadata again after setting it, but hey, a\nmultiple of linear time is still linear time :D).\n\nSo yeah, we allow static metadata setters to directly set the\ncapabilities if it alone is sufficient and necessary to determine some\ncapability.\n\n\nPaul\n\n> +\tstd::vector\n> +\t\tcapsVec = std::vector(capabilities.begin(), capabilities.end());\n> +\tstaticMetadata_->addEntry(ANDROID_REQUEST_AVAILABLE_CAPABILITIES, capsVec);\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> -- \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 8F66DBD794\n\tfor ;\n\tFri, 9 Jul 2021 11:34:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0F2C9684E7;\n\tFri, 9 Jul 2021 13:34:56 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4233A605AC\n\tfor ;\n\tFri, 9 Jul 2021 13:34:55 +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 A7BFEE7;\n\tFri, 9 Jul 2021 13:34:53 +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=\"s725vkpz\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1625830494;\n\tbh=mpzW8pUSwAqEyCfogD6XBRNBSGy9/2bnrPet9a+lbM4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=s725vkpzhQEKP3VlNliBTQoOcfiqw6q2hx/Hq1mHNIK+u8K2cAwAtUL1AeltkU2CU\n\tQ7Utyyk2IjaSVeiyUonBwaCpypranAUlLFlqs7D23CblRAWf5v86C9kv3M2LbrKHyZ\n\tdbNgoFlBgXZfQyShz2bSz6Ye9jmKLbCRzocMX4qk=","Date":"Fri, 9 Jul 2021 20:34:46 +0900","From":"paul.elder@ideasonboard.com","To":"libcamera-devel@lists.libcamera.org","Message-ID":"<20210709113446.GC5937@pyrite.rasen.tech>","References":"<20210709112814.87917-1-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20210709112814.87917-1-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC PATCH] wip: android: capabilities:\n\tCapability detection by population","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":18085,"web_url":"https://patchwork.libcamera.org/comment/18085/","msgid":"","date":"2021-07-11T20:50:44","subject":"Re: [libcamera-devel] [RFC PATCH] wip: android: capabilities:\n\tCapability detection by population","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 09, 2021 at 08:34:46PM +0900, paul.elder@ideasonboard.com wrote:\n> On Fri, Jul 09, 2021 at 08:28:14PM +0900, Paul Elder wrote:\n> > Implement capability and hardware level detection based on the static\n> > metadata that has been set, instead of disabling them as requirements\n> > are not met. This results in cleaner code where the static metadata is\n> > set.\n> > \n> > Signed-off-by: Paul Elder \n> > \n> > ---\n> > This currently does not work as the ControlInfo constructor for Spans is\n> > broken. This is more of an RFC for Jacopo to check that this is the\n> > direction that he wants.\n> > \n> > Clearly this is not meant to be merged, as the actual capability\n> > requirements are not fully specified yet. Plus they belong in separate\n> > patches anyway.\n> > \n> > This patch does not apply either, as it depends on many unreleased\n> > patches.\n\nThat's quite a long disclaimers list :-)\n\n> > ---\n> > src/android/camera_capabilities.cpp | 219 ++++++++++++++++++++--------\n> > 1 file changed, 157 insertions(+), 62 deletions(-)\n> > \n> > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp\n> > index 83c7f0d0..ceb5cfe8 100644\n> > --- a/src/android/camera_capabilities.cpp\n> > +++ b/src/android/camera_capabilities.cpp\n> > @@ -262,6 +262,149 @@ std::vector setMetadata(CameraMetadata *metadata, uint32_t tag,\n> > \treturn ret;\n> > }\n> > \n> > +\n> > +template\n> > +bool metadataContains(camera_metadata_ro_entry_t &entry, T value);\n> > +\n> > +template<>\n> > +bool metadataContains(camera_metadata_ro_entry_t &entry, uint8_t value)\n> > +{\n> > +\tfor (unsigned int i = 0; i < entry.count; i++)\n> > +\t\tif (entry.data.u8[i] == value)\n> > +\t\t\treturn true;\n> > +\n> > +\treturn false;\n> > +}\n\nThis function could also be moved to the CameraMetadata class (named\nentryContains() or something similar), and include the getEntry() call.\n\n> > +\n> > +bool validateManualSensorCapability(CameraMetadata *staticMetadata)\n> > +{\n> > +\tcamera_metadata_ro_entry_t entry;\n> > +\tbool found;\n> > +\n> > +\tfound = staticMetadata->getEntry(ANDROID_CONTROL_AE_AVAILABLE_MODES,\n> > +\t\t\t\t\t &entry);\n> > +\tif (!found || !metadataContains(entry, ANDROID_CONTROL_AE_MODE_OFF)) {\n> > +\t\tLOG(HAL, Info)\n\nShould this be downgraded to Debug ? An overall Info message that prints\nthe hardware level is good, but maybe detailed messages would be too\nverbose. Or, as we only print a message on the first missing metadata\nentry, would something along the lines of\n\n\t\"Manual sensor capability unvailable: missing AE mode OFF\"\n\nbe good to include as an Info message ?\n\n> > +\t\t\t<< \"Missing AE mode off: \"\n> > +\t\t\t<< (found ? \"not supported\" : \"not found\");\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> > +\t\tLOG(HAL, Info) << \"Missing AE lock\";\n> > +\t\treturn false;\n> > +\t}\n> > +\n> > +\tfound = staticMetadata->getEntry(ANDROID_EDGE_AVAILABLE_EDGE_MODES,\n> > +\t\t\t\t\t &entry);\n\nIf you think it could help making code more readable, the CameraMetadata\nclass could get a hasEntry() function.\n\n> > +\tif (!found) {\n> > +\t\tLOG(HAL, Info) << \"Missing edge modes\";\n> > +\t\treturn false;\n> > +\t}\n> > +\n> > +\treturn true;\n> > +}\n> > +\n> > +bool validateManualPostProcessingCapability(CameraMetadata *staticMetadata)\n> > +{\n> > +\tcamera_metadata_ro_entry_t entry;\n> > +\tbool found;\n> > +\n> > +\tfound = staticMetadata->getEntry(ANDROID_CONTROL_AWB_AVAILABLE_MODES,\n> > +\t\t\t\t\t &entry);\n> > +\tif (!found || !metadataContains(entry, ANDROID_CONTROL_AWB_MODE_OFF)) {\n> > +\t\tLOG(HAL, Info) << \"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) << \"Missing AWB lock\";\n> > +\t\treturn false;\n> > +\t}\n> > +\n> > +\treturn true;\n> > +}\n> > +\n> > +bool validateBurstCaptureCapability(CameraMetadata *staticMetadata)\n> > +{\n> > +\tcamera_metadata_ro_entry_t entry;\n> > +\tbool found;\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) << \"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) << \"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<< \"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::set\n> > +computeCapabilities(CameraMetadata *staticMetadata,\n> > +\t\t std::set &existingCaps)\n> > +{\n> > +\tstd::set\n> > +\tcapabilities = existingCaps;\n> > +\n> > +\tcapabilities.insert(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BACKWARD_COMPATIBLE);\n> > +\n> > +\tif (validateManualSensorCapability(staticMetadata))\n> > +\t\tcapabilities.insert(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR);\n> > +\n> > +\tif (validateManualPostProcessingCapability(staticMetadata))\n> > +\t\tcapabilities.insert(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_POST_PROCESSING);\n> > +\n> > +\tif (validateBurstCaptureCapability(staticMetadata))\n> > +\t\tcapabilities.insert(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE);\n> > +\n> > +\treturn capabilities;\n> > +}\n> > +\n> > +camera_metadata_enum_android_info_supported_hardware_level\n\nIt's impressive how long those names can be.\n\nIt could make sense to store the hardware level in the\nCameraCapabilities class, as I expect we'll need to tune the behaviour\nof the HAL based on it.\n\n> > +computeHwLevel(CameraMetadata *staticMetadata,\n> > +\t std::set capabilities)\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 (!capabilities.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR))\n> > +\t\thwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> > +\n> > +\tif (!capabilities.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_POST_PROCESSING))\n> > +\t\thwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> > +\n> > +\tif (!capabilities.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE))\n> > +\t\thwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\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> > +\treturn hwLevel;\n> > +}\n> > +\n> > } /* namespace */\n> > \n> > int CameraCapabilities::initialize(std::shared_ptr camera,\n> > @@ -655,17 +798,7 @@ int CameraCapabilities::initializeStaticMetadata()\n> > \t};\n> > \n> > \tstd::set\n> > -\tcapabilities = {\n> > -\t\tANDROID_REQUEST_AVAILABLE_CAPABILITIES_BACKWARD_COMPATIBLE,\n> > -\t\tANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR,\n> > -\t\tANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_POST_PROCESSING,\n> > -\t\tANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE,\n> > -\t};\n> > -\n> > -\tstd::set\n> > -\thwLevels = {\n> > -\t\tANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_FULL,\n> > -\t};\n> > +\tcapabilities = {};\n> > \n> > \t/* Color correction static metadata. */\n> > \t{\n> > @@ -692,19 +825,12 @@ int CameraCapabilities::initializeStaticMetadata()\n> > \tstaticMetadata_->addEntry(ANDROID_CONTROL_AE_AVAILABLE_ANTIBANDING_MODES,\n> > \t\t\t\t aeAvailableAntiBandingModes);\n> > \n> > -\tstd::vector aeModes = setMetadata(\n> > +\tsetMetadata(\n> > \t\tstaticMetadata_.get(),\n> > \t\tANDROID_CONTROL_AE_AVAILABLE_MODES,\n> > \t\tcontrolsInfo, &controls::AeEnable,\n> > \t\t{ ANDROID_CONTROL_AE_MODE_ON });\n> > \n> > -\tif (std::find(aeModes.begin(), aeModes.end(),\n> > -\t\t ANDROID_CONTROL_AE_MODE_OFF) == aeModes.end()) {\n> > -\t\tLOG(HAL, Info) << \"AE cannot be turned off\";\n> > -\t\thwLevels.erase(ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_FULL);\n> > -\t\tcapabilities.erase(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR);\n> > -\t}\n> > -\n> > \tint64_t minFrameDurationNsec = -1;\n> > \tint64_t maxFrameDurationNsec = -1;\n> > \tconst auto frameDurationsInfo = controlsInfo.find(&controls::FrameDurationLimits);\n> > @@ -791,17 +917,11 @@ int CameraCapabilities::initializeStaticMetadata()\n> > \tstaticMetadata_->addEntry(ANDROID_CONTROL_AVAILABLE_VIDEO_STABILIZATION_MODES,\n> > \t\t\t\t availableStabilizationModes);\n> > \n> > -\tstd::vector awbModes = setMetadata(\n> > +\tsetMetadata(\n> > \t\tstaticMetadata_.get(),\n> > \t\tANDROID_CONTROL_AWB_AVAILABLE_MODES,\n> > \t\tcontrolsInfo, &controls::AwbMode,\n> > \t\t{ ANDROID_CONTROL_AWB_MODE_AUTO });\n> > -\tif (std::find(awbModes.begin(), awbModes.end(),\n> > -\t\t ANDROID_CONTROL_AWB_MODE_OFF) == awbModes.end()) {\n> > -\t\tLOG(HAL, Info) << \"AWB cannot be turned off\";\n> > -\t\thwLevels.erase(ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_FULL);\n> > -\t\tcapabilities.erase(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_POST_PROCESSING);\n> > -\t}\n> > \n> > \tstd::vector availableMaxRegions = {\n> > \t\t0, 0, 0,\n> > @@ -817,29 +937,19 @@ int CameraCapabilities::initializeStaticMetadata()\n> > \tstaticMetadata_->addEntry(ANDROID_CONTROL_SCENE_MODE_OVERRIDES,\n> > \t\t\t\t sceneModesOverride);\n> > \n> > -\tuint8_t aeLockAvailable = setMetadata(\n> > +\tsetMetadata(\n> > \t\tstaticMetadata_.get(),\n> > \t\tANDROID_CONTROL_AE_LOCK_AVAILABLE,\n> > \t\tcontrolsInfo, &controls::AeLock,\n> > \t\tControlRange::Max,\n> > \t\tANDROID_CONTROL_AE_LOCK_AVAILABLE_FALSE);\n> > -\tif (aeLockAvailable != ANDROID_CONTROL_AE_LOCK_AVAILABLE_TRUE) {\n> > -\t\tLOG(HAL, Info) << \"AE lock is unavailable\";\n> > -\t\tcapabilities.erase(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE);\n> > -\t\tcapabilities.erase(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR);\n> > -\t}\n> > \n> > -\tuint8_t awbLockAvailable = setMetadata(\n> > +\tsetMetadata(\n> > \t\tstaticMetadata_.get(),\n> > \t\tANDROID_CONTROL_AWB_LOCK_AVAILABLE,\n> > \t\tcontrolsInfo, &controls::AwbLock,\n> > \t\tControlRange::Max,\n> > \t\tANDROID_CONTROL_AWB_LOCK_AVAILABLE_FALSE);\n> > -\tif (awbLockAvailable != ANDROID_CONTROL_AWB_LOCK_AVAILABLE_TRUE) {\n> > -\t\tLOG(HAL, Info) << \"AWB lock is unavailable\";\n> > -\t\tcapabilities.erase(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE);\n> > -\t\tcapabilities.erase(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_POST_PROCESSING);\n> > -\t}\n> > \n> > \tchar availableControlModes = ANDROID_CONTROL_MODE_AUTO;\n> > \tstaticMetadata_->addEntry(ANDROID_CONTROL_AVAILABLE_MODES,\n> > @@ -859,10 +969,6 @@ int CameraCapabilities::initializeStaticMetadata()\n> > \t\tavailableCharacteristicsKeys_.insert(ANDROID_EDGE_AVAILABLE_EDGE_MODES);\n> > \t\tavailableRequestKeys_.insert(ANDROID_EDGE_MODE);\n> > \t\tavailableResultKeys_.insert(ANDROID_EDGE_MODE);\n> > -\t} else {\n> > -\t\tLOG(HAL, Info) << \"Edge mode unavailable\";\n> > -\t\thwLevels.erase(ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_FULL);\n> > -\t\tcapabilities.erase(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR);\n> > \t}\n> > \n> > \t/* JPEG static metadata. */\n> > @@ -1046,17 +1152,11 @@ int CameraCapabilities::initializeStaticMetadata()\n> > \t}\n> > \n> > \t/* Sync static metadata. */\n> > -\tint32_t maxLatency = setMetadata(\n> > +\tsetMetadata(\n> > \t\tstaticMetadata_.get(), ANDROID_SYNC_MAX_LATENCY,\n> > \t\tcontrolsInfo, &controls::draft::MaxLatency,\n> > \t\tControlRange::Def,\n> > \t\tANDROID_SYNC_MAX_LATENCY_UNKNOWN);\n> > -\tLOG(HAL, Info) << \"Max sync latency is \" << maxLatency;\n> > -\t/* CTS allows a sync latency of up to 4 for burst capture capability */\n> > -\tif (maxLatency < 0 || 4 < maxLatency)\n> > -\t\tcapabilities.erase(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE);\n> > -\tif (maxLatency != 0)\n> > -\t\thwLevels.erase(ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_FULL);\n> > \n> > \t/* Flash static metadata. */\n> > \tchar flashAvailable = ANDROID_FLASH_INFO_AVAILABLE_FALSE;\n> > @@ -1220,19 +1320,14 @@ int CameraCapabilities::initializeStaticMetadata()\n> > \t\t\t\t numOutStreams);\n> > \n> > \t/* Check capabilities */\n> > -\tstd::vector availableCapabilities(capabilities.begin(),\n> > -\t\t\t\t\t\t capabilities.end());\n> > -\tstaticMetadata_->addEntry(ANDROID_REQUEST_AVAILABLE_CAPABILITIES,\n> > -\t\t\t\t availableCapabilities);\n> > -\n> > -\tuint8_t hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> > -\tif (capabilities.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR) &&\n> > -\t capabilities.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_POST_PROCESSING) &&\n> > -\t capabilities.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE) &&\n> > -\t hwLevels.count(ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_FULL))\n> > -\t\thwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_FULL;\n> > -\tstaticMetadata_->addEntry(ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL,\n> > -\t\t\t\t hwLevel);\n> > +\tcapabilities = computeCapabilities(staticMetadata_.get(), capabilities);\n> \n> Just to clarify, this is here because above this we have:\n> \n> \t/* Report if camera supports RAW. */\n> \tbool rawStreamAvailable = false;\n> \tstd::unique_ptr cameraConfig =\n> \t\tcamera_->generateConfiguration({ StreamRole::Raw });\n> \tif (cameraConfig && !cameraConfig->empty()) {\n> \t\tconst PixelFormatInfo &info =\n> \t\t\tPixelFormatInfo::info(cameraConfig->at(0).pixelFormat);\n> \t\t/* Only advertise RAW support if RAW16 is possible. */\n> \t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW &&\n> \t\t info.bitsPerPixel == 16) {\n> \t\t\trawStreamAvailable = true;\n> \t\t\tcapabilities.insert(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_RAW);\n> \t\t}\n> \t}\n> \n> There's no way to capture this information in the static metadata\n> otherwise, and I don't think it's a good idea to feed arbitrary\n> variables into the capabilities gatherer, since at that point we might\n> as well keep the method that I had earlier (which has the benefit of not\n> having to walk the static metadata again after setting it, but hey, a\n> multiple of linear time is still linear time :D).\n> \n> So yeah, we allow static metadata setters to directly set the\n> capabilities if it alone is sufficient and necessary to determine some\n> capability.\n\nThis looks fine to me. I'd have a slight preference for calling\ncomputeCapabilities() first, and then extending it with\nANDROID_REQUEST_AVAILABLE_CAPABILITIES_RAW (and possibly other\ncapabilities), but if that causes issue, I'm OK with the current order.\n\n> > +\tstd::vector\n> > +\t\tcapsVec = std::vector(capabilities.begin(), capabilities.end());\n> > +\tstaticMetadata_->addEntry(ANDROID_REQUEST_AVAILABLE_CAPABILITIES, capsVec);\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: \"","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 CBD08BD794\n\tfor ;\n\tSun, 11 Jul 2021 20:51:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3979E6851D;\n\tSun, 11 Jul 2021 22:51:32 +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 B4BE268519\n\tfor ;\n\tSun, 11 Jul 2021 22:51:30 +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 1380ACC;\n\tSun, 11 Jul 2021 22:51:30 +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=\"LzEkiAXP\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1626036690;\n\tbh=4GqKK9AQJLJ6cAu897RKECuMkPWhKKquEIEqoooQ1wk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=LzEkiAXP1RScKWpoYFsWeYgJ0pANx3bgg1wuzv7p5Io0DayHBX7rSK4hiVKCuAbta\n\tXHw2CJcbgONTqAmrnhdYTCy+6YJuyNxf4Q1lHq3p7Ge00Kx/VoU2DrEgaISQwynR/b\n\tHE5wo4XfgkWjYU92rcMs3b1nhYkxi4G8KyA6Bcg4=","Date":"Sun, 11 Jul 2021 23:50:44 +0300","From":"Laurent Pinchart ","To":"paul.elder@ideasonboard.com","Message-ID":"","References":"<20210709112814.87917-1-paul.elder@ideasonboard.com>\n\t<20210709113446.GC5937@pyrite.rasen.tech>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210709113446.GC5937@pyrite.rasen.tech>","Subject":"Re: [libcamera-devel] [RFC PATCH] wip: android: capabilities:\n\tCapability detection by population","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":18182,"web_url":"https://patchwork.libcamera.org/comment/18182/","msgid":"<20210715084401.kvoke6d3vcz5xrby@uno.localdomain>","date":"2021-07-15T08:44:01","subject":"Re: [libcamera-devel] [RFC PATCH] wip: android: capabilities:\n\tCapability detection by population","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Paul,\n I'm so sorry for getting that late to this one\n\nOn Sun, Jul 11, 2021 at 11:50:44PM +0300, Laurent Pinchart wrote:\n> Hi Paul,\n>\n> Thank you for the patch.\n>\n> On Fri, Jul 09, 2021 at 08:34:46PM +0900, paul.elder@ideasonboard.com wrote:\n> > On Fri, Jul 09, 2021 at 08:28:14PM +0900, Paul Elder wrote:\n> > > Implement capability and hardware level detection based on the static\n> > > metadata that has been set, instead of disabling them as requirements\n> > > are not met. This results in cleaner code where the static metadata is\n> > > set.\n> > >\n> > > Signed-off-by: Paul Elder \n> > >\n> > > ---\n> > > This currently does not work as the ControlInfo constructor for Spans is\n> > > broken. This is more of an RFC for Jacopo to check that this is the\n> > > direction that he wants.\n\nThanks, this goes exactly in the direction I asked for\n\n> > >\n> > > Clearly this is not meant to be merged, as the actual capability\n> > > requirements are not fully specified yet. Plus they belong in separate\n> > > patches anyway.\n> > >\n> > > This patch does not apply either, as it depends on many unreleased\n> > > patches.\n>\n> That's quite a long disclaimers list :-)\n>\n> > > ---\n> > > src/android/camera_capabilities.cpp | 219 ++++++++++++++++++++--------\n> > > 1 file changed, 157 insertions(+), 62 deletions(-)\n> > >\n> > > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp\n> > > index 83c7f0d0..ceb5cfe8 100644\n> > > --- a/src/android/camera_capabilities.cpp\n> > > +++ b/src/android/camera_capabilities.cpp\n> > > @@ -262,6 +262,149 @@ std::vector setMetadata(CameraMetadata *metadata, uint32_t tag,\n> > > \treturn ret;\n> > > }\n> > >\n> > > +\n> > > +template\n> > > +bool metadataContains(camera_metadata_ro_entry_t &entry, T value);\n> > > +\n> > > +template<>\n> > > +bool metadataContains(camera_metadata_ro_entry_t &entry, uint8_t value)\n> > > +{\n> > > +\tfor (unsigned int i = 0; i < entry.count; i++)\n> > > +\t\tif (entry.data.u8[i] == value)\n> > > +\t\t\treturn true;\n> > > +\n> > > +\treturn false;\n> > > +}\n>\n> This function could also be moved to the CameraMetadata class (named\n> entryContains() or something similar), and include the getEntry() call.\n>\n> > > +\n> > > +bool validateManualSensorCapability(CameraMetadata *staticMetadata)\n> > > +{\n> > > +\tcamera_metadata_ro_entry_t entry;\n> > > +\tbool found;\n> > > +\n> > > +\tfound = staticMetadata->getEntry(ANDROID_CONTROL_AE_AVAILABLE_MODES,\n> > > +\t\t\t\t\t &entry);\n> > > +\tif (!found || !metadataContains(entry, ANDROID_CONTROL_AE_MODE_OFF)) {\n> > > +\t\tLOG(HAL, Info)\n>\n> Should this be downgraded to Debug ? An overall Info message that prints\n> the hardware level is good, but maybe detailed messages would be too\n> verbose. Or, as we only print a message on the first missing metadata\n> entry, would something along the lines of\n>\n> \t\"Manual sensor capability unvailable: missing AE mode OFF\"\n>\n> be good to include as an Info message ?\n>\n> > > +\t\t\t<< \"Missing AE mode off: \"\n> > > +\t\t\t<< (found ? \"not supported\" : \"not found\");\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> > > +\t\tLOG(HAL, Info) << \"Missing AE lock\";\n> > > +\t\treturn false;\n> > > +\t}\n> > > +\n> > > +\tfound = staticMetadata->getEntry(ANDROID_EDGE_AVAILABLE_EDGE_MODES,\n> > > +\t\t\t\t\t &entry);\n>\n> If you think it could help making code more readable, the CameraMetadata\n> class could get a hasEntry() function.\n>\n> > > +\tif (!found) {\n> > > +\t\tLOG(HAL, Info) << \"Missing edge modes\";\n> > > +\t\treturn false;\n> > > +\t}\n> > > +\n> > > +\treturn true;\n> > > +}\n> > > +\n> > > +bool validateManualPostProcessingCapability(CameraMetadata *staticMetadata)\n> > > +{\n> > > +\tcamera_metadata_ro_entry_t entry;\n> > > +\tbool found;\n> > > +\n> > > +\tfound = staticMetadata->getEntry(ANDROID_CONTROL_AWB_AVAILABLE_MODES,\n> > > +\t\t\t\t\t &entry);\n> > > +\tif (!found || !metadataContains(entry, ANDROID_CONTROL_AWB_MODE_OFF)) {\n> > > +\t\tLOG(HAL, Info) << \"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) << \"Missing AWB lock\";\n> > > +\t\treturn false;\n> > > +\t}\n> > > +\n> > > +\treturn true;\n> > > +}\n> > > +\n> > > +bool validateBurstCaptureCapability(CameraMetadata *staticMetadata)\n> > > +{\n> > > +\tcamera_metadata_ro_entry_t entry;\n> > > +\tbool found;\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) << \"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) << \"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<< \"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::set\n> > > +computeCapabilities(CameraMetadata *staticMetadata,\n> > > +\t\t std::set &existingCaps)\n> > > +{\n> > > +\tstd::set\n> > > +\tcapabilities = existingCaps;\n> > > +\n> > > +\tcapabilities.insert(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BACKWARD_COMPATIBLE);\n> > > +\n> > > +\tif (validateManualSensorCapability(staticMetadata))\n> > > +\t\tcapabilities.insert(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR);\n> > > +\n> > > +\tif (validateManualPostProcessingCapability(staticMetadata))\n> > > +\t\tcapabilities.insert(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_POST_PROCESSING);\n> > > +\n> > > +\tif (validateBurstCaptureCapability(staticMetadata))\n> > > +\t\tcapabilities.insert(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE);\n> > > +\n> > > +\treturn capabilities;\n> > > +}\n> > > +\n> > > +camera_metadata_enum_android_info_supported_hardware_level\n>\n> It's impressive how long those names can be.\n>\n> It could make sense to store the hardware level in the\n> CameraCapabilities class, as I expect we'll need to tune the behaviour\n> of the HAL based on it.\n>\n> > > +computeHwLevel(CameraMetadata *staticMetadata,\n> > > +\t std::set capabilities)\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 (!capabilities.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR))\n> > > +\t\thwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> > > +\n> > > +\tif (!capabilities.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_POST_PROCESSING))\n> > > +\t\thwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> > > +\n> > > +\tif (!capabilities.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE))\n> > > +\t\thwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\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> > > +\treturn hwLevel;\n> > > +}\n> > > +\n> > > } /* namespace */\n> > >\n> > > int CameraCapabilities::initialize(std::shared_ptr camera,\n> > > @@ -655,17 +798,7 @@ int CameraCapabilities::initializeStaticMetadata()\n> > > \t};\n> > >\n> > > \tstd::set\n> > > -\tcapabilities = {\n> > > -\t\tANDROID_REQUEST_AVAILABLE_CAPABILITIES_BACKWARD_COMPATIBLE,\n> > > -\t\tANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR,\n> > > -\t\tANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_POST_PROCESSING,\n> > > -\t\tANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE,\n> > > -\t};\n> > > -\n> > > -\tstd::set\n> > > -\thwLevels = {\n> > > -\t\tANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_FULL,\n> > > -\t};\n> > > +\tcapabilities = {};\n> > >\n> > > \t/* Color correction static metadata. */\n> > > \t{\n> > > @@ -692,19 +825,12 @@ int CameraCapabilities::initializeStaticMetadata()\n> > > \tstaticMetadata_->addEntry(ANDROID_CONTROL_AE_AVAILABLE_ANTIBANDING_MODES,\n> > > \t\t\t\t aeAvailableAntiBandingModes);\n> > >\n> > > -\tstd::vector aeModes = setMetadata(\n> > > +\tsetMetadata(\n> > > \t\tstaticMetadata_.get(),\n> > > \t\tANDROID_CONTROL_AE_AVAILABLE_MODES,\n> > > \t\tcontrolsInfo, &controls::AeEnable,\n> > > \t\t{ ANDROID_CONTROL_AE_MODE_ON });\n> > >\n> > > -\tif (std::find(aeModes.begin(), aeModes.end(),\n> > > -\t\t ANDROID_CONTROL_AE_MODE_OFF) == aeModes.end()) {\n> > > -\t\tLOG(HAL, Info) << \"AE cannot be turned off\";\n> > > -\t\thwLevels.erase(ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_FULL);\n> > > -\t\tcapabilities.erase(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR);\n> > > -\t}\n> > > -\n> > > \tint64_t minFrameDurationNsec = -1;\n> > > \tint64_t maxFrameDurationNsec = -1;\n> > > \tconst auto frameDurationsInfo = controlsInfo.find(&controls::FrameDurationLimits);\n> > > @@ -791,17 +917,11 @@ int CameraCapabilities::initializeStaticMetadata()\n> > > \tstaticMetadata_->addEntry(ANDROID_CONTROL_AVAILABLE_VIDEO_STABILIZATION_MODES,\n> > > \t\t\t\t availableStabilizationModes);\n> > >\n> > > -\tstd::vector awbModes = setMetadata(\n> > > +\tsetMetadata(\n> > > \t\tstaticMetadata_.get(),\n> > > \t\tANDROID_CONTROL_AWB_AVAILABLE_MODES,\n> > > \t\tcontrolsInfo, &controls::AwbMode,\n> > > \t\t{ ANDROID_CONTROL_AWB_MODE_AUTO });\n> > > -\tif (std::find(awbModes.begin(), awbModes.end(),\n> > > -\t\t ANDROID_CONTROL_AWB_MODE_OFF) == awbModes.end()) {\n> > > -\t\tLOG(HAL, Info) << \"AWB cannot be turned off\";\n> > > -\t\thwLevels.erase(ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_FULL);\n> > > -\t\tcapabilities.erase(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_POST_PROCESSING);\n> > > -\t}\n> > >\n> > > \tstd::vector availableMaxRegions = {\n> > > \t\t0, 0, 0,\n> > > @@ -817,29 +937,19 @@ int CameraCapabilities::initializeStaticMetadata()\n> > > \tstaticMetadata_->addEntry(ANDROID_CONTROL_SCENE_MODE_OVERRIDES,\n> > > \t\t\t\t sceneModesOverride);\n> > >\n> > > -\tuint8_t aeLockAvailable = setMetadata(\n> > > +\tsetMetadata(\n> > > \t\tstaticMetadata_.get(),\n> > > \t\tANDROID_CONTROL_AE_LOCK_AVAILABLE,\n> > > \t\tcontrolsInfo, &controls::AeLock,\n> > > \t\tControlRange::Max,\n> > > \t\tANDROID_CONTROL_AE_LOCK_AVAILABLE_FALSE);\n> > > -\tif (aeLockAvailable != ANDROID_CONTROL_AE_LOCK_AVAILABLE_TRUE) {\n> > > -\t\tLOG(HAL, Info) << \"AE lock is unavailable\";\n> > > -\t\tcapabilities.erase(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE);\n> > > -\t\tcapabilities.erase(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR);\n> > > -\t}\n> > >\n> > > -\tuint8_t awbLockAvailable = setMetadata(\n> > > +\tsetMetadata(\n> > > \t\tstaticMetadata_.get(),\n> > > \t\tANDROID_CONTROL_AWB_LOCK_AVAILABLE,\n> > > \t\tcontrolsInfo, &controls::AwbLock,\n> > > \t\tControlRange::Max,\n> > > \t\tANDROID_CONTROL_AWB_LOCK_AVAILABLE_FALSE);\n> > > -\tif (awbLockAvailable != ANDROID_CONTROL_AWB_LOCK_AVAILABLE_TRUE) {\n> > > -\t\tLOG(HAL, Info) << \"AWB lock is unavailable\";\n> > > -\t\tcapabilities.erase(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE);\n> > > -\t\tcapabilities.erase(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_POST_PROCESSING);\n> > > -\t}\n> > >\n> > > \tchar availableControlModes = ANDROID_CONTROL_MODE_AUTO;\n> > > \tstaticMetadata_->addEntry(ANDROID_CONTROL_AVAILABLE_MODES,\n> > > @@ -859,10 +969,6 @@ int CameraCapabilities::initializeStaticMetadata()\n> > > \t\tavailableCharacteristicsKeys_.insert(ANDROID_EDGE_AVAILABLE_EDGE_MODES);\n> > > \t\tavailableRequestKeys_.insert(ANDROID_EDGE_MODE);\n> > > \t\tavailableResultKeys_.insert(ANDROID_EDGE_MODE);\n> > > -\t} else {\n> > > -\t\tLOG(HAL, Info) << \"Edge mode unavailable\";\n> > > -\t\thwLevels.erase(ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_FULL);\n> > > -\t\tcapabilities.erase(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR);\n> > > \t}\n> > >\n> > > \t/* JPEG static metadata. */\n> > > @@ -1046,17 +1152,11 @@ int CameraCapabilities::initializeStaticMetadata()\n> > > \t}\n> > >\n> > > \t/* Sync static metadata. */\n> > > -\tint32_t maxLatency = setMetadata(\n> > > +\tsetMetadata(\n> > > \t\tstaticMetadata_.get(), ANDROID_SYNC_MAX_LATENCY,\n> > > \t\tcontrolsInfo, &controls::draft::MaxLatency,\n> > > \t\tControlRange::Def,\n> > > \t\tANDROID_SYNC_MAX_LATENCY_UNKNOWN);\n> > > -\tLOG(HAL, Info) << \"Max sync latency is \" << maxLatency;\n> > > -\t/* CTS allows a sync latency of up to 4 for burst capture capability */\n> > > -\tif (maxLatency < 0 || 4 < maxLatency)\n> > > -\t\tcapabilities.erase(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE);\n> > > -\tif (maxLatency != 0)\n> > > -\t\thwLevels.erase(ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_FULL);\n> > >\n> > > \t/* Flash static metadata. */\n> > > \tchar flashAvailable = ANDROID_FLASH_INFO_AVAILABLE_FALSE;\n> > > @@ -1220,19 +1320,14 @@ int CameraCapabilities::initializeStaticMetadata()\n> > > \t\t\t\t numOutStreams);\n> > >\n> > > \t/* Check capabilities */\n> > > -\tstd::vector availableCapabilities(capabilities.begin(),\n> > > -\t\t\t\t\t\t capabilities.end());\n> > > -\tstaticMetadata_->addEntry(ANDROID_REQUEST_AVAILABLE_CAPABILITIES,\n> > > -\t\t\t\t availableCapabilities);\n> > > -\n> > > -\tuint8_t hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> > > -\tif (capabilities.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR) &&\n> > > -\t capabilities.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_POST_PROCESSING) &&\n> > > -\t capabilities.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE) &&\n> > > -\t hwLevels.count(ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_FULL))\n> > > -\t\thwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_FULL;\n> > > -\tstaticMetadata_->addEntry(ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL,\n> > > -\t\t\t\t hwLevel);\n> > > +\tcapabilities = computeCapabilities(staticMetadata_.get(), capabilities);\n\nSo far I share all comments Laurent made, there's space for\nsimplifying the implementation, but the direction is good!\n\n> >\n> > Just to clarify, this is here because above this we have:\n> >\n> > \t/* Report if camera supports RAW. */\n> > \tbool rawStreamAvailable = false;\n> > \tstd::unique_ptr cameraConfig =\n> > \t\tcamera_->generateConfiguration({ StreamRole::Raw });\n> > \tif (cameraConfig && !cameraConfig->empty()) {\n> > \t\tconst PixelFormatInfo &info =\n> > \t\t\tPixelFormatInfo::info(cameraConfig->at(0).pixelFormat);\n> > \t\t/* Only advertise RAW support if RAW16 is possible. */\n> > \t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW &&\n> > \t\t info.bitsPerPixel == 16) {\n> > \t\t\trawStreamAvailable = true;\n> > \t\t\tcapabilities.insert(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_RAW);\n> > \t\t}\n> > \t}\n> >\n> > There's no way to capture this information in the static metadata\n> > otherwise, and I don't think it's a good idea to feed arbitrary\n> > variables into the capabilities gatherer, since at that point we might\n> > as well keep the method that I had earlier (which has the benefit of not\n> > having to walk the static metadata again after setting it, but hey, a\n> > multiple of linear time is still linear time :D).\n> >\n> > So yeah, we allow static metadata setters to directly set the\n> > capabilities if it alone is sufficient and necessary to determine some\n> > capability.\n>\n> This looks fine to me. I'd have a slight preference for calling\n> computeCapabilities() first, and then extending it with\n> ANDROID_REQUEST_AVAILABLE_CAPABILITIES_RAW (and possibly other\n> capabilities), but if that causes issue, I'm OK with the current order.\n>\n\nI have a patch in my backlog that moves RAW capabilities detection to\ninitializetreamConfigurations() as the check for RAW support was\nduplicated. I recorded the RAW support information in a class\nvariable, and initializeStaticMetadata() inspects that to populate the\ncapabilities.\n\nPatch has not been sent as it was part of the frameduration work, but\nI can send it out with a few related cleanups. In the meantime\nhttps://paste.debian.net/1204421/\n\nDo you think it would be ok for the capabilities checker to inspect a\nclass variable ?\n\nThanks\n j\n\n> > > +\tstd::vector\n> > > +\t\tcapsVec = std::vector(capabilities.begin(), capabilities.end());\n> > > +\tstaticMetadata_->addEntry(ANDROID_REQUEST_AVAILABLE_CAPABILITIES, capsVec);\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>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 2A7CAC3225\n\tfor ;\n\tThu, 15 Jul 2021 08:43:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3CF2668528;\n\tThu, 15 Jul 2021 10:43:16 +0200 (CEST)","from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[217.70.183.195])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C9F7360281\n\tfor ;\n\tThu, 15 Jul 2021 10:43:14 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay3-d.mail.gandi.net (Postfix) with ESMTPSA id 2AF5860002;\n\tThu, 15 Jul 2021 08:43:12 +0000 (UTC)"],"Date":"Thu, 15 Jul 2021 10:44:01 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210715084401.kvoke6d3vcz5xrby@uno.localdomain>","References":"<20210709112814.87917-1-paul.elder@ideasonboard.com>\n\t<20210709113446.GC5937@pyrite.rasen.tech>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [RFC PATCH] wip: android: capabilities:\n\tCapability detection by population","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":18183,"web_url":"https://patchwork.libcamera.org/comment/18183/","msgid":"<20210715084641.GD2395@pyrite.rasen.tech>","date":"2021-07-15T08:46:41","subject":"Re: [libcamera-devel] [RFC PATCH] wip: android: capabilities:\n\tCapability detection by population","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 15, 2021 at 10:44:01AM +0200, Jacopo Mondi wrote:\n> Hi Paul,\n> I'm so sorry for getting that late to this one\n> \n> On Sun, Jul 11, 2021 at 11:50:44PM +0300, Laurent Pinchart wrote:\n> > Hi Paul,\n> >\n> > Thank you for the patch.\n> >\n> > On Fri, Jul 09, 2021 at 08:34:46PM +0900, paul.elder@ideasonboard.com wrote:\n> > > On Fri, Jul 09, 2021 at 08:28:14PM +0900, Paul Elder wrote:\n> > > > Implement capability and hardware level detection based on the static\n> > > > metadata that has been set, instead of disabling them as requirements\n> > > > are not met. This results in cleaner code where the static metadata is\n> > > > set.\n> > > >\n> > > > Signed-off-by: Paul Elder \n> > > >\n> > > > ---\n> > > > This currently does not work as the ControlInfo constructor for Spans is\n> > > > broken. This is more of an RFC for Jacopo to check that this is the\n> > > > direction that he wants.\n> \n> Thanks, this goes exactly in the direction I asked for\n\nPhew, that's good. I continue in this direction then.\n\n> \n> > > >\n> > > > Clearly this is not meant to be merged, as the actual capability\n> > > > requirements are not fully specified yet. Plus they belong in separate\n> > > > patches anyway.\n> > > >\n> > > > This patch does not apply either, as it depends on many unreleased\n> > > > patches.\n> >\n> > That's quite a long disclaimers list :-)\n> >\n> > > > ---\n> > > > src/android/camera_capabilities.cpp | 219 ++++++++++++++++++++--------\n> > > > 1 file changed, 157 insertions(+), 62 deletions(-)\n> > > >\n> > > > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp\n> > > > index 83c7f0d0..ceb5cfe8 100644\n> > > > --- a/src/android/camera_capabilities.cpp\n> > > > +++ b/src/android/camera_capabilities.cpp\n> > > > @@ -262,6 +262,149 @@ std::vector setMetadata(CameraMetadata *metadata, uint32_t tag,\n> > > > \treturn ret;\n> > > > }\n> > > >\n> > > > +\n> > > > +template\n> > > > +bool metadataContains(camera_metadata_ro_entry_t &entry, T value);\n> > > > +\n> > > > +template<>\n> > > > +bool metadataContains(camera_metadata_ro_entry_t &entry, uint8_t value)\n> > > > +{\n> > > > +\tfor (unsigned int i = 0; i < entry.count; i++)\n> > > > +\t\tif (entry.data.u8[i] == value)\n> > > > +\t\t\treturn true;\n> > > > +\n> > > > +\treturn false;\n> > > > +}\n> >\n> > This function could also be moved to the CameraMetadata class (named\n> > entryContains() or something similar), and include the getEntry() call.\n> >\n> > > > +\n> > > > +bool validateManualSensorCapability(CameraMetadata *staticMetadata)\n> > > > +{\n> > > > +\tcamera_metadata_ro_entry_t entry;\n> > > > +\tbool found;\n> > > > +\n> > > > +\tfound = staticMetadata->getEntry(ANDROID_CONTROL_AE_AVAILABLE_MODES,\n> > > > +\t\t\t\t\t &entry);\n> > > > +\tif (!found || !metadataContains(entry, ANDROID_CONTROL_AE_MODE_OFF)) {\n> > > > +\t\tLOG(HAL, Info)\n> >\n> > Should this be downgraded to Debug ? An overall Info message that prints\n> > the hardware level is good, but maybe detailed messages would be too\n> > verbose. Or, as we only print a message on the first missing metadata\n> > entry, would something along the lines of\n> >\n> > \t\"Manual sensor capability unvailable: missing AE mode OFF\"\n> >\n> > be good to include as an Info message ?\n> >\n> > > > +\t\t\t<< \"Missing AE mode off: \"\n> > > > +\t\t\t<< (found ? \"not supported\" : \"not found\");\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> > > > +\t\tLOG(HAL, Info) << \"Missing AE lock\";\n> > > > +\t\treturn false;\n> > > > +\t}\n> > > > +\n> > > > +\tfound = staticMetadata->getEntry(ANDROID_EDGE_AVAILABLE_EDGE_MODES,\n> > > > +\t\t\t\t\t &entry);\n> >\n> > If you think it could help making code more readable, the CameraMetadata\n> > class could get a hasEntry() function.\n> >\n> > > > +\tif (!found) {\n> > > > +\t\tLOG(HAL, Info) << \"Missing edge modes\";\n> > > > +\t\treturn false;\n> > > > +\t}\n> > > > +\n> > > > +\treturn true;\n> > > > +}\n> > > > +\n> > > > +bool validateManualPostProcessingCapability(CameraMetadata *staticMetadata)\n> > > > +{\n> > > > +\tcamera_metadata_ro_entry_t entry;\n> > > > +\tbool found;\n> > > > +\n> > > > +\tfound = staticMetadata->getEntry(ANDROID_CONTROL_AWB_AVAILABLE_MODES,\n> > > > +\t\t\t\t\t &entry);\n> > > > +\tif (!found || !metadataContains(entry, ANDROID_CONTROL_AWB_MODE_OFF)) {\n> > > > +\t\tLOG(HAL, Info) << \"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) << \"Missing AWB lock\";\n> > > > +\t\treturn false;\n> > > > +\t}\n> > > > +\n> > > > +\treturn true;\n> > > > +}\n> > > > +\n> > > > +bool validateBurstCaptureCapability(CameraMetadata *staticMetadata)\n> > > > +{\n> > > > +\tcamera_metadata_ro_entry_t entry;\n> > > > +\tbool found;\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) << \"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) << \"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<< \"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::set\n> > > > +computeCapabilities(CameraMetadata *staticMetadata,\n> > > > +\t\t std::set &existingCaps)\n> > > > +{\n> > > > +\tstd::set\n> > > > +\tcapabilities = existingCaps;\n> > > > +\n> > > > +\tcapabilities.insert(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BACKWARD_COMPATIBLE);\n> > > > +\n> > > > +\tif (validateManualSensorCapability(staticMetadata))\n> > > > +\t\tcapabilities.insert(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR);\n> > > > +\n> > > > +\tif (validateManualPostProcessingCapability(staticMetadata))\n> > > > +\t\tcapabilities.insert(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_POST_PROCESSING);\n> > > > +\n> > > > +\tif (validateBurstCaptureCapability(staticMetadata))\n> > > > +\t\tcapabilities.insert(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE);\n> > > > +\n> > > > +\treturn capabilities;\n> > > > +}\n> > > > +\n> > > > +camera_metadata_enum_android_info_supported_hardware_level\n> >\n> > It's impressive how long those names can be.\n> >\n> > It could make sense to store the hardware level in the\n> > CameraCapabilities class, as I expect we'll need to tune the behaviour\n> > of the HAL based on it.\n> >\n> > > > +computeHwLevel(CameraMetadata *staticMetadata,\n> > > > +\t std::set capabilities)\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 (!capabilities.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR))\n> > > > +\t\thwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> > > > +\n> > > > +\tif (!capabilities.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_POST_PROCESSING))\n> > > > +\t\thwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> > > > +\n> > > > +\tif (!capabilities.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE))\n> > > > +\t\thwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\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> > > > +\treturn hwLevel;\n> > > > +}\n> > > > +\n> > > > } /* namespace */\n> > > >\n> > > > int CameraCapabilities::initialize(std::shared_ptr camera,\n> > > > @@ -655,17 +798,7 @@ int CameraCapabilities::initializeStaticMetadata()\n> > > > \t};\n> > > >\n> > > > \tstd::set\n> > > > -\tcapabilities = {\n> > > > -\t\tANDROID_REQUEST_AVAILABLE_CAPABILITIES_BACKWARD_COMPATIBLE,\n> > > > -\t\tANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR,\n> > > > -\t\tANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_POST_PROCESSING,\n> > > > -\t\tANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE,\n> > > > -\t};\n> > > > -\n> > > > -\tstd::set\n> > > > -\thwLevels = {\n> > > > -\t\tANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_FULL,\n> > > > -\t};\n> > > > +\tcapabilities = {};\n> > > >\n> > > > \t/* Color correction static metadata. */\n> > > > \t{\n> > > > @@ -692,19 +825,12 @@ int CameraCapabilities::initializeStaticMetadata()\n> > > > \tstaticMetadata_->addEntry(ANDROID_CONTROL_AE_AVAILABLE_ANTIBANDING_MODES,\n> > > > \t\t\t\t aeAvailableAntiBandingModes);\n> > > >\n> > > > -\tstd::vector aeModes = setMetadata(\n> > > > +\tsetMetadata(\n> > > > \t\tstaticMetadata_.get(),\n> > > > \t\tANDROID_CONTROL_AE_AVAILABLE_MODES,\n> > > > \t\tcontrolsInfo, &controls::AeEnable,\n> > > > \t\t{ ANDROID_CONTROL_AE_MODE_ON });\n> > > >\n> > > > -\tif (std::find(aeModes.begin(), aeModes.end(),\n> > > > -\t\t ANDROID_CONTROL_AE_MODE_OFF) == aeModes.end()) {\n> > > > -\t\tLOG(HAL, Info) << \"AE cannot be turned off\";\n> > > > -\t\thwLevels.erase(ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_FULL);\n> > > > -\t\tcapabilities.erase(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR);\n> > > > -\t}\n> > > > -\n> > > > \tint64_t minFrameDurationNsec = -1;\n> > > > \tint64_t maxFrameDurationNsec = -1;\n> > > > \tconst auto frameDurationsInfo = controlsInfo.find(&controls::FrameDurationLimits);\n> > > > @@ -791,17 +917,11 @@ int CameraCapabilities::initializeStaticMetadata()\n> > > > \tstaticMetadata_->addEntry(ANDROID_CONTROL_AVAILABLE_VIDEO_STABILIZATION_MODES,\n> > > > \t\t\t\t availableStabilizationModes);\n> > > >\n> > > > -\tstd::vector awbModes = setMetadata(\n> > > > +\tsetMetadata(\n> > > > \t\tstaticMetadata_.get(),\n> > > > \t\tANDROID_CONTROL_AWB_AVAILABLE_MODES,\n> > > > \t\tcontrolsInfo, &controls::AwbMode,\n> > > > \t\t{ ANDROID_CONTROL_AWB_MODE_AUTO });\n> > > > -\tif (std::find(awbModes.begin(), awbModes.end(),\n> > > > -\t\t ANDROID_CONTROL_AWB_MODE_OFF) == awbModes.end()) {\n> > > > -\t\tLOG(HAL, Info) << \"AWB cannot be turned off\";\n> > > > -\t\thwLevels.erase(ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_FULL);\n> > > > -\t\tcapabilities.erase(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_POST_PROCESSING);\n> > > > -\t}\n> > > >\n> > > > \tstd::vector availableMaxRegions = {\n> > > > \t\t0, 0, 0,\n> > > > @@ -817,29 +937,19 @@ int CameraCapabilities::initializeStaticMetadata()\n> > > > \tstaticMetadata_->addEntry(ANDROID_CONTROL_SCENE_MODE_OVERRIDES,\n> > > > \t\t\t\t sceneModesOverride);\n> > > >\n> > > > -\tuint8_t aeLockAvailable = setMetadata(\n> > > > +\tsetMetadata(\n> > > > \t\tstaticMetadata_.get(),\n> > > > \t\tANDROID_CONTROL_AE_LOCK_AVAILABLE,\n> > > > \t\tcontrolsInfo, &controls::AeLock,\n> > > > \t\tControlRange::Max,\n> > > > \t\tANDROID_CONTROL_AE_LOCK_AVAILABLE_FALSE);\n> > > > -\tif (aeLockAvailable != ANDROID_CONTROL_AE_LOCK_AVAILABLE_TRUE) {\n> > > > -\t\tLOG(HAL, Info) << \"AE lock is unavailable\";\n> > > > -\t\tcapabilities.erase(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE);\n> > > > -\t\tcapabilities.erase(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR);\n> > > > -\t}\n> > > >\n> > > > -\tuint8_t awbLockAvailable = setMetadata(\n> > > > +\tsetMetadata(\n> > > > \t\tstaticMetadata_.get(),\n> > > > \t\tANDROID_CONTROL_AWB_LOCK_AVAILABLE,\n> > > > \t\tcontrolsInfo, &controls::AwbLock,\n> > > > \t\tControlRange::Max,\n> > > > \t\tANDROID_CONTROL_AWB_LOCK_AVAILABLE_FALSE);\n> > > > -\tif (awbLockAvailable != ANDROID_CONTROL_AWB_LOCK_AVAILABLE_TRUE) {\n> > > > -\t\tLOG(HAL, Info) << \"AWB lock is unavailable\";\n> > > > -\t\tcapabilities.erase(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE);\n> > > > -\t\tcapabilities.erase(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_POST_PROCESSING);\n> > > > -\t}\n> > > >\n> > > > \tchar availableControlModes = ANDROID_CONTROL_MODE_AUTO;\n> > > > \tstaticMetadata_->addEntry(ANDROID_CONTROL_AVAILABLE_MODES,\n> > > > @@ -859,10 +969,6 @@ int CameraCapabilities::initializeStaticMetadata()\n> > > > \t\tavailableCharacteristicsKeys_.insert(ANDROID_EDGE_AVAILABLE_EDGE_MODES);\n> > > > \t\tavailableRequestKeys_.insert(ANDROID_EDGE_MODE);\n> > > > \t\tavailableResultKeys_.insert(ANDROID_EDGE_MODE);\n> > > > -\t} else {\n> > > > -\t\tLOG(HAL, Info) << \"Edge mode unavailable\";\n> > > > -\t\thwLevels.erase(ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_FULL);\n> > > > -\t\tcapabilities.erase(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR);\n> > > > \t}\n> > > >\n> > > > \t/* JPEG static metadata. */\n> > > > @@ -1046,17 +1152,11 @@ int CameraCapabilities::initializeStaticMetadata()\n> > > > \t}\n> > > >\n> > > > \t/* Sync static metadata. */\n> > > > -\tint32_t maxLatency = setMetadata(\n> > > > +\tsetMetadata(\n> > > > \t\tstaticMetadata_.get(), ANDROID_SYNC_MAX_LATENCY,\n> > > > \t\tcontrolsInfo, &controls::draft::MaxLatency,\n> > > > \t\tControlRange::Def,\n> > > > \t\tANDROID_SYNC_MAX_LATENCY_UNKNOWN);\n> > > > -\tLOG(HAL, Info) << \"Max sync latency is \" << maxLatency;\n> > > > -\t/* CTS allows a sync latency of up to 4 for burst capture capability */\n> > > > -\tif (maxLatency < 0 || 4 < maxLatency)\n> > > > -\t\tcapabilities.erase(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE);\n> > > > -\tif (maxLatency != 0)\n> > > > -\t\thwLevels.erase(ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_FULL);\n> > > >\n> > > > \t/* Flash static metadata. */\n> > > > \tchar flashAvailable = ANDROID_FLASH_INFO_AVAILABLE_FALSE;\n> > > > @@ -1220,19 +1320,14 @@ int CameraCapabilities::initializeStaticMetadata()\n> > > > \t\t\t\t numOutStreams);\n> > > >\n> > > > \t/* Check capabilities */\n> > > > -\tstd::vector availableCapabilities(capabilities.begin(),\n> > > > -\t\t\t\t\t\t capabilities.end());\n> > > > -\tstaticMetadata_->addEntry(ANDROID_REQUEST_AVAILABLE_CAPABILITIES,\n> > > > -\t\t\t\t availableCapabilities);\n> > > > -\n> > > > -\tuint8_t hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> > > > -\tif (capabilities.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR) &&\n> > > > -\t capabilities.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_POST_PROCESSING) &&\n> > > > -\t capabilities.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE) &&\n> > > > -\t hwLevels.count(ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_FULL))\n> > > > -\t\thwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_FULL;\n> > > > -\tstaticMetadata_->addEntry(ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL,\n> > > > -\t\t\t\t hwLevel);\n> > > > +\tcapabilities = computeCapabilities(staticMetadata_.get(), capabilities);\n> \n> So far I share all comments Laurent made, there's space for\n> simplifying the implementation, but the direction is good!\n> \n> > >\n> > > Just to clarify, this is here because above this we have:\n> > >\n> > > \t/* Report if camera supports RAW. */\n> > > \tbool rawStreamAvailable = false;\n> > > \tstd::unique_ptr cameraConfig =\n> > > \t\tcamera_->generateConfiguration({ StreamRole::Raw });\n> > > \tif (cameraConfig && !cameraConfig->empty()) {\n> > > \t\tconst PixelFormatInfo &info =\n> > > \t\t\tPixelFormatInfo::info(cameraConfig->at(0).pixelFormat);\n> > > \t\t/* Only advertise RAW support if RAW16 is possible. */\n> > > \t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW &&\n> > > \t\t info.bitsPerPixel == 16) {\n> > > \t\t\trawStreamAvailable = true;\n> > > \t\t\tcapabilities.insert(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_RAW);\n> > > \t\t}\n> > > \t}\n> > >\n> > > There's no way to capture this information in the static metadata\n> > > otherwise, and I don't think it's a good idea to feed arbitrary\n> > > variables into the capabilities gatherer, since at that point we might\n> > > as well keep the method that I had earlier (which has the benefit of not\n> > > having to walk the static metadata again after setting it, but hey, a\n> > > multiple of linear time is still linear time :D).\n> > >\n> > > So yeah, we allow static metadata setters to directly set the\n> > > capabilities if it alone is sufficient and necessary to determine some\n> > > capability.\n> >\n> > This looks fine to me. I'd have a slight preference for calling\n> > computeCapabilities() first, and then extending it with\n> > ANDROID_REQUEST_AVAILABLE_CAPABILITIES_RAW (and possibly other\n> > capabilities), but if that causes issue, I'm OK with the current order.\n> >\n> \n> I have a patch in my backlog that moves RAW capabilities detection to\n> initializetreamConfigurations() as the check for RAW support was\n> duplicated. I recorded the RAW support information in a class\n> variable, and initializeStaticMetadata() inspects that to populate the\n> capabilities.\n> \n> Patch has not been sent as it was part of the frameduration work, but\n> I can send it out with a few related cleanups. In the meantime\n> https://paste.debian.net/1204421/\n> \n> Do you think it would be ok for the capabilities checker to inspect a\n> class variable ?\n\nOh yeah I think that would be better.\n\n\nThanks,\n\nPaul\n\n> \n> > > > +\tstd::vector\n> > > > +\t\tcapsVec = std::vector(capabilities.begin(), capabilities.end());\n> > > > +\tstaticMetadata_->addEntry(ANDROID_REQUEST_AVAILABLE_CAPABILITIES, capsVec);\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> >\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart","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 F2C5AC3226\n\tfor ;\n\tThu, 15 Jul 2021 08:46:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5D35360281;\n\tThu, 15 Jul 2021 10:46:52 +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 3489F60281\n\tfor ;\n\tThu, 15 Jul 2021 10:46:50 +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 3B04B340;\n\tThu, 15 Jul 2021 10:46:47 +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=\"GPNVl3MY\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1626338809;\n\tbh=RR3TIi8Ud+uyHXWS7uD3jE7MPxNM4jICC9ZJRLXphaA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=GPNVl3MYaAA56lDcMutfNsmWDE98nUSFk7fKKsW3F55P0sP+8CBPN4zkcSpRmmu97\n\t1pDLNIhMLeS2W0fEFl4udiB3A7ipumIw4s6A0ZdliLGsIhhKIWAdU+rqH0h2rRBup1\n\thPQWz7ZshzI6FKOTDPhIElcx4ZJftkJSCqi5kcj4=","Date":"Thu, 15 Jul 2021 17:46:41 +0900","From":"paul.elder@ideasonboard.com","To":"Jacopo Mondi ","Message-ID":"<20210715084641.GD2395@pyrite.rasen.tech>","References":"<20210709112814.87917-1-paul.elder@ideasonboard.com>\n\t<20210709113446.GC5937@pyrite.rasen.tech>\n\t\n\t<20210715084401.kvoke6d3vcz5xrby@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20210715084401.kvoke6d3vcz5xrby@uno.localdomain>","Subject":"Re: [libcamera-devel] [RFC PATCH] wip: android: capabilities:\n\tCapability detection by population","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\" "}}]