[{"id":17987,"web_url":"https://patchwork.libcamera.org/comment/17987/","msgid":"<20210705135221.32qhmuqu4jtde5ph@uno.localdomain>","date":"2021-07-05T13:52:21","subject":"Re: [libcamera-devel] [RFC PATCH v4 02/16] android: Add\n\tinfrastructure for 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 Fri, Jul 02, 2021 at 07:37:46PM +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> Since the raw capability is set to true when support is determined to be\n> available, leave that as default false and set to true, since copying\n> the pattern of the other capabilities would cause redundant code.\n>\n> Note that this will cause CTS to fail, as we don't yet declare the lack\n> of support for FULL based on the available controls.\n>\n> Bug: https://bugs.libcamera.org/show_bug.cgi?id=55\n> Signed-off-by: Paul Elder \n>\n\nShouldn't it be the other way around ?\n\nWe start with an empty set of capabilities and validates them one by\none.\n\nThe set of requirements is not trivial and I'm not sure we can express\nits negation easily. What I mean is that, copying the examples in my\nreply to \"[PATCH v2] android: Add infrastructure for determining\ncapabilities and hardware level\":\n\nFor MANUAL_SENSOR capability:\n - android.control.aeMode contains OFF\n - android.control.awbMode contains OFF\n - android.control.afMode contains OFF\n - ANDROID_REQUEST_AVAILABLE_RESULT_KEYS contains android.sensor.frameDuration\n - ANDROID_REQUEST_AVAILABLE_CHARACTERISTICS_KEYS contains\n android.sensor.info.maxFrameDuration\n - ...\n\nWe can operates in two ways:\n- Start with the capability enabled and verify for each required\n feature if it is present or not while populating the static metadata\n by duplicating a lot of:\n\n if (!condition)\n capabilities.erase(MANUAL_SENSOR)\n\n Causing the code that verifies if a capabilitiy is supported to be\n spread in many places and difficult to verify against the\n specification\n\n- Start with an empty set of capabilities. After having populated the\n static metadata as we currently do introduce an\n initializeCapabilties() function that takes the static metadata pack\n and verifies the requirements for each capabilities are supported:\n\n in PseudoCode:\n\n bool validateManualSensorCapability(staticMetadata) {\n if (!staticMetadata.get(ANDROID_CONTROL_AE_MODE).contains(OFF)\n return false;\n\n if ....\n\n return true;\n }\n\n void initializeCapabilities() {\n if (validateManualSensorCapability(staticMetadata))\n capabilities.push_back(MANUAL_SENSOR);\n\n ...\n }\n\nI would go for the second option, and after having plumbed most of the\nrequirements for FULL in.\n\nThanks\n j\n\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> This is my vision of how we would support the various capabilities.\n> Although we don't have anything for FULL yet; I imagine that we would\n> start the flags for manual sensor and manual post processing with true,\n> and then if a required control is unavailable, then we would set the\n> flag to false.\n>\n> I considered declaring an enum in CameraDevice to mirror the android\n> ones, just for shorthand, but it seemed like a lot of code for not much\n> gain. Unless the shorthand would be valuable because these constant\n> names are so long?\n>\n> I think the available keys lists will have to be moved to the head of\n> the function, and then as available controls are discovered add them to\n> that list.\n> ---\n> src/android/camera_capabilities.cpp | 50 +++++++++++++++++++++++------\n> 1 file changed, 40 insertions(+), 10 deletions(-)\n>\n> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp\n> index 6b5edb66..54bd71da 100644\n> --- a/src/android/camera_capabilities.cpp\n> +++ b/src/android/camera_capabilities.cpp\n> @@ -9,6 +9,7 @@\n>\n> #include \n> #include \n> +#include \n>\n> #include \n>\n> @@ -114,6 +115,15 @@ 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> int CameraCapabilities::initialize(std::shared_ptr camera,\n> @@ -376,6 +386,19 @@ int CameraCapabilities::initializeStaticMetadata()\n> \tconst ControlInfoMap &controlsInfo = camera_->controls();\n> \tconst ControlList &properties = camera_->properties();\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> +\n> \t/* Color correction static metadata. */\n> \t{\n> \t\tstd::vector data;\n> @@ -834,11 +857,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> @@ -859,10 +877,6 @@ 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> \tbool rawStreamAvailable = false;\n> \tstd::unique_ptr cameraConfig =\n> @@ -874,7 +888,7 @@ int CameraCapabilities::initializeStaticMetadata()\n> \t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW &&\n> \t\t info.bitsPerPixel == 16) {\n> \t\t\trawStreamAvailable = true;\n> -\t\t\tavailableCapabilities.push_back(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_RAW);\n> +\t\t\tcapabilities.insert(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_RAW);\n> \t\t}\n> \t}\n>\n> @@ -883,9 +897,25 @@ int CameraCapabilities::initializeStaticMetadata()\n> \tstaticMetadata_->addEntry(ANDROID_REQUEST_MAX_NUM_OUTPUT_STREAMS,\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> +\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> \t\tANDROID_CONTROL_AE_AVAILABLE_ANTIBANDING_MODES,\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 D50AEBD794\n\tfor ;\n\tMon, 5 Jul 2021 13:51:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 62138684F9;\n\tMon, 5 Jul 2021 15:51:33 +0200 (CEST)","from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[217.70.183.199])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7274D6050A\n\tfor ;\n\tMon, 5 Jul 2021 15:51:32 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay9-d.mail.gandi.net (Postfix) with ESMTPSA id E7A8AFF80D;\n\tMon, 5 Jul 2021 13:51:31 +0000 (UTC)"],"Date":"Mon, 5 Jul 2021 15:52:21 +0200","From":"Jacopo Mondi ","To":"Paul Elder ","Message-ID":"<20210705135221.32qhmuqu4jtde5ph@uno.localdomain>","References":"<20210702103800.41291-1-paul.elder@ideasonboard.com>\n\t<20210702103800.41291-3-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210702103800.41291-3-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC PATCH v4 02/16] android: Add\n\tinfrastructure for 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\" "}}]